From aaf88d39c82455579d42a6a9d43779124cef1a61 Mon Sep 17 00:00:00 2001 From: Chris Graf Date: Wed, 4 Nov 2026 09:47:44 -0600 Subject: [PATCH] Place the managed cron block first Summary Apply the managed cron environment to all following jobs by placing its block at the start of the crontab. - relocate unchanged legacy blocks and report misplaced blocks in doctor - reject ambiguous markers and recheck the crontab before rewriting it - preserve later unmanaged assignments as explicit overrides Testing - dot test (38 suites passed, one skipped) - dot test core-cron core-doctor - checkrun format and checkrun lint - ShellCheck inventory and git diff --check --- .config/dot/merge-hooks.d/cron/README.md | 16 +++ .config/shell/README.md | 6 +- .local/lib/dotfiles/doctor.d/lib/cron.sh | 5 +- .local/lib/dotfiles/merge-hooks.d/cron.sh | 59 +++++---- .../dotfiles/merge-hooks.d/lib/cron-list.sh | 22 ++++ .local/lib/dotfiles/tests/core/cron.sh | 119 ++++++++++++++++++ .local/lib/dotfiles/tests/core/doctor.sh | 16 +++ 7 files changed, 215 insertions(+), 28 deletions(-) diff --git a/.config/dot/merge-hooks.d/cron/README.md b/.config/dot/merge-hooks.d/cron/README.md index 99df35a7..8f0fb33b 100644 --- a/.config/dot/merge-hooks.d/cron/README.md +++ b/.config/dot/merge-hooks.d/cron/README.md @@ -9,3 +9,19 @@ are: The executable hook implementation lives at `~/.local/lib/dotfiles/merge-hooks.d/cron.sh`. + +The managed block is placed at the top of the account crontab. Its `SHELL`, +`BASH_ENV`, `PATH`, and any other variable assignments apply to all later jobs, +including unmanaged jobs. A later unmanaged assignment overrides that value +for jobs below it. Other accounts, system crontabs, and `/etc/cron.d` files +have independent environments. + +On the first update from the earlier layout, an unchanged block at the bottom +is moved to the top. An unmanaged assignment that previously preceded the +managed block (for example `MAILTO` or `CRON_TZ`) no longer applies to managed +jobs; it still applies to subsequent unmanaged jobs. Put a value intended for +managed jobs in a managed fragment. Ambiguous, duplicate or incomplete managed +markers cause the update to leave the crontab untouched for manual repair. +Before replacing a crontab the hook re-reads it and stops on a concurrent +change; crontab does not provide an atomic compare-and-swap, so a writer can +still race after that check. diff --git a/.config/shell/README.md b/.config/shell/README.md index 934bd226..914fed54 100644 --- a/.config/shell/README.md +++ b/.config/shell/README.md @@ -65,8 +65,10 @@ every shell regardless of mode. Cron jobs are fill-only children too. The cron merge hook writes `SHELL` (an absolute bash) and `BASH_ENV` (this loader) into the managed crontab block -ahead of its `PATH`, so jobs see the same `env.d` values as agent and script -shells instead of cron's bare `/bin/sh` environment. The crontab's `PATH` +ahead of its `PATH` at the top of the crontab, so managed and subsequent +unmanaged jobs see the same `env.d` values as agent and script shells instead +of cron's bare `/bin/sh` environment. Later unmanaged assignments may override +these values for subsequent jobs. The crontab's `PATH` arrives as an inherited value. When bash or the loader is missing, the hook leaves cron on its default shell. diff --git a/.local/lib/dotfiles/doctor.d/lib/cron.sh b/.local/lib/dotfiles/doctor.d/lib/cron.sh index 7cb84d09..efeab596 100644 --- a/.local/lib/dotfiles/doctor.d/lib/cron.sh +++ b/.local/lib/dotfiles/doctor.d/lib/cron.sh @@ -188,6 +188,9 @@ _dr_check_cron() { if [[ -z $marker || -z $mode ]]; then _dr_skip "managed cron block unchecked" "the cron merge hook could not load" + elif ((listed != 2)) && ! _cron_block_valid "$marker" "$crontab_out"; then + _dr_hint_row warn "managed cron block is malformed" \ + "ambiguous markers leave the crontab unchanged" "inspect and repair its markers manually" elif [[ -z $expected ]]; then if [[ $crontab_out != *"$marker begin"* ]]; then _dr_ok "no tracked cron entries apply to this host" @@ -204,7 +207,7 @@ _dr_check_cron() { # would send the reader round in circles; crontab's message says why. _dr_hint_row warn "crontab is not usable by this account" "$_CRON_LIST_ERR" \ "ask an administrator to allow crontab for this account (cron.allow or cron.deny, PAM, its setuid/setgid bit), then run 'dot update'" - elif [[ $crontab_out == *"$expected"* ]]; then + elif [[ $crontab_out == "$expected" || $crontab_out == "$expected"$'\n'* ]]; then _dr_ok "managed cron block is current" elif [[ $crontab_out == *"$marker begin"* ]]; then _dr_hint_row warn "managed cron block is stale" "it differs from the tracked entries" \ diff --git a/.local/lib/dotfiles/merge-hooks.d/cron.sh b/.local/lib/dotfiles/merge-hooks.d/cron.sh index 7767fc25..c3078d25 100644 --- a/.local/lib/dotfiles/merge-hooks.d/cron.sh +++ b/.local/lib/dotfiles/merge-hooks.d/cron.sh @@ -291,36 +291,45 @@ merge() { return 1 fi - # No active entries — strip any existing managed block and return. + # The block stripper is substring-based. Validate even an unchanged or + # strip-only block before it can discard ambiguous user-owned entries. + if ! _cron_block_valid "$cron_marker" "$current"; then + dot_hook_warn " warning: cron block markers are malformed; crontab left unchanged" + return 1 + fi + + local filtered new_crontab remove=0 + # No active entries — strip any existing managed block. if [[ -z "$managed_block" ]]; then - if [[ "$current" == *"$cron_marker begin"* ]]; then - local stripped - stripped="$(dot_managed_block_strip "$cron_marker" "$current")" - if [[ -n "$stripped" ]]; then - echo "$stripped" | "$cron_command" - - else - "$cron_command" -r 2>/dev/null || true - fi + [[ "$current" == *"$cron_marker begin"* ]] || return 0 + new_crontab="$(dot_managed_block_strip "$cron_marker" "$current")" + [[ -n "$new_crontab" ]] || remove=1 + else + # Position is part of the contract: a legacy block at the bottom must move. + if [[ "$current" == "$managed_block" || "$current" == "$managed_block"$'\n'* ]]; then + return 0 + fi + filtered="$(dot_managed_block_strip "$cron_marker" "$current")" + # Unmanaged assignments remain in place and can override these defaults. + if [[ -n "$filtered" ]]; then + new_crontab="$managed_block"$'\n\n'"$filtered" + else + new_crontab="$managed_block" fi - return 0 fi - # Already installed with same content — nothing to do. - if [[ "$current" == *"$managed_block"* ]]; then - return 0 + # A second read narrows the window in which a concurrent edit could be lost. + # Crontab has no compare-and-swap, so another writer can still race afterward. + local latest latest_status=0 + _cron_list "$cron_command" || latest_status=$? + latest=$REPLY + if ((latest_status != listed)) || [[ "$latest" != "$current" ]]; then + dot_hook_warn " warning: crontab changed or could not be re-read; crontab left unchanged" + return 1 fi - - # Strip any existing managed block. - local filtered - filtered="$(dot_managed_block_strip "$cron_marker" "$current")" - - # Append the new managed block. - local new_crontab - if [[ -n "$filtered" ]]; then - new_crontab="$filtered"$'\n\n'"$managed_block" + if ((remove)); then + "$cron_command" -r else - new_crontab="$managed_block" + printf '%s\n' "$new_crontab" | "$cron_command" - fi - - echo "$new_crontab" | "$cron_command" - } diff --git a/.local/lib/dotfiles/merge-hooks.d/lib/cron-list.sh b/.local/lib/dotfiles/merge-hooks.d/lib/cron-list.sh index 69076296..a125f031 100644 --- a/.local/lib/dotfiles/merge-hooks.d/lib/cron-list.sh +++ b/.local/lib/dotfiles/merge-hooks.d/lib/cron-list.sh @@ -24,6 +24,28 @@ # untranslated. Any other message or exit status (126 and 127 when crontab # cannot run) is a failure, the safe side for both callers. _CRON_LIST_ERR= + +# The generic block stripper matches substrings. Refuse ambiguous input before +# using it so an incomplete or lookalike marker cannot consume a user's jobs. +# Accept zero blocks or exactly one complete pair on lines by themselves. +_cron_block_valid() { + local marker="$1" input="$2" line state=0 + while IFS= read -r line || [[ -n $line ]]; do + case "$line" in + "$marker begin") + ((state == 0)) || return 1 + state=1 + ;; + "$marker end") + ((state == 1)) || return 1 + state=2 + ;; + *"$marker begin"* | *"$marker end"*) return 1 ;; + esac + done <<<"$input" + ((state != 1)) +} + _cron_list() { local sep=$'\x1f''dot-crontab-list'$'\x1f' captured err status line # crontab's stdout goes straight to the outer capture (fd 3), its stderr diff --git a/.local/lib/dotfiles/tests/core/cron.sh b/.local/lib/dotfiles/tests/core/cron.sh index 2c478d09..a8b519bd 100644 --- a/.local/lib/dotfiles/tests/core/cron.sh +++ b/.local/lib/dotfiles/tests/core/cron.sh @@ -180,6 +180,36 @@ EOF "$("${DOT_TEST_ENV_CLEAN[@]}" HOME="$TEST_HOME" PATH="$cron_job_path:$DOT_TEST_SYSTEM_PATH" \ SHELL="$cron_shell" BASH_ENV="$cron_bash_env" GH_TOKEN=ghp_job \ "$cron_shell" -c 'printf "token=[%s]\n" "${GH_TOKEN-}"' 2>&1)" + # A top block makes its environment apply to unmanaged jobs too. A later + # explicit assignment must remain below the managed default and before its + # own job, so the account owner can still override that default. + printf '%s\n' '0 1 * * * unmanaged-before-override' \ + 'SHELL=/bin/sh' 'BASH_ENV=/operator/env' 'PATH=/operator/bin' \ + '0 2 * * * unmanaged-after-override' | crontab - + _run_cron_merge 2>/dev/null + result=$(crontab -l 2>/dev/null) + _assert_eq "unmanaged jobs inherit managed shell, loader and PATH" \ + 'SHELL BASH_ENV PATH' \ + "$(printf '%s\n' "$result" | awk ' + /unmanaged-before-override/ { exit } + /^SHELL=/ { shell=1 } + /^BASH_ENV=/ { env=1 } + /^PATH=/ { path=1 } + END { if (shell && env && path) print "SHELL BASH_ENV PATH" } + ')" + _assert_eq "later unmanaged PATH overrides the managed default" \ + '/operator/bin' \ + "$(printf '%s\n' "$result" | awk ' + /^PATH=/ { value=substr($0,6) } + /unmanaged-after-override/ { print value; exit } + ')" + _assert_eq "later unmanaged shell and loader override managed defaults" \ + '/bin/sh /operator/env' \ + "$(printf '%s\n' "$result" | awk ' + /^SHELL=/ { shell=substr($0,7) } + /^BASH_ENV=/ { env=substr($0,10) } + /unmanaged-after-override/ { print shell, env; exit } + ')" rm -rf "$TEST_HOME/.config/shell" "$TEST_HOME/.config/gh" unset cron_shell cron_bash_env cron_job_path @@ -207,6 +237,95 @@ EOF result=$(crontab -l 2>/dev/null) _assert_contains "preserves other jobs" "other-job" "$result" _assert_contains "installs managed entry" "dot update --cron" "$result" + case "$result" in + '# dot-managed-cron begin'*) _pass "managed block precedes unmanaged jobs" ;; + *) _fail "managed block precedes unmanaged jobs" ;; + esac + + # A byte-identical block in the old appended position still needs migration. + _legacy_block=$(printf '%s\n' "$result" | awk ' + /^# dot-managed-cron begin$/ { in_block=1 } + in_block { print } + /^# dot-managed-cron end$/ { exit } + ') + printf '%s\n\n%s\n' 'MAILTO=custom@example.invalid' "$_legacy_block" | crontab - + _run_cron_merge 2>/dev/null + result=$(crontab -l 2>/dev/null) + case "$result" in + '# dot-managed-cron begin'*) _pass "unchanged legacy block moves to top" ;; + *) _fail "unchanged legacy block moves to top" ;; + esac + _assert_contains "later unmanaged environment remains present" 'MAILTO=custom@example.invalid' "$result" + _before_repeat=$result + _run_cron_merge 2>/dev/null + _assert_eq "top placement is idempotent" "$_before_repeat" "$(crontab -l 2>/dev/null)" + + # A substring-based block stripper must never consume user entries when a + # marker is incomplete, duplicated or merely embedded in a command. + for _bad_crontab in \ + $'# dot-managed-cron begin\n0 6 * * * keep-unclosed' \ + $'# dot-managed-cron end\n0 6 * * * keep-orphan' \ + $'0 6 * * * echo "# dot-managed-cron begin"\n0 7 * * * keep-embedded' \ + $'# dot-managed-cron begin\none\n# dot-managed-cron end\n# dot-managed-cron begin\ntwo\n# dot-managed-cron end\n0 7 * * * keep-duplicate'; do + printf '%s\n' "$_bad_crontab" | crontab - + _bad_status=0 + _run_cron_merge >/dev/null 2>&1 || _bad_status=$? + _assert_eq "malformed marker refuses merge" 1 "$_bad_status" + _assert_eq "malformed marker preserves the entire crontab" "$_bad_crontab" "$(crontab -l 2>/dev/null)" + done + _bad_crontab="$_legacy_block"$'\n# dot-managed-cron begin\n0 8 * * * keep-malformed-tail' + printf '%s\n' "$_bad_crontab" | crontab - + _bad_status=0 + _run_cron_merge >/dev/null 2>&1 || _bad_status=$? + _assert_eq "valid prefix with malformed tail refuses idempotence" 1 "$_bad_status" + _assert_eq "malformed tail remains byte-for-byte" "$_bad_crontab" "$(crontab -l 2>/dev/null)" + # The strip-only path is subject to the same protection. + printf '%s\n' '# filter: hosts=nonexistent-host-xyz' '0 1 * * * ignored' \ + >"$TEST_HOME/.config/dot/merge-hooks.d/cron/cron.d/10-base.cron" + _bad_crontab=$'# dot-managed-cron begin\n0 6 * * * keep-on-empty' + printf '%s\n' "$_bad_crontab" | crontab - + _bad_status=0 + _run_cron_merge >/dev/null 2>&1 || _bad_status=$? + _assert_eq "malformed marker refuses empty-source strip" 1 "$_bad_status" + _assert_eq "empty-source strip preserves malformed crontab" "$_bad_crontab" "$(crontab -l 2>/dev/null)" + + # Simulate another editor changing the crontab between the first listing + # and the final pre-install listing; the merge must not overwrite it. + printf '%s\n' '*/30 * * * * dot update --cron' >"$TEST_HOME/.config/dot/merge-hooks.d/cron/cron.d/10-base.cron" + _race_bin=$(_mock_bin) + _race_state=$(_tmpfile) + _race_count=$(_tmpfile) + printf '%s\n' '0 1 * * * original' >"$_race_state" + : >"$_race_count" + cat >"$_race_bin/crontab" <<'SH' +#!/usr/bin/env bash +case ${1:-} in + -l) + if [[ -s $CRON_RACE_COUNT ]]; then + printf '%s\n' '0 2 * * * concurrent-edit' >"$CRON_RACE_STATE" + else + printf '%s\n' seen >"$CRON_RACE_COUNT" + fi + cat "$CRON_RACE_STATE" ;; + -) cat >"$CRON_RACE_STATE" ;; + -r) : >"$CRON_RACE_STATE" ;; +esac +SH + chmod +x "$_race_bin/crontab" + _race_status=0 + DOT_TEST_CRONTAB="$_race_bin/crontab" CRON_RACE_COUNT="$_race_count" \ + CRON_RACE_STATE="$_race_state" _run_cron_merge >/dev/null 2>&1 || _race_status=$? + _assert_eq "concurrent crontab edit refuses install" 1 "$_race_status" + _assert_eq "concurrent crontab edit survives" '0 2 * * * concurrent-edit' "$(cat "$_race_state")" + printf '%s\n' '# filter: hosts=nonexistent-host-xyz' '0 1 * * * ignored' \ + >"$TEST_HOME/.config/dot/merge-hooks.d/cron/cron.d/10-base.cron" + printf '%s\n' "$_legacy_block" >"$_race_state" + : >"$_race_count" + _race_status=0 + DOT_TEST_CRONTAB="$_race_bin/crontab" CRON_RACE_COUNT="$_race_count" \ + CRON_RACE_STATE="$_race_state" _run_cron_merge >/dev/null 2>&1 || _race_status=$? + _assert_eq "concurrent edit refuses strip-only removal" 1 "$_race_status" + _assert_eq "concurrent edit survives strip-only removal" '0 2 * * * concurrent-edit' "$(cat "$_race_state")" # cron.local entries are merged with cron entries : >"$MOCK_CRONTAB" diff --git a/.local/lib/dotfiles/tests/core/doctor.sh b/.local/lib/dotfiles/tests/core/doctor.sh index cbe73f8e..a756b41a 100644 --- a/.local/lib/dotfiles/tests/core/doctor.sh +++ b/.local/lib/dotfiles/tests/core/doctor.sh @@ -355,6 +355,22 @@ SH _doctor_records _dr_check_cron) _assert_contains "doctor cron: a fresh install is current" \ $'ok\tmanaged cron block is current' "$result" + doctor_installed_block=$(cat "$doctor_cron_file") + printf '%s\n\n%s\n' '0 1 * * * unmanaged-job' "$doctor_installed_block" >"$doctor_cron_file" + result=$(HOME="$TEST_HOME" PATH="$doctor_bin:$PATH" \ + DOT_TEST_CRONTAB="$doctor_bin/crontab-file" \ + DOT_TEST_CRONTAB_FILE="$doctor_cron_file" \ + _doctor_records _dr_check_cron) + _assert_contains "doctor cron: unchanged block below unmanaged entries is stale" \ + $'warn\tmanaged cron block is stale' "$result" + printf '%s\n\n%s\n' "$doctor_installed_block" "$doctor_installed_block" >"$doctor_cron_file" + result=$(HOME="$TEST_HOME" PATH="$doctor_bin:$PATH" \ + DOT_TEST_CRONTAB="$doctor_bin/crontab-file" \ + DOT_TEST_CRONTAB_FILE="$doctor_cron_file" \ + _doctor_records _dr_check_cron) + _assert_contains "doctor cron: duplicate blocks are malformed" \ + $'warn\tmanaged cron block is malformed' "$result" + printf '%s\n' "$doctor_installed_block" >"$doctor_cron_file" # The listing keeps crontab's stdout and stderr apart from one run. Each # case prints the status, then what was listed (REPLY) or the message. cat >"$doctor_bin/crontab-noisy" <<'SH'