From bb46ee22eb4bd41f8f68ec0909c6bb10806d6351 Mon Sep 17 00:00:00 2001 From: bastien Date: Tue, 29 Sep 2026 15:36:18 +0200 Subject: [PATCH] fix(effort): harden lib/effort-pins.sh (security gate, 4 LOW) Last map line without newline read; unclosed frontmatter skipped with an err; level re-read after write, mismatch counted as failed; mktemp + cp -p + mv, temp removed on failure; rc 1 on any rejected or failed entry. Cases T11-T14 in the fixture suite; contract criteria 8-9. --- .../contracts/2026-09-29-effort-round-1315.md | 10 +++ lib/effort-pins.sh | 61 ++++++++++++++----- lib/tests/effort-pins.test.sh | 33 +++++++++- 3 files changed, 89 insertions(+), 15 deletions(-) diff --git a/.claude/tasks/contracts/2026-09-29-effort-round-1315.md b/.claude/tasks/contracts/2026-09-29-effort-round-1315.md index 11b337a..cb066d4 100644 --- a/.claude/tasks/contracts/2026-09-29-effort-round-1315.md +++ b/.claude/tasks/contracts/2026-09-29-effort-round-1315.md @@ -12,6 +12,7 @@ - Defect found in passing, fixed here: `update-all.sh` re-fetched the vendored skills but never re-applied the pins (lost until the next `make plugin`). - Defect found in passing, surfaced not fixed: ~94 % of sub-agent usage records carry no `output_tokens_details`, so `lib/effort-audit.py` read zero thinking on sub-agents; the script now prints coverage and a CAVEAT; EVAL-037's "executors stay cheap" is a measurement gap (registry correction pending user approval). - lib/tests/effort-routing.test.sh line 4 widens its shellcheck directive from SC2015 to SC2015,SC2016: the new `has … '$REPO'` locks are literal source text, the `$REPO` must NOT expand (authorized; a test file, informational). [verifier 2026-09-29 gap 3] +- Hardening round (criteria 8-9) added after the security gate on user go; the fixture suite may `chmod` its own mktemp directory (555 then back to 755 for the trap cleanup), never `-R`, never outside the fixture. - Frontmatter placement of the inserted `effort:` line (after `name:`, else before the closing `---`) has no harness effect; locked by the fixture suite only. ## ACCEPTANCE CRITERIA @@ -44,6 +45,15 @@ EXPECT: LINT_OK EVIDENCE: MET exit=0 marker-found :: LINT_OK +8. Hardening (security gate 2026-09-29, 4 LOW, user go): (a) a map whose last line has no trailing newline still applies that line; (b) a SKILL.md whose frontmatter has no closing `---` is skipped with an err line, file byte-identical; (c) a CRLF SKILL.md (`---\r`) is never counted as applied: the helper re-reads the level after the write and reports a mismatch as err, counted as failed (rc 1); (d) a write failure (read-only skill directory) is reported as err, counted as failed, and leaves no temporary file behind. Cases T11-T14 in lib/tests/effort-pins.test.sh, header comment of the helper updated. + CHECK: out=$(make test suite=lib/tests/effort-pins.test.sh 2>&1); echo "$out" | grep -q 'effort-pins: [0-9]* pass, 0 fail' || { echo "$out" | grep FAIL; exit 1; }; for k in T11-last-line-no-newline T12-unterminated-frontmatter-skipped T13-crlf-not-counted-applied T14-write-failure-no-temp; do echo "$out" | grep -q "PASS $k" || { echo "missing PASS $k"; exit 1; }; done; echo HARDEN_GREEN + EXPECT: HARDEN_GREEN + EVIDENCE: MET exit=0 marker-found :: HARDEN_GREEN +9. Hardening keeps everything else green: shellcheck clean on the helper and its suite, effort-routing census green, live tree still idempotent (0 applied). + CHECK: shellcheck lib/effort-pins.sh lib/tests/effort-pins.test.sh && make test suite=lib/tests/effort-routing.test.sh 2>&1 | grep -q 'census: [0-9]* pass, 0 fail' && bash lib/effort-pins.sh 2>&1 | grep -q ' 0 applied, [0-9]* already at level' && echo HARDEN_STABLE + EXPECT: HARDEN_STABLE + EVIDENCE: MET exit=0 marker-found :: HARDEN_STABLE + ## FILE SCOPE - lib/effort-pins.txt, lib/effort-pins.sh (new); lib/tests/effort-pins.test.sh (new); lib/tests/effort-routing.test.sh - install-plugins.sh, update-all.sh (re-apply call), lib/effort-audit.py (coverage) diff --git a/lib/effort-pins.sh b/lib/effort-pins.sh index 8ba6f14..3edb209 100755 --- a/lib/effort-pins.sh +++ b/lib/effort-pins.sh @@ -6,12 +6,16 @@ # it back after the last vendoring step of install-plugins.sh and # update-all.sh. Idempotent: same level → untouched, other level → # replaced inside the frontmatter only, skill not vendored → skipped, -# malformed map line → rejected loudly, never applied. Placement inside the +# malformed map line → rejected loudly, never applied. Four hardenings: +# a map whose last line lacks a newline is still read; a SKILL.md whose +# frontmatter never closes is skipped untouched; the level is re-read after +# every write and a mismatch (CRLF, malformed) counts as failed; the write +# goes through a mktemp sibling removed on any failure. Placement inside the # frontmatter has no effect on the harness, which reads the key anywhere. # # Usage: source it, then `apply_effort_pins [repo-root]` # or standalone: bash lib/effort-pins.sh [repo-root] -# Exit 1 when at least one map line was rejected. +# Exit 1 when at least one map line was rejected or a skill failed. EFFORT_PINS_REPO="$(cd "$(dirname "${BASH_SOURCE[0]}")/.." && pwd)" EFFORT_PIN_LEVEL_RE='^(low|medium|high|xhigh|max)$' @@ -29,12 +33,19 @@ _effort_pin_current() { | sed -n 's/^effort: //p' | head -1 } +# _effort_pin_closed → rc 0 when the frontmatter has a closing --- +_effort_pin_closed() { + awk 'NR==1&&/^---$/{p=1;next} p&&/^---$/{f=1;exit} END{exit !f}' "$1" +} + # _effort_pin_write — replace the frontmatter # `effort:` line, or insert one after `name: ` (before the closing # `---` when the frontmatter has no name line). Body lines never change. +# Writes a mktemp sibling then renames; any failure leaves no temp behind. _effort_pin_write() { - local file="$1" name="$2" level="$3" - awk -v n="$name" -v lvl="$level" ' + local file="$1" name="$2" level="$3" tmp + tmp="$(mktemp "$file.XXXXXX")" || return 1 + cp -p "$file" "$tmp" && awk -v n="$name" -v lvl="$level" ' NR==1 && /^---$/ { fm=1; print; next } fm && /^---$/ { if (!done) { print "effort: " lvl; done=1 } @@ -43,16 +54,36 @@ _effort_pin_write() { fm && /^effort: / { if (!done) { print "effort: " lvl; done=1 }; next } fm && $0 == "name: " n { print; if (!done) { print "effort: " lvl; done=1 }; next } { print } - ' "$file" > "$file.tmp" && mv "$file.tmp" "$file" + ' "$file" > "$tmp" && mv "$tmp" "$file" && return 0 + rm -f "$tmp" + return 1 +} + +# _effort_pin_apply_one → rc 0 applied, 2 already at +# level, 1 failed (err line printed, file untouched or write rolled back) +_effort_pin_apply_one() { + local file="$1" name="$2" level="$3" + if ! _effort_pin_closed "$file"; then + err "effort-pins: $file: frontmatter never closed — skipped"; return 1 + fi + [ "$(_effort_pin_current "$file")" = "$level" ] && return 2 + if ! _effort_pin_write "$file" "$name" "$level"; then + err "effort-pins: $file: write failed"; return 1 + fi + if [ "$(_effort_pin_current "$file")" != "$level" ]; then + err "effort-pins: $file: level not applied (CRLF or malformed frontmatter?)" + return 1 + fi + return 0 } # apply_effort_pins [repo-root] — walk the map, pin every vendored skill apply_effort_pins() { - local repo="${1:-$EFFORT_PINS_REPO}" map name level rest file - local applied=0 kept=0 rejected=0 + local repo="${1:-$EFFORT_PINS_REPO}" map name level rest file rc + local applied=0 kept=0 rejected=0 failed=0 map="$repo/lib/effort-pins.txt" [ -f "$map" ] || { err "effort-pins: map missing: $map"; return 1; } - while read -r name level rest; do + while read -r name level rest || [ -n "$name" ]; do case "$name" in ''|'#'*) continue ;; esac if [ -n "$rest" ] || ! [[ "$name" =~ $EFFORT_PIN_NAME_RE ]] \ || ! [[ "$level" =~ $EFFORT_PIN_LEVEL_RE ]]; then @@ -61,13 +92,15 @@ apply_effort_pins() { fi file="$repo/skills-external/$name/SKILL.md" [ -f "$file" ] || continue - if [ "$(_effort_pin_current "$file")" = "$level" ]; then - kept=$((kept + 1)); continue - fi - _effort_pin_write "$file" "$name" "$level" && applied=$((applied + 1)) + _effort_pin_apply_one "$file" "$name" "$level"; rc=$? + case "$rc" in + 0) applied=$((applied + 1)) ;; + 2) kept=$((kept + 1)) ;; + *) failed=$((failed + 1)) ;; + esac done < "$map" - ok "effort-pins: $applied applied, $kept already at level" - [ "$rejected" -eq 0 ] + ok "effort-pins: $applied applied, $kept already at level, $failed failed" + [ "$rejected" -eq 0 ] && [ "$failed" -eq 0 ] } if [[ "${BASH_SOURCE[0]}" == "$0" ]]; then diff --git a/lib/tests/effort-pins.test.sh b/lib/tests/effort-pins.test.sh index 7d0b1d7..256ce7a 100755 --- a/lib/tests/effort-pins.test.sh +++ b/lib/tests/effort-pins.test.sh @@ -5,7 +5,8 @@ # skip a skill not vendored, insert before the closing `---` when the # frontmatter has no name line, run idempotently, reject a bad level, a # traversal name and a three-field line before writing anything, and -# parse the real map without error. All on a throwaway fixture repo. +# parse the real map without error; hardening: last map line without a +# newline, unterminated frontmatter, CRLF file and read-only directory. All on a throwaway fixture repo. set -u ROOT="$(cd "$(dirname "$0")/../.." && pwd)" LIB="$ROOT/lib/effort-pins.sh" @@ -56,5 +57,35 @@ out="$(bash "$LIB" "$WORK/real" 2>&1)"; check T9-real-map-parses "$?" 0 check T9b-real-map-nothing-applied "$(printf '%s' "$out" | grep -c '0 applied, 0 already')" 1 check T10-missing-map-rc "$(bash "$LIB" "$WORK/nowhere" >/dev/null 2>&1; echo $?)" 1 +# hardening: each case in its own fixture repo +mkrepo() { R="$WORK/$1"; mkdir -p "$R/lib" "$R/skills-external/$2"; } +mkrepo h11 alpha; mkdir "$WORK/h11/skills-external/beta" +printf -- '---\nname: alpha\n---\nb\n' > "$WORK/h11/skills-external/alpha/SKILL.md" +printf -- '---\nname: beta\n---\nb\n' > "$WORK/h11/skills-external/beta/SKILL.md" +printf 'alpha high\nbeta low' > "$WORK/h11/lib/effort-pins.txt" +bash "$LIB" "$WORK/h11" >/dev/null 2>&1 +check T11-last-line-no-newline "$(fm_effort "$WORK/h11/skills-external/beta/SKILL.md")" low + +mkrepo h12 open; f12="$WORK/h12/skills-external/open/SKILL.md" +printf -- '---\nname: open\nbody effort: max\n' > "$f12"; b12="$(cat "$f12")" +printf 'open high\n' > "$WORK/h12/lib/effort-pins.txt" +out="$(bash "$LIB" "$WORK/h12" 2>&1)"; rc=$? +check T12-unterminated-frontmatter-skipped \ + "$rc|$(cat "$f12" | cmp -s - <(printf '%s\n' "$b12") && echo same)|$(printf '%s' "$out" | grep -c "ERR .*$f12")" "1|same|1" + +mkrepo h13 crlf +printf -- '---\r\nname: crlf\r\n---\r\nbody\r\n' > "$WORK/h13/skills-external/crlf/SKILL.md" +printf 'crlf high\n' > "$WORK/h13/lib/effort-pins.txt" +out="$(bash "$LIB" "$WORK/h13" 2>&1)"; rc=$? +check T13-crlf-not-counted-applied \ + "$rc|$(printf '%s' "$out" | grep -c 'ERR ')|$(printf '%s' "$out" | grep -c ' 0 applied, ')" "1|1|1" + +mkrepo h14 ro; d14="$WORK/h14/skills-external/ro" +printf -- '---\nname: ro\n---\nb\n' > "$d14/SKILL.md" +printf 'ro high\n' > "$WORK/h14/lib/effort-pins.txt" +chmod 555 "$d14"; out="$(bash "$LIB" "$WORK/h14" 2>&1)"; rc=$?; chmod 755 "$d14" +check T14-write-failure-no-temp \ + "$rc|$(printf '%s' "$out" | grep -c 'ERR ')|$(find "$d14" -name 'SKILL.md.*' | wc -l)" "1|1|0" + echo "effort-pins: $pass pass, $fail fail" [ "$fail" -eq 0 ]