panva / openid-client

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

Google authentication just started failing with the passport strategy #125

Closed morungos closed 6 years ago

morungos commented 6 years ago

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 commented 6 years ago

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 https://github.com/panva/node-openid-client/issues/90#issuecomment-383278314 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 commented 6 years ago

v2.4.2 released

panva commented 6 years ago

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

morungos commented 6 years ago

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.