feat(fe): switch OpenID callback to response_mode=form_post#4015
feat(fe): switch OpenID callback to response_mode=form_post#4015sea-snake wants to merge 5 commits into
Conversation
The OAuth callback previously used response_mode=fragment: the IdP
redirected back with the id_token in the URL hash, which Apple Sign In
drops name/email claims under, Okta/Auth0 handle inconsistently for
hybrid flows, and OAuth 2.1 removes entirely. The id_token also ended
up visible in the callback URL.
The IdP now POSTs {id_token, state} to /callback. The frontend canister
upgrades the POST to update mode and translates it into a certified HTML
page whose single, CSP-hash-pinned inline script delivers the payload to
the frontend: via BroadcastChannel for popup flows, via sessionStorage +
/authorize?flow=openid-resume for 1-click same-tab flows (discriminated
by the ii-openid-authorize-state marker, since in the authorize flow the
tab itself is a popup opened by the relying party). RFC 6749 error
reports ({error, error_description, state}) travel through the same page
so a misconfigured SSO app still surfaces its own message as
OAuthProviderError instead of a generic failure.
The POST arrives anonymously (the IdP submits the form), so the handler
cannot redeem the JWT: the salt + nonce + caller() binding still happens
through the existing signed-ingress flow from the frontend.
In hot-reload dev, a server hook performs the same translation since no
canister fronts the dev server; SvelteKit's CSRF origin check is disabled
as it would reject the IdP's cross-origin form POST before the hook runs
(adapter-static ships no server in production, so the check only ever
applied to the dev server). In NO_HOT_RELOAD e2e, the vite forward plugin
already routes the POST to the real canister handler.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The IdP's form_post response is answered by the canister directly, so the /callback route never renders: its GET page, the sendUrlToOpener helper and the layout's origin-redirect exclusion for the path are dead. The live popup utilities (redirectInPopup, CallbackPopupClosedError, CallbackPayload) move from the route folder into $lib/utils/openID.ts next to their only consumers. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
✅ No security or compliance issues detected. Reviewed everything up to ce54bfe. Security Overview
Detected Code Changes
|
There was a problem hiding this comment.
Pull request overview
This PR updates the Internet Identity frontend OpenID/OAuth callback handling to use response_mode=form_post instead of URL fragments, enabling stricter OIDC provider compatibility (Okta/Auth0) and avoiding id_token exposure in callback URLs. It adds a certified canister-side /callback POST translator that delivers {id_token,state} (or RFC6749 errors) to the SPA via BroadcastChannel (popup) or sessionStorage (same-tab resume), with dev-server parity via a SvelteKit hook.
Changes:
- Add a canister update-path (
http_requestupgrade →http_request_update) to translatePOST /callbackform bodies into a certified HTML landing page. - Update frontend OpenID utilities and authorize-resume flow to consume the structured callback payload (instead of parsing URL fragments).
- Adjust dev hot-reload behavior to translate
/callbackPOSTs inhooks.server.tsand disable SvelteKit CSRF origin checking to allow cross-origin IdP form POSTs in dev.
Reviewed changes
Copilot reviewed 14 out of 15 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| svelte.config.js | Disables SvelteKit CSRF origin check to allow cross-origin POST /callback in dev. |
| src/internet_identity_frontend/tests/integration/http.rs | Adds PocketIC integration coverage for callback upgrade + translation behaviors. |
| src/internet_identity_frontend/src/main.rs | Adds upgrade-to-update routing for POST /callback and introduces http_request_update. |
| src/internet_identity_frontend/src/callback.rs | New bounded parser + certified HTML landing page generator for response_mode=form_post. |
| src/internet_identity_frontend/internet_identity_frontend.did | Exposes http_request_update in the canister interface. |
| src/internet_identity_frontend/Cargo.toml | Adds form_urlencoded dependency for parsing callback form bodies. |
| src/frontend/src/routes/+layout.svelte | Removes legacy /callback exemption from client-side redirect logic. |
| src/frontend/src/routes/(new-styling)/callback/utils.ts | Deletes legacy fragment-era callback popup utilities. |
| src/frontend/src/routes/(new-styling)/callback/+page.svelte | Deletes legacy fragment-era callback page. |
| src/frontend/src/routes/(new-styling)/authorize/+page.svelte | Updates same-tab resume flow to read payload from sessionStorage and validate via extractIdTokenFromCallback. |
| src/frontend/src/lib/utils/openID.ts | Moves popup utilities here; requests form_post; validates structured callback payload. |
| src/frontend/src/lib/utils/openID.test.ts | Updates unit tests for structured payload parsing and form_post redirect URL creation. |
| src/frontend/src/hooks.server.ts | Implements dev-server stand-in for canister /callback translation. |
| src/canister_tests/src/api.rs | Adds http_request_update helper to simulate HTTP gateway upgrade behavior. |
| Cargo.lock | Records the new form_urlencoded dependency. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Replace the hand-rolled Reflect.get-based property reader and shape guard with Zod schemas, matching how the rest of the frontend validates untrusted boundary data (e.g. the ICRC channel handlers, ssoDiscovery, auth-handoff). isCallbackPayload becomes a strict union safeParse; extractIdTokenFromCallback reads fields through a lenient schema that still runs the CSRF state check first and treats any non-string field as absent. The schema accepts the JSON null the canister emits for an absent error_description and normalizes it to undefined. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Addresses Copilot review on #4015. The /callback landing page inherited the SPA-wide CSP, whose `script-src 'self' 'unsafe-inline' 'unsafe-eval'` (needed for SvelteKit and agent-js wasm) left the computed inline-script hash doing nothing unless the browser applied the CSP3 rule that a hash disables 'unsafe-inline'. Give the page its own policy instead — `default-src 'none'; script-src 'sha384-...'; base-uri 'none'; frame-ancestors 'none'` (error page: no script-src) — so the hash actually governs execution and 'unsafe-eval'/'self' are gone. The SPA-wide CSP is replaced on this response, not appended. Also narrow the 405 `Allow` header from `GET, POST` to `GET`: method_not_allowed is only reached for non-/callback resources, which serve GET only (POST is handled on /callback via the query->update upgrade). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
|
Thanks @copilot — both addressed in 50a8af1:
|
…eimplementing it The dev-server hook reimplemented the form_post → HTML translation in TypeScript, duplicating the canister's callback.rs (escaping, the flow-discriminator script, the payload shape). The frontend canister is always installed when working on OpenID, and the hook already round-trips to it to recover the injected <body> tag — so instead forward the /callback POST to the deployed canister and return its certified response. Dev now exercises the real translator; there is a single source of truth. Also migrate the CSRF escape hatch from the now-deprecated `csrf.checkOrigin: false` to `csrf.trustedOrigins: ['*']`. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
| service : (InternetIdentityFrontendInit) -> { | ||
| http_request : (request : HttpRequest) -> (HttpResponse) query; | ||
| // Handles the OAuth `response_mode=form_post` callback POSTed by the IdP | ||
| // to /callback, upgraded from `http_request` so the response is certified. |
There was a problem hiding this comment.
remove comment.
this function can be used in many ways, no need to clarify one use case in the API
| body: ByteBuf::from(b"id_token=abc.def.ghi&state=c3RhdGU".to_vec()), | ||
| certificate_version: None, | ||
| }; | ||
| let response = http_request(&env, canister_id, &request)?; |
There was a problem hiding this comment.
Why is this PR adding integration tests for a pre-existing endpoint?
| body: ByteBuf::new(), | ||
| certificate_version: None, | ||
| }; | ||
| let response = http_request(&env, canister_id, &request)?; |
| /// Upper bound on the accepted form body. The largest legitimate body is an | ||
| /// `id_token` near its own cap plus the small `state` and `code` fields; | ||
| /// anything bigger is rejected before parsing. | ||
| const MAX_BODY_BYTES: usize = 16 * 1024; |
There was a problem hiding this comment.
its own cap plus
where is this documented?
| /// CSP hash is constant; the per-request payload lives in a non-executing | ||
| /// `<script type="application/json">` data block instead. | ||
| const CALLBACK_SCRIPT: &str = r#"(function () { | ||
| var data = JSON.parse(document.getElementById("cb").textContent); |
There was a problem hiding this comment.
What is cb? Can we make this ID more verbose?
| ) -> HttpResponse<'static> { | ||
| // Take the shared security headers, then swap in this page's own CSP in | ||
| // place of the SPA-wide one (see `render_callback_landing`). | ||
| let mut headers: Vec<HeaderField> = dynamic_response_headers(vec![ |
There was a problem hiding this comment.
This function seems a bit hacky, why not add a parameter to dynamic_response_headers to control which CSP it produces rather than carving those out and substituting them here?
| /// Static page for bodies that can't be translated. `reason` is always one | ||
| /// of the fixed strings from [`parse_form_post`], never attacker-controlled. | ||
| fn render_error_page(reason: &str) -> HttpResponse<'static> { | ||
| let html = format!( |
There was a problem hiding this comment.
Could we move this into an asset file?
Could we easier to iterate on its styling in the future.
| /// escapes `"` and `\`; `<`, `>` and `&` are additionally escaped so the | ||
| /// serialized form can never contain `</script` even if a validation bug | ||
| /// lets a hostile value through. | ||
| fn payload_json(payload: &CallbackPayload) -> String { |
There was a problem hiding this comment.
This could be a serde decorator on CallbackPayload IIUC
|
|
||
| /// RFC 6749 `error` / `error_description` charset: printable ASCII except | ||
| /// `"` (0x22) and `\` (0x5C). | ||
| fn is_rfc6749_charset(value: &str) -> bool { |
There was a problem hiding this comment.
Any chance getting these predicates from a lightweight library?
There was a problem hiding this comment.
If not, we need to property-test them.
There was a problem hiding this comment.
Ditto for all other predicates defined in this file.
| /// anything able to break out of the JSON data block it is embedded in. | ||
| fn parse_form_post(body: &[u8]) -> Result<CallbackPayload, &'static str> { | ||
| if body.len() > MAX_BODY_BYTES { | ||
| return Err("form body too large"); |
There was a problem hiding this comment.
Always print the observed size and expected bounds to facilitate potential debugging.
| }); | ||
| } | ||
|
|
||
| Err("missing id_token") |
There was a problem hiding this comment.
Ideal code flow:
- Preconditions are invalid ? return Err : proceed
- Business logic failed ? return Err : proceed
- Ok(result)
| ); | ||
| } catch { | ||
| // A state mismatch, an IdP error report or a missing token falls | ||
| // back to the regular flow, matching the fragment-era behavior. |
There was a problem hiding this comment.
What is the fragment-era behavior?
| /// `response_type=code id_token` request) are ignored; for duplicate keys the | ||
| /// first occurrence wins. All accepted fields are validated against the | ||
| /// charset and length the OAuth/JWT specs allow, so the payload can't carry | ||
| /// anything able to break out of the JSON data block it is embedded in. |
There was a problem hiding this comment.
The comment is quite vague about the thread model, while implying that there is one.
Could you be more specific in the commend re. what are the bad things that we're protecting against?
Sign-in with Apple silently loses the user's name and email, and some strict OIDC providers (Okta, Auth0) reject our hybrid-flow callback (by default): both are artifacts of
response_mode=fragment, which OAuth 2.1 removes entirely and which also exposes the id_token in the callback URL. This implements Track A (§7) of the OpenID/SSO production-readiness design: the IdP now delivers the OAuth response withresponse_mode=form_postinstead.Changes
{id_token, state}to/callback; the frontend canister upgrades the POST to update mode (so the response is certified) and returns an HTML page whose single, CSP-hash-pinned inline script delivers the payload to the frontend: viaBroadcastChannelfor popup flows, via sessionStorage +/authorize?flow=openid-resumefor 1-click same-tab flows. The flows are discriminated by theii-openid-authorize-statemarker —window.openercannot tell them apart since the authorize tab is itself a popup opened by the relying party.caller()JWT redemption binding still happens through the existing signed-ingress flow.{error, error_description, state}) travel through the same page, so a misconfigured SSO app still surfaces its own message asOAuthProviderErrorinstead of a generic failure.createRedirectURLrequestsform_post;extractIdTokenFromCallbackvalidates the structured payload (state check first);resumeOpenIdreads the payload from sessionStorage.$lib/utils/openID.ts.Tests
/callbackupgrade flag, success/error translation, malformed-body rejection, security headers, 405s.createRedirectURLform_post coverage.🤖 Generated with Claude Code