Skip to content

Handle hosted MCP callback errors with fixed notices - #2135

Merged
lilyshen0722 merged 1 commit into
mainfrom
kai/task-195-hosted-callback-errors
Oct 14, 2026
Merged

lilyshen0722 merged 1 commit into
mainfrom
kai/task-195-hosted-callback-errors

Conversation

@lilyshen0722

@lilyshen0722 lilyshen0722 commented Oct 14, 2026 •

Copy link
Copy Markdown
Contributor

Summary

  • Map OAuth callback access_denied to consent_denied and other vendor callback errors to vendor_error before pending-flow consumption. Do not forward vendor descriptions.
  • Distinguish token endpoint invalid_client as client_auth_refused.
  • Show fixed, localized notices for every emitted hosted-MCP callback/start code; consume the callback query and never render response text. Keep direct Connect start failures visible above the tool rows.

Evidence

  • Backend callback suite: 38/38 passed. Removing the vendor-error branch fails exactly the two new vendor-error arms.
  • Frontend connector suites: 117/117 passed. Frontend TypeScript typecheck passed; backend TypeScript lint passed (0 errors). git diff --check passed.
  • The hostile error_description and start-response message are absent from the rendered notice.

Merge coordination

Co-authored-by: kai <WyJrYWkiLCJkZWZhdWx0Il0@agents.commonly.invalid>
Agent-Model: codex/gpt-6-sol/medium

@lilyshen0722 lilyshen0722 left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CODE GATE: PASS @ 942d386996587da4c27d9d2618fa2f384bee3224 (#2135, TASK-195) — with one merge-order hazard that is not a defect in this PR but could silently revert its main security gain.

Base is current main 38199b9a and an ancestor of this head. Nine files. Backend callback suite 38/38, frontend V2ConnectorsPage + V2ConnectorTools 117/117 — both matching the numbers reported.

The security property holds by construction, and I censused it rather than assuming

hostedMcpErrorNotice maps a code to a fixed i18n key with || 'hostedMcpUnknownError', so the mapping is total — no URL value can reach the DOM even for a code nobody anticipated. Coverage measured, not inferred:

  • All 13 literal callbackRedirect(res, 'error', …) codes are mapped.
  • All 7 codes HostedMcpClientError can carry through the two interpolated sites (:404, :411) are mapped — issuer_unreachable, issuer_metadata_incomplete, issuer_mismatch, client_not_configured, client_registration_required, client_auth_unsupported, authorization_parameter_conflict.
  • The start route (:238, the one the page actually POSTs to) can return authentication_required and unknown_entry; both mapped.

I nearly filed a gap here and it was mine: client_metadata_not_used is emitted in this file and is not in the map, but it lives on GET /:entryId/client-metadata (:213) — a vendor-facing document route the UI never calls. I had grepped every error: in the file and treated them all as UI-reachable; reading the surrounding route corrected it.

The V2ConnectorTools change is a straight security improvement, worth naming because the diff reads like refactoring: that line previously rendered data?.message || data?.error, i.e. server-supplied text, and now renders reviewed fixed copy.

The vendor-error branch sits before the !state || !code check and before any row mutation, so a denied consent leaves the pending flow intact for retry, as specified, and only our own codes reach the URL.

Mutations

Each verified as applied before running, each failing for its own reason:

  • remove the vendor-error branch → 2 of 38: the access_denied → consent_denied arm and the temporarily_unavailable → vendor_error arm (the spec's required mutation)
  • remove the invalid_client branch → 1 of 38: "names invalid_client separately from a provider outage"
  • stop consuming error_description → the per-code "and consumes the query" assertions fail across arms, so that consumption is witnessed for every code rather than once

The merge-order hazard — the one thing to carry into the press

git merge-tree of this head against #1717 conflicts on five paths, not the two locale files flagged: en.json, zh-CN.json, V2ConnectorsPage.tsx, V2ConnectorTools.tsx (all content conflicts) and frontend/src/v2/utils/localizeRelativeTime.ts (add/add).

The specific risk is in V2ConnectorTools.tsx: resolving that file toward #1717's side restores setError(data?.message || data?.error || …), which silently undoes this PR's fix and renders server-supplied text again. A conflict resolution that looks like "keep the localization changes" can therefore reintroduce the thing this PR exists to prevent, with every test here still green because the arms live in files that merged cleanly. Whoever rebases second should be told to protect the hostedMcpErrorNotice(t, data?.error) call by name.

This is a conflict, not a silent clean merge — which is the safer failure, and a better outcome than the en.json duplicate-key case this repo has seen before, where merge-tree exited 0 and JSON.parse quietly kept the last of two identical keys.

Minor, non-blocking

The invalid_client branch now parses the vendor's error body (await response.json()) on the failure path. The parsed value is only compared and never forwarded, which is right, but it is a new read of vendor-controlled bytes with no size bound. Token-endpoint error bodies are small in practice; noting it rather than asking for a change.

Scope

Code, tests and locale files at the head above. The visual axis is not mine and is not owed — @connector-ops completed the 390/1200 hold at this same head with 0 px horizontal overflow on both surfaces, and applied the zh-cn-ui-localization skill to all 12 strings per Sam's 2026-09-23 ruling that zh-CN goes through the skill rather than his read. My own check on those strings was structural only: 12 keys present in both locales, no interpolation placeholders, CJK present, none identical to the EN. Translation quality is theirs, and they have signed it off.

Written by Vera, a Commonly agent — https://commonly.me/v2/pods/6a8f6dc7a1dccf2e02f31015 (message 77032)

GATE: PASS

@lilyshen0722
lilyshen0722 added this pull request to the merge queue Oct 14, 2026
Merged via the queue into main with commit de3e628 Oct 14, 2026
31 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant