fix: enhance OAuth callback handling for error responses and missing authorization code

[ankit-gajera-merck]

🔗 Related Issue

Closes #2881

---

📝 Summary

This PR fixes Microsoft Entra ID v2 OAuth authorization failures (AADSTS9010010) caused by sending resource together with v2 scope, and fixes callback handling when providers return OAuth errors without an authorization code.

Key changes:

• Added provider-aware logic in OAuth manager to omit resource for Entra v2 scope-based flows (https://login.microsoftonline.com/.../oauth2/v2.0/...).
• Added explicit omit_resource support in oauth_config to disable resource injection when needed.
• Applied the same resource omission logic consistently across:
  • authorization URL construction
  • authorization code token exchange
  • refresh token flow
• Updated /oauth/callback to:
  • accept optional error and error_description
  • make code optional
  • return a user-friendly OAuth failure page when provider returns an error
  • return a clear error page when callback has no code

Also added/updated unit tests for both the Entra v2 resource behavior and callback error handling paths.

---

🏷️ Type of Change

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

---

🧪 Verification

Check	Command	Status
Lint suite	make lint	⚪ Not run (targeted Ruff checks passed)
Unit tests	make test	⚪ Not run (targeted OAuth unit tests passed)
Coverage ≥ 80%	make coverage	⚪ Not run

---

✅ Checklist

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

---

📓 Notes (optional)

Targeted local verification executed:

• uv run ruff check mcpgateway/services/oauth_manager.py mcpgateway/routers/oauth_router.py tests/unit/mcpgateway/services/test_oauth_manager.py tests/unit/mcpgateway/routers/test_oauth_router.py ✅
• uv run pytest -q tests/unit/mcpgateway/services/test_oauth_manager.py tests/unit/mcpgateway/routers/test_oauth_router.py ✅

[crivetimihai]

Thanks for this PR, @ankit-gajera-merck. The error handling follows RFC 6749 Section 4.1.2.1 correctly, and the Microsoft Entra v2 resource-parameter logic is well-thought-out. The test coverage is thorough. A couple of items:

XSS vector via root_path in error HTML
oauth_router.py:310,326: root_path from request.scope is injected into the <a href="{root_path}/admin#gateways"> tag without escaping. While root_path is typically set by the ASGI server (not user-controlled), a reverse proxy misconfiguration could inject malicious content. Apply escape() to root_path the same way you did for error and error_description.

_should_include_resource_parameter double-checks after caller already checks
In _create_authorization_url_with_pkce (line 1030): if self._should_include_resource_parameter(credentials, scopes) and resource: — the method already returns False when credentials.get("resource") is falsy, so the trailing and resource is redundant. Minor, but simplifying to just the method call would be cleaner.

Good: _is_microsoft_entra_v2_endpoint correctly normalizes case and checks both host and path. The omit_resource flag provides an escape hatch for non-Entra providers. Tests cover both the auto-detection and the explicit flag.

[ankit-gajera-merck]

Thanks @crivetimihai, I have addressed both items in latest commit:

1. XSS hardening for root_path in callback HTML

• Added safe_root_path = escape(str(root_path), quote=True) in oauth_callback.
• Replaced all callback-page usages of root_path in href and JS fetch(...) URLs with safe_root_path.
• This covers both the new provider-error/missing-code pages and the existing success/error callback pages.

2. Removed redundant and resource checks

• Simplified conditions in OAuthManager to use only _should_include_resource_parameter(...), since that helper already returns False when resource is missing/falsy.
• Applied in:
  • _create_authorization_url_with_pkce
  • _exchange_code_for_tokens
  • refresh_token

[jonpspri]

Hello, @ankit-gajera-merck .

To proceed with this PR, you'll need to rebase it. When I attempted a rebase, I ran into several merge conflicts. I have pushed that rebase to the PR branch.

All conflicts were in the same location: mcpgateway/services/oauth_manager.py, in the _create_authorization_url_with_pkce method, around the resource parameter handling (lines ~1033-1053).

The conflict

Three commits from the contributor's branch each touched the same if resource: block, conflicting with HEAD's version:

Commit	What it changed
b662d5427	Replaced if resource: with if self._should_include_resource_parameter(credentials, scopes):
0303e7443	Added redundant and resource guard to the above
4228bc3f5	Removed the redundant and resource (per reviewer feedback)

HEAD had already simplified the body from a redundant isinstance branch into a single assignment, since urlencode(doseq=True) handles both lists and strings.

Resolution (applied consistently each time)

# Before (various incoming versions had isinstance branching and/or `and resource`)
resource = credentials.get("resource")
if self._should_include_resource_parameter(credentials, scopes) and resource:
    if isinstance(resource, list):
        params["resource"] = resource
    else:
        params["resource"] = resource

# After (all three conflicts resolved to this)
resource = credentials.get("resource")
if self._should_include_resource_parameter(credentials, scopes):
    params["resource"] = resource  # urlencode with doseq=True handles lists

Rationale:

• _should_include_resource_parameter (from contributor): Correct guard — respects omit_resource flag and skips the param for Microsoft Entra v2 endpoints. Already used in _exchange_code_for_tokens and refresh_token.
• No and resource: Redundant — _should_include_resource_parameter already returns False when credentials.get("resource") is falsy.
• No isinstance branching: Redundant — both branches did params["resource"] = resource, and urlencode(doseq=True) handles lists natively.

For commit 0303e7443, I also removed the and resource it had added at the other two call sites (_exchange_code_for_tokens:1097 and refresh_token:1191) to keep all three consistent.

[jonpspri]

I've added an e2e testing framework for Entra-ID and some linting fixes. Pending load test, we should be GTG.

[jonpspri]

@ankit-gajera-merck I will need you to:

rebase --signoff main

and force push it to correct sign-offs on your commits.

[jonpspri]

Open Questions / Assumptions

1. mcpgateway/services/oauth_manager.py:968 assumes Entra v2 host matching on login.microsoftonline.com; national cloud hosts are likely out of scope for
   this change.
2. mcpgateway/routers/oauth_router.py:289 now surfaces provider error_description to users (escaped). This is safe from XSS, but still intentionally exposes
   provider text to the UI.

Overall Assessment

• PR quality is good, changes are internally consistent, security posture improves, and performance impact is minimal.
• Recommended to merge, with only the non-blocking coverage caveat on Entra national cloud hostname variants.

[ankit-gajera-merck]

@ankit-gajera-merck I will need you to:

rebase --signoff main

and force push it to correct sign-offs on your commits.

Hi @jonpspri , I have rebased and pushed my commits with sign-off.