Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Google authentication just started failing with the passport strategy #125

Closed
morungos opened this issue Sep 27, 2018 · 4 comments
Closed

Comments

@morungos
Copy link

morungos commented Sep 27, 2018

I've been using the module and the passport strategy happily for a while now, but I think it just started failing, and the issue is in the POST to /token, so it's deeper in the innards than I'm used to. I'll document what I've discovered so far, in case anyone else comes across this.

As far as I can tell, this is a new issue that manifested in the last 24 hours, on a server that was not touched. It's also reproduced in my development environment. It's showing in versions from 2.4.1 back to 2.2.1.

The initial symptom is an error in a catch error handler:

(node:11942) UnhandledPromiseRejectionWarning: TypeError: error.error.startsWith is not a function
    at callback.then.catch (/Users/stuart/git/api-server/node_modules/openid-client/lib/passport_strategy.js:169:29)
    at <anonymous>
    at process._tickCallback (internal/process/next_tick.js:188:7)

But I think this may simply be masking the issue, by the error returned from /token not being in the right place in the response code logic for the passport handlers.

As far as I can tell, all goes well in the redirect and back, until it gets to the POST to /token. The error returned from Google is:

{ error: 
      { code: 400,
        message: 'Request contains an invalid argument.',
        status: 'INVALID_ARGUMENT' } }

The POST request for /token contains approximately the following:

TOKEN { body: 
   { grant_type: 'authorization_code',
     code: '<< big string>>',
     redirect_uri: 'http://localhost:3000/api/authentication/callback',
     state: '<<string>>' } }

I was suspicious about the state, because it doesn't seem to be a standard part of the authorization code flow, which as far as I can tell is what is happening here. So I hacked the code briefly to remove that one field state, but it made no difference.

A suspicion? Maybe the client key and secret need to be added (based on this: https://aaronparecki.com/oauth-2-simplified/#web-server-apps) and this is what's showing. The Google error is desperately unhelpful, and this is where some logging would have made a big difference to debugging this.

@panva
Copy link
Owner

panva commented Sep 27, 2018

Hi @morungos,

this is 100% unrelated to a passport strategy (apart from the error handler, which i'll address below) and I am unable to reproduce with a standard implementation.

{ error: 
      { code: 400,
        message: 'Request contains an invalid argument.',
        status: 'INVALID_ARGUMENT' } }

I'm familiar with this google payload envelope. is_standard_body_error shouldn't return true for such envelope. This is actually further masking the response for you since i'm guessing there's also a top level member detail in that response. I'll fix that, this shouldn't be returned as an OpenIdConnectError at all FWIW. This will also make it so that the strategy will fall into error and not fail.

Providing state to the token endpoint (draft-ietf-oauth-mix-up-mitigation) was removed in v2.4.0 as it's no longer a BCP.

To aid in debugging this on your end you might look at #90 (comment) and if you're getting an OpenIdConnectError instance as a result of a backend call you can inspect the err.response property that gives you the full http response.

panva added a commit that referenced this issue Sep 27, 2018
```
{ error:
      { code: 400,
        message: 'Request contains an invalid argument.',
        status: 'INVALID_ARGUMENT' } }
```
This is not an OpenIdConnectError and will no longer be returned as
such, this will now fall under throwing HTTPError instances instead.

See #125
@panva
Copy link
Owner

panva commented Sep 27, 2018

v2.4.2 released

@panva
Copy link
Owner

panva commented Sep 27, 2018

and here's your problem :) https://twitter.com/busymac/status/1045112552626298880

@panva panva closed this as completed Sep 27, 2018
@morungos
Copy link
Author

Wow, thanks @panva -- I'd had a long day and didn't have the processing power to pursue it further. Despite your guess (mine too, I added some extra logging to poke inside the response) the body did not contain any other fields, the entire body was the single error object above. That was a huge part of the problem: the total lack of meaningful error information in the body.

Fortunately the Twitter thread does point to a Google snafu, and all is now functioning normally again. Many thanks for the error handling fix, and I'll put a little more effort into making sure I handle weird error cases at my end.

@github-actions github-actions bot locked and limited conversation to collaborators Apr 5, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

No branches or pull requests

2 participants