badgateway / oauth2-client

OAuth2 client for Node and browsers
https://www.npmjs.com/package/@badgateway/oauth2-client
MIT License
269 stars 31 forks source link

TypeError: Cannot read properties of undefined (reading 'access_token') #151

Closed imunderwater closed 3 weeks ago

imunderwater commented 4 weeks ago

When working with GitHub OAuth2, everything seems to work fine initially. However, when the auth server redirects to the client callback, an error is thrown indicating: TypeError: Cannot read properties of undefined (reading 'access_token'). I tested it with other libraries, such as simple-oauth2, and it works fine. This issue appears to be related to oauth2-client and seems to be a bug that needs to be fixed.

imunderwater commented 4 weeks ago

OAuth2Token should be of type unknown, we don't know what is the response format that the provider return.

@evert can you check this issue please

evert commented 4 weeks ago

Please share your code so I can reproduce this issue! Redact any secrets. I want to see your config + it would also be helpful to see an example of the url you're parsing (also redact any tokens)

imunderwater commented 4 weeks ago
  const client = new OAuth2Client({
    authorizationEndpoint,
    tokenEndpoint,
    clientSecret,
    clientId,
  });
  async function createAuthorizationUri(
    state: string,
    redirect_uri,
  ): Promise<string> {
    const authorizationUri = await client.authorizationCode.getAuthorizeUri({
      redirectUri: redirect_uri,
      state: state,
      scope: ["read:user"],
    });

    return authorizationUri;
  }

  async function validateAuthorizationCode(code: string, redirect_uri): Promise<object> {
      const token = await client.authorizationCode.getToken({ code, redirect_uri });
     return token;
}

its the same code that i got google auth working, but instead for github doesn't work (it throws an error)

evert commented 4 weeks ago

Could you share the configuration values you're using?

imunderwater commented 4 weeks ago

sorry for the late reply @evert here it is.

const client = new OAuth2Client({
    authorizationEndpoint: "https://github.com/login/oauth/authorize",
    tokenEndpoint: "https://github.com/login/oauth/access_token",
    clientSecret,
    clientId,
  });

as the Github oauth documentation.

evert commented 4 weeks ago

I started testing this, and at the very least this line is wrong (Typescript should error on this):

  const token = await client.authorizationCode.getToken({ code, redirect_uri });

Note that redirect_uri should be redirectUri. After that fix I was able to replicate the issue.

So it looks like Github actually 2 bugs, not this library. The issue is that Github replies with a application/x-www-form-urlencoded, when it should reply with application/json. This is non-standard behavior. Luckily it seems that adding an Accept header fixes this and doesn't have any real drawbacks.

A second issue is that Github responds with 200 OK for error =(. Pretty bad! But I can add a workaround for this too.

evert commented 4 weeks ago

I also submitted a bug report here: https://github.com/orgs/community/discussions/135818

Lastly, i think the feedback in this library could be a bit better: TypeError: Cannot read properties of undefined (reading 'access_token) is not a great error for this kind of thing!

imunderwater commented 4 weeks ago

I understand. Given that it works fine with other libraries, do you think the issue lies on oauth2-client or GitHub (or both?) Should I try adding an Accept header to the request?

evert commented 4 weeks ago

It's 100% a Github issue, but adding the Accept header will fix the issue for you (although errors will not yet be correctly processed). I do intend to add these workarounds to this library as well so in the future it's not an issue.

imunderwater commented 4 weeks ago

@evert It would be great if you could add these workarounds to avoid writing extra code. the application/json should be the default for oauth2-client, by adding that this issue will be fixed.

this line should be achieved on the getToken function too

If these headers are the default, the Accept header should be included here

imunderwater commented 4 weeks ago

one more thing OAuth2Token should be keys of unknown, We don't know the response format coming from the token endpoint.

evert commented 4 weeks ago

@evert It would be great if you could add these workarounds to avoid writing extra code. the application/json should be the default for oauth2-client, by adding that this issue will be fixed.

Yes, as I said I intend to add these workarounds.

one more thing OAuth2Token should be keys of unknown, We don't know the response format coming from the token endpoint.

OAuth2 is a standard format. We definitely do know what the format will be.

imunderwater commented 4 weeks ago

@evert how can i get the id_token from the google oauth that the auth server return?

evert commented 4 weeks ago

OpenID Connect support is out of scope for this library. I've started thinking a bit more about dealing with arbitrary properties here #145 but not sure yet how I will deal with that. Changing a type to unknown wouldn't do anything in this case.

imunderwater commented 4 weeks ago

Obtaining the actual response form is useful. I need to use the id_token and scoop, but it's not possible to do so. it would be great if you could achieve that as well.

imunderwater commented 3 weeks ago

hey @evert, any update on extra parameters or the Accept method?, i really need this for my project.

evert commented 3 weeks ago

No update. I will get to it when I can. In the meantime you can:

  1. monkeypatch this
  2. fork and change it
  3. submit a pull request to help move this along. If no tests failed after making this change, add a test.

if you're in a hurry 1) is probably the quickest, and you can just remove that again when this is changed in a future release.

evert commented 3 weeks ago

2 fixes are in, #152 and #153. Closing this! Thanks for the bug report.