mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-09 14:38:59 +02:00
fix: red-team findings — verify-gate identity, single quoting authority, Windows paths
Red-team pass over the hardened diff (several findings empirically verified by the reviewer before reporting): - KNOWN_HOOKS gains the sixth identity: gstack-verify-gate (README-documented opt-in Stop hook). A tag-stripped verify-gate entry previously survived prune-stale --all and errored at the end of EVERY turn after uninstall deleted the install root — the exact phantom-hook class this branch fixes. Uninstall also sweeps its tagged form. - add-event is now the single quoting authority: every registered command is normalized through the same gsQuoteCmd/gsStripWrap round-trip the healer uses. Pre-fix, only SessionStart got caller-side quoting — a spaced/metachar canonical root registered broken plan-tune/AUQ/timeline hooks that the very next heal rewrote (the codebase disagreed with its own registrations). - Windows: MSYS-form paths (/c/Users/...) are drive-translated for fs checks only (gsWinPath) — native bun resolved them drive-relative, so the heal judged every LIVE Windows hook dead and pruned it. The three AskUserQuestion hooks and the Stop hook now also get the mandatory 'bash ' prefix on Windows (previously only SessionStart did; extensionless bash shims otherwise hit the file-association dialog). - CANONICAL_GSTACK_ROOT falls back to $HOME/.claude/skills/gstack when a CLAUDE_CONFIG_DIR-derived root was never installed (the installer hardcodes the home path — split-brain left such users permanently hookless). - prune-stale preserves foreign entries that STARTED empty (they were silently deleted, uncounted, on every heal). - The timeline Stop registration and its list-sources guard join the zero-silent-mutations contract (stderr attached). Tests: verify-gate tag-stripped heal+sweep, started-empty preservation, add-event quoting-authority round-trip. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
19eed1b392
commit
6a3cf611ea
@@ -81,7 +81,8 @@ var KNOWN_HOOKS = {
|
||||
"question-preference-hook": { source: "plan-tune-cathedral", event: "PreToolUse", matcher: "(AskUserQuestion|mcp__.*__AskUserQuestion)", relpath: "hosts/claude/hooks/question-preference-hook" },
|
||||
"auq-error-fallback-hook": { source: "auq-error-fallback", event: "PostToolUse", matcher: "(AskUserQuestion|mcp__.*__AskUserQuestion)", relpath: "hosts/claude/hooks/auq-error-fallback-hook" },
|
||||
"timeline-stop-hook": { source: "gstack-timeline-stop", event: "Stop", matcher: "", relpath: "hosts/claude/hooks/timeline-stop-hook" },
|
||||
"gstack-session-update": { source: "gstack-session-update", event: "SessionStart", matcher: "", relpath: "bin/gstack-session-update" }
|
||||
"gstack-session-update": { source: "gstack-session-update", event: "SessionStart", matcher: "", relpath: "bin/gstack-session-update" },
|
||||
"gstack-verify-gate": { source: "verify-gate", event: "Stop", matcher: "", relpath: "bin/gstack-verify-gate" }
|
||||
};
|
||||
function gsHadBashPrefix(c) { return String(c == null ? "" : c).trim().indexOf("bash ") === 0; }
|
||||
function gsStripWrap(c) {
|
||||
@@ -105,9 +106,18 @@ function gsOwnedRow(cmd, event, matcher) {
|
||||
if (row.matcher && (matcher || "") !== row.matcher) return null;
|
||||
return row;
|
||||
}
|
||||
function gsWinPath(p) {
|
||||
// Git Bash writes MSYS-form paths (/c/Users/...) into settings.json, but
|
||||
// native bun resolves them drive-relative (C:\c\Users\...) -- translate for
|
||||
// fs calls only; stored commands keep the form the firing shell expects.
|
||||
if (process.platform === "win32" && /^\/[A-Za-z]\//.test(p)) {
|
||||
return p.charAt(1) + ":" + p.slice(2);
|
||||
}
|
||||
return p;
|
||||
}
|
||||
function gsIsAlive(cmd) {
|
||||
var fs = require("fs");
|
||||
var p = gsStripWrap(cmd);
|
||||
var p = gsWinPath(gsStripWrap(cmd));
|
||||
if (!p) return false;
|
||||
try {
|
||||
if (process.platform === "win32") return fs.existsSync(p);
|
||||
@@ -364,7 +374,12 @@ case "$ACTION" in
|
||||
if (!settings.hooks) settings.hooks = {};
|
||||
if (!settings.hooks[event]) settings.hooks[event] = [];
|
||||
|
||||
const hookEntry = { type: "command", command: cmd };
|
||||
// add-event is the single quoting authority: normalize the command
|
||||
// through the same round-trip the healer uses so metachar paths are
|
||||
// registered in the escaped-quoted form from the start (a caller-side
|
||||
// quoting step would drift per call site).
|
||||
const cmdNorm = gsQuoteCmd(gsStripWrap(cmd), gsHadBashPrefix(cmd));
|
||||
const hookEntry = { type: "command", command: cmdNorm };
|
||||
if (timeoutRaw) {
|
||||
const n = Number(timeoutRaw);
|
||||
if (Number.isFinite(n) && n > 0) hookEntry.timeout = n;
|
||||
@@ -379,12 +394,12 @@ case "$ACTION" in
|
||||
for (const entry of settings.hooks[event]) {
|
||||
if ((entry.matcher || "") !== matcher) continue;
|
||||
if (!Array.isArray(entry.hooks)) continue;
|
||||
let idx = entry.hooks.findIndex(h => h && h.command === cmd);
|
||||
let idx = entry.hooks.findIndex(h => h && h.command === cmdNorm);
|
||||
if (idx < 0) {
|
||||
idx = entry.hooks.findIndex(h => {
|
||||
if (!h || !h.command) return false;
|
||||
const row = gsOwnedRow(h.command, event, entry.matcher || "");
|
||||
return !!row && gsBaseOf(h.command) === gsBaseOf(cmd);
|
||||
return !!row && gsBaseOf(h.command) === gsBaseOf(cmdNorm);
|
||||
});
|
||||
}
|
||||
if (idx < 0 && entry._gstack_source === source && entry.hooks.length === 1) {
|
||||
@@ -542,6 +557,14 @@ case "$ACTION" in
|
||||
let removedHere = 0;
|
||||
const remain = [];
|
||||
const seenInEntry = new Set();
|
||||
if (entry.hooks.length === 0) {
|
||||
// Started-empty entries are foreign data we never touched --
|
||||
// preserve them (only a gstack-tagged empty entry is claimable,
|
||||
// and only by the --all sweep).
|
||||
if (all && entry._gstack_source) { removed++; continue; }
|
||||
rebuilt.push(entry);
|
||||
continue;
|
||||
}
|
||||
for (const h of entry.hooks) {
|
||||
const cmdRaw = h && h.command;
|
||||
const row = cmdRaw ? gsOwnedRow(cmdRaw, event, matcher) : null;
|
||||
|
||||
@@ -153,6 +153,10 @@ if [ -x "$SETTINGS_HOOK" ]; then
|
||||
if "$SETTINGS_HOOK" remove-source --source gstack-timeline-stop | grep -q "removed [1-9]"; then
|
||||
REMOVED+=("timeline Stop hook")
|
||||
fi
|
||||
# Verification stop hook (opt-in via README; user-registered, ours to sweep).
|
||||
if "$SETTINGS_HOOK" remove-source --source verify-gate | grep -q "removed [1-9]"; then
|
||||
REMOVED+=("verification Stop hook")
|
||||
fi
|
||||
# Identity sweep for untagged strays (Claude Code strips _gstack_source
|
||||
# tags; pre-v1.67 setups baked worktree paths). Removes every gstack-owned
|
||||
# hook item, live or dead — the binaries they point at are being deleted.
|
||||
|
||||
@@ -1870,6 +1870,14 @@ SETTINGS_HOOK="$SOURCE_GSTACK_DIR/bin/gstack-settings-hook"
|
||||
# WARNING for future code AND migrations (the v1.58.0.0.sh defect class):
|
||||
# NEVER register ${SCRIPT_DIR}/$SOURCE_GSTACK_DIR-relative hook paths.
|
||||
CANONICAL_GSTACK_ROOT="${CLAUDE_CONFIG_DIR:-$HOME/.claude}/skills/gstack"
|
||||
# Split-brain guard: the installer currently hardcodes $HOME/.claude/skills
|
||||
# (setup:1601 TODO), so a CLAUDE_CONFIG_DIR override can name a root that was
|
||||
# never installed. Fall back to where the install actually lives — both are
|
||||
# stable, neither is the running tree, so canonical-only still holds.
|
||||
if [ ! -x "$CANONICAL_GSTACK_ROOT/bin/gstack-session-update" ] \
|
||||
&& [ -x "$HOME/.claude/skills/gstack/bin/gstack-session-update" ]; then
|
||||
CANONICAL_GSTACK_ROOT="$HOME/.claude/skills/gstack"
|
||||
fi
|
||||
|
||||
# Echo the canonical path for a hook (repo-relative arg); fails when the hook
|
||||
# is not executable at the canonical install — callers must skip + log.
|
||||
@@ -1920,14 +1928,9 @@ fi
|
||||
SESSION_UPDATE_CMD="$(_hook_command_path bin/gstack-session-update || true)"
|
||||
HOOK_CMD=""
|
||||
if [ -n "$SESSION_UPDATE_CMD" ]; then
|
||||
# The registered command is executed by a shell when Claude Code fires the
|
||||
# hook — neutralize shell metacharacters, not just whitespace (mirrors
|
||||
# gsQuoteCmd in gstack-settings-hook).
|
||||
case "$SESSION_UPDATE_CMD" in
|
||||
*[\ \"\$\`\\]*)
|
||||
SESSION_UPDATE_CMD="\"$(printf '%s' "$SESSION_UPDATE_CMD" | sed 's/[\\"$`]/\\&/g')\""
|
||||
;;
|
||||
esac
|
||||
# No caller-side quoting: add-event is the single quoting authority — it
|
||||
# normalizes every registered command through the same gsQuoteCmd round-trip
|
||||
# the healer uses, so metachar/space paths cannot drift per call site.
|
||||
if [ "$IS_WINDOWS" -eq 1 ]; then
|
||||
HOOK_CMD="bash $SESSION_UPDATE_CMD"
|
||||
else
|
||||
@@ -2073,6 +2076,14 @@ fi
|
||||
PLAN_TUNE_LOG_HOOK="$(_hook_command_path hosts/claude/hooks/question-log-hook || true)"
|
||||
PLAN_TUNE_PREF_HOOK="$(_hook_command_path hosts/claude/hooks/question-preference-hook || true)"
|
||||
AUQ_ERROR_FALLBACK_HOOK="$(_hook_command_path hosts/claude/hooks/auq-error-fallback-hook || true)"
|
||||
# Windows: extensionless bash shims need the explicit 'bash ' prefix (same
|
||||
# rationale as HOOK_CMD above — the OS file-association dialog otherwise).
|
||||
# KNOWN_HOOKS identity round-trips the prefix, so healing preserves it.
|
||||
if [ "$IS_WINDOWS" -eq 1 ]; then
|
||||
[ -n "$PLAN_TUNE_LOG_HOOK" ] && PLAN_TUNE_LOG_HOOK="bash $PLAN_TUNE_LOG_HOOK"
|
||||
[ -n "$PLAN_TUNE_PREF_HOOK" ] && PLAN_TUNE_PREF_HOOK="bash $PLAN_TUNE_PREF_HOOK"
|
||||
[ -n "$AUQ_ERROR_FALLBACK_HOOK" ] && AUQ_ERROR_FALLBACK_HOOK="bash $AUQ_ERROR_FALLBACK_HOOK"
|
||||
fi
|
||||
PLAN_TUNE_INSTALL_MARKER="$HOME/.gstack/.plan-tune-hooks-prompted"
|
||||
|
||||
# Canonical-only: an ephemeral tree with no stable install gets a visible skip,
|
||||
@@ -2275,13 +2286,18 @@ fi
|
||||
# a session. Idempotent via the (event, source) dedup in gstack-settings-hook;
|
||||
# removed by --no-team and gstack-uninstall.
|
||||
TIMELINE_STOP_HOOK="$(_hook_command_path hosts/claude/hooks/timeline-stop-hook || true)"
|
||||
if [ "$IS_WINDOWS" -eq 1 ] && [ -n "$TIMELINE_STOP_HOOK" ]; then
|
||||
TIMELINE_STOP_HOOK="bash $TIMELINE_STOP_HOOK"
|
||||
fi
|
||||
# stderr stays attached on both calls below (zero silent settings mutations;
|
||||
# the list-sources corrupt-file warning must reach the user too).
|
||||
if [ "$NO_TEAM_MODE" -ne 1 ] && [ -x "$SETTINGS_HOOK" ] && [ -n "$TIMELINE_STOP_HOOK" ]; then
|
||||
if ! "$SETTINGS_HOOK" list-sources 2>/dev/null | grep -q "gstack-timeline-stop"; then
|
||||
if ! "$SETTINGS_HOOK" list-sources | grep -q "gstack-timeline-stop"; then
|
||||
if "$SETTINGS_HOOK" add-event \
|
||||
--event Stop \
|
||||
--command "$TIMELINE_STOP_HOOK" \
|
||||
--source gstack-timeline-stop \
|
||||
--timeout 5 >/dev/null 2>&1; then
|
||||
--timeout 5 >/dev/null; then
|
||||
log " registered Stop hook: session timeline entries now close even when a skill is interrupted (backup: settings.json.bak.<ts>; remove: $SETTINGS_HOOK remove-source --source gstack-timeline-stop)"
|
||||
fi
|
||||
fi
|
||||
|
||||
@@ -597,6 +597,56 @@ describe('review-army hardening (specialist findings)', () => {
|
||||
expect(fs.existsSync(latest)).toBe(true);
|
||||
});
|
||||
|
||||
test('tag-stripped verify-gate entry is table-owned: healed by --repoint, swept by --all', () => {
|
||||
// Red-team catch: verify-gate is a README-documented opt-in Stop hook.
|
||||
// Without a KNOWN_HOOKS row, a tag-stripped entry survived uninstall and
|
||||
// errored at the end of EVERY turn after the install root was deleted.
|
||||
const canon = mkCanon(tmpDir);
|
||||
const vg = path.join(canon, 'bin', 'gstack-verify-gate');
|
||||
fs.writeFileSync(vg, '#!/bin/sh\n');
|
||||
fs.chmodSync(vg, 0o755);
|
||||
fs.writeFileSync(settingsFile, JSON.stringify({
|
||||
hooks: { Stop: [hookEntry('/dead/install/bin/gstack-verify-gate')] }, // tag STRIPPED
|
||||
}, null, 2));
|
||||
runIso(['prune-stale', '--repoint', canon]);
|
||||
let s = settings();
|
||||
expect(s.hooks.Stop[0].hooks[0].command).toBe(vg);
|
||||
expect(s.hooks.Stop[0]._gstack_source).toBe('verify-gate');
|
||||
const r = runIso(['prune-stale', '--all']);
|
||||
expect(r.stdout).toMatch(/removed 1/);
|
||||
expect(settings().hooks).toBeUndefined();
|
||||
});
|
||||
|
||||
test('a foreign entry that STARTED empty survives prune-stale untouched', () => {
|
||||
fs.writeFileSync(settingsFile, JSON.stringify({
|
||||
hooks: { PreToolUse: [{ matcher: 'Bash', hooks: [] }] },
|
||||
}, null, 2) + '\n');
|
||||
const before = fs.readFileSync(settingsFile, 'utf-8');
|
||||
const r = runIso(['prune-stale', '--repoint', mkCanon(tmpDir, 'c2')]);
|
||||
expect(r.stdout).toMatch(/removed 0 gstack hook entries \(repointed 0\)/);
|
||||
expect(fs.readFileSync(settingsFile, 'utf-8')).toBe(before);
|
||||
});
|
||||
|
||||
test('add-event is the quoting authority: spaced canonical path stored escaped-quoted, healer idempotent', () => {
|
||||
// Red-team catch (empirically verified pre-fix): setup registered raw
|
||||
// paths and the very next heal rewrote them — fresh installs shipped a
|
||||
// form the codebase itself considered wrong.
|
||||
const spacedBase = path.join(tmpDir, 'canon root');
|
||||
fs.mkdirSync(spacedBase, { recursive: true });
|
||||
const canon = mkCanon(spacedBase);
|
||||
runIso([
|
||||
'add-event', '--event', 'PostToolUse', '--matcher', AUQ_MATCHER,
|
||||
'--command', `${canon}/hosts/claude/hooks/question-log-hook`,
|
||||
'--source', 'plan-tune-cathedral', '--timeout', '5',
|
||||
]);
|
||||
const stored = settings().hooks.PostToolUse[0].hooks[0].command;
|
||||
expect(stored).toBe(`"${canon}/hosts/claude/hooks/question-log-hook"`);
|
||||
const before = fs.readFileSync(settingsFile, 'utf-8');
|
||||
const r = runIso(['prune-stale', '--repoint', canon]);
|
||||
expect(r.stdout).toMatch(/removed 0 gstack hook entries \(repointed 0\)/);
|
||||
expect(fs.readFileSync(settingsFile, 'utf-8')).toBe(before);
|
||||
});
|
||||
|
||||
test('rollback refuses a pointer that names a non-backup file', () => {
|
||||
fs.writeFileSync(settingsFile, JSON.stringify({ a: 1 }, null, 2));
|
||||
const evil = path.join(tmpDir, 'evil.json');
|
||||
|
||||
Reference in New Issue
Block a user