-
Notifications
You must be signed in to change notification settings - Fork 680
WIP: add security event evidence and redaction #8574
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
base: main
Are you sure you want to change the base?
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,174 @@ | ||
| # Security Events and Sensitive Error Messages - Assessment Evidence | ||
|
|
||
| **Work Item**: [ADO #2984085](https://dev.azure.com/devdiv/DevDiv/_workitems/edit/2984085) | ||
| **Assessment Date**: 2026-07-03 | ||
| **Status**: PARTIAL - repo-backed evidence and targeted hardening added; owner review still required before claiming full compliance | ||
|
|
||
| ## Summary | ||
|
|
||
| PTVS has repo-backed evidence for telemetry/event logging, unhandled-exception reporting, ActivityLog/EventLog integration, and non-fatal logging paths. PTVS is primarily Visual Studio client functionality, so this control is interpreted as: product security-relevant events and failures should be logged for diagnostics, while user-facing errors and diagnostic logs should avoid exposing secrets or unnecessary sensitive internals. | ||
|
|
||
| This assessment found several existing logging and error-handling mechanisms. It also added targeted redaction for common secret patterns in central C# diagnostic exception output and user-visible task-dialog details. This does not prove full compliance for every security-sensitive flow and does not include runtime logs, telemetry dashboards, Watson records, or owner attestation that all user-visible errors are free of sensitive data. | ||
|
|
||
| ## Test Case 571: Log security events and do not expose security-sensitive errors/messages - PARTIAL | ||
|
|
||
| **Assessor Request**: Initial submission. The work item has no existing comments. | ||
|
|
||
| ### Evidence 1: Telemetry logging is opt-in and structured | ||
|
|
||
| **Location**: `Python/Product/PythonTools/PythonTools/Logging/VsTelemetryLogger.cs` | ||
|
|
||
| PTVS telemetry is posted through Visual Studio telemetry only when a session exists and the user has opted in. The implementation prefixes Python events consistently and avoids logging repeated package names. | ||
|
|
||
| ```csharp | ||
| public void LogEvent(PythonLogEvent logEvent, object argument) { | ||
| // No session is not a fatal error. | ||
| // Never send events when users have not opted in. | ||
| if (_session.Value == null || !_session.Value.IsOptedIn) { | ||
| return; | ||
| } | ||
|
|
||
| // Certain events are not collected | ||
| switch (logEvent) { | ||
| case PythonLogEvent.PythonPackage: | ||
| lock (_seenPackages) { | ||
| var name = (argument as PackageInfo)?.Name; | ||
| // Don't send empty or repeated names | ||
| if (string.IsNullOrEmpty(name) || !_seenPackages.Add(name)) { | ||
| return; | ||
| } | ||
| } | ||
| break; | ||
| } | ||
|
|
||
| var evt = new TelemetryEvent(EventPrefix + logEvent.ToString()); | ||
| ``` | ||
|
|
||
| **Control coverage**: | ||
|
|
||
| - Events are structured under the `vs/python/` prefix. | ||
| - Telemetry respects Visual Studio opt-in state. | ||
| - Repeated or empty package-name telemetry is filtered. | ||
|
|
||
| ### Evidence 2: Unhandled exceptions are reported to trace, EventLog, ActivityLog/UI with de-duplication | ||
|
|
||
| **Location**: `Python/Product/VSCommon/Infrastructure/VSTaskExtensions.cs` and `Python/Product/Common/Infrastructure/ExceptionExtensions.cs` | ||
|
|
||
| Unhandled exception handling centralizes task exception reporting. Non-critical task exceptions are converted to diagnostic messages, traced, written to Windows EventLog where possible, and de-duplicated before user display. | ||
|
|
||
| ```csharp | ||
| var message = ex.ToUnhandledExceptionMessage(callerType, callerFile, callerLineNumber, callerName); | ||
|
|
||
| // Send the message to the trace listener in case there is | ||
| // somebody out there listening. | ||
| Trace.TraceError(message); | ||
| ``` | ||
|
|
||
| ```csharp | ||
| try { | ||
| result = await task; | ||
| } catch (Exception ex) { | ||
| if (task.IsFaulted) { | ||
| if (ex.IsCriticalException()) { | ||
| throw; | ||
| } | ||
|
|
||
| ex.ReportUnhandledException(site, callerType, callerFile, callerLineNumber, callerName, allowUI); | ||
| } | ||
| } | ||
| ``` | ||
|
|
||
| **Control coverage**: | ||
|
|
||
| - Security-relevant/failure events are not silently swallowed in the common async path. | ||
| - Event logging failures are contained so logging failures do not destabilize the product. | ||
| - Critical exceptions are rethrown rather than hidden. | ||
| - Central exception-message output is sanitized for common secret patterns before being returned to trace/EventLog/debug consumers. | ||
|
|
||
| ### Evidence 3: User-visible task dialog details are sanitized for common secret patterns | ||
|
|
||
| **Location**: `Python/Product/VSCommon/Infrastructure/TaskDialog.cs` | ||
|
|
||
| Task-dialog expanded exception details are passed through `SensitiveDataRedactor.Sanitize(...)` before display. Retry dialogs also sanitize exception messages and expanded exception details. | ||
|
|
||
| **Control coverage**: | ||
|
|
||
| - User-visible expanded exception details avoid common secret-bearing key/value patterns, authorization headers, and URI user-info credentials. | ||
| - The redaction is deterministic and does not suppress exception reporting. | ||
|
|
||
| ### Evidence 4: Command failures are logged and command reuse is disabled after unexpected errors | ||
|
|
||
| **Location**: `Python/Product/PythonTools/PythonTools/Project/CustomCommand.cs` | ||
|
|
||
| Custom command execution disables the command after unexpected errors and logs to ActivityLog. ActivityLog messages added through these paths are sanitized before logging. | ||
|
|
||
| ```csharp | ||
| // Prevent the command from executing again until the project is | ||
| // reloaded. | ||
| _canExecute = false; | ||
| ``` | ||
|
|
||
| **Control coverage**: | ||
|
|
||
| - Prevents repeated execution after an unexpected command failure. | ||
| - Logs diagnostic data for investigation. | ||
| - Redacts common secret patterns before ActivityLog output for the covered custom-command failure paths. | ||
|
|
||
| ### Evidence 5: FastCGI helper logs request/runtime failures without breaking request handling | ||
|
|
||
| **Location**: `Python/Product/WFastCgi/wfastcgi.py` | ||
|
|
||
| WFastCGI logs to Application Insights when configured and to `WSGI_LOG` when present. Logging failures are intentionally non-fatal; unhandled exceptions in the FastCGI loop are logged. Log text is sanitized before it is sent to either sink. | ||
|
|
||
| ```python | ||
| def maybe_log(txt): | ||
| """Logs messages to a log file if WSGI_LOG env var is defined, and does not | ||
| raise exceptions if logging fails.""" | ||
| try: | ||
| log(txt) | ||
| except: # nosec B110 | ||
| pass # nosec B110 - maybe_log intentionally suppresses logging failures. | ||
| ``` | ||
|
|
||
| **Control coverage**: | ||
|
|
||
| - Runtime failures are logged when logging is configured. | ||
| - Logging failures do not break request processing. | ||
| - Unhandled FastCGI exceptions are captured for investigation. | ||
| - Common secret-bearing key/value patterns, authorization headers, and URI user-info credentials are redacted before configured FastCGI logging sinks receive the message. | ||
|
|
||
| ## Owner Review Needed | ||
|
|
||
| - Confirm whether PTVS has a documented list of security events that must be logged for Visual Studio client functionality. | ||
| - Confirm whether any error dialogs, output-window messages, ActivityLog entries, telemetry events, or FastCGI logs may contain secrets, tokens, credentials, customer content, full local paths, or other sensitive data outside the covered redaction paths. | ||
| - Confirm whether WFastCGI exception stack traces are acceptable for configured server logs, or whether additional redaction is required. | ||
| - Confirm whether telemetry properties and Watson records require a separate allowlist or scrub pass. | ||
|
|
||
| ## Draft ADO Comment | ||
|
|
||
| ```markdown | ||
| ## Security events and sensitive errors - Evidence Report | ||
|
|
||
| **Assessment Date:** 2026-07-03 | ||
| **Status:** PARTIAL - owner review required for complete sensitive-data and security-event coverage | ||
|
|
||
| ### Evidence | ||
|
|
||
| - `Python/Product/PythonTools/PythonTools/Logging/VsTelemetryLogger.cs` posts structured Python telemetry only when Visual Studio telemetry is available and the user has opted in. | ||
| - `Python/Product/VSCommon/Infrastructure/VSTaskExtensions.cs` centralizes unhandled task exception reporting to trace/EventLog and rethrows critical exceptions. | ||
| - `Python/Product/Common/Infrastructure/ExceptionExtensions.cs` now sanitizes central unhandled-exception diagnostic messages for common secret patterns. | ||
| - `Python/Product/Cookiecutter/Shared/Infrastructure/ExceptionExtensions.cs` now sanitizes the duplicated Cookiecutter unhandled-exception formatter for the same common secret patterns. | ||
| - `Python/Product/VSCommon/Infrastructure/TaskDialog.cs` now sanitizes user-visible expanded exception details and retry-dialog exception text. | ||
| - `Python/Product/PythonTools/PythonTools/Project/CustomCommand.cs` logs unexpected custom-command failures to ActivityLog, disables the command until reload, and sanitizes covered ActivityLog messages. | ||
| - `Python/Product/WFastCgi/wfastcgi.py` logs WSGI/FastCGI runtime failures to configured logs/Application Insights, keeps logging failures non-fatal, and sanitizes log text before both configured sinks. | ||
|
|
||
| ### Owner-reviewed limitations | ||
|
|
||
| - Need owner confirmation that product error messages/logs do not expose secrets or other sensitive data outside the newly covered redaction paths. | ||
| - Need owner confirmation that security-event coverage is sufficient for Visual Studio client functionality. | ||
| - Need owner confirmation for telemetry dashboard, Watson record, and WFastCGI stack-trace treatment. | ||
|
|
||
| Evidence file: `.github/compliance/evidence/reports/security/2984085-security-events-sensitive-errors-evidence-2026-07-03.md` | ||
|
|
||
| *written by [comply](https://aka.ms/comply), reviewed by Bill* | ||
| ``` | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,52 @@ | ||
| // Python Tools for Visual Studio | ||
| // Copyright(c) Microsoft Corporation | ||
| // All rights reserved. | ||
| // | ||
| // Licensed under the Apache License, Version 2.0 (the License); you may not use | ||
| // this file except in compliance with the License. You may obtain a copy of the | ||
| // License at http://www.apache.org/licenses/LICENSE-2.0 | ||
| // | ||
| // THIS CODE IS PROVIDED ON AN *AS IS* BASIS, WITHOUT WARRANTIES OR CONDITIONS | ||
| // OF ANY KIND, EITHER EXPRESS OR IMPLIED, INCLUDING WITHOUT LIMITATION ANY | ||
| // IMPLIED WARRANTIES OR CONDITIONS OF TITLE, FITNESS FOR A PARTICULAR PURPOSE, | ||
| // MERCHANTABILITY OR NON-INFRINGEMENT. | ||
| // | ||
| // See the Apache Version 2.0 License for specific language governing | ||
| // permissions and limitations under the License. | ||
|
|
||
| using System; | ||
| using System.Text.RegularExpressions; | ||
|
|
||
| namespace Microsoft.PythonTools.Infrastructure { | ||
| static class SensitiveDataRedactor { | ||
| internal const string RedactedValue = "<redacted>"; | ||
|
|
||
| private static readonly Regex AuthorizationHeader = new Regex( | ||
| @"(?im)(\bAuthorization\s*:\s*)([^\r\n]+)", | ||
| RegexOptions.CultureInvariant | ||
| ); | ||
|
|
||
| private static readonly Regex UriUserInfo = new Regex( | ||
| @"(?i)\b([a-z][a-z0-9+.-]*://)([^/\s:@]+(?::[^/\s@]*)?@)", | ||
| RegexOptions.CultureInvariant | ||
| ); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📍 Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs:30 [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 | ||
| ); | ||
|
|
||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📍 Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs:35 [verified]
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📍 Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs:35 [verified] |
||
| public static string Sanitize(string text) { | ||
| if (string.IsNullOrEmpty(text)) { | ||
| return text; | ||
| } | ||
|
|
||
| var sanitized = AuthorizationHeader.Replace(text, "$1" + RedactedValue); | ||
| sanitized = UriUserInfo.Replace(sanitized, "$1" + RedactedValue + "@"); | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. The Skeptic verified that JSON/quoted-key secrets pass through unredacted: There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
|
||
| sanitized = SecretKeyValue.Replace(sanitized, match => | ||
| match.Groups[1].Value + match.Groups[2].Value + RedactedValue + match.Groups[4].Value | ||
| ); | ||
| return sanitized; | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📍 Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs:44 [verified] |
||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📍 Python/Product/Common/Infrastructure/SensitiveDataRedactor.cs:40 [verified] |
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -42,11 +42,17 @@ public static bool IsCompatiblePythonAvailable(IServiceProvider provider) { | |
| private static CookiecutterPythonInterpreter FindCompatibleInterpreter(IServiceProvider provider) { | ||
|
|
||
| if (provider == null) { | ||
| 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. | ||
| return FindCompatibleInterpreterFromRegistry(); | ||
| } | ||
|
|
||
| var compModel = provider.GetService(typeof(SComponentModel)) as IComponentModel; | ||
| var interpreters = compModel.GetService<IInterpreterRegistryService>(); | ||
| var interpreters = compModel?.GetService<IInterpreterRegistryService>(); | ||
| if (interpreters == null) { | ||
| return null; | ||
| } | ||
|
|
||
| var all = interpreters.Configurations | ||
| .Where(x => File.Exists(x.InterpreterPath)) | ||
|
|
@@ -65,13 +71,39 @@ private static CookiecutterPythonInterpreter FindCompatibleInterpreter(IServiceP | |
| cpython.Add(configuration); | ||
| } | ||
| } | ||
|
|
||
| var best = cpython.FirstOrDefault() ?? all.FirstOrDefault(); | ||
|
|
||
| return best != null ? new CookiecutterPythonInterpreter( | ||
| best.GetPrefixPath(), | ||
| best.InterpreterPath | ||
| ) : null; | ||
| } | ||
|
|
||
| private static CookiecutterPythonInterpreter FindCompatibleInterpreterFromRegistry() { | ||
| // Enumerate Python installations directly from the registry. This path | ||
| // is used when no IServiceProvider is available (for example, in unit | ||
| // tests), where the MEF-based IInterpreterRegistryService can't be | ||
| // resolved. | ||
| var all = PythonRegistrySearch.PerformDefaultSearch() | ||
| .Where(info => File.Exists(info.Configuration.InterpreterPath)) | ||
| .Where(info => info.Configuration.Version >= new Version(3, 5)) | ||
| .OrderByDescending(info => info.Configuration.Version) | ||
| .ToList(); | ||
|
|
||
| // Prefer a CPython installation if there is one because | ||
| // some Anaconda installs have trouble creating a venv. | ||
| var cpython = all | ||
| .Where(info => info.Vendor != null && | ||
| info.Vendor.IndexOfOrdinal("Python Software Foundation", ignoreCase: true) == 0) | ||
| .ToList(); | ||
|
|
||
| var best = cpython.FirstOrDefault() ?? all.FirstOrDefault(); | ||
|
|
||
| return best != null ? new CookiecutterPythonInterpreter( | ||
| best.Configuration.GetPrefixPath(), | ||
| best.Configuration.InterpreterPath | ||
| ) : null; | ||
| } | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📍 Python/Product/Cookiecutter/Model/CookiecutterClientProvider.cs:79 [verified] |
||
| } | ||
| } | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
📍 .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 honestPARTIALframing is otherwise good.[verified]