panva / openid-client

OAuth 2 / OpenID Connect Client API for JavaScript Runtimes
MIT License
1.83k stars 392 forks source link

parameter not being used in `authorizationCodeGrant` #712

Closed terrymun closed 1 week ago

terrymun commented 1 week ago

What happened?

We are refactoring our code that uses the API from v5 → v6, but ran into an issue with the authorizationCodeGrant function (which is the equivalent of client.callback method in v5).

When trying to pass the URL search params into the parameter positional argument, we get an "invalid response encountered" error from our issuer. When these parameters are provided as part of the URL then the error is resolved.

Version

v6.1.1

Runtime

Node.js

Runtime Details

node@20.17.0

Code to reproduce

Legacy code using API from v5.7.0

This is the code that worked for us on v5:

const params = client.callbackParams(callbackUrl);

return await client.callback(currentUrl, params, {
  code_verifier: codeVerifier,
  state: this.#state,
});

Migrated code using API from v6.1.1

const { searchParams } = new URL(callbackUrl);

// NOTE: This does not work
await authorizationCodeGrant(
  clientConfiguration,
  currentUrl,
  {
    pkceCodeVerifier: codeVerifier,
    idTokenExpected: true,
    expectedState: this.#state,
  },
  searchParams,
);

// NOTE: This works if we manually copy over all the search params into `currentUrl`:
const currentUrlWithSearchParams = new URL(currentUrl);
for (const [key, value] of searchParams.entries()) {
  currentUrlWithSearchParams.searchParams.set(key, value);
}
await authorizationCodeGrant(
  clientConfiguration,
  currentUrlWithSearchParams,
  {
    pkceCodeVerifier: codeVerifier,
    idTokenExpected: true,
    expectedState: this.#state,
  }
);
The error message I get (just by logging the entire `Error` object): ```text ClientError: invalid response encountered at e (webpack-internal:///(rsc)/../../node_modules/openid-client/build/index.js:123:12) at errorHandler (webpack-internal:///(rsc)/../../node_modules/openid-client/build/index.js:146:23) at authorizationCodeGrant (webpack-internal:///(rsc)/../../node_modules/openid-client/build/index.js:684:28) at Authenticator.codeGrant (webpack-internal:///(rsc)/../../packages/account/src/server/authenticator.ts:100:101) ... 19 lines matching cause stack trace ... at async Server.requestListener (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/lib/start-server.js:141:13) { code: 'OAUTH_INVALID_RESPONSE', [cause]: OperationProcessingError: response parameter "iss" (issuer) missing at OPE (webpack-internal:///(rsc)/../../node_modules/oauth4webapi/build/index.js:179:12) at Module.validateAuthResponse (webpack-internal:///(rsc)/../../node_modules/oauth4webapi/build/index.js:2120:15) at authorizationCodeGrant (webpack-internal:///(rsc)/../../node_modules/openid-client/build/index.js:681:78) at Authenticator.codeGrant (webpack-internal:///(rsc)/../../packages/account/src/server/authenticator.ts:100:101) at process.processTicksAndRejections (node:internal/process/task_queues:95:5) at async authCallback (webpack-internal:///(rsc)/../../packages/account/src/routes/auth-callback.ts:44:56) at async /[REDACTED_REPO_ROOT]/node_modules/next/dist/compiled/next-server/app-route.runtime.dev.js:6:55778 at async eO.execute (/[REDACTED_REPO_ROOT]/node_modules/next/dist/compiled/next-server/app-route.runtime.dev.js:6:46527) at async eO.handle (/[REDACTED_REPO_ROOT]/node_modules/next/dist/compiled/next-server/app-route.runtime.dev.js:6:57112) at async doRender (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/base-server.js:1352:42) at async cacheEntry.responseCache.get.routeKind (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/base-server.js:1574:28) at async DevServer.renderToResponseWithComponentsImpl (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/base-server.js:1482:28) at async DevServer.renderPageComponent (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/base-server.js:1908:24) at async DevServer.renderToResponseImpl (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/base-server.js:1946:32) at async DevServer.pipeImpl (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/base-server.js:921:25) at async NextNodeServer.handleCatchallRenderRequest (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/next-server.js:272:17) at async DevServer.handleRequestImpl (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/base-server.js:817:17) at async /[REDACTED_REPO_ROOT]/node_modules/next/dist/server/dev/next-dev-server.js:339:20 at async Span.traceAsyncFn (/[REDACTED_REPO_ROOT]/node_modules/next/dist/trace/trace.js:154:20) at async DevServer.handleRequest (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/dev/next-dev-server.js:336:24) at async invokeRender (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/lib/router-server.js:173:21) at async handleRequest (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/lib/router-server.js:350:24) at async requestHandlerImpl (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/lib/router-server.js:374:13) at async Server.requestListener (/[REDACTED_REPO_ROOT]/node_modules/next/dist/server/lib/start-server.js:141:13) { code: 'OAUTH_INVALID_RESPONSE', [cause]: { parameters: URLSearchParams {} } } } ```

