Merge pull request from GHSA-8r69-3cvp-wxc3

In Apollo Server 2, plugins could not set HTTP response headers for HTTP-batched operations; the headers would simply not be written in that case. In Apollo Server 3, plugins can set HTTP response headers. They have access to individual `response.http.headers` objects. In parallel, after each operation is processed, any response headers it sets are copied to a shared HTTP response header object. If multiple operations wrote the same header, the one that finishes its processing last would "win", without any smart merging. Notably, this means that the `cache-control` response header set by the cache control plugin had surprising behavior when combined with batching. If you sent two operations in the same HTTP request with different cache policies, the response header would be set based only on one of their policies, not on the combination of the policies. This could lead to saving data that should not be cached in a cache, or saving data that should only be cached per-user (`private`) in a more public cache. In Apollo Server 3, we are fixing this by restoring the AS2 behavior: never setting the `cache-control` header for batched operations. While this is in a sense backwards-incompatible, the old behavior was nondeterministic and unpredictable and it seems unlikely that anyone was intentionally relying on it. (In Apollo Server 4, we will instead make improvements (in v4.1.0) that allow the cache control plugin to merge the header values across operations. This is not feasible in Apollo Server 3 because the data structure manipulation is more complex and this would be a serious backwards incompatibility to the plugin API.) As part of this, a new `requestIsBatched: boolean` field is added to `GraphQLRequestContext`. (We will also add this to v4.1.0.) For more information, see GHSA-8r69-3cvp-wxc3
apollographql · Nov 2, 2022 · 69be2f7 · 69be2f7
1 parent 40fcd3d
commit 69be2f7
Show file tree

Hide file tree

Showing 9 changed files with 84 additions and 20 deletions.
diff --git a/CHANGELOG.md b/CHANGELOG.md
@@ -10,6 +10,10 @@ The version headers in this history reflect the versions of Apollo Server itself
 
 ## vNEXT
 
+## v3.11.0
+- ⚠️ **SECURITY**: The cache control plugin no longer sets the `cache-control` HTTP response header if the operation is part of a batched HTTP request. Previously, it would set the header to a value describing the cache policy of only one of the operations in the request, which could lead to data being unintentionally cached by proxies or clients. This bug was introduced in v3.0.0 and this fix restores the behavior of Apollo Server 2. (In Apollo Server 4 (specifically, `@apollo/server@4.1.0` or newer), the features work properly together, setting the header based on the combined cache policy of all operations.) This could theoretically have led to data tagged as uncacheable being cached and potentially served to different users. More details are available at the [security advisory](https://github.com/apollographql/apollo-server/security/advisories/GHSA-8r69-3cvp-wxc3).
+- `apollo-server-core`: New field `GraphQLRequestContext.requestIsBatched` available to plugins.
+
 ## v3.10.4
 
 - `apollo-server-core`: Manage memory more efficiently in the usage reporting plugin by allowing large objects to be garbage collected more quickly. [PR #7106](https://github.com/apollographql/apollo-server/pull/7106)

diff --git a/packages/apollo-server-core/src/ApolloServer.ts b/packages/apollo-server-core/src/ApolloServer.ts
@@ -1032,6 +1032,7 @@ export class ApolloServerBase<
       },
       debug: options.debug,
       overallCachePolicy: newCachePolicy(),
+      requestIsBatched: false,
     };
 
     return processGraphQLRequest(options, requestCtx);

diff --git a/packages/apollo-server-core/src/__tests__/runQuery.test.ts b/packages/apollo-server-core/src/__tests__/runQuery.test.ts
@@ -59,6 +59,7 @@ function runQuery(
     debug: options.debug,
     cache: {} as any,
     overallCachePolicy: newCachePolicy(),
+    requestIsBatched: false,
     ...requestContextExtra,
   });
 }

