Skip to content

fix(qr): refuse extra keys in the v1 signing-request envelope - #178

Merged
illuzen merged 2 commits into
Quantus-Network:mainfrom
Kshot3000:fix/sign-request-v1-deny-unknown-fields
Oct 19, 2026
Merged

illuzen merged 2 commits into
Quantus-Network:mainfrom
Kshot3000:fix/sign-request-v1-deny-unknown-fields

Conversation

@Kshot3000

Copy link
Copy Markdown
Contributor

Bug

The v1 signing-request envelope (Wire in src/qr/sign_request.rs) accepts envelopes carrying extra keys, while every other reader of this wire format refuses them:

  • The module documentation states the wallets "accept these three keys and no others" (v, signer, payload).
  • The SDK's SigningRequest.decode (quantus-apps, quantus_sdk/lib/src/models/signing_request.dart) enforces exactly that key set for v1 via its exact-keys check.
  • The v2 NEAR envelope in this same file already enforces it with deny_unknown_fields, and has a test that refuses an added signer key.

The v1 struct was missed, so it has neither the attribute nor a test. This is not just a formatting nit: the cold-signing flow (src/cli/cold_signing.rs) decodes a scanned request with AnySignRequest::decode and signs the payload after checking the signer. A v1 request with extra keys — for example a smuggled chain field — is signed by this CLI but refused by the mobile app, the cold wallet app, and the Keystone firmware. The readers of a security envelope should agree on what they accept.

Fix

Add #[serde(deny_unknown_fields)] to the v1 Wire struct — one line, matching the NEAR envelope — plus a regression test (refuses_extra_keys_like_the_wallets_do) that mirrors the existing NEAR extra-key test: a v1 envelope with an added chain key must fail both SignRequest::decode and AnySignRequest::decode.

Verification

  • Red/green in this crate: SKIP_CIRCUIT_BUILD=1 cargo test --lib qr::sign_request — 11 passed / 1 failed without the fix (the new test: the extra-key envelope decoded successfully), 12/12 passed with it. (SKIP_CIRCUIT_BUILD is build.rs's documented flag for jobs that don't need the generated circuit binaries.)
  • Full cargo test --lib: 422 passed; the only 2 failures (batch-verifier loading, one keystore migration test) fail identically on the unpatched tree in this environment and are unrelated to this change.
  • Disclosure per this repo's SECURITY.md: I used AI tooling to help find and write up this issue.

@Quantus-Network — flagging for the team.

Built by @Kshot9000 (https://x.com/kshot9000) · QTC donations: qznY8nwuvWcCCVys4da1oQdysyh8YUZYRjRqgk3S8Wos8kbau

Kshot3000 and others added 2 commits October 7, 2026 17:24
The v1 Wire struct accepted envelopes with extra keys while the
wallets, the SDK, and the NEAR envelope in the same file all refuse
them. Add #[serde(deny_unknown_fields)] to match, plus a regression
test mirroring the NEAR extra-key test.
@illuzen
illuzen requested a review from n13 October 19, 2026 10:16

@n13 n13 left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Reviewer model: GPT-6.1 Sol

APPROVE — no blocking findings at head 653a1fe9ab85119cd55bd088e1ae189803102f33.

The v1 envelope now rejects unknown fields through both SignRequest::decode and AnySignRequest::decode, including the cold-signing simulator's shared decode path. This matches the existing v2 envelope policy and preserves valid v1 encoding and round trips. The regression test covers both entry points, and no equivalent v1 decoder bypasses the check.

Validation:

  • SKIP_CIRCUIT_BUILD=1 cargo test --locked --lib qr::sign_request: 12 passed.
  • cargo +nightly-2026-08-31 fmt --all -- --check: passed.
  • git diff --check a74fd735fdb7cae2381b66dbaaa513630c0d58d9...HEAD: passed.
  • GitHub CI: all seven checks passed, including Linux/macOS builds and full tests, Clippy/docs, security audit, and examples.

@illuzen
illuzen merged commit b05b208 into Quantus-Network:main Oct 19, 2026
7 checks passed
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.

3 participants