Skip to content

fix putData : detect collision on specific provided version id #6230

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
SylvainSenechal wants to merge 1 commit into
base: development/9.4
Choose a base branch
from
+72 −10
Open

fix putData : detect collision on specific provided version id #6230

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
31 changes: 23 additions & 8 deletions lib/routes/routeBackbeat.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down Expand Up @@ -438,20 +439,21 @@ 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) {
// ExternalNullVersionId means a pre-versioning null object: collision detection is not applicable.
if (incomingVersionIdEncoded !== undefined && 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: incomingVersionIdDecoded.message,
error: decoded.message,
});
return callback(
errorInstances.BadRequest.customizeDescription('bad request: invalid x-scal-version-id header'),
);
}
if (objMd && objMd.versionId === incomingVersionIdDecoded) {

@SylvainSenechal SylvainSenechal Jul 20, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No need for objMd.versionId === incomingVersionIdDecoded anymore as the middleware is already fetching objMd for the specific versionID provided in the header

// Data already at destination for this version; return 409 with the existing
if (objMd) {
Comment thread
SylvainSenechal marked this conversation as resolved.

@maeldonn maeldonn Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

if (objMd) only means "this version exists" because the router's isPutDataApi block fetched the header version instead of the master; that coupling is easy to break. Clearer to drop the router block and fetch the version here with metadataGetObject (returns undefined when missing):

const incomingVersionId = decode(incomingVersionIdEncoded);
// ... BadRequest on decode error ...
return metadataGetObject(request.bucketName, request.objectKey, incomingVersionId, null, log, (err, versionMd) => {
    if (err) {
        return callback(err);
    }
    if (versionMd) {
        // existing 409 conflict path, using versionMd.microVersionId
    }
    return writeData();
});

// objMd is the specific version (fetched by versionId from x-scal-version-id header)
// its existence means data is already at 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',
Expand Down Expand Up @@ -2069,7 +2071,20 @@ function routeBackbeat(clientIP, request, response, log) {
});
return next(errors.InvalidArgument);
}
const versionId = decodedVidResult;
let versionId = decodedVidResult;
// For putData api: the version to check is passed
// via x-scal-version-id header, not the URL query. Fetch that specific
// version so objMd matches the replicated version, not always the master.
const isPutDataApi = request.method === 'PUT' && request.resourceType === 'data';

@maeldonn maeldonn Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The router shouldn't special-case one handler, and these guards duplicate putData's. Move the version lookup into putData itself (see handler comment) so this block goes away.

if (isPutDataApi) {
const versionIdHeader = request.headers['x-scal-version-id'];
if (versionIdHeader !== undefined && versionIdHeader !== ExternalNullVersionId) {
const decoded = decode(versionIdHeader);
if (!(decoded instanceof Error)) {

@maeldonn maeldonn Jul 23, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Decode failure is ignore, should it be handled ?

versionId = decoded;
}
}
}
if (useMultipleBackend) {
if (request.resourceType === 'multiplebackendmetadata') {
return backbeatRoutes[request.method][request.resourceType](request, response, log, next);
Expand Down
51 changes: 49 additions & 2 deletions tests/functional/backbeat/putData.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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,
Expand Down
Loading