Skip to content

Commit 13aef3b

Browse files
jeffwall-curlewlabsjeffwall-curlewlabs
authored
Merge pull request #27 from curlewlabs-com/fix/restore-target-safety-guard
fix: refuse restore targets that would wipe the runner
2 parents 98a50ae + a09fa66 commit 13aef3b

2 files changed

Lines changed: 202 additions & 0 deletions

File tree

‎.github/workflows/ci.yml‎

Lines changed: 133 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -370,6 +370,139 @@ jobs:
370370
|| (echo "Expected path to not exist on dash prefix miss" && exit 1)
371371
echo "Dash prefix safety: OK"
372372
373+
# Restore-target safety guard. cache-restore.sh does an
374+
# unconditional `rm -rf "$path_to_cache"` on a stale-marker
375+
# re-sync, so a misconfigured workflow passing path: /, path: $HOME,
376+
# or a relative path could wipe the runner. The guard rejects
377+
# those targets up-front with exit 2 before any filesystem side
378+
# effect. One test per rejection class plus a positive control
379+
# confirming a safe sibling path is still accepted.
380+
- name: Restore-target guard rejects root
381+
run: |
382+
set +e
383+
out=$(sh lib/cache-restore.sh / any-key /tmp/local-cache "" 2>&1)
384+
rc=$?
385+
set -e
386+
[ "$rc" = "2" ] \
387+
|| (echo "Expected exit 2 for root path, got $rc. Output: $out" && exit 1)
388+
echo "$out" | grep -q 'system root' \
389+
|| (echo "Expected 'system root' in error, got: $out" && exit 1)
390+
echo "Reject /: OK"
391+
392+
- name: Restore-target guard rejects /.
393+
run: |
394+
set +e
395+
out=$(sh lib/cache-restore.sh /. any-key /tmp/local-cache "" 2>&1)
396+
rc=$?
397+
set -e
398+
[ "$rc" = "2" ] \
399+
|| (echo "Expected exit 2 for /., got $rc. Output: $out" && exit 1)
400+
echo "Reject /.: OK"
401+
402+
- name: Restore-target guard rejects /..
403+
run: |
404+
set +e
405+
out=$(sh lib/cache-restore.sh /.. any-key /tmp/local-cache "" 2>&1)
406+
rc=$?
407+
set -e
408+
[ "$rc" = "2" ] \
409+
|| (echo "Expected exit 2 for /.., got $rc. Output: $out" && exit 1)
410+
echo "Reject /..: OK"
411+
412+
- name: Restore-target guard rejects relative paths
413+
run: |
414+
set +e
415+
out=$(sh lib/cache-restore.sh some/relative any-key /tmp/local-cache "" 2>&1)
416+
rc=$?
417+
set -e
418+
[ "$rc" = "2" ] \
419+
|| (echo "Expected exit 2 for relative path, got $rc. Output: $out" && exit 1)
420+
echo "$out" | grep -q 'must be absolute' \
421+
|| (echo "Expected 'must be absolute' in error, got: $out" && exit 1)
422+
echo "Reject relative: OK"
423+
424+
- name: Restore-target guard rejects whitespace-only paths
425+
run: |
426+
set +e
427+
out=$(sh lib/cache-restore.sh " " any-key /tmp/local-cache "" 2>&1)
428+
rc=$?
429+
set -e
430+
# cache-restore.sh's empty check runs first for unambiguously
431+
# empty strings; tabs/spaces get past it and hit the new
432+
# whitespace-only guard. Both produce exit 2, which is the
433+
# invariant we're asserting.
434+
[ "$rc" = "2" ] \
435+
|| (echo "Expected exit 2 for whitespace-only path, got $rc. Output: $out" && exit 1)
436+
echo "Reject whitespace-only: OK"
437+
438+
- name: Restore-target guard rejects $HOME
439+
run: |
440+
set +e
441+
out=$(HOME=/tmp/guard-home sh lib/cache-restore.sh /tmp/guard-home any-key /tmp/local-cache "" 2>&1)
442+
rc=$?
443+
set -e
444+
[ "$rc" = "2" ] \
445+
|| (echo "Expected exit 2 for HOME path, got $rc. Output: $out" && exit 1)
446+
echo "$out" | grep -q 'HOME' \
447+
|| (echo "Expected HOME in error, got: $out" && exit 1)
448+
echo "Reject \$HOME: OK"
449+
450+
- name: Restore-target guard rejects parents of $HOME
451+
run: |
452+
set +e
453+
out=$(HOME=/tmp/guard-parent/home sh lib/cache-restore.sh /tmp/guard-parent any-key /tmp/local-cache "" 2>&1)
454+
rc=$?
455+
set -e
456+
[ "$rc" = "2" ] \
457+
|| (echo "Expected exit 2 for parent of HOME, got $rc. Output: $out" && exit 1)
458+
echo "$out" | grep -q 'HOME' \
459+
|| (echo "Expected HOME in error, got: $out" && exit 1)
460+
echo "Reject parent of \$HOME: OK"
461+
462+
- name: Restore-target guard rejects $GITHUB_WORKSPACE
463+
run: |
464+
set +e
465+
# Unset HOME so the HOME check doesn't fire first and mask the
466+
# GITHUB_WORKSPACE-specific diagnostic we're asserting on. Also
467+
# strip the runner's own GITHUB_WORKSPACE and RUNNER_WORKSPACE
468+
# from the outer env so we exercise exactly the value we pass.
469+
out=$(env -u HOME -u RUNNER_WORKSPACE GITHUB_WORKSPACE=/tmp/guard-ws sh lib/cache-restore.sh /tmp/guard-ws any-key /tmp/local-cache "" 2>&1)
470+
rc=$?
471+
set -e
472+
[ "$rc" = "2" ] \
473+
|| (echo "Expected exit 2 for GITHUB_WORKSPACE path, got $rc. Output: $out" && exit 1)
474+
echo "$out" | grep -q 'GITHUB_WORKSPACE' \
475+
|| (echo "Expected GITHUB_WORKSPACE in error, got: $out" && exit 1)
476+
echo "Reject \$GITHUB_WORKSPACE: OK"
477+
478+
- name: Restore-target guard rejects parents of $RUNNER_WORKSPACE
479+
run: |
480+
set +e
481+
out=$(env -u HOME -u GITHUB_WORKSPACE RUNNER_WORKSPACE=/tmp/guard-rw/inner sh lib/cache-restore.sh /tmp/guard-rw any-key /tmp/local-cache "" 2>&1)
482+
rc=$?
483+
set -e
484+
[ "$rc" = "2" ] \
485+
|| (echo "Expected exit 2 for parent of RUNNER_WORKSPACE, got $rc. Output: $out" && exit 1)
486+
echo "$out" | grep -q 'RUNNER_WORKSPACE' \
487+
|| (echo "Expected RUNNER_WORKSPACE in error, got: $out" && exit 1)
488+
echo "Reject parent of \$RUNNER_WORKSPACE: OK"
489+
490+
- name: Restore-target guard accepts a safe sibling path
491+
run: |
492+
# Positive control: a path under /tmp that is not the root, not
493+
# an ancestor of any guarded variable, and has no cache entry
494+
# should produce a clean cache miss (exit 0) rather than a
495+
# rejection. This confirms the guard is not over-rejecting.
496+
set +e
497+
out=$(env -u HOME -u GITHUB_WORKSPACE -u RUNNER_WORKSPACE sh lib/cache-restore.sh /tmp/guard-safe-sibling no-such-key /tmp/local-cache "" 2>&1)
498+
rc=$?
499+
set -e
500+
[ "$rc" = "0" ] \
501+
|| (echo "Expected exit 0 for safe sibling path, got $rc. Output: $out" && exit 1)
502+
echo "$out" | grep -q 'Cache miss' \
503+
|| (echo "Expected 'Cache miss' in output, got: $out" && exit 1)
504+
echo "Accept safe sibling: OK"
505+
373506
# Sequential idempotent save: verify that a save against an already-
374507
# populated entry exits cleanly without touching the existing content.
375508
# This does NOT exercise parallel mutex contention — real contention is

‎lib/cache-restore.sh‎

Lines changed: 69 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -61,6 +61,75 @@ if [ -z "$path_to_cache" ] || [ -z "$cache_key" ] || [ -z "$cache_dir" ]; then
6161
exit 1
6262
fi
6363

64+
# cache-restore.sh does an unconditional `rm -rf "$path_to_cache"` on a
65+
# stale-marker re-sync (see do_restore below). A misconfigured caller
66+
# passing `path: /` or `path: $HOME` must not be able to wipe the
67+
# machine, so reject targets that would destroy the system root or a
68+
# known runner workspace before any filesystem side effects. The check
69+
# runs up-front so it also fires in --check mode, which keeps the error
70+
# shape consistent across Phase 1 and Phase 2 and catches bad paths
71+
# before Phase 2 even acquires the mutex.
72+
check_not_restore_ancestor() {
73+
target_norm="$1"
74+
danger_label="$2"
75+
danger_raw="$3"
76+
[ -z "$danger_raw" ] && return 0
77+
# Normalize trailing slashes on the danger path to match target_norm.
78+
danger_norm="$danger_raw"
79+
while [ "${danger_norm%/}" != "$danger_norm" ]; do
80+
danger_norm="${danger_norm%/}"
81+
done
82+
[ -z "$danger_norm" ] && danger_norm="/"
83+
case "$danger_norm" in
84+
"$target_norm"|"$target_norm"/*)
85+
printf '::error::cache-restore: refusing to restore to %s — rm -rf would delete %s (%s)\n' "$path_to_cache" "$danger_label" "$danger_raw" >&2
86+
exit 2
87+
;;
88+
esac
89+
}
90+
91+
# Whitespace-only paths never resolve to a sensible target; they usually
92+
# indicate an expansion failure in the caller's workflow YAML.
93+
case "$path_to_cache" in
94+
*[![:space:]]*) ;;
95+
*)
96+
printf '::error::cache-restore: path must not be whitespace-only\n' >&2
97+
exit 2
98+
;;
99+
esac
100+
101+
# Absolute paths only; relative paths resolve against this script's CWD,
102+
# which varies between Phase 1 and Phase 2 (Phase 2 runs inside
103+
# local-mutex's working directory), and is therefore unsafe to trust.
104+
case "$path_to_cache" in
105+
/*) ;;
106+
*)
107+
printf '::error::cache-restore: path must be absolute: %s\n' "$path_to_cache" >&2
108+
exit 2
109+
;;
110+
esac
111+
112+
# Trivially unsafe paths — caught even when HOME/RUNNER_WORKSPACE/
113+
# GITHUB_WORKSPACE are all unset (e.g. local smoke-testing).
114+
case "$path_to_cache" in
115+
/|/.|/..)
116+
printf '::error::cache-restore: refusing to restore to %s — rm -rf would affect the system root\n' "$path_to_cache" >&2
117+
exit 2
118+
;;
119+
esac
120+
121+
# Normalize trailing slashes on the target so "/foo/" and "/foo" hit the
122+
# ancestor check identically. Preserve root ("/" must stay "/").
123+
path_to_cache_norm="$path_to_cache"
124+
while [ "${path_to_cache_norm%/}" != "$path_to_cache_norm" ]; do
125+
path_to_cache_norm="${path_to_cache_norm%/}"
126+
done
127+
[ -z "$path_to_cache_norm" ] && path_to_cache_norm="/"
128+
129+
check_not_restore_ancestor "$path_to_cache_norm" HOME "${HOME:-}"
130+
check_not_restore_ancestor "$path_to_cache_norm" RUNNER_WORKSPACE "${RUNNER_WORKSPACE:-}"
131+
check_not_restore_ancestor "$path_to_cache_norm" GITHUB_WORKSPACE "${GITHUB_WORKSPACE:-}"
132+
64133
# GITHUB_OUTPUT must exist before any output is written. Outside Actions it is
65134
# unset; a missing path with set -e would abort the script on the first write.
66135
if [ -z "${GITHUB_OUTPUT:-}" ]; then

0 commit comments

Comments
 (0)