node-oauth throws intermittent InternalOAuthErrors on fast connections

[8bitDesigner]

Hey all, I'm spinning up a new project using Passport and the Google OAuth 2.0 strategy, and I've spent the last day troubleshooting a particularly weird issue. Namely, when try to log in, locally, I get a InternalOAuthError the majority of the time, and occasionally get a successful login.

My coworker who's on a slower connection, does not have this issue.

After much speelunking through the node-oauth codebase, I've found the issue here: https://github.com/ciaranj/node-oauth/blob/master/lib/oauth2.js#L124-L163

What's happening is that for some requests, Google responds with an early close (eg: sends a no-content header and immediately closes the connection). node-oauth handles this cleanly by immediately triggering the callback (instead of waiting for an end event). However, when Google's servers then perform a connection reset, an error event is triggered, and node-oauth naively calls the callback again resulting in an InternalOAuthError being thrown.

So, why post this bug here? node-oauth appears to be a dead project (the last commit was in 2017), and this bug is going to impact users of passport-google-oauth2 on fast connections. Hopefully by posting this issue here, other folks will be able to quickly diagnose it and find a workaround.

[Rishabh570]

I'm currently in the middle of this and have opened an issue on node-auth.

@8bitDesigner I tried with a "Slow 3G" network from chrome but the issue is still there. Not able to pinpoint how to reproduce success & failure scenarios.

[8bitDesigner]

Ah, the bug occurs when doing the code exchange back to Google from the server, so the "Slow 3G" setting in Chrome won't effect it, as that only slows the client connection to Google, not the server's. I'm lucky enough that I can consistently reproduce this on my fibre connection when running the server locally on my Mac. However, when running via a Docker container, the container introduces enough latency that I don't run into the ECONNRESET. Alternately, switching from Ethernet/WiFi to a tethering over a cellphone will also work around it.

Our workaround for the time being has been to use patch-package to path node-oauth post-install.

[Rishabh570]

Oh, got it! Not sure if I want to walk away from Passport implementation altogether (I use it for multiple social providers). Hitting the Google APIs directly seems to be working just fine (using the token endpoint after getting the code in query params).

I was not aware of patch-package, looks like If I go with Passport, I'll use it to change the request.on('error') implementation to ignore ECONNRESET error. So far, it is consistently working:

   request.on('error', function(e) {
-    callbackCalled= true;
-    callback(e);
+    if (e.code !== 'ECONNRESET') {
+      callbackCalled= true;
+      callback(e);
+    }
   });

[8bitDesigner]

Passport's fine, it's just a grand-child dependency in node-oauth that's not handling ECONNRESET from Google correctly. Here's the patch I ended up using:

diff --git a/node_modules/oauth/lib/oauth2.js b/node_modules/oauth/lib/oauth2.js
index 77241c4..42dd372 100644
--- a/node_modules/oauth/lib/oauth2.js
+++ b/node_modules/oauth/lib/oauth2.js
@@ -158,6 +158,7 @@ exports.OAuth2.prototype._executeRequest= function( http_library, options, post_
     });
   });
   request.on('error', function(e) {
+    if (callbackCalled) { return }
     callbackCalled= true;
     callback(e);
   });

Basically, what's happening is that the success or error callback is being called first, and then when the connection gets reset, it triggers the error handler. Since the callback has already been called, we can treat it like a no-op.

[8bitDesigner]

Ah shoot, it doesn't look like I linked this in the original issue, but there is a PR to fix this behaviour in node-oauth:

ciaranj/node-oauth#363

[Rishabh570]

Yeah, would be great if that PR gets merged. Will be using the patch in the meantime. Thanks 🙌

[shayneczyzewski]

Great find, I am hitting this as well. Thanks for the patch! Looking at the commit history, I do worry node-oauth may be unmaintained now though... 🤔 Will have to try out the patch-package approach. Thanks!

[ciaranj]

node-oauth 0.10.0 now contains the aforementioned pull-requests.

[8bitDesigner]

🙇

[8bitDesigner]

Thank you, mate.

[jaredonline]

I just spent quite a bit of time trying to track down this error and bumping node-oath to 0.10.0 fixes it. Can we get a dependency update in passport-google-oauth2 to match?

[serniebanders]

I just spent quite a bit of time trying to track down this error and bumping node-oath to 0.10.0 fixes it. Can we get a dependency update in passport-google-oauth2 to match?

If anyone is of need of a fix now, I have a working fork.

You can use this as a dependency: git+https://github.com/serniebanders/passport-google-oauth2.git

[mikestead]

I fixed by adding this override to project.json. Overrides available from npm v8.3.0

"overrides": {
    "passport-oauth2": {
      "oauth": "^0.10.0"
    }
  }