feat: add post-OAuth redirect allowlist (redirect_uri_after_oauth)

[cafalchio]

Pull Request

🔗 Related Issue

Partially addresses #6309

---

📝 Summary

Adds redirect_uri_after_oauth — an optional field in a gateway's oauth_config that causes the browser to be sent to an operator-configured external URL after a successful Authorization Code OAuth callback, instead of landing on the gateway's built-in success page.

{
  "oauth_config": {
    "grant_type": "authorization_code",
    "redirect_uri_after_oauth": "https://app.example.com/oauth-complete"
  }
}

The destination must be within the single HTTPS origin declared by the new OAUTH_REDIRECT_ALLOWED_ORIGIN environment variable, which is validated at startup.

Flow: register MCP OAuth → user logs in → callback → 302 to redirect_uri_after_oauth

---

⚠️ Design note — divergence from issue #6309

Issue #6309 proposed a per-flow, dynamic mechanism: a post_login_redirect_uri query parameter passed at GET /oauth/authorize, encoded into the OAuth state, and decoded on callback.

This PR deliberately implements a static, per-gateway allowlist instead. The reason is security:

• The per-flow design gives every request an opportunity to inject an attacker-controlled redirect destination. A malicious login link (?post_login_redirect_uri=https://evil.com) would send the victim to an attacker-controlled page after authentication — a classic open-redirect / token-leakage vector documented in RFC 6819 §4.2.4.
• A static allowlist has zero per-request surface: the destination is set by an admin at configuration time, not by a caller at runtime.

Trade-off: multi-app deployments are currently capped at one external origin per gateway deployment. This covers the common single-app case (the reporter's stated need: "bring the user back to our own UI"). The per-flow dynamic mechanism from the issue remains unimplemented and is out of scope for this PR.

Admin UI field: Adding the field to the Admin UI is intentionally deferred to a follow-up — the feature is fully usable via the REST API today.

---

What changed

mcpgateway/utils/origin.py (new) — shared origin utilities extracted from admin.py:

• normalize_origin_parts() — normalises scheme/host/port for exact same-origin comparison (moved from admin._normalize_origin_parts)
• origin_from_url() — extracts scheme://host[:port] from any URL
• is_same_origin() — absolute URL same-origin check with backslash rejection
• is_exact_https_origin() — strict HTTPS-only origin validator (no path/query/fragment/credentials)
• is_allowed_redirect() — gate used at every enforcement point

mcpgateway/config.py — new oauth_redirect_allowed_origin field validated at startup by is_exact_https_origin(); rejects wildcards, HTTP, paths, credentials.

mcpgateway/schemas.py — _validate_oauth_config_urls enforces redirect_uri_after_oauth must be absolute and within allowed origins at schema validation time (Pydantic layer).

mcpgateway/admin.py — _assemble_oauth_config_from_fields enforces the same check for the admin form path (non-Pydantic path). _normalize_origin_parts removed in favour of the shared import.

mcpgateway/routers/oauth_router.py — custom_redirect_after_callback() validates and issues the 302 redirect with Referrer-Policy: no-referrer after a successful non-popup OAuth flow.

---

Security invariants

• Fail-closed at three layers: Pydantic schema validation, admin form assembler, and again at redirect time (covering legacy rows written before validation existed).
• Redirect-time validation runs before complete_authorization_code_flow, so a misconfigured URL does not burn the one-time state.
• Relative URLs rejected (no scheme/netloc → fail-closed).
• Backslash-normalised paths (/\\evil.example) rejected.
• HTTP external origins rejected (HTTPS-only).
• Credentials in URLs (user@host) rejected.
• Popup mode unaffected — redirect skipped when is_popup=True.
• No tokens in the redirect URL; Referrer-Policy: no-referrer prevents leaking the consumed code/state to the external origin.

---

📏 Reviewability

• This PR has one clear purpose
• The linked issue is not labeled triage
• Unrelated bugs or improvements are tracked in separate issues/PRs
• Tests are included with the code they validate
• If AI-assisted, I understand and can explain the generated changes

---

🏷️ Type of Change

• Bug fix
• Feature / Enhancement
• Documentation
• Refactor
• Chore (deps, CI, tooling)
• Other (describe below)

---

✅ Checklist

• Code formatted (make black isort pre-commit)
• Tests added/updated for changes
• Documentation updated (if applicable)
• No secrets or credentials committed

[jonpspri]

Review: post-OAuth redirect allowlist (redirect_uri_after_oauth)

Thanks for tackling this — the security engineering here is genuinely strong: fail-closed validation at three layers, a startup-validated exact-origin allowlist, and deny-path tests covering the nasty bypass classes (network-path references, backslash normalization, credentials-in-URL, malformed ports). I reviewed the full diff against main, read issue #6309, and ran the new/affected tests against the PR tree — all green (87 focused tests across utils/config/schemas/router, plus the 38 admin CSRF/assembly tests). Findings below, categorized.

---

Blocking (scope reconciliation)

The PR implements a different mechanism than issue #6309 specifies, and never says so. The issue asks for a per-flow post_login_redirect_uri passed when the flow is initiated (GET /oauth/authorize?...&post_login_redirect_uri=..) and encoded into state, with acceptance criteria written against that flow. This PR delivers a static, per-gateway admin config gated by a single global OAUTH_REDIRECT_ALLOWED_ORIGIN.

To be clear, I think the chosen design is defensible — arguably safer than the issue's sketch, since a static allowlist has no per-request open-redirect surface at all. It also does meet the reporter's underlying need ("bring the user back to our own UI") for the common single-app case. But the per-flow acceptance criteria are literally unimplemented, multi-app deployments are capped at one external origin per gateway deployment, and the PR body claims "Closes #6309" without acknowledging the divergence. Worth an explicit note in the PR description and a comment on the issue confirming the reporter accepts static-per-gateway config as the resolution (or scoping this PR as partial).

Functionally impacting

• No Admin UI field. admin.html carries oauth_redirect_uri inputs in four places, but oauth_redirect_uri_after_oauth appears nowhere in templates or admin_ui/. The feature is REST-API-only, and the new branch in _assemble_oauth_config_from_fields is unreachable from the actual UI. Either add the input or document the feature as API-only.
• A2A path accepts the field inertly. The shared assembler also serves A2A agent create/edit, so A2A oauth_config can now carry redirect_uri_after_oauth, which the A2A flow never consumes. Harmless, but confusing — worth gating or a comment.
• No documentation. Zero references in docs/ for the new env var or the new oauth_config key; the PR checklist leaves docs unchecked. A short section in the OAuth management docs would save operators real guesswork, especially around the single-global-origin limitation.

Security assessment — no blocking findings

The threat model (open redirect, token leakage, state burning) is well handled:

• Fail-closed at three layers: Pydantic schema (both GatewayCreate and GatewayUpdate), the admin form assembler, and again at redirect time — the last covering legacy rows written before validation existed. Redirect-time validation runs before complete_authorization_code_flow, so a bad config doesn't burn the one-time state (nice that a test asserts the exchange is never awaited).
• Allowlist entry validated at startup as an exact HTTPS origin; wildcards, HTTP, paths, query, credentials all rejected.
• No tokens in the redirect URL; Referrer-Policy: no-referrer prevents leaking the consumed code/state to the external origin; the redirect response carries no gateway-scoped cookies (the test asserting zero set-cookie headers is a good guard).
• Popup mode cleanly excluded, with a regression test asserting no Location header.

One maintenance risk worth noting (not a vulnerability today): the validation predicate and its error message are copy-pasted in three places, so a future hardening fix applied in one spot could silently miss another — see suggestions.

Suggestions (refactoring opportunities)

The extraction of _normalize_origin_parts from admin.py into a shared mcpgateway/utils/origin.py is a good move — a few follow-throughs in the same spirit:

1. Triplicated validation + error string. The identical is_allowed_redirect(...) check and verbatim f-string message appear in schemas.py, admin.py, and oauth_router.py. The router's _validate_post_oauth_redirect is already the right shape — hoisting one validate_post_oauth_redirect(url) helper into origin.py (callers wrap into ValueError/OAuthError) removes drift risk on a security control.
2. origin_from_url duplicates derive_resource_origin. mcpgateway/utils/oauth_resource.py already has a well-documented scheme://netloc extractor used by four call sites. One should delegate to the other — two origin-extraction helpers in adjacent utils is exactly the duplication this PR set out to kill.
3. Dead generality: custom_redirect_after_callback(url, status_code) is only ever called with 302. Dropping the parameter would simplify.
4. Wasted work in the callback: when the redirect fires, the code has already built the full success HTML page, a CSRF token, and a short-lived session JWT — all discarded. Short-circuiting right after token storage (when redirect_uri_after_oauth is set and non-popup) would be clearer and avoid minting a throwaway JWT.
5. Follow-up, not this PR: auth_middleware.py and rbac.py still carry their own inline same-origin Referer parsing; origin.py is now the natural home for a future consolidation.

Minor notes

• Three of four commits are titled secrets — DCO is present, but please squash/rename to a conventional feat: message before merge.
• CI observation: the pytest matrix jobs show as skipped on this PR (and the check name renders as the unevaluated py${{ matrix.python }}, which looks like a workflow bug). The unit suite didn't run in CI; I ran the new/affected tests locally against the PR tree to compensate — all pass.
• Good call including the tests/live_gateway/ black-box test for the 422 reject paths; happy-path redirect is covered by unit tests.

---

Summary: solid, security-first implementation of a reasonable design. Before merge: reconcile the design with #6309 explicitly (PR description + issue comment), add the Admin UI field or document API-only, add docs, consolidate the triplicated validation, and squash the commits. Happy to discuss any of these.

[madhu-mohan-jaishankar]

Any Admin-UI edit of an OAuth gateway silently wipes the new setting (mcpgateway/admin.py:13232, gateway_service.py:2956-2973)
The Admin UI field was deferred, so the edit form doesn't submit oauth_redirect_uri_after_oauth and update_gateway rebuilds oauth_config from the form fields. If an operator sets the redirect via the REST API and an admin later edits any unrelated field on that gateway in the UI, the setting is silently deleted with no error anywhere.

Fix: preserve the existing value when the form omits it (mirroring the client-secret preservation pattern), or add a hidden form field until the UI ships.

[jonpspri]

Review — cycle 2: redirect_uri_after_oauth

Thanks for the quick turnaround on the previous round. The scope-divergence note in the PR description is exactly what was needed, the new docs section is clear (the RFC 6819 rationale for rejecting per-flow redirect params is well stated), and the deny-path test coverage remains excellent. I re-verified against the diff, issue #6309, and a fresh test run on this tree: 648 focused tests across utils/config/schemas/oauth_router/gateway-service plus the 38 admin CSRF tests — all green.

Two blocking items below, then minor notes.

---

Blocking

1. The "silent wipe" fix is undermined by the new hidden form input (admin.py:13257, admin.html:5699 & 10303)

The preservation branch in admin_edit_gateway only fires when the form omits the field — but both gateway forms now render:

<input type="hidden" name="redirect_uri_after_success" value="{{ oauth_redirect_allowed_origin }}" />

So whenever OAUTH_REDIRECT_ALLOWED_ORIGIN is set (the only deployments where this feature works), the form always submits the bare origin, and the preservation branch never fires. Two consequences:

• A REST-configured https://app.example.com/oauth-complete gets clobbered to https://app.example.com on any unrelated UI edit — the same bug class @madhu-mohan-jaishankar flagged, still reachable.
• Every UI-created/edited OAuth gateway silently gets the external redirect enabled, even when the operator never asked for it.

Also, the comment in admin_edit_gateway says the form "never POSTs it back," which the hidden input contradicts. A couple of approaches: drop the hidden input so the display is truly read-only (simplest, and matches the docs' "REST API only" note), or populate the edit form from the gateway's actual stored config.

2. Unrelated issue-#5496 team-filter change riding in this PR (gateway_service.py:2545, test.sh)

list_gateways now applies query.where(DbGateway.team_id == team_id), with a test asserting exact-team filtering and a test.sh reproducer at repo root naming issue #5496. This is a separate concern from the OAuth redirect feature, and it changes Layer-1 list semantics for ?team_id=: it excludes platform-public rows, which contradicts the documented contract in _team_scoped_conditions ("team_id narrows team-scoped rows, it does not suppress platform-public ones", #4732/#4773) and makes gateways inconsistent with the other list endpoints.

That semantic question may well be worth revisiting, but it deserves its own PR where the #4732 trade-off can be debated on its merits — and the PR checklist here asserts one clear purpose with unrelated fixes tracked separately. Worth splitting out (and either dropping test.sh or moving it under tests/live_gateway/ if it's worth keeping).

---

Security & performance — no other concerns

The OAuth mechanism itself looks solid: fail-closed at schema, assembler, and redirect time; redirect validation runs before complete_authorization_code_flow so a bad config doesn't burn the one-time state (test-verified); network-path/backslash/credential bypasses are covered by tests; Referrer-Policy: no-referrer and no tokens in the URL; popup path unaffected; and the success-page CSRF/temp-JWT cookies are correctly dropped when the redirect replaces the response. Performance impact is negligible.

Minor

• package-lock.json churn (removed root name, stripped libc fields) looks like regeneration with a different npm version — unrelated to this PR; probably worth reverting.
• Docs say "Admin UI support planned for a future release" while the template ships a UI field — worth aligning one way or the other once item 1 is resolved.
• A2A oauth_config can still carry redirect_uri_after_oauth inertly via the shared assembler (carried over from last round; harmless, but a comment would help future readers).

[cafalchio]

@jonpspri There were a rebase issue.

• Silent wipe/default opt-in: addressed.
• Unrelated team-filter, test.sh, lockfile churn: removed.
• Docs/UI mismatch: addressed.
• Scope divergence from issue: documented; issue comment posted.
• A2A inert field: still present, non-blocking.
• Refactoring/performance suggestions: mostly unaddressed, non-blocking.
• Commit squash/conventional naming: unaddressed.

[madhu-mohan-jaishankar]

LGTM