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: add logging for invalid cache directory path validation #4693

Merged
merged 14 commits into from
Jan 17, 2025
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -2,12 +2,15 @@

## Unreleased

### Improvements

- Add error logging for invalid `cacheDirectoryPath` (#4693)

### Fixes

- Replace occurences of `strncpy` with `strlcpy` (#4636)
- Fix span recording for `NSFileManager.createFileAtPath` starting with iOS 18, macOS 15 and tvOS 18. This feature is experimental and must be enabled by setting the option `experimental.enableFileManagerSwizzling` to `true` (#4634)


### Internal

- Update to Xcode 16.2 in workflows (#4673)
Expand Down
2 changes: 1 addition & 1 deletion Samples/iOS-Swift/iOS-Swift/SentrySDKWrapper.swift
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,7 @@
options.beforeCaptureScreenshot = { _ in true }
options.beforeCaptureViewHierarchy = { _ in true }
options.debug = true

if #available(iOS 16.0, *), enableSessionReplay {
options.sessionReplay = SentryReplayOptions(sessionSampleRate: 0, onErrorSampleRate: 1, maskAllText: true, maskAllImages: true)
options.sessionReplay.quality = .high
Expand Down Expand Up @@ -52,7 +52,7 @@
options.enableNetworkBreadcrumbs = enableNetworkBreadcrumbs
options.enableSwizzling = enableSwizzling
options.enableCrashHandler = enableCrashHandling
options.enableTracing = enableTracing

Check warning on line 55 in Samples/iOS-Swift/iOS-Swift/SentrySDKWrapper.swift

View workflow job for this annotation

GitHub Actions / UI Tests for iOS-Swift iPhone 14 (16.4) Simulator

'enableTracing' is deprecated: Use tracesSampleRate or tracesSampler instead
options.enablePersistingTracesWhenCrashing = true
options.attachScreenshot = enableAttachScreenshot
options.attachViewHierarchy = enableAttachViewHierarchy
Expand Down
8 changes: 8 additions & 0 deletions Sentry.xcodeproj/project.pbxproj
Original file line number Diff line number Diff line change
Expand Up @@ -788,6 +788,8 @@
A8AFFCD42907E0CA00967CD7 /* SentryRequestTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = A8AFFCD32907E0CA00967CD7 /* SentryRequestTests.swift */; };
A8F17B2E2901765900990B25 /* SentryRequest.m in Sources */ = {isa = PBXBuildFile; fileRef = A8F17B2D2901765900990B25 /* SentryRequest.m */; };
A8F17B342902870300990B25 /* SentryHttpStatusCodeRange.m in Sources */ = {isa = PBXBuildFile; fileRef = A8F17B332902870300990B25 /* SentryHttpStatusCodeRange.m */; };
D42560CB2D2FD4C10069413F /* SentryOptionsValidator.swift in Sources */ = {isa = PBXBuildFile; fileRef = D42560CA2D2FD4BD0069413F /* SentryOptionsValidator.swift */; };
D42560CD2D2FEFF70069413F /* SentryOptionsValidatorTests.swift in Sources */ = {isa = PBXBuildFile; fileRef = D42560CC2D2FEFF10069413F /* SentryOptionsValidatorTests.swift */; };
D4AF00212D2E92FD00F5F3D7 /* SentryNSFileManagerSwizzling.m in Sources */ = {isa = PBXBuildFile; fileRef = D4AF00202D2E92FD00F5F3D7 /* SentryNSFileManagerSwizzling.m */; };
D4AF00232D2E931000F5F3D7 /* SentryNSFileManagerSwizzling.h in Headers */ = {isa = PBXBuildFile; fileRef = D4AF00222D2E931000F5F3D7 /* SentryNSFileManagerSwizzling.h */; };
D4AF00252D2E93C400F5F3D7 /* SentryNSFileManagerSwizzlingTests.m in Sources */ = {isa = PBXBuildFile; fileRef = D4AF00242D2E93C400F5F3D7 /* SentryNSFileManagerSwizzlingTests.m */; };
Expand Down Expand Up @@ -1874,6 +1876,8 @@
A8AFFCD32907E0CA00967CD7 /* SentryRequestTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryRequestTests.swift; sourceTree = "<group>"; };
A8F17B2D2901765900990B25 /* SentryRequest.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryRequest.m; sourceTree = "<group>"; };
A8F17B332902870300990B25 /* SentryHttpStatusCodeRange.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryHttpStatusCodeRange.m; sourceTree = "<group>"; };
D42560CA2D2FD4BD0069413F /* SentryOptionsValidator.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryOptionsValidator.swift; sourceTree = "<group>"; };
D42560CC2D2FEFF10069413F /* SentryOptionsValidatorTests.swift */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.swift; path = SentryOptionsValidatorTests.swift; sourceTree = "<group>"; };
D4AF00202D2E92FD00F5F3D7 /* SentryNSFileManagerSwizzling.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryNSFileManagerSwizzling.m; sourceTree = "<group>"; };
D4AF00222D2E931000F5F3D7 /* SentryNSFileManagerSwizzling.h */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.h; name = SentryNSFileManagerSwizzling.h; path = include/SentryNSFileManagerSwizzling.h; sourceTree = "<group>"; };
D4AF00242D2E93C400F5F3D7 /* SentryNSFileManagerSwizzlingTests.m */ = {isa = PBXFileReference; lastKnownFileType = sourcecode.c.objc; path = SentryNSFileManagerSwizzlingTests.m; sourceTree = "<group>"; };
Expand Down Expand Up @@ -2556,6 +2560,7 @@
A8AFFCD12907DA7600967CD7 /* SentryHttpStatusCodeRangeTests.swift */,
7B6CC50124EE5A42001816D7 /* SentryHubTests.swift */,
15D0AC872459EE4D006541C2 /* SentryNSURLRequestTests.swift */,
D42560CC2D2FEFF10069413F /* SentryOptionsValidatorTests.swift */,
7B0A54552523178700A71716 /* SentryScopeSwiftTests.swift */,
D8918B212849FA6D00701F9A /* SentrySDKIntegrationTestsBase.swift */,
7BA8409F24A1EC6E00B718AA /* SentrySDKTests.swift */,
Expand Down Expand Up @@ -3662,6 +3667,7 @@
D800942328F82E8D005D3943 /* Swift */ = {
isa = PBXGroup;
children = (
D42560CA2D2FD4BD0069413F /* SentryOptionsValidator.swift */,
D8CAC02D2BA0663E00E38F34 /* Integrations */,
621D9F2D2B9B030E003D94DE /* Helper */,
D8F016B42B962533007B9AFB /* Extensions */,
Expand Down Expand Up @@ -4898,6 +4904,7 @@
620379DD2AFE1432005AC0C1 /* SentryBuildAppStartSpans.m in Sources */,
7B77BE3727EC8460003C9020 /* SentryDiscardReasonMapper.m in Sources */,
63FE712520DA4C1000CDBAE8 /* SentryCrashSignalInfo.c in Sources */,
D42560CB2D2FD4C10069413F /* SentryOptionsValidator.swift in Sources */,
63FE70F320DA4C1000CDBAE8 /* SentryCrashMonitor_Signal.c in Sources */,
D859696F27BECDA20036A46E /* SentryCoreDataTracker.m in Sources */,
);
Expand Down Expand Up @@ -5161,6 +5168,7 @@
7BF9EF7A2722B58900B5BBEF /* SentrySubClassFinderTests.swift in Sources */,
7B59398224AB47650003AAD2 /* SentrySessionTrackerTests.swift in Sources */,
7B05A61824A4D14A00EF211D /* SentrySessionGeneratorTests.swift in Sources */,
D42560CD2D2FEFF70069413F /* SentryOptionsValidatorTests.swift in Sources */,
D8CB742B294B1DD000A5F964 /* SentryUIApplicationTests.swift in Sources */,
63FE720920DA66EC00CDBAE8 /* XCTestCase+SentryCrash.m in Sources */,
D8918B222849FA6D00701F9A /* SentrySDKIntegrationTestsBase.swift in Sources */,
Expand Down
8 changes: 7 additions & 1 deletion Sources/Sentry/PrivateSentrySDKOnly.mm
Original file line number Diff line number Diff line change
Expand Up @@ -135,7 +135,13 @@

+ (NSString *)installationID
{
return [SentryInstallation idWithCacheDirectoryPath:self.options.cacheDirectoryPath];
NSString *cacheDirectoryPath = self.options.cacheDirectoryPath;

Check warning on line 138 in Sources/Sentry/PrivateSentrySDKOnly.mm

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/PrivateSentrySDKOnly.mm#L138

Added line #L138 was not covered by tests
if (![SentryOptionsValidator isCacheDirectoryPathValidWithPath:cacheDirectoryPath]) {
SENTRY_LOG_FATAL(@"The configured cache directory path looks invalid, the SDK might not be "
@"able to write reports to disk: %@",
cacheDirectoryPath);
}
return [SentryInstallation idWithCacheDirectoryPath:cacheDirectoryPath];

Check warning on line 144 in Sources/Sentry/PrivateSentrySDKOnly.mm

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/PrivateSentrySDKOnly.mm#L141-L144

Added lines #L141 - L144 were not covered by tests
}

+ (SentryOptions *)options
Expand Down
12 changes: 11 additions & 1 deletion Sources/Sentry/SentryClient.m
Original file line number Diff line number Diff line change
Expand Up @@ -939,8 +939,18 @@
// We only want to set the id if the customer didn't set a user so we at least set something to
// identify the user.
if (event.user == nil) {
NSString *cacheDirectoryPath = self.options.cacheDirectoryPath;
if (![SentryOptionsValidator isCacheDirectoryPathValidWithPath:cacheDirectoryPath]) {
[SentryLog
logWithMessage:[NSString stringWithFormat:
@"The configured cache directory path looks invalid, "
@"the SDK might not be able to write reports to disk: %@",
cacheDirectoryPath]
andLevel:kSentryLevelError];

Check warning on line 949 in Sources/Sentry/SentryClient.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryClient.m#L944-L949

Added lines #L944 - L949 were not covered by tests
}

SentryUser *user = [[SentryUser alloc] init];
user.userId = [SentryInstallation idWithCacheDirectoryPath:self.options.cacheDirectoryPath];
user.userId = [SentryInstallation idWithCacheDirectoryPath:cacheDirectoryPath];
event.user = user;
}
}
Expand Down
7 changes: 7 additions & 0 deletions Sources/Sentry/SentryCrashIntegration.m
Original file line number Diff line number Diff line change
Expand Up @@ -118,6 +118,13 @@
}
#endif // TARGET_OS_OSX

