Bug fixing OIDCPolicy

[didierofrivia]

Closes #2017

Verification Steps

0. Dev cluster setup with some useful envs

export GATEWAY_NS=test-namespace      # Namespace for the Gateway
export GATEWAY_NAME=test-gateway # Name for the Gateway
kubectl create namespace $GATEWAY_NS
make local-setup

1. Deploy Kuadrant Instance

kubectl apply -f - <<EOF
apiVersion: kuadrant.io/v1beta1
kind: Kuadrant
metadata:
  name: kuadrant
  namespace: kuadrant-system
EOF

2. Install OIDCPolicy and required Gateway and HTTPRoute for verification

kubectl apply -f - <<EOF
apiVersion: gateway.networking.k8s.io/v1
kind: Gateway
metadata:
  name: ${GATEWAY_NAME}
  namespace: ${GATEWAY_NS}
spec:
  gatewayClassName: istio
  listeners:
  - name: http
    hostname: "example.com"
    port: 8001  # Non-standard port
    protocol: HTTP
---
apiVersion: gateway.networking.k8s.io/v1
kind: HTTPRoute
metadata:
  name: test-route
  namespace: ${GATEWAY_NS}
spec:
  parentRefs:
  - name: ${GATEWAY_NAME}
  hostnames:
  - "example.com"
  rules:
  - matches:
    - path:
        type: PathPrefix
        value: /
    backendRefs:
    - name: test-app
      port: 8080
---
apiVersion: extensions.kuadrant.io/v1alpha1
kind: OIDCPolicy
metadata:
  name: test-oidc
  namespace: ${GATEWAY_NS}
spec:
  targetRef:
    group: gateway.networking.k8s.io
    kind: HTTPRoute
    name: test-route
  provider:
    issuerURL: "https://your-oidc-provider.com"
    clientID: "your-client-id" 
EOF

Bug 1 Verification:

1. Wait for the OIDCPolicy to be enforced:

kubectl get -n $GATEWAY_NS oidcpolicy test-oidc -o jsonpath='{.status.conditions[?(@.type=="Enforced")].status}'

Should return: True

2. Check the generated callback AuthPolicy:
   Look for the redirectURI in the token request body CEL expression - it should include the port, In the metadata.token.http.body expression

  kubectl get -n $GATEWAY_NS authpolicy test-oidc-callback -o yaml | yq '.spec.overrides.rules.metadata.token.http.body'

You should see: "&redirect_uri=http%3A%2F%2Fexample.com%3A8001%2Fauth%2Fcallback"
Note the %3A8001 (URL-encoded :8001)

4. Check the authorize URL in the main AuthPolicy:
   Look in .spec.overrides.authScheme.response.unauthenticated.headers.location.value

  kubectl get -n $GATEWAY_NS authpolicy test-oidc -o yaml | yq '.spec.overrides.rules.response.unauthenticated.headers.location'

the redirect_uri parameter should include :8001

5. Test the cookie domain (should NOT include port):

  kubectl get -n $GATEWAY_NS authpolicy test-oidc -o yaml | yq '.spec.overrides.rules.response.unauthenticated.headers.set-cookie'

Should see: domain=example.com (not example.com:8001)

Bug 2 Verification:

1. Check the OPA authorization rule in the callback AuthPolicy:

kubectl get -n $GATEWAY_NS authpolicy test-oidc-callback -o yaml | yq '.spec.overrides.rules.authorization.location.opa.rego' | grep -o "indexof(trimmed"

