Skip to content

Commit b888a42

Browse files
wenshaowenshao
committed
Revert "fix(cli): close the cmd.exe state-persisting entrances in the daemon guard"
This reverts commit a8f137a.
1 parent a8f137a commit b888a42

2 files changed

Lines changed: 9 additions & 196 deletions

File tree

‎packages/cli/src/serve/daemon-git-worktree-guard.ts‎

Lines changed: 9 additions & 84 deletions
Original file line numberDiff line numberDiff line change
@@ -219,17 +219,6 @@ const TIMEOUT_VALUE_FLAGS = new Set(['-k', '-s', '--kill-after', '--signal']);
219219
// is checked while `bait` is still the original directory.
220220
const PATH_RELINKING_PROGRAMS = new Set(['cp', 'ln', 'mv']);
221221

222-
// The cmd.exe spellings of the same capability (`mklink` also covers
223-
// junctions/hardlinks, which point a name at another volume entirely).
224-
const WIN32_PATH_RELINKING_PROGRAMS = new Set([
225-
...PATH_RELINKING_PROGRAMS,
226-
'copy',
227-
'mklink',
228-
'move',
229-
'robocopy',
230-
'xcopy',
231-
]);
232-
233222
// Archive extractors do not name the paths they write: the archive decides.
234223
// Everything under their extraction directory is therefore suspect, which is
235224
// the directory itself rather than any operand.
@@ -813,9 +802,9 @@ const GIT_WORD_PATTERN = /\bgit\b/i;
813802
// A `cd`/`pushd` inside such a payload relocates the git that follows it just
814803
// as effectively as a `-C` flag (`su -c 'cd <outside> && git reset --hard'`).
815804
const TEXT_RELOCATION_MARKER_WITHOUT_C_PATTERN =
816-
/(^|\s)(--git-dir=?|--work-tree=?|-execdir)|(^|[\s;&|(){}])(cd|chdir|pushd)([\s;&|]|$)|(^|[\s;&|(){}])(GIT_DIR|GIT_WORK_TREE|GIT_COMMON_DIR|GIT_INDEX_FILE|GIT_EXEC_PATH|PATH)\+?=/;
805+
/(^|\s)(--git-dir=?|--work-tree=?|-execdir)|(^|[\s;&|(){}])(cd|pushd)([\s;&|]|$)|(^|[\s;&|(){}])(GIT_DIR|GIT_WORK_TREE|GIT_COMMON_DIR|GIT_INDEX_FILE|GIT_EXEC_PATH|PATH)\+?=/;
817806
const TEXT_RELOCATION_MARKER_PATTERN =
818-
/(^|\s)(-C|--git-dir=?|--work-tree=?|-execdir)|(^|[\s;&|(){}])(cd|chdir|pushd)([\s;&|]|$)|(^|[\s;&|(){}])(GIT_DIR|GIT_WORK_TREE|GIT_COMMON_DIR|GIT_INDEX_FILE|GIT_EXEC_PATH|PATH)\+?=/;
807+
/(^|\s)(-C|--git-dir=?|--work-tree=?|-execdir)|(^|[\s;&|(){}])(cd|pushd)([\s;&|]|$)|(^|[\s;&|(){}])(GIT_DIR|GIT_WORK_TREE|GIT_COMMON_DIR|GIT_INDEX_FILE|GIT_EXEC_PATH|PATH)\+?=/;
819808

820809
// Assignments that decide WHICH git binary the run executes. The guard
821810
// classifies the program word `git` and then reasons about paths; if the
@@ -1216,18 +1205,11 @@ function findChdirTarget(
12161205
run: GuardToken[],
12171206
start: number,
12181207
variant: 'cd' | 'popd' | 'pushd',
1219-
windows = false,
12201208
): GuardToken | undefined {
12211209
let index = start;
12221210
while (index < run.length) {
12231211
const token = run[index]!;
12241212
if (token.text === '--') return run[index + 1];
1225-
// cmd.exe `cd /D <dir>` also switches the drive; `/D` is an option, not
1226-
// the directory.
1227-
if (windows && /^\/[dD]$/.test(token.text)) {
1228-
index++;
1229-
continue;
1230-
}
12311213
if (variant === 'cd' && CHDIR_OPTION_PATTERN.test(token.text)) {
12321214
index++;
12331215
continue;
@@ -1391,55 +1373,6 @@ function analyzeRun(
13911373
for (const operand of operands) recordEnvAssignment(operand, state);
13921374
return { kind: 'export', state, operands };
13931375
}
1394-
if (
1395-
platform === 'win32' &&
1396-
getShellConfiguration().shell !== 'bash' &&
1397-
(program === 'path' || program === 'doskey')
1398-
) {
1399-
// cmd.exe builtins that change how every later chained command
1400-
// resolves: `path` rewrites the executable search path (so which
1401-
// `git` runs is no longer provable) and `doskey` installs macros.
1402-
state.unresolved = true;
1403-
return { kind: 'export', state, operands: [] };
1404-
}
1405-
if (
1406-
platform === 'win32' &&
1407-
getShellConfiguration().shell !== 'bash' &&
1408-
(program === 'set' || program === 'setx')
1409-
) {
1410-
// cmd.exe `set VAR=value` — and `setx VAR value`, which persists past
1411-
// the session — mutate the environment every later `&&`-chained command
1412-
// executes under, so they carry the same semantics as a POSIX
1413-
// `export VAR=value`; route the assignments through that machinery.
1414-
// Anything that is not a plain assignment (bare `set`, `/p`/`/a`,
1415-
// dynamic operands) is undecidable and fails closed.
1416-
const operands = run.slice(index + 1);
1417-
if (program === 'setx') {
1418-
const [name, value] = operands;
1419-
if (
1420-
operands.length === 2 &&
1421-
!name!.dynamic &&
1422-
!value!.dynamic &&
1423-
/^[A-Za-z_][A-Za-z0-9_]*$/.test(name!.text)
1424-
) {
1425-
recordEnvAssignment(
1426-
{ text: `${name!.text}=${value!.text}`, dynamic: false },
1427-
state,
1428-
);
1429-
return { kind: 'export', state, operands };
1430-
}
1431-
state.unresolved = true;
1432-
return { kind: 'export', state, operands: [] };
1433-
}
1434-
for (const operand of operands) {
1435-
if (operand.dynamic || leadingEnvAssignmentKey(operand.text) === null) {
1436-
state.unresolved = true;
1437-
return { kind: 'export', state, operands: [] };
1438-
}
1439-
recordEnvAssignment(operand, state);
1440-
}
1441-
return { kind: 'export', state, operands };
1442-
}
14431376
if (program === 'set') {
14441377
if (requestsAllExport(run, index + 1)) {
14451378
return { kind: 'all-export', state };
@@ -1517,17 +1450,11 @@ function analyzeRun(
15171450
if (program === 'git') {
15181451
return { kind: 'git', tokens: run.slice(index), state };
15191452
}
1520-
if (
1521-
program === 'cd' ||
1522-
program === 'pushd' ||
1523-
program === 'popd' ||
1524-
(platform === 'win32' && program === 'chdir')
1525-
) {
1526-
const variant = program === 'chdir' ? 'cd' : program;
1453+
if (program === 'cd' || program === 'pushd' || program === 'popd') {
15271454
return {
15281455
kind: 'cd',
1529-
variant,
1530-
target: findChdirTarget(run, index + 1, variant, platform === 'win32'),
1456+
variant: program,
1457+
target: findChdirTarget(run, index + 1, program),
15311458
// `cd -P` resolves each component through its symlinks before
15321459
// applying `..`, which a lexical resolve cannot reproduce.
15331460
physical: run
@@ -3036,16 +2963,14 @@ async function evaluateCommandWithCwd(
30362963
// symlink race rather than denying every `tar && git commit`.
30372964
if (trackedCwd !== undefined) relinkedTargets.push(trackedCwd);
30382965
}
3039-
const relinkPrograms =
3040-
platformNow === 'win32'
3041-
? WIN32_PATH_RELINKING_PROGRAMS
3042-
: PATH_RELINKING_PROGRAMS;
3043-
if (run.some((t) => relinkPrograms.has(executableBaseName(t)))) {
2966+
if (
2967+
run.some((t) => PATH_RELINKING_PROGRAMS.has(executableBaseName(t)))
2968+
) {
30442969
// Wrappers and leading assignments (`env ln …`, `X=1 ln …`) keep
30452970
// the relinking program out of run[0], so scan the whole run.
30462971
for (const operand of run) {
30472972
if (operand.text.startsWith('-')) continue;
3048-
if (relinkPrograms.has(executableBaseName(operand))) {
2973+
if (PATH_RELINKING_PROGRAMS.has(executableBaseName(operand))) {
30492974
continue;
30502975
}
30512976
if (operand.dynamic || trackedCwd === undefined) {

‎packages/cli/src/serve/daemon-git-worktree-guard.win32-lane.test.ts‎

Lines changed: 0 additions & 112 deletions
Original file line numberDiff line numberDiff line change
@@ -48,115 +48,3 @@ it('harness premise: the lane resolves to win32/cmd', async () => {
4848
expect(os.platform()).toBe('win32');
4949
expect(getShellConfiguration().shell).toBe('cmd');
5050
});
51-
52-
// cmd.exe builtins persist state into every later `&&`-chained command:
53-
// `set`/`setx` mutate the environment, `cd`/`chdir` the working directory,
54-
// `path`/`doskey` change which executable a bare name resolves to, and
55-
// `copy`/`mklink`/… relink paths. Each entrance must reach the analysis the
56-
// same way its POSIX equivalent does — a relocation through any of them is
57-
// a boundary escape, not a cwd-local command.
58-
59-
const fs = await import('node:fs');
60-
const path = await import('node:path');
61-
const { createDaemonToolGuard } = await import(
62-
'./daemon-git-worktree-guard.js'
63-
);
64-
65-
const cmdRoot = fs.mkdtempSync(path.join(os.tmpdir(), 'guard-win32-cmd-'));
66-
const cmdWorktree = path.join(cmdRoot, 'workspace', 'worktree');
67-
const cmdOutsideRepo = path.join(cmdRoot, 'outside', 'repo');
68-
fs.mkdirSync(path.join(cmdOutsideRepo, '.git'), { recursive: true });
69-
fs.mkdirSync(cmdWorktree, { recursive: true });
70-
71-
afterAll(() => {
72-
fs.rmSync(cmdRoot, { recursive: true, force: true });
73-
});
74-
75-
describe('cmd.exe state-persisting builtins fail closed', () => {
76-
const guard = createDaemonToolGuard();
77-
const request = (command: string) =>
78-
({
79-
sessionId: 'session-1',
80-
promptId: 'prompt-1',
81-
toolCallId: 'call-1',
82-
toolName: 'run_shell_command',
83-
arguments: { command },
84-
effectiveCwd: cmdWorktree,
85-
}) as never;
86-
87-
it('denies a relocation persisted through set', async () => {
88-
await expect(
89-
guard(request(`set GIT_WORK_TREE=${cmdOutsideRepo}&& git reset --hard`)),
90-
).resolves.toMatchObject({
91-
allowed: false,
92-
reason: expect.stringContaining(cmdOutsideRepo),
93-
});
94-
await expect(
95-
guard(request(`set GIT_DIR=${cmdOutsideRepo}\\.git&& git reset --hard`)),
96-
).resolves.toMatchObject({ allowed: false });
97-
});
98-
99-
it('denies a relocation persisted through setx', async () => {
100-
await expect(
101-
guard(
102-
request(`setx GIT_WORK_TREE ${cmdOutsideRepo} && git reset --hard`),
103-
),
104-
).resolves.toMatchObject({
105-
allowed: false,
106-
reason: expect.stringContaining(cmdOutsideRepo),
107-
});
108-
});
109-
110-
it('keeps an in-boundary set harmless', async () => {
111-
await expect(
112-
guard(request(`set GIT_WORK_TREE=${cmdWorktree}&& git reset --hard`)),
113-
).resolves.toEqual({ allowed: true });
114-
await expect(guard(request(`set FOO=1&& git status`))).resolves.toEqual({
115-
allowed: true,
116-
});
117-
});
118-
119-
it('fails closed on set forms it cannot resolve', async () => {
120-
await expect(
121-
guard(request(`set /p X=&& git reset --hard`)),
122-
).resolves.toMatchObject({ allowed: false });
123-
await expect(
124-
guard(request(`set GIT_WORK_TREE=%DYN%&& git reset --hard`)),
125-
).resolves.toMatchObject({ allowed: false });
126-
});
127-
128-
it('denies a relocation through chdir, including /D', async () => {
129-
await expect(
130-
guard(request(`chdir ${cmdOutsideRepo} && git reset --hard`)),
131-
).resolves.toMatchObject({
132-
allowed: false,
133-
reason: expect.stringContaining(cmdOutsideRepo),
134-
});
135-
await expect(
136-
guard(request(`chdir /D ${cmdOutsideRepo} && git reset --hard`)),
137-
).resolves.toMatchObject({
138-
allowed: false,
139-
reason: expect.stringContaining(cmdOutsideRepo),
140-
});
141-
});
142-
143-
it('denies git after path or doskey rewrote command resolution', async () => {
144-
await expect(
145-
guard(request(`path ${cmdOutsideRepo};; && git reset --hard`)),
146-
).resolves.toMatchObject({ allowed: false });
147-
await expect(
148-
guard(request(`doskey git=evil.exe $* && git reset --hard`)),
149-
).resolves.toMatchObject({ allowed: false });
150-
});
151-
152-
it('denies git after a cmd relink program', async () => {
153-
await expect(
154-
guard(
155-
request(`mklink bait ${cmdOutsideRepo} && git -C bait reset --hard`),
156-
),
157-
).resolves.toMatchObject({ allowed: false });
158-
await expect(
159-
guard(request(`copy ${cmdOutsideRepo} bait && git -C bait reset --hard`)),
160-
).resolves.toMatchObject({ allowed: false });
161-
});
162-
});

0 commit comments

Comments
 (0)