fix(proxy): forward the query string on the two routes that dropped it - #158
Merged
Conversation
GET /admin/users and GET /internal/auth-events/login-stats were built without a query, so both adapters called the auth API with a bare path. Every other route that reads query parameters forwarded them, which is what made these two hard to notice: the endpoints answered 200 with the wrong page or the wrong window, and nothing reported an error. The visible effect was that the admin dashboard's user search and paging did nothing. It sent ?search=...&limit=10&offset=N, the adapter forwarded /admin/users, and the API answered with its default first 50 users. The login statistics panel ignored its time range the same way, so the range control on the Security screen moved every other panel and not that one. The dashboard's own e2e tests pass because its Playwright mock intercepts at the dashboard boundary and never exercises an adapter, which is why nothing caught this. getUsersHandler and getLoginStatsHandler take an optional query like the other list handlers now, which is additive for anyone calling core directly, and both adapters pass the incoming query through. Both adapters are held to one table of every route the auth API reads query parameters on, rather than a test per route. The failure this guards against is silent, so a route added to an adapter without forwarding its query now fails a test instead of shipping quiet. The table was checked by reverting each fix and confirming the matching case fails.
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.
Found while checking which callers send a large
limitfor fells-code/seamless-auth-api#299.The bug
GET /admin/usersandGET /internal/auth-events/login-statswere built without a query, so both adapters called the auth API with a bare path. Every other route that reads query parameters forwarded them, which is exactly what made these two hard to notice: the endpoints answered200with the wrong page or the wrong window, and nothing reported an error.I swept every query-consuming route in the auth API against the built Express adapter rather than reading the code. Before:
Those eight are the complete set: they are every path in the API's
openapi.jsoncarrying query parameters, cross-checked against everyreq.queryreader in its controllers.GET /magic-linkalso takesredirectUribut is not exposed through the adapter, which routes/magic-link/verify/:tokeninstead.What users saw
The admin dashboard's user search and paging did nothing. It sends
?search=...&limit=10&offset=N, the adapter forwarded/admin/users, and the API answered with its default first 50 users. The Users screen therefore rendered 50 rows while its pager claimed a page size of 10, and Next re-fetched the same rows.The login statistics panel ignored its time range the same way, so the range control on the Security screen moved every other panel and not that one.
The dashboard's e2e tests pass because its Playwright mock intercepts at the dashboard boundary and never exercises an adapter. That is why nothing caught either of these.
The fix
getUsersHandlerandgetLoginStatsHandlerin@seamless-auth/corenow take an optionalquerylike the other list handlers, which is additive for anyone calling core directly. Both adapters pass the incoming query through. Five changed lines in total.The test
Rather than two one-off tests, each adapter is now held to one table of every route the auth API reads query parameters on. The failure this guards against is silent, so a route added to an adapter without forwarding its query fails a test instead of shipping quiet.
I checked the table is load-bearing by reverting each fix and confirming the matching case fails: reverting the Express users fix fails 1 of 8 there, reverting both Fastify entries fails 2 of 8, and reverting the Express login-stats fix fails 2 of 8.
The parity suite is deliberately not the home for this. It compares Express and Fastify responses to each other, and both adapters were equally broken here, so it passed throughout.
Release ordering
Independent of fells-code/seamless-auth-api#299 and safe in either order. Worth noting the combined effect once both land: a caller whose window was previously discarded will get the page it actually asked for rather than the API's default, and a query the API rejects will surface as a
400rather than being silently dropped. The changeset says so.Verification
pnpm buildclean across all three packagespnpm test: core 246, express 167 (up from 159), fastify 62 (up from 54), all passing