Skip to content

Commit bf9577a

Browse files
tcarmettcarmet
committed
CLDSRV-881: Return CORS headers on 403 responses
Cross-origin browser clients lost Access-Control-* headers whenever cloudserver returned 403, even though the bucket had a matching CORS rule. Responses now include the CORS headers for error responses on buckets with CORS configured, matching AWS behavior.
1 parent c1d3d6a commit bf9577a

File tree

3 files changed

+465
-0
lines changed

3 files changed

+465
-0
lines changed

‎lib/api/api.js‎

Lines changed: 40 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -81,6 +81,8 @@ const { isRequesterASessionUser } = require('./apiUtils/authorization/permission
8181
const checkHttpHeadersSize = require('./apiUtils/object/checkHttpHeadersSize');
8282
const constants = require('../../constants');
8383
const { config } = require('../Config.js');
84+
const metadata = require('../metadata/wrapper');
85+
const collectCorsHeaders = require('../utilities/collectCorsHeaders');
8486
const { validateMethodChecksumNoChunking } = require('./apiUtils/integrity/validateChecksums');
8587
const {
8688
getRateLimitFromCache,
@@ -92,6 +94,42 @@ const monitoringMap = policies.actionMaps.actionMonitoringMapS3;
9294

9395
auth.setHandler(vault);
9496

97+
// Wrap the API callback so that, on error, we look up the bucket's CORS
98+
// configuration and set the matching Access-Control-* headers directly on
99+
// the HTTP response. This is needed because auth / pre-handler errors
100+
// return before the API handler retrieves the bucket (where the existing
101+
// collectCorsHeaders call lives), so the route-level error response path
102+
// otherwise sends no CORS headers - breaking cross-origin browser clients.
103+
// We only pay the extra metadata lookup when an Origin header is present,
104+
// matching AWS behavior and the existing contract of collectCorsHeaders.
105+
function wrapCallbackWithErrorCorsHeaders(request, response, log, callback) {
106+
return (err, ...rest) => {
107+
if (!err || !request.headers || !request.headers.origin
108+
|| !request.bucketName) {
109+
return callback(err, ...rest);
110+
}
111+
return metadata.getBucket(request.bucketName, log, (mdErr, bucket) => {
112+
if (!mdErr && bucket && !response.headersSent) {
113+
const headers = collectCorsHeaders(
114+
request.headers.origin, request.method, bucket);
115+
Object.keys(headers).forEach(key => {
116+
if (headers[key] === undefined) {
117+
return;
118+
}
119+
try {
120+
response.setHeader(key, headers[key]);
121+
} catch (e) {
122+
log.debug('could not set cors header on error', {
123+
header: key, error: e.message,
124+
});
125+
}
126+
});
127+
}
128+
return callback(err, ...rest);
129+
});
130+
};
131+
}
132+
95133
function checkAuthResults(authResults, apiMethod, log) {
96134
let returnTagCount = true;
97135
const isImplicitDeny = {};
@@ -158,6 +196,8 @@ const api = {
158196
callApiMethod(apiMethod, request, response, log, callback) {
159197
// Attach the apiMethod method to the request, so it can used by monitoring in the server
160198
request.apiMethod = apiMethod;
199+
callback = wrapCallbackWithErrorCorsHeaders(
200+
request, response, log, callback);
161201
// Array of end of API callbacks, used to perform some logic
162202
// at the end of an API.
163203
request.finalizerHooks = [];

‎tests/functional/aws-node-sdk/test/object/corsErrorHeaders.js‎

Lines changed: 152 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,152 @@
1+
const { S3 } = require('aws-sdk');
2+
const assert = require('assert');
3+
const async = require('async');
4+
5+
const getConfig = require('../support/config');
6+
const { methodRequest, generateCorsParams } =
7+
require('../../lib/utility/cors-util');
8+
9+
const config = getConfig('default', { signatureVersion: 'v4' });
10+
const s3 = new S3(config);
11+
12+
const bucket = 'corserrorheadertest';
13+
const objectKey = 'objectKey';
14+
const allowedOrigin = 'http://www.allowed.test';
15+
const vary = 'Origin, Access-Control-Request-Headers, '
16+
+ 'Access-Control-Request-Method';
17+
18+
const expectedCorsHeaders = {
19+
'access-control-allow-origin': allowedOrigin,
20+
'access-control-allow-methods': 'GET, PUT, POST, DELETE, HEAD',
21+
'access-control-allow-credentials': 'true',
22+
vary,
23+
};
24+
25+
const corsParams = generateCorsParams(bucket, {
26+
allowedMethods: ['GET', 'PUT', 'POST', 'DELETE', 'HEAD'],
27+
allowedOrigins: [allowedOrigin],
28+
allowedHeaders: ['*'],
29+
});
30+
31+
// Raw unauthenticated requests - they always return 403.
32+
// Each spec describes (method, path, query) against the bucket.
33+
const unauthenticatedRequests = [
34+
{ description: 'GET bucket (list objects)',
35+
method: 'GET', query: null, objectKey: null },
36+
{ description: 'HEAD bucket',
37+
method: 'HEAD', query: null, objectKey: null },
38+
{ description: 'DELETE bucket',
39+
method: 'DELETE', query: null, objectKey: null },
40+
{ description: 'GET bucket ACL',
41+
method: 'GET', query: 'acl', objectKey: null },
42+
{ description: 'GET bucket CORS',
43+
method: 'GET', query: 'cors', objectKey: null },
44+
{ description: 'GET bucket versioning',
45+
method: 'GET', query: 'versioning', objectKey: null },
46+
{ description: 'GET bucket website',
47+
method: 'GET', query: 'website', objectKey: null },
48+
{ description: 'GET bucket tagging',
49+
method: 'GET', query: 'tagging', objectKey: null },
50+
{ description: 'GET object',
51+
method: 'GET', query: null, objectKey },
52+
{ description: 'HEAD object',
53+
method: 'HEAD', query: null, objectKey },
54+
{ description: 'PUT object',
55+
method: 'PUT', query: null, objectKey },
56+
{ description: 'DELETE object',
57+
method: 'DELETE', query: null, objectKey },
58+
{ description: 'GET bucket uploads (list multipart uploads)',
59+
method: 'GET', query: 'uploads', objectKey: null },
60+
// GET bucket policy and POST multi-delete are not covered here: the
61+
// first returns 405 (method rejected pre-auth), the second returns 400
62+
// (missing XML body fails validation pre-auth). Neither reaches the
63+
// 403 path. Both are exercised via the unit test that stubs auth
64+
// failure directly.
65+
];
66+
67+
function _waitForAWS(callback, err) {
68+
if (err) {
69+
return setTimeout(() => callback(err), 500);
70+
}
71+
return setTimeout(() => callback(), 500);
72+
}
73+
74+
describe('CORS headers on 403 responses when bucket has CORS configured', () => {
75+
before(done => async.series([
76+
cb => s3.createBucket({ Bucket: bucket }, err => _waitForAWS(cb, err)),
77+
cb => s3.putBucketCors(corsParams, err => _waitForAWS(cb, err)),
78+
], done));
79+
80+
after(done => s3.deleteBucket({ Bucket: bucket },
81+
err => _waitForAWS(done, err)));
82+
83+
unauthenticatedRequests.forEach(spec => {
84+
it(`returns CORS headers on 403 for ${spec.description} `
85+
+ 'when Origin matches a rule', done => {
86+
methodRequest({
87+
method: spec.method,
88+
bucket,
89+
objectKey: spec.objectKey,
90+
query: spec.query,
91+
headers: { origin: allowedOrigin },
92+
// Use numeric status: HEAD responses have no body, and some
93+
// endpoints (bucket policy, multi-delete) can fail with a
94+
// non-AccessDenied body before auth even runs. We only care
95+
// about the 403 status and the CORS headers here.
96+
code: 403,
97+
headersResponse: expectedCorsHeaders,
98+
}, done);
99+
});
100+
});
101+
102+
it('omits CORS headers on 403 when Origin does not match any rule',
103+
done => {
104+
methodRequest({
105+
method: 'GET',
106+
bucket,
107+
query: null,
108+
objectKey: null,
109+
headers: { origin: 'http://not-allowed.test' },
110+
code: 403,
111+
// headersResponse unset -> cors-util asserts CORS headers
112+
// are NOT present.
113+
}, done);
114+
});
115+
116+
it('omits CORS headers on 403 when no Origin header is sent',
117+
done => {
118+
methodRequest({
119+
method: 'GET',
120+
bucket,
121+
query: null,
122+
objectKey: null,
123+
headers: {},
124+
code: 403,
125+
}, done);
126+
});
127+
});
128+
129+
describe('CORS headers on 200 responses (regression guard)', () => {
130+
before(done => async.series([
131+
cb => s3.createBucket({ Bucket: bucket }, err => _waitForAWS(cb, err)),
132+
cb => s3.putBucketCors(corsParams, err => _waitForAWS(cb, err)),
133+
], done));
134+
135+
after(done => s3.deleteBucket({ Bucket: bucket },
136+
err => _waitForAWS(done, err)));
137+
138+
it('returns CORS headers on a successful list objects (200)', done => {
139+
const request = s3.listObjects({ Bucket: bucket });
140+
request.on('build', () => {
141+
request.httpRequest.headers.origin = allowedOrigin;
142+
});
143+
request.on('success', response => {
144+
const h = response.httpResponse.headers;
145+
assert.strictEqual(h['access-control-allow-origin'],
146+
allowedOrigin);
147+
done();
148+
});
149+
request.on('error', err => done(err));
150+
request.send();
151+
});
152+
});

0 commit comments

Comments
 (0)