mirror of
https://github.com/garrytan/gstack.git
synced 2026-05-07 05:56:41 +02:00
security: harden migration + context-save after adversarial review
Adversarial review (Claude + Codex, both high confidence) identified 6
critical production-harm findings in the /ship pre-landing pass.
All folded in.
Migration v1.0.1.0.sh hardening:
- Add explicit `[ -z "${HOME:-}" ]` guard. HOME="" survives set -u and
expands paths to /.claude/skills/... which could hit absolute paths
under root/containers/sudo-without-H.
- Add python3 fallback inside resolve_real() (was missing; broken
symlinks silently defeated ownership check).
- Ownership-guard Shape 2 (~/.claude/skills/gstack/checkpoint/). Was
unconditional rm -rf. Now: if symlink, check target resolves inside
gstack; if regular dir, check realpath resolves inside gstack. A
user's hand-edited customization or a symlink pointing outside gstack
is preserved with a notice.
- Use `rm --` and `rm -r --` consistently to resist hostile basenames.
- Use `find -type f -not -name .DS_Store -not -name ._*` instead of
`ls -A | grep`. macOS sidecars no longer mask a legit prefix-mode
install. Strip sidecars explicitly before removing the dir.
context-save/SKILL.md.tmpl:
- Sanitize title in bash, not LLM prose. Allowlist [a-z0-9.-], cap 60
chars, default to "untitled". Closes a prompt-injection surface where
`/context-save $(rm -rf ~)` could propagate into subsequent commands.
- Collision-safe filename. If ${TIMESTAMP}-${SLUG}.md already exists
(same-second double-save with same title), append a 4-char random
suffix. The skill contract says "saved files are append-only" — this
enforces it. Silent overwrite was a data-loss bug.
context-restore/SKILL.md.tmpl:
- Cap `find ... | sort -r` at 20 entries via `| head -20`. A user with
10k+ saved files no longer blows the context window just to pick one.
/context-save list still handles the full-history listing path.
test/skill-e2e-autoplan-dual-voice.test.ts:
- Filter transcript to tool_use / tool_result / assistant entries
before matching, so prompt-text mentions of "plan-ceo-review" don't
force the reachedPhase1 assertion to pass. Phase-1 assertion now
requires completion markers ("Phase 1 complete", "Phase 2 started"),
not mere name occurrence.
- claudeVoiceFired now requires JSON evidence of an Agent tool_use
(name:"Agent" or subagent_type field), not the literal string
"Agent(" which could appear anywhere.
- codexVoiceFired now requires a Bash tool_use with a `codex exec/review`
command string, not prompt-text mentions.
All SKILL.md files regenerated. Golden fixtures updated. bun test: 0
failures across 80+ targeted tests and the full suite.
Review source: /ship Step 11 adversarial pass (claude subagent + codex
exec). Same findings independently surfaced by both reviewers — this is
cross-model high confidence.
This commit is contained in:
Garry Tan
@@ -20,33 +20,42 @@
|
||||
# Idempotent: missing paths are no-ops.
|
||||
set -euo pipefail
|
||||
|
||||
# Guard: refuse to run if HOME is unset or empty. With `set -u`, unset HOME
|
||||
# errors out, but HOME="" (possible under sudo-without-H, systemd units, some
|
||||
# CI runners) survives and produces dangerous absolute paths like
|
||||
# "/.claude/skills/...". Abort cleanly.
|
||||
if [ -z "${HOME:-}" ]; then
|
||||
echo " [v1.0.1.0] HOME is unset or empty — skipping migration." >&2
|
||||
exit 0
|
||||
fi
|
||||
|
||||
SKILLS_DIR="${HOME}/.claude/skills"
|
||||
OLD_TOPLEVEL="${SKILLS_DIR}/checkpoint"
|
||||
OLD_NAMESPACED="${SKILLS_DIR}/gstack/checkpoint"
|
||||
GSTACK_ROOT_REAL=""
|
||||
|
||||
# Helper: canonical-path a target (symlink-safe). Prints the resolved path, or
|
||||
# empty on failure (broken symlink, ENOENT, ELOOP). Both realpath AND the python3
|
||||
# fallback are tried — a single tool failure shouldn't defeat the ownership
|
||||
# check. Returns empty string if both fail.
|
||||
resolve_real() {
|
||||
local target="$1"
|
||||
local out=""
|
||||
if command -v realpath >/dev/null 2>&1; then
|
||||
out=$(realpath "$target" 2>/dev/null || true)
|
||||
fi
|
||||
if [ -z "$out" ] && command -v python3 >/dev/null 2>&1; then
|
||||
out=$(python3 -c 'import os,sys;print(os.path.realpath(sys.argv[1]))' "$target" 2>/dev/null || true)
|
||||
fi
|
||||
printf '%s' "$out"
|
||||
}
|
||||
|
||||
# Resolve the canonical path of the gstack skills root. If gstack isn't
|
||||
# installed here, there's nothing to migrate.
|
||||
if [ -d "${SKILLS_DIR}/gstack" ]; then
|
||||
# Portable realpath: macOS BSD `readlink` lacks -f. Fall back to python3.
|
||||
if command -v realpath >/dev/null 2>&1; then
|
||||
GSTACK_ROOT_REAL=$(realpath "${SKILLS_DIR}/gstack" 2>/dev/null || true)
|
||||
fi
|
||||
if [ -z "$GSTACK_ROOT_REAL" ]; then
|
||||
GSTACK_ROOT_REAL=$(python3 -c 'import os,sys;print(os.path.realpath(sys.argv[1]))' "${SKILLS_DIR}/gstack" 2>/dev/null || true)
|
||||
fi
|
||||
GSTACK_ROOT_REAL=$(resolve_real "${SKILLS_DIR}/gstack")
|
||||
fi
|
||||
|
||||
# Helper: canonical-path a target (symlink-safe). Prints the resolved path.
|
||||
resolve_real() {
|
||||
local target="$1"
|
||||
if command -v realpath >/dev/null 2>&1; then
|
||||
realpath "$target" 2>/dev/null || true
|
||||
return
|
||||
fi
|
||||
python3 -c 'import os,sys;print(os.path.realpath(sys.argv[1]))' "$target" 2>/dev/null || true
|
||||
}
|
||||
|
||||
# Helper: does $1 (canonical path) live inside $2 (canonical path)?
|
||||
path_inside() {
|
||||
local inner="$1"
|
||||
@@ -65,20 +74,24 @@ if [ -L "$OLD_TOPLEVEL" ]; then
|
||||
# Directory symlink (or file symlink). Canonicalize and check ownership.
|
||||
target_real=$(resolve_real "$OLD_TOPLEVEL")
|
||||
if [ -n "$GSTACK_ROOT_REAL" ] && path_inside "$target_real" "$GSTACK_ROOT_REAL"; then
|
||||
rm "$OLD_TOPLEVEL"
|
||||
rm -- "$OLD_TOPLEVEL"
|
||||
echo " [v1.0.1.0] Removed stale /checkpoint symlink (was shadowing Claude Code's /rewind alias)."
|
||||
removed_any=1
|
||||
else
|
||||
echo " [v1.0.1.0] Leaving $OLD_TOPLEVEL alone — symlink target is outside gstack."
|
||||
echo " [v1.0.1.0] Leaving $OLD_TOPLEVEL alone — symlink target is outside gstack (or unresolvable)."
|
||||
fi
|
||||
elif [ -d "$OLD_TOPLEVEL" ]; then
|
||||
# Regular directory. Only remove if it contains exactly one file named
|
||||
# SKILL.md that's a symlink into gstack (gstack's prefix-install shape).
|
||||
entries=$(ls -A "$OLD_TOPLEVEL" 2>/dev/null)
|
||||
if [ "$entries" = "SKILL.md" ] && [ -L "$OLD_TOPLEVEL/SKILL.md" ]; then
|
||||
# Use find to count real files, ignoring .DS_Store (macOS sidecars).
|
||||
file_count=$(find "$OLD_TOPLEVEL" -maxdepth 1 -type f -not -name '.DS_Store' -not -name '._*' 2>/dev/null | wc -l | tr -d ' ')
|
||||
symlink_count=$(find "$OLD_TOPLEVEL" -maxdepth 1 -type l 2>/dev/null | wc -l | tr -d ' ')
|
||||
if [ "$file_count" = "0" ] && [ "$symlink_count" = "1" ] && [ -L "$OLD_TOPLEVEL/SKILL.md" ]; then
|
||||
target_real=$(resolve_real "$OLD_TOPLEVEL/SKILL.md")
|
||||
if [ -n "$GSTACK_ROOT_REAL" ] && path_inside "$target_real" "$GSTACK_ROOT_REAL"; then
|
||||
rm -r "$OLD_TOPLEVEL"
|
||||
# Strip macOS sidecars first (not user content), then remove the dir.
|
||||
find "$OLD_TOPLEVEL" -maxdepth 1 \( -name '.DS_Store' -o -name '._*' \) -type f -delete 2>/dev/null || true
|
||||
rm -r -- "$OLD_TOPLEVEL"
|
||||
echo " [v1.0.1.0] Removed stale /checkpoint install directory (gstack prefix-mode)."
|
||||
removed_any=1
|
||||
else
|
||||
@@ -90,11 +103,31 @@ elif [ -d "$OLD_TOPLEVEL" ]; then
|
||||
fi
|
||||
# Missing → no-op (idempotency).
|
||||
|
||||
# --- Shape 2: ~/.claude/skills/gstack/checkpoint/ (gstack owns this dir unconditionally)
|
||||
if [ -d "$OLD_NAMESPACED" ] || [ -L "$OLD_NAMESPACED" ]; then
|
||||
rm -rf "$OLD_NAMESPACED"
|
||||
echo " [v1.0.1.0] Removed stale ~/.claude/skills/gstack/checkpoint/ (replaced by context-save + context-restore)."
|
||||
removed_any=1
|
||||
# --- Shape 2: ~/.claude/skills/gstack/checkpoint/
|
||||
# Ownership guard applies here too: only remove if this path resolves inside the
|
||||
# gstack skills root. If a user replaced the directory with a symlink pointing
|
||||
# elsewhere (e.g., at their own fork), respect it.
|
||||
if [ -L "$OLD_NAMESPACED" ]; then
|
||||
target_real=$(resolve_real "$OLD_NAMESPACED")
|
||||
if [ -n "$GSTACK_ROOT_REAL" ] && path_inside "$target_real" "$GSTACK_ROOT_REAL"; then
|
||||
rm -- "$OLD_NAMESPACED"
|
||||
echo " [v1.0.1.0] Removed stale ~/.claude/skills/gstack/checkpoint symlink."
|
||||
removed_any=1
|
||||
else
|
||||
echo " [v1.0.1.0] Leaving $OLD_NAMESPACED alone — symlink target is outside gstack."
|
||||
fi
|
||||
elif [ -d "$OLD_NAMESPACED" ]; then
|
||||
# Regular directory. This is the gstack-prefix install location. Check that
|
||||
# it resolves to a path inside the gstack root (it should, unless someone
|
||||
# hand-edited the tree).
|
||||
target_real=$(resolve_real "$OLD_NAMESPACED")
|
||||
if [ -n "$GSTACK_ROOT_REAL" ] && path_inside "$target_real" "$GSTACK_ROOT_REAL"; then
|
||||
rm -rf -- "$OLD_NAMESPACED"
|
||||
echo " [v1.0.1.0] Removed stale ~/.claude/skills/gstack/checkpoint/ (replaced by context-save + context-restore)."
|
||||
removed_any=1
|
||||
else
|
||||
echo " [v1.0.1.0] Leaving $OLD_NAMESPACED alone — resolves outside gstack."
|
||||
fi
|
||||
fi
|
||||
|
||||
if [ "$removed_any" = "1" ]; then
|
||||
|
||||
Reference in New Issue
Block a user