fix(server-core): declare a sort allow-list for the client-scope schema - #3447
Conversation
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Plus Run ID: 📒 Files selected for processing (7)
Included review availability: Your plan includes up to 1 review per rolling hour; 0 remain after this review. 📝 WalkthroughWalkthroughThe PR adds indexed timestamp sorting for client scopes, validates unknown sort-key removal across schemas, documents the invariant, and adds regression coverage for pagination offset reset behavior. ChangesQuery sorting safeguards
Entity collection pagination
Estimated code review effort: 3 (Moderate) | ~20 minutes Merge Risk: ⚪ Minimal · up to The PR adds the client-scope sort allow-list and supporting indexes while pinning the offset-reset behavior in tests; no actionable merge-blocking risk remains after normal checks and review. Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Warning There were issues while running some tools. Please review the errors and either fix the tool's configuration or disable the tool if it's a critical failure. 🔧 ESLint
ESLint install timed out. The project may have too many dependencies for the sandbox. 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 |
authup
@authup/client-account-console
@authup/client-admin-console
@authup/client-auth-console
@authup/server-core
@authup/access
@authup/client-web-kit
@authup/client-web-kit-theme
@authup/client-web-nuxt
@authup/client-web-theme
@authup/core-http-kit
@authup/core-kit
@authup/core-realtime-kit
@authup/errors
@authup/i18n
@authup/kit
@authup/server-adapter-kit
@authup/server-adapter-node
@authup/server-adapter-socket-io
@authup/server-adapter-web
@authup/server-kit
@authup/server-test-kit
@authup/specs
commit: |
Audit outcomeReviewed adversarially against the runtime rather than the diff. Verdict: sound, no defects. The reviewer executed the real
Acted on since
Noted, not changed
|
`clientScopeSchema` was the only registered schema with no `sorts` block, so rapiq fell back to a syntactic name check: an arbitrary root sort key survived decode, reached `ORDER BY` on a column that does not exist, and `sanitizeError` mapped the driver rejection to a 500 where every sibling endpoint fails soft and returns unsorted rows. Allow-list every column the schema already indexes rather than the narrow junction-sibling triple. `default`, `clientId`, `scopeId` and the two owner-realm keys sort correctly today through the missing-allow-list fallback, and a narrow list would demote them from working-and-sorted to silently unsorted. `createdAt` / `updatedAt` were the only listed keys without a backing index, hence the migration. Pin the property per schema rather than per endpoint: the new case decodes a bogus key through the real codec for all 26 registered schemas, so the next schema that forgets the block fails the suite instead of one endpoint. Verified differentially (1 failure without the schema change). Closes #3441
From the audit of this branch: #3443 shipped with no test, and the manager is where a regression would reappear. The case drives the manager to page 3, then hands it the shape the sessions page uses (an assembled Query carrying filters plus `pagination: { offset: 0 }`) and asserts the request returns to the first page with the retained page size intact. Verified by dropping the reset from the load input: the request then carries offset 20, which is the reported bug. Also records in the client-scope schema itself why its sort allow-list is wider than its junction siblings', which until now lived only in the commit body and the architecture doc.
bd5b0a5 to
969e91a
Compare
Closes #3441
The sessions-page half of this PR (#3443) landed separately via #3462, so that commit was dropped in a rebase onto master. What remains is the client-scope fix plus the kit spec that pins the offset-reset contract at the collection manager (the place a regression would reappear; #3462 shipped without one).
#3441—clientScopeSchemadeclared no sort allow-listIt was the sole outlier among the 26 registered schemas (missed by the
#3425sweep). Without the block rapiq falls back to a syntactic name check, soGET /client-scopes?sort=totallyBogusColumnreachedORDER BY clientScope.totallyBogusColumnand surfaced as a 500, where every sibling endpoint fails soft and returns unsorted rows.The allow-list is wide on purpose.
default,clientId,scopeId,clientRealmIdandscopeRealmIdall sort correctly today through the missing-allow-list fallback and are all already indexed, so the narrow junction-sibling triple would have demoted them from working-and-sorted to silently unsorted.createdAt/updatedAtwere the only listed keys with no backing index, which is what the migration adds.Coverage
The new case is per schema, not per endpoint: a per-endpoint test would leave the next schema that forgets the block equally unguarded. It decodes
{ sort: 'totallyBogusColumn' }through the real codec for all 26 schemas and asserts the key is stripped.Verified differentially: with the schema change reverted the suite reports exactly one failure (
should strip an unknown sort key for clientScope); with it, the cases pass.Kit spec: the offset reset a filter load has to carry
entity-collection.spec.tsdrives the manager to page 3, then hands it the shape a filter control uses (an assembled Query carryingfilterspluspagination: { offset: 0 }) and asserts the request returns to the first page with the retained page size intact. Dropping the reset from the load input makes the request carry offset 20, the #3443 bug.Verification (after the rebase)
apps/server-corefull suite: 196 files / 2214 tests passed;indexed-invariant.spec.ts+console-search-surface.spec.ts: 70 passed; kitentity-collection.spec.ts: 16 passedmigration run/revert/runclean,test:schema-driftmatches;1786631686318-ClientScopeSortIndexesis the newest migration (master's1786436332251-QueryIndexesshipped in beta.60/61, so a new file is correct)auth_client_scopesonly, and neither column is a foreign key, so the MySQL implicit-FK-indexdown()trap does not applycheck:typesclean, eslint cleanSummary by CodeRabbit