CLDSRV-881: fix missing CORS headers on 403 responses

lib/api/api.js

@@ -81,6 +81,8 @@ const { isRequesterASessionUser } = require('./apiUtils/authorization/permission
 const checkHttpHeadersSize = require('./apiUtils/object/checkHttpHeadersSize');
 const constants = require('../../constants');
 const { config } = require('../Config.js');
+const metadata = require('../metadata/wrapper');
+const collectCorsHeaders = require('../utilities/collectCorsHeaders');
 const { validateMethodChecksumNoChunking } = require('./apiUtils/integrity/validateChecksums');
 const {
     getRateLimitFromCache,
@@ -92,6 +94,95 @@ const monitoringMap = policies.actionMaps.actionMonitoringMapS3;
 
 auth.setHandler(vault);
 
+// Detect CORS headers the handler already attached to its callback args.
+// Callback arity varies per route ((err, corsHeaders), (err, xml,
+// corsHeaders), (err, dataGetInfo, resMetaHeaders, range), ...) so we
+// scan the args instead of relying on a fixed position.
+function hasCorsHeaders(rest) {
+    for (const arg of rest) {
+        if (!arg || typeof arg !== 'object' || Array.isArray(arg)
+            || Buffer.isBuffer(arg)) {
+            continue;
+        }
+        if ('access-control-allow-origin' in arg) {
+            return true;
+        }
+    }
+    return false;
+}
+
+// Wrap the API callback so that, on error, we look up the bucket's CORS
+// configuration and set the matching Access-Control-* headers directly on
+// the HTTP response. This is needed because auth / pre-handler errors
+// return before the API handler retrieves the bucket (where the existing
+// collectCorsHeaders call lives), so the route-level error response path
+// otherwise sends no CORS headers - breaking cross-origin browser clients.
+// We only pay the extra metadata lookup when an Origin header is present,
+// matching AWS behavior and the existing contract of collectCorsHeaders.
+//
+// On the error path of a single request, this wrapper produces at most one
+// extra metadata.getBucket call - the fallback below. It only fires when:
+//   1. The handler ran and called metadata.getBucket itself (count = 1), AND
+//   2. It returned without surfacing CORS headers in its callback args
+//      (e.g. handler dropped the loaded bucket between waterfall steps,
+//      or the request did not match any CORS rule and the result was {}).
+// Combined with the handler's own call, total getBucket calls per failing
+// request stay capped at 2. Any future optimization must preserve or
+// improve that ceiling; the unit test in tests/unit/api/corsErrorHeaders.js
+// asserts it.
+function wrapCallbackWithErrorCorsHeaders(request, response, log, callback) {
+    function applyHeadersFromBucket(bucket) {
+        if (!bucket || response.headersSent) {
+            return;
+        }
+        const headers = collectCorsHeaders(
+            request.headers.origin, request.method, bucket);
+        Object.keys(headers).forEach(key => {
+            if (headers[key] === undefined) {
+                return;
+            }
+            try {
+                response.setHeader(key, headers[key]);
+            } catch (e) {
+                log.debug('could not set cors header on error', {
+                    header: key, error: e.message,
+                });
+            }
+        });
+    }
+
+    return (err, ...rest) => {
+        if (!err || !request.headers || !request.headers.origin
+            || !request.bucketName) {
+            return callback(err, ...rest);
+        }
+        // Fast path: most post-auth failures come back with corsHeaders
+        // already computed by the handler. The route will forward them
+        // to responseXMLBody/responseNoBody, so no extra lookup is needed.
+        if (hasCorsHeaders(rest)) {
+            return callback(err, ...rest);
+        }
+        // Fallback: either the bucket was never loaded (pre-handler
+        // failure like auth.server.doAuth denial, header-size check,
+        // copy-source parse error) or the handler returned without
+        // surfacing CORS headers. Re-fetch the bucket using the
+        // request's bucketName (the destination on copy operations) so
+        // we evaluate CORS against the right bucket.
+        return metadata.getBucket(request.bucketName, log, (mdErr, bucket) => {
+            if (mdErr) {
+                log.warn('could not fetch bucket CORS config for error '
+                    + 'response', {
+                    bucketName: request.bucketName,
+                    error: mdErr.code || mdErr.message,
+                });
+                return callback(err, ...rest);
+            }
+            applyHeadersFromBucket(bucket);
+            return callback(err, ...rest);
+        });
+    };
+}
+
 function checkAuthResults(authResults, apiMethod, log) {
     let returnTagCount = true;
     const isImplicitDeny = {};
@@ -158,6 +249,8 @@ const api = {
     callApiMethod(apiMethod, request, response, log, callback) {
         // Attach the apiMethod method to the request, so it can used by monitoring in the server
         request.apiMethod = apiMethod;
+        callback = wrapCallbackWithErrorCorsHeaders(
+            request, response, log, callback);
         // Array of end of API callbacks, used to perform some logic
         // at the end of an API.
         request.finalizerHooks = [];

lib/api/bucketGetCors.js

@@ -29,10 +29,7 @@ function bucketGetCors(authInfo, request, log, callback) {
         const corsHeaders = collectCorsHeaders(request.headers.origin, request.method, bucket);
         if (err) {
             monitoring.promMetrics('GET', bucketName, err.code, METRICS_ACTION);
-            if (err?.is?.AccessDenied) {
-                return callback(err, corsHeaders);
-            }
-            return callback(err);
+            return callback(err, null, corsHeaders);
         }
 
         const cors = bucket.getCors();

lib/api/bucketGetLocation.js

@@ -29,10 +29,7 @@ function bucketGetLocation(authInfo, request, log, callback) {
         const corsHeaders = collectCorsHeaders(request.headers.origin, request.method, bucket);
         if (err) {
             monitoring.promMetrics('GET', bucketName, err.code, METRICS_ACTION);
-            if (err?.is?.AccessDenied) {
-                return callback(err, corsHeaders);
-            }
-            return callback(err);
+            return callback(err, null, corsHeaders);
         }
 
         let locationConstraint = bucket.getLocationConstraint();

lib/api/bucketGetWebsite.js

@@ -30,10 +30,7 @@ function bucketGetWebsite(authInfo, request, log, callback) {
         const corsHeaders = collectCorsHeaders(request.headers.origin, request.method, bucket);
         if (err) {
             monitoring.promMetrics('GET', bucketName, err.code, METRICS_ACTION);
-            if (err?.is?.AccessDenied) {
-                return callback(err, corsHeaders);
-            }
-            return callback(err);
+            return callback(err, null, corsHeaders);
         }
 
         const websiteConfig = bucket.getWebsiteConfiguration();

lib/utilities/collectCorsHeaders.js

@@ -9,15 +9,11 @@ const { findCorsRule, generateCorsResHeaders } =
  * @return {object} - object containing CORS headers
  */
 function collectCorsHeaders(origin, httpMethod, bucket) {
-    // NOTE: Because collecting CORS headers requires making a call to
-    // metadata to retrieve the bucket's CORS configuration, we opt not to
-    // return the CORS headers if the request encounters an error before
-    // the api method retrieves the bucket from metadata (an example
-    // being if a request is not properly authenticated). This is a slight
-    // deviation from AWS compatibility, but has the benefit of avoiding
-    // additional backend calls for an invalid request. Also, we anticipate
-    // that the preflight OPTIONS route will serve most client needs regarding
-    // CORS.
+    // Returns {} when no bucket is supplied; this function does not itself
+    // fetch metadata. Callers that only have a bucketName (e.g. auth
+    // failures that happen before the API handler loads the bucket) should
+    // look it up themselves - see wrapCallbackWithErrorCorsHeaders in
+    // lib/api/api.js.
     if (!origin || !bucket) {
         return {};
     }

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

@@ -0,0 +1,152 @@
+const { S3 } = require('aws-sdk');
+const assert = require('assert');
+const async = require('async');
+
+const getConfig = require('../support/config');
+const { methodRequest, generateCorsParams } =
+    require('../../lib/utility/cors-util');
+
+const config = getConfig('default', { signatureVersion: 'v4' });
+const s3 = new S3(config);
+
+const bucket = 'corserrorheadertest';
+const objectKey = 'objectKey';
+const allowedOrigin = 'http://www.allowed.test';
+const vary = 'Origin, Access-Control-Request-Headers, '
+    + 'Access-Control-Request-Method';
+
+const expectedCorsHeaders = {
+    'access-control-allow-origin': allowedOrigin,
+    'access-control-allow-methods': 'GET, PUT, POST, DELETE, HEAD',
+    'access-control-allow-credentials': 'true',
+    vary,
+};
+
+const corsParams = generateCorsParams(bucket, {
+    allowedMethods: ['GET', 'PUT', 'POST', 'DELETE', 'HEAD'],
+    allowedOrigins: [allowedOrigin],
+    allowedHeaders: ['*'],
+});
+
+// Raw unauthenticated requests - they always return 403.
+// Each spec describes (method, path, query) against the bucket.
+const unauthenticatedRequests = [
+    { description: 'GET bucket (list objects)',
+        method: 'GET', query: null, objectKey: null },
+    { description: 'HEAD bucket',
+        method: 'HEAD', query: null, objectKey: null },
+    { description: 'DELETE bucket',
+        method: 'DELETE', query: null, objectKey: null },
+    { description: 'GET bucket ACL',
+        method: 'GET', query: 'acl', objectKey: null },
+    { description: 'GET bucket CORS',
+        method: 'GET', query: 'cors', objectKey: null },
+    { description: 'GET bucket versioning',
+        method: 'GET', query: 'versioning', objectKey: null },
+    { description: 'GET bucket website',
+        method: 'GET', query: 'website', objectKey: null },
+    { description: 'GET bucket tagging',
+        method: 'GET', query: 'tagging', objectKey: null },
+    { description: 'GET object',
+        method: 'GET', query: null, objectKey },
+    { description: 'HEAD object',
+        method: 'HEAD', query: null, objectKey },
+    { description: 'PUT object',
+        method: 'PUT', query: null, objectKey },
+    { description: 'DELETE object',
+        method: 'DELETE', query: null, objectKey },
+    { description: 'GET bucket uploads (list multipart uploads)',
+        method: 'GET', query: 'uploads', objectKey: null },
+    // GET bucket policy and POST multi-delete are not covered here: the
+    // first returns 405 (method rejected pre-auth), the second returns 400
+    // (missing XML body fails validation pre-auth). Neither reaches the
+    // 403 path. Both are exercised via the unit test that stubs auth
+    // failure directly.
+];
+
+function _waitForAWS(callback, err) {
+    if (err) {
+        return setTimeout(() => callback(err), 500);
+    }
+    return setTimeout(() => callback(), 500);
+}
+
+describe('CORS headers on 403 responses when bucket has CORS configured', () => {
+    before(done => async.series([
+        cb => s3.createBucket({ Bucket: bucket }, err => _waitForAWS(cb, err)),
+        cb => s3.putBucketCors(corsParams, err => _waitForAWS(cb, err)),
+    ], done));
+
+    after(done => s3.deleteBucket({ Bucket: bucket },
+        err => _waitForAWS(done, err)));
+
+    unauthenticatedRequests.forEach(spec => {
+        it(`returns CORS headers on 403 for ${spec.description} `
+            + 'when Origin matches a rule', done => {
+            methodRequest({
+                method: spec.method,
+                bucket,
+                objectKey: spec.objectKey,
+                query: spec.query,
+                headers: { origin: allowedOrigin },
+                // Use numeric status: HEAD responses have no body, and some
+                // endpoints (bucket policy, multi-delete) can fail with a
+                // non-AccessDenied body before auth even runs. We only care
+                // about the 403 status and the CORS headers here.
+                code: 403,
+                headersResponse: expectedCorsHeaders,
+            }, done);
+        });
+    });
+
+    it('omits CORS headers on 403 when Origin does not match any rule',
+        done => {
+            methodRequest({
+                method: 'GET',
+                bucket,
+                query: null,
+                objectKey: null,
+                headers: { origin: 'http://not-allowed.test' },
+                code: 403,
+                // headersResponse unset -> cors-util asserts CORS headers
+                // are NOT present.
+            }, done);
+        });
+
+    it('omits CORS headers on 403 when no Origin header is sent',
+        done => {
+            methodRequest({
+                method: 'GET',
+                bucket,
+                query: null,
+                objectKey: null,
+                headers: {},
+                code: 403,
+            }, done);
+        });
+});
+
+describe('CORS headers on 200 responses (regression guard)', () => {
+    before(done => async.series([
+        cb => s3.createBucket({ Bucket: bucket }, err => _waitForAWS(cb, err)),
+        cb => s3.putBucketCors(corsParams, err => _waitForAWS(cb, err)),
+    ], done));
+
+    after(done => s3.deleteBucket({ Bucket: bucket },
+        err => _waitForAWS(done, err)));
+
+    it('returns CORS headers on a successful list objects (200)', done => {
+        const request = s3.listObjects({ Bucket: bucket });
+        request.on('build', () => {
+            request.httpRequest.headers.origin = allowedOrigin;
+        });
+        request.on('success', response => {
+            const h = response.httpResponse.headers;
+            assert.strictEqual(h['access-control-allow-origin'],
+                allowedOrigin);
+            done();
+        });
+        request.on('error', err => done(err));
+        request.send();
+    });
+});