Skip to content

Commit

Permalink
feat(sdk-crash-detection): Exception allow list (#51028)
Browse files Browse the repository at this point in the history 
Define an allow list for the exception only to keep known properties not
containing PII. This PR is based on
#51022.

Fixes GH-50710
  • Loading branch information
@philipphofmann
philipphofmann authored Jun 29, 2023
1 parent 497c841 commit 6620a40
Show file tree
Hide file tree
Showing 5 changed files with 177 additions and 12 deletions.
50 changes: 42 additions & 8 deletions fixtures/sdk_crash_detection/crash_event.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -30,7 +30,7 @@ def get_frames(function: str, sentry_frame_in_app: bool = False) -> Sequence[Map
{
"function": "LoginViewController.viewDidAppear",
"symbol": "$s8Sentry9LoginViewControllerC13viewDidAppearyySbF",
"package": "SentryApp",
"package": "/private/var/containers/Bundle/Application/D9118D4F-E47F-47D3-96A2-35E854245CB4/iOS-Swift.app/iOS-Swift",
"in_app": True,
"filename": "LoginViewController.swift",
"image_addr": "0x100260000",
Expand All @@ -39,38 +39,54 @@ def get_frames(function: str, sentry_frame_in_app: bool = False) -> Sequence[Map
{
"function": "-[UIViewController _setViewAppearState:isAnimating:]",
"symbol": "-[UIViewController _setViewAppearState:isAnimating:]",
"package": "UIKitCore",
"package": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
{
"function": "-[UIViewController __viewDidAppear:]",
"symbol": "-[UIViewController __viewDidAppear:]",
"package": "UIKitCore",
"package": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
{
"function": "-[UIViewController _endAppearanceTransition:]",
"symbol": "-[UIViewController _endAppearanceTransition:]",
"package": "UIKitCore",
"package": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
{
"function": "-[UINavigationController navigationTransitionView:didEndTransition:fromView:toView:]",
"symbol": "-[UINavigationController navigationTransitionView:didEndTransition:fromView:toView:]",
"package": "UIKitCore",
"package": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
{
"function": "__49-[UINavigationController _startCustomTransition:]_block_invoke",
"symbol": "__49-[UINavigationController _startCustomTransition:]_block_invoke",
"package": "UIKitCore",
"package": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
{
"filename": "EventStripperTestFrame.swift",
"function": "function",
"raw_function": "raw_function",
"module": "module",
"abs_path": "abs_path",
"in_app": False,
"instruction_addr": "0x1a4e8f000",
"addr_mode": "0x1a4e8f000",
"symbol": "symbol",
"symbol_addr": "0x1a4e8f000",
"image_addr": "0x1a4e8f000",
"package": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore",
"platform": "platform",
"post_context": ["should_be_removed"],
},
]

# The frames have to be ordered from caller to callee, or oldest to youngest.
Expand Down Expand Up @@ -99,8 +115,26 @@ def get_crash_event_with_frames(
"stacktrace": {
"frames": frames,
},
"type": "SIGABRT",
"mechanism": {"handled": handled},
"type": "EXC_BAD_ACCESS",
"value": "crash > crash: > objectAtIndex: >\nAttempted to dereference null pointer.",
"mechanism": {
"handled": handled,
"type": "mach",
"meta": {
"signal": {
"number": 11,
"code": 0,
"name": "SIGSEGV",
"code_name": "SEGV_NOOP",
},
"mach_exception": {
"exception": 1,
"code": 1,
"subcode": 0,
"name": "EXC_BAD_ACCESS",
},
},
},
}
]
},
Expand Down
46 changes: 45 additions & 1 deletion src/sentry/utils/sdk_crashes/event_stripper.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -37,7 +37,47 @@ def with_explanation(self, explanation: str) -> "Allow":
"version": Allow.SIMPLE_TYPE,
"integrations": Allow.NEVER.with_explanation("Users can add their own integrations."),
},
"exception": Allow.ALL.with_explanation("We strip the exception data separately."),
"exception": {
"values": {
"stacktrace": {
"frames": {
"filename": Allow.SIMPLE_TYPE,
"function": Allow.SIMPLE_TYPE,
"raw_function": Allow.SIMPLE_TYPE,
"module": Allow.SIMPLE_TYPE,
"abs_path": Allow.SIMPLE_TYPE,
"in_app": Allow.SIMPLE_TYPE,
"instruction_addr": Allow.SIMPLE_TYPE,
"addr_mode": Allow.SIMPLE_TYPE,
"symbol": Allow.SIMPLE_TYPE,
"symbol_addr": Allow.SIMPLE_TYPE,
"image_addr": Allow.SIMPLE_TYPE,
"package": Allow.SIMPLE_TYPE,
"platform": Allow.SIMPLE_TYPE,
}
},
"value": Allow.NEVER.with_explanation("The exception value could contain PII."),
"type": Allow.SIMPLE_TYPE,
"mechanism": {
"handled": Allow.SIMPLE_TYPE,
"type": Allow.SIMPLE_TYPE,
"meta": {
"signal": {
"number": Allow.SIMPLE_TYPE,
"code": Allow.SIMPLE_TYPE,
"name": Allow.SIMPLE_TYPE,
"code_name": Allow.SIMPLE_TYPE,
},
"mach_exception": {
"exception": Allow.SIMPLE_TYPE,
"code": Allow.SIMPLE_TYPE,
"subcode": Allow.SIMPLE_TYPE,
"name": Allow.SIMPLE_TYPE,
},
},
},
}
},
"debug_meta": Allow.ALL,
"contexts": {
"device": {
Expand Down Expand Up @@ -106,6 +146,10 @@ def _strip_event_data_with_allowlist(
stripped_data[data_key] = _strip_event_data_with_allowlist(
data_value, allowlist_for_data
)
elif isinstance(data_value, Sequence):
stripped_data[data_key] = [
_strip_event_data_with_allowlist(item, allowlist_for_data) for item in data_value
]

return stripped_data

Expand Down
2 changes: 1 addition & 1 deletion src/sentry/utils/sdk_crashes/sdk_crash_detection.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ def detect_sdk_crash(self, event: Event, event_project_id: int) -> Optional[Even
# Getting the frames and checking if the event is unhandled might different per platform.
# We will change this once we implement this for more platforms.
is_unhandled = (
get_path(event.data, "exception", "values", -1, "mechanism", "data", "handled") is False
get_path(event.data, "exception", "values", -1, "mechanism", "handled") is False
)
if is_unhandled is False:
return None
Expand Down
84 changes: 82 additions & 2 deletions tests/sentry/utils/sdk_crashes/test_event_stripper.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@
from sentry.testutils import TestCase
from sentry.testutils.cases import BaseTestCase
from sentry.testutils.silo import region_silo_test
from sentry.utils.safe import get_path
from sentry.utils.safe import get_path, set_path
from sentry.utils.sdk_crashes.cocoa_sdk_crash_detector import CocoaSDKCrashDetector
from sentry.utils.sdk_crashes.event_stripper import strip_event_data

Expand Down Expand Up @@ -128,6 +128,86 @@ def test_strip_event_data_keeps_simple_types(self):
assert stripped_event_data.get("timestamp") == 1
assert stripped_event_data.get("platform") == "cocoa"

def test_strip_event_data_keeps_simple_exception_properties(self):
event = self.create_event(
data=get_crash_event(),
project_id=self.project.id,
)

stripped_event_data = strip_event_data(event.data, CocoaSDKCrashDetector())

assert get_path(stripped_event_data, "exception", "values", 0, "type") == "EXC_BAD_ACCESS"
assert get_path(stripped_event_data, "exception", "values", 0, "value") is None

def test_strip_event_data_keeps_exception_mechanism(self):
event = self.create_event(
data=get_crash_event(),
project_id=self.project.id,
)

# set extra data that should be stripped
set_path(event.data, "exception", "values", 0, "mechanism", "foo", value="bar")
set_path(
event.data, "exception", "values", 0, "mechanism", "meta", "signal", "foo", value="bar"
)
set_path(
event.data,
"exception",
"values",
0,
"mechanism",
"meta",
"mach_exception",
"foo",
value="bar",
)

stripped_event_data = strip_event_data(event.data, CocoaSDKCrashDetector())

mechanism = get_path(stripped_event_data, "exception", "values", 0, "mechanism")

assert mechanism == {
"handled": False,
"type": "mach",
"meta": {
"signal": {"number": 11, "code": 0, "name": "SIGSEGV", "code_name": "SEGV_NOOP"},
"mach_exception": {
"exception": 1,
"code": 1,
"subcode": 0,
"name": "EXC_BAD_ACCESS",
},
},
}

def test_strip_event_data_keeps_exception_stacktrace(self):
event = self.create_event(
data=get_crash_event(),
project_id=self.project.id,
)

stripped_event_data = strip_event_data(event.data, CocoaSDKCrashDetector())

first_frame = get_path(
stripped_event_data, "exception", "values", 0, "stacktrace", "frames", 0
)

assert first_frame == {
"filename": "EventStripperTestFrame.swift",
"function": "function",
"raw_function": "raw_function",
"module": "module",
"abs_path": "abs_path",
"in_app": False,
"instruction_addr": "0x1a4e8f000",
"addr_mode": "0x1a4e8f000",
"symbol": "symbol",
"symbol_addr": "0x1a4e8f000",
"image_addr": "0x1a4e8f000",
"package": "/System/Library/PrivateFrameworks/UIKitCore.framework/UIKitCore",
"platform": "platform",
}

def test_strip_event_data_strips_non_referenced_dsyms(self):
event = self.create_event(
data=get_crash_event(),
Expand Down Expand Up @@ -164,7 +244,7 @@ def _execute_strip_frames_test(self, frames):
stripped_event_data, "exception", "values", -1, "stacktrace", "frames"
)

assert len(stripped_frames) == 6
assert len(stripped_frames) == 7
assert (
len(
[
Expand Down
7 changes: 7 additions & 0 deletions tests/sentry/utils/sdk_crashes/test_sdk_crash_detection.py
View file
Original file line number Diff line number Diff line change
Expand Up @@ -185,12 +185,14 @@ def _get_crash_event(self, filename) -> Dict[str, Collection[str]]:
"symbol": "__handleUncaughtException",
"package": "CoreFoundation",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
{
"function": "_objc_terminate",
"symbol": "_ZL15_objc_terminatev",
"package": "libobjc.A.dylib",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
{
"function": "CPPExceptionTerminate",
Expand All @@ -199,12 +201,14 @@ def _get_crash_event(self, filename) -> Dict[str, Collection[str]]:
"symbol": "_ZL21CPPExceptionTerminatev",
"package": "MainApp",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
{
"function": "std::__terminate",
"symbol": "_ZSt11__terminatePFvvE",
"package": "libc++abi.dylib",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
]
)
Expand Down Expand Up @@ -242,19 +246,22 @@ def test_frames_only_non_in_app_after_sentry_frame_is_reported(self, mock_sdk_cr
"symbol": "__handleUncaughtException",
"package": "CoreFoundation",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
{
"function": "_objc_terminate",
"symbol": "_ZL15_objc_terminatev",
"package": "libobjc.A.dylib",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
get_sentry_frame("sentrycrashdl_getBinaryImage"),
{
"function": "std::__terminate",
"symbol": "_ZSt11__terminatePFvvE",
"package": "libc++abi.dylib",
"in_app": False,
"image_addr": "0x1a4e8f000",
},
]
),
Expand Down

0 comments on commit 6620a40

Please sign in to comment.