akiomik / akiomik/mado

Harden the GitHub Action's download and install

Aperta
#393 2 commenti 0 reazioni 0 assegnatari Vedi su GitHub
ci
Lingua principale
Rust
Stelle
407
Fork
12
Merge medio
5h 45m
PR unite (30g)
30

Descrizione

`action/entrypoint.sh` downloads a release archive and runs the binary out of
it. Several things about how it does that are worth fixing. None is urgent;
each is listed with what it actually costs today.

## Worth doing

- **Quote `$COMMAND_PATH`.** The last line is `$COMMAND_PATH ${INPUT_ARGS}`,
unquoted, so the action fails on a runner whose `$HOME` contains a space
(`C:\Users\First Last`, a self-hosted home directory). `${INPUT_ARGS}` has to
stay unquoted for word splitting; the path does not.
- **Verify the download.** Every release publishes a `.sha256` beside each
archive and nothing looks at it. Fetching it and checking before unpacking is
a few lines, and portable if it picks between `sha256sum` and `shasum -a 256`.
- **Stop at the first failure.** With no `set -e`, a missing release carries the
script through a failing `tar` to `./mado: not found`, which says nothing
about the 404 that caused it.
- **Unpack somewhere else.** `wget` writes the archive into the repository being
checked. A temporary directory of its own, removed by a trap, keeps the
caller's checkout clean.
- **The install guard never matches.** It tests `-x "$COMMAND"` — `./mado`,
relative to the workspace — while the binary is installed to
`$HOME/bin/mado`. So nothing is ever reused between steps, and a repository
that happens to contain an executable named `mado` skips the download and then
fails on a binary that was never installed.

## Needs a decision, not just a patch

Fixing that guard turns `$HOME/bin/mado` into a cache, and then it has to be
keyed by version — otherwise two steps asking for different `version` inputs get
the first one's binary — and by platform, if a `$HOME` is ever shared by
machines that are not alike. Keying it means moving the install under a
directory whose name a caller cannot write down, which is a breaking change for
any workflow that put `$HOME/bin` on `PATH`, and pulls in publishing the new
directory through `$GITHUB_PATH` (and translating it with `cygpath` on Windows).

That is a coherent change, but it is a change to what the action promises, not a
bug fix. Decide it on its own terms.

## Note on how the previous attempt failed

All of this was written inside #390, which was meant to fix one CI gate. That
pull request grew from 63 to 310 lines over 16 commits while a review loop ran
15 rounds, and from the third round on nearly every finding was about code added
in the round before it: unpack to a temp dir → the `mv` is not atomic across
filesystems → move the temp dir under `$HOME/bin` → the trap does not fire on a
signal → add `INT`/`TERM` → that trap now swallows the signal instead of ending
the script. The diff also acquired a user-visible breaking change nobody had
asked for. #390 was trimmed back to the gate it was about, and this is what came
out.

So, when picking this up: one concern per pull request; treat the breaking part
as its own decision; fix correctness and security findings and write the rest
down rather than acting on every one; and stop when a couple of rounds produce
nothing above low severity.

Guida per i contributori

Apri la guida per i contributori

Valutazione

Questa issue non è ancora stata valutata.

Ricevi le nuove issue nella tua casella

Un breve riepilogo di issue GitHub adatte ai principianti.