diff --git a/packages/apollo-server-core/src/plugin/cacheControl/index.ts b/packages/apollo-server-core/src/plugin/cacheControl/index.ts
@@ -232,18 +232,21 @@ export function ApolloServerPluginCacheControl(
         },
 
         async willSendResponse(requestContext) {
-          const { response, overallCachePolicy } = requestContext;
+          const { response, overallCachePolicy, requestIsBatched } =
+            requestContext;
 
           const policyIfCacheable = overallCachePolicy.policyIfCacheable();
 
           // If the feature is enabled, there is a non-trivial cache policy,
-          // there are no errors, and we actually can write headers, write the
-          // header.
+          // there are no errors, we actually can write headers, and the request
+          // is not batched (because we have no way of merging the header across
+          // operations in AS3), write the header.
           if (
             calculateHttpHeaders &&
             policyIfCacheable &&
             !response.errors &&
-            response.http
+            response.http &&
+            !requestIsBatched
           ) {
             response.http.headers.set(
               'Cache-Control',

diff --git a/packages/apollo-server-core/src/runHttpQuery.ts b/packages/apollo-server-core/src/runHttpQuery.ts
@@ -343,6 +343,7 @@ export async function processHTTPRequest<TContext extends BaseContext>(
 
   function buildRequestContext(
     request: GraphQLRequest,
+    requestIsBatched: boolean,
   ): GraphQLRequestContext<TContext> {
     // TODO: We currently shallow clone the context for every request,
     // but that's unlikely to be what people want.
@@ -370,6 +371,7 @@ export async function processHTTPRequest<TContext extends BaseContext>(
       debug: options.debug,
       metrics: {},
       overallCachePolicy: newCachePolicy(),
+      requestIsBatched,
     };
   }
 
@@ -399,7 +401,7 @@ export async function processHTTPRequest<TContext extends BaseContext>(
       const responses = await Promise.all(
         requests.map(async (request) => {
           try {
-            const requestContext = buildRequestContext(request);
+            const requestContext = buildRequestContext(request, true);
             const response = await processGraphQLRequest(
               options,
               requestContext,
@@ -429,7 +431,7 @@ export async function processHTTPRequest<TContext extends BaseContext>(
       // We're processing a normal request
       const request = parseGraphQLRequest(httpRequest.request, requestPayload);
 
-      const requestContext = buildRequestContext(request);
+      const requestContext = buildRequestContext(request, false);
 
       const response = await processGraphQLRequest(options, requestContext);
 

diff --git a/packages/apollo-server-core/src/utils/pluginTestHarness.ts b/packages/apollo-server-core/src/utils/pluginTestHarness.ts
@@ -133,6 +133,7 @@ export default async function pluginTestHarness<TContext extends BaseContext>({
     cache: new InMemoryLRUCache(),
     context,
     overallCachePolicy: newCachePolicy(),
+    requestIsBatched: false,
   };
 
   if (requestContext.source === undefined) {

diff --git a/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts b/packages/apollo-server-express/src/__tests__/ApolloServer.test.ts
@@ -424,20 +424,37 @@ describe('apollo-server-express', () => {
 
     describe('Cache Control Headers', () => {
       it('applies cacheControl Headers', async () => {
-        const { url: uri } = await createServer({ typeDefs, resolvers });
+        const { server, httpServer } = await createServer({
+          typeDefs,
+          resolvers,
+        });
 
-        const apolloFetch = createApolloFetch({ uri }).useAfter(
-          (response, next) => {
-            expect(response.response.headers.get('cache-control')).toEqual(
-              'max-age=200, public',
-            );
-            next();
-          },
-        );
-        const result = await apolloFetch({
-          query: `{ cooks { title author } }`,
+        const res = await request(httpServer)
+          .post(server.graphqlPath)
+          .send({ query: `{ cooks { title author } }` });
+        expect(res.status).toEqual(200);
+        expect(res.body.data).toEqual({ cooks: books });
+        expect(res.header['cache-control']).toEqual('max-age=200, public');
+      });
+
+      it('does not apply cacheControl Headers for batch', async () => {
+        const { server, httpServer } = await createServer({
+          typeDefs,
+          resolvers,
         });
-        expect(result.data).toEqual({ cooks: books });
+
+        const res = await request(httpServer)
+          .post(server.graphqlPath)
+          .send([
+            { query: `{ cooks { title author } }` },
+            { query: `{ cooks { title author } }` },
+          ]);
+        expect(res.status).toEqual(200);
+        expect(res.body).toEqual([
+          { data: { cooks: books } },
+          { data: { cooks: books } },
+        ]);
+        expect(res.header['cache-control']).toBeUndefined();
       });
 
       it('contains no cacheControl Headers when uncacheable', async () => {

diff --git a/packages/apollo-server-integration-testsuite/src/index.ts b/packages/apollo-server-integration-testsuite/src/index.ts
@@ -475,7 +475,19 @@ export default ({
       });
 
       it('can handle a basic request', async () => {
-        app = await createApp();
+        let requestIsBatched: boolean | undefined;
+        app = await createApp({
+          graphqlOptions: {
+            schema,
+            plugins: [
+              {
+                async requestDidStart(requestContext) {
+                  requestIsBatched = requestContext.requestIsBatched;
+                },
+              },
+            ],
+          },
+        });
         const expected = {
           testString: 'it works',
         };
@@ -485,6 +497,7 @@ export default ({
         return req.then((res) => {
           expect(res.status).toEqual(200);
           expect(res.body.data).toEqual(expected);
+          expect(requestIsBatched).toEqual(false);
         });
       });
 
@@ -793,7 +806,19 @@ export default ({
       });
 
       it('can handle batch requests', async () => {
-        app = await createApp();
+        let requestIsBatched: boolean | undefined;
+        app = await createApp({
+          graphqlOptions: {
+            schema,
+            plugins: [
+              {
+                async requestDidStart(requestContext) {
+                  requestIsBatched = requestContext.requestIsBatched;
+                },
+              },
+            ],
+          },
+        });
         const expected = [
           {
             data: {
@@ -826,6 +851,7 @@ export default ({
         return req.then((res) => {
           expect(res.status).toEqual(200);
           expect(res.body).toEqual(expected);
+          expect(requestIsBatched).toEqual(true);
         });
       });
 

diff --git a/packages/apollo-server-types/src/index.ts b/packages/apollo-server-types/src/index.ts
@@ -172,6 +172,15 @@ export interface GraphQLRequestContext<TContext = Record<string, any>> {
   debug?: boolean;
 
   readonly overallCachePolicy: CachePolicy;
+
+  /**
+   * True if this request is part of a potentially multi-operation batch. Note
+   * that if this is true, the headers and status code `response.http` will be
+   * be merged together; if two operations set the same header one will arbitrarily
+   * win. (In Apollo Server v4, `response.http` will be shared with the other
+   * operations in the batch.)
+   */
+  readonly requestIsBatched: boolean;
 }
 
 export type ValidationRule = (context: ValidationContext) => ASTVisitor;