Should return: indexof(trimmed (confirming the fix is present)

2. Verify it does NOT use the old broken pattern:

kubectl get -n $GATEWAY_NS  authpolicy test-oidc-callback -o yaml | yq '.spec.overrides.rules.authorization.location.opa.rego' | grep "count(kv) == 2"

Should return nothing (empty)

3. Check the cookie parsing logic:

kubectl get -n $GATEWAY_NS authpolicy test-oidc-callback -o yaml | yq '.spec.overrides.rules.authorization.location.opa.rego'

Should contain:

  eq_idx := indexof(trimmed, "=")
  eq_idx != -1
  name := trim(substring(trimmed, 0, eq_idx), " ")
  value := trim(substring(trimmed, eq_idx + 1, -1), " ")

Bug 3 Verification:

1. Check the target cookie expression in the main AuthPolicy:

kubectl get -n $GATEWAY_NS authpolicy test-oidc -o yaml | yq '.spec.overrides.rules.response.unauthenticated.headers.set-cookie.expression'

Should contain:

request.path + (request.query != "" ? "?" + request.query : "")

2. Verify it's NOT using the old broken pattern:

kubectl get -n $GATEWAY_NS authpolicy test-oidc -o yaml | yq '.spec.overrides.authScheme.response.unauthenticated.headers.set-cookie.expression' | grep 'request.path + "; domain='

Should return nothing (empty)

Summary by CodeRabbit

• New Features

  • Redirect handling now preserves non‑standard gateway ports and computes a base URL for post‑auth redirects.
  • Cookie target values include the request query when present; protocol-specific cookie attributes (including Secure) are applied.
  • Authorization rule generation updated to use the correct gateway/base/authorize URLs for redirects.

• Tests

  • Added extensive unit tests for URL/port handling, cookie expression construction and generated authorization rules.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: f13a25d7-564f-42fd-a47b-fa364ea48df7

📥 Commits

Reviewing files that changed from the base of the PR and between 1fac1f5 and e7fd60b.

📒 Files selected for processing (2)

• cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go
• cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go

🚧 Files skipped from review as they are similar to previous changes (2)

• cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go
• cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go

---

📝 Walkthrough

Walkthrough

Adds listener port in the proto and reconciler; introduces OIDCPolicy.GetBaseURL; preserves request.query in the target-cookie CEL expression; centralises OPA rego generation using indexof/substring for robust cookie parsing; and adds unit tests for these behaviours.

Changes

OIDC Policy redirect URI, cookie, and query fixes

Layer / File(s)	Summary
Proto schema and gateway listener resolution pkg/extension/grpc/v1/gateway_api.proto, internal/extension/reconciler.go, cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go	Listener proto message gains int32 port. toListeners() populates Name and Port; the ingress gateway resolution JSON now includes the listener port.
OIDCPolicy GetBaseURL and tests cmd/extensions/oidc-policy/api/v1alpha1/oidcpolicy_types.go, cmd/extensions/oidc-policy/api/v1alpha1/oidcpolicy_types_test.go	Adds func (p *OIDCPolicy) GetBaseURL(igwURL *url.URL) (*url.URL, error) which derives scheme+host from the redirect URL; tests cover standard/non-standard ports and stripping path/query.
URL construction with port handling cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go, cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go	ingressGatewayInfo adds Port and GetURL() which caches a URL and appends :port only for non-standard ports (not 80 for HTTP, not 443 for HTTPS). Tests validate formatting, caching, and cookie-domain behaviour.
Cookie target expression with query preservation cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go, cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go	Adds buildTargetCookieExpression(hostname, protocol) to produce CEL for set-cookie where target includes request.path plus ?request.query when non-empty; Secure flag applies for HTTPS. Integration replaces inline fmt.Sprintf. Tests validate ternary query handling and example behaviour.
OPA authorisation rule with improved cookie parsing cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler.go, cmd/extensions/oidc-policy/internal/controller/oidcpolicy_reconciler_test.go	Adds buildOpaAuthorizationRule(baseURL, igwURL, authorizeURL) which builds the rego rule using indexof/substring to split on the first =; buildCallbackAuthPolicy calls pol.GetBaseURL(igwURL) and uses the helper. Tests assert cookie parsing robustness, JWT padding handling, and correct redirect URL selection.

🎯 3 (Moderate) | ⏱️ ~25 minutes

A rabbit hops through the gates,
Ports now sit where hostname waits,
Cookies keep their queries tight,
Rego parses through the night,
Hooray — the callback's back in flight! 🐰🔐

🚥 Pre-merge checks | ✅ 3 | ❌ 2

❌ Failed checks (1 warning, 1 inconclusive)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.
Title check	❓ Inconclusive	The title 'Bug fixing OIDCPolicy' is vague and generic, using non-descriptive phrasing that does not clearly convey the specific bugs being addressed.	Use a more specific title that describes the main bugs fixed, such as 'Fix OIDCPolicy redirect URI port handling, cookie parsing, and query string preservation'.

✅ Passed checks (3 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Linked Issues check	✅ Passed	All three bugs from issue #2017 are addressed: port handling via ingressGatewayInfo and GetURL(), cookie parsing via buildOpaAuthorizationRule with indexof/substring, and query string preservation via buildTargetCookieExpression with conditional ternary.
Out of Scope Changes check	✅ Passed	All changes are directly aligned with issue #2017 objectives: port resolution infrastructure, OPA rego refactoring, cookie expression building, and comprehensive unit test coverage validate the fixes without introducing unrelated modifications.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

📝 Generate docstrings

• Create stacked PR
• Commit on current branch

🧪 Generate unit tests (beta)

• Create PR with unit tests
• Commit unit tests in branch oidc-bug-hunt

---

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[gitguardian]

️✅ There are no secrets present in this pull request anymore.

If these secrets were true positive and are still valid, we highly recommend you to revoke them.
While these secrets were previously flagged, we no longer have a reference to the
specific commits where they were detected. Once a secret has been leaked into a git
repository, you should consider it compromised, even if it was deleted immediately.
Find here more information about risks.

---

^{_{🦉 GitGuardian detects secrets in your source code to help developers and security teams secure the modern development process. You are seeing this because you or someone else with access to this repository has authorized GitGuardian to scan your pull request.}}

[codecov]

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 75.00%. Comparing base (5854bd4) to head (e7fd60b).
⚠️ Report is 36 commits behind head on main.

Additional details and impacted files

@@            Coverage Diff             @@
##             main    #2032      +/-   ##
==========================================
+ Coverage   74.90%   75.00%   +0.10%     
==========================================
  Files         127      127              
  Lines       12528    12619      +91     
==========================================
+ Hits         9384     9465      +81     
- Misses       2655     2668      +13     
+ Partials      489      486       -3     

Flag	Coverage Δ
bare-k8s-integration	20.12% <0.00%> (-0.20%)	⬇️
controllers-integration	53.49% <0.00%> (-0.32%)	⬇️
envoygateway-integration	40.77% <0.00%> (-0.09%)	⬇️
gatewayapi-integration	16.15% <0.00%> (-0.01%)	⬇️
istio-integration	44.59% <0.00%> (-0.25%)	⬇️
unit	28.66% <100.00%> (+0.32%)	⬆️

Flags with carried forward coverage won't be shown. Click here to find out more.

Components	Coverage Δ
api (u)	85.88% <ø> (ø)
internal (u)	76.43% <84.90%> (+0.11%)	⬆️
pkg (u)	39.13% <0.00%> (-0.12%)	⬇️

Files with missing lines	Coverage Δ
internal/extension/reconciler.go	98.54% <100.00%> (+0.02%)	⬆️

... and 13 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

[maleck13]

👀

[crstrn13]

LGTM! 🎉

[crstrn13]

The issue is that request.path in Authorino's CEL context already includes the query string (it's the full request URI, e.g. /get?foo=bar&baz=qux). Appending "?" + request.query on top of that duplicates the query parameters: e.g target=/get?foo=bar&baz=qux?foo=bar&baz=qux