From 2bc31cf58756d10463215e7cf1ece8d6db3f9eff Mon Sep 17 00:00:00 2001 From: kuhy Date: Fri, 28 Aug 2026 19:28:06 +0200 Subject: [PATCH] Ratchet MORNING_END_HOUR, in the direction it actually runs adjust_shutdown_schedule.sh compared only $1 and $2, so the third value passed through unchecked in both directions: the morning shutdown window could be shortened at will while the two evening hours were guarded. The fix is not the obvious one. day-specific-shutdown-check.sh shuts down while the clock is BELOW morning_end_minutes, so a LARGER MORNING_END_HOUR means a longer shutdown window -- stricter. Copying the evening hours' `-gt` comparison would have ratcheted it backwards and blessed exactly the loosening it was meant to stop. The guard is `-lt`, and the inversion is now stated in the header rather than left to be rediscovered. Two smaller fixes in the same path: validation accepted 24 while its own error message said 0-23, and the config is now unset before being sourced so an unreadable file falls back to the documented loosest defaults instead of inheriting whatever those names last held. --restore is deliberately unchanged. It looks like a bypass, and it is one, but it is load-bearing: shutdown_base.json says 21 while the live config is 23, because the workout reward pushes the schedule later THROUGH --restore. Bounding it at the base -- as was originally proposed -- would have capped the reward at 21 and broken a daily flow. Bounding it needs a policy ceiling, which is a decision, not a refactor. The script is now sourceable, with the config path overridable, so the ratchet has tests at all: 20 cases covering both directions, the source-clobber trap, and the missing/unreadable config fallbacks. A ratchet with no test is how the $3 gap survived for months. Verified in a vmbox sandbox as root against a real /etc and a real chattr (the immutable flag genuinely applied, which a tmpfs cannot do): a stricter write lands, a looser evening hour is refused, a shortened morning window is refused with the reason, and --restore still loosens. Not exercised against the host's own /etc/shutdown-schedule.conf, deliberately. Co-Authored-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_01B5jMviyk15MKw1PqW1pzKr --- screen_locker/adjust_shutdown_schedule.sh | 206 ++++++++++++------ .../shell/test_adjust_shutdown_schedule.sh | 161 ++++++++++++++ 2 files changed, 295 insertions(+), 72 deletions(-) create mode 100755 screen_locker/tests/shell/test_adjust_shutdown_schedule.sh diff --git a/screen_locker/adjust_shutdown_schedule.sh b/screen_locker/adjust_shutdown_schedule.sh index fc69504..76309dc 100755 --- a/screen_locker/adjust_shutdown_schedule.sh +++ b/screen_locker/adjust_shutdown_schedule.sh @@ -1,93 +1,155 @@ #!/bin/bash -# Helper script to adjust shutdown schedule -# This script should be allowed via sudoers for the workout locker +# Helper script to adjust the shutdown schedule, allowed via sudoers. # -# Usage: sudo adjust_shutdown_schedule.sh [--restore] +# Usage: sudo adjust_shutdown_schedule.sh [--restore] # -# --restore: Allow restoring to original (possibly later) times -# Without this flag, only stricter (earlier) times are allowed +# Without --restore only STRICTER schedules are accepted. "Stricter" is not the +# same direction for all three values, which is the subtlety this script got +# wrong for months: +# +# MON_WED_HOUR / THU_SUN_HOUR -> smaller is stricter (shut down earlier) +# MORNING_END_HOUR -> LARGER is stricter +# +# day-specific-shutdown-check.sh shuts down while the clock is *below* +# morning_end_minutes, so raising MORNING_END_HOUR lengthens the morning +# shutdown window. Comparing it with `-gt`, as the two evening hours do, would +# ratchet it backwards and quietly allow the window to be shortened. # # Add to /etc/sudoers.d/workout-locker: -# ALL=(root) NOPASSWD: /home/kuhy/testsAndMisc/python_pkg/screen_locker/adjust_shutdown_schedule.sh +# ALL=(root) NOPASSWD: /home/kuhy/screen-locker/screen_locker/adjust_shutdown_schedule.sh set -euo pipefail -CONFIG_FILE="/etc/shutdown-schedule.conf" -GUARD_NAME="shutdown-schedule" +# Overridable so the test harness can point at a fixture tree. Production never +# sets these; a test that forgets to is writing to the real /etc, which is why +# the harness sets both explicitly rather than relying on a default. +CONFIG_FILE="${SHUTDOWN_CONFIG_FILE:-/etc/shutdown-schedule.conf}" +GUARD_NAME="${SHUTDOWN_GUARD_NAME:-shutdown-schedule}" +readonly CONFIG_FILE GUARD_NAME -CANONICAL_FILE="$(guardctl file-guard canonical-path "$GUARD_NAME" 2>/dev/null || true)" -if [[ -z "$CANONICAL_FILE" ]]; then - echo "Error: guard-lib instance '$GUARD_NAME' is not installed (guardctl file-guard canonical-path returned empty)" >&2 - exit 1 -fi +readonly MAX_HOUR=23 -# Check for --restore flag -RESTORE_MODE=false -if [[ "${1:-}" == "--restore" ]]; then - RESTORE_MODE=true - shift -fi +usage() { + echo "Usage: $0 [--restore] " >&2 + exit 1 +} -# Validate arguments -if [[ $# -ne 3 ]]; then - echo "Usage: $0 [--restore] " >&2 - exit 1 -fi +# Validate that every argument is an hour in [0, 23]. +validate_hours() { + local hour + for hour in "$@"; do + if ! [[ "$hour" =~ ^[0-9]+$ ]] || [[ "$hour" -gt $MAX_HOUR ]]; then + echo "Error: Hours must be integers between 0 and ${MAX_HOUR}" >&2 + return 1 + fi + done +} -MON_WED_HOUR="$1" -THU_SUN_HOUR="$2" -MORNING_END_HOUR="$3" +# Reject a schedule that is looser than the one currently on disk. +# +# Reads the OLD values by sourcing the config, which overwrites any same-named +# variable in this shell -- so the new values must be passed in as arguments and +# never read from a global. Getting that wrong compares a value with itself and +# always passes. +check_stricter_only() { + local new_mon_wed="$1" new_thu_sun="$2" new_morning_end="$3" + local old_mon_wed old_thu_sun old_morning_end -# Validate hours are integers between 0-23 -for hour in "$MON_WED_HOUR" "$THU_SUN_HOUR" "$MORNING_END_HOUR"; do - if ! [[ "$hour" =~ ^[0-9]+$ ]] || [[ "$hour" -lt 0 ]] || [[ "$hour" -gt 24 ]]; then - echo "Error: Hours must be integers between 0 and 23" >&2 - exit 1 - fi -done + [[ -f "$CONFIG_FILE" ]] || return 0 -# Read current values to check if we're making schedule stricter -if [[ -f "$CONFIG_FILE" ]] && [[ "$RESTORE_MODE" == false ]]; then - # shellcheck source=/dev/null - source "$CONFIG_FILE" 2>/dev/null || true - OLD_MON_WED="${MON_WED_HOUR:-24}" - OLD_THU_SUN="${THU_SUN_HOUR:-24}" + # Cleared first so a config that is present but unreadable falls back to the + # defaults below instead of silently inheriting whatever these names last + # held. In production the script is a fresh process and they are unset + # anyway; this makes the function honest when called twice in one shell. + unset MON_WED_HOUR THU_SUN_HOUR MORNING_END_HOUR + # shellcheck source=/dev/null + source "$CONFIG_FILE" 2>/dev/null || true + # Defaults are the LOOSEST value in each direction, so an unreadable config + # can never reject a legitimate first write. + old_mon_wed="${MON_WED_HOUR:-$MAX_HOUR}" + old_thu_sun="${THU_SUN_HOUR:-$MAX_HOUR}" + old_morning_end="${MORNING_END_HOUR:-0}" - # Reset variables to new values for comparison - # shellcheck disable=SC2034 - MON_WED_HOUR_OLD="$OLD_MON_WED" - # shellcheck disable=SC2034 - THU_SUN_HOUR_OLD="$OLD_THU_SUN" + if [[ "$new_mon_wed" -gt "$old_mon_wed" ]] || + [[ "$new_thu_sun" -gt "$old_thu_sun" ]]; then + echo "Error: Can only make schedule stricter (earlier shutdown times)" >&2 + echo "Use --restore flag to restore original times" >&2 + return 1 + fi - # Only allow making schedule stricter (earlier shutdown) unless in restore mode - if [[ "$1" -gt "${MON_WED_HOUR_OLD:-24}" ]] || [[ "$2" -gt "${THU_SUN_HOUR_OLD:-24}" ]]; then - echo "Error: Can only make schedule stricter (earlier shutdown times)" >&2 - echo "Use --restore flag to restore original times" >&2 - exit 1 - fi -fi + # Note the inverted comparison: a SMALLER morning end is looser. + if [[ "$new_morning_end" -lt "$old_morning_end" ]]; then + echo "Error: Can only make schedule stricter (a later morning end)" >&2 + echo " ${old_morning_end}:00 -> ${new_morning_end}:00 shortens the" \ + "morning shutdown window" >&2 + echo "Use --restore flag to restore original times" >&2 + return 1 + fi +} -NEW_CONFIG="# Shutdown schedule configuration +# Resolve the guard-lib canonical copy, failing loudly when the guard is absent. +resolve_canonical() { + local canonical + canonical="$(guardctl file-guard canonical-path "$GUARD_NAME" 2>/dev/null || true)" + if [[ -z "$canonical" ]]; then + echo "Error: guard-lib instance '$GUARD_NAME' is not installed" \ + "(guardctl file-guard canonical-path returned empty)" >&2 + return 1 + fi + printf '%s' "$canonical" +} + +# Write both copies, canonical first. +# +# Order matters: shutdown-schedule-guard.path triggers on CONFIG_FILE and +# restores it from the canonical copy, so writing the watched file first races +# the guard and loses. +write_schedule() { + local canonical="$1" body="$2" + + chattr -i "$CONFIG_FILE" 2>/dev/null || true + chattr -i "$canonical" 2>/dev/null || true + + printf '%s' "$body" >"$canonical" + chmod 644 "$canonical" + chattr +i "$canonical" || echo "Warning: Could not set immutable on $canonical" >&2 + + printf '%s' "$body" >"$CONFIG_FILE" + chmod 644 "$CONFIG_FILE" + chattr +i "$CONFIG_FILE" || echo "Warning: Could not set immutable on $CONFIG_FILE" >&2 +} + +main() { + local restore_mode=false + if [[ "${1:-}" == "--restore" ]]; then + restore_mode=true + shift + fi + + [[ $# -eq 3 ]] || usage + local mon_wed="$1" thu_sun="$2" morning_end="$3" + + validate_hours "$mon_wed" "$thu_sun" "$morning_end" || exit 1 + + if [[ "$restore_mode" == false ]]; then + check_stricter_only "$mon_wed" "$thu_sun" "$morning_end" || exit 1 + fi + + local canonical body + canonical="$(resolve_canonical)" || exit 1 + body="# Shutdown schedule configuration # Modified by screen_locker sick day feature at $(date) -MON_WED_HOUR=$1 -THU_SUN_HOUR=$2 -MORNING_END_HOUR=$3 +MON_WED_HOUR=${mon_wed} +THU_SUN_HOUR=${thu_sun} +MORNING_END_HOUR=${morning_end} " + write_schedule "$canonical" "$body" -# Remove immutable attributes -chattr -i "$CONFIG_FILE" 2>/dev/null || true -chattr -i "$CANONICAL_FILE" 2>/dev/null || true + echo "Shutdown schedule updated: Mon-Wed=${mon_wed}:00," \ + "Thu-Sun=${thu_sun}:00, Morning end=${morning_end}:00" +} -# Write canonical copy FIRST (before the watched config) to avoid -# a race with shutdown-schedule-guard.path which triggers on CONFIG_FILE -# changes and restores from CANONICAL_FILE. -echo "$NEW_CONFIG" > "$CANONICAL_FILE" -chmod 644 "$CANONICAL_FILE" -chattr +i "$CANONICAL_FILE" || echo "Warning: Could not set immutable on $CANONICAL_FILE" >&2 - -# Now write the watched config — guard will see content matches canonical -echo "$NEW_CONFIG" > "$CONFIG_FILE" -chmod 644 "$CONFIG_FILE" -chattr +i "$CONFIG_FILE" || echo "Warning: Could not set immutable on $CONFIG_FILE" >&2 - -echo "Shutdown schedule updated: Mon-Wed=${1}:00, Thu-Sun=${2}:00, Morning end=${3}:00" +# Sourced by the test harness; executed in production. +if [[ "${BASH_SOURCE[0]}" == "${0}" ]]; then + main "$@" +fi diff --git a/screen_locker/tests/shell/test_adjust_shutdown_schedule.sh b/screen_locker/tests/shell/test_adjust_shutdown_schedule.sh new file mode 100755 index 0000000..3b82c52 --- /dev/null +++ b/screen_locker/tests/shell/test_adjust_shutdown_schedule.sh @@ -0,0 +1,161 @@ +#!/usr/bin/env bash +# Tests for the shutdown-schedule ratchet. +# +# This ratchet had no test at all, which is how MORNING_END_HOUR went +# unchecked for months while the two evening hours were guarded. The direction +# cases below are the point of the file: "stricter" means SMALLER for the +# evening hours and LARGER for the morning end. +# +# Nothing here touches /etc: SHUTDOWN_CONFIG_FILE points at a tmpdir, and +# chattr/guardctl are shimmed on PATH because chattr cannot mark a file +# immutable on tmpfs and would abort the write. +set -uo pipefail + +SCRIPT_DIR="$(cd "$(dirname "${BASH_SOURCE[0]}")" && pwd)" +readonly SCRIPT_DIR +readonly TARGET="${SCRIPT_DIR}/../../adjust_shutdown_schedule.sh" + +PASS=0 +FAIL=0 + +_t_pass() { + PASS=$((PASS + 1)) + printf ' OK: %s\n' "$1" +} + +_t_fail() { + FAIL=$((FAIL + 1)) + printf ' FAIL: %s\n' "$1" +} + +# The ratchet returns 0 to allow a write and 1 to block it. Which way it fails +# is the entire point, so assert on the exit status explicitly. +_t_allows() { + local what="$1" + shift + if "$@" >/dev/null 2>&1; then + _t_pass "$what" + else + _t_fail "$what (blocked when it should have allowed)" + fi +} + +_t_blocks() { + local what="$1" + shift + if "$@" >/dev/null 2>&1; then + _t_fail "$what (allowed when it should have blocked)" + else + _t_pass "$what" + fi +} + +_t_has() { + local haystack="$1" needle="$2" what="$3" + if [[ "$haystack" == *"$needle"* ]]; then + _t_pass "$what" + else + _t_fail "$what (want substring '${needle}')" + fi +} + +TMP="$(mktemp -d)" +trap 'rm -rf "$TMP"' EXIT + +# Shim the two commands that need root or a real filesystem. +mkdir -p "$TMP/bin" +cat >"$TMP/bin/chattr" <<'EOF' +#!/bin/bash +exit 0 +EOF +cat >"$TMP/bin/guardctl" <<'EOF' +#!/bin/bash +printf '%s' "${FAKE_CANONICAL:-}" +EOF +chmod +x "$TMP/bin/chattr" "$TMP/bin/guardctl" +export PATH="$TMP/bin:$PATH" + +export SHUTDOWN_CONFIG_FILE="$TMP/shutdown-schedule.conf" +export FAKE_CANONICAL="$TMP/canonical" + +write_config() { + cat >"$SHUTDOWN_CONFIG_FILE" <"$SHUTDOWN_CONFIG_FILE" +_t_allows "falls back to the loosest defaults rather than rejecting" \ + check_stricter_only 23 23 0 + +echo "end to end" +write_config 23 23 5 +if (main 21 21 5 >/dev/null 2>&1); then + _t_pass "a stricter write succeeds" +else + _t_fail "a stricter write succeeds" +fi +_t_has "$(cat "$SHUTDOWN_CONFIG_FILE")" "MON_WED_HOUR=21" "config was rewritten" +_t_has "$(cat "$FAKE_CANONICAL")" "MON_WED_HOUR=21" "canonical copy was written too" + +write_config 21 21 5 +if (main 23 23 5 >/dev/null 2>&1); then + _t_fail "a looser write is refused" +else + _t_pass "a looser write is refused" +fi +_t_has "$(cat "$SHUTDOWN_CONFIG_FILE")" "MON_WED_HOUR=21" "config was left untouched" + +# --restore still bypasses the ratchet on purpose: the workout reward pushes the +# schedule later than shutdown_base.json, so bounding it here would break a +# daily flow. Bounding it is a separate, open decision. +write_config 21 21 5 +if (main --restore 23 23 5 >/dev/null 2>&1); then + _t_pass "--restore still loosens (deliberately unchanged)" +else + _t_fail "--restore still loosens (deliberately unchanged)" +fi + +echo +printf 'passed: %d, failed: %d\n' "$PASS" "$FAIL" +[[ $FAIL -eq 0 ]]