test(canister): pin the deposit/withdraw amount-validation order - #243
test(canister): pin the deposit/withdraw amount-validation order#243mbjorkqvist wants to merge 3 commits into
Conversation
…oken branch The ordering guarantee — no request-error branch is reachable with an out-of-range amount — had no coverage: every amount-range test used a known token, so moving the check back below `is_known_token` kept the suite green. Add a deposit and a withdraw case combining an unsupported token with an oversized amount and asserting `AmountExceedsMaximum` wins. Both fail if the check is moved back down. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
… check The vulnerability's most severe framing was that it worked against a non-allowlisted caller in restricted mode, since the request was rendered before any authorization check ran. Nothing covered that path: every integration test installs the canister in `Mode::GeneralAvailability`. The call traps either way — pre-fix on the instruction limit, post-fix on the allowlist panic — so trap-vs-no-trap proves nothing here. Assert instead on *why* it trapped, which is what makes the rejection cheap rather than costing a full message's budget. Verified against a reintroduced pre-auth render: the same call dies with CanisterInstructionLimitExceeded after 40B instructions. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
Adds regression tests to lock in the validation/authorization ordering guarantees introduced in #237, ensuring oversized amount inputs are rejected before any error branch that could format/render them, and that restricted-mode authorization rejects non-allowlisted callers before any expensive work can occur.
Changes:
- Add an integration test that asserts restricted-mode rejection happens for the authorization reason (allowlist), not instruction exhaustion.
- Add unit tests that pin amount-range validation to occur before the unsupported-token branch for both
depositandwithdraw.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| integration_tests/tests/tests.rs | Adds restricted-mode integration coverage asserting trap reason corresponds to allowlist rejection (not instruction limit). |
| canister/src/tests.rs | Adds deposit/withdraw unit tests pinning amount validation to win over unsupported-token request errors. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| /// In restricted mode the vulnerable handler rendered the request — exhausting | ||
| /// the instruction budget — before any authorization check, so a | ||
| /// non-allowlisted caller could trap the call. Now the O(1) allowlist check | ||
| /// rejects it first. |
There was a problem hiding this comment.
🤖 Correct, and thanks — fixed in fcfd730.
Mode::RestrictedTo holds a BTreeSet<Principal> and assert_caller_is_allowed does allowed.contains(&caller), so the lookup is O(log n) in the allowlist size. The "O(1)" claim was simply wrong.
Reworded to the property the test actually relies on (tests.rs#L1677):
Now the allowlist check rejects it first, at a cost independent of the amount's magnitude.
That is what makes the difference the test asserts: before the fix the rejection cost scaled with the amount and exhausted the message's instruction budget; now it does not scale with the amount at all, whatever it costs in the allowlist size. The PR description carried the same imprecision ("constant-time allowlist check") and has been corrected the same way.
|
`Mode::RestrictedTo` holds a `BTreeSet`, so the allowlist membership check is O(log n) in the allowlist size, not O(1). The property the test actually relies on is that the check's cost does not depend on the amount's magnitude — say that instead. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
Adds regression coverage for the two guarantees #237 established but left untested. Tests only — no production code changes.
The second one needs care to be meaningful: the call traps either way — before the fix on the instruction limit, after it on the allowlist panic — so asserting that it traps proves nothing. The test asserts on why it trapped instead, which is what distinguishes a cheap rejection from one that costs a full message's budget.
Both tests were confirmed to fail against the pre-fix behaviour: reverting the reorder makes the first pair report
UnsupportedToken, and reintroducing the pre-authorization render makes the second reportCanisterInstructionLimitExceededafter 40B instructions.The behaviour-preserving refactors these tests do not depend on are split out into #242.