Fix/sse output redaction#1665
Conversation
utksh1
left a comment
There was a problem hiding this comment.
Thanks for the security fix — the parser sandbox changes are solid work. However, there are several issues that need to be addressed before this can be merged:
1. CI is failing (5 test failures directly caused by this PR):
test_stderr_captured_in_error— expectsstderr_excerptattribute that was removedtest_stderr_excerpt_truncated_to_2000_chars— same issuetest_timeout_expired_raises_parser_sandbox_error— still expects_BOOTSTRAP_TEMPLATE.format()test_timeout_error_includes_stderr— asserts stderr in exception message (which you correctly removed)test_sandbox_exec_fails_on_exec_statement—stderr_excerptaccess
You need to update these existing tests to assert the new secure behavior (no stderr in messages, template uses safe_substitute).
2. Unrelated commit should be separated:
The second commit (parser tests for subdomain_discovery plugin, referencing #1436) is completely unrelated to SSE output redaction. Please either:
- (a) Remove that commit from this PR and submit it as a separate PR, OR
- (b) Split this into two PRs
3. No tests for the actual redaction changes:
There should be tests verifying:
redact()is called on error messages inroutes.pyandexecutor.py_sanitize_stderr()behavior (paths stripped, line numbers stripped)- The
string.Template+safe_substitute()behavior
4. PR title is misleading:
"SSE output redaction" implies SSE streaming code was modified, but no SSE endpoints were touched. The actual changes are about parser sandbox stderr leakage and error message redaction. Please update the title to reflect reality (e.g., fix: prevent parser sandbox stderr leakage in error messages).
5. Fill out the PR description:
For a security fix, there should be a clear description of the vulnerability, the threat model, and how the fix mitigates it.
The core security changes in parser_sandbox.py are well-designed and should be merged — just needs the above cleanup.
Description
Related Issues
Type of Change
How Has This Been Tested?
Checklist