diff --git a/.changeset/curvy-cows-vanish.md b/.changeset/curvy-cows-vanish.md new file mode 100644 index 00000000000..7b3df136aef --- /dev/null +++ b/.changeset/curvy-cows-vanish.md @@ -0,0 +1,6 @@ +--- +'@apollo/server-integration-testsuite': minor +'@apollo/server': minor +--- + +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). diff --git a/.changeset/curvy-kiwis-work.md b/.changeset/curvy-kiwis-work.md new file mode 100644 index 00000000000..461b476e11b --- /dev/null +++ b/.changeset/curvy-kiwis-work.md @@ -0,0 +1,6 @@ +--- +'@apollo/server-integration-testsuite': minor +'@apollo/server': minor +--- + +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. diff --git a/.changeset/honest-kiwis-poke.md b/.changeset/honest-kiwis-poke.md new file mode 100644 index 00000000000..dda6478e106 --- /dev/null +++ b/.changeset/honest-kiwis-poke.md @@ -0,0 +1,6 @@ +--- +'@apollo/server-integration-testsuite': minor +'@apollo/server': minor +--- + +New field `GraphQLRequestContext.requestIsBatched` available to plugins. diff --git a/packages/integration-testsuite/src/httpServerTests.ts b/packages/integration-testsuite/src/httpServerTests.ts index 42e54c6cbb8..3089d894c84 100644 --- a/packages/integration-testsuite/src/httpServerTests.ts +++ b/packages/integration-testsuite/src/httpServerTests.ts @@ -794,7 +794,18 @@ export function defineIntegrationTestSuiteHttpServerTests( }); it('can handle a basic request', async () => { - const app = await createApp(); + let requestIsBatched: boolean | undefined; + const app = await createApp({ + schema, + allowBatchedHttpRequests: true, + plugins: [ + { + async requestDidStart(requestContext) { + requestIsBatched = requestContext.requestIsBatched; + }, + }, + ], + }); const expected = { testString: 'it works', }; @@ -807,6 +818,7 @@ export function defineIntegrationTestSuiteHttpServerTests( 'application/json; charset=utf-8', ); expect(res.body.data).toEqual(expected); + expect(requestIsBatched).toEqual(false); }); it.each([ @@ -897,6 +909,9 @@ export function defineIntegrationTestSuiteHttpServerTests( books: [Book] cooks: [Cook] pooks: [Pook] + uncached: ID + ten: ID @cacheControl(maxAge: 10) + twenty: ID @cacheControl(maxAge: 20, scope: PRIVATE) } enum CacheControlScope { @@ -928,6 +943,98 @@ export function defineIntegrationTestSuiteHttpServerTests( expect(res.headers['cache-control']).toEqual('max-age=200, public'); }); + it('applies cacheControl Headers for batched operation', async () => { + const app = await createApp({ + typeDefs, + resolvers, + allowBatchedHttpRequests: true, + }); + { + const res = await request(app) + .post('/') + .send([{ query: '{ten}' }, { query: '{twenty}' }]); + expect(res.status).toEqual(200); + expect(res.body).toMatchInlineSnapshot(` + [ + { + "data": { + "ten": null, + }, + }, + { + "data": { + "twenty": null, + }, + }, + ] + `); + expect(res.headers['cache-control']).toEqual('max-age=10, private'); + } + { + const res = await request(app) + .post('/') + .send([{ query: '{twenty}' }, { query: '{ten}' }]); + expect(res.status).toEqual(200); + expect(res.body).toMatchInlineSnapshot(` + [ + { + "data": { + "twenty": null, + }, + }, + { + "data": { + "ten": null, + }, + }, + ] + `); + expect(res.headers['cache-control']).toEqual('max-age=10, private'); + } + { + const res = await request(app) + .post('/') + .send([{ query: '{uncached}' }, { query: '{ten}' }]); + expect(res.status).toEqual(200); + expect(res.body).toMatchInlineSnapshot(` + [ + { + "data": { + "uncached": null, + }, + }, + { + "data": { + "ten": null, + }, + }, + ] + `); + expect(res.headers['cache-control']).toEqual('no-store'); + } + { + const res = await request(app) + .post('/') + .send([{ query: '{ten}' }, { query: '{uncached}' }]); + expect(res.status).toEqual(200); + expect(res.body).toMatchInlineSnapshot(` + [ + { + "data": { + "ten": null, + }, + }, + { + "data": { + "uncached": null, + }, + }, + ] + `); + expect(res.headers['cache-control']).toEqual('no-store'); + } + }); + it('applies cacheControl Headers with if-cacheable', async () => { const app = await createApp({ typeDefs, @@ -1276,7 +1383,18 @@ export function defineIntegrationTestSuiteHttpServerTests( }); it('can handle batch requests', async () => { - const app = await createApp({ schema, allowBatchedHttpRequests: true }); + let requestIsBatched: boolean | undefined; + const app = await createApp({ + schema, + allowBatchedHttpRequests: true, + plugins: [ + { + async requestDidStart(requestContext) { + requestIsBatched = requestContext.requestIsBatched; + }, + }, + ], + }); const expected = [ { data: { @@ -1289,7 +1407,7 @@ export function defineIntegrationTestSuiteHttpServerTests( }, }, ]; - const req = request(app) + const res = await request(app) .post('/') .send([ { @@ -1306,13 +1424,12 @@ export function defineIntegrationTestSuiteHttpServerTests( operationName: 'testX', }, ]); - return req.then((res) => { - expect(res.status).toEqual(200); - expect(res.body).toEqual(expected); - expect(res.header['content-length']).toEqual( - Buffer.byteLength(res.text, 'utf8').toString(), - ); - }); + expect(res.status).toEqual(200); + expect(res.body).toEqual(expected); + expect(res.header['content-length']).toEqual( + Buffer.byteLength(res.text, 'utf8').toString(), + ); + expect(requestIsBatched).toBe(true); }); it('can handle non-batch requests when allowBatchedHttpRequests is true', async () => { diff --git a/packages/server/src/ApolloServer.ts b/packages/server/src/ApolloServer.ts index 3911cc5ae95..123de66aafb 100644 --- a/packages/server/src/ApolloServer.ts +++ b/packages/server/src/ApolloServer.ts @@ -1197,6 +1197,7 @@ export class ApolloServer { graphQLRequest, internals: this.internals, schemaDerivedData, + sharedResponseHTTPGraphQLHead: null, }, options, ); @@ -1215,11 +1216,13 @@ export async function internalExecuteOperation( graphQLRequest, internals, schemaDerivedData, + sharedResponseHTTPGraphQLHead, }: { server: ApolloServer; graphQLRequest: GraphQLRequest; internals: ApolloServerInternals; schemaDerivedData: SchemaDerivedData; + sharedResponseHTTPGraphQLHead: HTTPGraphQLHead | null; }, options: ExecuteOperationOptions, ): Promise { @@ -1229,7 +1232,7 @@ export async function internalExecuteOperation( schema: schemaDerivedData.schema, request: graphQLRequest, response: { - http: newHTTPGraphQLHead(), + http: sharedResponseHTTPGraphQLHead ?? newHTTPGraphQLHead(), }, // We clone the context because there are some assumptions that every operation // execution has a brand new context object; specifically, in order to implement @@ -1249,6 +1252,7 @@ export async function internalExecuteOperation( contextValue: cloneObject(options?.contextValue ?? ({} as TContext)), metrics: {}, overallCachePolicy: newCachePolicy(), + requestIsBatched: sharedResponseHTTPGraphQLHead !== null, }; try { diff --git a/packages/server/src/externalTypes/requestPipeline.ts b/packages/server/src/externalTypes/requestPipeline.ts index 19362bfad13..da84237cb97 100644 --- a/packages/server/src/externalTypes/requestPipeline.ts +++ b/packages/server/src/externalTypes/requestPipeline.ts @@ -70,6 +70,13 @@ export interface GraphQLRequestContext { readonly metrics: GraphQLRequestMetrics; readonly overallCachePolicy: CachePolicy; + + /** + * True if this request is part of a potentially multi-operation batch. Note + * that if this is true, `response.http` will be shared with the other + * operations in the batch. + */ + readonly requestIsBatched: boolean; } export type GraphQLRequestContextDidResolveSource< diff --git a/packages/server/src/httpBatching.ts b/packages/server/src/httpBatching.ts index a12f34c6ea3..bade864e51c 100644 --- a/packages/server/src/httpBatching.ts +++ b/packages/server/src/httpBatching.ts @@ -11,19 +11,32 @@ import type { import { newHTTPGraphQLHead, runHttpQuery } from './runHttpQuery.js'; import { BadRequestError } from './internalErrorClasses.js'; -export async function runBatchHttpQuery( - server: ApolloServer, - batchRequest: HTTPGraphQLRequest, - body: unknown[], - contextValue: TContext, - schemaDerivedData: SchemaDerivedData, - internals: ApolloServerInternals, -): Promise { +async function runBatchedHttpQuery({ + server, + batchRequest, + body, + contextValue, + schemaDerivedData, + internals, +}: { + server: ApolloServer; + batchRequest: HTTPGraphQLRequest; + body: unknown[]; + contextValue: TContext; + schemaDerivedData: SchemaDerivedData; + internals: ApolloServerInternals; +}): Promise { if (body.length === 0) { throw new BadRequestError('No operations found in request.'); } - const combinedResponseHead = newHTTPGraphQLHead(); + // This single HTTPGraphQLHead is shared across all the operations in the + // batch. This means that any changes to response headers or status code from + // one operation can be immediately seen by other operations. Plugins that set + // response headers or status code can then choose to combine the data they + // are setting with data that may already be there from another operation as + // they choose. + const sharedResponseHTTPGraphQLHead = newHTTPGraphQLHead(); const responseBodies = await Promise.all( body.map(async (bodyPiece: unknown) => { const singleRequest: HTTPGraphQLRequest = { @@ -31,33 +44,25 @@ export async function runBatchHttpQuery( body: bodyPiece, }; - const response = await runHttpQuery( + const response = await runHttpQuery({ server, - singleRequest, + httpRequest: singleRequest, contextValue, schemaDerivedData, internals, - ); + sharedResponseHTTPGraphQLHead, + }); if (response.body.kind === 'chunked') { throw Error( 'Incremental delivery is not implemented for batch requests', ); } - for (const [key, value] of response.headers) { - // Override any similar header set in other responses. - combinedResponseHead.headers.set(key, value); - } - // If two responses both want to set the status code, one of them will win. - // Note that the normal success case leaves status empty. - if (response.status) { - combinedResponseHead.status = response.status; - } return response.body.string; }), ); return { - ...combinedResponseHead, + ...sharedResponseHTTPGraphQLHead, body: { kind: 'complete', string: `[${responseBodies.join(',')}]` }, }; } @@ -77,23 +82,24 @@ export async function runPotentiallyBatchedHttpQuery< Array.isArray(httpGraphQLRequest.body) ) ) { - return await runHttpQuery( + return await runHttpQuery({ server, - httpGraphQLRequest, + httpRequest: httpGraphQLRequest, contextValue, schemaDerivedData, internals, - ); + sharedResponseHTTPGraphQLHead: null, + }); } if (internals.allowBatchedHttpRequests) { - return await runBatchHttpQuery( + return await runBatchedHttpQuery({ server, - httpGraphQLRequest, - httpGraphQLRequest.body as unknown[], + batchRequest: httpGraphQLRequest, + body: httpGraphQLRequest.body as unknown[], contextValue, schemaDerivedData, internals, - ); + }); } throw new BadRequestError('Operation batching disabled.'); } diff --git a/packages/server/src/plugin/cacheControl/index.ts b/packages/server/src/plugin/cacheControl/index.ts index 8462e66f573..48ed5f69077 100644 --- a/packages/server/src/plugin/cacheControl/index.ts +++ b/packages/server/src/plugin/cacheControl/index.ts @@ -272,21 +272,47 @@ export function ApolloServerPluginCacheControl( const { response, overallCachePolicy } = requestContext; - const policyIfCacheable = overallCachePolicy.policyIfCacheable(); + // Look to see if something has already set the cache-control header. + // This could be a different plugin... or it could be this very plugin + // operating on a different operation in the same batched HTTP + // request. + const existingCacheControlHeader = parseExistingCacheControlHeader( + response.http.headers.get('cache-control'), + ); + + // If the header contains something other than a value that this + // plugin sets, then we leave it alone. We don't want to mangle + // something important that you set! That said, it's probably best to + // have only one piece of code that writes to a given header, so you + // should probably set `calculateHttpHeaders: false` on this plugin. + if (existingCacheControlHeader.kind === 'unparsable') { + return; + } + + const cachePolicy = newCachePolicy(); + cachePolicy.replace(overallCachePolicy); + if (existingCacheControlHeader.kind === 'parsable-and-cacheable') { + cachePolicy.restrict(existingCacheControlHeader.hint); + } + const policyIfCacheable = cachePolicy.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. if ( + // This code path is only for if we believe it is cacheable. policyIfCacheable && + // Either there wasn't a cache-control header already, or we've + // already incorporated it into policyIfCacheable. (If we couldn't + // parse it, that means some other plugin or mechanism set the + // header. This is confusing, so we just don't make any more + // changes. You should probably set `calculateHttpHeaders` to false + // in that case and only set the header from one place.) + existingCacheControlHeader.kind !== 'uncacheable' && // At least for now, we don't set cache-control headers for // incremental delivery responses, since we don't know if a later // part of the execution will affect the cache policy (perhaps // dynamically). (Note that willSendResponse is called when the // initial payload is sent, not the final payload.) response.body.kind === 'single' && - !response.body.singleResult.errors && - response.http + !response.body.singleResult.errors ) { response.http.headers.set( 'cache-control', @@ -294,14 +320,16 @@ export function ApolloServerPluginCacheControl( policyIfCacheable.maxAge }, ${policyIfCacheable.scope.toLowerCase()}`, ); - } else if ( - calculateHttpHeaders !== 'if-cacheable' && - !response.http.headers.has('cache-control') - ) { + } else if (calculateHttpHeaders !== 'if-cacheable') { // The response is not cacheable, so make sure it doesn't get // cached. This is especially important for GET requests, because // browsers and other agents cache many GET requests by default. - response.http.headers.set('cache-control', 'no-store'); + // (But if some other plugin set the header to a value that this + // plugin does not produce, we don't do anything.) + response.http.headers.set( + 'cache-control', + CACHE_CONTROL_HEADER_UNCACHEABLE, + ); } }, }; @@ -309,6 +337,38 @@ export function ApolloServerPluginCacheControl( }); } +const CACHE_CONTROL_HEADER_CACHEABLE_REGEXP = + /^max-age=(\d+), (public|private)$/; +const CACHE_CONTROL_HEADER_UNCACHEABLE = 'no-store'; + +type ExistingCacheControlHeader = + | { kind: 'no-header' } + | { kind: 'uncacheable' } + | { kind: 'parsable-and-cacheable'; hint: CacheHint } + | { kind: 'unparsable' }; + +function parseExistingCacheControlHeader( + header: string | undefined, +): ExistingCacheControlHeader { + if (!header) { + return { kind: 'no-header' }; + } + if (header === CACHE_CONTROL_HEADER_UNCACHEABLE) { + return { kind: 'uncacheable' }; + } + const match = CACHE_CONTROL_HEADER_CACHEABLE_REGEXP.exec(header); + if (!match) { + return { kind: 'unparsable' }; + } + return { + kind: 'parsable-and-cacheable', + hint: { + maxAge: +match[1], + scope: match[2] === 'public' ? 'PUBLIC' : 'PRIVATE', + }, + }; +} + function cacheAnnotationFromDirectives( directives: ReadonlyArray | undefined, ): CacheAnnotation | undefined { diff --git a/packages/server/src/runHttpQuery.ts b/packages/server/src/runHttpQuery.ts index a6437a9808f..9086c68ad35 100644 --- a/packages/server/src/runHttpQuery.ts +++ b/packages/server/src/runHttpQuery.ts @@ -114,13 +114,21 @@ function ensureQueryIsStringOrMissing(query: unknown) { } } -export async function runHttpQuery( - server: ApolloServer, - httpRequest: HTTPGraphQLRequest, - contextValue: TContext, - schemaDerivedData: SchemaDerivedData, - internals: ApolloServerInternals, -): Promise { +export async function runHttpQuery({ + server, + httpRequest, + contextValue, + schemaDerivedData, + internals, + sharedResponseHTTPGraphQLHead, +}: { + server: ApolloServer; + httpRequest: HTTPGraphQLRequest; + contextValue: TContext; + schemaDerivedData: SchemaDerivedData; + internals: ApolloServerInternals; + sharedResponseHTTPGraphQLHead: HTTPGraphQLHead | null; +}): Promise { let graphQLRequest: GraphQLRequest; switch (httpRequest.method) { @@ -198,6 +206,7 @@ export async function runHttpQuery( graphQLRequest, internals, schemaDerivedData, + sharedResponseHTTPGraphQLHead, }, { contextValue }, );