Required

panva commented 1 week ago
[cause]: { parameters: [Object] }

Can you expand this?

terrymun commented 1 week ago

@panva Sure thing. The error message for the obscured [object] I got is as follow:

code: 'OAUTH_INVALID_RESPONSE',
[cause]: { parameters: URLSearchParams {} }

And the new URL(callbackUrl) looks like this:

URL {
  href: 'http://localhost:3000/auth/auth-callback?code=E339437161E1F53C3DFDC78F0EEF1B23EDAEE789761D14BBAFD335CAFF62CDE6-1&scope=openid%20profile%20email%20dob%20offline_access&state=http%3A%2F%2Flocalhost%3A3000&session_state=3-sjwF_uZIt10iqEWx0n0KacI18g2gt03ZPTaus5f7c.FD2D70F633B1FE225BE072E7D90F47ED&iss=https%3A%2F%2Fidentity.webqa.lego.com',
  origin: 'http://localhost:3000',
  protocol: 'http:',
  username: '',
  password: '',
  host: 'localhost:3000',
  hostname: 'localhost',
  port: '3000',
  pathname: '/auth/auth-callback',
  search: '?code=E339437161E1F53C3DFDC78F0EEF1B23EDAEE789761D14BBAFD335CAFF62CDE6-1&scope=openid%20profile%20email%20dob%20offline_access&state=http%3A%2F%2Flocalhost%3A3000&session_state=3-sjwF_uZIt10iqEWx0n0KacI18g2gt03ZPTaus5f7c.FD2D70F633B1FE225BE072E7D90F47ED&iss=https%3A%2F%2Fidentity.webqa.lego.com',
  searchParams: URLSearchParams {
    'code' => 'E339437161E1F53C3DFDC78F0EEF1B23EDAEE789761D14BBAFD335CAFF62CDE6-1',
    'scope' => 'openid profile email dob offline_access',
    'state' => 'http://localhost:3000',
    'session_state' => '3-sjwF_uZIt10iqEWx0n0KacI18g2gt03ZPTaus5f7c.FD2D70F633B1FE225BE072E7D90F47ED',
    'iss' => 'https://identity.webqa.lego.com' },
  hash: ''
}
panva commented 1 week ago

And is new URL(callbackUrl) the assigned value to currentUrl?

terrymun commented 1 week ago

@panva We have to use a different currentUrl than the callbackUrl, due to restrictions with our issuer that only supports a very specific allowlist. We are using a proxy for the currentUrl.

My confusion is that when I copy all the search params from callbackUrlcurrentUrl, then it works. When I don't and provide the search params as a positional argument, then it fails. It seems that the parameter positional argument is being ignored.

panva commented 1 week ago

Ahhh, gotcha, I didn't see the positional parameters argument use in your code 🤦 .

This is expected behaviour then, the only means of passing the authorization response is with the currentUrl positional argument that can be a URL or Response. The parameters positional argument is for

Additional parameters that will be sent to the token endpoint, typically used for parameters such as resource.

https://github.com/panva/openid-client/blob/v6.1.1/docs/functions/authorizationCodeGrant.md#parameters

terrymun commented 1 week ago

@panva The positional argument is being used here:

const { searchParams } = new URL(callbackUrl);

await authorizationCodeGrant(
  clientConfiguration,
  currentUrl,
  {
    pkceCodeVerifier: codeVerifier,
    idTokenExpected: true,
    expectedState: this.#state,
  },
  searchParams, // Over here :)
);

This is expected behaviour, the only means of passing the authorization response is with the currentUrl positional argument that can be a URL or Response.

Thanks for the clarification :) it is my mistake, I didn't know what parameters is used for and I thought it works the same way as the params argument in the legacy client.callback() function.

panva commented 1 week ago

Sure thing. I'll probably rename parameters to tokenEndpointParameters or similar.

terrymun commented 1 week ago

Thanks @panva for your help! Closing it as it has cleared my doubts.

p/s: kudos on the refactoring to ESM, loving it!

panva commented 1 week ago

Thanks @panva for your help! Closing it as it has cleared my doubts.

p/s: kudos on the refactoring to ESM, loving it!

Thank you for the kind words.

P.S. (jokingly) I would humbly accept and proudly display set 75276 in my office

IMG_0083