CLDSRV-881: add fast-path to cors header to avoid extra md call

Author: tcarmet

lib/api/api.js

@@ -94,6 +94,23 @@ 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
@@ -108,8 +125,26 @@ function wrapCallbackWithErrorCorsHeaders(request, response, log, callback) {
             || !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: bucket was never loaded (pre-handler failure like
+        // auth.server.doAuth denial, header-size check, copy-source parse
+        // error). Look the bucket up so we can still reply with CORS
+        // headers.
         return metadata.getBucket(request.bucketName, log, (mdErr, bucket) => {
-            if (!mdErr && bucket && !response.headersSent) {
+            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);
+            }
+            if (bucket && !response.headersSent) {
                 const headers = collectCorsHeaders(
                     request.headers.origin, request.method, bucket);
                 Object.keys(headers).forEach(key => {

tests/unit/api/corsErrorHeaders.js

@@ -7,6 +7,7 @@ const { bucketGet } = require('../../../lib/api/bucketGet');
 const bucketGetCors = require('../../../lib/api/bucketGetCors');
 const { bucketPut } = require('../../../lib/api/bucketPut');
 const bucketPutCors = require('../../../lib/api/bucketPutCors');
+const DummyRequest = require('../DummyRequest');
 const {
     CorsConfigTester,
     DummyRequestLogger,
@@ -86,7 +87,10 @@ function setupBucketWithCors(done) {
 }
 
 function buildRequest(spec) {
-    const req = {
+    // DummyRequest is an http.IncomingMessage stream that emits 'end'
+    // synchronously. We need that because callApiMethod's waterfall
+    // waits for the request body on non-objectPut paths.
+    return new DummyRequest({
         bucketName,
         objectKey: spec.objectKey,
         headers: {
@@ -96,12 +100,7 @@ function buildRequest(spec) {
         url: spec.url,
         query: spec.query,
         method: spec.httpMethod,
-        actionImplicitDenies: false,
-        socket: { remoteAddress: '127.0.0.1', destroy: () => {} },
-    };
-    // Pretend the HTTP body is already drained so on('end') does not fire.
-    req.on = () => req;
-    return req;
+    }, Buffer.alloc(0));
 }
 
 function buildResponseSpy(sandbox) {
@@ -218,6 +217,63 @@ describe('CORS headers on 403 auth failures (api.callApiMethod)', () => {
         });
 });
 
+describe('CORS headers on 403 via handler (fast path)', () => {
+    // Verifies the wrapper's fast path: when auth succeeds but the
+    // handler's own ACL/policy check denies, the handler has already
+    // loaded the bucket and passed corsHeaders through its callback.
+    // The wrapper should forward them without calling metadata.getBucket
+    // itself.
+    let sandbox;
+
+    before(done => setupBucketWithCors(done));
+
+    beforeEach(() => {
+        sandbox = sinon.createSandbox();
+        // Stub auth to succeed as a *different* account. The handler then
+        // runs standardMetadataValidateBucket which denies because the
+        // bucket is owned by accessKey1.
+        const otherAuth = makeAuthInfo('accessKey2');
+        const authServer = {
+            doAuth: sandbox.stub().callsArgWith(2, null, otherAuth,
+                [{ isAllowed: true, isImplicit: false }], null, {}),
+        };
+        sandbox.stub(auth, 'server').value(authServer);
+    });
+
+    afterEach(() => sandbox.restore());
+
+    it('forwards handler-provided corsHeaders without setting headers '
+        + 'on the response directly', done => {
+        const request = buildRequest({
+            apiMethod: 'bucketGet', httpMethod: 'GET',
+            url: '/', query: {},
+        });
+        const response = buildResponseSpy(sandbox);
+        const log = buildLog(sandbox);
+
+        api.callApiMethod('bucketGet', request, response, log,
+            (err, xml, corsHeaders) => {
+                assert(err, 'expected an error');
+                assert(err.is && err.is.AccessDenied,
+                    `expected AccessDenied, got ${err.code}`);
+                assert(corsHeaders,
+                    'handler should have supplied corsHeaders');
+                assert.strictEqual(
+                    corsHeaders['access-control-allow-origin'], origin,
+                    'corsHeaders should include access-control-allow-origin');
+                // Fast path: the wrapper did not setHeader on the response.
+                // The route-level transport is what would ultimately call
+                // setCommonResponseHeaders in production.
+                assert.strictEqual(
+                    response.getHeader('access-control-allow-origin'),
+                    undefined,
+                    'wrapper should not set CORS headers directly when the '
+                    + 'handler already provided them');
+                done();
+            });
+    });
+});
+
 describe('CORS headers on 200 successful responses (per-handler)', () => {
     // Sanity-check that the existing per-handler path continues to work.
     // Pass-through tests: a request with matching CORS should receive