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

fix: discontinue NSApplicationSupportDirectory usage #4335

Merged
merged 5 commits into from
Sep 18, 2024

Conversation

armcknight
Copy link
Member

@armcknight armcknight commented Sep 16, 2024

Update: we will no longer use NSApplicationSupportDirectory and just move the few usages of it to use NSCachesDirectory.

Original description:

We received a question about what information we store to disk. Almost everything is stored in NSCachesDirectory, which is not included in device backups: https://developer.apple.com/library/archive/documentation/FileManagement/Conceptual/FileSystemProgrammingGuide/FileSystemOverview/FileSystemOverview.html

image

But, we started adding things to NSApplicationSupportDirectory, which is included in backups by default. Currently, no PII is written there, but this PR will mark the entire directory as excluded from backups as it's not necessary to back it up, and this will mitigate any accidental PII leaks in the future if we start writing more things there.

Fixes #4334

This comment was marked as outdated.

Copy link

github-actions bot commented Sep 16, 2024

🚨 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:

  • Sources/Sentry/SentryFileManager.m

Copy link

codecov bot commented Sep 16, 2024

Codecov Report

Attention: Patch coverage is 94.44444% with 1 line in your changes missing coverage. Please review.

Please upload report for BASE (main@6ccaa36). Learn more about missing BASE report.
Report is 1 commits behind head on main.

Files with missing lines Patch % Lines
Sources/Sentry/SentryFileManager.m 91.666% 1 Missing ⚠️
Additional details and impacted files

Impacted file tree graph

@@           Coverage Diff            @@
##             main     #4335   +/-   ##
========================================
  Coverage        ?   91.691%           
========================================
  Files           ?       622           
  Lines           ?     50503           
  Branches        ?     18271           
========================================
  Hits            ?     46307           
  Misses          ?      4103           
  Partials        ?        93           
Files with missing lines Coverage Δ
...urces/Sentry/Profiling/SentryProfilerTestHelpers.m 84.848% <100.000%> (ø)
Sources/Sentry/SentryLogC.m 66.666% <100.000%> (ø)
Sources/Sentry/SentryFileManager.m 94.225% <91.666%> (ø)

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 6ccaa36...ccd892e. Read the comment docs.

Copy link
Contributor

@brustolin brustolin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe an overkill, but we could write a test that checks if the flag was set after file manager creates that path, to avoid regression.

Sources/Sentry/SentryFileManager.m Outdated Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it's better to ditch using sentryApplicationSupportPath. I added my explanation in the comment #4335 (comment).

Sources/Sentry/SentryFileManager.m Outdated Show resolved Hide resolved
@brustolin
Copy link
Contributor

I also prefer to ditch sentryApplicationSupportPath all together.

Copy link

🚨 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:

  • Sources/Sentry/SentryFileManager.m

@armcknight armcknight changed the title fix: exclude our NSApplicationSupportDirectory from device backups fix: discontinue NSApplicationSupportDirectory usage Sep 17, 2024
Copy link

github-actions bot commented Sep 17, 2024

Performance metrics 🚀

  Plain With Sentry Diff
Startup time 1230.51 ms 1244.57 ms 14.06 ms
Size 21.58 KiB 714.30 KiB 692.72 KiB

Baseline results on branch: main

Startup times

Revision Plain With Sentry Diff
83887af 1196.94 ms 1206.82 ms 9.88 ms
542727d 1227.96 ms 1246.88 ms 18.92 ms
adcc7d8 1225.90 ms 1245.08 ms 19.18 ms
01a28a9 1200.78 ms 1227.90 ms 27.12 ms
dacf894 1232.32 ms 1236.34 ms 4.02 ms
d3abae0 1200.36 ms 1224.22 ms 23.87 ms
bd05478 1233.82 ms 1255.56 ms 21.74 ms
282cc99 1238.27 ms 1253.46 ms 15.19 ms
105a36c 1227.37 ms 1245.36 ms 17.99 ms
f1b97be 1212.69 ms 1226.12 ms 13.43 ms

App size

Revision Plain With Sentry Diff
83887af 21.58 KiB 419.64 KiB 398.06 KiB
542727d 21.58 KiB 571.85 KiB 550.26 KiB
adcc7d8 20.76 KiB 426.15 KiB 405.39 KiB
01a28a9 22.85 KiB 405.39 KiB 382.54 KiB
dacf894 20.76 KiB 426.34 KiB 405.58 KiB
d3abae0 20.76 KiB 434.92 KiB 414.16 KiB
bd05478 20.76 KiB 432.33 KiB 411.57 KiB
282cc99 22.85 KiB 414.09 KiB 391.24 KiB
105a36c 22.85 KiB 414.09 KiB 391.24 KiB
f1b97be 21.58 KiB 681.73 KiB 660.15 KiB

Previous results on branch: armcknight/fix/exclude-nsappsupport-from-backups

Startup times

Revision Plain With Sentry Diff
87eba61 1233.76 ms 1255.35 ms 21.59 ms

App size

Revision Plain With Sentry Diff
87eba61 21.58 KiB 713.54 KiB 691.96 KiB

CHANGELOG.md Outdated Show resolved Hide resolved
Copy link
Member

@philipphofmann philipphofmann left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you @armcknight 💯

Copy link

🚨 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:

  • Sources/Sentry/SentryFileManager.m

@armcknight armcknight merged commit c45c91c into main Sep 18, 2024
63 of 65 checks passed
@armcknight armcknight deleted the armcknight/fix/exclude-nsappsupport-from-backups branch September 18, 2024 18:03
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Exclude SDK's working/cache/temp files from backup
4 participants