fix(drive): bind proof-verifier queries to trusted context#4166
Conversation
|
Warning Review limit reached
Next review available in: 34 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (14)
✨ 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 |
|
🕓 Ready for review — 33 ahead in queue (commit e97fcee) |
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4166 +/- ##
============================================
- Coverage 87.63% 87.46% -0.18%
============================================
Files 2624 2651 +27
Lines 331678 334769 +3091
============================================
+ Hits 290659 292795 +2136
- Misses 41019 41974 +955
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex + Sonnet
Verified the combined Codex checkpoint and all four Sonnet reviewer lanes at head b535feb against actual source. The PR correctly binds vote-poll proof verification to caller-supplied context and epoch selection to request-derived values instead of unsigned response metadata. The two FFI suggestion findings are independently confirmed: both functions insert a new required expected_vote_poll_id: &Uint8Array parameter mid-argument-list and reinterpret poll bytes as a serialized ContestedDocumentResourceVotePoll, while package.json remains at 4.0.0 and no in-repo JS/TS consumer exercises either export.
Review provenance
- Codex reviewers:
gpt-5.6-sol— general (failed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— rust-quality (failed),gpt-5.6-sol— ffi-engineer (failed),gpt-5.6-sol— general (completed),gpt-5.6-sol— security-auditor (failed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed),gpt-5.6-sol— ffi-engineer (completed) - Verifier:
claude-sonnet-5— final-verifier - Sonnet reviewers:
claude-sonnet-5— general (failed),claude-sonnet-5— general (failed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— security-auditor (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— rust-quality (failed),claude-sonnet-5— ffi-engineer (failed),claude-sonnet-5— ffi-engineer (failed),claude-sonnet-5— general (failed),claude-sonnet-5— general (completed),claude-sonnet-5— security-auditor (completed),claude-sonnet-5— rust-quality (completed),claude-sonnet-5— ffi-engineer (completed),claude-sonnet-5— rust-quality (completed),claude-sonnet-5— ffi-engineer (completed)
🟡 2 suggestion(s)
2 additional finding(s) omitted (not in diff).
🤖 Prompt for all review comments with AI agents
These findings are from an automated code review. Verify each finding against the current code and only fix it if needed.
In `packages/wasm-drive-verify/src/voting/verify_vote_poll_vote_state_proof.rs`:
- [SUGGESTION] packages/wasm-drive-verify/src/voting/verify_vote_poll_vote_state_proof.rs:38-49: wasm_bindgen export signature changed in a breaking, unversioned way
`verify_vote_poll_vote_state_proof` (exported as `verifyVotePollVoteStateProof`) inserts a new required `expected_vote_poll_id: &Uint8Array` parameter between `index_name` and `contested_document_resource_vote_poll_bytes`. Since wasm-bindgen exports are called positionally from JS/TS, every existing caller's argument list shifts: the old vote-poll-identifier bytes now land in the `expected_vote_poll_id` slot, and `contested_document_resource_vote_poll_bytes` must now be a fully platform-serialized `ContestedDocumentResourceVotePoll` (via `ContestedDocumentResourceVotePoll::deserialize_from_bytes`) rather than a bare 32-byte `Identifier`. This is a legitimate, deliberate security fix, but it's a larger data-format change than the parameter diff suggests, and package.json for wasm-drive-verify is still at 4.0.0 with no changelog/migration note. No in-repo consumer exercises this export, so there's no compiled signal for downstream callers that the contract changed.
In `packages/wasm-drive-verify/src/voting/verify_vote_poll_votes_proof.rs`:
- [SUGGESTION] packages/wasm-drive-verify/src/voting/verify_vote_poll_votes_proof.rs:36-49: Same breaking positional-signature change as verify_vote_poll_vote_state_proof
`verify_vote_poll_votes_proof` (exported as `verifyVotePollVotesProof`) has the same shape of change as its sibling: a new required `expected_vote_poll_id: &Uint8Array` parameter inserted before `contested_document_resource_vote_poll_bytes`, and that parameter's expected byte format switches from a raw Identifier to a platform-serialized `ContestedDocumentResourceVotePoll`. Same unversioned-breaking-change concern as the sibling function.
…closed proof context The proof-context hardening deliberately fails closed on two client requests that previously leaned on unauthenticated inputs, but the SDK fetch tests still asserted the old permissive behavior: - test_epoch_fetch_current expected fetch_current (a descending epoch query with no explicit start) to succeed; it now returns the explicit-start rejection. - test_contested_resource_voters_for_identity_not_found passed a single index value for the two-property parentNameAndLabel index; the arity guard now rejects the under-specified poll. Update both tests to assert the new rejections so they document the intended security behavior (DS-CAND-314 / DS-CAND-374). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
QuantumExplorer
changed the title
…ontext-binding # Conflicts: # packages/swift-sdk/build_ios.sh
2 participants
Summary
This addresses security review findings DS-CAND-314, 368, and 374.
Compatibility
The two WASM vote-proof exports now require both an expected poll ID and the full serialized vote poll.
Proved epoch queries no longer derive their bound from unsigned metadata:
ExtendedEpochInfo::fetch_current(a descending epoch query with no explicit start) now fails closed withproved descending epoch queries require an explicit start epoch. There is no authenticated current-epoch marker to bound the query, so a node could otherwise cap the proof below the chain tip and pass off a stale epoch as current. Callers must fetch a specific epoch by explicit index until such a marker exists.A contested vote poll must now carry exactly as many index values as the index has properties; under-specified polls are rejected instead of silently resolving a partial path.
Validation
test_epoch_fetch_currentandtest_contested_resource_voters_for_identity_not_foundto assert the new fail-closed behavior