Repository navigation
Conversation
Contributor
Hello tcarmet,My role is to assist you with the merge of this Available options
Available commands
Status report is not available. |
Comment on lines
+258
to
+259
| const copySourceApiName = apiName === 'objectGetVersion' ? 'objectGet' : apiName; | ||
| if (actionNeedQuotaCheckCopy(copySourceApiName, apiMethod)) { |
Contributor
There was a problem hiding this comment.
This looks kinda convoluted to me, as actionNeedQuotaCheckCopy is a super simple function that may not deserve to be in Arsenal IMHO.
I think one of the following could be done:
- the whole logic could probably be just pasted there and then the new
objectGetVersioncase be inserted naturally in the tested condition - the copy-specific actions may be inserted in the
actionNeedQuotaCheckconstant object in arsenal, and the extra source action condition can be pasted in here, but this may have side-effects asactionNeedQuotaCheckis used elsewhere.
Contributor
Author
There was a problem hiding this comment.
Agreed. I went with your first option and inlined the condition, so objectGetVersion is now handled directly in the check and the Arsenal helper is no longer used here. It's in a separate commit (e3d8467) so it's easier to review, and I'll squash it once approved.
leif-scality
approved these changes
Oct 29, 2026
leif-scality
left a comment
leif-scality
left a comment
Contributor
There was a problem hiding this comment.
LGTM. This fixes CLDSRV-1039 and I found no regressions. #6324 should be merged too, to close the remaining cross-bucket case (CLDSRV-1021).
@davidtencer For the release notes: users or roles allowed s3:GetObject but not s3:GetObjectVersion will now get AccessDenied when they copy a specific version (matches AWS).
Contributor
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the |
❌ 1 Tests Failed:
View the top 1 failed test(s) by shortest run time
To view more test analytics, go to the Test Analytics Dashboard |
jonathan-gramain
approved these changes
Oct 29, 2026
tcarmet
force-pushed
the
bugfix/CLDSRV-1039-versioned-copy-source-authorization
branch
from
October 30, 2026 16:15
e3d8467 to
7ca918f
Compare
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
Comment on lines
+267
to
+351
| (apiName, done) => { | ||
| // Object copy operations first check the target object, | ||
| // meaning the source object, containing the current bytes, | ||
| // is checked second. This logic handles these APIs calls by | ||
| // ensuring the bytes are positives (i.e., not an object | ||
| // replacement). The source is checked with objectGetVersion | ||
| // instead of objectGet when the copy specifies its version. | ||
| const isCopySource = | ||
| (apiName === 'objectGet' || apiName === 'objectGetVersion') && | ||
| (apiMethod === 'objectCopy' || apiMethod === 'objectPutCopyPart'); | ||
| if (isCopySource) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| inflight = Math.abs(inflight); | ||
| } else if (!actionNeedQuotaCheck[apiName] && !actionWithDataDeletion[apiName]) { | ||
| return done(); | ||
| } | ||
| // When inflights are disabled, the sum of the current utilization metrics | ||
| // and the current bytes are compared with the quota. The current bytes | ||
| // are not sent to the utilization service. When inflights are enabled, | ||
| // the sum of the current utilization metrics only are compared with the | ||
| // quota. They include the current inflight bytes sent in the request. | ||
| let _inflights = shouldSendInflights ? inflight : undefined; | ||
| const inflightForCheck = shouldSendInflights ? 0 : inflight; | ||
| return _evaluateQuotas( | ||
| bucketQuota, | ||
| accountQuota, | ||
| bucket, | ||
| account, | ||
| _inflights, | ||
| inflightForCheck, | ||
| apiName, | ||
| log, | ||
| (err, _bucketQuotaExceeded, _accountQuotaExceeded) => { | ||
| if (err) { | ||
| return done(err); | ||
| } | ||
|
|
||
| bucketQuotaExceeded = _bucketQuotaExceeded; | ||
| accountQuotaExceeded = _accountQuotaExceeded; | ||
| bucketQuotaExceeded = _bucketQuotaExceeded; | ||
| accountQuotaExceeded = _accountQuotaExceeded; | ||
|
|
||
| // Inflights are inverted: in case of cleanup, we just re-issue | ||
| // the same API call. | ||
| if (_inflights) { | ||
| _inflights = -_inflights; | ||
| } | ||
| // Inflights are inverted: in case of cleanup, we just re-issue | ||
| // the same API call. | ||
| if (_inflights) { | ||
| _inflights = -_inflights; | ||
| } | ||
|
|
||
| request.finalizerHooks.push((errorFromAPI, _done) => { | ||
| const code = (bucketQuotaExceeded || accountQuotaExceeded) ? 429 : 200; | ||
| const quotaCleanUpStartTime = process.hrtime.bigint(); | ||
| // Quotas are cleaned only in case of error in the API | ||
| async.waterfall([ | ||
| cb => { | ||
| if (errorFromAPI) { | ||
| return _evaluateQuotas(bucketQuota, accountQuota, bucket, account, _inflights, | ||
| null, apiName, log, cb); | ||
| } | ||
| return cb(); | ||
| }, | ||
| ], () => { | ||
| monitorQuotaEvaluationDuration(apiMethod, type, code, quotaEvaluationDuration + | ||
| Number(process.hrtime.bigint() - quotaCleanUpStartTime)); | ||
| return _done(); | ||
| request.finalizerHooks.push((errorFromAPI, _done) => { | ||
| const code = bucketQuotaExceeded || accountQuotaExceeded ? 429 : 200; | ||
| const quotaCleanUpStartTime = process.hrtime.bigint(); | ||
| // Quotas are cleaned only in case of error in the API | ||
| async.waterfall( | ||
| [ | ||
| cb => { | ||
| if (errorFromAPI) { | ||
| return _evaluateQuotas( | ||
| bucketQuota, | ||
| accountQuota, | ||
| bucket, | ||
| account, | ||
| _inflights, | ||
| null, | ||
| apiName, | ||
| log, | ||
| cb, | ||
| ); | ||
| } | ||
| return cb(); | ||
| }, | ||
| ], | ||
| () => { | ||
| monitorQuotaEvaluationDuration( | ||
| apiMethod, | ||
| type, | ||
| code, | ||
| quotaEvaluationDuration + Number(process.hrtime.bigint() - quotaCleanUpStartTime), | ||
| ); | ||
| return _done(); | ||
| }, | ||
| ); | ||
| }); | ||
| }); | ||
|
|
||
| return done(); | ||
| }); | ||
| }, err => { | ||
| quotaEvaluationDuration = Number(process.hrtime.bigint() - requestStartTime); | ||
| if (err) { | ||
| log.warn('Error getting metrics from the quota service, allowing the request', { | ||
| error: err.name, | ||
| description: err.message, | ||
| }); | ||
| } | ||
| if (!actionWithDataDeletion[apiMethod] && | ||
| (bucketQuotaExceeded || accountQuotaExceeded)) { | ||
| return callback(errors.QuotaExceeded); | ||
| } | ||
| return callback(); | ||
| }); | ||
| return done(); | ||
| }, | ||
| ); | ||
| }, |
Comment on lines
+319
to
+334
| cb => { | ||
| if (errorFromAPI) { | ||
| return _evaluateQuotas( | ||
| bucketQuota, | ||
| accountQuota, | ||
| bucket, | ||
| account, | ||
| _inflights, | ||
| null, | ||
| apiName, | ||
| log, | ||
| cb, | ||
| ); | ||
| } | ||
| return cb(); | ||
| }, |
Comment on lines
+124
to
-200
| return next(errors.NoSuchBucket); | ||
| } | ||
| return next(null, destBucketMD); | ||
| }, | ||
| ); | ||
| }, | ||
| function checkSourceAuthorization(destBucketMD, next) { | ||
| return standardMetadataValidateBucketAndObj( | ||
| { | ||
| ...valGetParams, | ||
| serverAccessLogOptions: { copySource: true }, | ||
| }, | ||
| request.actionImplicitDenies, | ||
| log, | ||
| (err, sourceBucketMD, sourceObjMD) => { | ||
| if (err) { | ||
| log.debug('error validating get part of request', { error: err }); | ||
| // eslint-disable-next-line no-param-reassign | ||
| && (request.sourceServerAccessLog.error = errors.PreconditionFailed); | ||
| return next(errors.PreconditionFailed, destBucketMD); | ||
| } | ||
| const copyLocator = setUpCopyLocator(sourceObjMD, | ||
| request.headers['x-amz-copy-source-range'], log); | ||
| if (copyLocator.error) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| request.sourceServerAccessLog && (request.sourceServerAccessLog.error = copyLocator.error); | ||
| return next(copyLocator.error, destBucketMD); | ||
| } | ||
| let sourceVerId; |
Comment on lines
-201
to
+254
| // If specific version requested, include copy source | ||
| // version id in response. Include in request by default | ||
| // if versioning is enabled or suspended. | ||
| if (sourceBucketMD.getVersioningConfiguration() || | ||
| reqVersionId) { | ||
| if (sourceObjMD.isNull || !sourceObjMD.versionId) { | ||
| sourceVerId = 'null'; | ||
| } else { | ||
| sourceVerId = | ||
| versionIdUtils.encode(sourceObjMD.versionId); | ||
| request.sourceServerAccessLog && (request.sourceServerAccessLog.error = err); | ||
| return next(err, destBucketMD); | ||
| } | ||
| } | ||
| return next(null, copyLocator.dataLocator, destBucketMD, | ||
| copyLocator.copyObjectSize, sourceVerId, | ||
| sourceLocationConstraintName, sourceObjMD); | ||
| }); | ||
| }, | ||
| function _validateQuotas(dataLocator, destBucketMD, | ||
| copyObjectSize, sourceVerId, | ||
| sourceLocationConstraintName, sourceObjMD, next) { | ||
| return validateQuotas(request, destBucketMD, request.accountQuotas, valPutParams.requestType, | ||
| request.apiMethod, sourceObjMD?.['content-length'] || 0, false, log, err => | ||
| next(err, dataLocator, destBucketMD, copyObjectSize, sourceVerId, sourceLocationConstraintName)); | ||
| }, | ||
| // get MPU shadow bucket to get splitter based on MD version | ||
| function getMpuShadowBucket(dataLocator, destBucketMD, | ||
| copyObjectSize, sourceVerId, | ||
| sourceLocationConstraintName, next) { | ||
| return metadata.getBucket(mpuBucketName, log, | ||
| (err, mpuBucket) => { | ||
| if (!sourceObjMD) { | ||
| log.debug('no source object', { sourceObject }); | ||
| const err = reqVersionId ? errors.NoSuchVersion : errors.NoSuchKey; | ||
| // eslint-disable-next-line no-param-reassign | ||
| request.sourceServerAccessLog && (request.sourceServerAccessLog.error = err); | ||
| return next(err, destBucketMD); | ||
| } | ||
| let sourceLocationConstraintName = sourceObjMD.dataStoreName; | ||
| // for backwards compatibility before storing dataStoreName | ||
| // TODO: handle in objectMD class | ||
| if ( | ||
| !sourceLocationConstraintName && | ||
| sourceObjMD.location[0] && | ||
| sourceObjMD.location[0].dataStoreName | ||
| ) { | ||
| sourceLocationConstraintName = sourceObjMD.location[0].dataStoreName; | ||
| } | ||
| // check if object data is in a cold storage | ||
| const coldErr = verifyColdObjectAvailable(sourceObjMD); | ||
| if (coldErr) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| request.sourceServerAccessLog && (request.sourceServerAccessLog.error = coldErr); | ||
| return next(coldErr, null); | ||
| } | ||
| if (sourceObjMD.isDeleteMarker) { | ||
| log.debug('delete marker on source object', { sourceObject }); | ||
| let err; | ||
| if (reqVersionId) { | ||
| err = errorInstances.InvalidRequest.customizeDescription( | ||
| 'The source of a copy ' + | ||
| 'request may not specifically refer to a delete' + | ||
| 'marker by version id.', | ||
| ); | ||
| } else { | ||
| // if user specifies a key in a versioned source bucket | ||
| // without specifying a version, and the object has a | ||
| // delete marker, return NoSuchKey | ||
| err = errors.NoSuchKey; | ||
| } | ||
| // eslint-disable-next-line no-param-reassign | ||
| request.sourceServerAccessLog && (request.sourceServerAccessLog.error = err); | ||
| return next(err, destBucketMD); | ||
| } | ||
| const headerValResult = validateHeaders( | ||
| request.headers, | ||
| sourceObjMD['last-modified'], | ||
| sourceObjMD['content-md5'], | ||
| ); | ||
| if (headerValResult.error) { | ||
| request.sourceServerAccessLog && | ||
| // eslint-disable-next-line no-param-reassign | ||
| (request.sourceServerAccessLog.error = errors.PreconditionFailed); | ||
| return next(errors.PreconditionFailed, destBucketMD); | ||
| } | ||
| const copyLocator = setUpCopyLocator( | ||
| sourceObjMD, | ||
| request.headers['x-amz-copy-source-range'], | ||
| log, | ||
| ); | ||
| if (copyLocator.error) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| request.sourceServerAccessLog && (request.sourceServerAccessLog.error = copyLocator.error); | ||
| return next(copyLocator.error, destBucketMD); | ||
| } | ||
| let sourceVerId; | ||
| // If specific version requested, include copy source | ||
| // version id in response. Include in request by default | ||
| // if versioning is enabled or suspended. | ||
| if (sourceBucketMD.getVersioningConfiguration() || reqVersionId) { | ||
| if (sourceObjMD.isNull || !sourceObjMD.versionId) { | ||
| sourceVerId = 'null'; | ||
| } else { | ||
| sourceVerId = versionIdUtils.encode(sourceObjMD.versionId); | ||
| } | ||
| } | ||
| return next( | ||
| null, | ||
| copyLocator.dataLocator, | ||
| destBucketMD, | ||
| copyLocator.copyObjectSize, | ||
| sourceVerId, | ||
| sourceLocationConstraintName, | ||
| sourceObjMD, | ||
| ); | ||
| }, | ||
| ); | ||
| }, | ||
| function _validateQuotas( | ||
| dataLocator, | ||
| destBucketMD, | ||
| copyObjectSize, | ||
| sourceVerId, | ||
| sourceLocationConstraintName, | ||
| sourceObjMD, | ||
| next, | ||
| ) { | ||
| return validateQuotas( | ||
| request, | ||
| destBucketMD, | ||
| request.accountQuotas, | ||
| valPutParams.requestType, | ||
| request.apiMethod, | ||
| sourceObjMD?.['content-length'] || 0, | ||
| false, | ||
| log, | ||
| err => | ||
| next(err, dataLocator, destBucketMD, copyObjectSize, sourceVerId, sourceLocationConstraintName), | ||
| ); | ||
| }, | ||
| // get MPU shadow bucket to get splitter based on MD version |
Comment on lines
255
to
+281
| @@ -243,105 +276,140 @@ | |||
| if (mpuBucket.getMdBucketModelVersion() < 2) { | |||
| splitter = constants.oldSplitter; | |||
| } | |||
| return next(null, dataLocator, destBucketMD, | |||
| copyObjectSize, sourceVerId, splitter, | |||
| sourceLocationConstraintName); | |||
| return next( | |||
| null, | |||
| dataLocator, | |||
Comment on lines
371
to
-358
| @@ -352,158 +420,242 @@ | |||
| // Pull locations to clean up any potential orphans | |||
| // in data if object put is an overwrite of | |||
| // already existing object with same key and part number | |||
| oldLocations = Array.isArray(oldLocations) ? | |||
| oldLocations : [oldLocations]; | |||
| oldLocations = Array.isArray(oldLocations) ? oldLocations : [oldLocations]; | |||
| } | |||
| return next(null, destBucketMD, locations, totalHash, | |||
Comment on lines
424
to
466
| } | ||
| return next(null, destBucketMD, locations, totalHash, | ||
| prevObjectSize, copyObjectSize, sourceVerId, | ||
| serverSideEncryption, lastModified, oldLocations, splitter); | ||
| return next( | ||
| null, | ||
| destBucketMD, | ||
| locations, | ||
| totalHash, | ||
| prevObjectSize, | ||
| copyObjectSize, | ||
| sourceVerId, | ||
| serverSideEncryption, | ||
| lastModified, | ||
| oldLocations, | ||
| splitter, | ||
| ); | ||
| }); | ||
| }, | ||
| function storeNewPartMetadata(destBucketMD, locations, totalHash, | ||
| prevObjectSize, copyObjectSize, sourceVerId, serverSideEncryption, | ||
| lastModified, oldLocations, splitter, next) { | ||
| const metaStoreParams = { | ||
| partNumber: paddedPartNumber, | ||
| contentMD5: totalHash, | ||
| size: copyObjectSize, | ||
| uploadId, | ||
| splitter: constants.splitter, | ||
| }, | ||
| function storeNewPartMetadata( | ||
| destBucketMD, | ||
| locations, | ||
| totalHash, | ||
| prevObjectSize, | ||
| copyObjectSize, | ||
| sourceVerId, | ||
| serverSideEncryption, | ||
| lastModified, | ||
| overheadField: constants.overheadField, | ||
| ownerId: destBucketMD.getOwner(), | ||
| }; | ||
| return services.metadataStorePart(mpuBucketName, | ||
| locations, metaStoreParams, log, err => { | ||
| oldLocations, | ||
| splitter, | ||
| next, | ||
| ) { | ||
| const metaStoreParams = { | ||
| partNumber: paddedPartNumber, | ||
| contentMD5: totalHash, | ||
| size: copyObjectSize, | ||
| uploadId, | ||
| splitter: constants.splitter, | ||
| lastModified, | ||
| overheadField: constants.overheadField, | ||
| ownerId: destBucketMD.getOwner(), | ||
| }; | ||
| return services.metadataStorePart(mpuBucketName, locations, metaStoreParams, log, err => { | ||
| if (err) { | ||
| log.debug('error storing new metadata', | ||
| { error: err, method: 'storeNewPartMetadata' }); | ||
| log.debug('error storing new metadata', { error: err, method: 'storeNewPartMetadata' }); | ||
| return next(err); |
Comment on lines
382
to
-409
| } | ||
| return next(null, locations, oldLocations, destBucketMD, totalHash, | ||
| lastModified, sourceVerId, serverSideEncryption, | ||
| prevObjectSize, copyObjectSize, splitter); | ||
| return next( | ||
| null, | ||
| locations, | ||
| oldLocations, | ||
| destBucketMD, | ||
| totalHash, | ||
| lastModified, | ||
| sourceVerId, | ||
| serverSideEncryption, | ||
| prevObjectSize, | ||
| copyObjectSize, | ||
| splitter, | ||
| ); | ||
| }); | ||
| }, | ||
| function checkCanDeleteOldLocations(partLocations, oldLocations, destBucketMD, | ||
| totalHash, lastModified, sourceVerId, serverSideEncryption, | ||
| prevObjectSize, copyObjectSize, splitter, next) { | ||
| if (!oldLocations) { | ||
| return next(null, oldLocations, destBucketMD, totalHash, | ||
| lastModified, sourceVerId, serverSideEncryption, | ||
| prevObjectSize, copyObjectSize); | ||
| } | ||
| return services.isCompleteMPUInProgress({ | ||
| bucketName: destBucketName, | ||
| objectKey: destObjectKey, | ||
| uploadId, | ||
| }, | ||
| function checkCanDeleteOldLocations( | ||
| partLocations, | ||
| oldLocations, | ||
| destBucketMD, | ||
| totalHash, | ||
| lastModified, | ||
| sourceVerId, | ||
| serverSideEncryption, | ||
| prevObjectSize, | ||
| copyObjectSize, | ||
| splitter, | ||
| }, log, (err, completeInProgress) => { | ||
| if (err) { | ||
| return next(err, destBucketMD); | ||
| next, | ||
| ) { | ||
| if (!oldLocations) { | ||
| return next( | ||
| null, | ||
| oldLocations, | ||
| destBucketMD, | ||
| totalHash, | ||
| lastModified, | ||
| sourceVerId, | ||
| serverSideEncryption, | ||
| prevObjectSize, | ||
| copyObjectSize, | ||
| ); | ||
| } | ||
| let oldLocationsToDelete = oldLocations; | ||
| // Prevent deletion of old data if a completeMPU | ||
| // is already in progress because then there is no | ||
| // guarantee that the old location will not be the | ||
| // committed one. |
Comment on lines
-410
to
+579
| if (completeInProgress) { | ||
| log.warn('not deleting old locations because CompleteMPU is in progress', { | ||
| method: 'objectPutCopyPart::checkCanDeleteOldLocations', | ||
| return services.isCompleteMPUInProgress( | ||
| { | ||
| bucketName: destBucketName, | ||
| objectKey: destObjectKey, | ||
| uploadId, | ||
| partLocations, | ||
| oldLocations, | ||
| }); | ||
| oldLocationsToDelete = null; | ||
| } | ||
| return next(null, oldLocationsToDelete, destBucketMD, totalHash, | ||
| lastModified, sourceVerId, serverSideEncryption, | ||
| prevObjectSize, copyObjectSize); | ||
| }); | ||
| }, | ||
| function cleanupExistingData(oldLocationsToDelete, destBucketMD, totalHash, | ||
| lastModified, sourceVerId, serverSideEncryption, | ||
| prevObjectSize, copyObjectSize, next) { | ||
| // Clean up the old data now that new metadata (with new | ||
| // data locations) has been stored | ||
| if (oldLocationsToDelete) { | ||
| return data.batchDelete(oldLocationsToDelete, request.method, null, | ||
| log, err => { | ||
| splitter, | ||
| }, | ||
| log, | ||
| (err, completeInProgress) => { | ||
| if (err) { | ||
| return next(err, destBucketMD); | ||
| } | ||
| let oldLocationsToDelete = oldLocations; | ||
| // Prevent deletion of old data if a completeMPU | ||
| // is already in progress because then there is no | ||
| // guarantee that the old location will not be the | ||
| // committed one. | ||
| if (completeInProgress) { | ||
| log.warn('not deleting old locations because CompleteMPU is in progress', { | ||
| method: 'objectPutCopyPart::checkCanDeleteOldLocations', | ||
| bucketName: destBucketName, | ||
| objectKey: destObjectKey, | ||
| uploadId, | ||
| partLocations, | ||
| oldLocations, | ||
| }); | ||
| oldLocationsToDelete = null; | ||
| } | ||
| return next( | ||
| null, | ||
| oldLocationsToDelete, | ||
| destBucketMD, | ||
| totalHash, | ||
| lastModified, | ||
| sourceVerId, | ||
| serverSideEncryption, | ||
| prevObjectSize, | ||
| copyObjectSize, | ||
| ); | ||
| }, | ||
| ); | ||
| }, | ||
| function cleanupExistingData( | ||
| oldLocationsToDelete, | ||
| destBucketMD, | ||
| totalHash, | ||
| lastModified, | ||
| sourceVerId, | ||
| serverSideEncryption, | ||
| prevObjectSize, | ||
| copyObjectSize, | ||
| next, | ||
| ) { | ||
| // Clean up the old data now that new metadata (with new | ||
| // data locations) has been stored | ||
| if (oldLocationsToDelete) { | ||
| return data.batchDelete(oldLocationsToDelete, request.method, null, log, err => { | ||
| if (err) { | ||
| // if error, log the error and move on as it is not | ||
| // relevant to the client as the client's | ||
| // object already succeeded putting data, metadata | ||
| log.error('error deleting existing data', | ||
| { error: err }); | ||
| log.error('error deleting existing data', { error: err }); | ||
| } | ||
| return next(null, destBucketMD, totalHash, | ||
| lastModified, sourceVerId, serverSideEncryption, | ||
| prevObjectSize, copyObjectSize); | ||
| return next( | ||
| null, | ||
| destBucketMD, | ||
| totalHash, | ||
| lastModified, | ||
| sourceVerId, | ||
| serverSideEncryption, | ||
| prevObjectSize, |
Comment on lines
+580
to
+614
| copyObjectSize, | ||
| ); | ||
| }); | ||
| } | ||
| return next(null, destBucketMD, totalHash, | ||
| lastModified, sourceVerId, serverSideEncryption, | ||
| prevObjectSize, copyObjectSize); | ||
| }, | ||
| ], (err, destBucketMD, totalHash, lastModified, sourceVerId, | ||
| serverSideEncryption, prevObjectSize, copyObjectSize) => { | ||
| const corsHeaders = collectCorsHeaders(request.headers.origin, | ||
| request.method, destBucketMD); | ||
| } | ||
| return next( | ||
| null, | ||
| destBucketMD, | ||
| totalHash, | ||
| lastModified, | ||
| sourceVerId, | ||
| serverSideEncryption, | ||
| prevObjectSize, | ||
| copyObjectSize, | ||
| ); | ||
| }, | ||
| ], | ||
| ( | ||
| err, | ||
| destBucketMD, | ||
| totalHash, | ||
| lastModified, | ||
| sourceVerId, | ||
| serverSideEncryption, | ||
| prevObjectSize, | ||
| copyObjectSize, | ||
| ) => { | ||
| const corsHeaders = collectCorsHeaders(request.headers.origin, request.method, destBucketMD); | ||
|
|
||
| // Store full object size for server access logs | ||
| if (request.serverAccessLog) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| request.serverAccessLog.objectSize = copyObjectSize; | ||
| } | ||
| // Store full object size for server access logs | ||
| if (request.serverAccessLog) { | ||
| // eslint-disable-next-line no-param-reassign | ||
| request.serverAccessLog.objectSize = copyObjectSize; | ||
| } | ||
|
|
||
| // Initialize the queue for internal log request logging | ||
| initializeInternalLogRequestQueue(request); | ||
| // Queue the source-side access log (REST.COPY.PART_GET) | ||
| queueInternalLogRequest(request, { | ||
| operation: 'REST.COPY.PART_GET', | ||
| sourceBucket, | ||
| sourceObject, | ||
| objectSize: copyObjectSize || null, | ||
| }); | ||
| // Initialize the queue for internal log request logging |
…Object Authorize the source of a versioned copy with objectGetVersion, the action checked by the authorization service, so its implicit deny is no longer read as an allow and bucket policy statements on s3:GetObjectVersion apply. Keep evaluating quotas for such a copy.
tcarmet
force-pushed
the
bugfix/CLDSRV-1039-versioned-copy-source-authorization
branch
from
October 30, 2026 22:41
7ca918f to
649530a
Compare
Contributor
Author
|
/approve |
Contributor
Incorrect fix versionThe
Considering where you are trying to merge, I ignored possible hotfix versions and I expected to find:
Please check the The following options are set: approve |
Contributor
ConflictA conflict has been raised during the creation of I have not created the integration branch. Here are the steps to resolve this conflict: git fetch
git checkout -B w/9.4/bugfix/CLDSRV-1039-versioned-copy-source-authorization origin/development/9.4
git merge origin/bugfix/CLDSRV-1039-versioned-copy-source-authorization
# <intense conflict resolution>
git commit
git push -u origin w/9.4/bugfix/CLDSRV-1039-versioned-copy-source-authorizationThe following options are set: approve |
Contributor
Integration data createdI have created the integration data for the additional destination branches.
The following branches will NOT be impacted:
You can set option The following options are set: approve |
Contributor
|
I have successfully merged the changeset of this pull request
The following branches have NOT changed:
This pull request did not target the following hotfix branch(es) so they
Please check the status of the associated issue CLDSRV-1039. Goodbye tcarmet. The following options are set: approve |
bert-e
deleted the
bugfix/CLDSRV-1039-versioned-copy-source-authorization
branch
October 30, 2026 23:30
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
When a copy named a specific version of its source (
?versionId=, includingnull), the source read was checked as a GetObject, while the identity check had been made for GetObjectVersion. So the requester's identity policy was never applied, and bucket policy statements ons3:GetObjectVersionwere ignored. A copy of a specific version is now authorized like a GetObject of that version, and its quota is still evaluated. As a result, a bucket policy Deny meant to protect every version must covers3:GetObjectVersion(ors3:GetObject*), not onlys3:GetObject.