diff --git a/packages/identify/src/identify.ts b/packages/identify/src/identify.ts index 64db0bf..32b5a93 100644 --- a/packages/identify/src/identify.ts +++ b/packages/identify/src/identify.ts @@ -5,7 +5,7 @@ import { ValidPropertyType, SpecialEventType, } from '@amplitude/types'; -import { logger, generateBase36Id } from '@amplitude/utils'; +import { logger, generateBase36Id, isValidProperties } from '@amplitude/utils'; import { UNSET_VALUE, USER_IDENTIFY_OPERATIONS, GROUP_IDENTIFY_OPERATIONS } from './constants'; @@ -154,11 +154,6 @@ export class Identify { return false; } - if (typeof property !== 'string') { - identifyWarn(operation, 'expected string for property but got: ', typeof property, '. Skipping operation'); - return false; - } - if (this._propertySet.has(property)) { identifyWarn(operation, 'property ', property, ' already used. Skipping operation'); return false; @@ -167,19 +162,8 @@ export class Identify { if (operation === IdentifyOperation.ADD) { return typeof value === 'number'; } else if (operation !== IdentifyOperation.UNSET && operation !== IdentifyOperation.REMOVE) { - if (Array.isArray(value)) { - for (const valueElement of value) { - if (!(typeof valueElement === 'number' || typeof valueElement === 'string')) { - identifyWarn(operation, 'invalid array element type ', typeof valueElement, '. Skipping operation'); - return false; - } - } - } else if (!(typeof value === 'number' || typeof value === 'string')) { - identifyWarn(operation, 'invalid value type ', typeof value, '. Skipping operation'); - return false; - } + return isValidProperties(property, value); } - return true; } } diff --git a/packages/types/src/identify.ts b/packages/types/src/identify.ts index a95f051..4c0b0a9 100644 --- a/packages/types/src/identify.ts +++ b/packages/types/src/identify.ts @@ -20,7 +20,7 @@ export enum IdentifyOperation { CLEAR_ALL = '$clearAll', } -export type ValidPropertyType = number | string | Array; +export type ValidPropertyType = number | string | Array | { [key: string]: ValidPropertyType }; interface BaseOperationConfig { [key: string]: ValidPropertyType; diff --git a/packages/utils/src/index.ts b/packages/utils/src/index.ts index 4e1758f..f2c1bae 100644 --- a/packages/utils/src/index.ts +++ b/packages/utils/src/index.ts @@ -5,3 +5,4 @@ export { AsyncQueue } from './queue'; export { collectInvalidEventIndices, mapHttpMessageToResponse, mapJSONToResponse } from './response'; export { mapHttpCodeToStatus } from './status'; export { isValidEvent } from './validate'; +export { isValidProperties } from './validateProperties'; diff --git a/packages/utils/src/validateProperties.ts b/packages/utils/src/validateProperties.ts new file mode 100644 index 0000000..2d89584 --- /dev/null +++ b/packages/utils/src/validateProperties.ts @@ -0,0 +1,42 @@ +import { logger } from './logger'; +const MAX_PROPERTY_KEYS = 1000; + +const _isValidObject = (properties: { [key: string]: any }): boolean => { + if (Object.keys(properties).length > MAX_PROPERTY_KEYS) { + logger.warn('too many properties. Skipping operation'); + return false; + } + for (const key in properties) { + if (typeof key !== 'string') { + logger.warn('invalid properties format. Skipping operation'); + return false; + } + let value = properties[key]; + if (!isValidProperties(key, value)) return false; + } + return true; +}; + +const isValidProperties = (property: string, value: any): boolean => { + if (typeof property != 'string') return false; + if (Array.isArray(value)) { + for (const valueElement of value) { + if (Array.isArray(valueElement)) { + logger.warn('invalid array element type ', typeof valueElement); + return false; + } else if (typeof valueElement === 'object') { + return _isValidObject(value); + } else if (!(typeof valueElement === 'number' || typeof valueElement === 'string')) { + logger.warn('invalid array element type ', typeof valueElement); + return false; + } + } + } else if (typeof value === 'object') { + return _isValidObject(value); + } else if (!(typeof value === 'number' || typeof value === 'string')) { + logger.warn('invalid value type ', typeof value); + return false; + } + return true; +}; +export { isValidProperties }; diff --git a/packages/utils/test/validateProperties.test.ts b/packages/utils/test/validateProperties.test.ts new file mode 100644 index 0000000..d1dde66 --- /dev/null +++ b/packages/utils/test/validateProperties.test.ts @@ -0,0 +1,43 @@ +import { isValidProperties } from '../src/validateProperties'; +describe('isValidateProperties', () => { + it('should pass on valid properties', () => { + const validProperties = { + keyForString: 'stringValue', + keyForNumber: 123, + keyForArray: ['test', 456, { arrayObjKey1: 'arrayObjValue1' }], + keyForObj: { + objKey1: 'objValue1', + objKey2: 'objValue2', + }, + }; + expect(isValidProperties('property', validProperties)).toBe(true); + }); + + it('should fail on invalid properties with function as value', () => { + const testFunc = () => { + return 'test'; + }; + const inValidProperties = { + keyForFunct: testFunc, + }; + expect(isValidProperties('property', inValidProperties)).toBe(false); + }); + + it('should fail on invalid properties with array nested in array', () => { + const inValidProperties = ['item1', 123, ['subItem1', 'subItem2']]; + expect(isValidProperties('property', inValidProperties)).toBe(false); + }); + + it('should fail when any key is not string', () => { + const validProperties = { + keyForString: 'stringValue', + keyForNumber: 123, + keyForArray: ['test', 456], + keyForObj: { + objKey1: 'objValue1', + objKey2: 'objValue2', + }, + }; + expect(isValidProperties(1 as any, validProperties)).toBe(false); + }); +});