RESTDataSource should not cache requests to the same URL for different Authorization header values

[mcilvena]

Package: apollo-datasource-rest
Version: 0.9.0

TL;DR: RESTDataSource's default behaviour of using just the request URL as the cache key is dangerous in cases where a single instance of DataSource is used to make multiple request with differing Authorization headers to upstream services.

Background context:
The company I work for recently experienced a pretty serious production issue as a result of a code refactor that moved a datasource instantiation from inside ApolloServer's dataSources constructor property to and external file.

Example.

Before:

// app.ts
const server = new ApolloServer({
  context: async ({
    req,
  }: {
    req: express.Request;
  }): Promise<IRequestContext> => ({
    authHeader: getAuthHeader(req)
  }),
  dataSources: () => ({
    someDataSource: new SomeRESTDataSource(),
    //... and many others
  }),
});

After (defect introduced):

// datasources.ts
const someDataSource = new SomeRESTDataSource();

export default {
  someDataSource
  //... and many others
}

// app.ts
import datasources from './datasources';

const server = new ApolloServer({
  context: async ({
    req,
  }: {
    req: express.Request;
  }): Promise<IRequestContext> => ({
    authHeader: getAuthHeader(req)
  }),
  dataSources: () => ({
    // Yes, this is a bug, but unless you understand why, it's an easy mistake for a developer to make
    ...datasources
  }),
});

Actual Behaviour:
As a result of initialising the datasource outside of the dataSources factory function, each application instance can accept requests for multiple authorized users, reusing the same internal cache (keyed on upstream URL) of RESTDataSource. This results in data of initial request's user being returned to all subsequent users (this is obviously very bad).

Expected behaviour:
Whether new to ApolloServer or not, I've seem mutliple developers trip on this implicit requirement for datasources to be instantiated inside Configs dataSources function. This requirement and its potential consequences are not obvious or intuitive enough.

I would expect consistent behaviour regardless or where the datasource is instantiated. Maybe the cache should be reinitialised on each request as part of the requestPipeline; or default to separating the cache for different Authorization header values.

[glasser]

As is now better explained (after #5086), you shouldn't share data source objects across requests.

Given that, I don't think the suggested change about authorization makes sense. But if you do want to change how requests are cached, you can also override cacheKeyFor.

[mcilvena]

Hi @glasser, Thanks for following this up.

FWIW, I still think the default behaviour can be dangerous and I don't think updating the docs necessarily solves the problem. This feels like putting a warning in the user manual of a saw; just put a guard on it so no one loses a finger.

[shubhamzanwar]

I have to agree with @mcilvena - ran into a couple of issues that took away a bunch of time only to realise that the RESTDataSource memoises responses based on the url :(

Idk why, but I had imagined a more elegant way of creating cache keys