MF-L09: fix(nitronode): validate parsed app session nonce#751
Conversation
CreateAppSession previously rejected the raw nonce string "0" only, but
strconv.ParseUint accepts zero-padded inputs ("00", "000", ...) and yields
0. The empty-string branch was also unreachable because unmapAppDefinitionV1
errors out earlier on an unparseable nonce. Switch the check to the parsed
appDef.Nonce value and extend TestCreateAppSession_ZeroNonce to cover the
zero-padded bypass.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ 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 |
ihsraham
left a comment
ihsraham
left a comment
There was a problem hiding this comment.
Approving for the L-09 closure. The check now reads appDef.Nonce == 0 on the parsed uint64, so every base-10 zero-spelling that strconv.ParseUint accepts is rejected, and the new table-driven TestCreateAppSession_ZeroNonce covers "0", "00", and "000" against the PoC.
Side note: this brings app_session_v1 in line with the parsed-value pattern that channel_v1/request_creation.go:45 already uses, so the two surfaces no longer disagree on how a non-zero nonce is enforced. I grepped every strconv.ParseUint/Atoi site across nitronode, pkg, and sdk for the same anti-pattern and came up clean.
One small thing, non-blocking: with the check moved after unmapAppDefinitionV1, an empty "" nonce now surfaces invalid app definition: invalid nonce: strconv.ParseUint: parsing "": invalid syntax instead of the cleaner nonce is zero or not provided. Still a rejection, the error string is noisier. A short pre-check on reqPayload.Definition.Nonce == "" before the unmap would restore the friendlier message. Fine to skip if you'd rather not churn this PR.
nksazonov
left a comment
nksazonov
left a comment
There was a problem hiding this comment.
Good job! The fix is correct — the bypass via zero-padded decimal strings is properly closed by moving from a raw-string comparison to an integer check on the parsed value.
…ndler Empty/non-numeric nonces are already rejected earlier in unmapAppDefinitionV1 with a parse error, so this check only catches integer 0. Match channel_v1/request_creation.go wording. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
- MF-L01: fix(contracts/ChannelHub): cap ERC20 transfer returndata copy to 32 bytes (#726) - MF-H01: fix(nitronode): paginate get_last_key_states endpoints (#724) - MF-I01-I02: fix(contracts): address security audit findings I-01 and I-02 (#728) - MF-C01: rpc: cap inbound WebSocket frame size and rate-limit per connection (#723) - MF-L02: docs(protocol): qualify enforcement guarantee for intent-specific execution paths (#737) - MF-L02-I03-I04_I05: fix(contracts): add more Node trust assumptions and requirements (#738) - MF-M01: backfill state user_sig from on-chain events (#731) - MF-M02: fix(rpc): release Serve wait group on processSink overflow (#732) - Fix SDK acknowledgement before home channel creation (#734) - MF-I06: fix(nitronode): gate escrow transitions on home channel onchain materialization (#730) - MF-M05: fix(nitronode): enforce TLS by default for Postgres (#733) - MF-M07: Unblock receiver states after finalized escrow operations (#735) - MF-M04: feat: provide tooling for and enhance docs on ValidatorRegistered event (#744) - MF-L04: fix(contracts): reject redundant native value (#741) - MF-H02: bind session key registration to a single owner per kind (#739) - MF-I07: fix(contracts): enforce max challenge duration (#752) - MF-M08: fix(rpc): replace Origin label with application_id on connection gauge (#745) - MF-C02: fix(core): add ChannelStatusClosing to gate post-finalize state transitions (#746) - MF-L06: fix(contracts): clear stale challengeExpireAt on cooperative escrow finalization (#754) - MF-I08: docs: document ChannelClosed event orientation ambiguity during abandoned migration (#755) - MF-M09: fix(nitronode): auto-challenge home channel on withheld escrow finalize (#753) - MF-L09: fix(nitronode): validate parsed app session nonce (#751) - MF-L05: docs(contracts): document informational events not guaranteed to emit (#756) - MF-L08: fix(nitronode/api): default get_last_key_states to active-only with include_inactive opt-in (#749) - MF-L10: fix: emit escrowIds array in EscrowDepositsPurged event and handle it in Nitronode (#757)
3 participants
Summary
CreateAppSessionraw-string nonce check (Nonce == "" || Nonce == "0") had a dead branch and a bypass:unmapAppDefinitionV1errors out on empty input before the check, andstrconv.ParseUintaccepts zero-padded inputs ("00","000", ...) that parse to0and skip the== "0"comparison. Net result: an app session could be stored withNonce = 0.appDef.Nonce == 0so the validation runs against the parsed numeric value.TestCreateAppSession_ZeroNonceinto a table-driven test covering"0","00","000".Test plan
go test ./nitronode/api/app_session_v1/...go vet ./nitronode/api/app_session_v1/...🤖 Generated with Claude Code