fix(identify): reject invalid values passed to identify#3285
Open
pauldambra wants to merge 7 commits intomainfrom
Open
fix(identify): reject invalid values passed to identify#3285pauldambra wants to merge 7 commits intomainfrom
pauldambra wants to merge 7 commits intomainfrom
Conversation
When someone does `posthog.identify(`${undefinedVar}`)`, JS coerces
undefined to the string "undefined", which passes all existing
validation and silently identifies the user as literal "undefined".
Gated behind the new `defaults: '2026-04-01'` config so existing
users are not broken. Also adds test coverage for falsy values
(undefined, null, empty string, false) which were already rejected
but had no explicit tests.
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
Contributor
Important Files Changed
Prompt To Fix All With AIThis is a comment left during a code review.
Path: packages/browser/src/posthog-core.ts
Line: 2353-2358
Comment:
**`"null"` coercion not handled**
The same coercion bug exists for `null`: `` posthog.identify(`${nullVar}`) `` produces the string `"null"`, which passes all current guards and silently identifies the user as `"null"`.
Since the intent of this PR is to catch JS coercion mistakes, it would make sense to reject `"null"` under the same defaults gate:
```suggestion
if (this.config.defaults !== 'unset' && this.config.defaults >= '2026-04-01' && new_distinct_id.toLowerCase() === 'undefined') {
logger.critical(
`The string "undefined" was set in posthog.identify which indicates an error. This ID should be unique to the user and not a hardcoded string.`
)
return
}
if (this.config.defaults !== 'unset' && this.config.defaults >= '2026-04-01' && new_distinct_id.toLowerCase() === 'null') {
logger.critical(
`The string "null" was set in posthog.identify which indicates an error. This ID should be unique to the user and not a hardcoded string.`
)
return
}
```
(Or these two could be collapsed into a single guard checking a set of sentinel strings.)
How can I resolve this? If you propose a fix, please make it concise.Reviews (1): Last reviewed commit: "fix(identify): reject the string "undefi..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
Contributor
|
Size Change: +4.94 kB (+0.08%) Total Size: 6.57 MB
ℹ️ View Unchanged
|
… distinct IDs - Add PostHogConfigError that bypasses safewrap so callers can catch it - Add strict_identify config option (defaults to true with defaults >= '2026-04-01') - String "undefined" rejection is gated on strict_identify (no breaking change) - Upgrade falsy value rejection from debug-only logger.error to always-visible logger.critical - Parameterize tests for both log and throw behaviours Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
![graphite-app[bot]](https://avatars.githubusercontent.com/in/158384?s=60&v=4){kind=small}
…nnotation Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
pauldambra
changed the title
Collapses four separate check-error-return blocks into a single validation method, removing duplicate error message templates. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Extract shared prefix into a single template, with only the reason suffix varying per branch. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Comment on lines
+2299
to
+2301
| return reason | ||
| ? `The string "${id}" was set in posthog.identify which indicates an error. ${reason}` | ||
| : undefined |
There was a problem hiding this comment.
This multi-line ternary expression with template literal needs to be reformatted to match Prettier's style rules. Either put it on a single line or restructure the logic to avoid the formatting conflict.
Spotted by Graphite (based on CI logs)
Is this helpful? React 👍 or 👎 to let us know.
marandaneto
reviewed
Mar 26, 2026
Comment on lines
+1024
to
+1026
| * When true, posthog.identify() throws an error if called with an invalid distinct_id | ||
| * (e.g. undefined, null, empty string, or the string "undefined"). | ||
| * When false, invalid calls log a console error and return without identifying. |
Member
There was a problem hiding this comment.
i think SDKs should never throw, swallow users input and log is the best you can do to be a good citizen
throwing might 'break' people's apps, which is worse than just not getting that user identified.
Member
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
posthog.identify(${undefinedVar}), JS coercesundefinedto the string"undefined", which passes all existing validation and silently identifies the user as literal"undefined"defaults: '2026-04-01'gate so this is opt-in and not a breaking change — without the new defaults,"undefined"is still acceptedundefined,null,"",false) which were already rejected but had no explicit testsI guess a robot posted about this? https://twitter.com/CofounderGPT/status/2037068242829603138
Test plan
undefined,null,"",falseare rejected with error log"undefined"is accepted without2026-04-01defaults (backwards compatible)"undefined"is rejected with2026-04-01defaults🤖 Generated with Claude Code