migration: openid-client and jose migration#785
Conversation
32b378e to
56b8799
Compare
| const { client, issuer } = await getClient(config); | ||
| const redirectUri = options.redirectUri || this.getRedirectUri(); | ||
| const { configuration } = await getClient(config); | ||
| // Note: In openid-client v6, redirect_uri is automatically extracted from the |
There was a problem hiding this comment.
redirect_uri is still part of the public API and the typedocs still show res.oidc.callback({ redirectUri: ... }).
If we now always derive the redirect URI from the current request URL, apps with custom callback routes or multiple domains may send a different redirect_uri to the token endpoint than the one used during login.
If this is intentional, can we call it out as a breaking change and update the types/docs ?
| // Build host with port if needed | ||
| const host = needsPort ? `${hostname}:${port}` : hostname; | ||
|
|
||
| const currentUrl = new URL(`${protocol}://${host}${req.originalUrl}`); |
There was a problem hiding this comment.
IMO, we should not drop options.redirectUri support here. Even though openid-client v6 can infer the redirect_uri from the URL passed to authorizationCodeGrant(), this SDK can still decide which URL to pass in.
To preserve the existing express-openid-connect API, we should use options.redirectUri when it is supplied, and only infer the callback URL from the request when it is not.
Otherwise, existing apps that rely on res.oidc.callback({ redirectUri }) may break silently, even though the option is still documented and typed.
There was a problem hiding this comment.
yes, I completely agree, the support for options.redirectUri should not have been dropped. I already pointed this out earlier, I guess this got missed to address - #785 (comment)
pre-v6, the redirectUri was constructed from either options.redirectUri or using getRedirectUri method and explicitly passed to openid-client's callback method.
Have restored the same behavior.
There was a problem hiding this comment.
Updated:
- Removed the comment that said
options.redirectUriis no longer used - Added
callbackUrlconstruction aftercurrentUrlis built, restoring theoptions.redirectUri || this.getRedirectUri()fallback chain as earlier. - The
requestobj passed to oidcClient has thiscallcackUrlinstead of original request's url
|
|
||
| if (isCustomStore) { | ||
| const id = existingSessionValue || (await generateId(req)); | ||
|
|
There was a problem hiding this comment.
Can we add coverage for responses that flush headers before res.end() ?. Since session cookies are now written from the res.end wrapper, setCookie() may skip writing the cookie when res.headersSent is already true.
This can happen for streaming responses or routes that call res.write() before res.end(). Previously, on-headers wrote cookies right before headers were sent, so this may be a behavior change where session updates are silently lost.
There was a problem hiding this comment.
Right, this is a limitation introduced by removing on-headers. It can all be attributed to jose-v6 async encryption, which makes our setCookie async too. on-headers only supports synchronous callbacks, so it can no longer be used to write the cookie right before headers are flushed.
I've made the failure explicit with a debug log when headersSent is already true, covering not just streaming responses but any scenario where headers are flushed early (res.write(), res.writeHead(), res.flushHeaders(), res.sendFile()).
As per your suggestion I've also added a test that properly documents and asserts this behavior.
| ); | ||
| } | ||
|
|
||
| return fetch(fetchUrl, { |
There was a problem hiding this comment.
httpAgent is still listed as a public configuration option, but this new fetch implementation doesn’t seem to use it anymore. Some users might depend on it for things like proxies, custom TLS settings, or controlling network access.
Can we either make sure httpAgent is used in this new fetch logic, or clearly document this as a breaking change and update the types and docs?
There was a problem hiding this comment.
yes, support for passing httpAgent has been removed.
The core issue is that httpAgent worked because got (the HTTP library openid-client v4 used internally) had a first-class concept of agents, we could hand it an http.Agent or https.Agent and it would use that for every connection it made.
openid-client v6 replaced got with native fetch, and fetch simply has no equivalent concept, there's no way to pass a Node.js http.Agent into a fetch() call. The two APIs model transport-level control completely differently, so the old option can't be mapped across.
Update:
Reference - PR#787
Instead, we've exposed customFetch as the escape hatch. It gives consumers full control over the underlying HTTP behaviour, whether that's routing through a proxy, pinning a custom TLS certificate, adding retry logic, or just logging outbound requests for debugging. The SDK wraps whatever function is provided to inject required headers (User-Agent, telemetry) before the call goes out, so SDK concerns stay intact regardless of what the consumer does underneath. For the proxy case specifically, undici ships with Node.js 18+ so no extra dependency is needed
On the type side, removed httpAgent from index.d.ts (along with the HttpAgent/HttpsAgent imports) and typed customFetch as typeof fetch, This is broad enough to be compatible with undici and other fetch-compatible implementations.
Also added Example-13 to EXAMPLES.md showing the undici ProxyAgent pattern end-to-end.
For test coverage, added two behavioural tests, one asserting the provided function is actually invoked during discovery, and another confirming the SDK's header decoration still runs before the call reaches the consumer's function.
| "joi": "^17.13.3", | ||
| "jose": "^2.0.7", | ||
| "jose": "^6.1.3", | ||
| "on-headers": "^1.1.0", |
There was a problem hiding this comment.
Since this PR no longer uses on-headers, can we remove it from dependencies ?
If we decide to restore on-headers to preserve the old session-cookie timing behavior, then it should stay. Otherwise this looks like a stale dependency after the session persistence refactor.
There was a problem hiding this comment.
Removed it from dep list.
I know it was perfect at setting the cookie at right moment but don't think we can bring it back, as it expects a synchronous callback and our setCookie method changed to async.
| }); | ||
|
|
||
| it('should timeout for delay > httpTimeout', async function () { | ||
| it.skip('should timeout for delay > httpTimeout', async function () { |
There was a problem hiding this comment.
Can we avoid skipping this test ? httpTimeout is still a public config option, so we should keep coverage that requests actually fail when they exceed the configured timeout.
If timeout behavior changed with fetch/openid-client v6, we should update the assertion to match the new error shape rather than skipping the test entirely.
There was a problem hiding this comment.
I've re enabled and updated the test suite client respects httpTimeout configuration to better cover for all 3 httpTimeout scenarios.
| }); | ||
|
|
||
| describe('client respects clientAssertionSigningAlg configuration', function () { | ||
| describe.skip('client respects clientAssertionSigningAlg configuration', function () { |
There was a problem hiding this comment.
Can we avoid skipping this test suite ? clientAssertionSigningAlg is still a public option, and this PR changes the private key JWT implementation during the openid-client v6 migration.
We should keep coverage that the configured signing algorithm is actually used, even if the test needs to be rewritten around the v6 grant APIs.
There was a problem hiding this comment.
I've re enabled this test suite and updated the tests to cover -
Test 1: should use RS256 as the default signing algorithm in the client assertion
Test 2: should use the configured signing algorithm in the client assertion
Test 3: should fail when signing algorithm is incompatible with the key type
| }); | ||
|
|
||
| describe('client respects httpAgent configuration', function () { | ||
| describe.skip('client respects httpAgent configuration', function () { |
There was a problem hiding this comment.
Can we avoid skipping this test suite ?. httpAgent is still a public config option, but this PR no longer appears to pass it into the HTTP layer.
If httpAgent support is intentionally removed because of the fetch migration, we should document it as a breaking change and update the public types/docs.
Otherwise, this test should be rewritten to verify the new implementation still honors httpAgent.
| }, | ||
| "engines": { | ||
| "node": "^10.19.0 || >=12.0.0 < 13 || >=13.7.0 < 14 || >= 14.2.0" | ||
| "node": ">=20.0.0" |
There was a problem hiding this comment.
Can we keep this in sync with package.json ?
The package declares Node support as ^20.19.0 || ^22.12.0 || >= 23.0.0, but the lockfile root package still says >=20.0.0. Take a look at this:
express-openid-connect/package.json
Line 76 in bfe775e
Since the migration guide says the exact minimum versions matter for require(ESM) support, the lockfile should reflect the same engine range to avoid confusion.
There was a problem hiding this comment.
Addressed it for now,
But we will probably plan the removal of package lock file after after this in a separate PR.
There was a problem hiding this comment.
… be passed to openid-client@6 in callback handler
…e persisted. Add test coverage for these scenarios
…or all the openid-client's netowrk calls
… KeyObject, or JWK without an alg property
This PR needs further review. I am dismissing my approval.
| const issuerRespTypes = Array.isArray(serverMetadata.response_types_supported) | ||
| ? serverMetadata.response_types_supported | ||
| : []; | ||
| issuerRespTypes.map(sortSpaceDelimitedString); |
There was a problem hiding this comment.
.map() returns a new array and does not change issuerRespTypes. Doesn't this mean the comparison on line 186 always uses the original unsorted values ? For example, wouldn't "code id_token" fail to match "id_token code" ?
Should this be:
const sortedRespTypes = issuerRespTypes.map(sortSpaceDelimitedString);
if (!sortedRespTypes.includes(configRespType)) {
There was a problem hiding this comment.
yes, the comparison should have been made on sortedRespTypes result.
Updated.
| * this property will be required. | ||
| */ | ||
| clientAssertionSigningAlg?: | ||
| | 'RS256' |
There was a problem hiding this comment.
The TypeScript type still allows 'ES256K' and 'EdDSA', but the Joi schema in
express-openid-connect/lib/config.js
Lines 316 to 328 in be90b7d
Should we update the type to match the new valid set ('Ed25519' instead of 'EdDSA', and remove 'ES256K') ?
There was a problem hiding this comment.
yes, there was a mismatch between Joi schema for clientAssertionSigningAlg and type def file.
This was mistakenly left unchecked while determining breaking changes.
Updated the index.d.ts to reflect the same.
| this.token_type = | ||
| tokenSet.token_type?.toLowerCase() === 'bearer' | ||
| ? 'Bearer' | ||
| : tokenSet.token_type; | ||
|
|
There was a problem hiding this comment.
token_type normalization already happened during session write time. This is redundant.
…teed to provide crypto.hkdfSync
Summary
Upgrades
openid-clientfrom v4 to v6 andjosefrom v2 to v6. These are now ESM-only packages, which drives the Node.js version requirement change.The core middleware API, session handling, and all authentication methods remain unchanged.
Breaking Changes
See V3_MIGRATION_GUIDE.md for before/after examples and migration steps for each change.
^20.19.0 || ^22.12.0 || >= 23.0.0httpAgentconfig removed — replaced bycustomFetch. PassinghttpAgentthrows at startup.clientAssertionSigningAlgnow required — the implicitRS256default has been removed. Required whenclientAssertionSigningKeyis a PEM, Buffer, KeyObject, or JWK without analgproperty.ES256KandEdDSAremoved from validclientAssertionSigningAlgvalues. UseEd25519instead ofEdDSA.ES256Khas no replacement.afterCallbackbehavior change —req.oidcnow reflects the incoming user's new tokens during the callback, not the previous session state.res.end()time instead. If your route callsres.write(),res.flushHeaders(), orres.writeHead()before the response ends (e.g. SSE, chunked responses), the session cookie will be silently skipped. If you rely on token rotation, avoid using it on streaming routes.clientAssertionSigningKeytype updated — if you use TypeScript, the accepted type forclientAssertionSigningKeyhas changed to align with jose v6 and the Web Crypto API. In practice, all previously supported key formats (PEM string, Buffer, KeyObject, JWK) still work at runtime — only the TypeScript type definition has changedTesting
Manual testing against a real Auth0 tenant
customFetchwiringhttpAgentrejectionTypeError: "httpAgent" is not allowedat startup before the app bootsclientAssertionSigningAlg: 'RS256'client_assertionJWTclientAssertionSigningAlgafterCallbacknew behaviorreq.oidc.userreflects the incoming user's tokens, confirmedsubmatches the new ID tokenres.writeHeadbeforeres.end)Unit tests
Unit tests have been updated to reflect the new API and behaviour.
End-to-end tests
The existing end-to-end test suite covers the full login/logout flow, token refresh, backchannel logout (basic, re-login after logout, custom genid, custom query store), and custom session store scenarios using a local OIDC provider.