Maniacs_Mansion/MealQuest
11
0
Fork 0
Code Issues Pull Requests Actions Packages Projects Releases Wiki Activity

Feature 03 task 5: Trusted proxy handling for forwarded headers #23

Merged
approver_agent merged 2 commits from task/trusted-proxy_c2119148-37e3-40bb-8dda-04dbcd5a3c4c into feature/9c4eba48-9a49-4b73-a85f-a4bc19eb4062-authentik-oidc-sessions 2026-10-03 11:05:34 -04:00
Conversation 2 Commits 2 Files Changed 12
+901 -6
dev_agent commented 2026-10-03 10:58:04 -04:00
Member
Copy Link
Copy Source

Implements Feature 03 task 5 (c2119148, final Feature 03 task). Done-condition: X-Forwarded-For and X-Forwarded-Proto are honoured only when the direct peer matches MEALQUEST_TRUSTED_PROXY (comma-separated IPs/CIDRs), otherwise ignored; resolved client IP used in logs; scheme feeds redirect/cookie logic so behind Traefik the OIDC redirect URI and Secure cookies are correct; tests cover trusted peer, untrusted spoofing and the redirect URI from MEALQUEST_PUBLIC_URL; quality floor passes.

Evidence (developer-run): gofmt empty; go vet clean; go test -race all packages pass; CGO_ENABLED=0 build ok; real binary with a stub IdP: trusted peer forwarded proto honoured with resolved client_ip in logs, untrusted peer headers ignored, redirect_uri built from MEALQUEST_PUBLIC_URL, invalid CIDR refused at startup. Deploy note: if Docker userland proxy fronts 8090, the container peer may be the bridge gateway, not 10.60.1.33; MEALQUEST_TRUSTED_PROXY must match the actual peer.

Implements Feature 03 task 5 (c2119148, final Feature 03 task). Done-condition: X-Forwarded-For and X-Forwarded-Proto are honoured only when the direct peer matches MEALQUEST_TRUSTED_PROXY (comma-separated IPs/CIDRs), otherwise ignored; resolved client IP used in logs; scheme feeds redirect/cookie logic so behind Traefik the OIDC redirect URI and Secure cookies are correct; tests cover trusted peer, untrusted spoofing and the redirect URI from MEALQUEST_PUBLIC_URL; quality floor passes. Evidence (developer-run): gofmt empty; go vet clean; go test -race all packages pass; CGO_ENABLED=0 build ok; real binary with a stub IdP: trusted peer forwarded proto honoured with resolved client_ip in logs, untrusted peer headers ignored, redirect_uri built from MEALQUEST_PUBLIC_URL, invalid CIDR refused at startup. Deploy note: if Docker userland proxy fronts 8090, the container peer may be the bridge gateway, not 10.60.1.33; MEALQUEST_TRUSTED_PROXY must match the actual peer.
dev_agent added 1 commit 2026-10-03 10:58:05 -04:00
Honour forwarded headers only from MEALQUEST_TRUSTED_PROXY
CI / ci (pull_request) Successful in 1m26s
Details
Independent review Codex gpt-6-sol high: CHANGES_REQUIRED (1 high, 1 medium)
Details
CI / ci (push) Successful in 1m25s
Details
7e49b72ad4 
Add internal/forwarded: parse the trusted proxy list (comma-separated IPs
and CIDRs, validated at load), resolve each request's client IP and scheme,
and expose them through context helpers. X-Forwarded-For and
X-Forwarded-Proto are honoured only when the TCP peer is trusted; the client
is the rightmost X-Forwarded-For address that is not a trusted proxy.
Malformed forwarded headers from a trusted peer get 400.

The router resolves every request first; a slog handler adds client_ip to
request-scoped log lines. The auth routes refuse requests whose resolved
scheme differs from MEALQUEST_PUBLIC_URL's, so a proxy that is not trusted
fails loudly instead of breaking Secure cookies. The OIDC redirect URI is
still built only from MEALQUEST_PUBLIC_URL.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
review_agent left a comment
Member
Copy Link
Copy Source

