Navigation Menu

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

auth.setCredentials has side effects for all clients in the same event loop. #1594

Closed
ghmeier opened this issue Feb 12, 2019 · 7 comments · Fixed by #1660
Closed

auth.setCredentials has side effects for all clients in the same event loop. #1594

ghmeier opened this issue Feb 12, 2019 · 7 comments · Fixed by #1660
Assignees
Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.

Comments

@ghmeier
Copy link

ghmeier commented Feb 12, 2019

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.

@ghmeier ghmeier changed the title auth.setCredentials has side effects in the same event loop. auth.setCredentials has side effects for all clients in the same event loop. Feb 12, 2019
@wearhere
Copy link

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

@ghmeier ghmeier closed this as completed Feb 12, 2019
@ghmeier ghmeier reopened this Feb 12, 2019
@yoshi-automation yoshi-automation added the triage me I really want to be triaged. label Feb 12, 2019
@ghmeier
Copy link
Author

ghmeier commented Feb 13, 2019

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.

@uberism
Copy link

uberism commented Feb 16, 2019

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 email2343@gmail.com

@yoshi-automation yoshi-automation added the 🚨 This issue needs some love. label Feb 17, 2019
ajaaym added a commit to ajaaym/google-api-nodejs-client that referenced this issue Feb 19, 2019
@pvatterott
Copy link

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

@JustinBeckwith JustinBeckwith added type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns. priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. labels Mar 1, 2019
@JustinBeckwith JustinBeckwith self-assigned this Mar 7, 2019
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. triage me I really want to be triaged. labels Mar 7, 2019
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Mar 17, 2019
@yoshi-automation yoshi-automation added 🚨 This issue needs some love. and removed 🚨 This issue needs some love. labels Mar 22, 2019
@ghmeier
Copy link
Author

ghmeier commented Mar 27, 2019

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.

@jacobpgn
Copy link

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 😞

@JustinBeckwith
Copy link
Contributor

Greetings folks! And thanks for being patient here. We did a little root cause analysis, and found the cause of the problem in #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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
priority: p1 Important issue which blocks shipping the next release. Will be fixed prior to next release. 🚨 This issue needs some love. type: bug Error or flaw in code with unintended results or allowing sub-optimal usage patterns.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants