WIP: add security event evidence and redaction - #8574
WIP: add security event evidence and redaction#8574Bill Schnurr (bschnurr) wants to merge 3 commits into
Conversation
|
Azure Pipelines: There may be pipelines that require an authorized user to comment /azp run to run. |
- Use managed recursive delete with retries in FileUtils - Fallback to registry search for interpreter discovery in tests - Add async WaitAsync for process exit with timeout/cancel - Always set env vars in REPL; log spurious ERRE/DONE as warnings - Limit Python 2.x filtering to PythonCore entries - Clean up orphan registry keys in environment list tests - Make Django UI test assertions more robust to output changes - Update urls.py for Django 2.x+ compatibility with fallback
|
🔒 Automated review in progress — Rich Chiodo (@rchiodo) is auto-reviewing this PR. |
|
| @"(?ix)(\b(?:password|passwd|pwd|token|access[_-]?token|refresh[_-]?token|secret|client[_-]?secret|api[_-]?key|subscription[_-]?key|credential|authorization|connection\s*string|connectionstring|sharedaccesssignature|sig|key)\b\s*[:=]\s*)(['""']?)([^'"";\s,&]+)(['""']?)", | ||
| RegexOptions.CultureInvariant | ||
| ); | ||
|
|
There was a problem hiding this comment.
📍 Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs:35
Verified security bypass (Skeptic executed the byte-identical Python twin; Advocate conceded). The SecretKeyValue pattern requires the keyword to be immediately followed by :/=, so quoted-key forms leak verbatim: {"password": "hunter2"}, {'api_key': 'plain-secret'}, and HTTP_AUTHORIZATION (the _ word char defeats \bAuthorization\b) all pass through unchanged. This is the dominant serialized-secret shape for the very sinks this control protects (WSGI environ, ex.ToString(), TaskDialog details), so the redactor silently leaks what it exists to stop. Allow an optional quote between key and separator (e.g. \b(?:...)\b['\"]?\s*[:=]\s*), match a leading quote on the key, and add regression cases for JSON, Python-dict-repr, and HTTP_* keys. The current tests only cover the bare key=/key: forms that already work, giving false confidence.
[verified]
| r"\b(password|passwd|pwd|token|access[_-]?token|refresh[_-]?token|secret|client[_-]?secret|" | ||
| r"api[_-]?key|subscription[_-]?key|credential|authorization|connection\s*string|connectionstring|" | ||
| r"sharedaccesssignature|sig|key)\b(\s*[:=]\s*)(['\"]?)([^'\";\s,&]+)(['\"]?)", | ||
| re.I |
There was a problem hiding this comment.
📍 Python/Product/WFastCgi/wfastcgi.py:341
Same verified quoted-key bypass as the C# redactor (regexes are character-identical). Because environ is literally a dict and FastCGI logs frequently carry JSON/dict-repr, _sanitize_log_text will pass {"password": "..."} / HTTP_AUTHORIZATION straight into WSGI_LOG and AppInsights track_event. Fix the shared spec (optional quote before the separator; match leading key quote) and add the same regression cases here.
[verified]
| } | ||
|
|
||
| /// <summary> | ||
| /// An exception that should not be silently handled and logged. |
There was a problem hiding this comment.
📍 Python/Product/Cookiecutter/Shared/Infrastructure/ExceptionExtensions.cs:62
Review-rule violation (reuse-core-utils): this private SensitiveDataRedactor is a character-for-character duplicate of Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs (Architect verified byte-identical regexes via read_file). The using System.Text.RegularExpressions; added only for this copy signals the Common dependency was avoided rather than wired in. This copy is internal and untested, so it will inevitably drift from the Common one — any pattern fix (e.g. the quoted-key bypass) now has to land in three places across two languages. Remove this class and reference Microsoft.PythonTools.Infrastructure.SensitiveDataRedactor (link it as shared source or add a project reference to Common). Per the shared-review-policy hard gate, this stays an issue until the rule itself is amended.
[verified]
| sanitized = SecretKeyValue.Replace(sanitized, match => | ||
| match.Groups[1].Value + match.Groups[2].Value + RedactedValue + match.Groups[4].Value | ||
| ); | ||
| return sanitized; |
There was a problem hiding this comment.
📍 Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs:44
Verified partial leak: the value class [^'\";\s,&]+ stops at the first whitespace/comma, so any secret containing a space, comma, ;, &, or quote leaks its tail. Confirmed: password=my secret phrase → password=<redacted> secret phrase, token=a,b,c → token=<redacted>,b,c, and a connection string Server=x;Password=p@ss w0rd;... leaks w0rd. Passphrases and comma-joined token lists are realistic. For quoted values, consume to the closing quote instead of stopping at whitespace/comma.
[verified]
| @"(?i)\b([a-z][a-z0-9+.-]*://)([^/\s:@]+(?::[^/\s@]*)?@)", | ||
| RegexOptions.CultureInvariant | ||
| ); | ||
|
|
There was a problem hiding this comment.
📍 Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs:30
Verified partial leak in UriUserInfo: the userinfo class [^/\s:@]*/[^/\s@]* stops at the first @, so a password containing @ (legal and common) partially leaks — https://user:p@ssword@host/path → https://<redacted>@ssword@host/path, leaving ssword@host. Consume up to the last @ before the authority instead.
[verified]
| @"(?ix)(\b(?:password|passwd|pwd|token|access[_-]?token|refresh[_-]?token|secret|client[_-]?secret|api[_-]?key|subscription[_-]?key|credential|authorization|connection\s*string|connectionstring|sharedaccesssignature|sig|key)\b\s*[:=]\s*)(['""']?)([^'"";\s,&]+)(['""']?)", | ||
| RegexOptions.CultureInvariant | ||
| ); | ||
|
|
There was a problem hiding this comment.
📍 Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs:35
Verified over-redaction: the bare keywords key and sig are generic enough to mangle ordinary diagnostic text — Parsing failed at key: value in config → key: <redacted>, and the design sig = v2 → sig = <redacted>. This erodes the diagnostic value the evidence file claims to preserve. Consider dropping bare key/sig or requiring a tighter context (e.g. api_key, signing_sig). Note the current test deliberately encodes sig redaction as desired, so decide the intended behavior explicitly.
[verified]
| match.Groups[1].Value + match.Groups[2].Value + RedactedValue + match.Groups[4].Value | ||
| ); | ||
| return sanitized; | ||
| } |
There was a problem hiding this comment.
📍 Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs:40
Architectural concern (Architect, structurally confirmed): redaction is applied by remembering to wrap each producer (TaskDialog, CustomCommand, ExceptionExtensions, wfastcgi.log) rather than at the logging-sink boundary. This makes coverage aspirational — any new logging call site silently bypasses redaction, which the evidence file already admits ("outside the covered redaction paths"). Consider redacting once where text enters the Trace/EventLog/ActivityLog/AppInsights writers so the guarantee becomes structural. Additionally, share a golden input/expected corpus between the C# and Python copies to enforce spec parity across languages.
[verified]
| // reloaded. | ||
| _canExecute = false; | ||
| ``` | ||
|
|
There was a problem hiding this comment.
📍 .github/compliance/evidence/reports/security/2984085-security-events-sensitive-errors-evidence-2026-07-03.md:91
Given the verified JSON/quoted-key bypass, the Evidence 3/4/5 "Control coverage: … avoids common secret-bearing key/value patterns" claims are overstated — they hold only for unquoted k=v/k: v, not the JSON/dict/HTTP_* forms these sinks most often carry. Temper this wording (or fix the redactor first) before this file is cited as coverage, so the compliance artifact doesn't create false confidence. The file's honest PARTIAL framing is otherwise good.
[verified]
| best.Configuration.GetPrefixPath(), | ||
| best.Configuration.InterpreterPath | ||
| ) : null; | ||
| } |
There was a problem hiding this comment.
📍 Python/Product/Cookiecutter/Model/CookiecutterClientProvider.cs:79
Coupling concern (Architect): the new FindCompatibleInterpreterFromRegistry fallback is a production interpreter-selection branch added "for tests" (comment: "used when no IServiceProvider is available (for example, in unit tests)"). Test needs leaking into product code is a smell, and this path is now coupled to the sibling PythonRegistrySearch 2.x-filter change so they must move together. Consider injecting a test seam instead of branching production logic on the absence of a service provider. The compModel?.GetService null-guard added here is a genuine fix (removes a real NRE).
[verified]
| var env = processInfo.Environment; | ||
| foreach (var kv in _serviceProvider.GetPythonToolsService().GetFullEnvironment(Configuration)) { | ||
| env[kv.Key] = kv.Value; | ||
| } |
There was a problem hiding this comment.
📍 Python/Product/PythonTools/PythonTools/Repl/PythonInteractiveEvaluator.CommandProcessorThread.cs:104
Unexplained behavior change (Architect): removing the #if DEBUG / if (!debugMode) guard means debug builds now always apply the full environment via GetFullEnvironment, silently changing debug-build startup behavior. This is unrelated to redaction and undocumented in the diff. Add a rationale (or a separate PR) explaining why the debug-only carve-out is no longer needed.
[verified]
| // Only filter Python 2.x for PythonCore-compatible entries. | ||
| // Custom configurable interpreters (e.g. company == "VisualStudio") | ||
| // may legitimately have no SysVersion set, and must not be filtered here. | ||
| if (pythonCoreCompatibility && sysVersion > new Version(0, 0) && sysVersion < new Version(3, 0)) { |
There was a problem hiding this comment.
📍 Python/Product/VSInterpreters/Interpreter/PythonRegistrySearch.cs:188
Behavior change riding a redaction PR (Advocate, Skeptic, Architect, all [unverified] runtime): the 2.x filter now only applies when pythonCoreCompatibility && sysVersion > (0,0), so PythonCore entries with a missing/unparseable SysVersion (previously defaulted to (0,0) and filtered out) now pass through. The change is narrowly guarded and the comment explains the intent (custom configurable interpreters), but it widens what surfaces and has no accompanying test in the diff. Add regression coverage for the missing-SysVersion case, or split this into its own PR.
[verified]
| } | ||
|
|
||
| /// <summary> | ||
| /// Enables using 'await' on this object. |
There was a problem hiding this comment.
📍 Python/Product/Cookiecutter/Shared/Infrastructure/ProcessOutput.cs:741
No defect found — all three reviewers independently confirmed WaitAsync correctly closes the subscribe-after-exit race (_haveRaisedExitedEvent/HasExited recheck + idempotent TrySetResult), links cancellation with the delay CTS, cancels the delay timer on the exit-wins path, and unsubscribes in finally on every path. The only gap: this new non-trivial async primitive ships without a visible unit test in the diff ([unverified] whether coverage exists elsewhere). Add a targeted test for the timeout, cancellation, and already-exited paths.
[verified]
|
|
||
| def log(txt): | ||
| """Logs messages to a log file if WSGI_LOG env var is defined.""" | ||
| txt = _sanitize_log_text(txt) |
There was a problem hiding this comment.
📍 Python/Product/WFastCgi/wfastcgi.py:361
Scope/cohesion note (Advocate, Architect): beyond redaction, this PR carries ~8 independent changes (FileUtils managed-delete, WaitAsync, PythonRegistrySearch filter, REPL #if DEBUG removal, REPL Debug.Fail→Trace.TraceWarning, Django re_path compat, UIA fallback, EnvironmentList orphan cleanup). Each is individually defensible, but the bundle can't be reviewed or reverted selectively — if the redaction work needs rollback given the verified leak, it drags unrelated stabilization fixes with it. Recommend splitting into (1) redaction+evidence, (2) test-infra stabilization, (3) interpreter/registry behavior. The WIP: title is appropriate for now.
[verified]
|
Main blocker: the redaction regexes only match bare |
|
Main theme: the new SensitiveDataRedactor is the security-critical core of this PR, but its regex only handles bare |
|
🔒 Automated review in progress — Heejae Chang (@heejaechang) is auto-reviewing this PR. |
| } | ||
|
|
||
| var sanitized = AuthorizationHeader.Replace(text, "$1" + RedactedValue); | ||
| sanitized = UriUserInfo.Replace(sanitized, "$1" + RedactedValue + "@"); |
There was a problem hiding this comment.
The Skeptic verified that JSON/quoted-key secrets pass through unredacted: {"password": "hunter2secret"} is returned verbatim. The SecretKeyValue separator group requires the :/= to immediately follow the bare keyword, but in JSON the character after password is ". Allow an optional quote between key and separator or explicitly match quoted JSON keys. This regex is triplicated in Cookiecutter and wfastcgi, so fix all copies.
| } | ||
|
|
||
| var sanitized = AuthorizationHeader.Replace(text, "$1" + RedactedValue); | ||
| sanitized = UriUserInfo.Replace(sanitized, "$1" + RedactedValue + "@"); |
There was a problem hiding this comment.
SECRET_KEY = '...' and other underscored compound keys are never redacted. secret requires a trailing \b, which fails before _, while key requires a leading \b, which fails after _. This leaves common Django secret values in cleartext. Add secret[_-]?key and corresponding compound forms to the alternation, or otherwise handle compound secret names in all three redactor copies.
| return text; | ||
| } | ||
|
|
||
| var sanitized = AuthorizationHeader.Replace(text, "$1" + RedactedValue); |
There was a problem hiding this comment.
Issue · Please address or respond
The key/value regex does not redact JSON-quoted keys: {"password": "hunter2"} passes through because the closing quote between password and : prevents the delimiter match. Exception strings and FastCGI payloads commonly contain JSON, so allow an optional closing key quote and add a JSON test case. Apply the equivalent correction to each redactor copy.
|
|
||
| public static string Sanitize(string text) { | ||
| if (string.IsNullOrEmpty(text)) { | ||
| return text; |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
The value pattern stops at spaces, commas, semicolons, and ampersands, leaking the remainder of secrets with those characters. For example, password: p@ss w0rd_secret becomes password: <redacted> w0rd_secret. Redact through the matching quote or an appropriate value boundary instead.
|
|
||
| public static string Sanitize(string text) { | ||
| if (string.IsNullOrEmpty(text)) { | ||
| return text; |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
The bare key and sig alternatives redact ordinary diagnostics such as primary key=5 and dictionary key: customer_id. This can hide useful failure context; narrow these alternatives or add tests that document the intended behavior for bare key and sig.
|
|
||
| **Control coverage**: | ||
|
|
||
| - User-visible expanded exception details avoid common secret-bearing key/value patterns, authorization headers, and URI user-info credentials. |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
The evidence currently overstates redaction coverage: JSON-shaped secrets are not covered by the current key/value pattern. Update this claim together with the regex fix so the assessment accurately describes the implemented protection.
Rich Chiodo (rchiodo)
added
the
review-auto:changes-requested
|
🔒 Automated review in progress — Stella Huang (@StellaHuang95) is auto-reviewing this PR. |
| r"sharedaccesssignature|sig|key)\b(\s*[:=]\s*)(['\"]?)([^'\";\s,&]+)(['\"]?)", | ||
| re.I | ||
| ) | ||
|
|
There was a problem hiding this comment.
Issue · Please address or respond
The value class [^'";\s,&]+ stops at the first whitespace, so secrets containing spaces are only partially redacted: Password=P@ss w0rd; becomes Password=<redacted> w0rd;. Passphrases and connection-string password segments can contain spaces, so this leaks sensitive data. Match quoted values through their closing quote and redact unquoted values through their delimiter in all three redactor implementations. [verified]
|
|
||
| public static string Sanitize(string text) { | ||
| if (string.IsNullOrEmpty(text)) { | ||
| return text; |
There was a problem hiding this comment.
Issue · Please address or respond
This pattern does not match JSON keys such as {"password": "secret"} because it expects : or = immediately after password, but the closing quote intervenes. JSON configuration and exception payloads can therefore log secrets unchanged. Support optionally quoted keys and add regression coverage for the C#, Cookiecutter, and WFastCGI redactors.
| [TestMethod, Priority(UnitTestPriority.P0)] | ||
| public void SanitizesAuthorizationHeaders() { | ||
| var sanitized = SensitiveDataRedactor.Sanitize("Before\r\nAuthorization: Bearer abc123\r\nAfter"); | ||
|
|
There was a problem hiding this comment.
Warning · Non-blocking recommendation
These tests cover only whitespace-free values, so they would not catch the verified space-in-value leak. Add quoted and unquoted secrets containing spaces plus semicolon/comma-delimited values; add equivalent coverage for the Cookiecutter and WFastCGI implementations to prevent the copies from drifting. [verified]
| var env = processInfo.Environment; | ||
| foreach (var kv in _serviceProvider.GetPythonToolsService().GetFullEnvironment(Configuration)) { | ||
| env[kv.Key] = kv.Value; | ||
| } |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Removing the #if DEBUG / if (!debugMode) guard now injects GetFullEnvironment(Configuration) unconditionally in DEBUG builds, changing REPL startup behavior independently of redaction. Please confirm this is intentional and cover or separate the behavior change. [verified]
| // Only filter Python 2.x for PythonCore-compatible entries. | ||
| // Custom configurable interpreters (e.g. company == "VisualStudio") | ||
| // may legitimately have no SysVersion set, and must not be filtered here. | ||
| if (pythonCoreCompatibility && sysVersion > new Version(0, 0) && sysVersion < new Version(3, 0)) { |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Gating the Python-2.x reject on pythonCoreCompatibility means known Python 2.x interpreters from non-PythonCore vendors are no longer filtered, although the previous behavior rejected all versions below 3.0. Add coverage for both an unset custom interpreter version and a known non-PythonCore Python 2.x version so the intended discovery policy is explicit. [verified]
|
|
||
| private static readonly Regex SecretKeyValue = new Regex( | ||
| @"(?ix)(\b(?:password|passwd|pwd|token|access[_-]?token|refresh[_-]?token|secret|client[_-]?secret|api[_-]?key|subscription[_-]?key|credential|authorization|connection\s*string|connectionstring|sharedaccesssignature|sig|key)\b\s*[:=]\s*)(['""']?)([^'"";\s,&]+)(['""']?)", | ||
| RegexOptions.CultureInvariant |
There was a problem hiding this comment.
Issue · Please address or respond
📍 Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs:34
Verified inputs such as {"password":"hunter2"} remain unchanged, while password="correct horse battery staple" leaves horse battery staple exposed. Update the pattern to support quoted keys and consume complete quoted values, then add these cases to the C# conformance tests.
[verified]
| r"\b(password|passwd|pwd|token|access[_-]?token|refresh[_-]?token|secret|client[_-]?secret|" | ||
| r"api[_-]?key|subscription[_-]?key|credential|authorization|connection\s*string|connectionstring|" | ||
| r"sharedaccesssignature|sig|key)\b(\s*[:=]\s*)(['\"]?)([^'\";\s,&]+)(['\"]?)", | ||
| re.I |
There was a problem hiding this comment.
Issue · Please address or respond
📍 Python/Product/WFastCgi/wfastcgi.py:342
The exact Python regex was verified to leak JSON-style and whitespace-containing secrets before they reach both logging sinks. Mirror the corrected matching semantics here and add direct WFastCGI tests so the Python implementation cannot drift from C#.
[verified]
| @"(?ix)(\b(?:password|passwd|pwd|token|access[_-]?token|refresh[_-]?token|secret|client[_-]?secret|api[_-]?key|subscription[_-]?key|credential|authorization|connection\s*string|connectionstring|sharedaccesssignature|sig|key)\b\s*[:=]\s*)(['""']?)([^'"";\s,&]+)(['""']?)", | ||
| RegexOptions.CultureInvariant | ||
| ); | ||
|
|
There was a problem hiding this comment.
Warning · Non-blocking recommendation
📍 Python/Product/Cookiecutter/Shared/Infrastructure/ExceptionExtensions.cs:51
This embeds a third independently maintained copy of the redaction policy, but only the Common implementation receives tests. Reuse one C# implementation where assembly boundaries permit, or drive every implementation from shared conformance vectors covering JSON, quoting, whitespace, URLs, and connection strings.
[verified]
| // Best-effort cleanup; ignore failures. | ||
| } | ||
| } | ||
| } |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
📍 Python/Tests/Core.UI/EnvironmentListTests.cs:1127
This cleanup deletes every GUID-named key under the user's HKCU\Software\Python\VisualStudio, although configurable interpreters can legitimately use arbitrary names and carry no test-ownership marker. Restrict cleanup to IDs recorded by this test or use an unmistakable test-specific prefix/marker.
[verified]
| return null; | ||
| // No service provider is available (e.g. when called from tests). | ||
| // Fall back to enumerating Python installations directly from the | ||
| // registry so we can still find a compatible interpreter. |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
📍 Python/Product/Cookiecutter/Model/CookiecutterClientProvider.cs:42
[unverified] A null service provider now triggers machine-wide registry discovery, changing production semantics specifically to accommodate tests. Prefer injecting an interpreter source for tests, or verify and document every production caller where this fallback is intended.
[verified]
| if (sysVersion < new Version(3, 0)) { | ||
| // Only filter Python 2.x for PythonCore-compatible entries. | ||
| // Custom configurable interpreters (e.g. company == "VisualStudio") | ||
| // may legitimately have no SysVersion set, and must not be filtered here. |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
📍 Python/Product/VSInterpreters/Interpreter/PythonRegistrySearch.cs:188
[unverified] This condition preserves unknown 0.0 custom entries but also permits explicitly versioned Python 2 custom interpreters, despite the surrounding unsupported-version policy. Narrow the exception to unknown versions or add a test proving Python 2 custom entries must remain discoverable.
[verified]
| } finally { | ||
| // Ensure we don't leak the event subscription on any exit path. | ||
| Exited -= onExited; | ||
| } |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
📍 Python/Product/Cookiecutter/Shared/Infrastructure/ProcessOutput.cs:743
This substantial public concurrency API has no changed caller or regression tests and is unrelated to the security-focused PR. Move it and the other stabilization changes into focused PRs, or add concrete callers plus timeout, cancellation, exit-race, and handler-cleanup tests with a clear rationale.
[verified]
| return; | ||
| } catch (InvalidOperationException) { | ||
| // Fall back to mouse input for controls that do not support UIA selection. | ||
| } |
There was a problem hiding this comment.
Warning · Non-blocking recommendation
Returning after UIA selection bypasses the existing mouse movement and click path. If that path repairs stale mouse state for drag-and-drop tests, preserve the reset behavior or add coverage with the pointer intentionally left in the problematic state.
[verified]
4 participants
No description provided.