fix(imported-tokens): validate the entered canisters anonymously - #8039
Open
yhabib wants to merge 3 commits into
Open
fix(imported-tokens): validate the entered canisters anonymously#8039yhabib wants to merge 3 commits into
yhabib wants to merge 3 commits into
Conversation
The import token flow validates the ledger and the index canister before it imports the token. Both calls used the user identity, so both carried the user principal to the canister. A user can type the canister IDs, and the `import-ledger-id` and `import-index-id` URL parameters can also supply them. The modal validates a deep link as soon as the imported tokens are loaded, so one visit to a crafted link sent the user principal to two canisters that the link chose. The token metadata and the `ledger_id` of an index canister are public data, so `getIcrcTokenMetaData` and `getLedgerId` now use the anonymous identity. `addImportedToken` keeps the user identity, because it writes to the nns-dapp backend after the user clicks Confirm.
…lidation The test opens `/tokens/?import-ledger-id=...&import-index-id=...` with a signed-in user. It records every IC request that the browser sends before the first click. Each request to the two URL canisters must carry the anonymous sender and no `sender_pubkey` field. The same window must hold a signed request to the nns-dapp backend canister, which proves the session is authenticated and the check is not vacuous. The test also runs the rest of the deep link flow. The review step must open with no click, and Confirm must still import the token.
|
✅ No security or compliance issues detected. Reviewed everything up to 0defe20. Security Overview
Detected Code Changes
|
Contributor
There was a problem hiding this comment.
🟢 Approved
The core change is narrowly scoped to identity selection for untrusted-canister validation and is backed by updated unit coverage plus a targeted e2e regression test.
Pull request overview
Addresses a privacy leak in the “import token” deep-link flow by ensuring canister validation requests (based on untrusted URL/form inputs) are made with an anonymous identity so the user principal is not disclosed before confirmation.
Changes:
- Switch ICRC ledger token-metadata and ICRC index ledger-id validation calls to use an anonymous identity.
- Update/add unit and modal tests to assert anonymous identity is used for zero-click validation, while the actual import remains authenticated.
- Add a Playwright e2e test that inspects raw IC request envelopes to ensure URL-canister requests are unsigned/anonymous.
File summaries
| File | Description |
|---|---|
| frontend/src/lib/services/icrc-index.services.ts | Uses anonymous identity for index→ledger-id validation calls. |
| frontend/src/lib/services/icrc-accounts.services.ts | Uses anonymous identity for ledger token-metadata fetch during import validation. |
| frontend/src/tests/lib/services/icrc-index.services.spec.ts | Updates service unit test to expect anonymous identity usage. |
| frontend/src/tests/lib/services/icrc-accounts.services.spec.ts | Updates/adds unit tests asserting anonymous identity for metadata fetch. |
| frontend/src/tests/lib/pages/IcrcWallet.spec.ts | Updates page-level test to expect anonymous identity in index validation. |
| frontend/src/tests/lib/components/accounts/ImportTokenModal.spec.ts | Adds modal test ensuring URL-based validation uses anonymous identity, import uses authenticated identity. |
| frontend/src/tests/e2e/import-token-anonymous-validation.spec.ts | Adds e2e deep-link test validating raw IC requests are anonymous for URL canisters. |
| CHANGELOG-Nns-Dapp-unreleased.md | Adds Security changelog entry documenting the privacy fix. |
Review details
Suppressed comments (1)
frontend/src/tests/lib/services/icrc-accounts.services.spec.ts:864
- Same brittleness as above: comparing against
new AnonymousIdentity()by deep equality makes the test depend on implementation details rather than the requirement (anonymous principal). Useexpect.any(AnonymousIdentity)for the argument match.
expect(ledgerApi.queryIcrcToken).toHaveBeenCalledWith({
identity: new AnonymousIdentity(),
certified: false,
canisterId: ledgerCanisterId,
});
- Files reviewed: 8/8 changed files
- Comments generated: 4
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
…ality Replace the deep-equality checks on new AnonymousIdentity() with expect.any(AnonymousIdentity). A deep-equality check on the identity couples the test to how getAnonymousIdentity() builds the instance. The behavioral principal checks stay in place.
Copilot
AI
dismissed their stale review, a newer Copilot review was requested
September 4, 2026 13:12
Contributor
There was a problem hiding this comment.
🟢 Approved
The identity changes are narrowly scoped to validation calls and are covered by updated unit tests plus a targeted new e2e test that verifies request envelopes remain anonymous for URL-supplied canisters.
Review details
- Files reviewed: 8/8 changed files
- Comments generated: 0 new
- Review effort level: Lite
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.
Motivation
The import token flow validates the ledger and the index canister before it imports the token. Both validation calls used the user identity, so both carried the user principal to the canister the user or a URL supplied.
The
import-ledger-idandimport-index-idURL parameters prefill the import form and open the review step with no click. So one visit to a crafted deep link sent the user principal to two attacker-chosen canisters, before the user confirmed anything.Changes
getIcrcTokenMetaDatainicrc-accounts.services.tsto the anonymous identity.getLedgerIdinicrc-index.services.tsto the anonymous identity, keptcertified: true.sender_pubkey, while a parallel request to the nns-dapp backend does.Tests
npm run check,npm run test(672 files, 5906 passed, 9 skipped), and./scripts/check-relative-importsall pass.Confirmed the new tests catch the bug: reverting either identity switch alone fails the new modal test and the matching service test.
The e2e test needs a working local replica to run here, so CI must run
frontend/src/tests/e2e/import-token-anonymous-validation.spec.tsandfrontend/src/tests/e2e/import-token.spec.tsbefore merge. They are the only proof that the ICRC index canister accepts an anonymousledger_idupdate call.Todos