From ff0e1f052693ccea7dad0d321bbebf05e1926247 Mon Sep 17 00:00:00 2001 From: John Tran Date: Tue, 8 Jun 2021 22:37:10 -0700 Subject: [PATCH 1/6] Add validateDeviceId function --- src/amplitude-client.js | 7 ++++--- src/utils.js | 14 ++++++++++++++ 2 files changed, 18 insertions(+), 3 deletions(-) diff --git a/src/amplitude-client.js b/src/amplitude-client.js index 0f7fb275..397a2176 100644 --- a/src/amplitude-client.js +++ b/src/amplitude-client.js @@ -927,8 +927,9 @@ AmplitudeClient.prototype.regenerateDeviceId = function regenerateDeviceId() { }; /** - * Sets a custom deviceId for current user. Note: this is not recommended unless you know what you are doing - * (like if you have your own system for managing deviceIds). Make sure the deviceId you set is sufficiently unique + * Sets a custom deviceId for current user. **Values may not have `.` inside them** + * Note: this is not recommended unless you know what you are doing (like if you have your own system for managing deviceIds). + * Make sure the deviceId you set is sufficiently unique * (we recommend something like a UUID - see src/uuid.js for an example of how to generate) to prevent conflicts with other devices in our system. * @public * @param {string} deviceId - custom deviceId for current user. @@ -939,7 +940,7 @@ AmplitudeClient.prototype.setDeviceId = function setDeviceId(deviceId) { return this._q.push(['setDeviceId'].concat(Array.prototype.slice.call(arguments, 0))); } - if (!utils.validateInput(deviceId, 'deviceId', 'string')) { + if (!utils.validateDeviceId(deviceId)) { return; } diff --git a/src/utils.js b/src/utils.js index 59aeb0f2..2352c13a 100644 --- a/src/utils.js +++ b/src/utils.js @@ -97,6 +97,19 @@ var validateInput = function validateInput(input, name, expectedType) { return true; }; +const validateDeviceId = function validateDeviceId(deviceId) { + if (!validateInput(deviceId, 'deviceId', 'string')) { + return false; + } + for (let i = 0; i < deviceId.length; i++) { + if (deviceId[i] === '.') { + log.error(`Device IDs may not contain '.' characters. Value will be ignored: "${deviceId}"`); + return false; + } + } + return true; +}; + // do some basic sanitization and type checking, also catch property dicts with more than 1000 key/value pairs var validateProperties = function validateProperties(properties) { var propsType = type(properties); @@ -257,4 +270,5 @@ export default { validateGroups, validateInput, validateProperties, + validateDeviceId, }; From d839b975b44b7c5ed858583992caf78b31e55492 Mon Sep 17 00:00:00 2001 From: John Tran Date: Tue, 8 Jun 2021 23:10:55 -0700 Subject: [PATCH 2/6] Add device id check during init --- src/amplitude-client.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/src/amplitude-client.js b/src/amplitude-client.js index 397a2176..aa67d0f0 100644 --- a/src/amplitude-client.js +++ b/src/amplitude-client.js @@ -148,6 +148,12 @@ AmplitudeClient.prototype.init = function init(apiKey, opt_userId, opt_config, o this._pendingReadStorage = true; const initFromStorage = (storedDeviceId) => { + if (opt_config && opt_config.deviceId && !utils.validateDeviceId(opt_config.deviceId)) { + utils.log.error( + `Invalid device ID rejected. Randomly generated UUID will be used instead of "${opt_config.deviceId}"`, + ); + delete opt_config.deviceId; + } this.options.deviceId = this._getInitialDeviceId(opt_config && opt_config.deviceId, storedDeviceId); this.options.userId = (type(opt_userId) === 'string' && !utils.isEmptyString(opt_userId) && opt_userId) || From a74ed6018e25fb4a230b83c8bb4460e471896ae5 Mon Sep 17 00:00:00 2001 From: John Tran Date: Tue, 8 Jun 2021 23:16:39 -0700 Subject: [PATCH 3/6] add tests --- test/amplitude-client.js | 27 +++++++++++++++++++++++++++ 1 file changed, 27 insertions(+) diff --git a/test/amplitude-client.js b/test/amplitude-client.js index 24dee99d..73dbef6c 100644 --- a/test/amplitude-client.js +++ b/test/amplitude-client.js @@ -285,6 +285,20 @@ describe('AmplitudeClient', function () { amplitude._getUrlParams.restore(); }); + it('should reject invalid device ids that contain periods', function () { + const spyErrorWarning = sinon.spy(utils.log, 'error'); + const badDeviceId = 'bad.device.id'; + amplitude.init(apiKey, null, { deviceId: badDeviceId }); + assert.isTrue( + spyErrorWarning.calledWith( + `Invalid device ID rejected. Randomly generated UUID will be used instead of "${badDeviceId}"`, + ), + ); + assert.notEqual(amplitude.options.deviceId, badDeviceId); + + spyErrorWarning.restore(); + }); + it('should load device id from the cookie', function () { // deviceId and sequenceNumber not set, init should load value from localStorage var cookieData = { @@ -1048,6 +1062,19 @@ describe('AmplitudeClient', function () { var stored = amplitude._metadataStorage.load(); assert.propertyVal(stored, 'deviceId', 'deviceId'); }); + + it('should not take periods in deviceId', function () { + const spyErrorWarning = sinon.spy(utils.log, 'error'); + amplitude.init(apiKey, null, { deviceId: 'fakeDeviceId' }); + const badDeviceId = 'bad.device.id'; + amplitude.setDeviceId(badDeviceId); + var stored = amplitude._metadataStorage.load(); + assert.propertyVal(stored, 'deviceId', 'fakeDeviceId'); + assert.isTrue( + spyErrorWarning.calledWith(`Device IDs may not contain '.' characters. Value will be ignored "${badDeviceId}"`), + ); + spyErrorWarning.restore(); + }); }); describe('resetSessionId', function () { From 7531e98e1f7717646b8577af440d7cc325524909 Mon Sep 17 00:00:00 2001 From: John Tran Date: Wed, 9 Jun 2021 10:34:33 -0700 Subject: [PATCH 4/6] fix test --- test/amplitude-client.js | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) diff --git a/test/amplitude-client.js b/test/amplitude-client.js index 73dbef6c..65c4ef70 100644 --- a/test/amplitude-client.js +++ b/test/amplitude-client.js @@ -1071,7 +1071,9 @@ describe('AmplitudeClient', function () { var stored = amplitude._metadataStorage.load(); assert.propertyVal(stored, 'deviceId', 'fakeDeviceId'); assert.isTrue( - spyErrorWarning.calledWith(`Device IDs may not contain '.' characters. Value will be ignored "${badDeviceId}"`), + spyErrorWarning.calledWith( + `Device IDs may not contain '.' characters. Value will be ignored: "${badDeviceId}"`, + ), ); spyErrorWarning.restore(); }); From 25fb560555f53276922b132f6c515c53edeeaf41 Mon Sep 17 00:00:00 2001 From: John Tran Date: Thu, 10 Jun 2021 15:36:09 -0700 Subject: [PATCH 5/6] deviceId includes nit --- src/utils.js | 8 +++----- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/src/utils.js b/src/utils.js index 2352c13a..eab560f4 100644 --- a/src/utils.js +++ b/src/utils.js @@ -101,11 +101,9 @@ const validateDeviceId = function validateDeviceId(deviceId) { if (!validateInput(deviceId, 'deviceId', 'string')) { return false; } - for (let i = 0; i < deviceId.length; i++) { - if (deviceId[i] === '.') { - log.error(`Device IDs may not contain '.' characters. Value will be ignored: "${deviceId}"`); - return false; - } + if (deviceId.includes('.')) { + log.error(`Device IDs may not contain '.' characters. Value will be ignored: "${deviceId}"`); + return false; } return true; }; From 83d8d59c1eefe7bb0ce8ffc3f56f2ff1dbee177b Mon Sep 17 00:00:00 2001 From: John Tran Date: Thu, 10 Jun 2021 15:38:07 -0700 Subject: [PATCH 6/6] extra test --- test/amplitude-client.js | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/test/amplitude-client.js b/test/amplitude-client.js index 65c4ef70..5c8fee58 100644 --- a/test/amplitude-client.js +++ b/test/amplitude-client.js @@ -289,6 +289,12 @@ describe('AmplitudeClient', function () { const spyErrorWarning = sinon.spy(utils.log, 'error'); const badDeviceId = 'bad.device.id'; amplitude.init(apiKey, null, { deviceId: badDeviceId }); + + assert.isTrue( + spyErrorWarning.calledWith( + `Device IDs may not contain '.' characters. Value will be ignored: "${badDeviceId}"`, + ), + ); assert.isTrue( spyErrorWarning.calledWith( `Invalid device ID rejected. Randomly generated UUID will be used instead of "${badDeviceId}"`,