Independent review — Codex gpt-6-sol, reasoning high

Verdict: CHANGES_REQUIRED
Reviewed SHA: 7e49b72ad4d29461ed44f915212c89ba1c90f99a

{
  "verdict": "CHANGES_REQUIRED",
  "reviewed_sha": "7e49b72ad4d29461ed44f915212c89ba1c90f99a",
  "summary": "Trusted proxy parsing, forwarded-header resolution, and client-IP logging are wired into the router. gofmt, go vet, and go test -race -count=1 ./... passed. The new auth scheme gate creates a possible production sign-in outage, and the redirect test does not exercise the production router.",
  "findings": [
    {
      "id": "F1",
      "severity": "high",
      "location": "internal/auth/auth.go:217",
      "failure_case": "If Docker’s published-port path presents its bridge gateway or userland proxy as the direct peer instead of 10.60.1.33, forwarded headers are correctly ignored, but every login, callback, and logout request returns 400 because the resolved scheme is HTTP.",
      "evidence": "requirePublicScheme rejects any scheme differing from the HTTPS public URL. The documented deployment maps host port 8090 to container port 8080, while the documented trusted list contains only Traefik’s 10.60.1.33. The redirect URI and Secure cookie were already fixed by configuration before this change.",
      "suggested_correction": "Remove the scheme-mismatch refusal. Verify the direct peer seen through the deployed Docker port path and configure a network path that preserves Traefik’s identity before relying on its forwarded headers."
    },
    {
      "id": "F2",
      "severity": "medium",
      "location": "internal/auth/auth_test.go:516",
      "failure_case": "A production-router wiring error could break the OIDC login redirect while the new redirect test still passes.",
      "evidence": "TestLoginBehindTraefik builds its own chi router and copies the origin-resolution steps; the NewRouter tests use probeAuth instead of the real auth flow. No test sends a login request through NewRouter and checks the redirect URI from MEALQUEST_PUBLIC_URL.",
      "suggested_correction": "Exercise the real auth flow through server.NewRouter with trusted and untrusted direct peers, and assert the redirect URI and Secure cookie."
    }
  ]
}

Posted unaltered by review_agent. This is transport, not a second review.

## Independent review — Codex `gpt-6-sol`, reasoning high Verdict: **CHANGES_REQUIRED** Reviewed SHA: `7e49b72ad4d29461ed44f915212c89ba1c90f99a` ```json { "verdict": "CHANGES_REQUIRED", "reviewed_sha": "7e49b72ad4d29461ed44f915212c89ba1c90f99a", "summary": "Trusted proxy parsing, forwarded-header resolution, and client-IP logging are wired into the router. gofmt, go vet, and go test -race -count=1 ./... passed. The new auth scheme gate creates a possible production sign-in outage, and the redirect test does not exercise the production router.", "findings": [ { "id": "F1", "severity": "high", "location": "internal/auth/auth.go:217", "failure_case": "If Docker’s published-port path presents its bridge gateway or userland proxy as the direct peer instead of 10.60.1.33, forwarded headers are correctly ignored, but every login, callback, and logout request returns 400 because the resolved scheme is HTTP.", "evidence": "requirePublicScheme rejects any scheme differing from the HTTPS public URL. The documented deployment maps host port 8090 to container port 8080, while the documented trusted list contains only Traefik’s 10.60.1.33. The redirect URI and Secure cookie were already fixed by configuration before this change.", "suggested_correction": "Remove the scheme-mismatch refusal. Verify the direct peer seen through the deployed Docker port path and configure a network path that preserves Traefik’s identity before relying on its forwarded headers." }, { "id": "F2", "severity": "medium", "location": "internal/auth/auth_test.go:516", "failure_case": "A production-router wiring error could break the OIDC login redirect while the new redirect test still passes.", "evidence": "TestLoginBehindTraefik builds its own chi router and copies the origin-resolution steps; the NewRouter tests use probeAuth instead of the real auth flow. No test sends a login request through NewRouter and checks the redirect URI from MEALQUEST_PUBLIC_URL.", "suggested_correction": "Exercise the real auth flow through server.NewRouter with trusted and untrusted direct peers, and assert the redirect URI and Secure cookie." } ] } ``` --- Posted unaltered by `review_agent`. This is transport, not a second review.
dev_agent added 1 commit 2026-10-03 11:02:16 -04:00
Stop refusing sign-in over a scheme other than the public URL's
Independent review Codex gpt-6-sol high: APPROVED (0 findings)
Details
CI / ci (push) Successful in 1m25s
Details
CI / ci (pull_request) Successful in 1m25s
Details
17d5152247 
The auth routes no longer return 400 when the resolved request scheme
differs from MEALQUEST_PUBLIC_URL's. If Docker's published-port path
presents its bridge gateway instead of Traefik as the direct peer, the
forwarded headers are rightly ignored, but sign-in must still work: the
OIDC redirect URI and the Secure cookies are fixed by MEALQUEST_PUBLIC_URL,
not by the request. internal/auth is back to its state before the
trusted-proxy change; client IP and scheme resolution stays in the router.

Add TestLoginThroughRouter: it wires the real auth flow and session
manager through server.NewRouter, trusting Traefik only, and checks that
a login from Traefik and from an untrusted peer both redirect with the
redirect URI from MEALQUEST_PUBLIC_URL and set a Secure login cookie.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
review_agent left a comment
Member
Copy Link
Copy Source

Independent review — Codex gpt-6-sol, reasoning high

Verdict: APPROVED
Reviewed SHA: 17d5152247b8c1adc845340be225c19b7b3bd79a

{
  "verdict": "APPROVED",
  "reviewed_sha": "17d5152247b8c1adc845340be225c19b7b3bd79a",
  "summary": "Both prior findings are fixed. The production router resolves forwarded headers only for trusted peers, logs the resolved client IP, and the login test verifies the public URL redirect URI and Secure cookie for trusted and untrusted peers. Configuration rejects invalid proxy entries. gofmt, go vet, and go test -race -count=1 ./... passed. Deployment should verify the direct peer address: Docker userland proxying could present a bridge gateway instead of Traefik, causing forwarded headers to be ignored and that gateway to appear in logs; sign-in remains unaffected.",
  "findings": []
}

Posted unaltered by review_agent. This is transport, not a second review.

## Independent review — Codex `gpt-6-sol`, reasoning high Verdict: **APPROVED** Reviewed SHA: `17d5152247b8c1adc845340be225c19b7b3bd79a` ```json { "verdict": "APPROVED", "reviewed_sha": "17d5152247b8c1adc845340be225c19b7b3bd79a", "summary": "Both prior findings are fixed. The production router resolves forwarded headers only for trusted peers, logs the resolved client IP, and the login test verifies the public URL redirect URI and Secure cookie for trusted and untrusted peers. Configuration rejects invalid proxy entries. gofmt, go vet, and go test -race -count=1 ./... passed. Deployment should verify the direct peer address: Docker userland proxying could present a bridge gateway instead of Traefik, causing forwarded headers to be ignored and that gateway to appear in logs; sign-in remains unaffected.", "findings": [] } ``` --- Posted unaltered by `review_agent`. This is transport, not a second review.
approver_agent merged commit 5833a73541 into feature/9c4eba48-9a49-4b73-a85f-a4bc19eb4062-authentik-oidc-sessions 2026-10-03 11:05:34 -04:00
Sign in to join this conversation.
Reviewers
No Reviewers
review_agent
Labels
No items
No labels
Milestone
No items
No Milestone
Projects
Clear projects
No projects
Assignees
Clear assignees
approver_agent (The Approver) chatgpt claude-ai Claude-Code codex (Codex) dev_agent grok opencode orchestrator-agent overseer (Joshua) review_agent (The Reviewer)
No Assignees
2 Participants
Notifications
Due Date
No due date set.
Dependencies

No dependencies set.

Reference: Maniacs_Mansion/MealQuest#23