From 002cfa63b03f41849537443fb9df421eb97d9686 Mon Sep 17 00:00:00 2001 From: DarkIsDude Date: Mon, 5 Oct 2026 17:17:26 +0200 Subject: [PATCH 1/2] =?UTF-8?q?=F0=9F=90=9B=20Apply=20each=20key's=20own?= =?UTF-8?q?=20authorization=20verdict=20in=20multiObjectDelete?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue: CLDSRV-1017 --- lib/api/multiObjectDelete.js | 13 ++-- tests/unit/api/multiObjectDelete.js | 100 ++++++++++++++++++++++++++++ 2 files changed, 105 insertions(+), 8 deletions(-) diff --git a/lib/api/multiObjectDelete.js b/lib/api/multiObjectDelete.js index 276552c743..f567c62664 100644 --- a/lib/api/multiObjectDelete.js +++ b/lib/api/multiObjectDelete.js @@ -695,13 +695,6 @@ function multiObjectDelete(authInfo, request, log, callback) { }); return next(errors.InternalError); } - // Convert authorization results into an easier to handle format - const actionImplicitDenies = authorizationResults.reduce((acc, curr, idx) => { - const apiMethod = authorizationResults[idx].action; - // eslint-disable-next-line no-param-reassign - acc[apiMethod] = curr.isImplicit; - return acc; - }, {}); for (let i = 0; i < authorizationResults.length; i++) { const result = authorizationResults[i]; // result is { isAllowed: true, @@ -726,7 +719,11 @@ function multiObjectDelete(authInfo, request, log, callback) { continue; } - // Evaluate against the bucket policies + // Evaluate against the bucket policies with this + // key's own IAM verdict: all results share the same + // action, so a request-wide map keyed by action + // would collapse to the last key's verdict + const actionImplicitDenies = { [result.action]: result.isImplicit }; const areAllActionsAllowed = evaluateBucketPolicyWithIAM( bucketMD, Object.keys(actionImplicitDenies), diff --git a/tests/unit/api/multiObjectDelete.js b/tests/unit/api/multiObjectDelete.js index fe7a0a145d..41e08bece0 100644 --- a/tests/unit/api/multiObjectDelete.js +++ b/tests/unit/api/multiObjectDelete.js @@ -560,3 +560,103 @@ describe('multiObjectDelete checkPolicies request context', () => { }); }); }); + +describe('multiObjectDelete per-key authorization results', () => { + // IAM user within the bucket owner account, so that the request + // goes through the checkPolicies path + const userAuthInfo = makeAuthInfo(canonicalID, 'testuser'); + + function makeDeleteRequest(keys) { + const post = `${keys.map(key => `${key}`).join('')}`; + return new DummyRequest({ + bucketName, + namespace, + parsedHost: 'localhost', + headers: { + 'content-md5': crypto.createHash('md5').update(post, 'utf8').digest('base64'), + }, + post, + url: `/${bucketName}`, + socket: { + remoteAddress: '127.0.0.1', + }, + }); + } + + function putObject(key, cb) { + const putRequest = new DummyRequest( + { + bucketName, + namespace, + objectKey: key, + headers: {}, + url: `/${bucketName}/${key}`, + }, + postBody, + ); + return objectPut(authInfo, putRequest, undefined, log, cb); + } + + beforeEach(done => { + cleanup(); + sinon.stub(auth.server, 'extractParams').returns({ + params: { + version: 4, + data: { + signatureVersion: 'AWS4-HMAC-SHA256', + authType: 'REST-HEADER', + signatureAge: 0, + }, + }, + }); + // Mimic a Vault user policy that only allows deleting allow/*: + // explicit allow on allow/*, implicit deny on everything else + sinon.stub(vault, 'checkPolicies').callsFake((requestContextParams, arn, log, cb) => { + const results = requestContextParams.parameterize.specificResource.map(entry => ({ + isAllowed: entry.key.startsWith('allow/'), + isImplicit: !entry.key.startsWith('allow/'), + arn: `arn:aws:s3:::${bucketName}/${entry.key}`, + action: 'objectDelete', + versionId: entry.versionId, + })); + return cb(null, results); + }); + bucketPut(authInfo, testBucketPutRequest, log, done); + }); + + afterEach(() => { + sinon.restore(); + }); + + it('should deny a denied key even when the last key of the request is allowed', done => { + putObject('deny/d', () => + putObject('allow/d', () => { + const request = makeDeleteRequest(['deny/d', 'allow/d']); + multiObjectDelete.multiObjectDelete(userAuthInfo, request, log, (err, xml) => { + assert.strictEqual(err, null); + assert.strictEqual(xml.includes('deny/dAccessDenied'), true); + assert.strictEqual(xml.includes('allow/d'), true); + assert.strictEqual(metadata.keyMaps.get(bucketName).has('deny/d'), true); + assert.strictEqual(metadata.keyMaps.get(bucketName).has('allow/d'), false); + done(); + }); + }), + ); + }); + + it('should allow an allowed key even when the last key of the request is denied', done => { + putObject('allow/e', () => + putObject('deny/e', () => { + const request = makeDeleteRequest(['allow/e', 'deny/e']); + multiObjectDelete.multiObjectDelete(userAuthInfo, request, log, (err, xml) => { + assert.strictEqual(err, null); + assert.strictEqual(xml.includes('allow/e'), true); + assert.strictEqual(xml.includes('deny/eAccessDenied'), true); + assert.strictEqual(metadata.keyMaps.get(bucketName).has('allow/e'), false); + assert.strictEqual(metadata.keyMaps.get(bucketName).has('deny/e'), true); + done(); + }); + }), + ); + }); +}); From 2b713f633b39c2f89602fab15a16aab8c10f5991 Mon Sep 17 00:00:00 2001 From: DarkIsDude Date: Tue, 6 Oct 2026 16:49:33 +0200 Subject: [PATCH 2/2] =?UTF-8?q?=E2=9C=85=20Use=20assert.match=20in=20multi?= =?UTF-8?q?ObjectDelete=20per-key=20tests?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue: CLDSRV-1017 --- tests/unit/api/multiObjectDelete.js | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/tests/unit/api/multiObjectDelete.js b/tests/unit/api/multiObjectDelete.js index 41e08bece0..abb32bf603 100644 --- a/tests/unit/api/multiObjectDelete.js +++ b/tests/unit/api/multiObjectDelete.js @@ -634,8 +634,8 @@ describe('multiObjectDelete per-key authorization results', () => { const request = makeDeleteRequest(['deny/d', 'allow/d']); multiObjectDelete.multiObjectDelete(userAuthInfo, request, log, (err, xml) => { assert.strictEqual(err, null); - assert.strictEqual(xml.includes('deny/dAccessDenied'), true); - assert.strictEqual(xml.includes('allow/d'), true); + assert.match(xml, /deny\/d<\/Key>AccessDenied<\/Code>/); + assert.match(xml, /allow\/d<\/Key><\/Deleted>/); assert.strictEqual(metadata.keyMaps.get(bucketName).has('deny/d'), true); assert.strictEqual(metadata.keyMaps.get(bucketName).has('allow/d'), false); done(); @@ -650,8 +650,8 @@ describe('multiObjectDelete per-key authorization results', () => { const request = makeDeleteRequest(['allow/e', 'deny/e']); multiObjectDelete.multiObjectDelete(userAuthInfo, request, log, (err, xml) => { assert.strictEqual(err, null); - assert.strictEqual(xml.includes('allow/e'), true); - assert.strictEqual(xml.includes('deny/eAccessDenied'), true); + assert.match(xml, /allow\/e<\/Key><\/Deleted>/); + assert.match(xml, /deny\/e<\/Key>AccessDenied<\/Code>/); assert.strictEqual(metadata.keyMaps.get(bucketName).has('allow/e'), false); assert.strictEqual(metadata.keyMaps.get(bucketName).has('deny/e'), true); done();