Skip to content

Commit

Permalink
Disable graphql-upload integration when it is not used (#6476)
Browse files Browse the repository at this point in the history
By default, we run the graphql-upload middleware on all requests. This
middleware is vulnerable to mutation CSRF attacks because it parses POST
requests with `content-type: multipart/form-data`, which can happen in a
non-preflighted browser request. (Without graphql-upload, Apollo Server
won't process any mutations in non-preflighted requests, because
mutations must be in POST requests and normally that requires
`content-type: application/json` which must be preflighted.)

In order to safely use graphql-upload, you should upgrade to Apollo
Server v3.7 and use its new CSRF prevention feature. Because Apollo
Server 2 is not under active development we do not intend to backport
the full CSRF prevention feature to AS2.

However, we at least want to protect the users of Apollo Server 2 who
*don't* actually need graphql-upload to be enabled (which is probably
most of them). This PR changes the default behavior of Apollo Server 2
when no `uploads` parameter is passed. Instead of always executing the
graphql-upload middleware in this case, we only execute it if the
`Upload` scalar (which may be added automatically to the schema by AS
itself or may be provided by the user) is referenced somewhere in the
schema other than its own definition. This should be roughly
backwards-compatible; it only breaks the ability to use a
`graphql-upload`-based client with Apollo Servers that don't accept
uploads.

We also print a warning when uploads are enabled encouraging upgrades.

Part of GHSA-2p3c-p3qw-69r4
  • Loading branch information
glasser authored May 25, 2022
1 parent c98507e commit 82d4498
Show file tree
Hide file tree
Showing 12 changed files with 175 additions and 10 deletions.
2 changes: 1 addition & 1 deletion .nvmrc
Original file line number Diff line number Diff line change
@@ -1 +1 @@
v14
v12
3 changes: 3 additions & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -11,6 +11,9 @@ The version headers in this history reflect the versions of Apollo Server itself

> The changes noted within this `vNEXT` section have not been released yet. New PRs and commits which introduce changes should include an entry in this `vNEXT` section as part of their development. With few exceptions, the format of the entry should follow convention (i.e., prefix with package name, use markdown `backtick formatting` for package names and code, suffix with a link to the change-set à la `[PR #YYY](https://link/pull/YYY)`, etc.). When a release is being prepared, a new header will be (manually) created below and the appropriate changes within that release will be moved into the new section.
## v2.25.4

- ⚠️ **SECURITY**: If your server does not explicitly enable `graphql-upload` support via the `uploads` option to `new ApolloServer` and your schema does not use the `Upload` scalar (other than in its own definition), Apollo Server will not process the `multipart/form-data` requests sent by `graphql-upload` clients. This fixes a Cross-Site Request Forgery (CSRF) vulnerability where origins could cause browsers to execute mutations using a user's cookies even when those origins are not allowed by your CORS policy. If you *do* use uploads in your server, the vulnerability still exists with this version; you should instead upgrade to Apollo Server v3.7 and enable the CSRF prevention feature. (The AS3.7 CSRF prevention feature also protects against other forms of CSRF such as timing attacks against read-only query operations.) See [advisory GHSA-2p3c-p3qw-69r4](https://github.com/apollographql/apollo-server/security/advisories/GHSA-2p3c-p3qw-69r4) for more details.

## v2.25.3

Expand Down
8 changes: 5 additions & 3 deletions docs/source/api/apollo-server.md
Original file line number Diff line number Diff line change
Expand Up @@ -158,7 +158,7 @@ An executable GraphQL schema. You usually don't need to provide this value, beca

