fix: add another illegal id

plugin-server/src/worker/ingestion/person-state.ts

@@ -17,9 +17,23 @@ import { castTimestampOrNow, UUIDT } from '../../utils/utils'
 import { captureIngestionWarning } from './utils'
 
 const MAX_FAILED_PERSON_MERGE_ATTEMPTS = 3
+
+export const mergeFinalFailuresCounter = new Counter({
+    name: 'person_merge_final_failure_total',
+    help: 'Number of person merge final failures.',
+})
+
 // used to prevent identify from being used with generic IDs
 // that we can safely assume stem from a bug or mistake
-const CASE_INSENSITIVE_ILLEGAL_IDS = new Set([
+const BASE_ILLEGAL_IDS = [
+    '',
+    '[object Object]'.toLowerCase(),
+    'NaN'.toLowerCase(),
+    'None',
+    'none',
+    'null',
+    '0',
+    'undefined',
     'anonymous',
     'guest',
     'distinctid',
@@ -30,17 +44,17 @@ const CASE_INSENSITIVE_ILLEGAL_IDS = new Set([
     'undefined',
     'true',
     'false',
-])
-
-export const mergeFinalFailuresCounter = new Counter({
-    name: 'person_merge_final_failure_total',
-    help: 'Number of person merge final failures.',
-})
+]
+// we have seen illegal ids received but wrapped in double quotes
+// to protect ourselves from this we'll add the single- and double-quoted versions of the illegal ids
+const SINGLE_QUOTED_ILLEGAL_IDS = BASE_ILLEGAL_IDS.map((id) => `'${id}'`)
+const DOUBLE_QUOTED_ILLEGAL_IDS = BASE_ILLEGAL_IDS.map((id) => `"${id}"`)
 
-const CASE_SENSITIVE_ILLEGAL_IDS = new Set(['[object Object]', 'NaN', 'None', 'none', 'null', '0', 'undefined'])
+export const ILLEGAL_IDS = new Set(BASE_ILLEGAL_IDS.concat(SINGLE_QUOTED_ILLEGAL_IDS).concat(DOUBLE_QUOTED_ILLEGAL_IDS))
 
 const isDistinctIdIllegal = (id: string): boolean => {
-    return id.trim() === '' || CASE_INSENSITIVE_ILLEGAL_IDS.has(id.toLowerCase()) || CASE_SENSITIVE_ILLEGAL_IDS.has(id)
+    const trimmed = id.trim()
+    return trimmed === '' || ILLEGAL_IDS.has(trimmed.toLowerCase())
 }
 
 // This class is responsible for creating/updating a single person through the process-event pipeline
@@ -245,7 +259,7 @@ export class PersonState {
                     this.teamId,
                     this.timestamp
                 )
-            } else if (this.event.event === '$identify' && this.eventProperties['$anon_distinct_id']) {
+            } else if (this.event.event === '$identify' && '$anon_distinct_id' in this.eventProperties) {
                 return await this.merge(
                     String(this.eventProperties['$anon_distinct_id']),
                     this.distinctId,

plugin-server/tests/worker/ingestion/person-state.test.ts

@@ -7,7 +7,7 @@ import { createHub } from '../../../src/utils/db/hub'
 import { PostgresUse } from '../../../src/utils/db/postgres'
 import { defaultRetryConfig } from '../../../src/utils/retries'
 import { UUIDT } from '../../../src/utils/utils'
-import { ageInMonthsLowCardinality, PersonState } from '../../../src/worker/ingestion/person-state'
+import { ageInMonthsLowCardinality, ILLEGAL_IDS, PersonState } from '../../../src/worker/ingestion/person-state'
 import { delayUntilEventIngested } from '../../helpers/clickhouse'
 import { createOrganization, createTeam, fetchPostgresPersons, insertRow } from '../../helpers/sql'
 
@@ -25,17 +25,20 @@ describe('PersonState.update()', () => {
     let uuid2: UUIDT
     let teamId: number
     let poEEmbraceJoin: boolean
+    let organizationId: string
 
     beforeAll(async () => {
         ;[hub, closeHub] = await createHub({})
         await hub.db.clickhouseQuery('SYSTEM STOP MERGES')
+
+        organizationId = await createOrganization(hub.db.postgres)
     })
 
     beforeEach(async () => {
         poEEmbraceJoin = false
         uuid = new UUIDT()
         uuid2 = new UUIDT()
-        const organizationId = await createOrganization(hub.db.postgres)
+
         teamId = await createTeam(hub.db.postgres, organizationId)
 
         jest.spyOn(hub.db, 'fetchPerson')
@@ -1078,10 +1081,11 @@ describe('PersonState.update()', () => {
             hub.statsd = { increment: jest.fn() } as any
         })
 
-        it('stops $identify if current distinct_id is illegal', async () => {
+        const illegalIds = Array.from(ILLEGAL_IDS.values())
+        it.each(illegalIds)('stops $identify if current distinct_id is illegal: `%s`', async (illegalId: string) => {
             const person = await personState({
                 event: '$identify',
-                distinct_id: '[object Object]',
+                distinct_id: illegalId,
                 properties: {
                     $anon_distinct_id: 'anonymous_id',
                 },
@@ -1092,16 +1096,34 @@ describe('PersonState.update()', () => {
             expect(persons.length).toEqual(0)
 
             expect(hub.statsd!.increment).toHaveBeenCalledWith('illegal_distinct_ids.total', {
-                distinctId: '[object Object]',
+                distinctId: illegalId,
+            })
+        })
+
+        it('stops $identify if $anon_distinct_id is illegal: ``', async () => {
+            const person = await personState({
+                event: '$identify',
+                distinct_id: 'some_distinct_id',
+                properties: {
+                    $anon_distinct_id: '',
+                },
+            }).handleIdentifyOrAlias()
+
+            expect(person).toEqual(undefined)
+            const persons = await fetchPostgresPersonsH()
+            expect(persons.length).toEqual(0)
+
+            expect(hub.statsd!.increment).toHaveBeenCalledWith('illegal_distinct_ids.total', {
+                distinctId: '',
             })
         })
 
-        it('stops $identify if $anon_distinct_id is illegal', async () => {
+        it.each(illegalIds)('stops $identify if $anon_distinct_id is illegal: `%s`', async (illegalId: string) => {
             const person = await personState({
                 event: '$identify',
                 distinct_id: 'some_distinct_id',
                 properties: {
-                    $anon_distinct_id: 'undefined',
+                    $anon_distinct_id: illegalId,
                 },
             }).handleIdentifyOrAlias()
 
@@ -1110,7 +1132,7 @@ describe('PersonState.update()', () => {
             expect(persons.length).toEqual(0)
 
             expect(hub.statsd!.increment).toHaveBeenCalledWith('illegal_distinct_ids.total', {
-                distinctId: 'undefined',
+                distinctId: illegalId,
             })
         })