Skip to content

refactor(core,express): move the passthrough proxy into core - #128

Merged
Bccorb merged 1 commit into
mainfrom
refactor/core-proxy-request
Jul 30, 2026
Merged

refactor(core,express): move the passthrough proxy into core#128
Bccorb merged 1 commit into
mainfrom
refactor/core-proxy-request

Conversation

@Bccorb

@Bccorb Bccorb commented Jul 30, 2026

Copy link
Copy Markdown
Contributor

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 proxyWithIdentity in 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:

Export Purpose
proxyRequest({ authServerUrl, path, method, authorization, serviceAuthorization, forwardedClientIp, query, body }) forwards a request, returns upstream status and body unchanged
checkProxyIdentity({ subject, cookies, identity, ...cookieNames }) the session gate, returning a rejection or undefined
buildQueryString, buildUpstreamUrl one builder replacing three

The Express proxy handler is now a gate check, a call, and a response. createServer.ts drops 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:

Input core admin/internalMetrics adapter
{ type: ["login","logout"] } type=login%2Clogout type=login&type=logout
{ filter: { from: "x" } } filter=%5Bobject+Object%5D dropped
{ limit: 10 } limit=10 dropped

admin.ts passes req.query through raw, so the first row is reachable on a live route. The auth API's AuthEventQuerySchema explicitly accepts type as an array:

type: z.union([AuthEventTypeEnum, z.string(), z.array(...)]).optional()

So GET /admin/auth-events?type=login&type=logout was forwarded as type=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:

before: /admin/auth-events?type=login%2Clogout&limit=5
after:  /admin/auth-events?type=login&type=logout&limit=5

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]. QueryInput is deliberately Record<string, unknown>, because adapters hand over whatever their framework parsed and Express's ParsedQs can nest. The compiler caught this: an earlier stricter type rejected req.query, which was the honest signal that the value really can be a nested object.

Verification

Fifteen proxy scenarios captured on main and 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/json with a zero-length body, because the handler called res.json(undefined). Routing everything through applyResult made those responses end without a body, so they now send no content type.

Confirmed by probing 17c5487 (pre-#127) against main:

before PR2: {"status":200,"contentType":"application/json; charset=utf-8","len":"0"}
after  PR2: {"status":200,"contentType":null,"len":"0"}

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.md to state it rather than letting it ship undocumented. Content-Length: 0 is 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, every checkProxyIdentity accept and reject path including one identity's cookie not satisfying another, and proxyRequest forwarding, 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 build clean. pnpm test passes: 48 suites, 280 tests (up 24).

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
@Bccorb
Bccorb merged commit d17896b into main Jul 30, 2026
2 checks passed
@Bccorb
Bccorb deleted the refactor/core-proxy-request branch July 30, 2026 00:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant