Skip to content

Fixes #32671: relay IdP OAuth error callbacks to the MCP client redirect_uri - #32735

Queued
detail-app[bot] wants to merge 4 commits into
mainfrom
detail/bug-fix/fixes-32671-relay-idp-oauth-error-callbacks-to-the-58bc20
Queued

Fixes #32671: relay IdP OAuth error callbacks to the MCP client redirect_uri#32735
detail-app[bot] wants to merge 4 commits into
mainfrom
detail/bug-fix/fixes-32671-relay-idp-oauth-error-callbacks-to-the-58bc20

Conversation

@detail-app

@detail-app detail-app Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Detail bug report: View on Detail

Describe your changes:

Fixes #32671

When an IdP returned an error callback for an MCP login (login_required, access_denied, server_error), McpCallbackServlet ran the web-SSO handleCallback against a BufferedServletResponseWrapper. The wrapper swallowed the IdP error, so the MCP client never got the error=...&state=<mcp_state> redirect to its own redirect_uri that RFC 6749 §4.1.2.1 requires, and the user got an opaque 500 Authentication Failed page instead.

Root cause: AuthCallbackServlet forwards /callback to /mcp/callback on isMcpState(state) alone, without inspecting error. On that forwarded path handleCallback's sendRedirect("/signin") and its non-silent-error throw both land on the buffered wrapper, which leaves statusCode == 200 and never sets OIDC_CREDENTIAL_PROFILE, so McpCallbackServlet falls through to credentials == null.

Type of change:

  • Bug fix

High-level design:

N/A, small change.

Change File
Short-circuit the IdP error before handleCallback and relay it to the MCP client McpCallbackServlet.doGet
New handleSSOErrorCallback: re-validate the client redirect_uri, build the OAuth error response (error, error_description, state, RFC 9207 iss), serve a page that redirects to the client callback, clean up the pending request UserSSOOAuthProvider
Canonicalize the IdP error code against RFC 6749 §4.1.2.1 + OIDC Core §3.1.2.6, returning the matched constant. Unknown codes collapse to server_error. The free-text error_description is not relayed (OPTIONAL per spec) UserSSOOAuthProvider.canonicalizeIdpErrorCode
Error page shows a generic message. No IdP-supplied string reaches it, rendered or inside the redirect URL. Raw upstream values go to the WARN log for the operator UserSSOOAuthProvider.serveErrorPage
Stop sending prompt=none on the MCP path. Silent auth is a web-SSO optimization and can only return login_required to a client that just opened a browser for the user to log in. login / consent / select_account still apply AuthenticationCodeFlowHandler.handleLogin
Record the implicit 302 on a buffered sendRedirect so statusCode is not a misleading 200 BufferedServletResponseWrapper

Tests:

Unit tests

  • I added unit tests for the new/changed logic.
  • McpCallbackServletTest (+7): IdP error routed to the relay for login_required / access_denied / null description; empty error falls through; missing pending request keeps the 400 ERR_STATE_NOT_FOUND; a success callback never hits the relay; buffered sendRedirect records 302.
  • UserSSOOAuthProviderTest (+12, new file): error redirect carries error / state / iss; state and iss omitted rather than sent as null; a known code (access_denied) is relayed verbatim and an unknown one (vendor_weird_code) as server_error; pending request deleted after the relay; unregistered redirect_uri fails safe with nothing written and no cleanup; missing pending request and missing client throw; null/blank error code throws; the IdP-supplied description appears in no form in the page, not rendered, not escaped, not percent-encoded.
  • AuthenticationCodeFlowHandlerTest (+3): prompt=none dropped for MCP, kept for web login, and prompt=consent kept for MCP.

Verified locally: 632/632 in openmetadata-mcp, 94/94 across AuthenticationCodeFlowHandlerTest / AuthCallbackServletTest / SecurityUtilTest, spotless:check clean. Reverting each fix turns the matching tests red.

Backend integration tests

  • Not applicable. No new endpoints; the MCP OAuth endpoints are covered by McpOAuthIT.

Ingestion integration tests

  • Not applicable.

Playwright (UI) tests

  • Not applicable. No UI changes.

Manual testing performed

  • Ran the affected unit tests and McpOAuthIT against a local Testcontainers stack (MySQL + Elasticsearch).
  • Brought up the dev Docker stack and confirmed the MCP OAuth endpoints are live with the fix in the running image.
  • No browser E2E for the IdP error callback: McpOAuthIT runs a basic-auth server and McpCallbackServlet returns 503 unless OIDC is configured at startup, the Playwright sso-auth specs self-skip without SSO credentials, and the bundled Keycloak fixture is SAML-only while this bug is OIDC-specific. An OIDC fixture is the follow-up.

UI screen recording / screenshots:

Not applicable.

Checklist:

  • I have read the CONTRIBUTING document.
  • My PR title is Fixes <issue-number>: <short explanation>
  • My PR is linked to a GitHub issue via Fixes #<issue-number> above.
  • I have commented on my code, particularly in hard-to-understand areas.
  • For JSON Schema changes: I updated the migration scripts or explained why it is not needed.
  • For UI changes: I attached a screen recording and/or screenshots above.
  • I have added tests (unit / integration / Playwright as applicable) and listed them above.
  • I have added a test that covers the exact scenario we are fixing.

Automatic Fixes PRs can be configured here.

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

