Suppress UNC host access errors from unhandled-error telemetry#312236
Suppress UNC host access errors from unhandled-error telemetry#312236
Conversation
Agent-Logs-Url: https://github.com/microsoft/vscode/sessions/ced760c1-3e96-49dc-9650-5a4b1ea18dcb Co-authored-by: bryanchen-d <41454397+bryanchen-d@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
This PR aims to prevent UNC host access-denial errors (ERR_UNC_HOST_NOT_ALLOWED)—which result from a user-configured security policy—from being reported as unhandled-error telemetry by marking them as “no telemetry” while also mapping them to a permissions-related file-system error code.
Changes:
- Map
ERR_UNC_HOST_NOT_ALLOWEDtoFileSystemProviderErrorCode.NoPermissionsinDiskFileSystemProvider.toFileSystemProviderError(). - Introduce
markAsErrorNoTelemetry(error)as a helper to mark an existing error instance as “no telemetry” without wrapping it. - Add a unit test validating that
markAsErrorNoTelemetrypreserves the original error instance/type/properties.
Show a summary per file
| File | Description |
|---|---|
| src/vs/platform/files/node/diskFileSystemProvider.ts | Treat UNC-host-denied as NoPermissions and attempt to suppress telemetry by marking the provider error as no-telemetry. |
| src/vs/base/common/errors.ts | Add markAsErrorNoTelemetry helper that marks an error by mutating its name. |
| src/vs/base/test/common/errors.test.ts | Add test ensuring markAsErrorNoTelemetry preserves instance/type/properties while being detected as no-telemetry. |
Copilot's findings
- Files reviewed: 3/3 changed files
- Comments generated: 3
| const providerError = createFileSystemProviderError(resultError, code); | ||
| if (isExpected) { | ||
| markAsErrorNoTelemetry(providerError); | ||
| } |
There was a problem hiding this comment.
markAsErrorNoTelemetry(providerError) overwrites FileSystemProviderError.name. For file-system errors, the name field is used to carry the FileSystemProviderErrorCode across IPC boundaries (because the IPC channel only serializes message/name/stack for Error instances), and toFileSystemProviderErrorCode() relies on that name format when the error is no longer an actual FileSystemProviderError. With name set to CodeExpectedError, callers in other processes will see this as Unknown (and lose the intended NoPermissions semantics). Consider avoiding name mutation for FileSystemProviderError and instead propagating an explicit no-telemetry marker (e.g. include a noTelemetry/code field in IPC error serialization, or update the no-telemetry mechanism to not repurpose name).
Agent-Logs-Url: https://github.com/microsoft/vscode/sessions/5fe8b3aa-6580-4b37-b5e8-3c4a11350ee2 Co-authored-by: bryanchen-d <41454397+bryanchen-d@users.noreply.github.com>
| export function markAsErrorNoTelemetry<T extends Error>(error: T): T { | ||
| error.name = 'CodeExpectedError'; | ||
| // eslint-disable-next-line local/code-no-any-casts | ||
| (<any>error)[noTelemetryFlag] = true; |
There was a problem hiding this comment.
instead of any, can we just cast it to a Record<> with the noTelemetryFlag property?
There was a problem hiding this comment.
Replaced the any casts with a typed ErrorWithNoTelemetryFlag = Error & { [noTelemetryFlag]?: boolean } alias used for both setting and reading the flag. Done in 6f3dfaf.
Agent-Logs-Url: https://github.com/microsoft/vscode/sessions/94e6d14a-9e39-41af-b69e-b2a6a715e8d5 Co-authored-by: bryanchen-d <41454397+bryanchen-d@users.noreply.github.com>
Suppresses
ERR_UNC_HOST_NOT_ALLOWEDerrors from being reported as unhandled errors in telemetry. These errors originate from a user-configured allowlist (security.allowedUNCHosts) and represent an expected access denial rather than a bug.Changes Made
src/vs/platform/files/node/diskFileSystemProvider.ts: When mapping a Node.js error to aFileSystemProviderError, treatERR_UNC_HOST_NOT_ALLOWEDasNoPermissions(instead ofUnknown) and mark the resulting error as no-telemetry so it does not surface in unhandled-error telemetry.src/vs/base/common/errors.ts: Added amarkAsErrorNoTelemetryhelper that flags an existingErrorinstance as no-telemetry while preserving its original class, prototype chain,name, and additional properties. The flag is implemented as an internal__vscodeNoTelemetryproperty on the error rather than by mutatingerror.name. This avoids side effects for errors that encode semantics inname— most importantlyFileSystemProviderError, whose name carries theFileSystemProviderErrorCodeacross IPC boundaries.ErrorNoTelemetry.isErrorNoTelemetrychecks the new flag and still recognizes the legacyname === 'CodeExpectedError'for compatibility with deserialized errors.src/vs/base/test/common/errors.test.ts: Added a unit test verifying thatmarkAsErrorNoTelemetrypreserves the original error class,name, message, and additional properties while making the error detectable as no-telemetry.Testing
npm run compile-check-ts-native)markAsErrorNoTelemetry