Skip to content

Commit 2a2d1e3

Browse files
glasserglasser
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. Apollo Server 4 has the same overall behavior (though the details of the particular objects in question have changed a bit). 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. To fix this, we are making a change to the `GraphQLRequestContext.response` object as seen by plugins. This object's `http` field is now shared across all operations in a batched request, instead of being specific to the individual operation. This means that any changes to headers (or HTTP status code) in plugins processing one operation are immediately visible to plugins processing another operation, and so they can properly merge header values. **This change is technically backwards-incompatible.** However, as this is a relatively subtle change and as Apollo Server 4 is very new and its adoption is relatively low (we haven't even formally announced AS4 on our blog yet), we feel comfortable making this change in a minor version instead of in a new major version. The cache control plugin is changed to merge `cache-control` response headers across multiple operations in a batched request. Note that if the `cache-control` header is ever set to any value that could not be written by the cache control plugin, the plugin will refuse to update it further so as not to lose information. (It is best to only set the header from one place, and to disable the cache control plugin's header-writing feature with `calculateHttpHeaders: false` if you want to write the header from elsewhere.) Additionally, a new `requestIsBatched: boolean` field is added to `GraphQLRequestContext`. This isn't read by anything in this PR. However, we will backport this field to Apollo Server 3, and use it to disable cache-control header writing for batched requests. (Backporting the full solution to AS3 would be challenging because the relevant data structures are more complex, and making a backwards-incompatible change to an older version like AS3 is more problematic.) For more information, see GHSA-8r69-3cvp-wxc3
1 parent 761b4b4 commit 2a2d1e3

File tree

9 files changed

+279
-58
lines changed

9 files changed

+279
-58
lines changed

‎.changeset/curvy-cows-vanish.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@apollo/server-integration-testsuite': minor
3+
'@apollo/server': minor
4+
---
5+
6+
The `cache-control` HTTP response header set by the cache control plugin now properly reflects the cache policy of all operations in a batched HTTP request. (If you write the `cache-control` response header via a different mechanism to a format that the plugin would not produce, the plugin no longer writes the header.) For more information, see [advisory GHSA-8r69-3cvp-wxc3](https://github.com/apollographql/apollo-server/security/advisories/GHSA-8r69-3cvp-wxc3).

‎.changeset/curvy-kiwis-work.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@apollo/server-integration-testsuite': minor
3+
'@apollo/server': minor
4+
---
5+
6+
Plugins processing multiple operations in a batched HTTP request now have a shared `requestContext.request.http` object. Changes to HTTP response headers and HTTP status code made by plugins operating on one operation can be immediately seen by plugins operating on other operations in the same HTTP request.

‎.changeset/honest-kiwis-poke.md‎

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,6 @@
1+
---
2+
'@apollo/server-integration-testsuite': minor
3+
'@apollo/server': minor
4+
---
5+
6+
New field `GraphQLRequestContext.requestIsBatched` available to plugins.

‎packages/integration-testsuite/src/httpServerTests.ts‎

Lines changed: 127 additions & 10 deletions
Original file line numberDiff line numberDiff line change
@@ -794,7 +794,18 @@ export function defineIntegrationTestSuiteHttpServerTests(
794794
});
795795

796796
it('can handle a basic request', async () => {
797-
const app = await createApp();
797+
let requestIsBatched: boolean | undefined;
798+
const app = await createApp({
799+
schema,
800+
allowBatchedHttpRequests: true,
801+
plugins: [
802+
{
803+
async requestDidStart(requestContext) {
804+
requestIsBatched = requestContext.requestIsBatched;
805+
},
806+
},
807+
],
808+
});
798809
const expected = {
799810
testString: 'it works',
800811
};
@@ -807,6 +818,7 @@ export function defineIntegrationTestSuiteHttpServerTests(
807818
'application/json; charset=utf-8',
808819
);
809820
expect(res.body.data).toEqual(expected);
821+
expect(requestIsBatched).toEqual(false);
810822
});
811823

812824
it.each([
@@ -897,6 +909,9 @@ export function defineIntegrationTestSuiteHttpServerTests(
897909
books: [Book]
898910
cooks: [Cook]
899911
pooks: [Pook]
912+
uncached: ID
913+
ten: ID @cacheControl(maxAge: 10)
914+
twenty: ID @cacheControl(maxAge: 20, scope: PRIVATE)
900915
}
901916
902917
enum CacheControlScope {
@@ -928,6 +943,98 @@ export function defineIntegrationTestSuiteHttpServerTests(
928943
expect(res.headers['cache-control']).toEqual('max-age=200, public');
929944
});
930945

946+
it('applies cacheControl Headers for batched operation', async () => {
947+
const app = await createApp({
948+
typeDefs,
949+
resolvers,
950+
allowBatchedHttpRequests: true,
951+
});
952+
{
953+
const res = await request(app)
954+
.post('/')
955+
.send([{ query: '{ten}' }, { query: '{twenty}' }]);
956+
expect(res.status).toEqual(200);
957+
expect(res.body).toMatchInlineSnapshot(`
958+
[
959+
{
960+
"data": {
961+
"ten": null,
962+
},
963+
},
964+
{
965+
"data": {
966+
"twenty": null,
967+
},
968+
},
969+
]
970+
`);
971+
expect(res.headers['cache-control']).toEqual('max-age=10, private');
972+
}
973+
{
974+
const res = await request(app)
975+
.post('/')
976+
.send([{ query: '{twenty}' }, { query: '{ten}' }]);
977+
expect(res.status).toEqual(200);
978+
expect(res.body).toMatchInlineSnapshot(`
979+
[
980+
{
981+
"data": {
982+
"twenty": null,
983+
},
984+
},
985+
{
986+
"data": {
987+
"ten": null,
988+
},
989+
},
990+
]
991+
`);
992+
expect(res.headers['cache-control']).toEqual('max-age=10, private');
993+
}
994+
{
995+
const res = await request(app)
996+
.post('/')
997+
.send([{ query: '{uncached}' }, { query: '{ten}' }]);
998+
expect(res.status).toEqual(200);
999+
expect(res.body).toMatchInlineSnapshot(`
1000+
[
1001+
{
1002+
"data": {
1003+
"uncached": null,
1004+
},
1005+
},
1006+
{
1007+
"data": {
1008+
"ten": null,
1009+
},
1010+
},
1011+
]
1012+
`);
1013+
expect(res.headers['cache-control']).toEqual('no-store');
1014+
}
1015+
{
1016+
const res = await request(app)
1017+
.post('/')
1018+
.send([{ query: '{ten}' }, { query: '{uncached}' }]);
1019+
expect(res.status).toEqual(200);
1020+
expect(res.body).toMatchInlineSnapshot(`
1021+
[
1022+
{
1023+
"data": {
1024+
"ten": null,
1025+
},
1026+
},
1027+
{
1028+
"data": {
1029+
"uncached": null,
1030+
},
1031+
},
1032+
]
1033+
`);
1034+
expect(res.headers['cache-control']).toEqual('no-store');
1035+
}
1036+
});
1037+
9311038
it('applies cacheControl Headers with if-cacheable', async () => {
9321039
const app = await createApp({
9331040
typeDefs,
@@ -1276,7 +1383,18 @@ export function defineIntegrationTestSuiteHttpServerTests(
12761383
});
12771384

12781385
it('can handle batch requests', async () => {
1279-
const app = await createApp({ schema, allowBatchedHttpRequests: true });
1386+
let requestIsBatched: boolean | undefined;
1387+
const app = await createApp({
1388+
schema,
1389+
allowBatchedHttpRequests: true,
1390+
plugins: [
1391+
{
1392+
async requestDidStart(requestContext) {
1393+
requestIsBatched = requestContext.requestIsBatched;
1394+
},
1395+
},
1396+
],
1397+
});
12801398
const expected = [
12811399
{
12821400
data: {
@@ -1289,7 +1407,7 @@ export function defineIntegrationTestSuiteHttpServerTests(
12891407
},
12901408
},
12911409
];
1292-
const req = request(app)
1410+
const res = await request(app)
12931411
.post('/')
12941412
.send([
12951413
{
@@ -1306,13 +1424,12 @@ export function defineIntegrationTestSuiteHttpServerTests(
13061424
operationName: 'testX',
13071425
},
13081426
]);
1309-
return req.then((res) => {
1310-
expect(res.status).toEqual(200);
1311-
expect(res.body).toEqual(expected);
1312-
expect(res.header['content-length']).toEqual(
1313-
Buffer.byteLength(res.text, 'utf8').toString(),
1314-
);
1315-
});
1427+
expect(res.status).toEqual(200);
1428+
expect(res.body).toEqual(expected);
1429+
expect(res.header['content-length']).toEqual(
1430+
Buffer.byteLength(res.text, 'utf8').toString(),
1431+
);
1432+
expect(requestIsBatched).toBe(true);
13161433
});
13171434

13181435
it('can handle non-batch requests when allowBatchedHttpRequests is true', async () => {

‎packages/server/src/ApolloServer.ts‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1197,6 +1197,7 @@ export class ApolloServer<in out TContext extends BaseContext = BaseContext> {
11971197
graphQLRequest,
11981198
internals: this.internals,
11991199
schemaDerivedData,
1200+
sharedResponseHTTPGraphQLHead: null,
12001201
},
12011202
options,
12021203
);
@@ -1215,11 +1216,13 @@ export async function internalExecuteOperation<TContext extends BaseContext>(
12151216
graphQLRequest,
12161217
internals,
12171218
schemaDerivedData,
1219+
sharedResponseHTTPGraphQLHead,
12181220
}: {
12191221
server: ApolloServer<TContext>;
12201222
graphQLRequest: GraphQLRequest;
12211223
internals: ApolloServerInternals<TContext>;
12221224
schemaDerivedData: SchemaDerivedData;
1225+
sharedResponseHTTPGraphQLHead: HTTPGraphQLHead | null;
12231226
},
12241227
options: ExecuteOperationOptions<TContext>,
12251228
): Promise<GraphQLResponse> {
@@ -1229,7 +1232,7 @@ export async function internalExecuteOperation<TContext extends BaseContext>(
12291232
schema: schemaDerivedData.schema,
12301233
request: graphQLRequest,
12311234
response: {
1232-
http: newHTTPGraphQLHead(),
1235+
http: sharedResponseHTTPGraphQLHead ?? newHTTPGraphQLHead(),
12331236
},
12341237
// We clone the context because there are some assumptions that every operation
12351238
// execution has a brand new context object; specifically, in order to implement
@@ -1249,6 +1252,7 @@ export async function internalExecuteOperation<TContext extends BaseContext>(
12491252
contextValue: cloneObject(options?.contextValue ?? ({} as TContext)),
12501253
metrics: {},
12511254
overallCachePolicy: newCachePolicy(),
1255+
requestIsBatched: sharedResponseHTTPGraphQLHead !== null,
12521256
};
12531257

12541258
try {

‎packages/server/src/externalTypes/requestPipeline.ts‎

Lines changed: 7 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -70,6 +70,13 @@ export interface GraphQLRequestContext<TContext extends BaseContext> {
7070
readonly metrics: GraphQLRequestMetrics;
7171

7272
readonly overallCachePolicy: CachePolicy;
73+
74+
/**
75+
* True if this request is part of a potentially multi-operation batch. Note
76+
* that if this is true, `response.http` will be shared with the other
77+
* operations in the batch.
78+
*/
79+
readonly requestIsBatched: boolean;
7380
}
7481

7582
export type GraphQLRequestContextDidResolveSource<

‎packages/server/src/httpBatching.ts‎

Lines changed: 35 additions & 29 deletions
Original file line numberDiff line numberDiff line change
@@ -11,53 +11,58 @@ import type {
1111
import { newHTTPGraphQLHead, runHttpQuery } from './runHttpQuery.js';
1212
import { BadRequestError } from './internalErrorClasses.js';
1313

14-
export async function runBatchHttpQuery<TContext extends BaseContext>(
15-
server: ApolloServer<TContext>,
16-
batchRequest: HTTPGraphQLRequest,
17-
body: unknown[],
18-
contextValue: TContext,
19-
schemaDerivedData: SchemaDerivedData,
20-
internals: ApolloServerInternals<TContext>,
21-
): Promise<HTTPGraphQLResponse> {
14+
async function runBatchedHttpQuery<TContext extends BaseContext>({
15+
server,
16+
batchRequest,
17+
body,
18+
contextValue,
19+
schemaDerivedData,
20+
internals,
21+
}: {
22+
server: ApolloServer<TContext>;
23+
batchRequest: HTTPGraphQLRequest;
24+
body: unknown[];
25+
contextValue: TContext;
26+
schemaDerivedData: SchemaDerivedData;
27+
internals: ApolloServerInternals<TContext>;
28+
}): Promise<HTTPGraphQLResponse> {
2229
if (body.length === 0) {
2330
throw new BadRequestError('No operations found in request.');
2431
}
2532

26-
const combinedResponseHead = newHTTPGraphQLHead();
33+
// This single HTTPGraphQLHead is shared across all the operations in the
34+
// batch. This means that any changes to response headers or status code from
35+
// one operation can be immediately seen by other operations. Plugins that set
36+
// response headers or status code can then choose to combine the data they
37+
// are setting with data that may already be there from another operation as
38+
// they choose.
39+
const sharedResponseHTTPGraphQLHead = newHTTPGraphQLHead();
2740
const responseBodies = await Promise.all(
2841
body.map(async (bodyPiece: unknown) => {
2942
const singleRequest: HTTPGraphQLRequest = {
3043
...batchRequest,
3144
body: bodyPiece,
3245
};
3346

34-
const response = await runHttpQuery(
47+
const response = await runHttpQuery({
3548
server,
36-
singleRequest,
49+
httpRequest: singleRequest,
3750
contextValue,
3851
schemaDerivedData,
3952
internals,
40-
);
53+
sharedResponseHTTPGraphQLHead,
54+
});
4155

4256
if (response.body.kind === 'chunked') {
4357
throw Error(
4458
'Incremental delivery is not implemented for batch requests',
4559
);
4660
}
47-
for (const [key, value] of response.headers) {
48-
// Override any similar header set in other responses.
49-
combinedResponseHead.headers.set(key, value);
50-
}
51-
// If two responses both want to set the status code, one of them will win.
52-
// Note that the normal success case leaves status empty.
53-
if (response.status) {
54-
combinedResponseHead.status = response.status;
55-
}
5661
return response.body.string;
5762
}),
5863
);
5964
return {
60-
...combinedResponseHead,
65+
...sharedResponseHTTPGraphQLHead,
6166
body: { kind: 'complete', string: `[${responseBodies.join(',')}]` },
6267
};
6368
}
@@ -77,23 +82,24 @@ export async function runPotentiallyBatchedHttpQuery<
7782
Array.isArray(httpGraphQLRequest.body)
7883
)
7984
) {
80-
return await runHttpQuery(
85+
return await runHttpQuery({
8186
server,
82-
httpGraphQLRequest,
87+
httpRequest: httpGraphQLRequest,
8388
contextValue,
8489
schemaDerivedData,
8590
internals,
86-
);
91+
sharedResponseHTTPGraphQLHead: null,
92+
});
8793
}
8894
if (internals.allowBatchedHttpRequests) {
89-
return await runBatchHttpQuery(
95+
return await runBatchedHttpQuery({
9096
server,
91-
httpGraphQLRequest,
92-
httpGraphQLRequest.body as unknown[],
97+
batchRequest: httpGraphQLRequest,
98+
body: httpGraphQLRequest.body as unknown[],
9399
contextValue,
94100
schemaDerivedData,
95101
internals,
96-
);
102+
});
97103
}
98104
throw new BadRequestError('Operation batching disabled.');
99105
}

0 commit comments

Comments
 (0)