mirror of https://github.com/garrytan/gstack.git
fix(security): delete chain's shadow dispatcher that skipped every security gate
meta-commands.ts carried a 'CLI mode' fallback that re-implemented command
routing without the server pipeline's gates: no scope check, no domain check,
no tab ownership, no rate limit, no hidden-element stripping, no scoped-token
enveloping — and it called handleReadCommand without a BrowserManager, which
also skipped the JS-origin cookie-exfiltration assertion. It was unreachable
in production (server.ts always passes executeCommand) and one boolean away
from being live.
chain now hard-errors without a server context. handleReadCommand's bm param
is required and assertJsOriginAllowed runs unconditionally. The chain tests
that exercised the deleted fallback now route through a server-shaped
executeCommand adapter (real handlers + trust wrapping + {status,result}
envelope), so their behavioral coverage — sequencing, trust markers, pipe
format, aliases, error reporting — survives on the production-shaped path.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
Garry Tan
parent
b241a79ee5
commit
ef03646e07
|
|
@ -667,40 +667,13 @@ export async function handleMetaCommand(
|
|||
lastWasWrite = WRITE_COMMANDS.has(c.name);
|
||||
}
|
||||
} else {
|
||||
// Fallback: direct dispatch (CLI mode, no server context)
|
||||
const { handleReadCommand } = await import('./read-commands');
|
||||
const { handleWriteCommand } = await import('./write-commands');
|
||||
|
||||
for (const c of commands) {
|
||||
const name = c.name;
|
||||
const cmdArgs = c.args;
|
||||
const label = c.rawName === name ? name : `${c.rawName}→${name}`;
|
||||
try {
|
||||
let result: string;
|
||||
if (WRITE_COMMANDS.has(name)) {
|
||||
if (bm.isWatching()) {
|
||||
result = 'BLOCKED: write commands disabled in watch mode';
|
||||
} else {
|
||||
result = await handleWriteCommand(name, cmdArgs, session, bm);
|
||||
}
|
||||
lastWasWrite = true;
|
||||
} else if (READ_COMMANDS.has(name)) {
|
||||
result = await handleReadCommand(name, cmdArgs, session);
|
||||
if (PAGE_CONTENT_COMMANDS.has(name)) {
|
||||
result = wrapUntrustedContent(result, bm.getCurrentUrl());
|
||||
}
|
||||
lastWasWrite = false;
|
||||
} else if (META_COMMANDS.has(name)) {
|
||||
result = await handleMetaCommand(name, cmdArgs, bm, shutdown, tokenInfo, opts);
|
||||
lastWasWrite = false;
|
||||
} else {
|
||||
throw new Error(`Unknown command: ${c.rawName}`);
|
||||
}
|
||||
results.push(`[${label}] ${result}`);
|
||||
} catch (err: any) {
|
||||
results.push(`[${label}] ERROR: ${err.message}`);
|
||||
}
|
||||
}
|
||||
// No fallback dispatcher. The old direct-dispatch branch here
|
||||
// re-implemented command routing WITHOUT the server pipeline's
|
||||
// security gates (scope, domain, tab ownership, rate limit, hidden
|
||||
// element stripping, scoped-token enveloping, JS-origin assertion).
|
||||
// It was unreachable in production (server.ts always passes
|
||||
// executeCommand) and one boolean away from being live.
|
||||
throw new Error('chain requires the browse server (no executeCommand context)');
|
||||
}
|
||||
|
||||
// Wait for network to settle after write commands before returning
|
||||
|
|
|
|||
|
|
@ -212,7 +212,7 @@ export async function handleReadCommand(
|
|||
command: string,
|
||||
args: string[],
|
||||
session: TabSession,
|
||||
bm?: BrowserManager,
|
||||
bm: BrowserManager,
|
||||
): Promise<string> {
|
||||
const page = session.getPage();
|
||||
// Frame-aware target for content extraction
|
||||
|
|
@ -293,7 +293,7 @@ export async function handleReadCommand(
|
|||
const { outPath, raw, rest } = parseOutArgs(args);
|
||||
const expr = rest[0];
|
||||
if (!expr) throw new Error('Usage: browse js <expression> [--out <file>] [--raw]');
|
||||
if (bm) assertJsOriginAllowed(bm, page.url());
|
||||
assertJsOriginAllowed(bm, page.url());
|
||||
const wrapped = wrapForEvaluate(expr);
|
||||
const result = await target.evaluate(wrapped);
|
||||
const str = resultToString(result);
|
||||
|
|
@ -308,7 +308,7 @@ export async function handleReadCommand(
|
|||
const { outPath, raw, rest } = parseOutArgs(args);
|
||||
const filePath = rest[0];
|
||||
if (!filePath) throw new Error('Usage: browse eval <js-file> [--out <file>] [--raw]');
|
||||
if (bm) assertJsOriginAllowed(bm, page.url());
|
||||
assertJsOriginAllowed(bm, page.url());
|
||||
validateReadPath(filePath);
|
||||
if (!fs.existsSync(filePath)) throw new Error(`File not found: ${filePath}`);
|
||||
const code = fs.readFileSync(filePath, 'utf-8');
|
||||
|
|
|
|||
|
|
@ -12,6 +12,7 @@ import { resolveServerScript } from '../src/cli';
|
|||
import { handleReadCommand as _handleReadCommand, parseOutArgs, hasOutArg, resultToString } from '../src/read-commands';
|
||||
import { handleWriteCommand as _handleWriteCommand } from '../src/write-commands';
|
||||
import { handleMetaCommand } from '../src/meta-commands';
|
||||
import { WRITE_COMMANDS, READ_COMMANDS, META_COMMANDS, PAGE_CONTENT_COMMANDS, wrapUntrustedContent } from '../src/commands';
|
||||
import { consoleBuffer, networkBuffer, dialogBuffer, addConsoleEntry, addNetworkEntry, addDialogEntry, CircularBuffer } from '../src/buffers';
|
||||
import * as fs from 'fs';
|
||||
import { spawn } from 'child_process';
|
||||
|
|
@ -19,10 +20,41 @@ import * as path from 'path';
|
|||
|
||||
// Thin wrappers that bridge old test calls (bm as 3rd arg) to new signatures (session + bm)
|
||||
const handleReadCommand = (cmd: string, args: string[], b: BrowserManager) =>
|
||||
_handleReadCommand(cmd, args, b.getActiveSession());
|
||||
_handleReadCommand(cmd, args, b.getActiveSession(), b);
|
||||
const handleWriteCommand = (cmd: string, args: string[], b: BrowserManager) =>
|
||||
_handleWriteCommand(cmd, args, b.getActiveSession(), b);
|
||||
|
||||
// Chain routes every subcommand through the server's executeCommand pipeline in
|
||||
// production (the direct-dispatch fallback was deleted — it skipped the security
|
||||
// gates). Tests mirror the pipeline minimally: real handlers + trust-wrapping,
|
||||
// server-shaped {status, result} envelope.
|
||||
function makeChainExecute(b: BrowserManager) {
|
||||
return async (body: { command: string; args?: string[] }) => {
|
||||
const name = body.command;
|
||||
const args = body.args ?? [];
|
||||
try {
|
||||
let result: string;
|
||||
if (WRITE_COMMANDS.has(name)) {
|
||||
result = await _handleWriteCommand(name, args, b.getActiveSession(), b);
|
||||
} else if (READ_COMMANDS.has(name)) {
|
||||
result = await _handleReadCommand(name, args, b.getActiveSession(), b);
|
||||
if (PAGE_CONTENT_COMMANDS.has(name)) {
|
||||
result = wrapUntrustedContent(result, b.getCurrentUrl());
|
||||
}
|
||||
} else if (META_COMMANDS.has(name)) {
|
||||
result = await handleMetaCommand(name, args, b, async () => {});
|
||||
} else {
|
||||
return { status: 404, result: JSON.stringify({ error: `Unknown command: ${name}` }) };
|
||||
}
|
||||
return { status: 200, result };
|
||||
} catch (err: any) {
|
||||
return { status: 500, result: JSON.stringify({ error: err.message }) };
|
||||
}
|
||||
};
|
||||
}
|
||||
const chainMeta = (b: BrowserManager, args: string[]) =>
|
||||
handleMetaCommand('chain', args, b, async () => {}, null, { executeCommand: makeChainExecute(b) });
|
||||
|
||||
// ─── Pure arg-parser + result-conversion unit tests (no browser) ───
|
||||
describe('parseOutArgs / hasOutArg', () => {
|
||||
test('--out <path> splits the flag from the positional', () => {
|
||||
|
|
@ -804,7 +836,7 @@ describe('Chain', () => {
|
|||
['js', 'document.title'],
|
||||
['css', 'h1', 'color'],
|
||||
]);
|
||||
const result = await handleMetaCommand('chain', [commands], bm, async () => {});
|
||||
const result = await chainMeta(bm, [commands]);
|
||||
expect(result).toContain('[goto]');
|
||||
expect(result).toContain('Test Page - Basic');
|
||||
expect(result).toContain('[css]');
|
||||
|
|
@ -812,7 +844,7 @@ describe('Chain', () => {
|
|||
|
||||
test('chain wraps page-content sub-commands with trust markers', async () => {
|
||||
await handleWriteCommand('goto', [baseUrl + '/basic.html'], bm);
|
||||
const result = await handleMetaCommand('chain', ['text'], bm, async () => {});
|
||||
const result = await chainMeta(bm, ['text']);
|
||||
expect(result).toContain('BEGIN UNTRUSTED EXTERNAL CONTENT');
|
||||
expect(result).toContain('END UNTRUSTED EXTERNAL CONTENT');
|
||||
});
|
||||
|
|
@ -821,7 +853,7 @@ describe('Chain', () => {
|
|||
const commands = JSON.stringify([
|
||||
['goto', 'http://localhost:1/unreachable'],
|
||||
]);
|
||||
const result = await handleMetaCommand('chain', [commands], bm, async () => {});
|
||||
const result = await chainMeta(bm, [commands]);
|
||||
expect(result).toContain('[goto] ERROR:');
|
||||
expect(result).not.toContain('Unknown meta command');
|
||||
expect(result).not.toContain('Unknown read command');
|
||||
|
|
@ -1505,14 +1537,14 @@ describe('Errors', () => {
|
|||
test('chain with invalid JSON falls back to pipe format', async () => {
|
||||
// Non-JSON input is now treated as pipe-delimited format
|
||||
// 'not json' → [["not", "json"]] → "not" is unknown command → error in result
|
||||
const result = await handleMetaCommand('chain', ['not json'], bm, async () => {});
|
||||
const result = await chainMeta(bm, ['not json']);
|
||||
expect(result).toContain('ERROR');
|
||||
expect(result).toContain('Unknown command: not');
|
||||
});
|
||||
|
||||
test('chain with no arg throws', async () => {
|
||||
try {
|
||||
await handleMetaCommand('chain', [], bm, async () => {});
|
||||
await chainMeta(bm, []);
|
||||
expect(true).toBe(false);
|
||||
} catch (err: any) {
|
||||
expect(err.message).toContain('Usage');
|
||||
|
|
@ -2006,7 +2038,7 @@ describe('Chain with cookie-import', () => {
|
|||
const commands = JSON.stringify([
|
||||
['cookie-import', tmpCookies],
|
||||
]);
|
||||
const result = await handleMetaCommand('chain', [commands], bm, async () => {});
|
||||
const result = await chainMeta(bm, [commands]);
|
||||
expect(result).toContain('[cookie-import]');
|
||||
expect(result).toContain('Loaded 1 cookie');
|
||||
} finally {
|
||||
|
|
@ -2051,24 +2083,14 @@ describe('Network idle', () => {
|
|||
|
||||
describe('Chain pipe format', () => {
|
||||
test('pipe-delimited commands work', async () => {
|
||||
const result = await handleMetaCommand(
|
||||
'chain',
|
||||
[`goto ${baseUrl}/basic.html | js document.title`],
|
||||
bm,
|
||||
async () => {}
|
||||
);
|
||||
const result = await chainMeta(bm, [`goto ${baseUrl}/basic.html | js document.title`]);
|
||||
expect(result).toContain('[goto]');
|
||||
expect(result).toContain('[js]');
|
||||
expect(result).toContain('Test Page - Basic');
|
||||
});
|
||||
|
||||
test('pipe format with quoted args', async () => {
|
||||
const result = await handleMetaCommand(
|
||||
'chain',
|
||||
[`goto ${baseUrl}/forms.html | fill #email "pipe@test.com"`],
|
||||
bm,
|
||||
async () => {}
|
||||
);
|
||||
const result = await chainMeta(bm, [`goto ${baseUrl}/forms.html | fill #email "pipe@test.com"`]);
|
||||
expect(result).toContain('[fill]');
|
||||
expect(result).toContain('Filled');
|
||||
// Verify the fill actually worked
|
||||
|
|
@ -2081,18 +2103,13 @@ describe('Chain pipe format', () => {
|
|||
['goto', baseUrl + '/basic.html'],
|
||||
['js', 'document.title'],
|
||||
]);
|
||||
const result = await handleMetaCommand('chain', [commands], bm, async () => {});
|
||||
const result = await chainMeta(bm, [commands]);
|
||||
expect(result).toContain('[goto]');
|
||||
expect(result).toContain('Test Page - Basic');
|
||||
});
|
||||
|
||||
test('pipe format with unknown command includes error', async () => {
|
||||
const result = await handleMetaCommand(
|
||||
'chain',
|
||||
['bogus command'],
|
||||
bm,
|
||||
async () => {}
|
||||
);
|
||||
const result = await chainMeta(bm, ['bogus command']);
|
||||
expect(result).toContain('ERROR');
|
||||
expect(result).toContain('Unknown command: bogus');
|
||||
});
|
||||
|
|
@ -2569,14 +2586,14 @@ describe('Command aliases', () => {
|
|||
|
||||
test('setcontent alias routes to load-html via chain', async () => {
|
||||
// Chain canonicalizes aliases end-to-end; verifies the dispatch path
|
||||
const result = await handleMetaCommand('chain', [JSON.stringify([['setcontent', aliasFix]])], bm, async () => {});
|
||||
const result = await chainMeta(bm, [JSON.stringify([['setcontent', aliasFix]])]);
|
||||
expect(result).toContain('Loaded HTML:');
|
||||
const text = await handleReadCommand('text', [], bm);
|
||||
expect(text).toContain('alias routing ok');
|
||||
});
|
||||
|
||||
test('set-content (hyphenated) alias also routes', async () => {
|
||||
const result = await handleMetaCommand('chain', [JSON.stringify([['set-content', aliasFix]])], bm, async () => {});
|
||||
const result = await chainMeta(bm, [JSON.stringify([['set-content', aliasFix]])]);
|
||||
expect(result).toContain('Loaded HTML:');
|
||||
});
|
||||
});
|
||||
|
|
|
|||
|
|
@ -339,15 +339,18 @@ describe('frame --url ReDoS fix', () => {
|
|||
// ─── Task 7: watch-mode guard in chain command ───────────────────────────────
|
||||
|
||||
describe('chain command watch-mode guard', () => {
|
||||
it('chain loop contains isWatching() guard before write dispatch', () => {
|
||||
// Post-alias refactor: loop iterates over canonicalized `c of commands`.
|
||||
const block = sliceBetween(META_SRC, 'for (const c of commands)', 'Wait for network to settle');
|
||||
expect(block).toContain('isWatching');
|
||||
// The direct-dispatch fallback (which carried its own isWatching() guard)
|
||||
// was deleted — it skipped every OTHER server gate. Chain subcommands now
|
||||
// route exclusively through executeCommand -> handleCommandInternal, whose
|
||||
// watch-mode write gate covers them. Pin both halves of that contract.
|
||||
it('chain has no direct-dispatch fallback (executeCommand is mandatory)', () => {
|
||||
const block = sliceBetween(META_SRC, 'const executeCmd = opts?.executeCommand', 'Wait for network to settle');
|
||||
expect(block).toContain('chain requires the browse server (no executeCommand context)');
|
||||
expect(block).not.toContain('handleWriteCommand(');
|
||||
});
|
||||
|
||||
it('chain loop BLOCKED message appears for write commands in watch mode', () => {
|
||||
const block = sliceBetween(META_SRC, 'for (const c of commands)', 'Wait for network to settle');
|
||||
expect(block).toContain('BLOCKED: write commands disabled in watch mode');
|
||||
it('server pipeline blocks write commands in watch mode (covers chain subcommands)', () => {
|
||||
expect(SERVER_SRC).toMatch(/isWatching\(\)\s*&&\s*isWriteInvocation\(command, args\)/);
|
||||
});
|
||||
});
|
||||
|
||||
|
|
|
|||
Loading…
Reference in New Issue