More aggressively mask user code errors when masking enabled #27183
Conversation
benpankow
changed the title
benpankow
force-pushed
the
benpankow/user-code-errors-mask-aggressively
branch
from
333ee23 to
c6aef4f
Compare
| 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}) |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
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
| error_id_by_exception: ContextVar[Mapping[int, str]] = ContextVar( | ||
| "error_id_by_exception", default={} | ||
| ) |
There was a problem hiding this comment.
the name here should probably have 'redacted' in it: redacted_user_code_error_id_by_exception?
| 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}) |
There was a problem hiding this comment.
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
| if isinstance(e, DagsterUserCodeExecutionError): | ||
| return SerializableErrorInfo( | ||
| message=( | ||
| f"Error occurred during user code execution, error ID {err_id}. " | ||
| "The error has been masked to prevent leaking sensitive information. " | ||
| "Search in logs for this error ID for more details." | ||
| ), | ||
| stack=[], | ||
| cls_name="DagsterRedactedUserCodeError", | ||
| cause=None, | ||
| context=None, | ||
| ) | ||
| else: | ||
| tb_exc = traceback.TracebackException(exc_type, e, tb) |
There was a problem hiding this comment.
maybe worth explaining the difference between these two cases - with user code errors, you don't even want to show the message - but with other errors (framework errors or interrupts or Failure / RetryRequested raised within the error boundary), the message is not sensitive and can be displayed for clarity, but the traceback is.
Summary
Test Plan
More unit tests.