fix: close pre-GA decode-hardening gaps - #893
Merged
Merged
Conversation
Pagination was the one parameter the #883 hardening did not cover: it never expands or groups its keys, so a hostile page key was silently ignored instead of rejected. The guard contract is now uniform — a pagination key whose path addresses an inherited prototype member raises the same typed ParseError as the other four parameters, in both dialects (the expression pagination parser subclasses the simple one). Nesting is bounded by the shared traversal depth, mirroring expandObject. BREAKING CHANGE: a pagination key containing a __proto__, constructor or prototype segment now raises a typed ParseError instead of being ignored.
qs treats a leading '?' as part of the first key name, so a caller
passing url.search verbatim decoded to an empty query — every
parameter silently lost ('?filter' is not a known wire key). A single
leading '?' is now stripped before parsing, the same tolerance
URLSearchParams applies. The strip is shared by the codec facade's
stamp dispatch, the dialect decoders' main path and every per-parameter
decode helper, so dialect detection and decoding see the same payload.
FiltersParseOptions.throwOnFailure is honored by the simple and mongo dialects but deliberately ignored by the expression dialect, which always throws on a key resolution failure — an expression cannot be partially reinterpreted safely, since pruning a leaf inside or(...) changes the compound's meaning rather than narrowing it. The guide documents this in several places; the option's own declaration now says it too, so a caller passing throwOnFailure: false to the expression parser is not left wondering why nothing changed.
|
Note Reviews pausedUse the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughThe PR centralizes URL query-string parsing through ChangesCodec and parser hardening
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@packages/codec-url/src/utils/parse.ts`:
- Around line 16-17: Update parseQueryString to validate decoded query keys
before passing them to parse, rejecting constructor and prototype path segments
(and __proto__ consistently) using the typed ParseError.inputInvalid() contract;
ensure nested bracket keys are checked rather than relying only on top-level
Object.keys validation.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 857739ef-0f98-47b4-b48e-481e6547b0d9
📒 Files selected for processing (10)
packages/codec-url/src/decoder/module.tspackages/codec-url/src/module.tspackages/codec-url/src/utils/index.tspackages/codec-url/src/utils/parse.tspackages/codec-url/test/unit/codec.spec.tspackages/codec-url/test/unit/simple-pagination.spec.tspackages/core/src/parser/base.tspackages/core/src/parser/parameter/filters/types.tspackages/parser-simple/src/parameter/pagination/module.tspackages/parser-simple/test/unit/parser/prototype-pollution.spec.ts
Owner
Author
|
@coderabbitai pause |
✅ Action performedReviews paused. |
qs itself drops __proto__/constructor bracket structures (they are Object.prototype members), so those never reach the parser; prototype under a safe parent is the one form qs passes through, and the parser-level guard rejects it. Two pins hold both halves of that boundary on the wire path.
Merged
tada5hi
added a commit
to authup/authup
that referenced
this pull request
Aug 11, 2026
…-beta.20) (#3425) * feat: bump rapiq to 2.0.0-beta.20 and answer decode failures with 400 The release ships schema index declarations (tada5hi/rapiq#895) and closes the pre-GA decode-hardening gaps (tada5hi/rapiq#893): a pagination key carrying a prototype segment now raises a typed ParseError instead of being ignored, and routup's qs passes 'prototype' segments through, so the throw is reachable over HTTP. sanitizeError maps rapiq ParseError/CodecError onto BAD_REQUEST: a decode failure is client wire input, not a server fault. The expression dialect has always thrown on disallowed keys, so those requests answered 500 before this. Pinned over HTTP for both paths. * feat: declare schema indexes and back the query surface with entity indexes Every registered schema declares its `indexes` and opts into the rapiq 2.0.0-beta.20 indexed filters (anchor mode) and sort policies, under one invariant: every allowed filter/sort key LEADS a declared index backed by a real database structure, so enforcement never rejects a query the allow-lists permit. The only narrowing is a multi-key sort without a matching composite prefix, which drops whole-parameter (no UI surface sends one). Migration 1786436332251-QueryIndexes backs the declarations: 126 new indexes covering the query vocabulary, every remaining FK scalar column (junction *_realm_id and policy_id columns, client.access_policy_id, the EA and identity-provider mapping tables; a Postgres-only gap since MySQL keeps implicit FK indexes), the login-throttle composite (actor_name, request_ip_address, created_at) replacing the redundant actor_name single, session_tokens.parent_id (grace-window lookup), and the reset_hash/activate_hash token lookups (sole selective predicate on unauthenticated endpoints). It also drops the three orphaned legacy tables (auth_authorization_codes, auth_refresh_tokens, auth_identity_provider_roles); down() recreates them exactly as the pre-existing chain leaves them. The mysql down() wraps the generated statements with hand-authored FK drop/re-add phases: MySQL silently drops a constraint's implicit index once a created index can serve it, so the plain DROP INDEX would fail with ER 1553 on 50 constraints. assertSchemaIndexesMatchEntity joins the boot-time validation pass (a declared sequence must be a leftmost prefix of a real PK/unique/index; tada5hi/rapiq#898 tracks upstreaming it). Verified on both dialects: run, revert x16, re-run, schema-drift gate, populated round-trip. * docs: record the index declarations and the mysql FK-index down trap Query IR flow gains the index-declaration rules (adding a filterable or sortable key now requires a backing entity index plus migration); conventions and the typeorm reference record that MySQL auto-drops a foreign key's implicit index when a created index can serve the constraint, breaking generated down() paths, and the wrap pattern that restores the exact pre-migration state. * build: bump rapiq to the stable 2.0.0 release A clean re-tag of 2.0.0-beta.20 - no content changes; the full suite re-verified against the resolved release artifacts.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Closes the pre-GA gaps surfaced while validating the FLAME hub against
2.0.0-beta.19(all hub suites green on the beta — these are the residual findings).Pagination joins the prototype-member key guard
Pagination was the one parameter the #883 hardening did not cover: it never expands or groups its keys, so a hostile
pagekey was silently ignored rather than rejected.BaseParsergainsassertSafeObjectKeys()(depth-bounded likeexpandObject), andSimplePaginationParser.parseruns it explicitly — a pagination key with a__proto__/constructor/prototypesegment now raises the same typedParseErroras the other four parameters, in both dialects.BREAKING: such keys previously parsed (ignored); they now throw. Legitimate clients are unaffected.
url.searchtolerance on decodecodec.decode('?filter[name]=a')decoded to an empty query — qs reads?filteras the first key name, so every parameter was silently lost. A single leading?is now stripped before parsing (the toleranceURLSearchParamsapplies), shared by the facade's stamp dispatch, the dialect decoders and every per-parameter helper — so dialect detection and decoding see the same payload.??filterstays unrecognized: only one?is stripped.throwOnFailuredoc on the expression exceptionThe dialect asymmetry on disallowed filter keys (simple prunes per the schema's drop-vs-throw policy; expression always throws) is deliberate and documented in the guide — but
FiltersParseOptions.throwOnFailureitself did not say the expression parser ignores it. The option's declaration now states the exception and its rationale, so a caller passingthrowOnFailure: falseto the expression parser is not left wondering why nothing changed.Verification
npm run build+npm run testgreen on this branch (10 test projects).N:-prefixed and nested own-property forms, no-pollution negative controls,?-tolerance on stamped/unstamped payloads and thedecodePaginationhelper, single-?-only semantics.2.0.0-beta.19with its own capability suite pinning the guard, the 400 mapping, and the codec round trip (PrivateAIM/hub@116dbdb50).Summary by CodeRabbit
Bug Fixes
?across decoding, filtering, sorting, relationships, and pagination.limitandoffsetparsing.Security
Documentation
Tests