Skip to content

Refuse update calls whose method name carries a financial keyword - #214

Merged
aterga merged 11 commits into
mainfrom
claude/guard-financial-keywords-69frs8
Oct 28, 2026
Merged

aterga merged 11 commits into
mainfrom
claude/guard-financial-keywords-69frs8

Conversation

@aterga

@aterga aterga commented Oct 28, 2026 •

Copy link
Copy Markdown
Collaborator

Summary

canister_update_call now refuses, before any network work and on every canister, a call whose method name contains a financial / crypto keyword as a whole word: the maintainer's list (trade, swap, transfer, withdraw, exchange, pay, buy) plus the value verbs (deposit, purchase, sell, mint, burn, stake, unstake, redeem, refund, liquidity, allowance, escrow, loan, lend, borrow, bridge), modulo a literal list of exempt control-handover names (transfer_ownership and its camelCase spelling, each with its source in a comment). This reverses the #154 review decision that left such generic names unmatched on ordinary canisters; the module doc records the reversal and what stays unlisted on purpose (approve, send, claim, …, and derived forms such as payment).

Plan 2 of 2, on top of #213 (merged). The branch carries #213's history from when it was stacked; the diff against main is this change alone, and the last merge commit resolves the conflicts with the squash of #213 while keeping main's own changes since the branch point (#212).

Related issues

Follows #154, #163 and #213.

Changes

  • compliance.rs: FINANCIAL_KEYWORDS, the literal EXEMPT_METHODS (each entry with its source), name_words (split on _, -, digits and camelCase humps, lowercase) and financial_keyword; the keyword scope runs last of the request-only scopes so the sharper registers keep precedence (transfer on the ICP ledger still says the call itself carries out the operation; swap on a listed exchange still names the service). Its own register says only what the guard knows: the keyword is in the name, refused on every canister rather than judged app by app, so this refusal does not mean this particular call moves assets. Module doc, function doc and the system-canister table doc updated; the Sonic DIP-20 comment corrected.
  • Tests: word splitting in every spelling convention, whole-word matching (swapTokens, withdraw_all, Transfer, withdraw2 match; payload, buyer, swapped, trader do not), the literal exemptions, every keyword's refusal and register, precedence pins, list well-formedness, and that the standard names stay literal (a re-spelled icrc1_transfer is refused by keyword, never with the standard's label). allows_non_financial_methods and the system-canister scoping test are reworked for the new rule.
  • e2e (PocketIC): withdraw on the canister that publishes no interface is refused by name, before the interface is read, with the keyword scope's own wording.
  • tools.rs call-site comment; README intro and tool row; both directory submission docs. Server instructions unchanged.

Accepted false positives, chosen by the maintainer and recorded in the module doc: deposit_count, get_exchange_rate, buy_ticket, transfer_fee (as an update call) are refused by name.

Testing

  • cargo build --locked --workspace --all-targets
  • cargo test --locked --workspace --all-targets (71 + 8 + 10 + 203 + 21)
  • cargo fmt --all -- --check / cargo clippy --all-targets (9 pre-existing warnings, none new, with and without --features e2e)
  • POCKET_IC_BIN=… cargo test --locked -p imcp2-core --features e2e e2e_canister_tools -- --nocapture (4 passed against PocketIC 15.0.0)
  • 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.

Generated by Claude Code

The financial-transactions guard decided everything from the method name
and the canister id: the exact ICRC transfer/approval names and
manage_neuron on every canister, a few names on the system ledgers, and
every update on a curated list of finance canisters. A ledger that is
not on that list - an SNS or chain-key token, a new ledger built on the
ic-icrc1 code - was reachable through any method the list did not name,
and the list can never stay complete.

The guard now reads the canister itself. Once canister_update_call has
the canister's Candid interface in hand - the published candid:service,
else the caller-supplied text, which resolve_did consults only when the
canister publishes none, so it can only add a refusal - a canister whose
interface declares ANY icrc1_* / icrc2_* method is a financial canister,
and every update call to it is refused before the call is encoded or
signed. That covers every ICRC ledger and the ICP ledger with no list to
keep current. The check runs where the interface is legitimately in
hand: after the discoverability gate (reading an interface is a contact
with the canister, so an undeclared ledger is refused as undeclared) and
on the read the tool already makes for encoding, so it costs no extra
round-trip. No interface, or one the parser cannot read, fails open: the
guard remains a guardrail, and the stated policy covers the rest.

