Gate every canister-reaching tool on the discoverability manifest by aterga · Pull Request #184 · dfinity/imcp2

aterga · 2026-09-02T13:58:31Z

Summary

Reads were open on any canister; only writes were gated. They now run the same gate: get_canister_candid, get_canister_api_doc, get_canister_oql_schema and canister_query (both paths) reach a canister only when the app that owns it declares that canister at /.well-known/ic-architecture — exactly as canister_update_call already did.

The arguments are the ones canister_update_call takes: an optional app_url naming the app whose manifest decides, falling back to derivation_origin when the app serves its manifest there, refusing when neither is present. Where a call carries both, the identity binding holds them to the same app — which matters for an authenticated read as much as for a write, since a read runs as the user's principal at whatever app the caller named.

Two consequences worth flagging for review, both stated in the module's scope notes rather than left implicit:

On reads this is stricter than the protocol asks for. The guide frames discoverability as an aid to reading, and reading the IC is open to anyone with an ordinary agent, so gating reads withholds nothing from an attacker — it is a statement about whose canisters this connector will read on a user's behalf.
Discovery pays for it. A candidate mined from an app's /env.json or JS bundle can no longer be confirmed by reading its interface, so a manifest is now the only route from an app to a usable canister.

There is no manifest cache (deliberate, per the same behaviour on the write path): a read now carries a DNS resolve, TLS handshake and two HTTP GETs ahead of the boundary-node call. If that latency shows up in practice, a TTL cache in discoverability is the follow-up.

Related issues

None.

Changes

discoverability: authorize_update_call → authorize_call, taking a new CallKind so each refusal names the operation actually attempted (an agent told its read was refused as "a state-changing call" relays something false). bind_identity takes it too. the_rule() stays kind-neutral — a caller who hits it with one operation is told the whole policy.
The sentence promising that reading is unaffected is gone (it would now be false). What keeps a refusal from reading as "this app is off limits" is a new sentence naming what still works: open_app resolves the app and lists what it declares.
tools: one helper, authorize_canister_call, performs the whole check (origin selection + manifest check + identity binding, the last two concurrently) for all five tools, so a read and a write cannot drift into checking different things. canister_update_call moves onto it and loses its inline copy.
calls: app_url on the four read arg types; declared_by / declared_at on their outputs. The two anonymous metadata reads also accept derivation_origin as an origin fallback only — they sign as nobody, so they skip the binding.
open_app probes only declared canisters for their oql / api_doc_available flags; an undeclared candidate keeps null flags rather than being probed. Declared canisters are marked [declared] in the listing, and the returned handle now carries both derivation_origin and app_url.
Server instructions rewritten (WRITES → DECLARED CANISTERS); README intro, tools table, typical flow, and the gate section updated.
Tests: the read-path-stays-open test is replaced by two — every refusal points at what still works, and every refusal names the operation it refused (checked for both kinds). The tool-surface test now pins app_url on all five canister-reaching tools and its absence everywhere else, so adding a tool without wiring the gate fails there.

Follow-up commit (`71b2bf7`) — two review findings, both real

The [declared] marker over-claimed. It read the ic-architecture provenance, which is not the authorization set: canisters_from_app_manifest keeps an id as spelled and drops blanks before the 100-entry cap, while manifest_canister_ids caps first, then parses and keeps only canister principals. So open_app could mark and capability-probe aaaaa-aa, the anonymous principal, or a real id past the cap — all refused by every gated tool. Found / DiscoveredCanister now carry a declared field set from the gate's own parse of the same document, and is_declared reads that.
The OQL examples dropped app_url. The schema reply's ready-to-run canister_query lines carried derivation_origin + account but not the origin that authorized the read, so copying one fell back to the derivation origin and got refused — for the 13 of 17 registry apps whose website and derivation origin differ, i.e. exactly the case the fallback was meant to cover. oql_query_examples now takes and emits it.

Testing

cargo build --locked --all-targets
cargo test --locked --all-targets — 267 pass, 0 fail
cargo fmt --all / cargo clippy --all-targets (tree is warning-free) — clippy introduces no new warnings (one too_many_arguments on canister_oql_query is silenced the way its sibling already was); cargo fmt is not run, since the tree is not rustfmt-formatted today (587 diffs on main) and reformatting would bury this diff
npm test --prefix monitoring/mcp-status (dashboard unchanged)

Checklist

I have read the Contributing guidelines.
Docs (README / comments) updated for any user-visible change.
No secrets, credentials, or internal-only information are included.

One pre-existing wart this diff makes more visible, left alone rather than widened into: when the origin came from app_url, not_declared_refusal ends "pass that app's URL as app_url instead of app_url". It predates this change and is a one-line fix in source.arg() handling.

🤖 Generated with Claude Code

https://claude.ai/code/session_015pKeV5VZvkfEgFR8RX9Niy

Reads were open on any canister; only writes were gated. They now run the same gate: get_canister_candid, get_canister_api_doc, get_canister_oql_schema and canister_query reach a canister only when the app that owns it declares that canister at /.well-known/ic-architecture, exactly as canister_update_call already did. The arguments are the ones canister_update_call takes. Each read grows an optional `app_url` naming the app whose manifest decides, falling back to `derivation_origin` when the app serves its manifest there; where a call carries both, the identity binding holds them to the same app, which matters for an authenticated read as much as for a write, since a read runs as the user's principal at whatever app they named. The two anonymous metadata reads (get_canister_candid, get_canister_api_doc) accept `derivation_origin` as an origin fallback only, and skip the binding because they sign as nobody. Every successful reply echoes `declared_by`/`declared_at`. One helper, authorize_canister_call, now performs the whole check for all five tools, so a read and a write cannot drift into checking different things; canister_update_call moves onto it and loses its inline copy. Refusals carry a CallKind so each names the operation actually attempted — an agent told its read was refused as "a state-changing call" relays something false — while the rule sentence stays kind-neutral, since a caller who hits it with one operation should be told the whole policy. The sentence promising that reading is unaffected is gone (it would now be false); what keeps a refusal from reading as "this app is off limits" is naming what still works: open_app resolves the app and lists what it declares. Discovery pays for this. open_app now probes only declared canisters for their oql/api-doc flags, because probing an undeclared one would hand back as a capability flag exactly what get_canister_candid would refuse to return; a candidate mined from /env.json or the JS bundle keeps null flags and is marked by the absence of [declared] in the listing. A manifest is now the only route from an app to a usable canister. That cost, and the fact that gating reads is stricter than the protocol asks for, are stated in the module's scope notes rather than left for a reader to discover. There is no manifest cache, so the fetch is paid per call: a read now carries a DNS resolve, a TLS handshake and two HTTP GETs ahead of the boundary-node call. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pKeV5VZvkfEgFR8RX9Niy

Copilot

🟡 Changes recommended

Discovery can mislabel unauthorized IDs, and generated OQL examples omit the required app URL.

Once you've addressed the issues Copilot identified, you can request another Copilot review.

Pull request overview

Extends manifest authorization from writes to all canister-reading tools.

Changes:

Adds shared read/write authorization and identity binding.
Adds declaration provenance to outputs and discovery listings.
Updates documentation and refusal messaging.

File summaries

File	Description
`README.md`	Documents manifest-gated reads and writes.
`crates/imcp2-core/src/tools.rs`	Applies authorization across canister tools.
`crates/imcp2-core/src/discoverability.rs`	Generalizes the gate and refusal messages.
`crates/imcp2-core/src/discover.rs`	Identifies declared discovery results.
`crates/imcp2-core/src/calls.rs`	Adds authorization arguments and provenance outputs.

Review details

Files reviewed: 5/5 changed files
Comments generated: 2
Review effort level: Balanced

💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.

…url into the OQL examples Two findings from review, both real. The `[declared]` marker read the `ic-architecture` provenance, which is not the authorization set. The two manifest parsers deliberately disagree: `canisters_from_app_manifest` keeps an id as spelled, drops blanks BEFORE the 100-entry cap, and never checks what kind of principal it is, while `manifest_canister_ids` caps first, parses, and keeps only canister principals. So open_app could mark — and capability-probe — an id every gated tool then refuses: `aaaaa-aa`, the anonymous principal, or a real id sitting past the cap behind blank entries. The marker now comes from the gate's own parse of the same document, carried on `Found`/ `DiscoveredCanister` as `declared` and surfaced in the structured output, so it cannot over-claim. A test runs both parsers over one body that exercises both divergences. The OQL schema reply's ready-to-run examples carried `derivation_origin` and `account` but not the `app_url` that authorized the schema read, so copying one fell back to checking the manifest at the derivation origin — which fails for the 13 of 17 registry apps whose website and derivation origin differ, exactly the case the fallback was meant to cover. The examples now carry both. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015pKeV5VZvkfEgFR8RX9Niy

Copilot

🔵 Needs a closer look

The central enforcement behavior lacks regression coverage, and the documented authenticated-read request cost is inaccurate.

Review details

Suppressed comments (2)

Previously missed (2) — in code that hasn't changed since the last review.

crates/imcp2-core/src/tools.rs:2614

This test only proves that each input schema contains an app_url property; it does not verify that the implementation invokes authorize_canister_call, or that authorization happens before target-canister I/O. Removing the gate from any of these five handlers would still pass, so the PR's central enforcement behavior lacks a regression test. Add an injected/mock authorization or agent seam and assert each path refuses before any target call.
README.md:390
This understates authenticated-read latency. When both app_url and derivation_origin are supplied, authorize_canister_call also runs bind_identity; resolve_app_identity performs two additional well-known GETs and may also fetch ii-alternative-origins or the origin root. Only anonymous reads (or reads using the derivation-origin fallback) pay just the two manifest GETs, so document the two paths separately.

Files reviewed: 5/5 changed files
Comments generated: 0 new
Review effort level: Balanced

