feat(query): origin-aware pagination scan, limit, and offset caps - #3948
feat(query): origin-aware pagination scan, limit, and offset caps#3948amir-deris wants to merge 9 commits into
Conversation
|
The latest Buf updates on your PR. Results from workflow Buf / buf (pull_request).
|
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #3948 +/- ##
==========================================
- Coverage 58.66% 57.66% -1.01%
==========================================
Files 2324 2229 -95
Lines 198757 187711 -11046
==========================================
- Hits 116600 108237 -8363
+ Misses 71407 69620 -1787
+ Partials 10750 9854 -896
Flags with carried forward coverage won't be shown. Click here to find out more.
🚀 New features to boost your workflow:
|
PR SummaryHigh Risk Overview BaseApp now reads Upgrade impact: empty Reviewed by Cursor Bugbot for commit 3bc5663. Bugbot is set up for automated code reviews on this repo. Configure here. |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
![seidroid[bot]](https://avatars.githubusercontent.com/in/2745135?s=60&v=4){kind=small}
There was a problem hiding this comment.
The origin-aware scan-limit plumbing (config, trust matcher, context propagation, call-site migration) is coherent, but the enforcement logic itself has two holes that defeat the PR's purpose: Paginate applies no cap to a large Limit, and the refactored checkPostPage makes the post-page break unreachable, removing an existing bound on the frozen v6.6 consensus path. The new [query] section also lands with none of the characterization coverage testutil/configtest/AGENTS.md requires.
Findings: 3 blocking | 10 non-blocking | 7 posted inline
Blockers
- None at the file/PR level.
- 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- Existing deployments'
app.tomlwill not contain the new[query]section after upgrade, sotrusted-cidrsdefaults to empty and every caller — including the operator's own CLI over localhost — gets the 10k cap. Anycount_total=truequery over a >10k store (e.g.bank totalon a tokenfactory-heavy chain,wasm all-contract-state) starts returningInvalidArgument. Worth an explicit upgrade/release note. - Test coverage gaps for the new enforcement: nothing exercises a large
Limitagainst an untrusted context, nothing exercises the post-page path withcount_total=false, nothing coverstrusted-scan-limit = 0(unlimited), and there is no end-to-end test throughenrichABCIQueryContextproving a gRPC peer in a trusted CIDR actually receives the relaxed limit.TestPaginateTrustedOriginUsesHigherLimitusesOffset: 450withtrustedLimit: 500, where the offset guardcount <= offset && count > limitis unsatisfiable, so it passes for any limit above ~450. query.trusted-cidrs/query.trusted-scan-limitare read by two independent readers with duplicated key strings:baseapp.readQueryConfig(overAppOptions, guarded byv != nil) andconfig.ParseQueryConfig(over viper, guarded byIsSet). Only the BaseApp reader affects behavior;Config.Queryexists to render the template. Have the baseapp reader usesrvconfig's key constants (or a single shared reader) so a rename cannot move one and not the other.- PR description says keepers use
PaginateForContext/FilteredPaginateForContext, but noPaginateForContextexists —Paginatederives enforcement from the context internally. Minor description drift, but it makes the two paginators look symmetric when they are not. - 4 suggestion(s)/nit(s) flagged inline on specific lines.
- 2 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
checkOffsetBeforePageFilled(and the base-branch expression it was extracted from) compares againstpaginationEnd(offset, limit), which saturates atmath.MaxUint64. A request with an offset nearMaxUint64therefore makes the guard permanently false and the filtered paginators scan the whole prefix store uncapped. Present on base infilteredPaginate/genericFilteredPaginateunderenforceV66ScanLimit; the refactor carries it forward to the new untrusted path. - [suggestion] The
onResultclosures insei-cosmos/x/authz/keeper/grpc_query.go(Grants,GranterGrants,GranteeGrants) open withauth1 := auth.GetAuthorization(); if err != nil { return nil, err }, testing the enclosing function's staleerrrather than anything produced inside the closure. The check is dead in every call. Present on base; the PR only reshapes the surrounding call.
8e186f9 to
ba46ceb
Compare
|
@seidroid review |
There was a problem hiding this comment.
Cursor Bugbot has reviewed your changes using default effort and found 1 potential issue.
❌ Bugbot Autofix is OFF. To automatically fix reported issues with cloud agents, enable autofix in the Cursor dashboard.
Want higher recall? High effort reviews run extra passes and find more bugs. A team admin can switch effort levels in the Cursor dashboard.
Reviewed by Cursor Bugbot for commit ba46ceb. Configure here.
There was a problem hiding this comment.
The origin-aware plumbing (config, CIDR matcher, context propagation, call-site migration, configtest coverage) is now solid, but two enforcement holes flagged in the previous review are still present in the restructured code — Paginate applies no cap to a large Limit, and checkPostPage still returns nil for !countTotal, leaving the post-page break unreachable — and the new TestWiringMatchesTheRecord calls a configtest.CheckWiring helper that does not exist, so the sei-cosmos/baseapp test package will not compile.
Findings: 3 blocking | 6 non-blocking | 5 posted inline
Blockers
- None at the file/PR level.
- 3 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion] Test coverage for the new enforcement is still thin: nothing exercises a large
Limitagainst an enforcing context (which is why thePaginatehole survived the last round), nothing covers the key path, nothing coverstrusted-scan-limit = 0(unlimited), nothing covers the post-page path withcount_total=false, and there is no end-to-end test throughenrichABCIQueryContextproving a gRPC peer inside a trusted CIDR actually receives the relaxed limit. - [suggestion] Existing deployments'
app.tomlwill not contain the new[query]section after upgrade, sotrusted-cidrsdefaults to empty and every caller — including the operator's own CLI over localhost — gets the 10k cap. Anycount_total=truequery over a >10k store (e.g.bank totalon a tokenfactory-heavy chain,wasm all-contract-state) starts returningInvalidArgument. This is intentional per the design, but it needs an explicit upgrade/release note. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 2 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion] The offset guard compares against
paginationEnd(offset, limit), which saturates atmath.MaxUint64: a request with an offset nearMaxUint64makes the guard permanently false and the filtered paginators scan the whole prefix store uncapped. Present on base infilteredPaginate/genericFilteredPaginateunderenforceV66ScanLimit; the refactor carries it intooffsetScanCursor.checkOffsetBeforePageFilled(sei-cosmos/types/query/paginate_driver.go:181). - [suggestion] The
onResultclosures insei-cosmos/x/authz/keeper/grpc_query.go(Grants,GranterGrants,GranteeGrants) open withauth1 := auth.GetAuthorization(); if err != nil { return nil, err }, testing the enclosing function's staleerrrather than anything produced inside the closure. The check is dead in every call. Present on base; the PR only reshapes the surrounding call.
Harden Paginate and filtered post-page paths so untrusted callers cannot bypass the 10k cap via large limits or count_total=false, restore baseapp [query] config characterization without the removed CheckWiring helper, and document the upgrade impact in CHANGELOG. Co-authored-by: Cursor <cursoragent@cursor.com>
…lper Add GenericFilteredPaginateForContext so authz query handlers pick the ABCI or v6.6 paginator at the choke point instead of branching at each call site. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
There was a problem hiding this comment.
The three blockers from the previous round (unbounded Limit on Paginate, the dead post-page break, and the missing configtest.CheckWiring) are all genuinely fixed, and the configtest/CIDR/authz/test-coverage suggestions were addressed. One new defect was introduced by the restructure: runOffsetPathUnfiltered signals early exit via a loop variable instead of break, so the loop's post statement runs one extra gas-metered iterator.Next() relative to the base branch — a gas/LastResultsHash change on the EVM precompile path this PR explicitly freezes.
Findings: 1 blocking | 4 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
- 1 blocking issue(s) flagged inline on specific lines.
Non-blocking
- [suggestion]
offsetScanCursor.recordUnfilteredHitsetsc.hits = c.scannedon the unfiltered path, buttotal()andphase()both ignorehitswhenfilteredis false. The assignment is dead and invites a reader to believehitsis meaningful for unfiltered scans. - 2 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion] The
onResultclosures insei-cosmos/x/authz/keeper/grpc_query.go(Grants,GranterGrants,GranteeGrants) open withauth1 := auth.GetAuthorization()followed byif err != nil { return nil, err }, testing the enclosing function's staleerrrather than anything produced inside the closure. The check is dead in every call. Present on base; this PR only lifts the closure into a namedonResultvariable.
Restore the request-level DoS caps from the original pagination hardening without applying them during consensus, so trusted CIDRs stay unblocked and EVM precompile pagination stays on the frozen v6.6 path. Co-authored-by: Cursor <cursoragent@cursor.com>
amir-deris
changed the title
Break out of the offset loop when nextKey is found so precompile paths do not pay an extra gas-metered iterator step, and fix the unfiltered post-page budget off-by-one that could drop nextKey under a tight cap. Co-authored-by: Cursor <cursoragent@cursor.com>
|
@seidroid review |
There was a problem hiding this comment.
All three blockers from the previous round are genuinely fixed: Paginate now bounds limit/offset up front via checkRequest, checkPostPage returns an explicit stop signal so the caller's break is live, the extra gas-metered iterator.Next() is gone (break loop), and the [query] section now carries full configtest coverage in both readers. Remaining findings are non-blocking: the effective scan bound is a multiple of the advertised 10k cap, scanLimitParams.limit == 0 means two contradictory things across the file, and the over-budget count_total=false path silently truncates.
Findings: 0 blocking | 5 non-blocking | 3 posted inline
Blockers
- None at the file/PR level.
Non-blocking
- [suggestion]
queryOriginIP'srpctypes.GetCallInfobranch is untested, yet it is the branch that governs the main untrusted surface: Tendermint RPC/abci_query→BaseApp.Query→handleQueryGRPC. Only the gRPC-peer branch has a test (TestQueryOriginIPFromGRPCPeer). A test that attaches aCallInfowith a remote address and asserts the resulting context is trusted would pin the path operators actually rely on when they allowlist a CIDR. - 3 suggestion(s)/nit(s) flagged inline on specific lines.
- 1 non-blocking pre-existing issue(s) listed below under pre-existing issues.
Pre-existing issues
- [suggestion]
sei-cosmos/x/authz/keeper/grpc_query.go— theonResultcallbacks inGrants(:56),GranterGrants(:101) andGranteeGrants(:150) open withauth1 := auth.GetAuthorization()followed byif err != nil { return nil, err }, which tests the enclosing function's staleerr(from the earlierAccAddressFromBech32) rather than anything produced in the closure. The check is dead in every call. Present on base; this PR only lifts the closure into a named variable. (Also reported by Codex.)
| // being filled. Filtered and unfiltered offset paths share this guard. | ||
| func (c *offsetScanCursor) checkScanBudgetBeforePageFilled() error { | ||
| if c.scanLimit.enforce && c.phase() != scanPhasePostPage && | ||
| c.scanned > paginationEnd(c.req.offset, c.scanLimit.limit) { |
There was a problem hiding this comment.
[suggestion] The effective scan bound is a multiple of the advertised 10k cap, not 10k.
The pre-page budget allows offset + scanLimit.limit raw entries, and once the page fills checkPostPageBudget grants a fresh scanLimit.limit. Since checkRequest caps both offset and limit at 10k, an untrusted filtered request of {Offset: 10000, Limit: 10000, CountTotal: true} over a dense store scans ~20k entries before the page is complete and up to another ~10k post-page — roughly 30k store reads per request, versus the flat "10k store entries per paginator scan" the CHANGELOG and PR description advertise.
This is bounded (unlike the pre-checkRequest version, which is why I'd call it a suggestion rather than a blocker — Codex rates it blocking), but the number operators will size their rate limits against is wrong. Either bound c.scanned independently of offset and share one budget across both phases, or state the real worst case (offset + 2×limit) in the changelog and in MaxScanLimit's doc comment.
|
|
||
| // checkRequest rejects limit and offset that cannot be served within `limit`. | ||
| func (p scanLimitParams) checkRequest(req pageRequestNorm) error { | ||
| if !p.boundRequest || p.limit == 0 { |
There was a problem hiding this comment.
[suggestion] limit == 0 means two opposite things inside this one file.
Here it means unlimited — checkRequest returns nil and refuses to bound anything. But checkKeyPath (:51) and checkScanBudgetBeforePageFilled (paginate_driver.go:189) read enforce && x > 0 as a zero budget, so scanLimitParams{enforce: true, limit: 0} would reject the very first entry of every scan while accepting any limit/offset the caller asks for.
Nothing produces that combination today — enrichABCIQueryContext maps trusted-scan-limit = 0 to WithQueryScanLimit(false, 0), and paginate_driver_test.go spells unlimited as {enforce: false, limit: 0}. That makes the p.limit == 0 clause here dead, and it advertises a convention the other two readers do not honour. Drop it and let enforce be the only switch, so the field has one meaning.
| if countTotal { | ||
| return false, scanLimitError(p.limit, "use key-based pagination instead") | ||
| } | ||
| return true, nil |
There was a problem hiding this comment.
[suggestion] Two things about this branch are worth pinning down before merge.
Silent truncation is undocumented. With count_total=false, blowing the post-page budget returns stop, the caller breaks, and the response carries NextKey: nil with no error — which every paginating client reads as "end of results". The count_total=true sibling returns InvalidArgument. The CHANGELOG only describes the error case. On base this asymmetry was confined to the V66/precompile paths; this PR extends it to every untrusted ABCI caller, so TestFilteredPaginateStopsPostPageScanWhenCountTotalFalse is pinning a truncation clients cannot detect. Either document it in the upgrade guide or return an error here too.
The error text regressed. scanLimitError says "scanned more than N entries without filling the page", but the countTotal branch above fires precisely when the page was filled and the post-page scan overran. Base distinguished these with "past the end of the page"; passing a distinct hint from checkPostPage restores an accurate message.
Superseded: latest AI review found no blocking issues.
Superseded: latest AI review found no blocking issues.
1 participant
Summary
limit/offset(rejected before any iteration)Behavior
Untrusted callers (any origin not in
query.trusted-cidrs, including localhost by default) during ABCI/gRPC queries:limitoroffsetabove 10k returnsInvalidArgumentbefore the paginator walks the storePaginate/FilteredPaginatealso error if a scan exceeds 10k store entries (sparse filters,count_total=true)Trusted callers (IP matches an entry in
query.trusted-cidrs) receive a configurable cap viaquery.trusted-scan-limit(default 100k;0= unlimited). Indexers that needValidatorDelegations(or other unindexed filtered queries) over large stores should be allowlisted.Origin detection uses the gRPC peer address or JSON-RPC
CallInforemote address. BaseApp enriches the SDK context on the query path with scan-limit state; keepers usePaginateForContext/FilteredPaginateForContextso consensus and EVM precompile paths continue to use the frozen v6.6 paginators (no request-levellimit/offsetreject there).New
[query]section inapp.toml:Startup warns on overly broad CIDR entries (e.g.
0.0.0.0/0).Design: pagination-scan-limits.md
Validation
go test ./sei-cosmos/baseapp/... ./sei-cosmos/server/config/... ./sei-cosmos/types/query/...go test ./sei-cosmos/x/staking/keeper/... ./sei-cosmos/x/gov/keeper/...(representative keeper call-site updates)