Skip to content
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.

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

fix(NODE-4960): UUID validation too strict #572

Merged
merged 11 commits into from
Apr 24, 2023
74 changes: 40 additions & 34 deletions src/binary.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,3 @@
import { bufferToUuidHexString, uuidHexStringToBuffer, uuidValidateString } from './uuid_utils';
import { isUint8Array } from './parser/utils';
import type { EJSONOptions } from './extended_json';
import { BSONError } from './error';
Expand Down Expand Up @@ -288,7 +287,7 @@ export class Binary extends BSONValue {
}
} else if ('$uuid' in doc) {
type = 4;
data = uuidHexStringToBuffer(doc.$uuid);
data = UUID.uuidBytesFromString(doc.$uuid);
}
if (!data) {
throw new BSONError(`Unexpected Binary Extended JSON format ${JSON.stringify(doc)}`);
Expand All @@ -311,42 +310,39 @@ export class Binary extends BSONValue {
export type UUIDExtended = {
$uuid: string;
};

const UUID_BYTE_LENGTH = 16;
const UUID_WITHOUT_DASHES = /^[0-F]{32}$/i;
const UUID_WITH_DASHES = /^[0-F]{8}-[0-F]{4}-[0-F]{4}-[0-F]{4}-[0-F]{12}$/i;
addaleax marked this conversation as resolved.
Show resolved Hide resolved

/**
* A class representation of the BSON UUID type.
* @public
*/
export class UUID extends Binary {
static cacheHexString: boolean;
Copy link
Contributor

Choose a reason for hiding this comment

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

Can this safely be removed? It's a public property

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good catch, added it back and filed follow ups, we can deprecate it in the subtask I made and remove in v6


/** UUID hexString cache @internal */
private __id?: string;

/**
* Create an UUID type
* Create a UUID type
*
* When the argument to the constructor is omitted a random v4 UUID will be generated.
*
* @param input - Can be a 32 or 36 character hex string (dashes excluded/included) or a 16 byte binary Buffer.
*/
constructor(input?: string | Uint8Array | UUID) {
let bytes: Uint8Array;
let hexStr;
if (input == null) {
bytes = UUID.generate();
} else if (input instanceof UUID) {
bytes = ByteUtils.toLocalBufferType(new Uint8Array(input.buffer));
hexStr = input.__id;
} else if (ArrayBuffer.isView(input) && input.byteLength === UUID_BYTE_LENGTH) {
addaleax marked this conversation as resolved.
Show resolved Hide resolved
bytes = ByteUtils.toLocalBufferType(input);
} else if (typeof input === 'string') {
bytes = uuidHexStringToBuffer(input);
bytes = UUID.uuidBytesFromString(input);
} else {
throw new BSONError(
'Argument passed in UUID constructor must be a UUID, a 16 byte Buffer or a 32/36 character hex string (dashes excluded/included, format: xxxxxxxx-xxxx-xxxx-xxxx-xxxxxxxxxxxx).'
);
}
super(bytes, BSON_BINARY_SUBTYPE_UUID_NEW);
this.__id = hexStr;
}

/**
Expand All @@ -359,28 +355,23 @@ export class UUID extends Binary {

set id(value: Uint8Array) {
this.buffer = value;

if (UUID.cacheHexString) {
this.__id = bufferToUuidHexString(value);
}
}

/**
* Returns the UUID id as a 32 or 36 character hex string representation, excluding/including dashes (defaults to 36 character dash separated)
* @param includeDashes - should the string exclude dash-separators.
* */
toHexString(includeDashes = true): string {
if (UUID.cacheHexString && this.__id) {
return this.__id;
if (includeDashes) {
return [
ByteUtils.toHex(this.buffer.subarray(0, 4)),
ByteUtils.toHex(this.buffer.subarray(4, 6)),
ByteUtils.toHex(this.buffer.subarray(6, 8)),
ByteUtils.toHex(this.buffer.subarray(8, 10)),
ByteUtils.toHex(this.buffer.subarray(10, 16))
].join('-');
}

const uuidHexString = bufferToUuidHexString(this.id, includeDashes);

if (UUID.cacheHexString) {
this.__id = uuidHexString;
}

return uuidHexString;
return ByteUtils.toHex(this.buffer);
}

/**
Expand Down Expand Up @@ -456,16 +447,11 @@ export class UUID extends Binary {
}

if (typeof input === 'string') {
return uuidValidateString(input);
return UUID.isValidUUIDString(input);
}

if (isUint8Array(input)) {
// check for length & uuid version (https://tools.ietf.org/html/rfc4122#section-4.1.3)
if (input.byteLength !== UUID_BYTE_LENGTH) {
return false;
}

return (input[6] & 0xf0) === 0x40 && (input[8] & 0x80) === 0x80;
return input.byteLength === UUID_BYTE_LENGTH;
}

return false;
Expand All @@ -476,7 +462,7 @@ export class UUID extends Binary {
* @param hexString - 32 or 36 character hex string (dashes excluded/included).
*/
static override createFromHexString(hexString: string): UUID {
const buffer = uuidHexStringToBuffer(hexString);
const buffer = UUID.uuidBytesFromString(hexString);
return new UUID(buffer);
}

Expand All @@ -485,6 +471,26 @@ export class UUID extends Binary {
return new UUID(ByteUtils.fromBase64(base64));
}

/** @internal */
static uuidBytesFromString(representation: string) {
Copy link
Contributor

Choose a reason for hiding this comment

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

The naming is redundant here since it's a static UUID method

Suggested change
static uuidBytesFromString(representation: string) {
static bytesFromString(representation: string) {

Copy link
Contributor Author

Choose a reason for hiding this comment

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

SGTM

if (!UUID.isValidUUIDString(representation)) {
throw new BSONError(
'UUID string representation must be 32 hex digits or canonical hyphenated representation'
);
}
return ByteUtils.fromHex(representation.replace(/-/g, ''));
}

/**
* @internal
*
* Validates a string to be a hex digit sequence with or without dashes.
* The canonical hyphenated representation of a uuid is hex in 8-4-4-4-12 groups.
*/
static isValidUUIDString(representation: string) {
return UUID_WITHOUT_DASHES.test(representation) || UUID_WITH_DASHES.test(representation);
}

/**
* Converts to a string representation of this Id.
*
Expand Down
33 changes: 0 additions & 33 deletions src/uuid_utils.ts

This file was deleted.

8 changes: 8 additions & 0 deletions test/node/tools/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -128,6 +128,14 @@ const bufferFromHexArray = array => {

exports.bufferFromHexArray = bufferFromHexArray;

function int32ToHex(int32) {
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
const buf = Buffer.alloc(4);
buf.writeInt32LE(int32, 0);
return buf.toString('hex');
}

exports.int32ToHex = int32ToHex;

/**
* A helper to calculate the byte size of a string (including null)
*
Expand Down
81 changes: 29 additions & 52 deletions test/node/uuid.test.ts
Original file line number Diff line number Diff line change
@@ -1,10 +1,11 @@
import { Binary, UUID } from '../register-bson';
import { Binary, EJSON, UUID } from '../register-bson';
import { inspect } from 'util';
import { validate as uuidStringValidate, version as uuidStringVersion } from 'uuid';
import { BSON, BSONError } from '../register-bson';
const BSON_DATA_BINARY = BSON.BSONType.binData;
import { BSON_BINARY_SUBTYPE_UUID_NEW } from '../../src/constants';
import { expect } from 'chai';
import { bufferFromHexArray, int32ToHex } from './tools/utils';

// Test values
const UPPERCASE_DASH_SEPARATED_UUID_STRING = 'AAAAAAAA-AAAA-4AAA-AAAA-AAAAAAAAAAAA';
Expand All @@ -13,19 +14,13 @@ const LOWERCASE_DASH_SEPARATED_UUID_STRING = 'aaaaaaaa-aaaa-4aaa-aaaa-aaaaaaaaaa
const LOWERCASE_VALUES_ONLY_UUID_STRING = 'aaaaaaaaaaaa4aaaaaaaaaaaaaaaaaaa';

describe('UUID', () => {
/**
* @ignore
*/
it('should correctly generate a valid UUID v4 from empty constructor', () => {
const uuid = new UUID();
const uuidHexStr = uuid.toHexString();
expect(uuidStringValidate(uuidHexStr)).to.be.true;
expect(uuidStringVersion(uuidHexStr)).to.equal(Binary.SUBTYPE_UUID);
});

/**
* @ignore
*/
it('should correctly create UUIDs from UPPERCASE & lowercase 36 char dash-separated hex string', () => {
const uuid1 = new UUID(UPPERCASE_DASH_SEPARATED_UUID_STRING);
expect(uuid1.equals(UPPERCASE_DASH_SEPARATED_UUID_STRING)).to.be.true;
Expand All @@ -36,9 +31,6 @@ describe('UUID', () => {
expect(uuid2.toString()).to.equal(LOWERCASE_DASH_SEPARATED_UUID_STRING);
});

/**
* @ignore
*/
it('should correctly create UUIDs from UPPERCASE & lowercase 32 char hex string (no dash separators)', () => {
const uuid1 = new UUID(UPPERCASE_VALUES_ONLY_UUID_STRING);
expect(uuid1.equals(UPPERCASE_VALUES_ONLY_UUID_STRING)).to.be.true;
Expand All @@ -49,9 +41,6 @@ describe('UUID', () => {
expect(uuid2.toHexString(false)).to.equal(LOWERCASE_VALUES_ONLY_UUID_STRING);
});

/**
* @ignore
*/
it('should correctly create UUID from Buffer', () => {
const uuid1 = new UUID(Buffer.from(UPPERCASE_VALUES_ONLY_UUID_STRING, 'hex'));
expect(uuid1.equals(UPPERCASE_DASH_SEPARATED_UUID_STRING)).to.be.true;
Expand All @@ -62,51 +51,23 @@ describe('UUID', () => {
expect(uuid2.toString()).to.equal(LOWERCASE_DASH_SEPARATED_UUID_STRING);
});

/**
* @ignore
*/
it('should correctly create UUID from UUID (copying existing buffer)', () => {
const org = new UUID();
const copy = new UUID(org);
expect(org.id).to.not.equal(copy.id);
expect(org.id).to.deep.equal(copy.id);
});

/**
* @ignore
*/
it('should throw if passed invalid 36-char uuid hex string', () => {
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
expect(() => new UUID(LOWERCASE_DASH_SEPARATED_UUID_STRING)).to.not.throw();
expect(() => new UUID('aaaaaaaa-aaaa-aaaa-aaaa-aaaaaaaaaaaa')).to.throw(BSONError);
// Note: The version is missing here ^
});

/**
* @ignore
*/
it('should throw if passed unsupported argument', () => {
expect(() => new UUID(LOWERCASE_DASH_SEPARATED_UUID_STRING)).to.not.throw();
expect(() => new UUID({})).to.throw(BSONError);
});

/**
* @ignore
*/
it('should correctly check if a buffer isValid', () => {
const validBuffer = Buffer.from(UPPERCASE_VALUES_ONLY_UUID_STRING, 'hex');
const invalidBuffer1 = Buffer.from('aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa', 'hex');
const invalidBuffer2 = Buffer.alloc(16);

expect(validBuffer.length).to.equal(invalidBuffer1.length);
expect(validBuffer.length).to.equal(invalidBuffer2.length);
expect(UUID.isValid(invalidBuffer1)).to.be.false;
expect(UUID.isValid(invalidBuffer2)).to.be.false;
baileympearson marked this conversation as resolved.
Show resolved Hide resolved
expect(UUID.isValid(validBuffer)).to.be.true;
});

/**
* @ignore
*/
it('should correctly convert to and from a Binary instance', () => {
const uuid = new UUID(LOWERCASE_DASH_SEPARATED_UUID_STRING);
expect(UUID.isValid(uuid)).to.be.true;
Expand All @@ -118,9 +79,6 @@ describe('UUID', () => {
expect(uuid2.toHexString()).to.equal(LOWERCASE_DASH_SEPARATED_UUID_STRING);
});

/**
* @ignore
*/
it('should correctly convert to and from a Binary instance', () => {
const uuid = new UUID(LOWERCASE_DASH_SEPARATED_UUID_STRING);
expect(UUID.isValid(uuid)).to.be.true;
Expand All @@ -132,9 +90,6 @@ describe('UUID', () => {
expect(uuid.equals(uuid2)).to.be.true;
});

/**
* @ignore
*/
it('should throw when converted from an incompatible Binary instance', () => {
const validRandomBuffer = Buffer.from('Hello World!');
const binRand = new Binary(validRandomBuffer);
Expand All @@ -154,9 +109,6 @@ describe('UUID', () => {
expect(() => binV4.toUUID()).to.not.throw();
});

/**
* @ignore
*/
it('should correctly allow for node.js inspect to work with UUID', () => {
const uuid = new UUID(UPPERCASE_DASH_SEPARATED_UUID_STRING);
expect(inspect(uuid)).to.equal(`new UUID("${LOWERCASE_DASH_SEPARATED_UUID_STRING}")`);
Expand Down Expand Up @@ -199,6 +151,31 @@ describe('UUID', () => {
};
expect(deserializedUUID).to.deep.equal(expectedResult);
});

context('when UUID bytes are not in v4 format', () => {
it('returns UUID instance', () => {
const nullUUID = '00'.repeat(16);
const serializedUUID = bufferFromHexArray([
'05', // binData type
'6100', // 'a' & null
int32ToHex(nullUUID.length / 2), // binary starts with int32 length
'04', // uuid subtype
nullUUID // uuid bytes
]);
const deserializedUUID = BSON.deserialize(serializedUUID);
const expectedResult = { a: new UUID(nullUUID) };
expect(deserializedUUID).to.deep.equal(expectedResult);
});
});
});

context('fromExtendedJSON()', () => {
it('returns UUID instance', () => {
const nullUUID = '00'.repeat(16);
const deserializedUUID = EJSON.parse(`{ "a": { "$uuid": "${'00'.repeat(16)}" } }`);
const expectedResult = { a: new UUID(nullUUID) };
expect(deserializedUUID).to.deep.equal(expectedResult);
});
});

context('createFromHexString()', () => {
Expand All @@ -217,8 +194,8 @@ describe('UUID', () => {
});

context('when called with an incorrect length string', () => {
it('throws an error indicating the expected length of 32 or 36 characters', () => {
expect(() => UUID.createFromHexString('')).to.throw(/32 or 36 character/);
it('throws an error indicating the expected length', () => {
expect(() => UUID.createFromHexString('')).to.throw(/must be 32 hex digits/);
});
});
});
Expand Down