fix putData : detect collision on specific provided version id

Issue: CLDSRV-953

Author: SylvainSenechal

lib/routes/routeBackbeat.js

@@ -12,6 +12,7 @@ const backbeatProxy = httpProxy.createProxyServer({
 });
 const { auth, errors, errorInstances, s3middleware, s3routes, models, storage, versioning } = require('arsenal');
 const { decode, encode } = versioning.VersionID;
+const { ExternalNullVersionId } = versioning.VersioningConstants;
 const {
     VersionIdCollisionException,
     StaleMicroVersionIdException,
@@ -434,35 +435,38 @@ function putData(request, response, bucketInfo, objMd, log, callback) {
 
     const incomingVersionIdEncoded = request.headers['x-scal-version-id'];
     if (incomingVersionIdEncoded !== undefined) {
-        const incomingVersionIdDecoded =
-            incomingVersionIdEncoded !== 'null' ? decode(incomingVersionIdEncoded) : 'null';
-        if (incomingVersionIdDecoded instanceof Error) {
-            log.error('crr putData: failed to decode x-scal-version-id header', {
-                method: 'putData',
-                error: incomingVersionIdDecoded.message,
-            });
-            return callback(
-                errorInstances.BadRequest.customizeDescription('bad request: invalid x-scal-version-id header'),
-            );
-        }
-        if (objMd && objMd.versionId === incomingVersionIdDecoded) {
-            // Data already at destination for this version; return 409 with the existing
-            // microVersionId so backbeat can decide if putMetadata is still needed.
-            log.debug('crr putData: version already at destination', {
-                method: 'putData',
-                bucketName: request.bucketName,
-                objectKey: request.objectKey,
-                hasMicroVersionId: !!objMd.microVersionId,
-            });
-            request.resume();
-            return _respondWithHeaderCrrConflict(
-                response,
-                log,
-                callback,
-                VersionIdCollisionException.name,
-                'version id already at destination',
-                objMd.microVersionId,
-            );
+        // ExternalNullVersionId means a null version, which does not need decoding
+        if (incomingVersionIdEncoded !== ExternalNullVersionId) {
+            const decoded = decode(incomingVersionIdEncoded);
+            if (decoded instanceof Error) {
+                log.error('crr putData: failed to decode x-scal-version-id header', {
+                    method: 'putData',
+                    error: decoded.message,
+                });
+                return callback(
+                    errorInstances.BadRequest.customizeDescription('bad request: invalid x-scal-version-id header'),
+                );
+            }
+            if (objMd) {
+                // objMd is the specific version requested by the header x-scal-version-id.
+                // Its existence means the data is already at the destination. Return 409 with the
+                // existing microVersionId so backbeat can decide if putMetadata is still needed.
+                log.debug('crr putData: version already at destination', {
+                    method: 'putData',
+                    bucketName: request.bucketName,
+                    objectKey: request.objectKey,
+                    hasMicroVersionId: !!objMd.microVersionId,
+                });
+                request.resume();
+                return _respondWithHeaderCrrConflict(
+                    response,
+                    log,
+                    callback,
+                    VersionIdCollisionException.name,
+                    'version id already at destination',
+                    objMd.microVersionId,
+                );
+            }
         }
     }
 
@@ -2039,15 +2043,31 @@ function routeBackbeat(clientIP, request, response, log) {
                 if (!isObjectRequest) {
                     return routeNonObjectRequest(request, response, userInfo, log, next);
                 }
-                const decodedVidResult = decodeVersionId(request.query);
-                if (decodedVidResult instanceof Error) {
-                    log.trace('invalid versionId query', {
-                        versionId: request.query.versionId,
-                        error: decodedVidResult,
-                    });
-                    return next(errors.InvalidArgument);
+                const versionIdHeader = request.headers['x-scal-version-id'];
+                let versionId;
+                if (versionIdHeader !== undefined) {
+                    if (versionIdHeader !== ExternalNullVersionId) {
+                        const decoded = decode(versionIdHeader);
+                        if (decoded instanceof Error) {
+                            return next(
+                                errorInstances.BadRequest.customizeDescription(
+                                    'bad request: invalid x-scal-version-id header',
+                                ),
+                            );
+                        }
+                        versionId = decoded;
+                    }
+                } else {
+                    const decodedVidResult = decodeVersionId(request.query);
+                    if (decodedVidResult instanceof Error) {
+                        log.trace('invalid versionId query', {
+                            versionId: request.query.versionId,
+                            error: decodedVidResult,
+                        });
+                        return next(errors.InvalidArgument);
+                    }
+                    versionId = decodedVidResult;
                 }
-                const versionId = decodedVidResult;
                 if (useMultipleBackend) {
                     if (request.resourceType === 'multiplebackendmetadata') {
                         return backbeatRoutes[request.method][request.resourceType](request, response, log, next);

tests/functional/backbeat/putData.js

@@ -106,17 +106,64 @@ describe('putData : VersionId collision detection', () => {
         const output = await putData(key, { versionId: differentVersionId });
         assert.ok(output.Location, 'should return a Location when data is written normally');
     });
+
+    it('should throw VersionIdCollisionException when a non-current version already exists', async () => {
+        const key = 'putdata-non-current-collision';
+
+        const v1Result = await s3.send(
+            new PutObjectCommand({
+                Bucket: TEST_BUCKET,
+                Key: key,
+                Body: Buffer.from(OBJECT_BODY),
+                ContentType: 'text/plain',
+            }),
+        );
+        const v1VersionId = v1Result.VersionId;
+        assert.ok(v1VersionId, 'first PutObject should return a VersionId');
+
+        const v2Result = await s3.send(
+            new PutObjectCommand({
+                Bucket: TEST_BUCKET,
+                Key: key,
+                Body: Buffer.from(OBJECT_BODY),
+                ContentType: 'text/plain',
+            }),
+        );
+        assert.ok(v2Result.VersionId, 'second PutObject should return a VersionId');
+
+        // putData on v1 (non-current) must detect the collision, not just compare against master
+        try {
+            await putData(key, { versionId: v1VersionId });
+            assert.fail('expected VersionIdCollisionException');
+        } catch (err) {
+            assert.ok(
+                err instanceof VersionIdCollisionException,
+                `expected VersionIdCollisionException, got ${err.constructor.name}`,
+            );
+            assert.strictEqual(err.microVersionId, '', 'microVersionId should be empty for original write state');
+        }
+    });
 });
 
 describe('putData : null-version objects (ExternalNullVersionId)', () => {
     // Null-version objects created before versioning was enabled use Arsenal constant ExternalNullVersionId = 'null'
     // getEncodedVersionId() returns 'null' as-is (no base62 encoding), and objMd.versionId is
     // undefined in metadata : collision detection is not possible, so putData must write normally.
-    it('should write normally when VersionId is "null" (ExternalNullVersionId)', async () => {
+    it('should write normally when VersionId is "null" even when a master already exists', async () => {
+        const key = 'putdata-null-version';
+        await s3.send(
+            new PutObjectCommand({
+                Bucket: TEST_BUCKET,
+                Key: key,
+                Body: Buffer.from(OBJECT_BODY),
+                ContentType: 'text/plain',
+            }),
+        );
+
         const output = await backbeatClient.send(
             new PutDataCommand({
                 Bucket: TEST_BUCKET,
-                Key: 'putdata-null-version',
+                Key: key,
                 ContentMD5: OBJECT_MD5_HEX,
                 CanonicalID: CANONICAL_ID,
                 VersioningRequired: true,