From 1480052e77556645b55bc11af31c54a37bc7ccea Mon Sep 17 00:00:00 2001 From: Nehr <127654909+AgileInnov8tor@users.noreply.github.com> Date: Mon, 13 Jul 2026 01:28:13 +0700 Subject: [PATCH] fix(review): close Grok packaging fail-open paths from code review Fail closed on ARCHIVE_PATH allowlist when SPAWN_PATH is missing, require non-empty monorepo root before runtime links, drop default --always-approve, prefer --prompt-file for large Grok benchmark prompts, and make compat-audit refuse theater-green when required bridge skills are absent. --- bin/gstack-grok-compat-audit | 48 +++++++++++++++++++++--------- scripts/resolvers/spec-spawn.ts | 36 ++++++++++++++--------- setup | 17 ++++++++++- test/helpers/providers/grok.ts | 52 ++++++++++++++++++++++++--------- 4 files changed, 111 insertions(+), 42 deletions(-) diff --git a/bin/gstack-grok-compat-audit b/bin/gstack-grok-compat-audit index 8f6890456..67b3df488 100755 --- a/bin/gstack-grok-compat-audit +++ b/bin/gstack-grok-compat-audit @@ -16,7 +16,7 @@ import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; -import { execSync } from 'child_process'; +import { execFileSync } from 'child_process'; const ROOT = path.resolve(import.meta.dir, '..'); const args = process.argv.slice(2); @@ -227,16 +227,24 @@ function auditBridges() { // Install prose alone must not claim primary READY when CLI missing const area = 'bridge-policy'; - // benchmark-models: grok provider must exist in binary + // Prefer installed runtime root binary (what users run); fall back to monorepo. + const installedBench = path.join(GSTACK_ROOT, 'bin', 'gstack-model-benchmark'); + const monorepoBench = path.join(ROOT, 'bin', 'gstack-model-benchmark'); + const benchBin = exists(installedBench) ? installedBench : monorepoBench; + if (!exists(installedBench) && exists(monorepoBench)) { + warn(area, 'benchmark: using monorepo binary (installed runtime bin missing) — packaging incomplete?'); + } try { - const help = execSync(`"${path.join(ROOT, 'bin/gstack-model-benchmark')}" --prompt "x" --models grok --dry-run 2>&1`, { + // No shell: argv list avoids monorepo-path injection. + const help = execFileSync(benchBin, ['--prompt', 'x', '--models', 'grok', '--dry-run'], { encoding: 'utf-8', timeout: 15000, + maxBuffer: 2 * 1024 * 1024, }); if (/unknown provider.*grok/i.test(help)) { fail(area, 'benchmark: grok still unknown provider'); } else if (/grok:\s*(OK|NOT READY)/i.test(help)) { - ok(area, 'benchmark: grok provider wired (dry-run READY/NOT READY is boolean auth)'); + ok(area, `benchmark: grok provider wired via ${benchBin} (dry-run READY/NOT READY is boolean auth)`); } else { warn(area, `benchmark dry-run unexpected output: ${help.slice(0, 200)}`); } @@ -248,30 +256,34 @@ function auditBridges() { else warn(area, `benchmark dry-run error: ${String(msg).slice(0, 200)}`); } - // spec: Grok package should not default to claude -p outside execute-claude + // spec: at least one installed package must exist and use Grok-native spawn + let specSeen = 0; for (const name of ['gstack-spec', 'spec']) { const skill = path.join(SKILLS_DIR, name, 'SKILL.md'); if (!exists(skill)) continue; + specSeen++; const body = readText(skill); if (/\$\(cat\s+["']?\$ARCHIVE/.test(body) || /grok[^\n]*\$\(cat/.test(body)) { fail(area, `${name}: banned $(cat …) into grok argv`); } - if (/Spawned:.*claude -p/.test(body) && !/--execute-claude/.test(body) && !/optional/i.test(body)) { - // Default path still Claude — fail for Grok host package - if (/grok --prompt-file/.test(body) || /Spawn \*\*Grok\*\*/.test(body)) { - ok(area, `${name}: Grok-native spawn present`); - } else { - fail(area, `${name}: default execute still only claude -p (no Grok spawn)`); - } - } else if (/grok --prompt-file/.test(body)) { - ok(area, `${name}: uses researched grok --prompt-file`); + if (/grok --prompt-file/.test(body) || /Spawn \*\*Grok\*\*/.test(body)) { + ok(area, `${name}: Grok-native spawn present`); + } else if (/Spawned:.*claude -p/.test(body) && !/--execute-claude/.test(body)) { + fail(area, `${name}: default execute still only claude -p (no Grok spawn)`); + } else { + fail(area, `${name}: missing researched grok --prompt-file spawn`); } } + if (specSeen === 0) { + fail(area, 'spec: neither gstack-spec nor spec package installed under skills dir'); + } // setup-gbrain: must not hard-require which claude for success + let gbrainSeen = 0; for (const name of ['gstack-setup-gbrain', 'setup-gbrain']) { const skill = path.join(SKILLS_DIR, name, 'SKILL.md'); if (!exists(skill)) continue; + gbrainSeen++; const body = readText(skill); if (/Grok Build success path/.test(body) || /no Claude required/i.test(body)) { ok(area, `${name}: Grok success path documented`); @@ -279,11 +291,16 @@ function auditBridges() { warn(area, `${name}: missing explicit Grok success path (CLI+AGENTS)`); } } + if (gbrainSeen === 0) { + warn(area, 'setup-gbrain: package not installed (bridge not audited)'); + } // pair-agent: local must not hard-require ngrok; remote has install+security+teardown + let pairSeen = 0; for (const name of ['gstack-pair-agent', 'pair-agent']) { const skill = path.join(SKILLS_DIR, name, 'SKILL.md'); if (!exists(skill)) continue; + pairSeen++; const body = readText(skill); if (/teardown/i.test(body) && /ngrok/i.test(body) && /127\.0\.0\.1|security/i.test(body)) { ok(area, `${name}: remote ngrok install+security+teardown present`); @@ -292,6 +309,9 @@ function auditBridges() { } if (/DEPENDENT/.test(body)) ok(area, `${name}: honest DEPENDENT labeling`); } + if (pairSeen === 0) { + warn(area, 'pair-agent: package not installed (bridge not audited)'); + } } // ─── Main ───────────────────────────────────────────────────── diff --git a/scripts/resolvers/spec-spawn.ts b/scripts/resolvers/spec-spawn.ts index 3ff1c24b2..d9b43fd7b 100644 --- a/scripts/resolvers/spec-spawn.ts +++ b/scripts/resolvers/spec-spawn.ts @@ -25,29 +25,37 @@ if [ ! -f "$HOME/.grok/auth.json" ] && [ -z "\${XAI_API_KEY:-}\${GROK_API_KEY:-} echo "STOP: Grok not authenticated (no ~/.grok/auth.json and no XAI_API_KEY/GROK_API_KEY). Log in via \`grok\`, or use --no-execute." exit 1 fi -# ARCHIVE_PATH must stay under SPAWN_PATH parent or the gstack projects allowlist -ARCHIVE_REAL=$(cd "$(dirname "$ARCHIVE_PATH")" && pwd -P)/$(basename "$ARCHIVE_PATH") +# ARCHIVE_PATH must stay under SPAWN_PATH or the gstack projects allowlist (fail closed). +if [ ! -f "$ARCHIVE_PATH" ]; then + echo "STOP: ARCHIVE_PATH missing: $ARCHIVE_PATH"; exit 1 +fi +if command -v realpath >/dev/null 2>&1; then + ARCHIVE_REAL=$(realpath "$ARCHIVE_PATH") +else + ARCHIVE_REAL=$(cd "$(dirname "$ARCHIVE_PATH")" && pwd -P)/$(basename "$ARCHIVE_PATH") +fi SPAWN_REAL=$(cd "$SPAWN_PATH" 2>/dev/null && pwd -P || echo "") +if [ -z "$SPAWN_REAL" ]; then + echo "STOP: SPAWN_PATH is not a real directory: $SPAWN_PATH"; exit 1 +fi +STATE_PROJECTS="\${GSTACK_STATE_ROOT:-\$HOME/.gstack}/projects" case "$ARCHIVE_REAL" in - "$HOME/.gstack/projects"/*|"$HOME/.gstack/projects"/*/*) ;; # allowlisted archive dir + "$STATE_PROJECTS"/*|"$SPAWN_REAL"/*) ;; # allowlisted *) - if [ -n "$SPAWN_REAL" ]; then - case "$ARCHIVE_REAL" in - "$SPAWN_REAL"/*) ;; - *) echo "STOP: ARCHIVE_PATH realpath not under SPAWN_PATH or allowlisted archive dir."; exit 1 ;; - esac - fi + echo "STOP: ARCHIVE_PATH realpath not under SPAWN_PATH or allowlisted archive dir ($STATE_PROJECTS)."; exit 1 ;; esac \`\`\` -**Security:** \`--always-approve\` auto-approves tool use (elevated). Only spawn -when the user confirmed the D16 gate. Spec archives must not contain secrets. -Third-party note: archive body is sent to xAI for processing. +**Security:** default spawn does **not** pass \`--always-approve\` (elevated +auto-approve). Only spawn after the user confirmed the D16 gate. If the user +explicitly opts into unattended tool use, append \`--always-approve\` to the +command below — never enable it by default. Spec archives must not contain +secrets. Third-party note: archive body is sent to xAI for processing. \`\`\`bash -# Prefer --prompt-file (researched). Optional elevated approve is documented opt-in. -(cd "$SPAWN_PATH" && grok --prompt-file "$ARCHIVE_PATH" --cwd "$SPAWN_PATH" --always-approve 2>&1) & +# Prefer --prompt-file (researched). Elevated --always-approve is opt-in only. +(cd "$SPAWN_PATH" && grok --prompt-file "$ARCHIVE_PATH" --cwd "$SPAWN_PATH" 2>&1) & SPAWN_PID=$! echo "Spawned: PID $SPAWN_PID in $SPAWN_PATH (branch $SPAWN_BRANCH)" echo "Follow with: cd $SPAWN_PATH && grok --continue" diff --git a/setup b/setup index d72f7bcab..3366ae809 100755 --- a/setup +++ b/setup @@ -1012,8 +1012,23 @@ _grok_link_under_monorepo() { fi local src_real monorepo_real - src_real=$(cd "$(dirname "$src")" 2>/dev/null && pwd -P)/$(basename "$src") monorepo_real=$(cd "$monorepo_root" 2>/dev/null && pwd -P) + # Fail closed: empty monorepo_real makes "$monorepo_real"/* expand to /* and + # match every absolute path — never allow that. + if [ -z "$monorepo_real" ] || [ ! -d "$monorepo_real" ]; then + echo "error: refusing Grok runtime link — monorepo root unresolved: $monorepo_root" >&2 + return 1 + fi + # Prefer full realpath when available so a final-component symlink cannot escape. + if command -v realpath >/dev/null 2>&1; then + src_real=$(realpath "$src" 2>/dev/null) || src_real="" + else + src_real=$(cd "$(dirname "$src")" 2>/dev/null && pwd -P)/$(basename "$src") + fi + if [ -z "$src_real" ]; then + echo "error: refusing Grok runtime link — cannot realpath src: $src" >&2 + return 1 + fi case "$src_real" in "$monorepo_real"|"$monorepo_real"/*) ;; *) diff --git a/test/helpers/providers/grok.ts b/test/helpers/providers/grok.ts index a8882ed1c..fd7d74c4c 100644 --- a/test/helpers/providers/grok.ts +++ b/test/helpers/providers/grok.ts @@ -1,12 +1,12 @@ import type { ProviderAdapter, RunOpts, RunResult, AvailabilityCheck } from './types'; import { estimateCostUsd } from '../pricing'; -import { execFileSync } from 'child_process'; +import { execFileSync, spawnSync } from 'child_process'; import * as fs from 'fs'; import * as path from 'path'; import * as os from 'os'; /** - * Grok adapter — wraps the `grok` CLI via -p / --single. + * Grok adapter — wraps the `grok` CLI via -p / --single / --prompt-file. * * Auth readiness is boolean only: CLI present + (~/.grok/auth.json OR * XAI_API_KEY / GROK_API_KEY env names present). Never log token values. @@ -16,12 +16,14 @@ export class GrokAdapter implements ProviderAdapter { readonly family = 'grok' as const; async available(): Promise { - // Boolean PATH presence only — never log secrets - let hasBinary = false; - try { - execFileSync('which', ['grok'], { encoding: 'utf-8', stdio: ['ignore', 'pipe', 'ignore'] }); - hasBinary = true; - } catch { + // Boolean PATH presence only — never log secrets. Bound to ≤2s like peer adapters. + const which = spawnSync('sh', ['-c', 'command -v grok'], { + timeout: 2000, + encoding: 'utf-8', + stdio: ['ignore', 'pipe', 'ignore'], + }); + let hasBinary = which.status === 0; + if (!hasBinary) { try { execFileSync('grok', ['--version'], { encoding: 'utf-8', @@ -55,8 +57,22 @@ export class GrokAdapter implements ProviderAdapter { async run(opts: RunOpts): Promise { const start = Date.now(); - // Short single-turn: -p / --single. Prefer file-free short prompts for benchmark. - const args = ['--single', opts.prompt]; + // Prefer --prompt-file for multi-line / large prompts (ARG_MAX + quoting). + // Short single-line prompts use --single to avoid temp files. + const useFile = + opts.prompt.includes('\n') || opts.prompt.length > 2000 || Buffer.byteLength(opts.prompt, 'utf8') > 2000; + let promptFile: string | null = null; + let args: string[]; + if (useFile) { + promptFile = path.join( + os.tmpdir(), + `gstack-grok-bench-${process.pid}-${Date.now()}-${Math.random().toString(36).slice(2)}.txt`, + ); + fs.writeFileSync(promptFile, opts.prompt, 'utf-8'); + args = ['--prompt-file', promptFile, '--cwd', opts.workdir]; + } else { + args = ['--single', opts.prompt]; + } if (opts.model) args.push('--model', opts.model); if (opts.extraArgs) args.push(...opts.extraArgs); @@ -79,19 +95,29 @@ export class GrokAdapter implements ProviderAdapter { const durationMs = Date.now() - start; const e = err as { code?: string; stderr?: Buffer; signal?: string; message?: string }; const stderr = e.stderr?.toString() ?? ''; + // Never surface raw stderr for auth paths (may contain token-shaped text). + const safeSlice = (s: string) => s.replace(/[A-Za-z0-9_\-]{20,}/g, '[redacted]').slice(0, 200); if (e.signal === 'SIGTERM' || e.code === 'ETIMEDOUT') { return this.emptyResult(durationMs, { code: 'timeout', reason: `exceeded ${opts.timeoutMs}ms` }, opts.model); } if (/unauthorized|auth|login|api.?key/i.test(stderr)) { - return this.emptyResult(durationMs, { code: 'auth', reason: stderr.slice(0, 400) }, opts.model); + return this.emptyResult(durationMs, { code: 'auth', reason: 'authentication failed (details redacted)' }, opts.model); } if (/rate[- ]?limit|429/i.test(stderr)) { - return this.emptyResult(durationMs, { code: 'rate_limit', reason: stderr.slice(0, 400) }, opts.model); + return this.emptyResult(durationMs, { code: 'rate_limit', reason: safeSlice(stderr) }, opts.model); } if (/ENOENT|not found/i.test(e.message ?? '') || e.code === 'ENOENT') { return this.emptyResult(durationMs, { code: 'binary_missing', reason: 'grok CLI not found' }, opts.model); } - return this.emptyResult(durationMs, { code: 'unknown', reason: (e.message ?? stderr ?? 'unknown').slice(0, 400) }, opts.model); + return this.emptyResult(durationMs, { code: 'unknown', reason: safeSlice(e.message ?? stderr ?? 'unknown') }, opts.model); + } finally { + if (promptFile) { + try { + fs.unlinkSync(promptFile); + } catch { + // best-effort cleanup of temp prompt file + } + } } }