Show friendly error screens for browser-facing sign-in/handshake failures - #74
Merged
Conversation
…ures Users who hit an error during the OAuth sign-in or the Internet Identity connect handshake reached these front-channel endpoints in a browser but saw a raw JSON blob for every failure except the allow-list rejection. Render an on-brand error screen instead, reusing the connect flow's self-contained shell (parchment grid, serif headline, "Hosted by" mark, light/dark theming, strict nonce'd non-scripted unframeable CSP): - Generalize assets/connect-error.html into a title/headline/detail/hint template rendered by a new `error_screen` helper; `not_allowlisted_page` now builds on it. - `/oauth/authorize` errors (bad response_type, unknown client, missing or unsupported PKCE) now content-negotiate: a browser gets the screen with a best-effort diagnostic and the line "If this error is unexpected, please contact mcp@dfinity.org to report it."; a programmatic OAuth caller keeps the RFC-style JSON error. - The pinned connect-callback page gains a contact/report line that stays hidden during a normal connect and appears once a handshake/redeem failure puts it in its error state, so every handshake error carries the contact. Rename ALLOWLIST_CONTACT to CONTACT (one address for every sign-in path). Back-channel endpoints (/oauth/token, /oauth/register, the /mcp bearer gate) stay JSON — no browser ever lands on them. Tests updated for the new content negotiation; docs note the behavior. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Pw6dN2imxtGQfpujcFFeG
There was a problem hiding this comment.
Pull request overview
This PR improves the UX of MCP sign-in/connect front-channel failures by replacing raw JSON error blobs with consistent, on-brand HTML error screens (while preserving RFC-style JSON for programmatic OAuth callers via content negotiation).
Changes:
- Generalizes
connect-error.htmlinto a reusabletitle/headline/detail/hinttemplate rendered via a newerror_screenhelper. - Adds HTML-vs-JSON content negotiation for
/oauth/authorizefailures viaaccepts_html+signin_error, keeping back-channel endpoints JSON-only. - Updates the pinned connect callback page + shared CSS to include a hidden “contact us to report it” line that appears when the page enters an error state.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| src/auth.rs | Adds generalized error-screen rendering + /oauth/authorize content negotiation and updates callback page templating/tests. |
| src/assets/connect.css | Adds styling to hide/show the callback page contact hint only in error state. |
| src/assets/connect-error.html | Converts the error page into a placeholder-driven template for error_screen. |
| src/assets/connect-callback.html | Adds a contact-hint line (mailto) that is revealed on callback error state. |
| README.md | Documents the new browser-facing error screen behavior and negotiation rules. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
The content-negotiation check was a case-sensitive substring match for "text/html" that ignored quality values, so `Accept: text/html;q=0, application/json` (an explicit "not acceptable") would still be served the HTML error screen, and `TEXT/HTML` would be missed. Parse the header into media ranges instead: match the media type case-insensitively (RFC 9110 §12.5.1) and treat an explicit `;q=0` as not acceptable (§12.4.2). Wildcards still don't opt into HTML — only an explicit, acceptable text/html does — so the machine default stays JSON. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Pw6dN2imxtGQfpujcFFeG
Make the sign-in/handshake error screens wrap more elegantly: headlines use text-wrap: balance (with a little more width) so a line never leaves a lone trailing word, and body copy uses text-wrap: pretty to avoid orphan words, with slightly roomier line-height. Both degrade gracefully where the property is unsupported. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Pw6dN2imxtGQfpujcFFeG
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated 1 comment.
Comments suppressed due to low confidence (1)
src/auth.rs:859
- The doc comment says the request "prefers" HTML, but
accepts_htmlonly checks whethertext/htmlis explicitly acceptable (it does not compare q-values vs JSON). This mismatch can mislead future readers about the intended negotiation semantics.
/// Whether the request prefers an HTML response — i.e. it comes from a browser.
/// `/oauth/authorize` is a front-channel endpoint reached by top-level navigation,
/// so a browser (which always sends `Accept: text/html`) gets the friendly
/// [`error_screen`], while a programmatic OAuth caller (`Accept: application/json`,
/// `*/*`, or no `Accept` at all) keeps the machine-readable JSON. Keying on an
…ication
Refine the browser-facing sign-in and handshake copy per request:
- No em or en dashes anywhere in the screens; long sentences are split into
short ones that wrap into balanced lines.
- Callback handshake failures now read as proper capitalized sentences
instead of lowercase fragments (they render as the on-screen headline).
- Warmer, plainer wording ("We couldn't..."); the report line now reads
"please email mcp@dfinity.org to report it".
Also address PR review feedback:
- accepts_html: the doc comment now states it is a coarse browser-vs-machine
accept check, not a full q-value preference ranking (it already honors q=0).
- authorize: a malformed or ineligible redirect_uri (unparseable, non-https,
or userinfo-bearing) is now classified invalid_request and shown the generic
sign-in error, rather than the misleading "request access" allow-list page,
which is meant only for a well-formed hosted redirect that isn't approved
yet. Adds is_wellformed_hosted_redirect and a covering test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Pw6dN2imxtGQfpujcFFeG
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 5 out of 5 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
src/auth.rs:274
is_wellformed_hosted_redirectcurrently treatshttps://localhost/.../https://127.0.0.1/.../https://[::1]/...as “well‑formed hosted”. Those redirects are not permitted byredirect_uri_permitted(loopback is http-only here), so/oauth/authorizewill misclassify them as an allow‑list approval gap and show the “request access” screen instead of aninvalid_request/generic sign-in error.
Ok(url) => {
url.scheme() == "https"
&& url.username().is_empty()
&& url.password().is_none()
&& url.host_str().is_some_and(|h| !h.is_empty())
- End the error headlines with a full stop, so every headline reads as a
complete sentence (the callback headlines already did).
- Make the allow-list request actionable: instead of asking for the redirect
URI (which a user has no way to find), ask them to tell us the name of
their MCP client or AI chatbot, which the operator can map to the vendor.
- Give the malformed-handshake message an explicit next step ("Restart from
your client."), so that screen is actionable too.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_015Pw6dN2imxtGQfpujcFFeG
- Restore "please contact mcp@dfinity.org to report it" in the error screens and the callback page, matching the README/PR wording and the original request (the copy pass had drifted it to "please email"). - Reword the machine-facing invalid_request description for a malformed redirect_uri to "redirect_uri must be a valid https or loopback URL", dropping the awkward "an https" phrasing. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Pw6dN2imxtGQfpujcFFeG
- Wrap the callback status message and its contact line in one role=status (polite, atomic) live region, so a screen reader is told the error AND the "please contact ... to report it" line together when the page enters its error state, instead of only the error. A .status flex wrapper preserves the visual spacing. - README: reference the stylesheet by its real path, src/assets/connect.css. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015Pw6dN2imxtGQfpujcFFeG
…ens-0pyxtr # Conflicts: # src/auth.rs
aterga
pushed a commit
that referenced
this pull request
Jul 23, 2026
Follow-up reconciliation after #26 (library crate), #70, #73, #74 landed on main. Verified against the current source; three concrete fixes: - Deploy: the connect/landing HTML/CSS/SVG live in `src/assets/` (compiled in via include_str! from src/main.rs and src/auth.rs), not `static/`; `static/` holds only the reference docs. The old text conflated the two and contradicted the "Browser-facing error screens" section. Reframed as the deployment binary being self-contained, with both dirs as build-time inputs. - Intro: ic-agent pin is 0.49 (Cargo.toml), not 0.48. - Tools table: `canister_query` and `canister_update_call` take an optional `candid` arg (the .did fallback when a canister publishes no candid:service metadata); added it to both rows. A wider audit found the rest of the doc already matches main (tool set, endpoints, OAuth path-pinned allow-list, env vars, II methods, library API). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym
aterga
pushed a commit
that referenced
this pull request
Jul 28, 2026
Follow-up reconciliation after #26 (library crate), #70, #73, #74 landed on main. Verified against the current source; three concrete fixes: - Deploy: the connect/landing HTML/CSS/SVG live in `src/assets/` (compiled in via include_str! from src/main.rs and src/auth.rs), not `static/`; `static/` holds only the reference docs. The old text conflated the two and contradicted the "Browser-facing error screens" section. Reframed as the deployment binary being self-contained, with both dirs as build-time inputs. - Intro: ic-agent pin is 0.49 (Cargo.toml), not 0.48. - Tools table: `canister_query` and `canister_update_call` take an optional `candid` arg (the .did fallback when a canister publishes no candid:service metadata); added it to both rows. A wider audit found the rest of the doc already matches main (tool set, endpoints, OAuth path-pinned allow-list, env vars, II methods, library API). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
When a user hit an error during the MCP OAuth sign-in or the Internet Identity connect handshake, they reached these front-channel endpoints in a browser but were shown a raw JSON blob for every failure except the allow-list rejection. This makes those errors render as a nicely formatted, on-brand error screen that always offers the line "If this error is unexpected, please contact mcp@dfinity.org to report it." plus a best-effort diagnostic, matching the existing connect flow's look.
Related issues
Changes
src/assets/connect-error.htmlbecomes atitle/headline/detail/hinttemplate rendered by a newerror_screenhelper. It reuses the connect flow's self-contained styling (parchment grid, editorial serif headline, foot-of-page "Hosted by" mark, light/dark theming) under the same strict, nonce'd, non-scripted, unframeable CSP that reflects no request value.not_allowlisted_pagenow builds on it./oauth/authorizeerrors now render friendly screens for browsers. Badresponse_type, unknown client, and missing/unsupported PKCE go through a newsignin_errorhelper that content-negotiates (accepts_html): a browser (Accept: text/html) gets the on-brand screen with a tailored diagnostic and the report-it line; a programmatic OAuth caller keeps the RFC-style JSON error. (Errors reached before a validatedredirect_urican't be redirected to the client, so they surface here.).contact-hintline that stays hidden during a normal connect and appears once the page enters its error state, so every handshake/redeem failure the user lands on shows the contact.ALLOWLIST_CONTACT→CONTACT(one address for every sign-in path) and updated the module/README docs. Back-channel endpoints (/oauth/token,/oauth/register, the/mcpbearer gate) stay JSON — no browser ever lands on them.auth.rstests for the new content negotiation, the generalized template, and the callback contact line.Screens were rendered from the real assets and eyeballed in light and dark themes (authorize error, allow-list rejection, callback error state, and the normal loading state confirming the contact line stays hidden).
Testing
cargo build --locked --all-targetscargo test --locked --all-targets(130 passed)cargo fmt --all/cargo clippy --all-targets(tree is warning-free) — repo is not rustfmt-clean and carries two pre-existing dead-code warnings unrelated to this change; new code follows the surrounding stylenpm test --prefix monitoring/mcp-status(dashboard unchanged)Checklist
🤖 Generated with Claude Code
https://claude.ai/code/session_015Pw6dN2imxtGQfpujcFFeG
Generated by Claude Code