Skip to content

Conversation

krystofwoldrich
Copy link
Contributor

@krystofwoldrich krystofwoldrich commented Jun 13, 2025

📜 Description

SFSafariView doesn't use WebView component and thus is not redacted with the current set of classes to redact.

💡 Motivation and Context

fixes: getsentry/sentry-react-native#4920

💚 How did you test it?

tested with adjusted reproducible example: https://github.com/krystofwoldrich/expo-auth-session-sentry

example of replay before this pr: https://krystof-woldrich.sentry.io/explore/replays/9d5c14e2372c4d4ca23e6238bdf63aa9

example of replay after this pr: https://krystof-woldrich.sentry.io/explore/replays/7b883d4d68594c06a4869884788b4e9c

📝 Checklist

You have to check all boxes before merging:

  • I added tests to verify the changes.
  • No new PII added or SDK only sends newly added PII if sendDefaultPII is enabled.
  • I updated the docs if needed.
  • I updated the wizard if needed.
  • Review from the native team if needed.
  • No breaking change or entry added to the changelog.
  • No breaking change for hybrid SDKs or communicated to hybrid SDKs.

Copy link

codecov bot commented Jun 13, 2025

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 86.031%. Comparing base (8df2e0d) to head (b01ca13).
Report is 1 commits behind head on main.

Additional details and impacted files

Impacted file tree graph

@@              Coverage Diff              @@
##              main     #5408       +/-   ##
=============================================
+ Coverage   86.022%   86.031%   +0.009%     
=============================================
  Files          399       399               
  Lines        34621     34629        +8     
  Branches     14974     14995       +21     
=============================================
+ Hits         29782     29792       +10     
+ Misses        4797      4794        -3     
- Partials        42        43        +1     
Files with missing lines Coverage Δ
...Core/Tools/ViewCapture/SentryUIRedactBuilder.swift 93.913% <100.000%> (+0.163%) ⬆️

... and 7 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 8df2e0d...b01ca13. Read the comment docs.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

Copy link
Contributor

github-actions bot commented Jun 13, 2025

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1216.00 ms 1242.35 ms 26.35 ms
Size 23.75 KiB 846.87 KiB 823.13 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
17afc4b 1228.94 ms 1251.10 ms 22.16 ms
ebeb68c 1216.27 ms 1243.80 ms 27.53 ms
01a28a9 1225.55 ms 1249.96 ms 24.41 ms
fb53d97 1235.00 ms 1241.88 ms 6.88 ms
245f981 1228.65 ms 1248.76 ms 20.11 ms
3723833 1205.22 ms 1216.94 ms 11.71 ms
b055f7b 1232.90 ms 1247.65 ms 14.75 ms
7cd187e 1243.04 ms 1244.79 ms 1.75 ms
dd34512 1221.45 ms 1251.23 ms 29.77 ms
72f0262 1239.84 ms 1258.42 ms 18.59 ms

App size

Revision Plain With Sentry Diff
17afc4b 20.76 KiB 436.25 KiB 415.49 KiB
ebeb68c 21.58 KiB 698.37 KiB 676.79 KiB
01a28a9 22.85 KiB 405.39 KiB 382.55 KiB
fb53d97 20.76 KiB 425.80 KiB 405.04 KiB
245f981 23.75 KiB 847.17 KiB 823.42 KiB
3723833 21.58 KiB 424.34 KiB 402.76 KiB
b055f7b 23.75 KiB 840.58 KiB 816.83 KiB
7cd187e 20.76 KiB 401.65 KiB 380.89 KiB
dd34512 23.76 KiB 869.05 KiB 845.29 KiB
72f0262 21.58 KiB 418.14 KiB 396.56 KiB

Previous results on branch: kw-fix-safari-view-redaction

Startup times

Revision Plain With Sentry Diff
8ef582f 1233.71 ms 1257.56 ms 23.85 ms
b7f52f8 1222.65 ms 1240.54 ms 17.89 ms
c78b91b 1235.33 ms 1247.79 ms 12.46 ms
bb4b506 1241.00 ms 1251.98 ms 10.98 ms
82da6d1 1205.81 ms 1232.00 ms 26.19 ms
3644101 1219.80 ms 1239.54 ms 19.75 ms

App size

Revision Plain With Sentry Diff
8ef582f 23.75 KiB 847.02 KiB 823.28 KiB
b7f52f8 23.75 KiB 847.02 KiB 823.27 KiB
c78b91b 23.75 KiB 847.02 KiB 823.28 KiB
bb4b506 23.75 KiB 846.85 KiB 823.11 KiB
82da6d1 23.75 KiB 847.02 KiB 823.28 KiB
3644101 23.75 KiB 847.02 KiB 823.27 KiB

@krystofwoldrich krystofwoldrich force-pushed the kw-fix-safari-view-redaction branch from c3f45c3 to da1944e Compare June 13, 2025 17:37
@philprime philprime self-assigned this Jun 16, 2025
@philprime
Copy link
Member

Thank you for the PR. I'll take over here and make sure we have an example in the sample apps and tests/verification

@philprime
Copy link
Member

  • Added a SFSafariViewController to iOS sample
  • Added a ASWebAuthenticationSession for web-based authentication to iOS sample
  • Cleanup and tests

@philprime philprime added the Waiting for: CI 🤖 Marks PR as ready-for-merge when CI passes. To be replaced with required checks. label Jun 16, 2025
@philprime
Copy link
Member

I manually tested the redaction for iOS 16, 17 and 18.
Both SFSafariViewController and the ASWebAuthenticationSession are redacted correctly.

@philprime philprime changed the title fix(replay): Redact SFSafariView fix(session-replay): Add redaction for SFSafariView Jun 17, 2025
@philprime philprime merged commit 2e790c6 into main Jun 17, 2025
129 of 130 checks passed
@philprime philprime deleted the kw-fix-safari-view-redaction branch June 17, 2025 08:44
Copy link
Contributor

Fails
🚫 Please consider adding a changelog entry for the next release.

Instructions and example for changelog

Please add an entry to CHANGELOG.md to the "Unreleased" section. Make sure the entry includes this PR's number.

Example:

## Unreleased

- Add redaction for SFSafariView ([#5408](https://github.com/getsentry/sentry-cocoa/pull/5408))

If none of the above apply, you can opt out of this check by adding #skip-changelog to the PR description.

Generated by 🚫 dangerJS against b01ca13

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Waiting for: CI 🤖 Marks PR as ready-for-merge when CI passes. To be replaced with required checks.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No way to mask expo-web-browser when using expo-auth-session for authentication.
2 participants