Google-api-nodejs-client: `auth.setCredentials` has side effects for all clients in the same event loop.

Created on 12 Feb 2019  路  7Comments  路  Source: googleapis/google-api-nodejs-client

With the upgrade of google-auth-library and the refactor to use entirely googleapis-common, setting credentials on any auth client used in a GoogleApi instance has side effects on all other clients.

See the following code:

const { GoogleApis } = require('googleapis');

function client(value) {
  let auth;

  let client = new GoogleApis();
  auth = new client.auth.OAuth2({
    clientId: value,
    clientSecret: value
  });
  auth.setCredentials({ access_token: value });
  return {
    client: client.gmail({
      version: 'v1',
      auth
    }),
    auth
  };
}

function request(client) {
  return new Promise((resolve, reject) => {
    client.users.drafts.get({ userId: 'me' }).then(resolve).catch(reject)
  });
}

async function checkAuth() {
  const { client: a, auth: authA } = client('1');
  const { client: b, auth: authB } = client('2');
  try { await request(a); } catch (e) {}
  authB.setCredentials({ access_token: '3' });
  try { await request(a); } catch (e) {}
}

checkAuth().then(() => process.exit(0)).catch((e) => { console.log(e); process.exit(1); })

In this example, if you print the credentials of the authClient used in each request, you'll see that while we use client a in both requests, it actually uses the credentials set in client b (console.log(authClient.credentials) there outputs: {access_token:'2'}). This is a result of using GoogleAuth as a singleton, pointed out in both these issues:

This means that if the same event loop happens to interleave creation of a google client with one authorization and making a request with another, it'll use the last authorization credentials set before the request rather than the authorization credentials we passed into the initial client.

Conversely, if we create and set the auth client on a last, we'll update the access token used in the request for b to'3'.

async function checkAuth() {
  const { client: b, auth: authB } = client('2');
  const { client: a, auth: authA } = client('1');
  try { await request(a); } catch (e) {}
  authA.setCredentials({ access_token: '3' });
  try { await request(b); } catch (e) {}
}

checkAuth().then(() => process.exit(0)).catch((e) => { console.log(e); process.exit(1); })

This bug requires an urgent patch as the behavior is unexpected and can produce potentially hazardous results in situations where interleaving execution of the googleapis module may happen and is unexpected given both a new client and auth client are created.

p1 bug

Most helpful comment

So keen to see this get fixed, there are dangerous side effects for our users here!

I'm surprised to see new major versions being shipped with a known bug like this included and no warning, especially as releases of this library usually involve tens of thousands of LOC so they're not practical to review the safety of 馃槥

All 7 comments

The issue started when we recently upgraded google-apis from 30.0 -> 37.0. We don't know the exact version that it regressed.

Adding on here, the global and service-level auth section of the README should be changed to note that you cannot safely use global or service auth in conjunction with per-user access tokens.

Updated from version 30.0 to 37.1.0 as well and noticed a huge number of errors due to this issue. API requests are failing with Delegation denied for [email protected]

+1 on this - this really really nasty. It appears to have been introduced between version 35 and 36

What's the status of fixes for this? I think versions 36 and 37 should be deprecated in the mean time since they produce pretty gnarly and subtle side effects if you're running any sort of application that could interleave execution on the same event loop.

So keen to see this get fixed, there are dangerous side effects for our users here!

I'm surprised to see new major versions being shipped with a known bug like this included and no warning, especially as releases of this library usually involve tens of thousands of LOC so they're not practical to review the safety of 馃槥

Greetings folks! And thanks for being patient here. We did a little root cause analysis, and found the cause of the problem in https://github.com/googleapis/google-api-nodejs-client/pull/1476.

This means that versions 36.0.0 => 39.0.0 went out with this issue. The fix is in #1660, and the release is on its way out in #1661. We are working with npm on an advisory so others know to update to the latest release as well. Version 39.1.0 will have the security fix.

Thanks for sticking with us on this one!

Was this page helpful?
0 / 5 - 0 ratings

Related issues

ACMerriman picture ACMerriman  路  3Comments

suparnavg picture suparnavg  路  3Comments

streamnsight picture streamnsight  路  4Comments

eduDorus picture eduDorus  路  3Comments

oliverjessner picture oliverjessner  路  3Comments