This field is most commonly used with [Apollo Federation](https://www.apollographql.com/docs/federation/implementing-services/#generating-a-federated-schema), which uses a special `buildFederatedSchema` function to generate its schema.

Note that if you are using [file uploads](../data/file-uploads/), you need to add the `Upload` scalar to your schema manually before providing it here, as Apollo Server's automatic uploads integration does not apply if you use this particular option..
Note that if you are using [file uploads](../data/file-uploads/), you need to add the `Upload` scalar to your schema manually before providing it here, as Apollo Server's automatic uploads integration does not apply if you use this particular option. (We do not recommend the use of file uploads in Apollo Server 2 for security reasons detailed on its page.)
</td>
</tr>

Expand Down Expand Up @@ -235,9 +235,11 @@ An object containing custom functions to use as additional [validation rules](ht
</td>
<td>

By default, Apollo Server 2 [integrates](../data/file-uploads/) an outdated version of [the `graphql-upload` npm module](https://www.npmjs.com/package/graphql-upload#graphql-upload) into your schema which does not support Node 14. If you provide an object here, it will be passed as the third `options` argument to that module's `processRequest` option.
By default, Apollo Server 2 [integrates](../data/file-uploads/) an outdated version of [the `graphql-upload` npm module](https://www.npmjs.com/package/graphql-upload#graphql-upload) into your schema which does not support Node 14 or newer. (As of v2.25.4, it is only enabled by default if you actually use the `Upload` scalar in your schema outside of its definition.) If you provide an object here, it will be passed as the third `options` argument to that module's `processRequest` option.

We recommend instead that you pass `false` here, which will disable Apollo Server's integration with `graphql-upload`, and instead integrate the latest version of that module directly. This integration will be removed in Apollo Server 3.
We recommend instead that you pass `false` here, which will disable Apollo Server's integration with `graphql-upload`. We then recommend that you find an alternate mechanism for accepting uploads in your app that does not involve `graphql-upload`, or that you upgrade to Apollo Server 3.7 and use its [CSRF prevention feature](https://www.apollographql.com/docs/apollo-server/security/cors/#preventing-cross-site-request-forgery-csrf).

If you must use the upload feature with Apollo Server 2, then you can do so on Node 14 by passing `false` here and directly integrating the latest version of `graphql-upload` directly. This will probably expose your server to CSRF mutation vulnerabilities so you must find some way of protecting yourself. (We recommend protecting yourself by upgrading and using our protection feature as described in the previous paragraph.) This integration has been removed in Apollo Server 3.
</td>
</tr>

Expand Down
4 changes: 3 additions & 1 deletion docs/source/data/file-uploads.md
Original file line number Diff line number Diff line change
Expand Up @@ -3,9 +3,11 @@ title: File uploads
description: Enabling file uploads in Apollo Server
---

> **WARNING**: The file upload mechanism described in this file (which we removed in Apollo Server 3) inherently exposes your server to [CSRF mutation attacks](https://www.apollographql.com/docs/apollo-server/security/cors/#preventing-cross-site-request-forgery-csrf). These attacks allow untrusted websites to ask users' browsers to send mutations to your Apollo Server which can execute even if your server's CORS security policy specifies that the origin in question should not be able to send that request. This is because the `multipart/form-data` content type that is parsed by the file upload feature is special-cased by browsers and can be sent in a POST request without the browser needing to "ask permission" via a "preflight" OPTIONS request. Attackers can run any mutation with your cookies, not just upload-specific mutations. Since Apollo Server v2.25.4, we no longer automatically enable this `multipart/form-data` parser unless you explicitly enable it with the `uploads` option to `new ApolloServer()` or if your schema uses the `Upload` scalar in it; in this case, your server will be protected from the CSRF mutation vulnerability. You can also pass `uploads: false` to `new ApolloServer()` in any version of Apollo Server 2 to ensure that this dangerous parser is disabled. If you actually use the upload feature (particularly if your app uses cookies for authentication), we highly encourage you to [upgrade to Apollo Server v3.7 or newer](https://www.apollographql.com/docs/apollo-server/migration/) and enable its [CSRF prevention feature](https://www.apollographql.com/docs/apollo-server/security/cors/#preventing-cross-site-request-forgery-csrf), or remove the use of uploads from your server.

> Note: Apollo Server's built-in file upload mechanism is not fully supported in Node 14 and later, and it will be removed in Apollo Server 3. For details, [see below](#uploads-in-node-14-and-later).
For server integrations that support file uploads (e.g. Express, hapi, Koa), Apollo Server enables file uploads by default. To enable file uploads, reference the `Upload` type in the schema passed to the Apollo Server construction.
For server integrations that support file uploads (e.g. Express, hapi, Koa), Apollo Server enables file uploads by default if your schema references the `Upload` type.

```js
const { ApolloServer, gql } = require('apollo-server');
Expand Down
67 changes: 66 additions & 1 deletion packages/apollo-server-core/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,6 +20,9 @@ import {
FieldDefinitionNode,
DocumentNode,
print,
printSchema,
parse,
visit,
} from 'graphql';
import resolvable, { Resolvable } from '@josephg/resolvable';
import { GraphQLExtension } from 'graphql-extensions';
Expand Down Expand Up @@ -119,6 +122,7 @@ type SchemaDerivedData = {
// versions of operations in-memory, allowing subsequent parses/validates
// on the same operation to be executed immediately.
documentStore?: InMemoryLRUCache<DocumentNode>;
disableUploads: boolean;
};

type ServerState =
Expand Down Expand Up @@ -160,6 +164,7 @@ export class ApolloServerBase {

protected subscriptionServerOptions?: SubscriptionServerOptions;
protected uploadsConfig?: FileUploadOptions;
private disableUploadsIfSchemaDoesNotUseUploadScalar: boolean;

// set by installSubscriptionHandlers.
private subscriptionServer?: SubscriptionServer;
Expand Down Expand Up @@ -284,6 +289,7 @@ export class ApolloServerBase {

this.requestOptions = requestOptions as GraphQLServerOptions;

this.disableUploadsIfSchemaDoesNotUseUploadScalar = false;
if (uploads !== false && !forbidUploadsForTesting) {
if (this.supportsUploads()) {
if (!runtimeSupportsUploads) {
Expand All @@ -294,10 +300,15 @@ export class ApolloServerBase {
);
}

if (uploads === true || typeof uploads === 'undefined') {
if (uploads === true) {
this.uploadsConfig = {};
warnAboutUploads(this.logger, false);
} else if (typeof uploads === 'undefined') {
this.uploadsConfig = {};
this.disableUploadsIfSchemaDoesNotUseUploadScalar = true;
} else {
this.uploadsConfig = uploads;
warnAboutUploads(this.logger, false);
}
//This is here to check if uploads is requested without support. By
//default we enable them if supported by the integration
Expand Down Expand Up @@ -845,14 +856,55 @@ export class ApolloServerBase {
// Initialize the document store. This cannot currently be disabled.
const documentStore = this.initializeDocumentStore();

// If `uploads` wasn't explicitly specified, we look to see if the schema
// uses the Upload scalar and if it doesn't, we disable the upload
// middleware.
let disableUploads = false;
if (this.disableUploadsIfSchemaDoesNotUseUploadScalar) {
// We're not going to stress too much about error handling
// in this patch to an old version of Apollo Server. If
// this crashes for anyone they can file a bug and in the
// meantime pass `uploads: true` or `uploads: false` explicitly
// to work around.
const ast = parse(printSchema(schema));

// Assume that we are going to disable it unless we find a use of Upload.
disableUploads = true;

visit(ast, {
// This will find things like field arguments and input object fields
// (including if it's nested in list or non-null). Notably, it will
// *not* find the `scalar Upload` definition that we may have auto-added
// to the schema because ScalarTypeDefinitionNode has a "NameNode", not
// a "NamedTypeNode".
NamedType(node) {
if (node.name.value === 'Upload') {
disableUploads = false;
}
}
})

if (!disableUploads) {
warnAboutUploads(this.logger, true);
}
}

return {
schema,
schemaHash,
extensions,
documentStore,
disableUploads,
};
}

// Let the middleware know dynamically whether we are disabling uploads.
// (this.uploadsConfig also needs to be true in order to enable uploads.)
public disableUploads(): boolean {
// If the server isn't started, we shouldn't be processing operations anyway.
return this.state.phase !== 'started' || this.state.schemaDerivedData.disableUploads;
}

public async stop() {
// Calling stop more than once should have the same result as the first time.
if (this.state.phase === 'stopped') {
Expand Down Expand Up @@ -1353,3 +1405,16 @@ function printNodeFileUploadsMessage(logger: Logger) {
].join('\n'),
);
}

function warnAboutUploads(logger: Logger, implicit: boolean) {
logger.error([
'The third-party `graphql-upload` package is enabled in your server',
(implicit
? 'because you use the `Upload` scalar in your schema.'
: 'because you explicitly enabled it with the `uploads` option.'),
'This package is vulnerable to Cross-Site Request Forgery (CSRF) attacks.',
'We recommend you either disable uploads if it is not a necessary part of',
'your server, or upgrade to Apollo Server 3.7 and enable CSRF prevention.',
'See https://go.apollo.dev/s/graphql-upload-csrf for more details.',
].join('\n'))
}
1 change: 1 addition & 0 deletions packages/apollo-server-express/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,7 @@ const fileUploadMiddleware = (
) => {
// Note: we use typeis directly instead of via req.is for connect support.
if (
!server.disableUploads() &&
typeof processFileUploads === 'function' &&
typeis(req, ['multipart/form-data'])
) {
Expand Down
91 changes: 91 additions & 0 deletions packages/apollo-server-express/src/__tests__/ApolloServer.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -558,6 +558,97 @@ describe('apollo-server-express', () => {
if (error.code !== 'EPIPE') throw error;
}
});

it('multipart requests work when Upload in schema', async () => {
const { port } = await createServer({
typeDefs: gql`
type Query {
f(f: Upload!): ID
}
`,
});

const body = new FormData();
body.append(
'operations',JSON.stringify({ query: '{__typename}',
}),
);
body.append('map', '{}');

try {
const resolved = await fetch(`http://localhost:${port}/graphql`, {
method: 'POST',
body: body as any,
});
const response = await resolved.json();
expect(response).toEqual({data: {__typename: 'Query'}});
} catch (error) {
// This error began appearing randomly and seems to be a dev dependency bug.
// https://github.com/jaydenseric/apollo-upload-server/blob/18ecdbc7a1f8b69ad51b4affbd986400033303d4/test.js#L39-L42
if (error.code !== 'EPIPE') throw error;
}
});