A call addressed to any other icrc1_* / icrc2_* method by name is also
refused, before any network work: the caller is treating the canister as
a ledger. The prefixes are literal, underscore included, so ICRC-10 and
ICRC-21 names and other token namespaces are untouched.

Each refusal keeps the register rule - it says only what the guard
knows. The interface refusal reuses the listed-canister destination
register (every state-changing call to that service is refused, which
says nothing about this call), naming the standard(s) and the methods
that established them; the by-name refusal disclaims the call too. The
sharper registers keep precedence: icrc1_transfer still says the call
itself carries out the operation, and a listed service still names the
service.

Tests: calls::declared_methods reads a bare service, a service
constructor (the form every real ledger publishes), an alias, and fails
open on unreadable or over-limit text (alias-chain bomb included); the
compliance tests pin the detection, the literal prefixes, the registers
and their precedence, and the plain e2e fixture shape. The e2e suite
gains a declared ledger fixture: icrc1_transfer on it is refused before
the network, set_message on it is refused from its interface with the
store unchanged, each with a positive control on the replica; reads of
it still work; an update to the canister that publishes no interface
goes through, and goes through no longer once a ledger interface is
supplied for it. README, both directory submission docs, and the module
and call-site docs describe the new scope; the server instructions are
unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VP2vTGhAZfkTv1hwmKERA3
Generic value-moving names - transfer, withdraw, swap - were allowed on
every canister the guard did not list by id: review on #154 found them
too abstract to refuse everywhere without breaking non-financial apps,
and the module doc recorded that the stated policy covered them. The
maintainer reversed that decision: refusing a real ledger's, pool's or
DIP20 token's method is worth more than allowing an app's non-financial
one.

canister_update_call now refuses, before any network work and on every
canister, a call whose method name contains a financial / crypto keyword
- the maintainer's list (trade, swap, transfer, withdraw, exchange, pay,
buy) plus the value verbs (deposit, purchase, sell, mint, burn, stake,
unstake, redeem, refund, liquidity, allowance, escrow, loan, lend,
borrow, bridge). Matching is by whole word: the name is split on `_`,
`-`, digits and camelCase humps and lowercased, so swapTokens,
withdraw_all, icrc1_transfer and Transfer match while payload, buyer and
swapped do not - a substring test would have refused every set_payload.
A literal list of control-handover names (transfer_ownership and its
spellings) is exempt, by exact bytes. Derived forms such as payment or
withdrawal are deliberately unlisted for now, as are approve, send,
claim and the other ordinary app verbs; each is one line to add.

The scope runs last of the request-only scopes, so the sharper registers
keep precedence: transfer on the ICP ledger still says the call itself
carries out the operation, and swap on a listed exchange still names the
service. Its own register says only what the guard knows - the keyword
is in the name, refused on every canister rather than judged app by app,
so this refusal does not mean this particular call moves assets - and
the accepted false positives (deposit_count, get_exchange_rate,
transfer_fee as an update) are recorded in the module doc along with the
reversal itself.

Tests pin the word splitting, whole-word matching in every spelling
convention, the literal exemptions, every keyword's refusal and
register, precedence over the listed-canister and ledger-scoped
registers, the well-formedness of both lists, and that the standard
names stay literal (a re-spelled icrc1_transfer is refused by keyword,
never with the standard's label). The e2e suite shows withdraw refused
by name on a canister that publishes no interface. README, both
directory submission docs, and the module and call-site docs describe
the scope; the server instructions are unchanged.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VP2vTGhAZfkTv1hwmKERA3

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🟢 Approval recommended

No active review issues remain for the expanded financial-transactions guard.

No open issues 🎉

What changed in this PR

Extends the existing financial-transactions guard to refuse update calls whose method names contain whole-word financial keywords, before any network work and on every canister.

Changes:

  • Adds keyword matching across naming conventions, literal control-handover exemptions, and refusal wording that preserves existing rule precedence.
  • Adds unit and PocketIC coverage for matching, exemptions, refusal messages, and precedence.
  • Updates public documentation and guard comments to describe the expanded policy.
File Description
README.md Documents keyword-based refusals and exemptions.
docs/​openai-directory-submission.md Updates the submission’s guard description.
docs/​anthropic-directory-submission.md Updates the submission’s guard description.
crates/​imcp2-core/​src/​tools.rs Updates the call-site comment to include keyword checks; no functional change.
crates/​imcp2-core/​src/​e2e_canister_tools.rs Adds a keyword-refusal integration test.
crates/​imcp2-core/​src/​compliance.rs Implements keyword matching, exemptions, refusal wording, and unit tests.

Review effort: Balanced

@aterga
aterga marked this pull request as ready for review October 28, 2026 10:40
@aterga
aterga requested a review from a team as a code owner October 28, 2026 10:40
Comment thread crates/imcp2-core/src/compliance.rs Resolved
sea-snake
sea-snake previously approved these changes Oct 28, 2026
Review on #214 asked where the exempt names come from, with a comment per
entry naming its source so the list can be audited. The exemption list
now carries that rule in its doc comment: a name with no source does not
get in, and an entry whose source goes away goes with it.

The ownership pair stays, with its source: `transferOwnership` is the
OpenZeppelin `Ownable` convention that app code ported from the EVM keeps
when it hands an admin role to a new principal, and `transfer_ownership`
is the ic-cdk (Rust) spelling of the same method. It is kept at the
maintainer's direction as the one control-handover name worth exempting;
the comment records that no mainnet interface declaring either spelling
has been verified yet (the IC token standards use `setOwner` or
`set_custodians`, which carry no keyword) and that the exemption goes
should a canister whose `transfer_ownership` moves value turn up.

The four controller spellings (`transfer_controller(s)`,
`transferController(s)`) had no source: on the Internet Computer a
canister's controllers change through the management canister's
`update_settings`, never through an app method. They are removed, so a
method of that name is now refused by the `transfer` keyword like any
other; the tests pin that, and pin the sourced pair as allowed.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VP2vTGhAZfkTv1hwmKERA3
…ares a ledger

Review on #213: resolve_did folded every failure of the candid:service
read into "no interface", so a transport, HTTP or certificate error at
that read was indistinguishable from a canister that publishes none, and
the interface scope let the update through on the name-based scopes
alone. For the case the scope exists for — a declared canister that is a
ledger under non-standard names — one failed read at that point was the
way past it.

The read now says what it established. `calls::read_candid_service`
returns `CandidServiceRead`: `Published` (the `.did` text),
`NotPublished` (a CERTIFIED absence — ic-agent's `LookupPathAbsent`, the
certificate proving the path is not in the state tree — or bytes that
are not text, both properties of the canister) or `Failed` (every other
error: HTTP, transport, a certificate that did not verify, a lookup the
certificate leaves unknown). `resolve_did` and `candid_service` are
re-implemented on top with unchanged semantics for the read-only paths.

`canister_update_call` reads with it and, on `Failed`, refuses the call
with `compliance::unchecked_update_refusal` before anything is encoded
or signed: a retry message that names the method and the error, says a
call that cannot be checked is refused rather than sent unchecked, claims
nothing about the call (the guard learned nothing about it), and says
what a persistent error means. A canister that publishes none keeps
today's behaviour — the name scopes decide, the caller-supplied `candid`
is read in its place and can only add a refusal — and published text the
parser cannot read fails open like an absent interface, since a canister
that wants to hide its interface need only not publish it.

The limit is stated where the guard is described: the module doc, the
README, and both directory-submission docs, including the
financial-transactions acknowledgment for Anthropic, which now states it
rather than implying a seal.

Tests: the classification (only a certified absence means "publishes
none"; HTTP, timeout, certificate and unknown-path errors are failed
reads), the fallback rule, the refusal's register (disclaims the call,
names no venue, carries the pinned phrases), and a PocketIC case where a
canister id no subnet serves yields a failed read and the refusal while
the canister that publishes none still goes through.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VP2vTGhAZfkTv1hwmKERA3
The maintainer's direction, simplifying the rule from the previous
commit: a canister must publish its Candid interface to receive update
calls through this server, or they are denied. The interface scope of
the financial-transactions guard therefore always has the interface it
checks against, and the "checked by name only" caveat for a canister
that publishes none is gone.

`canister_update_call` refuses, before anything is encoded or signed,
whenever the published interface is not in hand, with
`compliance::unchecked_update_refusal` and a `MissingInterface` that
says which case it is and what follows: `NotPublished` (a certified
absence, or bytes that are not text) tells the user the canister must
publish its interface; `Unreadable` (published text the parser cannot
read: not valid Candid, no service, or over the CWE-674 limits) says a
readable one is what unblocks it; `ReadFailed` (a transport, HTTP or
certificate error, named in the message) is the one case worth a retry.
Every variant disclaims the call, names no venue, and carries the
pinned phrases.

The `candid` argument is removed from `canister_update_call`: it was a
fallback for a canister that publishes no interface, and such a call is
now refused outright, so the argument had no effect left. It stays on
`canister_query`, where a query cannot commit state. `resolve_did` and
`candid_service` are unchanged for the read-only paths.

Docs state the rule and its remaining limit — a bespoke method on a
canister whose published interface is not a ledger's is not judged — in
the module doc, the README, both directory-submission docs and the
financial-transactions acknowledgment for Anthropic.

Tests: the refusal's three variants and their register; a PocketIC case
for each — the canister that publishes none, a canister whose published
interface is truncated Candid, and a canister id no subnet serves — each
refused with its own wording and never reached.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VP2vTGhAZfkTv1hwmKERA3
…/guard-financial-keywords-69frs8

Carries the must-publish rule (e732ed3) and the failed-read refusal
(55b1b8f) from #213 into the keyword scope's branch. Conflicts resolved
by keeping both sides: the README tool row and the OpenAI submission
doc carry the keyword clause and the must-publish clause; the compliance
tests keep both branches' register constants and the keyword tests, with
the missing-interface register check placed in the register test it
belongs to.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VP2vTGhAZfkTv1hwmKERA3
Review on #213 (Copilot) caught a sentence the must-publish rule had
left behind: the compliance module doc still said the interface scope
reads the canister's `candid:service` "else the caller-supplied
`candid`" through `resolve_did`, which no longer holds — the update
path takes no fallback and refuses a canister that publishes none. The
sentence now names the published interface as the only evidence, the
pre-network call-site comment no longer points at `resolve_did`, and
the calls module doc says the caller-supplied definition is a query's.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VP2vTGhAZfkTv1hwmKERA3
… guide

Adversarial review of the must-publish rule found three wording defects,
none with a runtime effect:

- The served Candid syntax guide (`candid_syntax_guide`, the
  `candid://textual-syntax` resource) still said `canister_update_call`
  falls back to encoding arguments by inference from the literals when
  the interface cannot be read. That case is now a refusal, so the
  fallback paragraph is scoped to `canister_query`, and the guide says
  an update call always has the interface or is not sent.
- The sentence every variant of `unchecked_update_refusal` shared said
  the refusal is "about the missing interface", which a FAILED read does
  not establish; it now says "about the check that could not run".
- `NotPublished` asserted "no `candid:service` metadata" for bytes that
  are not text, and "this one does not" publish about an id that may
  hold no canister at all: a certified absence is also what the state
  tree says of an id in a subnet's range that no canister occupies. Bytes
  that are not text are now their own read outcome, `NotText`, refused
  as `Unreadable` (published, unreadable), and the `NotPublished` text
  says the canister publishes none or no canister exists at this id.

Tests pin the new wording, and the PocketIC suite gains the in-range
nonexistent canister (installed, stopped and deleted): refused as a
certified absence, never reached.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VP2vTGhAZfkTv1hwmKERA3
@aterga
aterga changed the base branch from claude/guard-value-moving-detection-69frs8 to main October 28, 2026 14:49
@aterga
aterga dismissed sea-snake’s stale review October 28, 2026 14:49

The base branch was changed.

