fix(core): Restrict getDataFromUri native bridge methods#6045
fix(core): Restrict getDataFromUri native bridge methods#6045
Conversation
iOS: require file URLs to resolve inside NSTemporaryDirectory, caches, or documents. Android: allow content URIs only with known media / SAF document authorities or the app's own FileProvider; file URIs must resolve inside the app's internal/external files or caches dirs. Adds Android unit tests for the validator. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
@sentry review |
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit b51165d. Configure here.
Android (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4953e94+dirty | 442.02 ms | 456.52 ms | 14.50 ms |
| 2c735cc+dirty | 414.09 ms | 438.47 ms | 24.38 ms |
| df5d108+dirty | 527.06 ms | 603.58 ms | 76.52 ms |
| 3ce5254+dirty | 410.57 ms | 448.48 ms | 37.91 ms |
| 3d377b5+dirty | 406.18 ms | 453.52 ms | 47.34 ms |
| 0d9949d+dirty | 403.57 ms | 437.00 ms | 33.43 ms |
| 4b87b12+dirty | 421.82 ms | 413.60 ms | -8.22 ms |
| 7ac3378+dirty | 404.78 ms | 439.84 ms | 35.06 ms |
| 890d145+dirty | 504.54 ms | 491.55 ms | -12.99 ms |
| 3817909+dirty | 406.67 ms | 416.58 ms | 9.91 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4953e94+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| 2c735cc+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| df5d108+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
| 3ce5254+dirty | 43.75 MiB | 48.12 MiB | 4.37 MiB |
| 3d377b5+dirty | 43.75 MiB | 48.14 MiB | 4.39 MiB |
| 0d9949d+dirty | 43.75 MiB | 48.13 MiB | 4.37 MiB |
| 4b87b12+dirty | 43.75 MiB | 48.14 MiB | 4.39 MiB |
| 7ac3378+dirty | 43.75 MiB | 48.13 MiB | 4.37 MiB |
| 890d145+dirty | 43.75 MiB | 48.14 MiB | 4.39 MiB |
| 3817909+dirty | 43.75 MiB | 48.08 MiB | 4.33 MiB |
📲 Install BuildsAndroid
|
iOS (legacy) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7ac3378+dirty | 1213.37 ms | 1218.15 ms | 4.78 ms |
| 4b87b12+dirty | 1212.90 ms | 1222.09 ms | 9.19 ms |
| 890d145+dirty | 1223.59 ms | 1231.37 ms | 7.78 ms |
| 0d9949d+dirty | 1211.38 ms | 1219.67 ms | 8.29 ms |
| 04207c4+dirty | 1191.27 ms | 1189.78 ms | -1.48 ms |
| 3ce5254+dirty | 1219.93 ms | 1221.90 ms | 1.96 ms |
| 4953e94+dirty | 1212.06 ms | 1214.83 ms | 2.77 ms |
| 2c735cc+dirty | 1229.67 ms | 1221.50 ms | -8.17 ms |
| a50b33d+dirty | 1197.74 ms | 1197.17 ms | -0.57 ms |
| df5d108+dirty | 1225.90 ms | 1220.14 ms | -5.76 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7ac3378+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 4b87b12+dirty | 3.38 MiB | 4.77 MiB | 1.39 MiB |
| 890d145+dirty | 3.38 MiB | 4.77 MiB | 1.38 MiB |
| 0d9949d+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 04207c4+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 3ce5254+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 4953e94+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 2c735cc+dirty | 3.38 MiB | 4.74 MiB | 1.35 MiB |
| a50b33d+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| df5d108+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
iOS (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7ac3378+dirty | 1202.35 ms | 1198.31 ms | -4.04 ms |
| 4b87b12+dirty | 1199.49 ms | 1199.78 ms | 0.29 ms |
| 890d145+dirty | 1212.98 ms | 1220.10 ms | 7.12 ms |
| 0d9949d+dirty | 1203.94 ms | 1202.27 ms | -1.67 ms |
| 04207c4+dirty | 1228.55 ms | 1226.04 ms | -2.51 ms |
| 3ce5254+dirty | 1217.70 ms | 1224.69 ms | 6.99 ms |
| 4953e94+dirty | 1217.41 ms | 1223.53 ms | 6.12 ms |
| 2c735cc+dirty | 1223.33 ms | 1224.38 ms | 1.04 ms |
| a50b33d+dirty | 1207.11 ms | 1212.10 ms | 5.00 ms |
| df5d108+dirty | 1207.34 ms | 1210.50 ms | 3.16 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 7ac3378+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 4b87b12+dirty | 3.38 MiB | 4.77 MiB | 1.39 MiB |
| 890d145+dirty | 3.38 MiB | 4.77 MiB | 1.38 MiB |
| 0d9949d+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 04207c4+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 3ce5254+dirty | 3.38 MiB | 4.76 MiB | 1.38 MiB |
| 4953e94+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| 2c735cc+dirty | 3.38 MiB | 4.74 MiB | 1.35 MiB |
| a50b33d+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
| df5d108+dirty | 3.38 MiB | 4.73 MiB | 1.35 MiB |
Android (new) Performance metrics 🚀
|
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4953e94+dirty | 398.80 ms | 431.81 ms | 33.01 ms |
| 2c735cc+dirty | 435.20 ms | 459.48 ms | 24.28 ms |
| df5d108+dirty | 434.82 ms | 447.39 ms | 12.57 ms |
| 3ce5254+dirty | 373.90 ms | 427.84 ms | 53.94 ms |
| 3d377b5+dirty | 425.38 ms | 440.67 ms | 15.30 ms |
| 0d9949d+dirty | 414.88 ms | 428.68 ms | 13.81 ms |
| 4b87b12+dirty | 356.23 ms | 399.86 ms | 43.63 ms |
| 7ac3378+dirty | 410.67 ms | 442.60 ms | 31.92 ms |
| 890d145+dirty | 486.42 ms | 514.85 ms | 28.43 ms |
| 3817909+dirty | 357.52 ms | 391.52 ms | 34.00 ms |
App size
| Revision | Plain | With Sentry | Diff |
|---|---|---|---|
| 4953e94+dirty | 43.94 MiB | 48.94 MiB | 5.00 MiB |
| 2c735cc+dirty | 43.94 MiB | 48.94 MiB | 5.00 MiB |
| df5d108+dirty | 43.94 MiB | 48.94 MiB | 5.00 MiB |
| 3ce5254+dirty | 43.94 MiB | 48.98 MiB | 5.04 MiB |
| 3d377b5+dirty | 43.94 MiB | 49.00 MiB | 5.06 MiB |
| 0d9949d+dirty | 43.94 MiB | 48.99 MiB | 5.05 MiB |
| 4b87b12+dirty | 43.94 MiB | 49.00 MiB | 5.06 MiB |
| 7ac3378+dirty | 43.94 MiB | 48.99 MiB | 5.05 MiB |
| 890d145+dirty | 43.94 MiB | 49.00 MiB | 5.06 MiB |
| 3817909+dirty | 43.94 MiB | 48.94 MiB | 5.00 MiB |
Fixes CI lint:android failure — two line-wrapping tweaks flagged by google-java-format. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
| #if TARGET_OS_IPHONE || TARGET_OS_MACCATALYST | ||
| static BOOL RNSentryIsPathUnderAllowedRoots(NSString *path) | ||
| { | ||
| if (path.length == 0) { | ||
| return NO; | ||
| } | ||
| for (NSString *component in path.pathComponents) { | ||
| if ([component isEqualToString:@".."]) { | ||
| return NO; | ||
| } | ||
| } | ||
| NSString *resolved = [path stringByResolvingSymlinksInPath]; | ||
| NSMutableArray<NSString *> *roots = [NSMutableArray arrayWithCapacity:3]; | ||
| NSString *tmp = NSTemporaryDirectory(); | ||
| if (tmp.length > 0) { | ||
| [roots addObject:[tmp stringByResolvingSymlinksInPath]]; | ||
| } | ||
| NSArray<NSString *> *caches = NSSearchPathForDirectoriesInDomains( | ||
| NSCachesDirectory, NSUserDomainMask, YES); | ||
| if (caches.firstObject) { | ||
| [roots addObject:[caches.firstObject stringByResolvingSymlinksInPath]]; | ||
| } | ||
| NSArray<NSString *> *docs = NSSearchPathForDirectoriesInDomains( | ||
| NSDocumentDirectory, NSUserDomainMask, YES); | ||
| if (docs.firstObject) { | ||
| [roots addObject:[docs.firstObject stringByResolvingSymlinksInPath]]; | ||
| } | ||
| for (NSString *root in roots) { | ||
| NSString *rootWithSlash = [root hasSuffix:@"/"] ? root : [root stringByAppendingString:@"/"]; | ||
| if ([resolved isEqualToString:root] || [resolved hasPrefix:rootWithSlash]) { | ||
| return YES; | ||
| } | ||
| } | ||
| return NO; | ||
| } |
There was a problem hiding this comment.
No unit tests for iOS path validation function
The RNSentryIsPathUnderAllowedRoots function implements security-critical path validation logic but has no corresponding unit tests, unlike the Android implementation which has comprehensive tests in RNSentryUriValidationTest.java. This validation function handles path traversal prevention and symlink resolution - edge cases that should be tested to ensure bypass attempts are properly blocked.
Verification
Searched for iOS test files using patterns Test, Tests, Spec in packages/core/ios and found none. Searched for references to RNSentryIsPathUnderAllowedRoots and found only the implementation in RNSentry.mm. Confirmed Android has comprehensive tests in packages/core/android/src/test/java/io/sentry/react/RNSentryUriValidationTest.java covering traversal escape, prefix collision, and directory boundary cases.
Identified by Warden code-review · 6RE-5C3
Type of change
Description
Tightens the URI validation performed by the
getDataFromUrinative bridge method on both iOS and Android so it only accepts URIs that map to the attachment/screenshot flows the SDK actually uses.iOS (
RNSentry.mm): on top of the existingisFileURLcheck, the resolved path must sit insideNSTemporaryDirectory(), caches, or documents. Paths containing..components are rejected before symlink resolution.Android (
RNSentryModuleImpl.java):content://— allow onlymedia, the standard SAF document authorities, and the app's ownFileProvider.file://— only paths undergetFilesDir(),getCacheDir(),getExternalFilesDir(null), orgetExternalCacheDir()(canonical-path prefix check resolves..and symlinks).The only in-SDK caller is
FeedbackForm.tsx(image picker / screenshot attach), whose URIs fall within the allowed sets on both platforms.Motivation and Context
Internal security review.
How did you test it?
RNSentryUriValidationTestwith 17 cases covering accepted and rejected URIs, traversal, prefix collision, unknown schemes, and foreign authorities — all pass via./gradlew :sentry_react-native:testDebugUnitTest.yarn build,yarn test,yarn lint,yarn circularDepCheckall clean.Checklist
Next steps
Consider setting up an iOS XCTest target as a follow-up so native-side validators have unit coverage on both platforms.