WIP: Feat/disable legacy nonces #152
Conversation
WalkthroughReplaces Option-based nonce parsing with a Result-based TryFrom → VersionedNonce conversion, updates call sites to handle conversion errors, narrows engine handling to the V1 variant, and removes/adjusts legacy-nonce tests and related test flows. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant Caller as Engine / Contract
participant Nonce as Nonce bytes
participant Conv as VersionedNonce::try_from
participant Handler as V1 handler (salt/deadline checks)
Note over Caller,Nonce: Incoming request includes `Nonce`
Caller->>Conv: try_from(Nonce)
alt valid and versioned
Conv-->>Caller: Ok(VersionedNonce::V1)
Caller->>Handler: match V1 -> validate salt/deadline
alt checks pass
Handler-->>Caller: proceed with intent execution
else checks fail
Handler-->>Caller: return error (e.g., expired/invalid salt)
end
else invalid / legacy
Conv-->>Caller: Err(DefuseError::InvalidNonce)
Caller-->>Caller: short-circuit / return false for cleanup or propagate error
end
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes
Possibly related PRs
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (3 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (4)
core/src/nonce/versioned.rs (2)
21-25: Update legacy-nonce comment to reflect new behaviorThe note now contradicts the implementation: legacy/non-versioned nonces are rejected via
TryFrom<Nonce>and treated asDefuseError::InvalidNonce, not “still usable”.-// NOTE: Legacy nonces can still be used at this time, but will be prohibited out in the near future. +// NOTE: Legacy/non-versioned nonces are treated as invalid; only versioned nonces +// with `VERSIONED_MAGIC_PREFIX` are accepted by the engine.
62-80: Renamemaybe_from_testto match TryFrom semanticsThe test now exercises
VersionedNonce::try_fromrather thanmaybe_from, but the function name still references the old API. Renaming will avoid confusion for future readers.- #[rstest] - fn maybe_from_test(random_bytes: Vec<u8>) { + #[rstest] + fn try_from_nonce_test(random_bytes: Vec<u8>) {tests/src/tests/defuse/accounts/nonces.rs (1)
42-60: Legacy nonce commit test matches new InvalidNonce semanticsThe new “can't commit legacy nonce” block correctly asserts that executing an intent with a non-versioned/random nonce now fails with
"invalid nonce", which aligns with the engine’s stricter verification.As a minor improvement, you could use a deterministic value that’s guaranteed not to start with
VERSIONED_MAGIC_PREFIX(e.g., a zeroed nonce) instead ofrng.random(), to remove even the tiny chance of accidentally generating a valid versioned nonce.- let deadline = Deadline::MAX; - let legacy_nonce = rng.random(); + let deadline = Deadline::MAX; + // Deterministic non-versioned nonce; guaranteed to fail TryFrom<Nonce>. + let legacy_nonce = Default::default();tests/src/tests/defuse/intents/mod.rs (1)
302-362: Consider using#[ignore]instead of fully commenting out WebAuthn testThe WebAuthn test is temporarily disabled with a large commented block. Using
#[ignore]on the test instead keeps the code compiled and closer to the final shape while still excluding it from regular runs.-// TODO: temporary disabled test until we have correct way to generate WebAuthn signatures -// #[tokio::test] -// #[rstest] -// async fn webauthn(#[values(false, true)] no_registration: bool) { +// TODO: temporary disabled test until we have correct way to generate WebAuthn signatures +#[tokio::test] +#[rstest] +#[ignore = "WebAuthn signatures generation not yet implemented for this test"] +async fn webauthn(#[values(false, true)] no_registration: bool) { /* existing body */ -// } +}
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (6)
core/src/engine/mod.rs(1 hunks)core/src/nonce/versioned.rs(4 hunks)defuse/src/contract/garbage_collector.rs(1 hunks)tests/src/tests/defuse/accounts/nonces.rs(3 hunks)tests/src/tests/defuse/intents/legacy_nonce.rs(0 hunks)tests/src/tests/defuse/intents/mod.rs(4 hunks)
💤 Files with no reviewable changes (1)
- tests/src/tests/defuse/intents/legacy_nonce.rs
🧰 Additional context used
🧬 Code graph analysis (2)
tests/src/tests/defuse/intents/mod.rs (1)
core/src/payload/mod.rs (1)
nonce(68-70)
defuse/src/contract/garbage_collector.rs (1)
core/src/nonce/versioned.rs (1)
try_from(30-36)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (7)
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Shared security analysis / Static Analysis (intents-contract, contract, near, defuse, 1, ., 1.18, false, --filter-paths "nod...
- GitHub Check: Shared security analysis / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Security Audit - report
🔇 Additional comments (4)
core/src/nonce/versioned.rs (1)
27-36: TryFrom implementation correctly gates nonces by magic prefixConversion now strictly requires the versioned magic prefix and successful Borsh deserialization; any mismatch or decode error becomes
DefuseError::InvalidNonce, which matches the PR goal of disabling legacy nonces. The use ofstrip_prefixplusdeserialize_readeris succinct and sound for the current V1 layout.defuse/src/contract/garbage_collector.rs (1)
33-43: Confirm intent to leave legacy/non-versioned nonces uncleared
is_nonce_cleanablenow returnsfalsewheneverVersionedNonce::try_fromfails, so legacy or otherwise invalid nonces will never be cleaned, even if they exist in old state. This matches the comment but is a behavioral change from earlier, more permissive cleanup logic.Can you confirm there are no remaining production flows that rely on garbage-collecting legacy nonces (e.g., old used nonces that should eventually disappear), and that keeping them “stuck” as used is acceptable from a UX/state-size perspective?
core/src/engine/mod.rs (1)
88-106: Nonce verification now correctly rejects non-versioned/invalid nonces
verify_intent_noncenow requires aVersionedNoncevianonce.try_into()?; any non-versioned or malformed nonce fails fast withDefuseError::InvalidNonce, while the existing salt/deadline checks remain intact for V1. This matches the desired “versioned-only” policy.tests/src/tests/defuse/intents/mod.rs (1)
3-4: simulate_is_view_method now validates nonce via payload extractionSwitching to
sign_defuse_payload_defaultplusextract_nonce()makes the test exercise the same payload construction path as the engine and asserts directly against the nonce embedded in the signed payload. This is a solid way to keep the test aligned with the versioned-nonce flow.Also applies to: 220-252
There was a problem hiding this comment.
Actionable comments posted: 1
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
tests/src/tests/defuse/intents/mod.rs(4 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
tests/src/tests/defuse/intents/mod.rs (2)
tests/src/tests/defuse/env/builder.rs (1)
no_registration(80-83)core/src/payload/mod.rs (1)
nonce(68-70)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (5)
- GitHub Check: Security Audit - deny
- GitHub Check: Build Reproducible
- GitHub Check: Shared / Static Analysis (intents-poa-token, contract, near, poa-token, 1, ., 1.18, false, --filter-paths ...
- GitHub Check: Shared / Static Analysis (intents-poa-factory, contract, near, poa-factory, 1, ., 1.18, false, --filter-pa...
- GitHub Check: Security Audit - report
🔇 Additional comments (3)
tests/src/tests/defuse/intents/mod.rs (3)
1-2: LGTM!The import changes correctly support the new payload-based nonce extraction pattern.
212-213: LGTM!The simplified signature correctly reflects the shift to payload-based signing. The
DefuseSignerExttrait import provides the necessary signing functionality.
239-244: Nonce extraction mechanism verified—no issues found.The implementation correctly maintains nonce uniqueness and proper extraction:
Unique nonce generation:
unique_nonce()combines three components for strong uniqueness: a per-account/contract salt, 15 cryptographically random bytes (viaTestRng::from_entropy()), and a 120-second expiration deadline.Correct extraction:
extract_nonce()deserializes theDefusePayloadand retrieves the embedded nonce directly—straightforward and reliable.Uniqueness guarantees: The salt ensures per-account/contract isolation, random bytes provide cryptographic uniqueness, and the deadline adds temporal scoping. This approach is equivalent to or stronger than random-only nonce generation and effectively prevents replay attacks.
The nonce is embedded in the signed payload during
sign_defuse_payload_default()and extracted immediately thereafter, with no intervening state changes—ensuring deterministic, safe behavior across the test suite.
| // TODO: temporary disabled test until we have correct way to generate WebAuthn signatures | ||
| // #[tokio::test] | ||
| // #[rstest] | ||
| // async fn webauthn(#[values(false, true)] no_registration: bool) { | ||
| // const SIGNER_ID: &AccountIdRef = | ||
| // AccountIdRef::new_or_panic("0x3602b546589a8fcafdce7fad64a46f91db0e4d50"); | ||
|
|
||
| // let env = Env::builder() | ||
| // .no_registration(no_registration) | ||
| // .build() | ||
| // .await; | ||
|
|
||
| // let (user, ft) = futures::join!( | ||
| // env.create_named_user("user1"), | ||
| // env.create_named_token("ft1") | ||
| // ); | ||
|
|
||
| // let ft_id = TokenId::from(Nep141TokenId::new(ft.clone())); | ||
|
|
||
| // env.initial_ft_storage_deposit(vec![user.id()], vec![&ft]) | ||
| // .await; | ||
|
|
||
| // // deposit | ||
| // env.defuse_ft_deposit_to(&ft, 2000, &SIGNER_ID.to_owned()) | ||
| // .await | ||
| // .unwrap(); | ||
|
|
||
| // env.defuse | ||
| // .execute_intents(env.defuse.id(), [serde_json::from_str(r#"{ | ||
| // "standard": "webauthn", | ||
| // "payload": "{\"signer_id\":\"0x3602b546589a8fcafdce7fad64a46f91db0e4d50\",\"verifying_contract\":\"defuse.test.near\",\"deadline\":\"2050-03-30T00:00:00Z\",\"nonce\":\"A3nsY1GMVjzyXL3mUzOOP3KT+5a0Ruy+QDNWPhchnxM=\",\"intents\":[{\"intent\":\"transfer\",\"receiver_id\":\"user1.test.near\",\"tokens\":{\"nep141:ft1.poa-factory.test.near\":\"1000\"}}]}", | ||
| // "public_key": "p256:2V8Np9vGqLiwVZ8qmMmpkxU7CTRqje4WtwFeLimSwuuyF1rddQK5fELiMgxUnYbVjbZHCNnGc6fAe4JeDcVxgj3Q", | ||
| // "signature": "p256:2wpTbs61923xQU9L4mqBGSdHSdv5mqMn3zRA2tFmDirm8t4mx1PYAL7Vhe9uta4WMbHoMMTBZ8KQSM7nWug3Nrc7", | ||
| // "client_data_json": "{\"type\":\"webauthn.get\",\"challenge\":\"DjS-6fxaPS3avW-4ls8dDYAynCmsAXWCF86cJBTkHbs\",\"origin\":\"https://defuse-widget-git-feat-passkeys-defuse-94bbc1b2.vercel.app\"}", | ||
| // "authenticator_data": "933cQogpBzE3RSAYSAkfWoNEcBd3X84PxE8iRrRVxMgdAAAAAA==" | ||
| // }"#).unwrap(), serde_json::from_str(r#"{ | ||
| // "standard": "webauthn", | ||
| // "payload": "{\"signer_id\":\"0x3602b546589a8fcafdce7fad64a46f91db0e4d50\",\"verifying_contract\":\"defuse.test.near\",\"deadline\":\"2050-03-30T00:00:00Z\",\"nonce\":\"B3nsY1GMVjzyXL3mUzOOP3KT+5a0Ruy+QDNWPhchnxM=\",\"intents\":[{\"intent\":\"transfer\",\"receiver_id\":\"user1.test.near\",\"tokens\":{\"nep141:ft1.poa-factory.test.near\":\"1000\"}}]}", | ||
| // "public_key": "p256:2V8Np9vGqLiwVZ8qmMmpkxU7CTRqje4WtwFeLimSwuuyF1rddQK5fELiMgxUnYbVjbZHCNnGc6fAe4JeDcVxgj3Q", | ||
| // "signature": "p256:5Zq1w2ntVi5EowuKPnaSyuM2XB3JsQZub5CXB1fHsP6MWMSV1RXEoqpgVn5kNK43ZiUoXGBKVvUSS3DszwWCWgG6", | ||
| // "client_data_json": "{\"type\":\"webauthn.get\",\"challenge\":\"6ULo-LNIjd8Gh1mdxzUdHzv2AuGDWMchOORdDnaLXHc\",\"origin\":\"https://defuse-widget-git-feat-passkeys-defuse-94bbc1b2.vercel.app\"}", | ||
| // "authenticator_data": "933cQogpBzE3RSAYSAkfWoNEcBd3X84PxE8iRrRVxMgdAAAAAA==" | ||
| // }"#).unwrap()]) | ||
| // .await | ||
| // .unwrap(); | ||
|
|
||
| // assert_eq!( | ||
| // env.defuse | ||
| // .mt_balance_of(user.id(), &ft_id.to_string()) | ||
| // .await | ||
| // .unwrap(), | ||
| // 2000 | ||
| // ); | ||
| // assert_eq!( | ||
| // env.defuse | ||
| // .mt_balance_of(&SIGNER_ID.to_owned(), &ft_id.to_string()) | ||
| // .await | ||
| // .unwrap(), | ||
| // 0 | ||
| // ); | ||
| // } |
There was a problem hiding this comment.
Track WebAuthn test coverage restoration in an issue.
The removal of the WebAuthn test eliminates end-to-end coverage for passkey/biometric authentication flows. While understandable given the nonce format migration and the PR's WIP status, this represents a significant gap that must be addressed before final release.
Since the PR description indicates this should only be merged after systems are ready, please create a tracking issue for:
- Implementing the correct WebAuthn signature generation path with the new nonce format
- Re-enabling and updating this test
- Verifying WebAuthn flows work end-to-end with the new nonce system
Would you like me to help draft the issue description or generate a skeleton for the updated test implementation?
🤖 Prompt for AI Agents
In tests/src/tests/defuse/intents/mod.rs around lines 295 to 355, the WebAuthn
end-to-end test is temporarily disabled; create a tracking issue to restore
coverage and re-enable the test once nonce/signature generation is updated. Open
an issue that (1) describes implementing correct WebAuthn signature generation
for the new nonce format, (2) updates and re-enables this test with examples of
valid test vectors and harness changes required, and (3) includes an acceptance
checklist to verify full end-to-end WebAuthn flows against the new nonce system;
reference this file and the specific commented test, assign to the feature
owner, label with "webauthn", "testing", and "blocker", and include a note to
link the issue in the PR before merging.
This PR can be merged only after all system will be ready for the update + main part of the solvers will switch to new nonce version
Summary by CodeRabbit
Breaking Changes
Improvements
Tests