mirror of
https://github.com/garrytan/gstack.git
synced 2026-07-09 09:38:09 +02:00
fix: harden auth-token validation, TDZ try/catch, lockfile path safety
Three security hardening fixes from /ship adversarial review: 1. AUTH_TOKEN unicode-whitespace bypass (server.ts:67-83). Old: `process.env.AUTH_TOKEN?.trim() || randomUUID()` only stripped ASCII whitespace. A misconfigured embedder shipping AUTH_TOKEN=$'' (BOM) or $'' (zero-width space) would silently get a one-character bearer secret. New `sanitizeAuthToken()` strips all unicode whitespace via regex and requires >= 16 chars after stripping; anything shorter falls back to crypto.randomUUID(). Same sanitizer used by `resolveConfigFromEnv()` so the embedder path is hardened too. 2. security-classifier.ts checkTranscript safety net. `resolveClaudeCommand()` and `spawn()` can throw under transient conditions (PATH probe failure, posix_spawn ENOMEM). Old code let the throw propagate and rejected the Promise with a raw exception. Now wrapped in try/catch that calls finish() with a degraded signal, matching the graceful-degradation contract the layer already promises for missing-CLI / exit-nonzero / parse-error. 3. cleanSingletonLocks defensive guard tightened (config.ts). Old: basename === 'chromium-profile' OR userDataDir === $CHROMIUM_PROFILE. The second branch was env-controlled and the first was bypassable by passing a relative path that resolved to chromium-profile via CWD drift. New guard: refuses relative paths outright, resolves both sides via path.resolve(), and only accepts the env-match path when $CHROMIUM_PROFILE is itself absolute. Test updates: replace the old `.trim()` test with three new cases covering unicode-whitespace stripping, short-token rejection, and zero-width-only rejection (server-factory.test.ts). Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
+19
-8
@@ -186,24 +186,35 @@ export function resolveChromiumProfile(explicit?: string): string {
|
|||||||
* ProcessSingleton refuses to start when these exist from a prior crash
|
* ProcessSingleton refuses to start when these exist from a prior crash
|
||||||
* (SIGKILL, hard crash, etc.) since they point at a PID that no longer exists.
|
* (SIGKILL, hard crash, etc.) since they point at a PID that no longer exists.
|
||||||
*
|
*
|
||||||
* Defensive guard: refuses to operate unless the directory basename is
|
* Defensive guard: refuses to operate unless ALL of these hold:
|
||||||
* 'chromium-profile' OR the path matches the explicit CHROMIUM_PROFILE env
|
* 1. `userDataDir` is an absolute path (no CWD-relative footguns)
|
||||||
* value. Prevents accidentally deleting lock files from an unrelated
|
* 2. basename is exactly 'chromium-profile' OR the absolute path matches
|
||||||
* directory if profile resolution is misconfigured upstream.
|
* the absolute form of $CHROMIUM_PROFILE env value
|
||||||
|
*
|
||||||
|
* Prevents accidentally deleting lock files from an unrelated directory if
|
||||||
|
* profile resolution is misconfigured upstream (CWD drift, env injection).
|
||||||
*
|
*
|
||||||
* Caller MUST ensure external coordination has already guaranteed no live
|
* Caller MUST ensure external coordination has already guaranteed no live
|
||||||
* peer is using this profile (gbd.lock for gbrowser; single-instance CLI
|
* peer is using this profile (gbd.lock for gbrowser; single-instance CLI
|
||||||
* check for gstack).
|
* check for gstack).
|
||||||
*/
|
*/
|
||||||
export function cleanSingletonLocks(userDataDir: string): void {
|
export function cleanSingletonLocks(userDataDir: string): void {
|
||||||
const basename = path.basename(userDataDir);
|
if (!path.isAbsolute(userDataDir)) {
|
||||||
|
console.warn(`[browse] cleanSingletonLocks: refusing relative path: ${userDataDir}`);
|
||||||
|
return;
|
||||||
|
}
|
||||||
|
const resolved = path.resolve(userDataDir);
|
||||||
|
const basename = path.basename(resolved);
|
||||||
const explicitProfile = process.env.CHROMIUM_PROFILE;
|
const explicitProfile = process.env.CHROMIUM_PROFILE;
|
||||||
const isSafe = basename === 'chromium-profile' || userDataDir === explicitProfile;
|
const explicitAbs = explicitProfile && path.isAbsolute(explicitProfile)
|
||||||
|
? path.resolve(explicitProfile)
|
||||||
|
: null;
|
||||||
|
const isSafe = basename === 'chromium-profile' || (explicitAbs !== null && resolved === explicitAbs);
|
||||||
if (!isSafe) {
|
if (!isSafe) {
|
||||||
console.warn(`[browse] cleanSingletonLocks: refusing to clean unrecognized profile dir: ${userDataDir}`);
|
console.warn(`[browse] cleanSingletonLocks: refusing to clean unrecognized profile dir: ${resolved}`);
|
||||||
return;
|
return;
|
||||||
}
|
}
|
||||||
for (const lockFile of ['SingletonLock', 'SingletonSocket', 'SingletonCookie']) {
|
for (const lockFile of ['SingletonLock', 'SingletonSocket', 'SingletonCookie']) {
|
||||||
safeUnlinkQuiet(path.join(userDataDir, lockFile));
|
safeUnlinkQuiet(path.join(resolved, lockFile));
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -511,16 +511,29 @@ export async function checkTranscript(params: {
|
|||||||
resolve(signal);
|
resolve(signal);
|
||||||
};
|
};
|
||||||
|
|
||||||
const claude = resolveClaudeCommand();
|
// Wrap resolveClaudeCommand + spawn in try/catch so any unexpected
|
||||||
|
// throw (PATH probe failure, transient FS error) degrades gracefully
|
||||||
|
// instead of rejecting the Promise with a raw exception.
|
||||||
|
let claude: ReturnType<typeof resolveClaudeCommand>;
|
||||||
|
try {
|
||||||
|
claude = resolveClaudeCommand();
|
||||||
|
} catch (err: any) {
|
||||||
|
return finish({ layer: 'transcript_classifier', confidence: 0, meta: { degraded: true, reason: `resolve_error_${err?.message ?? 'unknown'}` } });
|
||||||
|
}
|
||||||
if (!claude) {
|
if (!claude) {
|
||||||
return finish({ layer: 'transcript_classifier', confidence: 0, meta: { degraded: true, reason: 'claude_cli_not_found' } });
|
return finish({ layer: 'transcript_classifier', confidence: 0, meta: { degraded: true, reason: 'claude_cli_not_found' } });
|
||||||
}
|
}
|
||||||
const p = spawn(claude.command, [
|
let p: ReturnType<typeof spawn>;
|
||||||
...claude.argsPrefix,
|
try {
|
||||||
'-p', prompt,
|
p = spawn(claude.command, [
|
||||||
'--model', HAIKU_MODEL,
|
...claude.argsPrefix,
|
||||||
'--output-format', 'json',
|
'-p', prompt,
|
||||||
], { stdio: ['ignore', 'pipe', 'pipe'], cwd: os.tmpdir() });
|
'--model', HAIKU_MODEL,
|
||||||
|
'--output-format', 'json',
|
||||||
|
], { stdio: ['ignore', 'pipe', 'pipe'], cwd: os.tmpdir() });
|
||||||
|
} catch (err: any) {
|
||||||
|
return finish({ layer: 'transcript_classifier', confidence: 0, meta: { degraded: true, reason: `spawn_throw_${err?.message ?? 'unknown'}` } });
|
||||||
|
}
|
||||||
|
|
||||||
p.stdout.on('data', (d: Buffer) => (stdout += d.toString()));
|
p.stdout.on('data', (d: Buffer) => (stdout += d.toString()));
|
||||||
p.on('exit', (code) => {
|
p.on('exit', (code) => {
|
||||||
|
|||||||
+19
-4
@@ -67,9 +67,21 @@ initAuditLog(config.auditLog);
|
|||||||
// ─── Auth ───────────────────────────────────────────────────────
|
// ─── Auth ───────────────────────────────────────────────────────
|
||||||
// AUTH_TOKEN is injectable via process.env.AUTH_TOKEN so embedders
|
// AUTH_TOKEN is injectable via process.env.AUTH_TOKEN so embedders
|
||||||
// (gbrowser's gbd daemon spawn) can pre-allocate the token and hand it to
|
// (gbrowser's gbd daemon spawn) can pre-allocate the token and hand it to
|
||||||
// the Bun child via env. Whitespace-only values fall back to randomUUID so
|
// the Bun child via env.
|
||||||
// the security boundary is never silently weakened by misconfiguration.
|
//
|
||||||
const AUTH_TOKEN = (process.env.AUTH_TOKEN?.trim()) || crypto.randomUUID();
|
// Validation: require >= 16 chars after stripping ALL unicode whitespace
|
||||||
|
// (not just ASCII — .trim() misses U+200B / U+FEFF / U+00A0 / etc., which
|
||||||
|
// would otherwise let a misconfigured embedder ship a one-character BOM as
|
||||||
|
// the bearer secret). Reject tokens that are too short or contain only
|
||||||
|
// whitespace; fall back to randomUUID so the security boundary is never
|
||||||
|
// silently weakened by misconfiguration.
|
||||||
|
function sanitizeAuthToken(raw: string | undefined): string | null {
|
||||||
|
if (!raw) return null;
|
||||||
|
const stripped = raw.replace(/[\s -]/g, '');
|
||||||
|
if (stripped.length < 16) return null;
|
||||||
|
return stripped;
|
||||||
|
}
|
||||||
|
const AUTH_TOKEN = sanitizeAuthToken(process.env.AUTH_TOKEN) || crypto.randomUUID();
|
||||||
initRegistry(AUTH_TOKEN);
|
initRegistry(AUTH_TOKEN);
|
||||||
const BROWSE_PORT = parseInt(process.env.BROWSE_PORT || '0', 10);
|
const BROWSE_PORT = parseInt(process.env.BROWSE_PORT || '0', 10);
|
||||||
const IDLE_TIMEOUT_MS = parseInt(process.env.BROWSE_IDLE_TIMEOUT || '1800000', 10); // 30 min
|
const IDLE_TIMEOUT_MS = parseInt(process.env.BROWSE_IDLE_TIMEOUT || '1800000', 10); // 30 min
|
||||||
@@ -178,7 +190,10 @@ export function resolveConfigFromEnv(): Omit<ServerConfig, 'browserManager' | 's
|
|||||||
config: ReturnType<typeof resolveConfig>;
|
config: ReturnType<typeof resolveConfig>;
|
||||||
} {
|
} {
|
||||||
return {
|
return {
|
||||||
authToken: (process.env.AUTH_TOKEN?.trim()) || crypto.randomUUID(),
|
// Same sanitizer as the module-level AUTH_TOKEN: strips ALL unicode
|
||||||
|
// whitespace and rejects tokens shorter than 16 chars so a misconfigured
|
||||||
|
// embedder can't ship a BOM/zero-width as the bearer secret.
|
||||||
|
authToken: sanitizeAuthToken(process.env.AUTH_TOKEN) || crypto.randomUUID(),
|
||||||
browsePort: parseInt(process.env.BROWSE_PORT || '0', 10),
|
browsePort: parseInt(process.env.BROWSE_PORT || '0', 10),
|
||||||
idleTimeoutMs: parseInt(process.env.BROWSE_IDLE_TIMEOUT || '1800000', 10),
|
idleTimeoutMs: parseInt(process.env.BROWSE_IDLE_TIMEOUT || '1800000', 10),
|
||||||
config: resolveConfig(),
|
config: resolveConfig(),
|
||||||
|
|||||||
@@ -53,12 +53,41 @@ describe('server.ts factory API surface', () => {
|
|||||||
}
|
}
|
||||||
});
|
});
|
||||||
|
|
||||||
test('AUTH_TOKEN value is trimmed', () => {
|
test('AUTH_TOKEN whitespace is stripped (including unicode whitespace)', () => {
|
||||||
const orig = process.env.AUTH_TOKEN;
|
const orig = process.env.AUTH_TOKEN;
|
||||||
process.env.AUTH_TOKEN = ' padded-token ';
|
// 22 chars after stripping leading/trailing whitespace including BOM (U+FEFF)
|
||||||
|
// and zero-width space (U+200B), so passes the 16-char minimum.
|
||||||
|
process.env.AUTH_TOKEN = ' padded-token-abc123xyz ';
|
||||||
try {
|
try {
|
||||||
const cfg = resolveConfigFromEnv();
|
const cfg = resolveConfigFromEnv();
|
||||||
expect(cfg.authToken).toBe('padded-token');
|
expect(cfg.authToken).toBe('padded-token-abc123xyz');
|
||||||
|
} finally {
|
||||||
|
if (orig === undefined) delete process.env.AUTH_TOKEN;
|
||||||
|
else process.env.AUTH_TOKEN = orig;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('AUTH_TOKEN shorter than 16 chars after stripping falls back to randomUUID', () => {
|
||||||
|
const orig = process.env.AUTH_TOKEN;
|
||||||
|
// Only 5 chars of content — too short for the 16-char minimum.
|
||||||
|
process.env.AUTH_TOKEN = 'short';
|
||||||
|
try {
|
||||||
|
const cfg = resolveConfigFromEnv();
|
||||||
|
// Must be a UUID, not the rejected short token.
|
||||||
|
expect(cfg.authToken).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/);
|
||||||
|
} finally {
|
||||||
|
if (orig === undefined) delete process.env.AUTH_TOKEN;
|
||||||
|
else process.env.AUTH_TOKEN = orig;
|
||||||
|
}
|
||||||
|
});
|
||||||
|
|
||||||
|
test('AUTH_TOKEN of only zero-width unicode whitespace falls back to randomUUID', () => {
|
||||||
|
const orig = process.env.AUTH_TOKEN;
|
||||||
|
// U+200B (ZWSP), U+FEFF (BOM), U+00A0 (NBSP) — would pass .trim() but not the unicode-aware strip.
|
||||||
|
process.env.AUTH_TOKEN = ' ';
|
||||||
|
try {
|
||||||
|
const cfg = resolveConfigFromEnv();
|
||||||
|
expect(cfg.authToken).toMatch(/^[0-9a-f]{8}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{4}-[0-9a-f]{12}$/);
|
||||||
} finally {
|
} finally {
|
||||||
if (orig === undefined) delete process.env.AUTH_TOKEN;
|
if (orig === undefined) delete process.env.AUTH_TOKEN;
|
||||||
else process.env.AUTH_TOKEN = orig;
|
else process.env.AUTH_TOKEN = orig;
|
||||||
|
|||||||
Reference in New Issue
Block a user