if (![SentryOptionsValidator isCacheDirectoryPathValidWithPath:options.cacheDirectoryPath]) {
[SentryLog logWithMessage:[NSString stringWithFormat:
@"The configured cache directory path looks invalid, the "
@"SDK might not be able to write reports to disk: %@",
options.cacheDirectoryPath]
andLevel:kSentryLevelFatal];

Check warning on line 126 in Sources/Sentry/SentryCrashIntegration.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryCrashIntegration.m#L122-L126

Added lines #L122 - L126 were not covered by tests
}
[self startCrashHandler:options.cacheDirectoryPath
enableSigtermReporting:enableSigtermReporting
enableReportingUncaughtExceptions:enableUncaughtNSExceptionReporting];
Expand Down
7 changes: 7 additions & 0 deletions Sources/Sentry/SentryDependencyContainer.m
Original file line number Diff line number Diff line change
Expand Up @@ -151,6 +151,13 @@ - (SentryCrash *)crashReporter SENTRY_DISABLE_THREAD_SANITIZER(
if (_crashReporter == nil) {
@synchronized(sentryDependencyContainerLock) {
if (_crashReporter == nil) {
NSString *cacheDirectoryPath = SentrySDK.options.cacheDirectoryPath;
if (![SentryOptionsValidator
isCacheDirectoryPathValidWithPath:cacheDirectoryPath]) {
SENTRY_LOG_FATAL(@"The configured cache directory path looks invalid, the "
@"SDK might not be able to write reports to disk: %@",
cacheDirectoryPath);
}
_crashReporter =
[[SentryCrash alloc] initWithBasePath:SentrySDK.options.cacheDirectoryPath];
}
Expand Down
7 changes: 6 additions & 1 deletion Sources/Sentry/SentryFileManager.m
Original file line number Diff line number Diff line change
Expand Up @@ -705,9 +705,14 @@ + (BOOL)createDirectoryAtPath:(NSString *)path withError:(NSError **)error
- (void)createPathsWithOptions:(SentryOptions *)options
{
NSString *cachePath = options.cacheDirectoryPath;

SENTRY_LOG_DEBUG(@"SentryFileManager.cachePath: %@", cachePath);

if (![SentryOptionsValidator isCacheDirectoryPathValidWithPath:cachePath]) {
SENTRY_LOG_FATAL(@"The configured cache directory path looks invalid, the SDK might not be "
@"able to write reports to disk: %@",
cachePath);
}

self.basePath = [cachePath stringByAppendingPathComponent:@"io.sentry"];
self.sentryPath = [self.basePath stringByAppendingPathComponent:[options.parsedDsn getHash]];
self.currentSessionFilePath =
Expand Down
13 changes: 11 additions & 2 deletions Sources/Sentry/SentryHub.m
Original file line number Diff line number Diff line change
Expand Up @@ -103,8 +103,17 @@
lastSession = _session;
}

NSString *distinctId =
[SentryInstallation idWithCacheDirectoryPath:options.cacheDirectoryPath];
NSString *cacheDirectoryPath = options.cacheDirectoryPath;
if (![SentryOptionsValidator isCacheDirectoryPathValidWithPath:cacheDirectoryPath]) {
[SentryLog
logWithMessage:[NSString stringWithFormat:
@"The configured cache directory path looks invalid, "
@"the SDK might not be able to write reports to disk: %@",
cacheDirectoryPath]
andLevel:kSentryLevelFatal];

Check warning on line 113 in Sources/Sentry/SentryHub.m

View check run for this annotation

Codecov / codecov/patch

Sources/Sentry/SentryHub.m#L108-L113

Added lines #L108 - L113 were not covered by tests
}

NSString *distinctId = [SentryInstallation idWithCacheDirectoryPath:cacheDirectoryPath];

_session = [[SentrySession alloc] initWithReleaseName:options.releaseName
distinctId:distinctId];
Expand Down
2 changes: 1 addition & 1 deletion Sources/Sentry/SentryOptions.m
Original file line number Diff line number Diff line change
Expand Up @@ -360,7 +360,7 @@ - (BOOL)validateOptions:(NSDictionary<NSString *, id> *)options
self.maxCacheItems = [options[@"maxCacheItems"] unsignedIntValue];
}

if ([options[@"cacheDirectoryPath"] isKindOfClass:[NSString class]]) {
if ([SentryOptionsValidator isCacheDirectoryPathValidWithPath:options[@"cacheDirectoryPath"]]) {
self.cacheDirectoryPath = options[@"cacheDirectoryPath"];
}

Expand Down
2 changes: 2 additions & 0 deletions Sources/Sentry/SentrySDK.m
Original file line number Diff line number Diff line change
Expand Up @@ -204,6 +204,8 @@ + (void)startWithOptions:(SentryOptions *)options
startOption = options;
[SentryLog configure:options.debug diagnosticLevel:options.diagnosticLevel];

[SentryOptionsValidator validateWithOptions:options];

// We accept the tradeoff that the SDK might not be fully initialized directly after
// initializing it on a background thread because scheduling the init synchronously on the main
// thread could lead to deadlocks.
Expand Down
37 changes: 37 additions & 0 deletions Sources/Swift/SentryOptionsValidator.swift
Original file line number Diff line number Diff line change
@@ -0,0 +1,37 @@
@objcMembers
class SentryOptionsValidator: NSObject {
@objc
static func validate(options: Options) {
SentryLog.debug("Validating Sentry SDK options")
if !Self.isCacheDirectoryPathValid(path: options.cacheDirectoryPath) {
SentryLog.fatal("The configured cache directory path looks invalid, the SDK might not be able to write reports to disk: \(options.cacheDirectoryPath)")
}
}

@objc
static func isCacheDirectoryPathValid(path: Any) -> Bool {
guard let path = path as? String else {
return false
}
let fileUrl = URL(fileURLWithPath: path)

// The cache directory path is used as a base path in the SDK,
// therefore actual paths can be even longer and it is necessary
// to include some reserved space for appendix.
//
// The following length is assumed based on paths used in the SDK.
let reservedInternalLength: Int32 = 256

// PATH_MAX is defined in <sys/syslimits.h> and is the maximum length of a path in bytes.
if path.count > PATH_MAX - reservedInternalLength {
return false
}

// NAME_MAX is defined in <sys/syslimits.h> and is the maximum length of a filename / path component in bytes.
if fileUrl.pathComponents.contains(where: { component in component.count > NAME_MAX }) {
return false
}

return true
}
}
17 changes: 17 additions & 0 deletions Tests/SentryTests/SentryOptionsTest.m
Original file line number Diff line number Diff line change
Expand Up @@ -271,6 +271,23 @@ - (void)testCacheDirectoryPath
XCTAssertEqualObjects(options3.cacheDirectoryPath, [self getDefaultCacheDirectoryPath]);
}

- (void)testCacheDirectoryPath_invalidPathLength_shouldNotBeUsed
{
// -- Arrange --
NSString *invalidPathSegment = [@"A" stringByPaddingToLength:256
withString:@"a"
startingAtIndex:0];
NSString *pathWithInvalidSegment = [[@"/path/to/invalid/segment/"
stringByAppendingPathComponent:invalidPathSegment] stringByAppendingPathComponent:@"file"];

// -- Act --
SentryOptions *options =
[self getValidOptions:@{ @"cacheDirectoryPath" : pathWithInvalidSegment }];

// -- Assert --
XCTAssertEqualObjects(options.cacheDirectoryPath, [self getDefaultCacheDirectoryPath]);
}

- (NSString *)getDefaultCacheDirectoryPath
{
return NSSearchPathForDirectoriesInDomains(NSCachesDirectory, NSUserDomainMask, YES)
Expand Down
Loading
Loading