refactor(core,express): move the passthrough proxy into core - #128
Merged
Conversation
The 33 organizations, step-up, TOTP, users, and admin passthrough routes existed only inside the Express adapter, so a new adapter would have had to rebuild both the upstream call and the session gate that guards it. Core now exports proxyRequest for the call, checkProxyIdentity for the gate, and a single buildQueryString replacing three builders that had drifted apart. The Express proxy handler is now a gate check, a call, and a response, and createServer.ts drops from 705 to 667 lines. fix: a repeated query parameter reached the auth API joined into one comma-separated value on the admin and internal-metrics routes. GET /admin/auth-events?type=login&type=logout was forwarded as type=login,logout, and the API's AuthEventQuerySchema accepts type as an array, so the joined value matched no event type and the filter silently returned the wrong set. Arrays are now forwarded as repeated parameters everywhere, and nested objects are dropped rather than reaching the API as [object Object]. Also records, in the still-unreleased changeset for the response contract, that empty success responses no longer carry a JSON content type. That followed from routing every handler through applyResult and was not called out at the time. Refs #72
6 tasks
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.
PR 3 of #72, the last of the high-leverage items.
Problem
The 33 organizations, step-up, TOTP, users, and admin passthrough routes existed only inside
proxyWithIdentityin the adapter, with no core equivalent. A new adapter would have had to rebuild both the upstream call and the session gate that guards it.Change
Core now exports:
proxyRequest({ authServerUrl, path, method, authorization, serviceAuthorization, forwardedClientIp, query, body })checkProxyIdentity({ subject, cookies, identity, ...cookieNames })undefinedbuildQueryString,buildUpstreamUrlThe Express proxy handler is now a gate check, a call, and a response.
createServer.tsdrops from 705 to 667 lines (56 insertions against 85 deletions), and the adapter loses 38 lines net while core gains the reusable parts.The querystring builders had drifted, and one was wrong
"Fold in the querystring builder duplicated 3x" turned out not to be a pure dedupe. The three copies disagreed:
admin/internalMetrics{ type: ["login","logout"] }type=login%2Clogouttype=login&type=logout{ filter: { from: "x" } }filter=%5Bobject+Object%5D{ limit: 10 }limit=10admin.tspassesreq.querythrough raw, so the first row is reachable on a live route. The auth API'sAuthEventQuerySchemaexplicitly acceptstypeas an array:So
GET /admin/auth-events?type=login&type=logoutwas forwarded astype=login,logout, which matches no event type, and the filter silently returned the wrong set rather than erroring. Confirmed against a running adapter, not just by reading:Unified on repeated parameters, with numbers and booleans coerced for core's typed callers and non-scalars dropped so nothing can reach the API as
[object Object].QueryInputis deliberatelyRecord<string, unknown>, because adapters hand over whatever their framework parsed and Express'sParsedQscan nest. The compiler caught this: an earlier stricter type rejectedreq.query, which was the honest signal that the value really can be a nested object.Verification
Fifteen proxy scenarios captured on
mainand on this branch: every identity rejection path, GET/POST/DELETE passthrough, scalar, repeated and nested queries, no query, upstream 4xx, and empty bodies at 200 and 204. Captured the upstream URL, method, and body as well as the response status, content type, and body.Exactly two differences, both intended: the repeated-parameter fix above, and the content-type note below. Everything else byte-identical.
A correction to PR 2
While verifying the empty-body path I found that #127 changed something I did not catch or disclose. A route whose upstream returned success with no body previously sent
Content-Type: application/jsonwith a zero-length body, because the handler calledres.json(undefined). Routing everything throughapplyResultmade those responses end without a body, so they now send no content type.Confirmed by probing
17c5487(pre-#127) againstmain:My #127 probe covered empty failure bodies but not empty success bodies, which is why it passed clean.
Nothing is released yet, so this PR amends the still-pending
.changeset/core-applies-results.mdto state it rather than letting it ship undocumented.Content-Length: 0is unchanged, and a client reading the body sees nothing either way, since parsing an empty body fails regardless of content type. The new behavior is the consistent one, so I have kept it rather than reintroducing the split. Say the word if you would rather restore the old content type.New tests
packages/core/tests/proxyRequest.test.js: query building including the repeated-parameter case and the[object Object]case, URL joining, everycheckProxyIdentityaccept and reject path including one identity's cookie not satisfying another, andproxyRequestforwarding, GET body omission, and failure passthrough.packages/express/tests/proxyQueryForwarding.test.js: locks repeated parameters end to end on both an admin route and a passthrough route, asserting the exact upstream URL.Checks
pnpm buildclean.pnpm testpasses: 48 suites, 280 tests (up 24).