Repository navigation
Comply with peer design-review outcomes (H3 Consent-Bound Completion, H2 read-only, H1 derivation, M2 drop device grant) - #32
Merged
Conversation
Implement the required changes from the IC MCP server design-review:
H3 (account takeover): bind the OAuth browser session across
/oauth/authorize -> /oauth/finish with an unguessable HttpOnly; Secure;
SameSite=Lax cookie scoped to the instance's /oauth path. finish now
requires a matching cookie before minting the code, defeating the
session-fixation takeover where an attacker phishes their own II connect
link to a victim who trusts the origin.
H2 (read-only sessions): parse the completion POST's new `permissions`
field ("queries" | "all") to learn the session access level at connect;
gate every management (update) tool with require_write, returning an
actionable "reconnect with read-only off" message instead of an opaque
IC ingress rejection; surface a read-only session in get_principal. The
genuine query tool (cycles_balance) stays ungated. Unknown level falls
back to the ingress-error path.
H1a: normalize target_origin to a bare https://<host> (strip :443, path,
query, fragment, trailing slash) before the gateway remap, so a stray
port/path no longer derives a different principal.
H1b: stop claiming exact browser-principal parity in tool descriptions,
instructions and README; describe derivation as domain-based and add the
custom derivation-origin (ii-alternative-origins) fallback hint.
M2: drop the RFC 8628 device grant entirely — routes, handlers, structs,
AS metadata grant_types_supported, and the register intersection.
Doc corrections: replace "synthetic" with "the anchor's current
(user-controllable) default", note refresh tokens as a deliberate
non-goal, and rewrite the README auth section accordingly.
Add tests for target_origin normalization, read-only gating, and the
binding-cookie extraction.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym
An adversarial review found the browser-binding cookie does not fully close the session-fixation takeover it targets: because /oauth/authorize requires no auth, the attacker can be the flow initiator and thus holds the cookie. After phishing only the II link to a victim who consents (registering the session key under the victim's anchor), the attacker completes /oauth/finish themselves with their own cookie and redeems the code as the victim. The cookie only stops the zero-click variant (the victim's browser passively delivering the code); consent happens cross-origin at II keyed by the shared state, so the server can't tie "who receives the code" to "who consented". Keep the cookie as defense-in-depth (it eliminates the zero-click variant) but correct the overstated "closes the takeover" claims in the module docs, the AuthzPending::cookie field, the authorize/finish comments, and the README to describe it accurately as a partial mitigation, with the residual risk and the note that a complete fix needs an II-side control identifying the client (not just the origin). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym
There was a problem hiding this comment.
Pull request overview
Implements the IC MCP server design-review outcomes by hardening the OAuth/II connect flow, making read-only sessions explicit/actionable, correcting principal derivation behavior/copy, and removing the unused device grant.
Changes:
- Added
/oauth/authorize→/oauth/finishbrowser-session binding via an HttpOnly/Secure/SameSite=Lax cookie; removed RFC 8628 device grant support and updated AS metadata/routes accordingly. - Introduced read-only session detection (
permissions) and gated management/update tools with an actionable “reconnect with read-only off” message; surfaced read-only state inget_principal. - Normalized
target_originderivation inputs (strip:443, path/query/fragment, etc.) and updated README/tool copy to reflect domain-based derivation and limitations.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| src/management.rs | Adds require_write gating for management/update calls (including canister_status). |
| src/main.rs | Updates tool descriptions and surfaces read-only session status in get_principal. |
| src/identities.rs | Tracks session access level (read_only) from completion permissions; adds gating helper and origin normalization. |
| src/auth.rs | Adds browser-binding cookie check for /oauth/finish; removes device grant; updates connect callback to record permissions. |
| README.md | Updates docs for domain-based derivation, browser-binding cookie (and residual risk), read-only sessions, and removal of device grant. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
set_permissions treated any non-"queries" value as full access
(Some(false)). For an unrecognized/future permissions value that
wrongly asserts "known writable" instead of leaving the level unknown,
which — once the deferred delegation-probe fallback keys on None — would
suppress the fallback and re-surface opaque ingress errors. Only accept
the known vocabulary ("queries" => read-only, "all" => full); leave the
level None (unknown) otherwise, with a warning log. Add a test.
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym
…tion)
Replace the cookie-only partial mitigation with the reviewed-and-approved
Consent-Bound Completion design, which closes the split-browser
authorization-code injection for all transports (including loopback).
/oauth/finish now mints a code only when the requesting browser proves it
is BOTH:
1. the initiator — the `sid` cookie set at /oauth/authorize; and
2. the consenter — a one-time `finish_secret` (256-bit CSPRNG) minted at
the key request and disclosed only in that response (embedded in
finish_url's query), so only the browser that drove the II handshake
holds it;
plus proven registration (a signed mcp_get_accounts returning Ok), not a
bare completion POST.
Because the single-use key request delivers both the finish_secret and the
public_key a victim would register, the party that can register as the
victim is exactly the party that gets the secret, and the sid cookie pins
the finisher to the initiator — so the two proofs co-reside in one browser
only in the legitimate same-browser flow.
Load-bearing invariants (per the design's P1–P3, P5, P6):
- The key request is a single ATOMIC compare-and-set on connect_state:
the first claims the PA (stores H(finish_secret)); any later/racing key
request 403s. Prevents the TOCTOU double-consume.
- finish_secret rides in the query (not a path segment) so path-only
access logs don't capture it; /oauth/finish and the propagation-retry
page send Referrer-Policy: no-referrer so it can't leak via Referer;
stored hashed (SHA-256), constant-time compared.
- `registered` is proven by grant_is_live (signed mcp_get_accounts), never
by the unauthenticated completion POST (now a latency hint for
expiration/permissions only).
- The propagation-retry page threads finish_secret across reloads and
consumes it only on success.
The same-browser variant (a victim driven to a hosted attacker
redirect_uri) is out of scope and needs hosted-redirect allow-listing;
loopback/native are safe either way. Documented in the module docs + README.
Add tests: secret helpers (uniqueness/entropy/constant-time), single-use
key request (first mints + claims, replay 403s, secret in query not path),
and completion-POST-does-not-materialize (P3).
Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym
aterga
changed the title
Observability (requested follow-up): count and warn-log key requests that hit a still-valid but already-consumed connect_state — i.e. a *repeat* key request. Under strict single-use (P1) these are ~zero; a sustained rise means II's frontend regressed to re-issuing the key request (which strict single-use turns into failed connects — a benign-but-breaking availability regression, never a takeover), while an isolated hit is a stray replay/probe. Surfaced as `repeat_key_requests` on /version and warn-logged with the instance name, so a silent regression is caught without log scraping. Distinguishes the consumed-repeat case from the ordinary unknown/expired 403 via a KeyClaim enum. Also addresses Copilot review of the Consent-Bound Completion commit: - The `sid` cookie's `Secure` attribute is now conditional on an https PUBLIC_URL. A `Secure` cookie is dropped by browsers over plain HTTP, so the unconditional flag broke the initiator check for local `http://localhost` development; production (https) is unchanged. - log_request's comment now notes `/oauth/finish`'s one-time `?fs=` (finish_secret) is kept out of logs by the never-log-query rule (P2). - README H2 wording corrected: a missing/unrecognized `permissions` value leaves the level unknown (not "full access") and falls through to the IC ingress-rejection fallback, matching the implementation. 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.
What & why
Implements the changes required by the IC MCP server design-review outcomes (companion to the "Design & Gotchas" doc, reviewed against II PR dfinity/internet-identity#4086). Three high-severity items needed server code; one was a decision; the rest are factual doc/copy corrections.
Changes
H3 — Close the split-browser authorization-code injection (account takeover, required)
The
statein the II connect link is echoed back to the client, so it can't prove the browser finishing/oauth/finishis the one that started/oauth/authorize— nor the one that consented at II. That enables a split-browser session-fixation takeover: an attacker (open DCR + ownredirect_uri/PKCE) initiates the flow, phishes the II link to a victim who trusts the origin, and after the victim consents, the attacker completes the flow and redeems the code as the victim.This went through two review rounds:
Implemented (Consent-Bound Completion):
/oauth/finishmints a code only when the browser proves it is both:sidcookie (HttpOnly; Secure; SameSite=Lax, scoped to the instance's…/oauthpath) set at/oauth/authorize; andfinish_secret(256-bit CSPRNG) minted at the single-use key request and disclosed only in that response (embedded infinish_url's query);plus proven registration (a signed
mcp_get_accounts=Ok), not a bare completion POST.Because the single-use key request delivers both the
finish_secretand thepublic_keya victim would register, the party that can register as the victim is exactly the party that gets the secret — andsidpins the finisher to the initiator. So the two proofs co-reside in one browser only in the legitimate same-browser flow. Closes the split-browser injection for all transports incl. loopback.Load-bearing invariants (design's P1–P3/P5/P6), all enforced + tested:
connect_stateconsume (compare-and-set): first key request claims the PA (storesH(finish_secret)); any later/racing one403s (prevents TOCTOU double-consume).finish_secretin the query (not path, so path-only logs don't capture it), stored hashed, constant-time compared;/oauth/finish+ retry page sendReferrer-Policy: no-referrer(noRefererleak).registeredproven bygrant_is_live(signed call), never by the unauthenticated completion POST (now a latency hint for expiry/permissions only).finish_secretacross reloads, consumes only on success.H2 — Handle read-only sessions (silent failure for the common user, required)
II's consent screen defaults to read-only (opt-out), giving
permissions = "queries"per-app delegations the IC rejects for update calls at ingress — making the management surface inert under the default session.permissions: "queries" | "all"(§0) to learn the level at connect; unrecognized/missing ⇒ unknown ⇒ falls through to the ingress-rejection fallback.require_write— actionable "reconnect with read-only off" instead of an opaque IC rejection. The genuine query tool (cycles_balance) stays ungated.get_principaloutput.H1 — Stop claiming exact browser-principal parity (silent wrong principal)
target_originnormalizes to a barehttps://<host>(strips:443, path, query, fragment, trailing slash) before the gateway remap.get_infoinstructions, and README describe derivation as domain-based and give the custom-derivation-origin (/.well-known/ii-alternative-origins) fallback hint.M2 — Drop the RFC 8628 device grant (decision)
Removed the routes, handlers/structs, AS metadata (
grant_types_supported=["authorization_code"]), and the register intersection.Doc corrections (low severity)
"synthetic" → "the anchor's current (user-controllable) default at the origin"; refresh tokens noted as a deliberate non-goal; README auth section rewritten.
Testing
cargo buildclean (pre-existing macro warning only);cargo test— 41/41 (network-dependentdiscover/skillstests are occasionally flaky offline; all deterministic tests stable).403s; secret in query not path), completion-POST-does-not-materialize (P3); plus H2 read-only gating incl. unknown-value fallback; H1atarget_originnormalization; binding-cookie extraction.mainafter Add /mcp-prod — a second instance against production Internet Identity (id.ai) #31's squash-merge; the diff is only this change set.Notes
§-numbered doc corrections target the standalone design doc (a separately-delivered PDF, not in-repo); equivalents are applied to the README and code comments.🤖 Generated with Claude Code
https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym