More aggressively mask user code errors when masking enabled #27183

benpankow · 2025-01-16T23:21:29Z

Summary

Follow-on to the functionality introduced in #18688. The original PR ensures that if the DAGSTER_REDACT_USER_CODE_ERRORS env var is set to 1, instances of DagsterUserCodeExecutionError which are serialized are masked so that their potentially sensitive contents are not leaked. However, there is a class of errors that can still expose elements of user code: system/framework errors or interrupts, for example, will include their stack trace and snippets of user code.

This PR attempts to handle this second class of errors, in a few ways:

Any error which is raised in a user code context has its stacktrace logged and then cleared, so if the error makes it to serialization, it will only include the error message.
User code exceptions have their original_exc_info purged.
A global mapping stores the error, so that a unique error ID can be raised in the UI, which can be used to locate the original error in logs.

Test Plan

New unit tests which specifically handle the intterrupt/system error cases.

benpankow · 2025-01-16T23:21:49Z

More aggressively mask user code errors when masking enabled #27183 👈 (View in Graphite)
master

This stack of pull requests is managed by Graphite. Learn more about stacking.

alangenfeld

I think a summary is warranted for this PR given its complexity

alangenfeld · 2025-01-17T20:03:50Z

python_modules/dagster/dagster/_utils/error.py

+error_id_by_exception: ContextVar[Mapping[int, str]] = ContextVar(
+    "error_id_by_exception", default={}
+)
+
+
+@contextlib.contextmanager
+def redact_user_stacktrace_if_enabled():
+    """Context manager which, if a user has enabled redacting user code errors, logs exceptions raised from within,
+    and clears the stacktrace from the exception. It also marks the exception to be redacted if it was to be persisted
+    or otherwise serialized to be sent to Dagster Plus. This is useful for preventing sensitive information from
+    being leaked in error messages.
+    """
+    if not _should_redact_user_code_error():
+        yield
+    else:
+        try:
+            yield
+        except BaseException as e:
+            exc_info = sys.exc_info()
+
+            # Generate a unique error ID for this error, or re-use an existing one
+            # if this error has already been seen
+            existing_error_id = error_id_by_exception.get().get(id(e))
+
+            if not existing_error_id:
+                error_id = str(uuid.uuid4())
+
+                # Track the error ID for this exception so we can redact it later
+                error_id_by_exception.set({**error_id_by_exception.get(), id(e): error_id})


I think the way you are using the context var here is equivalent to just having a process global dict. What exactly is the intention here and do any of the existing tests validate that?

i believe the goal is to increase the set of cases that this logic can handle (all kinds of exceptions besides DagsterUserCodeExecutionError can be emitted within user code, like KeyboardInterrupt or SystemExit or other DagsterError subclasses) while still only triggering the redaction if the exception was actually raised within a op_execution_error_boundary or user_code_error_boundary. I don't have a strong opinion about global dict vs. contextvar

Dropped contextvar - as Daniel mentioned the main goal is to track which exceptions come out of a user code context, and to link them to a user-visible ID so we can render that in the UI when the redacted error is shown

gibsondan

I'll be out next week so may lean on you alex to do the final accept once you have full context, but this broadly makes sense to me

gibsondan · 2025-01-18T05:17:41Z

python_modules/dagster/dagster/_utils/error.py

+error_id_by_exception: ContextVar[Mapping[int, str]] = ContextVar(
+    "error_id_by_exception", default={}
+)


the name here should probably have 'redacted' in it: redacted_user_code_error_id_by_exception?

gibsondan · 2025-01-18T05:21:18Z

python_modules/dagster/dagster/_utils/error.py

+error_id_by_exception: ContextVar[Mapping[int, str]] = ContextVar(
+    "error_id_by_exception", default={}
+)
+
+
+@contextlib.contextmanager
+def redact_user_stacktrace_if_enabled():
+    """Context manager which, if a user has enabled redacting user code errors, logs exceptions raised from within,
+    and clears the stacktrace from the exception. It also marks the exception to be redacted if it was to be persisted
+    or otherwise serialized to be sent to Dagster Plus. This is useful for preventing sensitive information from
+    being leaked in error messages.
+    """
+    if not _should_redact_user_code_error():
+        yield
+    else:
+        try:
+            yield
+        except BaseException as e:
+            exc_info = sys.exc_info()
+
+            # Generate a unique error ID for this error, or re-use an existing one
+            # if this error has already been seen
+            existing_error_id = error_id_by_exception.get().get(id(e))
+
+            if not existing_error_id:
+                error_id = str(uuid.uuid4())
+
+                # Track the error ID for this exception so we can redact it later
+                error_id_by_exception.set({**error_id_by_exception.get(), id(e): error_id})


i believe the goal is to increase the set of cases that this logic can handle (all kinds of exceptions besides DagsterUserCodeExecutionError can be emitted within user code, like KeyboardInterrupt or SystemExit or other DagsterError subclasses) while still only triggering the redaction if the exception was actually raised within a op_execution_error_boundary or user_code_error_boundary. I don't have a strong opinion about global dict vs. contextvar

python_modules/dagster/dagster/_utils/error.py

alangenfeld

to your queue to dial this in for final review

python_modules/dagster/dagster/_utils/error.py

benpankow · 2025-01-21T19:31:52Z

Summary added + lots of comments dropped into code

queue

alangenfeld · 2025-01-21T21:48:13Z

python_modules/dagster/dagster/_utils/error.py

@@ -94,6 +97,106 @@ def _should_redact_user_code_error() -> bool:
 )


+redacted_exception_id_to_user_facing_identifier: dict[int, str] = {}


https://docs.python.org/3/library/weakref.html#weakref.WeakKeyDictionary i think might be better

alangenfeld · 2025-01-21T22:18:47Z

python_modules/dagster/dagster_tests/core_tests/test_mask_user_code_errors.py

+        ("Failure", True, lambda: Failure("asdf")),
+    ],
+)
+def test_masking_op_execution(


does this test verify the error id stuff is working?

Added explicit check that the error ID shows up in the right spots

## Summary Follow-on to the functionality introduced in #18688. The original PR ensures that if the `DAGSTER_REDACT_USER_CODE_ERRORS` env var is set to `1`, instances of `DagsterUserCodeExecutionError` which are serialized are masked so that their potentially sensitive contents are not leaked. However, there is a class of errors that can still expose elements of user code: system/framework errors or interrupts, for example, will include their stack trace and snippets of user code. This PR attempts to handle this second class of errors, in a few ways: - Any error which is raised in a user code context has its stacktrace logged and then cleared, so if the error makes it to serialization, it will only include the error message. - User code exceptions have their `original_exc_info` purged. - A global mapping stores the error, so that a unique error ID can be raised in the UI, which can be used to locate the original error in logs. ## Test Plan New unit tests which specifically handle the intterrupt/system error cases.

benpankow requested a review from gibsondan January 16, 2025 23:39

benpankow changed the title ~~aggressively mask user code errors~~ More aggressively mask user code errors when masking enabled Jan 17, 2025

benpankow marked this pull request as ready for review January 17, 2025 19:48

benpankow force-pushed the benpankow/user-code-errors-mask-aggressively branch from 333ee23 to c6aef4f Compare January 17, 2025 19:49

gibsondan requested a review from alangenfeld January 17, 2025 19:50

alangenfeld reviewed Jan 17, 2025

View reviewed changes

gibsondan reviewed Jan 18, 2025

View reviewed changes

alangenfeld previously requested changes Jan 21, 2025

View reviewed changes

benpankow added 9 commits January 21, 2025 11:25

aggressively mask user code errors

6eb3324

tests

01b87e1

more tests

74b1979

update

66776f2

special case exception types

3f603bc

errid

b0de1e0

comments

8795f59

more docs

57542f7

rename

ff77f7b

benpankow force-pushed the benpankow/user-code-errors-mask-aggressively branch from c6aef4f to ff77f7b Compare January 21, 2025 19:26

graphite-app bot reviewed Jan 21, 2025

View reviewed changes

python_modules/dagster/dagster/_utils/error.py Outdated Show resolved Hide resolved

benpankow requested review from alangenfeld and gibsondan January 21, 2025 19:31

gi0;95;0ct pusmore aggressive masking

7405b58

alangenfeld reviewed Jan 21, 2025

View reviewed changes

WeakKeyDictionary

e25449c

alangenfeld approved these changes Jan 21, 2025

View reviewed changes

alangenfeld reviewed Jan 21, 2025

View reviewed changes

use setattr instead

0ea464a

explicitly test uuid matching

1a51d90

benpankow merged commit 1e84f70 into master Jan 21, 2025
4 of 5 checks passed

benpankow deleted the benpankow/user-code-errors-mask-aggressively branch January 21, 2025 23:26

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

More aggressively mask user code errors when masking enabled #27183

More aggressively mask user code errors when masking enabled #27183

benpankow commented Jan 16, 2025 •

edited

Loading

benpankow commented Jan 16, 2025

alangenfeld left a comment

alangenfeld Jan 17, 2025

gibsondan Jan 18, 2025

benpankow Jan 21, 2025

gibsondan left a comment

gibsondan Jan 18, 2025

gibsondan Jan 18, 2025

alangenfeld left a comment

benpankow commented Jan 21, 2025

alangenfeld Jan 21, 2025

alangenfeld Jan 21, 2025

benpankow Jan 21, 2025

		@@ -94,6 +97,106 @@ def _should_redact_user_code_error() -> bool:
		)


		redacted_exception_id_to_user_facing_identifier: dict[int, str] = {}

More aggressively mask user code errors when masking enabled #27183

More aggressively mask user code errors when masking enabled #27183

Conversation

benpankow commented Jan 16, 2025 • edited Loading

Summary

Test Plan

benpankow commented Jan 16, 2025

alangenfeld left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

gibsondan left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

alangenfeld left a comment

Choose a reason for hiding this comment

benpankow commented Jan 21, 2025

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

benpankow commented Jan 16, 2025 •

edited

Loading