fix(sdk): enforce response freshness anchors - #4167
Conversation
|
Warning Review limit reached
Next review available in: 19 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 (4)
✨ 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 — 24 ahead in queue (commit 4585c42) |
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The PR improves the default freshness policy and correctly prevents stale-time responses from mutating the height watermark, but the new trusted-height alternative does not provide the promised independent anchor in every accepted configuration. Address trunk responses bypass height validation entirely when time validation is disabled, and trusted heights at or below their rollback tolerance impose no effective lower bound.
Validated blockers were found in the Codex precheck. Sonnet is deferred until a fresh Codex revalidation clears the blocker gate.
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— general (completed),gpt-5.6-sol— security-auditor (completed),gpt-5.6-sol— rust-quality (completed) - Verifier:
gpt-5.6-sol— verifier - Sonnet: not run (deferred by blocker gate)
🔴 2 blocking
🤖 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/rs-sdk/src/sdk.rs`:
- [BLOCKING] packages/rs-sdk/src/sdk.rs:278-282: Height-only mode leaves address trunk proofs without a freshness anchor
The builder now accepts a proof-enabled network SDK with time validation disabled when a trusted initial height and height tolerance are configured, but `freshness_criteria` unconditionally disables height validation for `get_addresses_trunk_state`. This produces `(None, None)` for the signed trunk response in that supported configuration. The trunk proof and metadata describe the node's latest stored checkpoint, and the address scan uses its returned height and root to pin all subsequent branch requests. A stale node can therefore replay an old, correctly signed checkpoint without comparing it to the caller's trusted height. The incremental catch-up does not make this safe because its first query treats failures as non-fatal and can return the accepted stale trunk state. Apply a trusted-height check suitable for checkpoint responses or reject time-disabled configurations for operations that require address trunk synchronization.
- [BLOCKING] packages/rs-sdk/src/sdk.rs:1093-1094: Builder accepts trusted heights that impose no freshness floor
`has_height_anchor` checks only that the height and tolerance options are present. A configuration such as `with_trusted_initial_height(0)` or any trusted height less than or equal to its tolerance therefore passes construction with time validation disabled. In `verify_metadata_height`, the stale-response comparison is skipped whenever `expected_height <= tolerance`, so every possible response height is accepted and the first historical response establishes the effective watermark. Require the trusted height to exceed the configured rollback tolerance and add builder tests for zero and `height <= tolerance`.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## v4.1-dev #4167 +/- ##
==========================================
Coverage 87.44% 87.45%
==========================================
Files 2648 2648
Lines 334178 334327 +149
==========================================
+ Hits 292230 292372 +142
- Misses 41948 41955 +7
🚀 New features to boost your workflow:
|
|
Reviewed |
2 participants
Summary
This addresses security review findings DS-CAND-232, 249, and 267.
Compatibility
Network builders now default to a 31-minute signed-time window. Deployments with intentionally unsynchronized clocks or longer network pauses should configure an appropriate time tolerance or seed a trusted initial Platform height. Mock builders retain their previous no-time-check default.
Validation