Skip to content

Commit

Permalink
fix: Use options.reportAccessibilityIdentifier for Breadcrumbs and …
Browse files Browse the repository at this point in the history
…UIEvents (#4569)

We added the new options reportAccessibilityIdentifier but did not use it in other places.
accessibilityIdentifier may contain PII also in this other places.
  • Loading branch information
brustolin authored Nov 27, 2024
1 parent 319f0f5 commit b07998b
Show file tree
Hide file tree
Showing 13 changed files with 160 additions and 34 deletions.
1 change: 1 addition & 0 deletions CHANGELOG.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@
### Fixes

- Session replay touch tracking race condition (#4548)
- Use `options.reportAccessibilityIdentifier` for Breadcrumbs and UIEvents (#4569)
- Session replay transformed view masking (#4529)
- Load integration from same binary (#4541)

Expand Down
3 changes: 2 additions & 1 deletion Samples/iOS-Swift/iOS-Swift/Tools/SentryExposure.h
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@

@interface SentryBreadcrumbTracker : NSObject

+ (NSDictionary *)extractDataFromView:(UIView *)view;
+ (NSDictionary *)extractDataFromView:(UIView *)view
withAccessibilityIdentifier:(BOOL)includeIdentifier;

@end
Original file line number Diff line number Diff line change
Expand Up @@ -14,8 +14,8 @@ class InfoForBreadcrumbController: UIViewController {

@IBAction func buttonPressed(_ sender: Any) {
guard let view = self.view,
let viewInfo = SentryBreadcrumbTracker.extractData(from: view),
let buttonInfo = SentryBreadcrumbTracker.extractData(from: button)
let viewInfo = SentryBreadcrumbTracker.extractData(from: view, withAccessibilityIdentifier: true),
let buttonInfo = SentryBreadcrumbTracker.extractData(from: button, withAccessibilityIdentifier: true)
else {
label?.text = "ERROR"
return
Expand Down
12 changes: 8 additions & 4 deletions Sources/Sentry/SentryAutoBreadcrumbTrackingIntegration.m
Original file line number Diff line number Diff line change
Expand Up @@ -27,16 +27,20 @@ - (BOOL)installWithOptions:(SentryOptions *)options
return NO;
}

[self installWithOptions:options
breadcrumbTracker:[[SentryBreadcrumbTracker alloc] init]
#if TARGET_OS_IOS && SENTRY_HAS_UIKIT
[self installWithOptions:options
breadcrumbTracker:[[SentryBreadcrumbTracker alloc] initReportAccessibilityIdentifier:
options.reportAccessibilityIdentifier]
systemEventBreadcrumbs:
[[SentrySystemEventBreadcrumbs alloc]
initWithFileManager:[SentryDependencyContainer sharedInstance].fileManager
andNotificationCenterWrapper:[SentryDependencyContainer sharedInstance]
.notificationCenterWrapper]
.notificationCenterWrapper]];
#else
[self installWithOptions:options
breadcrumbTracker:[[SentryBreadcrumbTracker alloc]
initReportAccessibilityIdentifier:false]];
#endif // TARGET_OS_IOS && SENTRY_HAS_UIKIT
];

return YES;
}
Expand Down
20 changes: 17 additions & 3 deletions Sources/Sentry/SentryBreadcrumbTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -34,7 +34,9 @@ @interface SentryBreadcrumbTracker ()

@end

@implementation SentryBreadcrumbTracker
@implementation SentryBreadcrumbTracker {
BOOL _reportAccessibilityIdentifier;
}

#if SENTRY_HAS_REACHABILITY
- (void)dealloc
Expand All @@ -43,6 +45,14 @@ - (void)dealloc
}
#endif // !TARGET_OS_WATCH

- (instancetype)initReportAccessibilityIdentifier:(BOOL)report
{
if (self = [super init]) {
_reportAccessibilityIdentifier = report;
}
return self;
}

- (void)startWithDelegate:(id<SentryBreadcrumbDelegate>)delegate
{
_delegate = delegate;
Expand Down Expand Up @@ -211,7 +221,9 @@ - (void)swizzleSendAction
NSDictionary *data = nil;
for (UITouch *touch in event.allTouches) {
if (touch.phase == UITouchPhaseCancelled || touch.phase == UITouchPhaseEnded) {
data = [SentryBreadcrumbTracker extractDataFromView:touch.view];
data = [SentryBreadcrumbTracker
extractDataFromView:touch.view
withAccessibilityIdentifier:self->_reportAccessibilityIdentifier];
}
}

Expand Down Expand Up @@ -264,6 +276,7 @@ - (void)swizzleViewDidAppear
}

+ (NSDictionary *)extractDataFromView:(UIView *)view
withAccessibilityIdentifier:(BOOL)includeIdentifier
{
NSMutableDictionary *result =
@{ @"view" : [NSString stringWithFormat:@"%@", view] }.mutableCopy;
Expand All @@ -272,7 +285,8 @@ + (NSDictionary *)extractDataFromView:(UIView *)view
[result setValue:[NSNumber numberWithInteger:view.tag] forKey:@"tag"];
}

if (view.accessibilityIdentifier && ![view.accessibilityIdentifier isEqualToString:@""]) {
if (includeIdentifier && view.accessibilityIdentifier
&& ![view.accessibilityIdentifier isEqualToString:@""]) {
[result setValue:view.accessibilityIdentifier forKey:@"accessibilityIdentifier"];
}

Expand Down
8 changes: 6 additions & 2 deletions Sources/Sentry/SentryUIEventTracker.m
Original file line number Diff line number Diff line change
Expand Up @@ -19,12 +19,16 @@ @interface SentryUIEventTracker ()

@end

@implementation SentryUIEventTracker
@implementation SentryUIEventTracker {
BOOL _reportAccessibilityIdentifier;
}

- (instancetype)initWithMode:(id<SentryUIEventTrackerMode>)mode
reportAccessibilityIdentifier:(BOOL)report
{
if (self = [super init]) {
self.uiEventTrackerMode = mode;
_reportAccessibilityIdentifier = report;
}
return self;
}
Expand Down Expand Up @@ -73,7 +77,7 @@ - (void)sendActionCallback:(NSString *)action
NSString *operation = [self getOperation:sender];

NSString *accessibilityIdentifier = nil;
if ([[sender class] isSubclassOfClass:[UIView class]]) {
if (_reportAccessibilityIdentifier && [[sender class] isSubclassOfClass:[UIView class]]) {
UIView *view = sender;
accessibilityIdentifier = view.accessibilityIdentifier;
}
Expand Down
2 changes: 1 addition & 1 deletion Sources/Sentry/SentryUIEventTrackerTransactionMode.m
Original file line number Diff line number Diff line change
Expand Up @@ -36,7 +36,7 @@ - (instancetype)initWithIdleTimeout:(NSTimeInterval)idleTimeout

- (void)handleUIEvent:(NSString *)action
operation:(NSString *)operation
accessibilityIdentifier:(NSString *)accessibilityIdentifier
accessibilityIdentifier:(nullable NSString *)accessibilityIdentifier
{

// There might be more active transactions stored, but only the last one might still be
Expand Down
4 changes: 3 additions & 1 deletion Sources/Sentry/SentryUIEventTrackingIntegration.m
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,9 @@ - (BOOL)installWithOptions:(SentryOptions *)options
SentryUIEventTrackerTransactionMode *mode =
[[SentryUIEventTrackerTransactionMode alloc] initWithIdleTimeout:options.idleTimeout];

self.uiEventTracker = [[SentryUIEventTracker alloc] initWithMode:mode];
self.uiEventTracker =
[[SentryUIEventTracker alloc] initWithMode:mode
reportAccessibilityIdentifier:options.reportAccessibilityIdentifier];

[self.uiEventTracker start];

Expand Down
4 changes: 4 additions & 0 deletions Sources/Sentry/include/SentryBreadcrumbTracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,10 @@ NS_ASSUME_NONNULL_BEGIN

@interface SentryBreadcrumbTracker : NSObject

SENTRY_NO_INIT

- (instancetype)initReportAccessibilityIdentifier:(BOOL)report;

- (void)startWithDelegate:(id<SentryBreadcrumbDelegate>)delegate;
#if SENTRY_HAS_UIKIT
- (void)startSwizzle;
Expand Down
3 changes: 2 additions & 1 deletion Sources/Sentry/include/SentryUIEventTracker.h
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,8 @@ NS_ASSUME_NONNULL_BEGIN
@interface SentryUIEventTracker : NSObject
SENTRY_NO_INIT

- (instancetype)initWithMode:(id<SentryUIEventTrackerMode>)mode;
- (instancetype)initWithMode:(id<SentryUIEventTrackerMode>)mode
reportAccessibilityIdentifier:(BOOL)report;

- (void)start;
- (void)stop;
Expand Down
2 changes: 1 addition & 1 deletion Sources/Sentry/include/SentryUIEventTrackerMode.h
Original file line number Diff line number Diff line change
Expand Up @@ -8,7 +8,7 @@ NS_ASSUME_NONNULL_BEGIN

- (void)handleUIEvent:(NSString *)action
operation:(NSString *)operation
accessibilityIdentifier:(NSString *)accessibilityIdentifier;
accessibilityIdentifier:(nullable NSString *)accessibilityIdentifier;

@end

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,12 @@
import SentryTestUtils
import XCTest

extension SentryBreadcrumbTracker {
override convenience init() {
self.init(reportAccessibilityIdentifier: true)
}
}

class SentryBreadcrumbTrackerTests: XCTestCase {

private var delegate: SentryBreadcrumbTestDelegate!
Expand All @@ -21,15 +27,15 @@ class SentryBreadcrumbTrackerTests: XCTestCase {

func testStopRemovesSwizzleSendAction() {
let sut = SentryBreadcrumbTracker()

sut.start(with: delegate)
sut.startSwizzle()
sut.stop()

let dict = Dynamic(SentryDependencyContainer.sharedInstance().swizzleWrapper).swizzleSendActionCallbacks().asDictionary
XCTAssertEqual(0, dict?.count)
}

func testNetworkConnectivityChangeBreadcrumbs() throws {
let testReachability = TestSentryReachability()

Expand All @@ -38,11 +44,11 @@ class SentryBreadcrumbTrackerTests: XCTestCase {
let sut = SentryBreadcrumbTracker()
sut.start(with: delegate)
let states = [SentryConnectivityCellular,
SentryConnectivityWiFi,
SentryConnectivityNone,
SentryConnectivityWiFi,
SentryConnectivityCellular,
SentryConnectivityWiFi
SentryConnectivityWiFi,
SentryConnectivityNone,
SentryConnectivityWiFi,
SentryConnectivityCellular,
SentryConnectivityWiFi
]
states.forEach {
testReachability.setReachabilityState(state: $0)
Expand All @@ -64,7 +70,7 @@ class SentryBreadcrumbTrackerTests: XCTestCase {
sut.start(with: delegate)
testReachability.setReachabilityState(state: SentryConnectivityCellular)
sut.stop()

guard let breadcrumb = delegate.addCrumbInvocations.invocations.dropFirst().first else {
XCTFail("No connectivity breadcrumb")
return
Expand All @@ -80,7 +86,7 @@ class SentryBreadcrumbTrackerTests: XCTestCase {
XCTAssertEqual(payload["category"] as? String, "device.connectivity")
XCTAssertEqual(payloadData["state"] as? String, "cellular")
}

func testSwizzlingStarted_ViewControllerAppears_AddsUILifeCycleBreadcrumb() throws {
let testReachability = TestSentryReachability()

Expand All @@ -99,7 +105,7 @@ class SentryBreadcrumbTrackerTests: XCTestCase {
let sut = SentryBreadcrumbTracker()
sut.start(with: delegate)
sut.startSwizzle()

// Using UINavigationController as a parent doesn't work on tvOS 17.0
// for an unknown reason. Therefore, we manually set the parent.
class ParentUIViewController: UIViewController {
Expand All @@ -113,15 +119,15 @@ class SentryBreadcrumbTrackerTests: XCTestCase {
print("delegate: \(String(describing: delegate))")
print("tracker: \(sut); SentryBreadcrumbTracker.delegate: \(String(describing: Dynamic(sut).delegate.asObject))")
viewController.viewDidAppear(false)

let crumbs = delegate.addCrumbInvocations.invocations

// one breadcrumb for starting the tracker, one for the first reachability breadcrumb and one final one for the swizzled viewDidAppear
guard crumbs.count == 2 else {
XCTFail("Expected exactly 2 breadcrumbs, got: \(crumbs)")
return
}

let lifeCycleCrumb = try XCTUnwrap(crumbs.element(at: 1))
XCTAssertEqual("navigation", lifeCycleCrumb.type)
XCTAssertEqual("ui.lifecycle", lifeCycleCrumb.category)
Expand Down Expand Up @@ -221,7 +227,7 @@ class SentryBreadcrumbTrackerTests: XCTestCase {
XCTFail("No touch breadcrumb")
return
}

let result = try XCTUnwrap(sut.convert(from: crumb) as? SentryRRWebBreadcrumbEvent)
let crumbData = try XCTUnwrap(result.data)
let payload = try XCTUnwrap(crumbData["payload"] as? [String: Any])
Expand All @@ -230,6 +236,78 @@ class SentryBreadcrumbTrackerTests: XCTestCase {
XCTAssertEqual(payload["message"] as? String, "methodPressed:")
}

func testTouchBreadcrumb_DontReportAccessibilityIdentifier() throws {
let scope = Scope()
let client = TestClient(options: Options())
let hub = TestHub(client: client, andScope: scope)
SentrySDK.setCurrentHub(hub)

let swizzlingWrapper = TestSentrySwizzleWrapper()
SentryDependencyContainer.sharedInstance().swizzleWrapper = swizzlingWrapper

let tracker = SentryBreadcrumbTracker(reportAccessibilityIdentifier: false)
tracker.start(with: delegate)
tracker.startSwizzle()

let button = UIButton()
button.accessibilityIdentifier = "TestAccessibilityIdentifier"

swizzlingWrapper.execute(action: "methodPressed:", target: self, sender: button, event: TestEvent(touchedView: button))

guard let crumb = delegate.addCrumbInvocations.invocations.first(where: { $0.category == "touch" }) else {
XCTFail("No touch breadcrumb")
return
}

let crumbData = try XCTUnwrap(crumb.data)

XCTAssertNil(crumbData["accessibilityIdentifier"])
}

func testTouchBreadcrumb_ReportAccessibilityIdentifier() throws {
let scope = Scope()
let client = TestClient(options: Options())
let hub = TestHub(client: client, andScope: scope)
SentrySDK.setCurrentHub(hub)

let swizzlingWrapper = TestSentrySwizzleWrapper()
SentryDependencyContainer.sharedInstance().swizzleWrapper = swizzlingWrapper

let tracker = SentryBreadcrumbTracker(reportAccessibilityIdentifier: true)
tracker.start(with: delegate)
tracker.startSwizzle()

let button = UIButton()
button.accessibilityIdentifier = "TestAccessibilityIdentifier"

swizzlingWrapper.execute(action: "methodPressed:", target: self, sender: button, event: TestEvent(touchedView: button))

let crumb = try XCTUnwrap(delegate.addCrumbInvocations.invocations.first(where: { $0.category == "touch" }))
let crumbData = try XCTUnwrap(crumb.data)

XCTAssertEqual(crumbData["accessibilityIdentifier"] as? String, "TestAccessibilityIdentifier")
}

private class TestEvent: UIEvent {
let touchedView: UIView?
class TestEndTouch: UITouch {
let touchedView: UIView?
init(touchedView: UIView?) {
self.touchedView = touchedView
}
override var phase: UITouch.Phase { .ended }
override var view: UIView? { touchedView }
}

init(touchedView: UIView?) {
self.touchedView = touchedView
}

override var allTouches: Set<UITouch> { [
TestEndTouch(touchedView: touchedView)
] }
}

#endif

}
Loading

0 comments on commit b07998b

Please sign in to comment.