Refresh the design doc: reconcile README with current implementation - #62
Refresh the design doc: reconcile README with current implementation#62aterga wants to merge 3 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Updates README.md as the server’s design doc to match the current main implementation, especially around the two Internet Identity connect protocols (v1 fetched-key vs Phase-2 registration delegation), operational behavior, and client usage examples.
Changes:
- Refreshes the connect/auth narrative to clearly distinguish v1 vs Phase-2 flows and their per-instance defaults (
/mcpvs/mcp-prod). - Updates operational and deployment documentation (landing page,
/versionprobe, shutdown draining, SSRF hardening, CWE-674 guard description). - Fixes and modernizes the “Try it (raw MCP over curl)” example for stateless JSON responses and bearer auth.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
0540b63 to
625b4e9
Compare
625b4e9 to
827d2d3
Compare
827d2d3 to
77a770b
Compare
77a770b to
2711165
Compare
2711165 to
443ee8a
Compare
443ee8a to
6f9bee6
Compare
6f9bee6 to
5638b19
Compare
4c093a1 to
e289cb8
Compare
e289cb8 to
e856a6e
Compare
| `open_app(name-or-URL)` is the one-call entry point when the user names or links an | ||
| app: it resolves the Internet Identity `derivation_origin` **and** discovers the | ||
| canisters behind the app together (see [Typical flow](#typical-flow)). |
There was a problem hiding this comment.
Fixed in 31ac35f. Reworded to "open_app (its app argument takes a name or a URL)", matching the argument name in the tools table.
Generated by Claude Code
| Discovery fetches are **SSRF-hardened** (CWE-918). Only `https` with a real host | ||
| is followed, and every outbound fetch runs under a redirect guard (a 3xx may only |
There was a problem hiding this comment.
Fixed in 31ac35f. Reworded to "Only https URLs with a real host are fetched, and every outbound fetch runs under a redirect guard …".
Generated by Claude Code
e856a6e to
31ac35f
Compare
31ac35f to
51daa25
Compare
51daa25 to
0d2735f
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
README.md:121
- The new prose clarifies that
open_apptakes an argument namedapp, but the “Typical flow” step still uses the call-like placeholderopen_app(name-or-URL), which suggests a different signature/argument name than the tools table.
To keep the README internally consistent, rephrase that step to refer to open_app and the app argument (name or URL) rather than open_app(name-or-URL).
`open_app` (its `app` argument takes a name **or** a URL) is the one-call entry point
when the user names or links an app: it resolves the Internet Identity
`derivation_origin` **and** discovers the
canisters behind the app together (see [Typical flow](#typical-flow)).
Rebased onto main after #64 (identity tools take derivation_origin only), reconciliation is now carried by #66, so this narrows to the non-auth improvements that main still lacks: - call_canister: document the candid? arg; state the OQL query-call rejection accurately (fires when get_canister_candid reports oql: true, not on any OQL canister). - discover: reframe open_app as the one-call entry point; note the domain argument accepts a domain or URL. - App Connect: add the SSRF-hardening note (CWE-918), scoped precisely (user-supplied fetches are address-pinned; dashboard/registry enrichment uses the redirect guard only). - OQL: define the CWE-674 guard (1 MiB / 128-depth pre-parse check over textual Candid; the OQL query path is JSON and only shares the size cap). - Creating canisters: correct the CMC recovery hint (re-notify with the same arguments; the block index alone is not enough). - Run/Deploy: self-contained binary via include_str!; OAUTH_CLIENTS_FILE is always used (default oauth-clients.json, read on startup, best-effort write), needs a writable path; graceful SIGTERM/SIGINT drain; landing page. - Try it: fix the stateless plain-JSON example (drop the SSE parsing and session-id dance, show the bearer token, label the read anonymous). - Domain identities: correct the stale custom-derivation-origin note (the app_url path resolves declared/known origins via resolve_app; only a raw derivation_origin is used verbatim). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym
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
…aging beta #92 merged into main after this branch was last rebased: it made /mcp the production Internet Identity endpoint, removed /mcp-prod, and added an opt-in /mcp-beta (beta II) for staging. The rebase brought main's new wording in cleanly, but this branch's own landing-page sentence still described the old model ("/mcp on beta II, /mcp-prod on production II"). Updated it to name the production /mcp endpoint and note staging's /mcp-beta. No /mcp-prod references remain in the README. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym
670d3ba to
4920177
Compare
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 1 out of 1 changed files in this pull request and generated no new comments.
Comments suppressed due to low confidence (1)
README.md:102
- The
candid?argument description is a bit too narrow: the implementation uses this.didtext fallback whenever the canister’scandid:servicemetadata can’t be read (e.g. access-restricted, fetch failure, or rejected by the CWE-674 guard), not only when the canister “exposes no metadata”. Rewording this avoids misleading readers about when they should supplycandid.
| `canister_query` | `canister_id`, `method?` **or** `oql?`, `args?` (textual Candid), `derivation_origin?`, `account?`, `candid?` | READ a canister — provide EITHER a Candid `query` `method` (with `args`) OR an `oql` query (a JSON object string, run against `execute`). A Candid `method` query may be anonymous or as your account and returns textual Candid; an `oql` query **requires** `derivation_origin` and returns `columns` + `rows` (a table) with `has_more`, validating `start` against the schema on an empty result. On an OQL canister a Candid `method` query is rejected — use `oql`. `candid` is a fallback: the `.did` interface text to encode/decode against when the canister exposes no `candid:service` metadata. Echoes `derived_for_origin` / `requested` / `acted_as_principal` |
| `canister_update_call` | `canister_id`, `method`, `args` (textual Candid), `derivation_origin?`, `account?`, `candid?` | Make an UPDATE (state-changing) call; reply as textual Candid; anonymous, or as your account at an app (identified by its canonical II `derivation_origin`, obtained once from `open_app`/`resolve_app`). `candid` is the same `.did` fallback as on `canister_query`, used when the interface isn't published on-chain. Echoes `derived_for_origin` / `requested` / `acted_as_principal` |
What
A holistic pass over
README.md(the server's design doc) to reconcile it with the code onmain. Docs-only change, no code touched. Rebased onto currentmain(includes #63, which switched prod to the Phase-2 default).The doc had drifted as recent work merged (#55–#63). Verified every change against the source with file:line evidence before editing.
Highest-impact fixes
/mcp-prodswitched over to match beta (Switch prod II instance to the Phase-2 handshake protocol (same as beta) #63 flippedMCP_REGISTRATION_DELEGATION_PRODto default on). Reconciled the Auth intro, the Phase-2 section (heading, per-instance bullets, the "verified against beta II" blockquote), the Production-instance note, the Status note, and the Roadmap so v1 is described as the outbound-compatible fallback, not the default for either instance. Also resolved the earlier self-contradiction over whether the II canister methods were deployed.X,registration_key, pinned callback page,/oauth/connect/redeem,mcp_register_v2).app_urlpath already resolves declared/known origins and fetchesii-alternative-origins; only a rawderivation_originis used verbatim.grep '^data: {') and themcp-session-iddance were dead. Rewrote it to parse the JSON body directly, and added the required bearer token (the endpoint is bearer-gated with no method exemption, which the old example ignored).include_str!);OAUTH_CLIENTS_FILEis the only runtime file, and it is read on startup and written as client registrations change (per Copilot's review comment).Gaps filled
SIGTERM/SIGINTdrain-then-cancel on shutdown (informs redeploy timing)./.Smaller fixes
call_canister'scandid?argument added to the tools table.sidcookie'sSecureattribute is conditional on HTTPS (documented as such).base-uri/form-action/frame-ancestorsall'none').How this was produced
Orchestrated a multi-agent pass: parallel ground-truth readers over every source module, a per-section audit of the README against that ground truth, and a whole-document consistency/completeness critic. Every asserted fact was then spot-verified directly against the code.
Note on the branch
This landed on
claude/refresh-design-docrather than the originally-designatedclaude/mcp-spec-compliance-9202ka: that branch is behindmainand its tip is the commit from the closed-unmerged PR #45. Placing a current-main-based docs refresh there would have required force-pushing over that abandoned commit, so this uses a fresh branch instead. Happy to move it if preferred.🤖 Generated with Claude Code
https://claude.ai/code/session_014T9N8USDfNK5yzznPGg7Ym