fix: Only match prefixes in sensitive name regex #2643
Closed
Conversation
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
|
The latest updates on your projects. Learn more about Vercel for GitHub.
|
Contributor
📝 No Changeset FoundThis PR doesn't include a changeset. A changeset (and the release label) is required to release a new version. How to add a changesetRun this command and follow the prompts: pnpm changesetRemember: Never use |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
Contributor
pauldambra
reviewed
Nov 26, 2025
| // it's possible for el.name or el.id to be a DOM element if el is a form with a child input[name="name"] | ||
| const sensitiveNameRegex = | ||
| /^cc|cardnum|ccnum|creditcard|csc|cvc|cvv|exp|pass|pwd|routing|seccode|securitycode|securitynum|socialsec|socsec|ssn/i | ||
| /^(cc|cardnum|ccnum|creditcard|csc|cvc|cvv|exp|pass|pwd|routing|seccode|securitycode|securitynum|socialsec|socsec|ssn)/i |
Member
There was a problem hiding this comment.
i think i understand the change here but
can you ask Claude to write parameterised tests proving the change and behaviour?
Contributor
Author
There was a problem hiding this comment.
made a change - tbh I'm not totally convinced by this set of sensitive patterns anyway - there's lots of things that aren't sensitive that might start with exp.
Also there's part of me that thinks we shouldn't have the check here anyway:
posthog-js/packages/browser/src/autocapture.ts
Lines 41 to 44 in f7d6b69
if a user has added the attributes to an element (even if it's a potentially sensitive element), then surely we should capture the attributes regardless?
Member
There was a problem hiding this comment.
it is way more preferable to miss something safe than to capture something sensitive!
what we should do is let someone override this so that if they disagree then they can set their own and we add a property to say "hey, this is your regex, it's not on us"
(that's how we do it in replay payload capture"
Contributor
Author
There was a problem hiding this comment.
Having got into this a bit more, the way things are at the moment (as I understand it), we detect an element is sensitive with shouldCaptureElement, but that check doesn't apply in all places, so the elements would be captured regardless in elements_chain. I think it maybe needs some wider thought than this one small fix
Contributor
Author
There was a problem hiding this comment.
Line 106 in 19fc73b
I believe this is the check we do for capturing the elements_chain
Contributor
Author
There was a problem hiding this comment.
I'm going to close this PR for now - working on something over on #2647 instead
Contributor
|
Size Change: +24 B (0%) Total Size: 5.08 MB
ℹ️ View Unchanged
|
luke-belton
commented
Nov 26, 2025
| expect(shouldCaptureElement(input)).toBe(false) | ||
| }) | ||
|
|
||
| it.each(['document-expiry', 'success_message', 'account-settings', 'compass-icon'])( |
Contributor
Author
There was a problem hiding this comment.
document-expiry and compass-icon would have failed before because of exp and pass appearing anywhere in the string.
success_message and account-setting would have passed because cc wasn't at the start.
15 tasks
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
It looks like this has been in the SDK forever, but the regex pattern we use was matching any substring for most of our sensitive looking fields (everything except
cc). I think the intent would've been to only look at prefixes, as we were doing forcc.Currently if, for example,
expappeared anywhere inside thename/idattribute of an element then we wouldn't capture it.https://posthoghelp.zendesk.com/agent/tickets/42648
Release info Sub-libraries affected
Libraries affected
Checklist
If releasing new changes
pnpm changesetto generate a changeset file