fix(sdk): enforce the server's limit contract when lowering document queries - #4434
Conversation
…queries The DocumentQuery to DriveDocumentQuery lowering diverged from the server's DriveDocumentQuery::from_typed_clauses in three proof-sensitive ways: an omitted limit lowered to None, which SizedQuery treats as unbounded while the server applies config.default_query_limit, so a malicious node could attach an authenticated proof covering every matching document to a request that legitimately proves at most the default; explicit limits 101..=65535 passed through even though the server refuses them with InvalidLimit before answering; and limits above u16::MAX were silently truncated by an as cast (65537 became 1). Lower the sentinel to the compile-time default and refuse over-cap limits with the server's own InvalidLimit instead of truncating or passing through. Callers of the conversion see Some(100) where they previously saw None for an omitted limit, and Error::Drive(QuerySyntaxError::InvalidLimit) instead of silent acceptance for over-cap ones. No in-tree caller depends on the old shapes; the pre-existing FromProof-for-Documents verification path inherits the fix.
|
Warning Review limit reached
Next review available in: 59 minutes Limit details: You’ve used the included review currently available. You've used all free OSS reviews for now. Wait for the free limit to reset to keep reviewing this public repository. How can I continue?Wait for the limit to reset, then comment An organization admin can change what happens after included review limits in Billing. How do review limits work?CodeRabbit enforces per-developer PR review limits within each organization. For paid Pro and Pro+ reviews, CodeRabbit uses a developer's included PR review attempts over the past 7 days to set the current hourly allowance. At typical activity levels, the full plan allowance applies. Higher sustained activity can lower the allowance until earlier attempts leave the 7-day window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (2)
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 |
|
✅ Final review complete — no blockers (commit 9f82577) |
Codecov Report❌ Patch coverage is
❌ Your patch check has failed because the patch coverage (19.04%) is below the target coverage (50.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## v4.2-dev #4434 +/- ##
============================================
- Coverage 87.21% 87.08% -0.14%
============================================
Files 2729 2729
Lines 347524 347994 +470
============================================
- Hits 303110 303037 -73
- Misses 44414 44957 +543
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Final validation — Codex/Sol only (Phase 2 disabled)
The exact-head diff correctly maps an omitted document limit to the canonical default, rejects every value above the server cap before narrowing to u16, and preserves the typed InvalidLimit error. The regression test covers the default, boundary, over-cap, former truncation, and maximum-u32 cases; no in-scope defects were found.
Source: reviewer backend model gpt-5.6-sol (general, security-auditor, and rust-quality lanes); final verifier backend model gpt-5.6-sol; orchestration-only openclaw-agent/cliproxy/gpt-5.6-sol (not reviewer evidence).
Validated zero-blocker Codex/Sol precheck evidence was promoted to final because Phase 2 (Sonnet/Opus) is temporarily disabled. This is Codex/Sol-only final validation, not Codex + Sonnet/Opus coverage.
Review provenance
- Codex reviewers:
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/Opus: not run (Phase 2 disabled — temporary Codex/Sol-only final)
- Secondary pass: disabled (
temporary_phase2_sonnet_disable)
|
Note on the red codecov/patch: the new lowering branches are all exercised — tests/document_limit_lowering.rs drives unset (→ Some(100)), at-cap, and each rejection case (101, 65535, 65537, u32::MAX) including the typed InvalidLimit match — but the coverage job does not attribute this package's integration-test runs, so the patch metric reads 19%. The gate is advisory in this repo (#4388 merged with it red for the same reason). Every functional check is green and thepastaclaw's gate passed with no blockers. 🤖 Posted autonomously by Claude on behalf of pasta. |
3 participants
Issue being fixed or feature implemented
The
DocumentQuery→DriveDocumentQuerylowering diverges from the server's limit contract in three proof-sensitive ways.SizedQuery::limitis part of the reconstructed path query, so each divergence lets an untrusted transport pair a request the server would refuse (or answer differently) with a genuine proof produced for another query — the pre-existingFromProofverification path forDocumentsis affected today:limit: None— unbounded inSizedQuery— while the server appliesconfig.default_query_limitbefore building the path query. A malicious node could attach an authenticated proof covering every matching document to a request that legitimately proves at most the default.InvalidLimitbefore answering.u16::MAXwere silently truncated by anascast (65537 became 1).Split out of #4389 so that PR stays a pure refactor: its new
verify_documents_responseentry point delegates through this lowering and needs this contract, so #4389 is stacked on this PR.What was done?
The lowering now mirrors
DriveDocumentQuery::from_typed_clausesexactly: the0sentinel falls back toSome(DEFAULT_QUERY_LIMIT)(the compile-time value ofconfig.default_query_limit, 100), and anything above the cap is refused with the server's ownError::Drive(QuerySyntaxError::InvalidLimit).How Has This Been Tested?
cargo test -p dash-platform-queries: a regression test pins the full contract — unset →Some(100), at-cap passthrough, and rejection (with the typedInvalidLimitvariant) for 101, 65535, 65537 (the old truncation case) andu32::MAX. Fulldash-sdkbuild and lib tests.Breaking Changes
Callers of the conversion see
Some(100)where they previously sawNonefor an omitted limit, andInvalidLimitinstead of silent acceptance/truncation for over-cap ones. No in-tree caller depends on the old shapes. The verifier consequence: parity targets the canonical configuration — an operator who raisesdefault_query_limitabove 100 serves limits this lowering refuses; the SDK cannot observe per-operator tuning, and proof bytes must not depend on it.Checklist: