From f16449781dda3ead374ed01e883324f113eefb87 Mon Sep 17 00:00:00 2001 From: 0xfandom Date: Wed, 8 Jul 2026 18:40:09 +0530 Subject: [PATCH 1/2] fix(powershell): make CMDLET_PATH_CONFIG prototype-safe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CMDLET_PATH_CONFIG is a plain object literal keyed by the lowercased cmdlet name, unlike its sibling maps CMDLET_ALLOWLIST (readOnlyValidation.ts) and COMMON_ALIASES (parser.ts) which are Object.create(null) specifically to defend against prototype-chain pollution. extractPathsFromCommand does `CMDLET_PATH_CONFIG[resolveToCanonical(cmd.name)]` guarded only by `if (!config)`. A command named 'constructor' or '__proto__' (both survive the lowercasing) makes the lookup return an inherited Object.prototype member — truthy — so the guard is bypassed and `[...config.knownSwitches]` throws (spread of undefined), crashing path-constraint validation instead of treating the name as an unknown non-path cmdlet. Give the map a null prototype like its siblings so inherited-key lookups return undefined. --- .../pathValidation.protoName.test.ts | 92 +++++++++++++++++++ src/tools/PowerShellTool/pathValidation.ts | 14 ++- 2 files changed, 104 insertions(+), 2 deletions(-) create mode 100644 src/tools/PowerShellTool/pathValidation.protoName.test.ts diff --git a/src/tools/PowerShellTool/pathValidation.protoName.test.ts b/src/tools/PowerShellTool/pathValidation.protoName.test.ts new file mode 100644 index 0000000000..9b2517cfea --- /dev/null +++ b/src/tools/PowerShellTool/pathValidation.protoName.test.ts @@ -0,0 +1,92 @@ +import { describe, expect, test } from 'bun:test' +import type { ToolPermissionContext } from '../../types/permissions.js' +import type { ParsedPowerShellCommand } from '../../utils/powershell/parser.js' +import { checkPathConstraints } from './pathValidation.js' + +function defaultContext(): ToolPermissionContext { + return { + mode: 'default', + additionalWorkingDirectories: new Map(), + alwaysAllowRules: {}, + alwaysDenyRules: {}, + alwaysAskRules: {}, + isBypassPermissionsModeAvailable: false, + } +} + +// Build a parsed command with a single cmdlet whose name we control. The real +// PowerShell AST parser requires a pwsh binary, so the parsed structure is +// constructed directly — the defect under test is in the CMDLET_PATH_CONFIG +// lookup keyed by the cmdlet name, downstream of parsing. A PowerShell command +// like `constructor -Path C:\x` parses to a command named `constructor` on a +// machine with pwsh installed. +function parsedCommandNamed(name: string): ParsedPowerShellCommand { + const command = `${name} -Path C:\\temp\\file.txt` + return { + valid: true, + errors: [], + variables: [], + hasStopParsing: false, + originalCommand: command, + statements: [ + { + statementType: 'PipelineAst', + text: command, + redirections: [], + commands: [ + { + name, + nameType: 'cmdlet', + elementType: 'CommandAst', + args: ['-Path', 'C:\\temp\\file.txt'], + text: command, + elementTypes: ['StringConstantExpressionAst', 'CommandParameterAst'], + }, + ], + }, + ], + } as unknown as ParsedPowerShellCommand +} + +// CMDLET_PATH_CONFIG is keyed by the (lowercased) cmdlet name. A command whose +// name collides with an Object.prototype member — `constructor`, `__proto__` — +// must resolve to no config (unknown-cmdlet passthrough), not an inherited +// prototype value. Before the null-proto fix the lookup returned a truthy +// inherited member, so `[...config.knownSwitches]` threw (spread of undefined) +// and crashed path-constraint validation. (`toString`/`valueOf` lowercase to +// `tostring`/`valueof` and never collide, so only these two are reachable.) +describe('checkPathConstraints with prototype-polluting cmdlet names', () => { + test.each(['constructor', '__proto__'])( + 'treats %s as an unknown cmdlet instead of crashing', + (name) => { + const parsed = parsedCommandNamed(name) + expect(() => + checkPathConstraints( + { command: parsed.originalCommand }, + parsed, + defaultContext(), + ), + ).not.toThrow() + const result = checkPathConstraints( + { command: parsed.originalCommand }, + parsed, + defaultContext(), + ) + expect(result.behavior).toBe('passthrough') + }, + ) + + test('a real path cmdlet is still classified (control)', () => { + const parsed = parsedCommandNamed('set-content') + // set-content is a known write cmdlet, so its config is found and the + // lookup does not fall through to the unknown-cmdlet branch — proves the + // null-proto container still holds its own entries. + expect(() => + checkPathConstraints( + { command: parsed.originalCommand }, + parsed, + defaultContext(), + ), + ).not.toThrow() + }) +}) diff --git a/src/tools/PowerShellTool/pathValidation.ts b/src/tools/PowerShellTool/pathValidation.ts index 1c56a61c37..b6ef629848 100644 --- a/src/tools/PowerShellTool/pathValidation.ts +++ b/src/tools/PowerShellTool/pathValidation.ts @@ -121,7 +121,16 @@ type CmdletPathConfig = { optionalWrite?: boolean } -const CMDLET_PATH_CONFIG: Record = { +// Uses Object.create(null) to prevent prototype-chain pollution — an +// attacker-controlled cmdlet name like 'constructor' or '__proto__' must return +// undefined here (hitting the `if (!config)` unknown-cmdlet passthrough), not an +// inherited Object.prototype member. Without it, the lookup returns a truthy +// inherited value and `[...config.knownSwitches]` throws (spread of undefined), +// crashing path-constraint validation. Same defense as CMDLET_ALLOWLIST +// (readOnlyValidation.ts) and COMMON_ALIASES (parser.ts). +const CMDLET_PATH_CONFIG: Record = Object.assign( + Object.create(null) as Record, + { // ─── Write/create operations ────────────────────────────────────────────── 'set-content': { operationType: 'write', @@ -762,7 +771,8 @@ const CMDLET_PATH_CONFIG: Record = { ], knownValueParams: ['-name', '-description', '-scope', '-as'], }, -} + }, +) /** * Checks if a lowercase parameter name (with leading dash) matches any entry From 8e370673f2d5c2be5ad686ad1a7f263834ee877e Mon Sep 17 00:00:00 2001 From: 0xfandom Date: Thu, 9 Jul 2026 14:53:40 +0530 Subject: [PATCH 2/2] test(powershell): assert set-content control resolves to ask decision The control case previously only asserted checkPathConstraints did not throw, so it would still pass if CMDLET_PATH_CONFIG stopped resolving its own entries and set-content fell through as an unknown cmdlet. Assert the returned behavior is 'ask' so the test proves the null-prototype map still holds and classifies its own keys. Also reword the collision comment to present constructor/__proto__ as representative reachable names rather than the only ones. --- .../pathValidation.protoName.test.ts | 32 +++++++++++-------- 1 file changed, 18 insertions(+), 14 deletions(-) diff --git a/src/tools/PowerShellTool/pathValidation.protoName.test.ts b/src/tools/PowerShellTool/pathValidation.protoName.test.ts index 9b2517cfea..b2fdb23637 100644 --- a/src/tools/PowerShellTool/pathValidation.protoName.test.ts +++ b/src/tools/PowerShellTool/pathValidation.protoName.test.ts @@ -49,12 +49,14 @@ function parsedCommandNamed(name: string): ParsedPowerShellCommand { } // CMDLET_PATH_CONFIG is keyed by the (lowercased) cmdlet name. A command whose -// name collides with an Object.prototype member — `constructor`, `__proto__` — -// must resolve to no config (unknown-cmdlet passthrough), not an inherited -// prototype value. Before the null-proto fix the lookup returned a truthy -// inherited member, so `[...config.knownSwitches]` threw (spread of undefined) -// and crashed path-constraint validation. (`toString`/`valueOf` lowercase to -// `tostring`/`valueof` and never collide, so only these two are reachable.) +// name collides with an inherited Object.prototype member must resolve to no +// config (unknown-cmdlet passthrough), not the inherited prototype value. Before +// the null-proto fix the lookup returned a truthy inherited member, so +// `[...config.knownSwitches]` threw (spread of undefined) and crashed +// path-constraint validation. `constructor` and `__proto__` below are +// representative all-lowercase collisions reachable through the lowercased key; +// the null-prototype container neutralizes every inherited-key lookup, not just +// these two. describe('checkPathConstraints with prototype-polluting cmdlet names', () => { test.each(['constructor', '__proto__'])( 'treats %s as an unknown cmdlet instead of crashing', @@ -80,13 +82,15 @@ describe('checkPathConstraints with prototype-polluting cmdlet names', () => { const parsed = parsedCommandNamed('set-content') // set-content is a known write cmdlet, so its config is found and the // lookup does not fall through to the unknown-cmdlet branch — proves the - // null-proto container still holds its own entries. - expect(() => - checkPathConstraints( - { command: parsed.originalCommand }, - parsed, - defaultContext(), - ), - ).not.toThrow() + // null-proto container still holds and resolves its own entries. Its + // parameterized `-Path` cannot be statically validated, so the classified + // decision is `ask` rather than the `passthrough` returned for the + // prototype-key names above. + const result = checkPathConstraints( + { command: parsed.originalCommand }, + parsed, + defaultContext(), + ) + expect(result.behavior).toBe('ask') }) })