feat(python): MCP server OAuth form quick wins (read-only redirect URI, drop password grant)

[Altamimi-Dev]

Pull Request

🔗 Related Issue

Relates to #5721 (delivers the backend pieces of User Stories 1, 2, and 4; Story 3 — pre-fill from MCP initialize — is deferred, see #5719)

---

📝 Summary

Backend half of the MCP server OAuth form quick wins: closes an OAuth 2.1 foot-gun (deprecated password grant) and hardens redirect_uri handling for the authorization-code flow. The React UI half (read-only derived redirect URI field, grant-type selector) now lives in contextforge-org/contextforge-web-ui#15 — client/ has moved to its own repo and epic/ui-rewrite had diverged too far from main to keep both halves in one PR, so this one was rescoped to target main directly.

• Deprecated password grant rejected (Story 4): GatewayCreate rejects grant_type == "password" at the schema layer, so new MCP server registrations can't use it regardless of how they're created (UI or API). Existing gateways that already use it keep working through GatewayUpdate — but GatewayUpdate now also rejects newly adopting password on a gateway that wasn't already using it, so the create-time restriction can't be bypassed by flipping an existing gateway's grant type via the update path. The legacy admin UI's gateway creation form no longer offers the option either (the edit form still does, for loading/saving existing legacy configs).
• redirect_uri defaulting (Story 1, server-side): initiate_oauth_flow and oauth_callback default a missing redirect_uri to the gateway's own global callback ({app_domain}{app_root_path}/oauth/callback), so API-created and legacy configs can't reach OAuthManager's PKCE paths without one. The default accounts for a reverse-proxy app_root_path, logs when it's actually applied, and the substitution logic is centralized in OAuthManager (rather than duplicated per call site) so any future caller is protected the same way.
• redirect_uri pinned across the flow: the value sent to the IdP at authorize time is now pinned into server-side OAuth state (alongside the PKCE code_verifier) and reused at token-exchange time, instead of each side independently recomputing it — so a concurrent gateway config change or app_domain change between authorize and callback can't cause the two to diverge (RFC 6749 §4.1.3 requires them to match). Adds a redirect_uri column to oauth_states (migration included).
• Resource (RFC 8707) — Story 2: already satisfied server-side; no change needed beyond confirming the behavior (the gateway derives resource from the normalised MCP server URL when unset).

---

📏 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

---

🏷️ Type of Change

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

---

🧪 Verification

Check	Command	Status
Schema/gateway tests	pytest tests/unit/mcpgateway/test_schemas_validators_extra.py tests/unit/mcpgateway/services/test_gateway_service.py -k "oauth or gateway_create or gateway_update or password"	✅
OAuth router tests	pytest tests/unit/mcpgateway/routers/test_oauth_router.py	✅
OAuth manager tests	pytest tests/unit/mcpgateway/services/test_oauth_manager.py	✅
Lint / docstrings	make ruff interrogate	✅
Alembic heads	alembic heads	✅ single head

Manual: GatewayCreate with grant_type=password is rejected; GatewayUpdate accepts it only when the gateway already used it, and rejects flipping a non-password gateway to it. A gateway with no redirect_uri configured gets one derived and logged on both the authorize and callback legs, matching each other even if config changes mid-flow.

---

✅ Checklist

• Code formatted
• Tests added/updated for changes
• Documentation updated (if applicable)
• No secrets or credentials committed

[marekdano]

Hi, @Altamimi-Dev! Thank you for your contribution. To keep the future merging of this branch epic/ui-rewrite as clean as possible, we're avoiding merging edits to the python files. Would you please split the backend work for this pull request into a separate PR, pushing it straight to the main branch?

[marekdano]

Hey @Altamimi-Dev — thanks for this, the redirect URI and password grant fixes are exactly what #5721 needed.

A heads up on what changed here: since client/ has moved out to its own repo (contextforge-org/contextforge-web-ui) and epic/ui-rewrite has diverged quite a bit from
main, I rescoped this PR to the backend-only pieces so it could target main directly:

• GatewayCreate rejecting the deprecated password grant
• initiate_oauth_flow/oauth_callback defaulting a missing redirect_uri server-side

To do that I force-pushed this branch, so the history here is now just those backend commits rebased onto main — worth knowing if you have a local copy of the old branch.