it('multipart requests work when uploads: true is passed', async () => {
const { port } = await createServer({
typeDefs: gql`
type Query {
f: ID
}
`,
uploads: true,
});

const body = new FormData();
body.append(
'operations',JSON.stringify({ query: '{__typename}',
}),
);
body.append('map', '{}');

try {
const resolved = await fetch(`http://localhost:${port}/graphql`, {
method: 'POST',
body: body as any,
});
const response = await resolved.json();
expect(response).toEqual({data: {__typename: 'Query'}});
} catch (error) {
// This error began appearing randomly and seems to be a dev dependency bug.
// https://github.com/jaydenseric/apollo-upload-server/blob/18ecdbc7a1f8b69ad51b4affbd986400033303d4/test.js#L39-L42
if (error.code !== 'EPIPE') throw error;
}
});

it('multipart requests do not work when uploads unspecified and Upload is not used in the schema', async () => {
const { port } = await createServer({
typeDefs: gql`
type Query {
f: ID
}
`,
});

const body = new FormData();
body.append(
'operations',JSON.stringify({ query: '{__typename}',
}),
);
body.append('map', '{}');

try {
const resolved = await fetch(`http://localhost:${port}/graphql`, {
method: 'POST',
body: body as any,
});
const response = await resolved.text();
expect(response).toEqual("POST body missing. Did you forget use body-parser middleware?");
} catch (error) {
// This error began appearing randomly and seems to be a dev dependency bug.
// https://github.com/jaydenseric/apollo-upload-server/blob/18ecdbc7a1f8b69ad51b4affbd986400033303d4/test.js#L39-L42
if (error.code !== 'EPIPE') throw error;
}
});
},
);

Expand Down
1 change: 1 addition & 0 deletions packages/apollo-server-fastify/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,6 +41,7 @@ const fileUploadMiddleware = (
done: (err: Error | null, body?: any) => void,
) => {
if (
!server.disableUploads() &&
(req.req as any)[kMultipart] &&
typeof processFileUploads === 'function'
) {
Expand Down
2 changes: 1 addition & 1 deletion packages/apollo-server-hapi/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,7 @@ export class ApolloServer extends ApolloServerBase {
return h.continue;
}

if (this.uploadsConfig && typeof processFileUploads === 'function') {
if (this.uploadsConfig && !this.disableUploads() && typeof processFileUploads === 'function') {
await handleFileUploads(this.uploadsConfig)(request);
}

Expand Down
2 changes: 1 addition & 1 deletion packages/apollo-server-koa/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ const fileUploadMiddleware = (
uploadsConfig: FileUploadOptions,
server: ApolloServerBase,
) => async (ctx: Koa.Context, next: Function) => {
if (typeis(ctx.req, ['multipart/form-data'])) {
if (!server.disableUploads() && typeis(ctx.req, ['multipart/form-data'])) {
try {
ctx.request.body = await processFileUploads(
ctx.req,
Expand Down
2 changes: 1 addition & 1 deletion packages/apollo-server-lambda/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -296,7 +296,7 @@ export class ApolloServer<EventT extends APIGatewayProxyEventV1OrV2 = APIGateway
ReturnType<Exclude<typeof processFileUploads, undefined>>
>
| undefined;
if (isMultipart && typeof processFileUploads === 'function') {
if (isMultipart && !this.disableUploads() && typeof processFileUploads === 'function') {
const request = new FileUploadRequest() as IncomingMessage;
request.push(
Buffer.from(
Expand Down
2 changes: 1 addition & 1 deletion packages/apollo-server-micro/src/ApolloServer.ts
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,7 @@ export class ApolloServer extends ApolloServerBase {
return async (req, res) => {
this.graphqlPath = path || '/graphql';

if (typeof processFileUploads === 'function') {
if (!this.disableUploads() && typeof processFileUploads === 'function') {
await this.handleFileUploads(req, res);
}

Expand Down

0 comments on commit 82d4498

Please sign in to comment.