Skip to content

ci: add shellcheck, gitleaks and script smoke-test workflow - #3

Merged
MRegra merged 3 commits into
mainfrom
fleet/f-0600
Sep 27, 2026
Merged

MRegra merged 3 commits into
mainfrom
fleet/f-0600

Conversation

@MRegra

@MRegra MRegra commented Sep 27, 2026

Copy link
Copy Markdown
Owner

Closes #2

Problem

fedora-workstation-scripts is a public repo with no CI (issue #2 / F-0600). It also has no guard against the failure that took down its automation: the fedora-maintenance and system-health systemd units failed with status=203/EXEC (F-0024) after the scripts were reorganised into per-tool subdirectories, because each unit's ExecStart= still pointed at the old flat path. 203/EXEC means "the executable could not be run" — file missing, no exec bit, or no valid interpreter.

Root cause

No automation validated the scripts or caught a moved/renamed entrypoint. Nothing ran shellcheck, scanned for secrets (relevant for a public bug-bounty repo), or verified that each script was still runnable at its documented path.

Fix

Add .github/workflows/ci.yml, running on push and pull_request with minimal contents: read permissions and all actions pinned to full commit SHAs (with # vX.Y comments):

  • shellcheck — static analysis of every *.sh (runner-provided shellcheck).
  • gitleaks — gitleaks/gitleaks-action scan for committed secrets (fetch-depth: 0).
  • script smoke test — bash -n on every script, then scripts/test-smoke.sh.

scripts/test-smoke.sh is the F-0024 regression guard. For each documented entrypoint it asserts the script exists, starts with a shebang, parses (bash -n), and answers --help with exit 0 — reproducing the 203/EXEC failure class (a moved/renamed/non-runnable entrypoint turns the job red). The executable bit is reported as a warning (CI cannot assert committed file mode; the READMEs document chmod +x).

To make the entrypoints smoke-testable without sudo and without side effects, each of the six shell entrypoints (fedora-maintenance.sh, system-health-check.sh, soft-reboot.sh, system-audit.sh, nightly-launch.sh, install-tools.sh) gained a -h/--help handler that prints usage and exits 0 before any privileged or destructive work (notably install-tools.sh, which otherwise installs packages on any argument). Normal behaviour is otherwise unchanged.

The three pre-existing shellcheck info findings are cleared so the job is green at default severity: nightly-launch.sh now uses an argument array (EXTRA_ARGS=("$@")), and the two intentional cases carry justified inline disables — SC2086 on dnf -y remove $old_kernels (word splitting of a package list is required) and SC2009 on ps aux | grep (needs full command-line text to match attack patterns; pgrep matches process names only).

Tests

  • shellcheck <all 7 scripts> → exit 0, no findings.
  • bash -n <all 7 scripts> → pass.
  • gitleaks dir . → no leaks found (~272 KB scanned).
  • bash scripts/test-smoke.sh → not run in sandbox (arbitrary bash <script> is refused by the fleet allowlist); it runs in the CI smoke job and is re-run by fleet gate. Every --help path was traced by hand and exits 0 without root.
  • actionlint / yamllint → not installed in sandbox; the workflow YAML was reviewed manually and runs in CI.

Risk / rollout

Low. Additive CI plus non-behavioural --help handlers and shellcheck-clean tweaks; no production runtime is touched. gitleaks/gitleaks-action is free for public repos on personal accounts (a GITLEAKS_LICENSE is only required for GitHub organizations), which matches MRegra/fedora-workstation-scripts.

Follow-ups (not in this PR)

  • F-0024 owns fixing the actual systemd unit files (their ExecStart paths live in the user's ~/.config, not in this repo); this PR only guards against the failure class recurring in the repo.
  • Python entrypoints (orchestrator.py, etc.) are out of scope for this shell-focused slice — a follow-up could add ruff/py_compile and a Python smoke check.
  • Committing the scripts with the executable bit (git update-index --chmod=+x) would let the smoke test promote the exec-bit warning to a hard failure.

🤖 Generated with Claude Code


Fleet item F-0600 · owner maya · risk low

Tests run by the implementer

  • shellcheck scripts/test-smoke.sh + all 6 entrypoints → exit 0, no findings
  • bash -n on all 7 scripts → pass
  • gitleaks dir . --no-banner → no leaks found (~272 KB scanned)
  • bash scripts/test-smoke.sh → refused by allowlist (arbitrary bash <script>); runs in CI smoke job, --help paths traced by hand
  • actionlint / yamllint → not installed in sandbox; workflow YAML reviewed manually, runs in CI

🤖 Generated with Claude Code

The repo is public with no CI, and both systemd units failed with
status=203/EXEC (F-0024) after scripts moved into subdirectories,
because ExecStart pointed at a path that no longer existed.

Add .github/workflows/ci.yml running, on push and pull_request:
shellcheck over every *.sh, a gitleaks secret scan, and a smoke job
(bash -n plus scripts/test-smoke.sh). The smoke test guards the
203/EXEC failure class: it asserts each documented entrypoint exists,
has a shebang, parses, and answers --help with exit 0 without sudo.

To make entrypoints smoke-testable, add safe --help handling to all
six shell entrypoints (no behavioural change otherwise), and clear the
three pre-existing shellcheck info findings (array in nightly-launch;
justified SC2086/SC2009 disables). Actions are pinned to full SHAs.

Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>

Fleet-Item: F-0600
Co-Authored-By: Claude <noreply@anthropic.com>
@MRegra

MRegra commented Sep 27, 2026

Copy link
Copy Markdown
Owner Author

fleet review

reviewer: BLOCK

I'm blocking this PR. The workflow, the --help flags and the smoke test are well built and nearly done, but the shellcheck job this PR adds fails on the PR's own head (04e6e2e) in both CI runs (push 36299902216 and pull_request 36299903110). The cause is SC2015 at system-audit/system-audit.sh:38. The implementer's local shellcheck is 0.11.0, which reports nothing. The Ubuntu 24.04 runner uses the older shellcheck from apt, which flags it, so the local check and CI disagree.

Acceptance criteria:

  1. Runs on push and pull_request: met (on: push and pull_request; both runs fired).
  2. shellcheck passes, or warnings are fixed or excluded with a justification: NOT met. CI is red. The two disable directives that were added (SC2086, SC2009) are justified.
  3. gitleaks runs: met, passed in both runs. The action is SHA-pinned and fetch-depth: 0 is set, so the full history is scanned.
  4. Smoke test runs every script with --help or dry-run, without sudo: met. The job is green in CI. The --help check runs before need_root and before any mkdir, touch or log writes in every entrypoint, so it has no side effects.
  5. actionlint passes: not verified. actionlint isn't installed here and nothing in CI runs it. GitHub did parse and run the workflow, and by eye the syntax is fine.
  6. Issue criterion "a regression test reproduces the F-0024 failure mode": partly met. scripts/test-smoke.sh fails when a canonical entrypoint is missing or has no shebang. That covers the "script was moved" cause of 203/EXEC. It does not cover a missing executable bit (only a warning) and does not cross-check the paths the READMEs tell users to symlink.

Safety: this is a repo with no deploy, so there is nothing to roll out. permissions: contents: read, all actions are SHA-pinned, and no secrets are exposed beyond GITHUB_TOKEN for gitleaks. The nightly-launch.sh change from a string to an array keeps normal usage the same and fixes word-splitting.

  • blocker The shellcheck job this PR adds is red on its own head (system-audit/system-audit.sh)
    _color() { [[ -t 1 ]] && printf '%b' "$1" || true; } at line 38 triggers SC2015 (info) on the runner's shellcheck. xargs exits 123 and the job fails. Local shellcheck 0.11.0 is clean, so the implementer's local check didn't catch it. Fix: rewrite it as _color() { if [[ -t 1 ]]; then printf '%b' "$1"; fi; }, or add a justified # shellcheck disable=SC2015. Then pin the shellcheck version in ci.yml (download a fixed release) so local runs and CI use the same version, or set an explicit --severity.
    proof
`gh pr checks 3`: shellcheck fail in runs 36299902216 and 36299903110. Failed log: "In ./system-audit/system-audit.sh line 38 ... SC2015 (info): Note that A && B || C is not if-then-else ... ##[error]Process completed with exit code 123." Local `shellcheck` 0.11.0 on all 7 scripts: no output.
- **minor** The regression test only warns on a missing executable bit, and its reason is wrong (`scripts/test-smoke.sh`) The test says 'CI cannot assert the committed file mode', but git stores the mode (100755 or 100644) and actions/checkout keeps it. A missing +x on `fedora-maintenance/fedora-maintenance.sh` or `system-health/system-health-check.sh` is exactly what gives 203/EXEC through the `/usr/local/bin` symlink in the documented `ExecStart`. For those two systemd targets the check should fail, not warn. The ENTRYPOINTS list also lives inside the test itself. If someone moves a script and updates ENTRYPOINTS but not the README `ln -s` or ExecStart lines, the test still passes. Deriving the paths from the README `ln -s` lines would make it a real guard against F-0024.
proof
`git ls-files -s`: fedora-maintenance.sh and system-health-check.sh are 100755; nightly-launch.sh and scripts/test-smoke.sh are 100644. The test prints WARN, not FAIL, for `! -x`.
- **nit** Each PR commit runs CI twice (`.github/workflows/ci.yml`) Plain `on: push` plus `pull_request` runs every job twice on a PR branch in the same repo (two run IDs per commit). Consider `push: branches: [main]`, or add a concurrency group.
proof
gh pr checks shows every job twice (runs 36299902216 and 36299903110).
attempted
  • git diff origin/main...HEAD (full review of all 9 files)
  • gh pr checks 3 -R MRegra/fedora-workstation-scripts: shellcheck fail x2, gitleaks pass x2, smoke pass x2
  • gh run view 36299902216 --log-failed: SC2015 at system-audit.sh:38
  • shellcheck 0.11.0 locally on all *.sh: clean (differs from CI)
  • bash scripts/test-smoke.sh: refused by the permission mode, not retried; relied on the green CI smoke job
  • actionlint: not installed locally and not run in CI, so nobody has run it
  • git ls-files -s: checked file modes for the exec-bit claim
  • Grep of README ExecStart and ln -s paths against ENTRYPOINTS; top-level side effects checked before --help

Approvals: 0/2 on 04e6e2e

🤖 Generated with Claude Code

The runner's shellcheck (ubuntu-24.04 apt, 0.9.x) flags SC2015 on
`[[ -t 1 ]] && printf || true` in system-audit.sh, which local 0.11 does
not, so the shellcheck job was red on the PR head. Rewrite it as an if,
pin the runner image and print the shellcheck version.

The smoke test now reads the symlink and ExecStart paths from the READMEs
and fails (not warns) when a symlinked script is missing, has no shebang
or is not committed as 100755: the actual 203/EXEC causes behind F-0024.
A self-test proves it goes red on each failure mode. Push CI runs only on
main to avoid duplicate runs, and actionlint lints the workflow.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Fleet-Item: F-0600
Co-Authored-By: Claude <noreply@anthropic.com>
@MRegra

MRegra commented Sep 27, 2026

Copy link
Copy Markdown
Owner Author

fleet implementer, round 2

Round 2 of F-0600, addressing all three review findings.
(1) Blocker: fixed SC2015 at system-audit.sh:38 by rewriting it as if [[ -t 1 ]]; then printf; fi (same behaviour). Pinned CI to runs-on: ubuntu-24.04, which fixes the apt shellcheck version, and added a step that prints shellcheck --version so any drift is visible. Pinning a downloaded shellcheck release wasn't possible: the download needed to compute its checksum was refused.
(2) Minor: rewrote the F-0024 regression test scripts/test-smoke.sh. It now reads the ln -s and ExecStart= lines from the READMEs and fails (instead of warning) when a symlinked script is missing, has no shebang, or isn't committed as 100755 (checked with git ls-files -s). It also fails when an ExecStart binary isn't created by any README symlink. The entrypoint list is now discovered (every *.sh outside scripts/) instead of hand-maintained. Added scripts/test-smoke-selftest.sh, which builds fixture trees (healthy, moved, noexec, unlinked, nohelp) and asserts the smoke test goes red on each failure and green on the healthy one.
(3) Nit: push now only fires on main, so each PR commit runs CI once.
Also added an actionlint job, run with go run …actionlint@v1.7.7 and verified by the Go checksum DB. actionlint passes locally.
Local limits: bash scripts/test-smoke.sh was refused by the permission mode, so I didn't run the smoke test or the self-test locally. I checked them by reading the code; the CI smoke job runs both.

Tests

  • shellcheck --severity=style scripts/test-smoke.sh scripts/test-smoke-selftest.sh system-audit/system-audit.sh soft-reboot/soft-reboot.sh fedora-maintenance/fedora-maintenance.sh system-health/system-health-check.sh bug-bounty-agent/nightly-launch.sh bug-bounty-agent/install-tools.sh -> clean, exit 0 (shellcheck 0.11.0)
  • bash -n scripts/test-smoke-selftest.sh -> exit 0
  • bash -n system-audit/system-audit.sh -> exit 0
  • bash -n scripts/test-smoke.sh -> exit 0
  • go run github.com/rhysd/actionlint/cmd/actionlint@v1.7.7 .github/workflows/ci.yml -> clean, exit 0
  • bash scripts/test-smoke.sh -> refused by allowlist (not run locally; runs in the CI smoke job)
  • bash scripts/test-smoke-selftest.sh -> not run locally (same refusal class); runs in the CI smoke job

🤖 Generated with Claude Code

@MRegra

MRegra commented Sep 27, 2026

Copy link
Copy Markdown
Owner Author

fleet review

reviewer: PASS

I'm passing this PR. The new head (bb6fa48) fixes the blocker from round 1 and both of my earlier minor findings. All four CI jobs are green on the pull_request run 36309575488 (merge commit dc913d8 = bb6fa48 into main): shellcheck, actionlint, gitleaks and the script smoke test.

Acceptance criteria (assignment and issue #2):

  1. Runs on push and pull_request: met. on: push: branches: [main] plus pull_request, which also fixes the double-run nit from last round.
  2. shellcheck passes, with any exclusions justified: met. The SC2015 case at system-audit.sh:38 is rewritten as if/then. The two disable directives (SC2086 on the kernel list, SC2009 on ps aux) have written reasons. The runner image is pinned to ubuntu-24.04 and CI prints the shellcheck version. CI shellcheck is green, and local shellcheck on all 8 scripts gives no output.
  3. gitleaks runs: met. The action is SHA-pinned, fetch-depth: 0 scans the full history, and the job is green.
  4. Smoke test runs every entrypoint with --help, without sudo: met. All 6 entrypoints return exit 0 on --help in the CI log, and bash -n passes on every *.sh. In each script the --help branch comes before need_root and before any mkdir, touch or log write. In fedora-maintenance.sh, for example, the only top-level code before main is a variable assignment.
  5. actionlint passes: met. A new job runs go run github.com/rhysd/actionlint/cmd/actionlint@v1.7.7, and it is green.
  6. Issue criterion "a regression test reproduces the F-0024 failure mode": met.
    • test-smoke.sh now reads the symlink paths from the README ln -s lines instead of keeping its own list.
    • It fails (no longer just warns) when a symlinked script is missing, has no shebang, or is not committed as 100755. It also fails when an ExecStart binary is not created by any README symlink.
    • test-smoke-selftest.sh shows the test goes red on each failure mode (moved, noexec, unlinked, nohelp) and stays green on a healthy tree. The CI log shows all 5 cases ok.

Root cause: the item asks for CI. The 203/EXEC cause (a script moved away from the path behind the unit's ExecStart) is now guarded in CI. The live unit files on the host are not in this repo, so fixing F-0024 on the machine itself is still separate work.

Scope: minimal. Beyond the CI files, the changes are --help flags (needed for the smoke test), the shellcheck fixes, and the nightly-launch.sh change from a string to an array. That last one keeps normal usage the same and fixes word-splitting.

Safety: nothing to deploy (env: repo). permissions: contents: read, all actions are SHA-pinned, and no secret is used beyond GITHUB_TOKEN.

Check I could not run: running bash scripts/test-smoke.sh and bash scripts/test-smoke-selftest.sh locally was refused by the permission mode, and I did not retry. I relied on the CI log for the same commit instead, and both pass there.

  • minor ExecStart check ignores units that call a repo path directly (scripts/test-smoke.sh)
    Section 2 only picks up ExecStart binaries under /usr/local/bin or .local/bin. If a README line were ExecStart=/home/<user>/dev/fedora-workstation-scripts/fedora-maintenance.sh (the pre-symlink shape and a plausible F-0024 variant), the test would skip it silently and still pass. Suggested fix: also match */fedora-workstation-scripts/<path> in ExecStart lines and check that the path exists and is 100755. All current READMEs go through /usr/local/bin, so nothing is broken today.
    proof
grep -oE '(/usr/local/bin|\.local/bin)/[A-Za-z0-9._-]+' is the only extractor applied to ExecStart lines. The current README ExecStart lines (fedora-maintenance/README.md:81,121; system-health/README.md:65,99) all use /usr/local/bin.
- **minor** The local smoke run really executes any future script that ignores --help (`scripts/test-smoke.sh`) Section 3 runs `bash "$script" --help` on every `*.sh` outside scripts/. The README tells developers to run this locally. A future script that doesn't handle `--help` would then run for real on the developer's workstation as that user, not just fail. On CI that's harmless. Consider a static check first (for example, grep for a `--help` branch) or document the risk. The self-test fixture for this case (`exit 3`) is harmless.
proof
test-smoke.sh: `if bash "$script" --help >/dev/null 2>&1; then` with no prior guard. README.md tells users to run `bash scripts/test-smoke.sh` locally.
- **nit** actions/checkout v4.2.2 triggers the Node 20 deprecation warning (`.github/workflows/ci.yml`) The runner forces Node 24 and warns on every job. Bump to a Node 24 checkout release (SHA-pinned) when convenient.
proof
The CI log for run 36309575488 shows: '##[warning]Node.js 20 is deprecated ... actions/checkout@11bd719...'
attempted
  • git diff origin/main...HEAD: reviewed all 10 files; commits 04e6e2e and bb6fa48
  • gh pr checks 3 -R MRegra/fedora-workstation-scripts: actionlint, gitleaks, script smoke test and shellcheck all pass (run 36309575488, head bb6fa48)
  • gh run view 36309575488 --log --job 108592794873: self-test ok for healthy, moved, noexec, unlinked and nohelp; smoke test ALL PASSED, including the README symlink and ExecStart checks
  • gh run list --branch fleet/f-0600: the earlier red runs (36299902216, 36299903110) were on the old head 04e6e2e
  • shellcheck --severity=style locally on all 8 scripts: clean
  • bash scripts/test-smoke.sh and bash scripts/test-smoke-selftest.sh locally: refused by the permission mode, not retried; relied on the CI log for the same commit
  • Grep of README ln -s and ExecStart lines against the test's parser: all 4 ExecStart lines resolve to existing 100755 scripts
  • Grep of top-level statements in fedora-maintenance.sh: no side effects before main, so --help is safe

adversary: BLOCK

Blocking. The CI workflow and the --help flags look sound. shellcheck 0.11 is clean on all 8 scripts. The SC2015 fix (an explicit if/then) is correct. Every script checks --help before any side effect. Reverting this merge is safe: it only touches CI, help flags and the nightly-launch array. The problem is the issue's own acceptance criterion, "a regression test reproduces the F-0024 failure mode", which is still not met. F-0024's recorded failures were: (1) 203/EXEC from a user unit whose ExecStart=%h/dev/fedora-workstation-scripts/fedora-maintenance.sh pointed straight at a repo path, not through a /usr/local/bin symlink; (2) now status=1/FAILURE after 4ms, because ExecStart passes no mode argument and main() runs usage; exit 1. scripts/test-smoke.sh catches neither. Its ExecStart parser keeps only /usr/local/bin/<name> and .local/bin/<name> tokens, so the literal F-0024 ExecStart line is silently dropped. It only ever runs --help, so an ExecStart with no mode stays green. CI would pass on exactly the configuration that caused F-0024, while the README says the test "guards against the systemd status=203/EXEC failure". No open queue item tracks this test gap. F-0024 is needs-human for the host-side unit fix; its operator code_fix proposes versioning the unit files, but no item was filed for that.

  • major The regression test misses both recorded F-0024 failure modes: ExecStart pointing straight at a repo path, and a missing mode argument (scripts/test-smoke.sh)
    Section 2 only considers ExecStart binaries matching (/usr/local/bin|\.local/bin)/[A-Za-z0-9._-]+. F-0024's unit was ExecStart=%h/dev/fedora-workstation-scripts/fedora-maintenance.sh (F-0024.yaml triage evidence, lines 38-41), a direct repo path whose script had been moved. The regex yields nothing for that line, so it is never checked and the test passes. F-0024's current failure is status=1/FAILURE in 4ms: the unit calls fedora-maintenance.sh with no mode, and fedora-maintenance.sh:319 runs [[ -n "$mode" ]] || { usage; exit 1; }. The smoke test only calls --help, which exits 0, so an ExecStart with no argument is also green. The issue's acceptance criterion "a regression test reproduces the F-0024 failure mode" is therefore not met, and the README paragraph added by this PR claims coverage the test doesn't give. Suggested fix: (a) in section 2, also resolve any ExecStart token containing /fedora-workstation-scripts/ (with %h or an absolute prefix) to its repo-relative path, and fail if that file is missing, lacks a shebang or isn't 100755. (b) For fedora-maintenance, fail when a README ExecStart invokes it without a mode argument (daily|monthly|major|%i). (c) Add selftest cases direct-moved and no-mode to scripts/test-smoke-selftest.sh that go red on each. Versioning the real unit files (F-0024 operator code_fix) and checking them with systemd-analyze verify would be stronger still.
    proof
Scratch fixture scratch-f0024/README.md contains the literal F-0024 unit line plus an ExecStart with no mode:
  ExecStart=%h/dev/fedora-workstation-scripts/fedora-maintenance.sh
  ExecStart=/usr/local/bin/fedora-maintenance
First stage of the smoke test pipeline: `grep -hE '^ExecStart=' scratch-f0024/README.md` returns both lines.
Second stage: `grep -oE '(/usr/local/bin|\.local/bin)/[A-Za-z0-9._-]+' scratch-f0024/README.md` returns only `/usr/local/bin/fedora-maintenance`. The `%h/.../fedora-maintenance.sh` path (which does not exist in the repo) is dropped and never checked. The remaining name passes because README.md:25 symlinks it. No check ever runs the entrypoint with the ExecStart arguments, so the no-mode line (which exits 1 per fedora-maintenance.sh:319) is also green.
Suggested selftest case to adopt:
  write_tool "$WORK/direct" tool/tool.sh 755
  printf 'ExecStart=%%h/dev/fedora-workstation-scripts/tool.sh\n' >"$WORK/direct/README.md"
  (plus a valid ln -s line)
  expect direct fail 'tool.sh: missing'
This currently passes (exit 0), because section 2 never sees the path.
- **nit** Symlink lines written as `ln -sf` or `ln -sfn` are silently skipped (`scripts/test-smoke.sh`) The parser regex `ln -s +[^ ]+ +[^ ]+` requires whitespace right after `-s`. If a README switches to the common `ln -sf ` form, that symlink is ignored, and the only guard is the "no ln -s lines found" check when all of them disappear. A pattern such as `ln -s[a-zA-Z]* +` would cover it.
proof
By reading the regex at scripts/test-smoke.sh:65: in `ln -sf /x /y`, `-s` is followed by `f`, not a space, so the line doesn't match.
attempted
  • git diff origin/main...HEAD: read all 10 changed files
  • shellcheck --severity=style on all 8 *.sh with local shellcheck 0.11: clean (CI runs the 0.9.x from ubuntu-24.04 apt; not re-verified because gh is not available)
  • bash scripts/test-smoke.sh: refused by the allowlist; did not retry test-smoke-selftest in another form. bash scripts/test-smoke-selftest.sh: also refused
  • actionlint: not installed locally (exit 127); CI runs it with go run at v1.7.7
  • Read F-0024.yaml: its actual failure modes are a direct %h ExecStart (203/EXEC) and then a missing mode argument (exit 1)
  • Scratch fixture + the smoke test's own two grep stages: the direct-path ExecStart line is dropped
  • Read the queue for open fedora-workstation-scripts items: F-0025/F-0123/F-0142/F-0254 are closed; F-0024 is needs-human (host fix); no open item tracks the test gap
  • Checked each entrypoint for side effects before --help (mkdir, touch, pip, need_root): none; all return before side effects
  • nightly-launch.sh string-to-array change: ${EXTRA_ARGS[*]:-none} and "${EXTRA_ARGS[@]}" are safe under set -u on bash 5
  • Workflow security: permissions contents: read, actions SHA-pinned, GITHUB_TOKEN only for gitleaks, push trigger limited to main
  • Rollback: a revert only removes CI, help flags and the array quoting; no runtime state is involved

Approvals: 1/2 on bb6fa48

🤖 Generated with Claude Code

The adversary showed the smoke test stayed green on the exact F-0024
configuration: an ExecStart pointing straight at a moved repo path
(%h/dev/fedora-workstation-scripts/...) was never parsed, and an
ExecStart with no mode argument (main() exits 1 with usage) was never
checked.

test-smoke.sh now parses whole ExecStart lines and the unit they belong
to. It fails on a missing or non-executable direct repo path, on an
ExecStart that runs nothing from the repo, on a missing or unknown mode
(declared with `# smoke-modes:` in the script and cross-checked against
its case branches), on %i in a unit that is not a template, on a timer
instance that is not a mode, and on a user unit that runs a need_root
script without sudo. It also parses `ln -sf`, never runs a script that
has no --help branch, and adds a timeout.

The new check caught a real doc bug: fedora-maintenance README Option A
passed %i in a non-template user unit, so the mode was empty (the
F-0024 exit 1). It now passes `daily`.

The self-test goes from 5 to 14 cases, one per failure mode.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>

Fleet-Item: F-0600
Co-Authored-By: Claude <noreply@anthropic.com>
@MRegra

MRegra commented Sep 27, 2026

Copy link
Copy Markdown
Owner Author

fleet implementer, round 3

Round 3 of F-0600 fixes the adversary's block: the regression test now catches every recorded F-0024 failure mode. scripts/test-smoke.sh section 2 now reads the whole ExecStart= command along with the unit it belongs to. It fails when: a direct repo path (the literal F-0024 unit %h/dev/fedora-workstation-scripts/...) is missing, has no shebang or isn't 100755; ExecStart runs nothing from the repo; a script that declares # smoke-modes: gets no mode, an unknown mode, or %i in a unit that isn't a template; a name@inst.timer uses an unknown mode; or a user unit runs a need_root script without sudo. Declared modes are cross-checked against the script's case branches.

The new check found a real documentation bug. Option A in fedora-maintenance/README.md was a non-template user unit passing %i, which gives an empty mode (the same failure as F-0024). It now passes daily, and a short note explains why.

Review findings also fixed:

  • ln -sf / ln -sfn lines are now parsed.
  • The smoke test no longer runs a script whose source never mentions --help, and --help runs are limited by timeout 30 with stdin from /dev/null.
  • The test now fails if no ExecStart lines are found.
  • The self-test grows from 5 to 14 cases (direct-moved, no-mode, pct-i-plain, bad-mode, bad-instance, user-no-sudo, mode-drift, foreign, help-fails, plus a check that nohelp never runs the script).

Left out: the Node 20 checkout nit. I couldn't look up a v5 SHA (git ls-remote was refused) and didn't want to guess one.

Not run locally: bash scripts/test-smoke.sh and bash scripts/test-smoke-selftest.sh were refused by the allowlist, as for both reviewers. I checked their logic by hand against the real READMEs using Grep, and CI runs both.

Tests

  • shellcheck --severity=style scripts/test-smoke.sh scripts/test-smoke-selftest.sh fedora-maintenance/fedora-maintenance.sh system-health/system-health-check.sh system-audit/system-audit.sh soft-reboot/soft-reboot.sh bug-bounty-agent/install-tools.sh bug-bounty-agent/nightly-launch.sh -> clean (exit 0)
  • bash -n scripts/test-smoke.sh -> ok
  • bash -n scripts/test-smoke-selftest.sh -> ok
  • bash scripts/test-smoke-selftest.sh -> refused by allowlist (not run locally; CI job 'script smoke test' runs it)
  • bash scripts/test-smoke.sh -> not run (the self-test was refused by the allowlist and I didn't try other forms; CI job 'script smoke test' runs it)
  • awk extraction check on READMEs -> refused by allowlist; checked instead with a Grep of .service/ExecStart= lines

🤖 Generated with Claude Code

@MRegra

MRegra commented Sep 27, 2026

Copy link
Copy Markdown
Owner Author

fleet review

reviewer: PASS

Passing round-3 head 80b7952. It fixes the adversary round-1 major block: the smoke test now catches both recorded F-0024 failure modes, a direct-repo-path ExecStart and a missing mode argument. All acceptance criteria met, CI green, scope tight, safe to deploy since env is repo/CI only.

adversary: PASS

Pass. I couldn't break head 80b7952. It closes the major finding from the last adversary round. The regression test now covers all three F-0024 failure modes recorded in state/items/F-0024.yaml:

  1. 203/EXEC from a moved script, symlinked or called directly. Section 2 now picks up any ExecStart token that contains /fedora-workstation-scripts/, turns it into a repo path and runs check_target on it (test-smoke.sh:140-143).
  2. Exit 1 when no mode is given, or %i in a unit that isn't a template. Handled at test-smoke.sh:166-191. # smoke-modes is cross-checked against the script's case branches, so the list can't drift.
  3. need_root in a user unit that doesn't go through sudo. Handled at test-smoke.sh:193.

The live host unit (a user unit with ExecStart=%h/.../fedora-maintenance/fedora-maintenance.sh and no mode) fails both the no-mode check and the needs-root check. Each mode has its own selftest case (14 cases in total), including direct-moved, no-mode, pct-i-plain and user-no-sudo. The new check found a real bug in the docs: README Option A passed %i in a unit that isn't a template, and the PR fixes it (it now passes daily). The ln -sf nit from last round is fixed (ln +-[A-Za-z]*s[A-Za-z]*). So is the reviewer's minor finding: a script that never mentions --help is no longer executed, and every --help run now has a 30s timeout.

Other checks:

  • shellcheck 0.11 is clean on all 8 scripts.
  • The workflow has permissions: contents: read, SHA-pinned actions, and uses only GITHUB_TOKEN.
  • Reverting this merge is safe: it only touches CI, --help flags, one README fix and the array quoting, with no runtime state.

Not verified:

  • I couldn't run either test script myself: permission mode refused bash scripts/test-smoke.sh, and I didn't retry.
  • I didn't check CI status for 80b7952 (no gh). The reviewer has to confirm it's green.

Remaining gaps: two parser nits, both about README layouts the repo doesn't use today. Nothing currently in the repo slips through.

  • nit User-unit need_root check is skipped when the unit header is a bare file name (scripts/test-smoke.sh)
    The needs-root check (line 193) only fires when unit matches *systemd/user/*. unit is the last *.service token seen before the ExecStart line (awk at lines 120-124). Suppose a README follows the prose it already uses, "Create these files under ~/.config/systemd/user.", with a bare header like fedora-maintenance.service. Then unit=fedora-maintenance.service, the check is skipped, and a user unit that runs fedora-maintenance.sh daily without sudo stays green. That is F-0024 mode 3. Today's READMEs spell out the full ~/.config/systemd/user/...service path, so nothing slips through now. Possible fix: also treat a unit as a user unit when the nearest heading or prose mentions systemd/user or systemctl --user, or fail when a unit's scope (user or system) can't be worked out.
    proof
Scratch fixture scratch-adv/README.md: line 3 `Create these files under ~/.config/systemd/user.`, line 5 `fedora-maintenance.service`, line 10 `ExecStart=%h/dev/fedora-workstation-scripts/fedora-maintenance/fedora-maintenance.sh daily`. Line 3 has no `.service` token, so the awk match() at test-smoke.sh:124 sets unit=`fedora-maintenance.service` from line 5. That fails the `*systemd/user/*` test at :193. The path exists, `daily` is a valid mode, and the line is reported PASS. (I couldn't run the awk stage itself: the permission mode refused it. This is read from the regex.)
- **nit** Indented ExecStart lines (e.g. inside a Markdown list) are never parsed (`scripts/test-smoke.sh`) The awk filter `/^ExecStart=/` (line 123) needs the directive to start in column 0. An ini block indented under a list item is skipped without any warning. The only safety net is the 'no ExecStart lines found' check, and that fires only when every such line disappears. Matching `/^[[:space:]]*ExecStart=/` and stripping the leading whitespace would close this. No current README indents unit blocks.
proof
scratch-adv/README.md line 17: `   ExecStart=%h/dev/fedora-workstation-scripts/fedora-maintenance.sh` (the literal F-0024 moved path, indented 3 spaces). Grep `^ExecStart=` over the fixture returns only line 10. Grep `ExecStart=` returns lines 10 and 17. Line 17 would never be checked.
attempted
  • git log/diff origin/main...HEAD: read all 11 changed files and the new commit 80b7952, which answers the last adversary's major finding
  • Read state/items/F-0024.yaml: recorded modes are 203/EXEC from a direct %h path, exit 1 from no mode, and need_root in a user unit. Traced each one through test-smoke.sh sections 1-2: all are caught. The live host unit shape (user unit, direct path, no mode) fails two checks
  • Read test-smoke-selftest.sh: 14 cases, one per failure mode (direct-moved, no-mode, pct-i-plain, bad-mode, bad-instance, user-no-sudo, mode-drift, nohelp, help-fails, ...). The nohelp case also asserts that the script was never executed
  • Checked the README Option A fix (%i -> daily in a unit that isn't a template) and Option B (%i in fedora-maintenance@.service with @daily/@monthly timers): the test logic accepts both. The pre-PR README would have gone red on 'is not a template'
  • shellcheck --severity=style on all 8 *.sh (local 0.11): clean. bash -n scripts/test-smoke.sh: clean
  • bash scripts/test-smoke.sh: refused by the permission mode, not retried. Running the test's awk stage directly on a scratch fixture: also refused, not retried. Used Grep on the fixture instead
  • Hostile README shapes, checked statically: quoted ExecStart (safe false-fail), sudo -n -u root (handled), bash <repo path> (handled), ExecStart prefixes -@:+! (stripped), ln -sf/-sfn (now matched), bare unit header (gap, nit), indented ExecStart (gap, nit)
  • is_executable for fixtures under a TMPDIR inside a git work tree: git ls-files returns nothing, so it falls back to -x. Robust
  • Workflow security: contents: read, checkout v4.2.2 and gitleaks-action v2.3.9 pinned by SHA, actionlint via go run @v1.7.7 (checksum DB), push limited to main, no secrets beyond GITHUB_TOKEN
  • Cron (Option C) lines pass valid modes via /usr/local/bin. The test doesn't cover them, but F-0024 is systemd-only
  • Rollback: a revert removes CI, --help branches, the README Option A fix and the nightly-launch array. No runtime or migration impact
  • CI status for head 80b7952: not verified (no gh). The reviewer must confirm it's green

security: PASS

Pass on head 80b7952. The PR adds no exploitable issue and doesn't weaken any test, gate or scanner; it adds gates where there were none. CI workflow: permissions: contents: read at the top level, no pull_request_target, only GITHUB_TOKEN (read-only on fork PRs) goes to gitleaks, all third-party actions are pinned to full SHAs, and actionlint comes from a fixed module version checked against sum.golang.org. No new dependencies or install scripts. Script changes: every new --help branch comes before need_root, mkdir, touch or any other side effect. The nightly-launch.sh string-to-array change removes word splitting and globbing of user arguments, so it is a hardening. Test harness: test-smoke.sh runs a repo script only if its source mentions --help, with a 30s timeout and stdin from /dev/null. Values parsed from the READMEs go only into reads (head, git ls-files, grep); the one sed interpolation takes a unit basename limited to [-A-Za-z0-9_.@~%], so it can't break out of the s/// expression. The selftest's rm -rf only targets its own mktemp -d directory. gitleaks over the full history (8 commits) finds no leaks, and local shellcheck (style severity) is clean on every script, including the two new test scripts. Out of scope for this PR's security: whether the tests fully cover F-0024 is the reviewer and adversary's call; the latest commit (80b7952) adds direct-repo-path, missing-mode, %i and need_root-without-sudo checks, which address the adversary's round-2 major finding.

  • minor Documented passwordless sudo on a user-writable script lets any process running as the user become root (pre-existing, re-documented here) (fedora-maintenance/README.md)
    The README symlinks /home/mregra/dev/fedora-workstation-scripts/fedora-maintenance/fedora-maintenance.sh into /usr/local/bin and tells the user to allow sudo -n on it without a password. The target file is owned and writable by the unprivileged user, so any process running as that user (a malicious npm or pip install script, a compromised browser extension helper, and so on) can edit the script and then run it as root through the NOPASSWD rule. This PR didn't introduce it: sudo -n and the passwordless advice were already there. It only changes %i to daily and adds the smoke check that pushes user units toward sudo. Not a blocker. Suggested follow-up issue: install a root-owned copy (sudo install -o root -g root -m 0755 ... /usr/local/sbin/fedora-maintenance) instead of a symlink into $HOME, or use the system timer (Option B, root unit) and drop the NOPASSWD rule.
    proof
fedora-maintenance/README.md: ExecStart=/usr/bin/sudo -n /usr/local/bin/fedora-maintenance daily, and 'make sure sudo is configured for passwordless execution of the maintenance script'. The root README uses `sudo ln -s /home/mregra/dev/fedora-workstation-scripts/... /usr/local/bin/...`: a symlink, so root runs whatever file sits at the user-owned path.
- **nit** I couldn't check here that the action SHAs match their tags (`.github/workflows/ci.yml`) actions/checkout@11bd719 is the well-known v4.2.2 commit. For gitleaks/gitleaks-action@ff98106 (v2.3.9), `gh api` and `git ls-remote` were refused in this session, so I couldn't rule out an imposter commit (a commit that exists only in a fork but can still be referenced through the upstream repo) against the upstream tag. CI resolved both SHAs on the earlier green run. When convenient, confirm with `git ls-remote https://github.com/gitleaks/gitleaks-action 'refs/tags/v2.3.9^{}'` and add Dependabot for github-actions so the pins get updated (this would also clear the reviewer's Node 20 checkout deprecation nit).
proof
ci.yml lines 22/33/42/50/61 pin checkout; line 46 pins gitleaks-action. Network tag lookups were refused by the permission mode.
attempted
  • git diff origin/main...HEAD: read the workflow and the 5 changed entrypoints/READMEs in full, and scripts/test-smoke.sh in full at 80b7952
  • Workflow review: no pull_request_target, top-level permissions contents: read, secrets limited to GITHUB_TOKEN, push trigger limited to main, all actions pinned to SHAs, actionlint via go run @v1.7.7
  • gitleaks detect over the full worktree history: 8 commits scanned, no leaks found
  • shellcheck --severity=style on scripts/*.sh and /.sh at head 80b7952: clean
  • Checked each new --help branch comes before need_root, mkdir, touch or log writes: it does, in all 6 entrypoints
  • Injection review of test-smoke.sh: README-derived paths only reach read-only commands; the sed/grep interpolation of the unit basename is limited to [-A-Za-z0-9_.@~%], so it can't break out of s///
  • test-smoke-selftest.sh: WORK=$(mktemp -d), trap 'rm -rf -- "$WORK"' EXIT, so the delete stays inside its own temp directory
  • git diff --summary: new files committed 100644 (they are run through bash, which is fine); no entrypoint mode changes
  • gh api / git ls-remote to check the action SHAs against their tags: refused by the permission mode, not retried

Approvals: 3/3 on 80b7952

🤖 Generated with Claude Code

@MRegra
MRegra merged commit 5cebcbb into main Sep 27, 2026
5 checks passed
@MRegra
MRegra deleted the fleet/f-0600 branch September 27, 2026 15:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Add CI: shellcheck, gitleaks, script smoke test

1 participant