-
-
Notifications
You must be signed in to change notification settings - Fork 366
fix(session-replay): Add detection for potential PII leaks disabling session replay #6389
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
fix(session-replay): Add detection for potential PII leaks disabling session replay #6389
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for driving this. We still need a changelog entry, and my comments are mostly about the naming of the option.
Sources/Swift/Integrations/SessionReplay/SentryReplayOptions.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
e0b8508
to
d19908f
Compare
Codecov Report❌ Patch coverage is
Additional details and impacted files@@ Coverage Diff @@
## v8.x #6389 +/- ##
=============================================
+ Coverage 86.701% 86.803% +0.102%
=============================================
Files 437 441 +4
Lines 37162 37382 +220
Branches 17396 17483 +87
=============================================
+ Hits 32220 32449 +229
+ Misses 4897 4888 -9
Partials 45 45
... and 10 files with indirect coverage changes Continue to review full report in Codecov by Sentry.
|
Performance metrics 🚀
|
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a7a0a2b | 1218.61 ms | 1248.69 ms | 30.08 ms |
f76f6bf | 1207.70 ms | 1233.27 ms | 25.57 ms |
c6afcc7 | 1224.31 ms | 1256.22 ms | 31.92 ms |
3af1ae9 | 1225.60 ms | 1252.65 ms | 27.05 ms |
5e3fb04 | 1239.84 ms | 1267.39 ms | 27.55 ms |
b66be9b | 1218.22 ms | 1244.19 ms | 25.96 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
a7a0a2b | 23.75 KiB | 996.04 KiB | 972.29 KiB |
f76f6bf | 23.74 KiB | 981.30 KiB | 957.56 KiB |
c6afcc7 | 23.75 KiB | 996.03 KiB | 972.28 KiB |
3af1ae9 | 23.74 KiB | 981.29 KiB | 957.55 KiB |
5e3fb04 | 23.74 KiB | 981.30 KiB | 957.56 KiB |
b66be9b | 23.75 KiB | 996.03 KiB | 972.28 KiB |
Previous results on branch: philprime/hotfix/disable-session-replay-ios26
Startup times
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7f548c7 | 1203.63 ms | 1240.19 ms | 36.56 ms |
99fac9e | 1218.98 ms | 1252.16 ms | 33.19 ms |
c77274a | 1233.69 ms | 1264.40 ms | 30.70 ms |
App size
Revision | Plain | With Sentry | Diff |
---|---|---|---|
7f548c7 | 23.75 KiB | 998.22 KiB | 974.47 KiB |
99fac9e | 23.74 KiB | 990.99 KiB | 967.25 KiB |
c77274a | 23.75 KiB | 998.22 KiB | 974.48 KiB |
From a sync between @philipphofmann and myself on Oct 10th 2025:
|
I think we should still include SR in the option name, maybe something like |
why not |
because it's not mentioned in your comment - we'll still show a log message right? |
Fine for me, as it is going to be an experimental option we can reduce the usage scope to session replay. In case we want to repurpose it at some point we should keep the name broader.
I would propose we go for a minor because it has quite some impact on the SDK user.
Yes |
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com> (cherry picked from commit 3e57e15)
6c22528
to
416889b
Compare
🚨 Detected changes in high risk code 🚨High-risk code can easily blow up and is hard to test. We had severe bugs in the past. Be extra careful when changing these files, and have an extra careful look at these:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks good, but I have some questions
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
Tests/SentryTests/Integrations/SessionReplay/SentrySessionReplayTests.swift
Outdated
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks, I'm sorry I added plenty of comments. Also some nits. Feel free to ignore some of the nits, if you want to speed the PR up.
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
Sources/Swift/Integrations/SessionReplay/SentrySessionReplay.swift
Outdated
Show resolved
Hide resolved
…ble-session-replay-ios26
Based on this PR comment I did an analysis of the
|
Tests/SentryTests/Integrations/SessionReplay/SentrySessionReplayTests.swift
Show resolved
Hide resolved
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, when resolving the comment about the public enum SentryInfoPlistKey
Sources/Swift/Integrations/SessionReplay/SentrySessionReplayEnvironmentChecker.swift
Outdated
Show resolved
Hide resolved
As mentioned in this PR comment we should not use Therefore I am going to remove it from the PR now. |
…t enough reason to adopt it
…ble-session-replay-ios26
…ive-C usage - Changed the default initializer to override and set the bundle to Bundle.main. - Added a new public initializer to allow custom bundle injection while maintaining compatibility with Objective-C.
📜 Description
DTXcode
from the app'sInfo.plist
to detect if the environment should be considereddangerous
for PII.💡 Motivation and Context
While working on #6292 I noticed that standard SwiftUI components like
Text
,Image
,Label
are not properly masked. Further research has shown that this is caused by the masking/redaction logic not being able to detect sensitive views due to Apple changing the internal rendering logic with their release of Liquid Glass on iOS 26.0.Liquid Glass is only used by an app running on iOS 26.0 if the app was built with Xcode 26.0, therefore we can rule out apps built with older Xcode versions. This can be checked by looking at the
DT*
fields in the app'sInfo.plist
.The approach taken in this PR is defensive, meaning that we assume the runtime environment is dangerous for PII unless there are strong indicators it's safe. This approach should reduce the risk of PII being leaked by changes outside of our control, but it could cause session replay not being started even though it's safe ("better safe than sorry").
I am going to create a follow-up issue with more insight into the issue and link it here afterwards.
💚 How did you test it?
Info.plist
of the two apps to confirm the values of the fieldDTXcode
. Using Xcode 16.4options.sessionReplay.disableInDangerousEnvironment = true
(default) → session replay is disabledoptions.sessionReplay.disableInDangerousEnvironment = false
→ session replay was enabled📝 Checklist
You have to check all boxes before merging:
sendDefaultPII
is enabled.