Hi there 👋 Thanks for your contribution!

The OpenMetadata team will review the PR shortly! Once it has been labeled as safe to test, the CI workflows
will start executing and we'll be able to make sure everything is working as expected.

Let us know if you need any help!

@github-actions

github-actions Bot commented Sep 6, 2026

Copy link
Copy Markdown
Contributor

✅ Playwright Results — workflow succeeded

Validated commit acfec64ee7c078725b323692c5aa182163391631 in Playwright run 34196807553, attempt 3.

✅ 4476 passed · ❌ 0 failed · 🟡 7 flaky · ⏭️ 1 skipped · 🧰 0 lifecycle flaky

Performance

Blocking targets: ✅ met · Optimization targets: 🟡 in progress

Shard-job maxima below are not the full workflow wall time; the linked run includes build, fixture, planning, and reporting.

🕒 Full workflow signal wall (to summary) 3h 13m 0s

⏱️ Max setup 4m 20s · max shard execution 21m 49s · max shard-job elapsed before upload 25m 49s · reporting 22s

🌐 217.03 requests/attempt · 2.31 app boots/UI scenario · 41.48% common-shard skew

Optimization targets still in progress:

  • Common shard skew was 41.48% (convergence target: at most 15%).
  • Browser traffic was 217.03 requests per attempt (convergence target: fewer than 200).
  • Application boot ratio was 2.31 per UI scenario (10928 boots / 4730 scenarios; convergence target: at most 1).
Shard Passed Failed Flaky Skipped Lifecycle failed Lifecycle flaky
✅ Shard advanced-search-01 130 0 0 0 0 0
✅ Shard chromium-01 163 0 0 0 0 0
✅ Shard chromium-02 127 0 0 0 0 0
✅ Shard chromium-03 141 0 0 0 0 0
✅ Shard chromium-04 144 0 0 0 0 0
✅ Shard chromium-05 172 0 0 0 0 0
✅ Shard chromium-06 191 0 0 0 0 0
✅ Shard chromium-07 148 0 0 0 0 0
✅ Shard chromium-08 166 0 0 0 0 0
🟡 Shard chromium-09 202 0 1 0 0 0
🟡 Shard chromium-10 199 0 1 0 0 0
🟡 Shard chromium-11 156 0 1 0 0 0
✅ Shard chromium-12 174 0 0 0 0 0
✅ Shard chromium-13 164 0 0 0 0 0
🟡 Shard chromium-14 159 0 2 1 0 0
✅ Shard chromium-15 145 0 0 0 0 0
✅ Shard chromium-16 167 0 0 0 0 0
✅ Shard chromium-17 177 0 0 0 0 0
🟡 Shard chromium-18 136 0 1 0 0 0
✅ Shard chromium-19 153 0 0 0 0 0
✅ Shard chromium-20 152 0 0 0 0 0
✅ Shard chromium-21 166 0 0 0 0 0
✅ Shard chromium-22 175 0 0 0 0 0
✅ Shard chromium-23 189 0 0 0 0 0
✅ Shard chromium-24 161 0 0 0 0 0
✅ Shard data-asset-rules-01 65 0 0 0 0 0
✅ Shard domain-isolation-01 16 0 0 0 0 0
✅ Shard global-state-01 34 0 0 0 0 0
✅ Shard import-export-01 46 0 0 0 0 0
✅ Shard import-export-02 104 0 0 0 0 0
✅ Shard ingestion-01 43 0 0 0 0 0
🟡 Shard ingestion-02 42 0 1 0 0 0
✅ Shard reindex-01 28 0 0 0 0 0
✅ Shard search-01 12 0 0 0 0 0
✅ Shard search-rbac-01 29 0 0 0 0 0
🟡 7 flaky test(s) (passed on retry)
  • Pages/EditClassification.spec.tsEdit a user classification from the manage button (shard chromium-09, 1 retry)
  • Features/Glossary/GlossaryMiscOperations.spec.tsshould delete glossary and remove tags from assets (shard chromium-10, 1 retry)
  • Pages/TasksUIFlow.spec.tsCreate and resolve description task for Pipeline via UI (shard chromium-11, 1 retry)
  • Pages/Entity.spec.tsAnnouncement create, edit & delete (shard chromium-14, 1 retry)
  • VersionPages/ServiceEntityVersionPage.spec.tsStorage Service (shard chromium-14, 1 retry)
  • Pages/Lineage/DataAssetLineage.spec.tsColumn lineage for mlModel -> apiEndpoint (shard chromium-18, 1 retry)
  • Pages/DataContracts.spec.tsCreate Data Contract and validate for Table (shard ingestion-02, 1 retry)

📦 Download artifacts

How to debug locally
# Download playwright-test-results-<shard> artifact and unzip
npx playwright show-trace path/to/trace.zip    # view trace

@Vishnuujain
Vishnuujain requested a review from a team as a code owner September 7, 2026 16:04
@Vishnuujain Vishnuujain self-assigned this Sep 7, 2026
@mohityadav766
mohityadav766 added this pull request to the merge queue Sep 8, 2026
Any commits made after this event will not be merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

safe to test Add this label to run secure Github workflows on PRs

Projects

None yet

Development

Successfully merging this pull request may close these issues.

[Detail Bug] MCP OAuth: IdP error callbacks return opaque 500 instead of OAuth error redirect

3 participants