imcp2 64c48be ("Gate every canister-reaching tool on the discoverability manifest", dfinity/imcp2#184) restricted every tool that reaches a canister to canisters a manifest declares: get_canister_candid, get_canister_api_doc, get_canister_oql_schema, canister_query on both its paths, and canister_update_call. Reads and writes now share one mechanism and differ only in what a refusal says. The caution said the opposite: that publication was the additional condition for state-changing calls and that ICP MCP did not require a manifest for metadata retrieval or query calls. Both were accurate when written. They are not now, so an operator reading this page would expect their undeclared canisters to stay readable. It now says publication is the condition for every call that reaches a canister, and draws the line where the code draws it: an app that publishes no manifest is still resolved and described, because that surface reaches no canister, but none of its canisters is read or called. Removal stops new calls of every kind. Matches the App Operator Terms sections 3, 4 and 7 as revised in dfinity/internetcomputer-org#97. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01Xpg83AWtkQo34jbMxpX8WB

A minor bump rather than a patch, because the served tool surface changed incompatibly since 0.3.0. Every tool that reaches a canister is now gated on the owning app's service-discoverability manifest (#184). `get_canister_candid`, `get_canister_api_doc`, `get_canister_oql_schema` and `canister_query` used to work on any canister; they reach one only when the app that owns it declares that canister at `/.well-known/ic-architecture`, exactly as `canister_update_call` already did. Each of the four grew an optional `app_url` naming the app whose manifest decides, and every successful reply echoes `declared_by`/`declared_at`. A client that read an undeclared canister against 0.3.0 gets a refusal here. `open_app` moved with it: it probes only declared canisters for their oql/api-doc flags, so a candidate mined from `/env.json` or the JS bundle comes back with null flags and without the `[declared]` marker, and the server instructions no longer promise that reading is ungated. No public Rust item changed — the gate lives in the private `discover` and `discoverability` modules — so an embedder still compiles against 0.4.0 unchanged. What changed is what its server answers, which is the part its own users see. Under cargo's 0.x rules `0.3` is the compatibility range, so shipping this as 0.3.1 would hand that break to everyone on `imcp2 = "0.3"` — the requirement our own README recommends — the next time they ran `cargo update`. 0.4.0 opens a new range instead; existing users stay on 0.3.x until they choose to move. The README's dependency line follows, as `"0.3"` would no longer resolve to a current release. The other two changes since 0.3.0 are the status dashboard's landing-page check (#181) and the operator-facing terms references (#182); neither touches the crate. `imcp2-core` moves in lockstep: publish-crate.yml checks the tag against both manifests, and the workspace pin that binds them (`imcp2-core = { path = …, version = … }`) has to name the same number or the path dependency stops resolving. `imcp2-local` moves with them — it releases on its own `imcp2-local-…` version tags and has never been cut, so there is nothing to keep it behind, and its `--version` output would otherwise report 0.3.0 from a 0.4.0 tree. Co-authored-by: Claude Opus 5 (1M context) <noreply@anthropic.com>

@sea-snake

## Summary Follow-up to #376. That PR correctly retargeted the opt-in caution from the old single terms of service to the [App Operator Terms](https://internetcomputer.org/icp-mcp/app-operator-terms/), but it inherited the ambiguity those Terms carried: it presented registration as the acceptance step while also treating manifest publication as acceptance by conduct, so a reader could not tell what they must do before their app can be acted on. dfinity/internetcomputer-org#96 resolved that in the Terms themselves. This states the same model here, which matters because the Terms point at this page for how acceptance happens (and say that where the two differ, the Terms prevail). ## What the caution now says - **Publishing the Layer 1 manifest at `/.well-known/ic-architecture` opts the app in and constitutes the operator's acceptance** of the App Operator Terms. It is sufficient to make the app's declared canisters eligible for everything ICP MCP does with them, queries and state-changing calls alike; no separate registration is required. - **Publication is the condition ICP MCP applies to every call that reaches a canister.** It reads and calls only canisters a manifest declares, so an app that publishes none is still resolved and described, but none of its canisters is read or called. Removing the manifest stops new calls of every kind. - **Registration is optional** and is not a condition of participation. It is how an operator tells DFINITY who they are, so they can be reached with the notices the terms provide for, and so there is a record of who accepted and which version; it can carry the URL of the app's **own** privacy policy so ICP MCP can present it to users. - The privacy sentence describes what the [ICP MCP Privacy Policy](https://internetcomputer.org/icp-mcp/privacy-policy/) actually covers (what is disclosed to a participating app, and what registration processes), and states that the operator remains responsible for handling that data lawfully and for keeping the app's own privacy notice accurate — rather than asking operators to "comply with" a notice that describes DFINITY's processing and expressly does not govern theirs. - The **caution title** now names the trigger ("Publishing the Layer 1 manifest opts your app in"), which review caught contradicting the body: the old title read as though any layer opts an app in, while the body pins opt-in to Layer 1 and notes the layers are otherwise independently adoptable. The five-layer content below the caution is unchanged. ## Revision history This PR went through three models of what publication gates, because the code moved under it: 1. An early revision gated state-changing calls on DFINITY confirming the operator's registration. That was the wrong branch of the two the review offered; publication really was sufficient, so registration became optional instead. 2. Two review suggestions then narrowed what publication gates to **state-changing calls only**, on the correct ground that `crates/imcp2-core/src/discoverability.rs` gated writes only and its `READS_ARE_FINE` refusal text told agents that reads worked regardless. Both were applied verbatim in `60a73a0` and `8ec933f`. 3. **`e4ce4be`** undoes that narrowing, because dfinity/imcp2#184 (`64c48be`, "Gate every canister-reaching tool on the discoverability manifest") has since made reads gated the same way. `get_canister_candid`, `get_canister_api_doc`, `get_canister_oql_schema`, `canister_query` on both paths and `canister_update_call` are all restricted to declared canisters; per the module docs, "Reads and writes share one mechanism and differ only in what a refusal SAYS", `the_rule()` now says the server "**reads and calls** ONLY canisters an app declares", and `READS_ARE_FINE` is gone. So the two suggested sentences were accurate when written and are not now. Step 3 keeps their substance where it still holds (publication is sufficient, registration is optional) and moves the line to where the code now draws it: not reads versus writes, but **calls that reach a canister versus the surface that does not**. `open_app`, the static guides and the identity tools reach no canister and stay ungated, which is why the caution still says an app publishing no manifest is resolved and described. @sea-snake, your approval was on `8ec933f`, so this last commit is newer than what you read. ## Verification - `npm run validate` — **210 files, all checks passed** on each revision, including `e4ce4be` (the repo's no-em-dash rule caught new prose in the first draft, fixed before pushing). - Rendered preview checked, not just the source, on the earlier revisions: the caution reads as intended and the page links only `app-operator-terms/` and `privacy-policy/`, with no `/icp-mcp/terms/` link that would send operators to the User Terms. - The claims about the gate are quoted from `crates/imcp2-core/src/discoverability.rs` at imcp2 `main` (`64c48be`), not from memory. ## Companion dfinity/internetcomputer-org#97 makes the matching correction to App Operator Terms §3, §4 and §7. Merge that with or before this, since the Terms are the authority this page defers to. 🤖 Generated with [Claude Code](https://claude.com/claude-code) https://claude.ai/code/session_01Xpg83AWtkQo34jbMxpX8WB --------- Co-authored-by: Claude <noreply@anthropic.com>

aterga requested a balanced review from Copilot September 2, 2026 14:44

Copilot started reviewing on behalf of aterga September 2, 2026 14:45 View session

Copilot AI reviewed Sep 2, 2026

View reviewed changes

Comment thread crates/imcp2-core/src/discover.rs Outdated

Comment thread crates/imcp2-core/src/tools.rs

aterga marked this pull request as ready for review September 2, 2026 14:59

aterga requested review from a team and Copilot September 2, 2026 14:59

Copilot started reviewing on behalf of aterga September 2, 2026 14:59 View session

sea-snake approved these changes Sep 2, 2026

View reviewed changes

Copilot AI reviewed Sep 2, 2026

View reviewed changes

aterga merged commit 64c48be into main Sep 2, 2026
13 checks passed

sea-snake mentioned this pull request Sep 2, 2026

Bump to 0.4.0 #185

Merged

aterga mentioned this pull request Sep 2, 2026

Test the canister tools end to end, over MCP against a real replica #186

Merged

9 tasks

aterga mentioned this pull request Sep 2, 2026

docs: clarify manifest opt-in and optional registration dfinity/developer-docs#377

Merged

Navigation Menu

Uh oh!

Gate every canister-reaching tool on the discoverability manifest - #184

Gate every canister-reaching tool on the discoverability manifest#184
aterga merged 2 commits into
mainfrom
claude/icp-mcp-canister-discovery-6m9d4i

aterga commented Sep 2, 2026 •

edited

Loading

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

aterga commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Related issues

Changes

Follow-up commit (71b2bf7) — two review findings, both real

Testing

Checklist

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🟡 Changes recommended

Uh oh!

Uh oh!

Uh oh!

Copilot AI left a comment

Choose a reason for hiding this comment

🔵 Needs a closer look

Suppressed comments (2)

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

aterga commented Sep 2, 2026 •

edited

Loading

Follow-up commit (`71b2bf7`) — two review findings, both real