The UI piece (read-only derived redirect URI with copy button + localhost warning, password grant hidden except for legacy configs) is carried over as its own PR against the new UI repo: contextforge-org/contextforge-web-ui#15.
Attribution/credit for that work is yours — happy to add you as co-author on that PR if you'd like, just let me know your preferred email for the Co-authored-by trailer.

Let me know if anything here looks off.

[Altamimi-Dev]

Thanks for taking the time to do the split yourself, @marekdano — and for the heads-up on the force-push. I still have the old epic-based head on a local branch, so nothing was lost on my side.

I went through both halves and the rescope looks good to me:

Backend (this PR) — both reviewed pieces survived intact:

• GatewayCreate.validate_oauth_config rejecting grant_type == "password" at the schema layer, with the message we settled on.
• _default_redirect_uri() applied on both sides of the flow — in initiate_oauth_flow (before DCR, so the registered redirect URI matches what's later sent to the IdP) and in oauth_callback.
• The tests came along, so the deny paths are still covered.

UI (contextforge-org/contextforge-web-ui#15) — reads as a faithful port of the reviewed behavior:

• The derived URI is lifted into form state via onRedirectUriChange, and the effect only fires while the stored value is empty — so a config that already has a redirect_uri is displayed verbatim and never overwritten.
• The localhost/127.0.0.1 warning is keyed off displayRedirectUri, i.e. the value actually shown, which is the behavior we wanted.
• Password grant is rendered only when it's already the selected value, with the OAuth 2.1 deprecation note — legacy configs stay editable without offering the option to new ones.
• Nice bonus on the i18n pass and the copy-to-clipboard affordance; both are improvements over what I had.

One small thing you may want to weigh: the OAuth Password Grant Validation block in MCPServerForm.test.tsx was dropped in #15. That makes sense as written — the helper drove the dropdown option that no longer renders — but it does leave the form-level "username/password required for password grant" validation untested. If it's worth keeping, driving it from initialData with grant_type: "password" instead of the dropdown would exercise the same paths without needing the hidden option. Entirely optional, and not something I'd hold #15 for.

And yes please on the co-author credit, much appreciated:

Co-authored-by: Ahmad Al Tamimi <altamimi.dev@gmail.com>

For context on the two related PRs: I'm applying the same treatment to #5930 (retargeted to main — it was Python + legacy admin-UI JS only, so no UI-repo counterpart needed) and #5934 (backend-only on main, with the React half going to contextforge-web-ui), following the pattern you established here.

[a-effort]

LGTM! 🚀

[msureshkumar88]

Thanks for tackling the OAuth password-grant deprecation and redirect_uri defaulting from #5721 — the backend direction (rejecting the deprecated grant at the schema layer, defaulting redirect_uri server-side) is the right approach, and the added tests give reasonable coverage for the code paths that were touched.

A few things worth addressing before merge:

Scope vs. PR description
The PR summary describes a React UI change (read-only redirect URI field with copy button, grant-type selector with password removed) as delivered, but the diff against main only touches mcpgateway/routers/oauth_router.py, mcpgateway/schemas.py, and three test files — no client/ or UI files are present. It looks like that UI work lives on a separate branch (epic/ui-rewrite) rather than this PR. Worth correcting the description (or pulling the UI piece in) so reviewers don't approve believing Stories 1 and 4's UI requirements are covered here.

Security: password grant still reachable via update path
GatewayCreate rejects grant_type == "password" in schemas.py, but GatewayUpdate.validate_oauth_config (~line 3581) only calls _validate_oauth_config_urls and doesn't apply the same check. Since GatewayUpdate backs the public PUT/PATCH /gateways/{id} endpoint, an existing gateway can still be flipped to the deprecated password grant through the update path. This should mirror the same rejection as GatewayCreate — otherwise the fix is bypassable.

UI: legacy admin form still offers the deprecated grant
mcpgateway/templates/admin.html (~line 5495) still lists <option value="password">Resource Owner Password Credentials (Keycloak/Legacy)</option> in the gateway-creation form with no guard. With the schema-level rejection now in place, selecting it in the UI will hit a 422 with no prior warning — worth removing/guarding the option here too, both for UX and to actually satisfy Story 4's grant-selector requirement in the UI that does ship.

redirect_uri default doesn't account for app_root_path
_default_redirect_uri() (~line 119) builds the callback URL from settings.app_domain alone. Other URL builders in the codebase (e.g. email_auth_service.py) combine app_domain with app_root_path/request-scoped root_path for exactly this reason. In a reverse-proxied deployment with a non-empty root path, the default redirect_uri would be wrong. Suggest aligning with the existing pattern.

Missing logging when the default is used
Issue #5721's security requirements call out that domain misconfiguration should be surfaced, not silently mis-derived. Right now _default_redirect_uri() doesn't log when it's invoked (including when app_domain falls back to localhost), so there's no server-side signal for an operator debugging a failed callback in production. A log line at use would help close that gap.

redirect_uri recomputed at callback instead of pinned at authorize
The default-uri guard runs independently at authorize time (~line 540) and again at callback time (~line 818), both reading live gateway state. If the gateway's oauth_config changes between authorize and callback (e.g. a concurrent update), the value sent to the IdP at authorize and the value used at callback can diverge, breaking the token exchange per RFC 6749 §4.1.3. Pinning the computed redirect_uri at authorize time (e.g. in session/state) and reusing it at callback would avoid this.

Minor: duplicated guard logic
The if not oauth_config.get("redirect_uri"): oauth_config["redirect_uri"] = _default_redirect_uri() block is duplicated in both initiate_oauth_flow and oauth_callback. Since the actual failure this PR fixes originates in OAuthManager (which indexes credentials["redirect_uri"] directly), centralizing the default there would prevent the bug from resurfacing if a future call path bypasses these two router functions. Non-blocking, but worth a follow-up.

No Alembic migration is needed here (no schema change), and that's correctly reflected in the diff.

[marekdano]

@Altamimi-Dev - I've pushed the updates to unblock this PR and get it merged asap.
But please feel free to continue on this PR when a new request for changes arises.

[Altamimi-Dev]

@marekdano thanks a lot for jumping in with the review-fix commits — they cover every point @msureshkumar88 raised (password-grant rejection on the update path, the admin.html option removal, app_root_path in the server-side default, the logging touch-ups, redirect_uri pinned in oauth_states via the new migration, and the centralized fallback in OAuthManager), so no further code changes were needed here.

I've rebased the branch onto current main to clear the conflict. Details:

• The only collision was .secrets.baseline. Per the repo convention I took main's version at each replay, which made your two baseline-only commits empty (they were dropped), then regenerated once with make detect-secrets-scan — the result is a single chore: regenerate .secrets.baseline commit that is content-equivalent to what you had. No code changes were touched by the rebase.
• Alembic is happy: alembic heads prints exactly one head, db41939315aa.
• Verification on the rebased branch: the OAuth-focused suites (test_oauth_router.py, test_oauth_manager.py ×2, test_gateway_service.py) pass at 850 passed / 1 skipped, and the full make test run is green at 22008 passed / 843 skipped. The one failure I saw locally (test_jq_runner.py::test_pool_is_rebuilt_after_pid_change) reproduces identically on unmodified main on macOS — the file is byte-identical to main on this branch, so it's unrelated to this PR.

The PR now shows as mergeable. Should be good to go once CI is green.

[Lang-Akshay]

LGTM ✅

[msureshkumar88]

Verified all requested changes from the earlier review are addressed:

• GatewayUpdate now rejects newly adopting the password grant on a gateway that wasn't already using it (gateway_service.py), closing the create-time bypass.
• Legacy admin gateway-creation form no longer offers the password grant option; edit form retains it for existing configs.
• _default_redirect_uri() now accounts for app_root_path via resolve_root_path(), and logs when the default is applied (both authorize and callback legs, centralized in OAuthManager._apply_default_redirect_uri).
• redirect_uri is now pinned into oauth_states at authorize time and reused at callback (RFC 6749 §4.1.3), with migration db41939315aa adding the column.
• Ran the PR's listed test commands locally — 71/71 pass.

One minor, non-blocking observation: the gateway edit form (admin.html ~line 10066) still unconditionally lists the password grant option for every gateway regardless of current grant type. Selecting it on a non-password gateway now hits the new 422 rejection with no UI guard — a small UX gap, not a functional blocker. Worth a quick follow-up to disable/hide the option when the gateway isn't already on password grant.

Approving — backend changes are correct, well-tested, and match the PR description.

[msureshkumar88]

Closing in favor of #6315, which carries these same commits (unchanged authorship/sign-off from @marekdano, opened here by @Altamimi-Dev) rebased onto current main. That PR's head lives on a fork this account can't push to, so the rebase had to land as a fresh PR — see credit note there. #6315 has been merged.