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

chore: make password capture more explicit #1345

Merged
merged 4 commits into from
Aug 9, 2024

Conversation

daibhin
Copy link
Contributor

@daibhin daibhin commented Aug 7, 2024

Changes

Since #1334 we mask passwords by default.

I want to take this a step further and always include password input masking unless it is explicitly opted out using the dangerouslyCapturePasswordInputs. While I can't see any reasonably good reason that you'd want to unmask a password input, we should still allow it if someone is certain that's what they want to do. It will be slightly more difficult / confusing to configure but that feels like a worthy trade-off to avoid customers accidentally unmasking password inputs by leaving the password: true field out of their maskInputOptions config

Copy link

vercel bot commented Aug 7, 2024

The latest updates on your projects. Learn more about Vercel for Git ↗︎

Name Status Preview Updated (UTC)
posthog-js ✅ Ready (Inspect) Visit Preview Aug 7, 2024 3:50pm

@daibhin daibhin requested review from a team and benjackwhite August 7, 2024 12:30
Copy link

github-actions bot commented Aug 7, 2024

Size Change: +244 B (+0.02%)

Total Size: 1.16 MB

Filename Size Change
dist/array.full.js 333 kB +61 B (+0.02%)
dist/array.js 154 kB +61 B (+0.04%)
dist/main.js 154 kB +61 B (+0.04%)
dist/module.js 154 kB +61 B (+0.04%)
ℹ️ View Unchanged
Filename Size
dist/exception-autocapture.js 10.4 kB
dist/recorder-v2.js 110 kB
dist/recorder.js 110 kB
dist/surveys-preview.js 59.8 kB
dist/surveys.js 66 kB
dist/tracing-headers.js 8.26 kB
dist/web-vitals.js 5.79 kB

compressed-size-action

@marandaneto
Copy link
Member

On Mobile, for certain types of views such as for sure containing PII data (password/email, etc), it's not possible to disable the masking, see https://posthog.com/docs/session-replay/ios#masking-in-swiftui
Even if people allow it, it's still "us" leaking the data hence no opt-out, it's too risky and I understand that I may have to relax that in the future, so far nobody asked for it, only on how to mask it, which is enabled by default.

@marandaneto
Copy link
Member

If people can do maskInputOptions: {password: false}, already, dangerouslyCapturePasswordInputs feels like just making the API more complicated/degrading UX, I'd rather document this better if needed, not a blocker, but happy to hear another opinion.

@daibhin daibhin requested a review from benjackwhite August 8, 2024 13:26
@benjackwhite benjackwhite added the bump patch Bump patch version when this PR gets merged label Aug 9, 2024
@daibhin daibhin merged commit 44478bc into main Aug 9, 2024
12 checks passed
@daibhin daibhin deleted the dn-chore/make-password-capture-more-explicit branch August 9, 2024 08:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bump patch Bump patch version when this PR gets merged
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants