Skip to content

Commit 69be2f7

Browse files
authored
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
1 parent 40fcd3d commit 69be2f7

File tree

9 files changed

+84
-20
lines changed

9 files changed

+84
-20
lines changed

CHANGELOG.md

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -10,6 +10,10 @@ The version headers in this history reflect the versions of Apollo Server itself
1010

1111
## vNEXT
1212

13+
## v3.11.0
14+
- ⚠️ **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).
15+
- `apollo-server-core`: New field `GraphQLRequestContext.requestIsBatched` available to plugins.
16+
1317
## v3.10.4
1418

1519
- `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)

packages/apollo-server-core/src/ApolloServer.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1032,6 +1032,7 @@ export class ApolloServerBase<
10321032
},
10331033
debug: options.debug,
10341034
overallCachePolicy: newCachePolicy(),
1035+
requestIsBatched: false,
10351036
};
10361037

10371038
return processGraphQLRequest(options, requestCtx);

packages/apollo-server-core/src/__tests__/runQuery.test.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -59,6 +59,7 @@ function runQuery(
5959
debug: options.debug,
6060
cache: {} as any,
6161
overallCachePolicy: newCachePolicy(),
62+
requestIsBatched: false,
6263
...requestContextExtra,
6364
});
6465
}

packages/apollo-server-core/src/plugin/cacheControl/index.ts

Lines changed: 7 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -232,18 +232,21 @@ export function ApolloServerPluginCacheControl(
232232
},
233233

234234
async willSendResponse(requestContext) {
235-
const { response, overallCachePolicy } = requestContext;
235+
const { response, overallCachePolicy, requestIsBatched } =
236+
requestContext;
236237

237238
const policyIfCacheable = overallCachePolicy.policyIfCacheable();
238239

239240
// If the feature is enabled, there is a non-trivial cache policy,
240-
// there are no errors, and we actually can write headers, write the
241-
// header.
241+
// there are no errors, we actually can write headers, and the request
242+
// is not batched (because we have no way of merging the header across
243+
// operations in AS3), write the header.
242244
if (
243245
calculateHttpHeaders &&
244246
policyIfCacheable &&
245247
!response.errors &&
246-
response.http
248+
response.http &&
249+
!requestIsBatched
247250
) {
248251
response.http.headers.set(
249252
'Cache-Control',

packages/apollo-server-core/src/runHttpQuery.ts

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -343,6 +343,7 @@ export async function processHTTPRequest<TContext extends BaseContext>(
343343

344344
function buildRequestContext(
345345
request: GraphQLRequest,
346+
requestIsBatched: boolean,
346347
): GraphQLRequestContext<TContext> {
347348
// TODO: We currently shallow clone the context for every request,
348349
// but that's unlikely to be what people want.
@@ -370,6 +371,7 @@ export async function processHTTPRequest<TContext extends BaseContext>(
370371
debug: options.debug,
371372
metrics: {},
372373
overallCachePolicy: newCachePolicy(),
374+
requestIsBatched,
373375
};
374376
}
375377

@@ -399,7 +401,7 @@ export async function processHTTPRequest<TContext extends BaseContext>(
399401
const responses = await Promise.all(
400402
requests.map(async (request) => {
401403
try {
402-
const requestContext = buildRequestContext(request);
404+
const requestContext = buildRequestContext(request, true);
403405
const response = await processGraphQLRequest(
404406
options,
405407
requestContext,
@@ -429,7 +431,7 @@ export async function processHTTPRequest<TContext extends BaseContext>(
429431
// We're processing a normal request
430432
const request = parseGraphQLRequest(httpRequest.request, requestPayload);
431433

432-
const requestContext = buildRequestContext(request);
434+
const requestContext = buildRequestContext(request, false);
433435

434436
const response = await processGraphQLRequest(options, requestContext);
435437

packages/apollo-server-core/src/utils/pluginTestHarness.ts

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -133,6 +133,7 @@ export default async function pluginTestHarness<TContext extends BaseContext>({
133133
cache: new InMemoryLRUCache(),
134134
context,
135135
overallCachePolicy: newCachePolicy(),
136+
requestIsBatched: false,
136137
};
137138

138139
if (requestContext.source === undefined) {

packages/apollo-server-express/src/__tests__/ApolloServer.test.ts

Lines changed: 29 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -424,20 +424,37 @@ describe('apollo-server-express', () => {
424424

425425
describe('Cache Control Headers', () => {
426426
it('applies cacheControl Headers', async () => {
427-
const { url: uri } = await createServer({ typeDefs, resolvers });
427+
const { server, httpServer } = await createServer({
428+
typeDefs,
429+
resolvers,
430+
});
428431

429-
const apolloFetch = createApolloFetch({ uri }).useAfter(
430-
(response, next) => {
431-
expect(response.response.headers.get('cache-control')).toEqual(
432-
'max-age=200, public',
433-
);
434-
next();
435-
},
436-
);
437-
const result = await apolloFetch({
438-
query: `{ cooks { title author } }`,
432+
const res = await request(httpServer)
433+
.post(server.graphqlPath)
434+
.send({ query: `{ cooks { title author } }` });
435+
expect(res.status).toEqual(200);
436+
expect(res.body.data).toEqual({ cooks: books });
437+
expect(res.header['cache-control']).toEqual('max-age=200, public');
438+
});
439+
440+
it('does not apply cacheControl Headers for batch', async () => {
441+
const { server, httpServer } = await createServer({
442+
typeDefs,
443+
resolvers,
439444
});
440-
expect(result.data).toEqual({ cooks: books });
445+
446+
const res = await request(httpServer)
447+
.post(server.graphqlPath)
448+
.send([
449+
{ query: `{ cooks { title author } }` },
450+
{ query: `{ cooks { title author } }` },
451+
]);
452+
expect(res.status).toEqual(200);
453+
expect(res.body).toEqual([
454+
{ data: { cooks: books } },
455+
{ data: { cooks: books } },
456+
]);
457+
expect(res.header['cache-control']).toBeUndefined();
441458
});
442459

443460
it('contains no cacheControl Headers when uncacheable', async () => {

packages/apollo-server-integration-testsuite/src/index.ts

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -475,7 +475,19 @@ export default ({
475475
});
476476

477477
it('can handle a basic request', async () => {
478-
app = await createApp();
478+
let requestIsBatched: boolean | undefined;
479+
app = await createApp({
480+
graphqlOptions: {
481+
schema,
482+
plugins: [
483+
{
484+
async requestDidStart(requestContext) {
485+
requestIsBatched = requestContext.requestIsBatched;
486+
},
487+
},
488+
],
489+
},
490+
});
479491
const expected = {
480492
testString: 'it works',
481493
};
@@ -485,6 +497,7 @@ export default ({
485497
return req.then((res) => {
486498
expect(res.status).toEqual(200);
487499
expect(res.body.data).toEqual(expected);
500+
expect(requestIsBatched).toEqual(false);
488501
});
489502
});
490503

@@ -793,7 +806,19 @@ export default ({
793806
});
794807

795808
it('can handle batch requests', async () => {
796-
app = await createApp();
809+
let requestIsBatched: boolean | undefined;
810+
app = await createApp({
811+
graphqlOptions: {
812+
schema,
813+
plugins: [
814+
{
815+
async requestDidStart(requestContext) {
816+
requestIsBatched = requestContext.requestIsBatched;
817+
},
818+
},
819+
],
820+
},
821+
});
797822
const expected = [
798823
{
799824
data: {
@@ -826,6 +851,7 @@ export default ({
826851
return req.then((res) => {
827852
expect(res.status).toEqual(200);
828853
expect(res.body).toEqual(expected);
854+
expect(requestIsBatched).toEqual(true);
829855
});
830856
});
831857

packages/apollo-server-types/src/index.ts

Lines changed: 9 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,15 @@ export interface GraphQLRequestContext<TContext = Record<string, any>> {
172172
debug?: boolean;
173173

174174
readonly overallCachePolicy: CachePolicy;
175+
176+
/**
177+
* True if this request is part of a potentially multi-operation batch. Note
178+
* that if this is true, the headers and status code `response.http` will be
179+
* be merged together; if two operations set the same header one will arbitrarily
180+
* win. (In Apollo Server v4, `response.http` will be shared with the other
181+
* operations in the batch.)
182+
*/
183+
readonly requestIsBatched: boolean;
175184
}
176185

177186
export type ValidationRule = (context: ValidationContext) => ASTVisitor;

0 commit comments

Comments
 (0)