Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 25 additions & 1 deletion server/routes.ts
Original file line number Diff line number Diff line change
Expand Up @@ -307,7 +307,31 @@ export default async (app: express.Application) => {
},
};

app.use('/graphql', graphqlUploadExpress());
// Wrap graphqlUploadExpress with error handling to provide better error messages
// for multipart upload parsing failures (see https://github.com/opencollective/opencollective-api/issues/11293)
app.use('/graphql', (req, res, next) => {
const uploadMiddleware = graphqlUploadExpress();
uploadMiddleware(req, res, (err?: Error) => {
if (err) {
logger.error(`GraphQL upload error: ${err.message}`, { error: err });
Comment on lines +313 to +316
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug: The error handling callback for graphqlUploadExpress is never called because the middleware uses next(error) to propagate errors, bypassing the intended logic.
Severity: CRITICAL | Confidence: High

🔍 Detailed Analysis

The middleware wrapper for graphqlUploadExpress uses a callback (err) => {...} to handle file upload errors. However, the graphqlUploadExpress middleware follows the standard Express.js error handling pattern by calling next(error) upon failure. This bypasses the custom callback entirely, meaning the error handling logic within it will never be executed. As a result, file upload parsing errors will not be formatted as GraphQL responses and will instead be passed to the global error handler, defeating the purpose of the change.

💡 Suggested Fix

Remove the custom callback. Instead, pass next directly to the uploadMiddleware and let errors propagate to the standard Express error-handling middleware. The graphqlUploadExpress middleware should be invoked as uploadMiddleware(req, res, next); to allow Express to correctly route errors.

🤖 Prompt for AI Agent
Review the code at the location below. A potential bug has been identified by an AI
agent.
Verify if this is a real issue. If it is, propose a fix; if not, explain why it's not
valid.

Location: server/routes.ts#L313-L316

Potential issue: The middleware wrapper for `graphqlUploadExpress` uses a callback
`(err) => {...}` to handle file upload errors. However, the `graphqlUploadExpress`
middleware follows the standard Express.js error handling pattern by calling
`next(error)` upon failure. This bypasses the custom callback entirely, meaning the
error handling logic within it will never be executed. As a result, file upload parsing
errors will not be formatted as GraphQL responses and will instead be passed to the
global error handler, defeating the purpose of the change.

Did we get this right? 👍 / 👎 to inform future reviews.
Reference ID: 8139129

// Return a proper JSON error response instead of letting Express error handler return a generic 500
return res.status(400).json({
errors: [
{
message: `File upload failed: ${err.message}`,
extensions: {
code: 'BAD_REQUEST',
exception: {
name: err.name,
},
},
},
],
});
}
next();
});
});

/**
* GraphQL v1
Expand Down
82 changes: 82 additions & 0 deletions test/server/routes/graphql.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -219,3 +219,85 @@ describe('GraphQL Armor Protection Tests', () => {
});
});
});

describe('GraphQL Multipart Upload Error Handling', () => {
let app;

before(async () => {
app = await startTestServer();
});

after(async () => {
await stopTestServer();
});

describe('/graphql/v2', () => {
it('should return errors in GraphQL format for malformed multipart requests', async () => {
// Send a multipart request with invalid JSON in the operations field.
// This triggers an error in graphqlUploadExpress middleware.
// See: https://github.com/opencollective/opencollective-api/issues/11293
const boundary = '----WebKitFormBoundary7MA4YWxkTrZu0gW';
const body = [
`--${boundary}`,
'Content-Disposition: form-data; name="operations"',
'',
'{ invalid json }', // Invalid JSON triggers parsing error in graphqlUploadExpress
`--${boundary}`,
'Content-Disposition: form-data; name="map"',
'',
'{}',
`--${boundary}--`,
].join('\r\n');

const response = await request(app)
.post('/graphql/v2')
.set('Content-Type', `multipart/form-data; boundary=${boundary}`)
.set('Authorization', 'Bearer test-token')
.send(body);

// The error should be returned in GraphQL format (errors array) not Express format (error object)
// This is important because GraphQL clients expect the standard errors array format.
expect(response.status).to.eq(400);
expect(response.body.errors).to.exist;
expect(response.body.errors).to.be.an('array');
expect(response.body.errors[0].message).to.include('File upload failed');
});

it('should return GraphQL response (not 500) for valid multipart upload requests', async () => {
// Send a valid multipart upload request following GraphQL multipart spec.
// Even if auth fails, the response should be in GraphQL format, not a raw 500.
// See: https://github.com/opencollective/opencollective-api/issues/11293
const response = await request(app)
.post('/graphql/v2')
.set('Authorization', 'Bearer test-token')
.field(
'operations',
JSON.stringify({
query: `mutation UploadFile($files: [UploadFileInput!]!) {
uploadFile(files: $files) { file { id url name } }
}`,
variables: { files: [{ kind: 'EXPENSE_ITEM', file: null }] },
}),
)
.field('map', JSON.stringify({ '0': ['variables.files.0.file'] }))
.attach('0', Buffer.from('test file content'), {
filename: 'test.txt',
contentType: 'text/plain',
});

// Should NOT return 500 - should return proper GraphQL response
// Even if auth fails (401-ish error), it should be in GraphQL format
expect(response.status).to.not.eq(500);

// If there's an error, it should be in GraphQL format
if (response.body.errors) {
expect(response.body.errors).to.be.an('array');
expect(response.body.errors[0]).to.have.property('message');
}
// If successful, it should have data
if (response.body.data) {
expect(response.body.data).to.have.property('uploadFile');
}
});
});
});