#213 landed on main as one squash commit (434b41a), and GitHub
retargeted this stacked PR to main, where the branch's copy of #213's
history conflicted with the squash. This merge resolves that by taking,
for every file, exactly the tree of main plus the two keyword commits
(the keyword scope, and the sourced exemptions): main's own changes
since the branch point (#212) are kept, and nothing of #213 is carried
twice.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VP2vTGhAZfkTv1hwmKERA3
@aterga
aterga requested a review from sea-snake October 28, 2026 14:57
@aterga
aterga merged commit db093af into main Oct 28, 2026
16 checks passed
aterga pushed a commit that referenced this pull request Oct 28, 2026
… still pending

#213 and #214 merged on 2026-10-28: a financial canister is now detected from
the interface it publishes, a published and readable interface is required
for any update call, and method names carrying a financial keyword are
refused everywhere. The submission log says so and lists what still stands
between here and the reply on the review thread: the server instructions'
"not a complete filter" clause, a release reaching production, and the
reviewers' test app publishing its interface.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Ui6YJGczXE8vZv2pjyxX2F
aterga added a commit that referenced this pull request Oct 28, 2026
…p the keyword scope's exemption list (#215)

* Describe the guard's three checks in the server instructions instead of calling it incomplete

Directory review asked, alongside the guard extension that landed in #213
and #214, that the server instructions "no longer describe the guard as
incomplete". The financial-transactions paragraph still carried the
pre-extension clause "that guard is a safeguard, not a complete filter",
and the unit test pinned it.

The paragraph now describes what an update call is checked against: by
method name (the ICRC ledger methods, neuron management, and financial
keywords as whole words, naming transfer, withdraw and swap as examples),
by destination (the known financial-service canisters), and by the
canister's own published Candid interface (ICRC-1/ICRC-2 methods make it
a financial service; a missing or unreadable interface means no update
call at all). It states the one thing none of the checks judges, a
bespoke value-moving method under an unrelated name on a non-ledger
canister, so the limit stays stated rather than implied away, and keeps
the redirect. The keyword list, interface markers and canister ids are
still not restated, for the same drift reason as before.

The test now pins the three checks and the stated limit, pins the old
clause out, and checks the three example keywords against the guard so
the paragraph cannot name a keyword the guard does not refuse. The two
submission docs that described the instructions are aligned.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PruVfYuaS7ZAgNBMTsuRP6

* Mention the control-handover exemption in the instructions' keyword check

Review on #215 found the rewritten paragraph over-claimed: it said every
method name carrying a financial keyword is refused, while the keyword
check exempts the control-handover names (`transfer_ownership` and its
camelCase spelling), which `exemptions_are_literal` pins. The paragraph
now says so in general terms with the one example, and the test pins
that example against the guard the same way it pins the three keywords,
so the instructions cannot name an exemption the guard does not honour.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PruVfYuaS7ZAgNBMTsuRP6

* Drop the keyword scope's exemption list

The list held one name in two spellings, `transfer_ownership` and
`transferOwnership`, kept on the OpenZeppelin `Ownable` convention
alone: no mainnet interface declaring either could be found when review
asked for sources. The list's own rule was that a name with no source
does not get in, so it goes (at the maintainer's direction), and a name
that carries a keyword is now refused whatever the app means by it.

`financial_keyword` no longer consults a list; the module doc records
that the exemption was tried and why it went; the tests pin the
ownership spellings as refused (`no_name_is_exempt`), the keyword list's
well-formedness check loses its exemption half, and the server
instructions, their pinning test, the README and both directory
submission docs no longer mention an exemption — the test now pins that
the instructions mention none and that `transfer_ownership` is refused.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01VP2vTGhAZfkTv1hwmKERA3

---------

Co-authored-by: Claude Fable 5.1 <noreply@anthropic.com>
@aterga aterga mentioned this pull request Oct 28, 2026
6 of 8 tasks
aterga added a commit that referenced this pull request Oct 28, 2026
All three crates share one version. The minor bump carries the financial
guard's new scopes (#213, #214, #215): update calls that 0.5.1 allowed —
any method name carrying a financial keyword, every update to a canister
whose interface declares ICRC-1/ICRC-2 methods, and every update to a
canister whose interface is missing or unreadable — are refused now,
which is a behaviour change for anyone embedding the crate.


Claude-Session: https://claude.ai/code/session_01PruVfYuaS7ZAgNBMTsuRP6

Co-authored-by: Claude <noreply@anthropic.com>
@aterga aterga mentioned this pull request Oct 28, 2026
6 of 7 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants