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: Prevent periods in device id check #405

Merged
merged 6 commits into from
Jun 10, 2021
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
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
13 changes: 10 additions & 3 deletions src/amplitude-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -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) ||
Expand Down Expand Up @@ -927,8 +933,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.
Expand All @@ -939,7 +946,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;
}

Expand Down
12 changes: 12 additions & 0 deletions src/utils.js
Original file line number Diff line number Diff line change
Expand Up @@ -97,6 +97,17 @@ var validateInput = function validateInput(input, name, expectedType) {
return true;
};

const validateDeviceId = function validateDeviceId(deviceId) {
if (!validateInput(deviceId, 'deviceId', 'string')) {
return false;
}
if (deviceId.includes('.')) {
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);
Expand Down Expand Up @@ -257,4 +268,5 @@ export default {
validateGroups,
validateInput,
validateProperties,
validateDeviceId,
};
35 changes: 35 additions & 0 deletions test/amplitude-client.js
Original file line number Diff line number Diff line change
Expand Up @@ -285,6 +285,26 @@ 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(
`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}"`,
),
);
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 = {
Expand Down Expand Up @@ -1048,6 +1068,21 @@ 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 () {
Expand Down