fix(sdk): enforce server limit parity in the aggregate proof verifiers - #4432
fix(sdk): enforce server limit parity in the aggregate proof verifiers#4432PastaPastaPasta wants to merge 2 commits into
Conversation
The COUNT, SUM and AVG proof verifiers accepted request limits from 101 through 65535: they only guarded the u16 narrowing, while the server's aggregate dispatchers refuse anything above max_query_limit with InvalidLimit before producing proof bytes. When the matching range holds fewer entries than either limit, a genuine proof for a server-valid query also satisfies the wider path query, so an untrusted transport could pair a server-invalid request with a genuine proof. Gate all three verifiers behind one shared aggregate_limit check that mirrors the server cap (the compile-time default, pinned to stay within max_query_limit's default by a const assertion) and translate the unset-limit sentinel per walk shape in one place. Extracted unchanged from the trust-boundary hardening review rounds of #4389; the regression test drives all three FromProof entry points with a provider that panics if proof machinery is reached.
The shared carrier converter treated every carrier proof as the In-outer shape, but the COUNT dispatcher applies shape-dependent rules: range-outer (G8) lowers an unset limit to MAX_CARRIER_AGGREGATE_OUTER_RANGE_LIMIT and refuses explicit limits above that cap, while In-outer (G7) keeps None and refuses every explicit limit. The old translation both rejected honest G8 proofs (reconstructing limit: None where the server proved limit: 10) and accepted explicit limits the server refuses. Mirror the server's own shape test (two range operators among the where clauses) and its per-shape lowering. The cap check now returns a ServerCappedLimit witness whose methods perform the walk conversions, so the check-before-convert ordering is enforced by the type system instead of a debug_assert.
|
Warning Review limit reached
Next review available in: 42 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 (6)
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 |
|
⛔ Blockers found — Opus deferred (commit 7769e31) |
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (0.00%) 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 #4432 +/- ##
============================================
- Coverage 87.21% 87.05% -0.17%
============================================
Files 2729 2730 +1
Lines 347524 348160 +636
============================================
- Hits 303110 303087 -23
- Misses 44414 45073 +659
🚀 New features to boost your workflow:
|
thepastaclaw
left a comment
thepastaclaw
left a comment
There was a problem hiding this comment.
Preliminary review — Codex only
The COUNT carrier conversion now matches the server's G7/G8 path-query shapes, but the new shared gate treats the configurable default of 100 as a universal server ceiling. This rejects valid proofs from servers configured above 100 and cannot establish the claimed request/server parity for servers configured below 100, so the limit policy must be made fixed on the server or conveyed to the verifier through authenticated input. Source: Codex reviewer lanes codex-general, codex-security-auditor, and codex-rust-quality (exact backend model identifiers were not present in the supplied evidence); final verifier: Anthropic Claude Agent SDK (exact model identifier not exposed); openclaw-agent/cliproxy/gpt-5.6-sol is orchestration-only and not reviewer evidence.
Validated blockers were found in the Codex precheck. Opus is deferred until a fresh Codex revalidation clears the blocker gate.
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: not run (deferred by blocker gate)
🔴 1 blocking | 🟡 2 suggestion(s)
🤖 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/dash-platform-queries/src/documents/aggregate_limit.rs`:
- [BLOCKING] packages/dash-platform-queries/src/documents/aggregate_limit.rs:62-68: The default limit is not the server's actual cap
`DEFAULT_MAX_QUERY_LIMIT` is only the default for the environment-configurable `DriveConfig::max_query_limit`; `PlatformConfig` flattens that configuration and the COUNT, SUM, and AVG proof dispatchers compare requests against `request.drive_config.max_query_limit`. For example, a server configured with `max_query_limit = 200` can legitimately produce a RangeDistinctProof for an explicit limit of 101, but this verifier rejects that authentic response before verification. Conversely, a server configured with a cap of 50 refuses a limit of 75 while this gate accepts it, so the gate does not provide the claimed server-policy binding. Make the proof-serving policy a shared compile-time ceiling on the server as well, or provide the effective policy to the verifier through authenticated input; using the configuration default as though it were every server's runtime value is neither parity-preserving nor backward-compatible.
In `packages/dash-platform-queries/src/documents/count_proof_helpers.rs`:
- [SUGGESTION] packages/dash-platform-queries/src/documents/count_proof_helpers.rs:282-288: Exercise COUNT carrier shape translation through the real verifier path
The proof-sensitive behavior depends on both deriving `has_outer_range` from the request clauses and passing the converted limit into `verify_carrier_aggregate_count_proof`. The new unit tests bypass that composition by supplying `true` or `false` directly, while the integration test only exercises the shared over-100 gate before mode detection. A transposed predicate, incorrect mode route, or wrong argument at the proof call would therefore leave all new tests passing while valid G7/G8 proofs fail. Add entry-point coverage for G8 with unset, 10, and 11 limits and G7 with unset and explicit limits; server-produced or checked-in proof fixtures should also verify that the reconstructed `SizedQuery::limit` matches end to end.
In `packages/dash-platform-queries/tests/aggregate_limit_parity.rs`:
- [SUGGESTION] packages/dash-platform-queries/tests/aggregate_limit_parity.rs:93-98: Assert the typed RequestError instead of only its Display text
The regression test checks only the formatted error text, so another error variant containing the same substring would satisfy it. Destructure `drive_proof_verifier::Error::RequestError` first and then inspect its payload; this pins both the typed API result and the intended limit-gate message.
| if limit > u32::from(DEFAULT_MAX_QUERY_LIMIT) { | ||
| return Err(drive_proof_verifier::Error::RequestError { | ||
| error: format!( | ||
| "limit {limit} exceeds the server's max_query_limit {DEFAULT_MAX_QUERY_LIMIT} \ | ||
| on the prove path ({surface}); the server refuses such requests with \ | ||
| InvalidLimit before producing proof bytes, so no proved response can \ | ||
| belong to this request" |
There was a problem hiding this comment.
🔴 Blocking: The default limit is not the server's actual cap
DEFAULT_MAX_QUERY_LIMIT is only the default for the environment-configurable DriveConfig::max_query_limit; PlatformConfig flattens that configuration and the COUNT, SUM, and AVG proof dispatchers compare requests against request.drive_config.max_query_limit. For example, a server configured with max_query_limit = 200 can legitimately produce a RangeDistinctProof for an explicit limit of 101, but this verifier rejects that authentic response before verification. Conversely, a server configured with a cap of 50 refuses a limit of 75 while this gate accepts it, so the gate does not provide the claimed server-policy binding. Make the proof-serving policy a shared compile-time ceiling on the server as well, or provide the effective policy to the verifier through authenticated input; using the configuration default as though it were every server's runtime value is neither parity-preserving nor backward-compatible.
source: ['codex']
| let has_outer_range = request | ||
| .where_clauses | ||
| .iter() | ||
| .filter(|wc| DriveDocumentCountQuery::is_range_operator(wc.operator)) | ||
| .count() | ||
| == 2; | ||
| let limit_u16 = capped_limit.count_carrier_walk_limit(has_outer_range)?; |
There was a problem hiding this comment.
🟡 Suggestion: Exercise COUNT carrier shape translation through the real verifier path
The proof-sensitive behavior depends on both deriving has_outer_range from the request clauses and passing the converted limit into verify_carrier_aggregate_count_proof. The new unit tests bypass that composition by supplying true or false directly, while the integration test only exercises the shared over-100 gate before mode detection. A transposed predicate, incorrect mode route, or wrong argument at the proof call would therefore leave all new tests passing while valid G7/G8 proofs fail. Add entry-point coverage for G8 with unset, 10, and 11 limits and G7 with unset and explicit limits; server-produced or checked-in proof fixtures should also verify that the reconstructed SizedQuery::limit matches end to end.
source: ['codex']
| assert!( | ||
| error | ||
| .to_string() | ||
| .contains("exceeds the server's max_query_limit 100"), | ||
| "unexpected error for {surface} limit {limit}: {error}" | ||
| ); |
There was a problem hiding this comment.
🟡 Suggestion: Assert the typed RequestError instead of only its Display text
The regression test checks only the formatted error text, so another error variant containing the same substring would satisfy it. Destructure drive_proof_verifier::Error::RequestError first and then inspect its payload; this pins both the typed API result and the intended limit-gate message.
| assert!( | |
| error | |
| .to_string() | |
| .contains("exceeds the server's max_query_limit 100"), | |
| "unexpected error for {surface} limit {limit}: {error}" | |
| ); | |
| let message = match error { | |
| drive_proof_verifier::Error::RequestError { error } => error, | |
| other => panic!( | |
| "expected RequestError for {surface} limit {limit}, got {other:?}" | |
| ), | |
| }; | |
| assert!( | |
| message.contains("exceeds the server's max_query_limit 100"), | |
| "unexpected RequestError for {surface} limit {limit}: {message}" | |
| ); |
source: ['codex']
2 participants
Issue being fixed or feature implemented
The COUNT / SUM / AVG proof verifiers in
dash-platform-queriesaccept request limits the server refuses, and translate the unset-limit sentinel differently from the server's dispatchers. Both mismatches are proof-sensitive:SizedQuery::limitis part of the reconstructed path query, so a divergence either rejects honest proofs or verifies request/response pairings no honest server produced (an untrusted transport can pair a server-invalid request with a genuine proof for the narrower query).Split out of #4389 on scope grounds: these verifiers predate that PR and its new verification surface (
verify_documents_response) never routes aggregate selects, so the fixes stand alone. They resolve the review findings originally filed there (finding=29b040f358c5, plus the shared-cap findings from the earlier rounds).What was done?
aggregate_limitgate rejects limits above the server's compile-time query cap (100) in all three verifiers, before any proof or context-provider machinery. A const assertion pins the fallback within the cap so a future divergence is a build error.MAX_CARRIER_AGGREGATE_OUTER_RANGE_LIMIT(10) and refuses explicit limits above that cap; In-outer (G7) keepsNoneand refuses every explicit limit. SUM / AVG keep the plain translation their dispatchers use.ServerCappedLimitwitness whose methods perform the walk conversions, so check-before-convert ordering is a compile-time property.How Has This Been Tested?
cargo test -p dash-platform-queries: unit tests pin the cap boundaries and both COUNT carrier shapes; a regression test drives all threeFromProofentry points with a context provider that panics if proof machinery is reached, proving rejection happens up front.Follow-ups deliberately not in this PR
Inherited from #4389's follow-up list; they concern this surface and now track here:
DocumentQuerywith no wire-request wrapper, so there is no oneof arm to check yet; adding wrappers analogous toverify_documents_responseremains open.Aggregatemode — and the SUM/AVG equivalents — still apply only the numeric cap, so alimit: 50plain-aggregate request verifies against a proof produced forlimit: None. Closing that requires the same dispatcher-mirroring treatment per mode.Breaking Changes
None. Requests an honest server serves verify exactly as before; the newly rejected shapes are ones the server refuses with
InvalidLimit.Checklist: