fix: the three things that must be true before v0.7.0 is tagged or its advisories published - #359
Merged
Merged
Conversation
…d or its advisories published Found by a five-way pre-tag sweep. Each is small; each blocks something. ## 1. The give-up log line leaked the stream key, and the advisory says it does not GHSA-7jqx-76vq-hvfc claims "The supervisor scrubs the `process exited` line". It does. #311 fixed that line and left `giving up on process` thirty-four lines below it in the same function, reading the same `msg` -- runOnce's error, which carries FFmpeg's stderr and with it the publish URL and its key. The give-up path is the worse of the two to leak on. It fires only after MaxRestarts CONSECUTIVE failures, which is to say after a destination has been refused over and over: exactly the state an operator is in when they stop watching and start copying server.log into an issue. The retry line got the attention because it fires often; this one fires when someone is about to ask a stranger for help. Publishing the advisory before this landed would have claimed a fix that was half done, so this gates the advisories rather than only the tag. TestTheGivingUpLogLineCarriesNoStreamKey covers it, and needed AutoRestart plus a low MaxRestarts and a short backoff to reach the branch at all -- which is why the existing test could not see it: it pins a supervisor that retries for ever, so the give-up arm never ran. Mutation: reverting to `"err", msg` fails the new test and leaves the old one passing, which is the property that matters. ## 2. The secret.key upgrade guard refused upgrades it should have allowed `tar tzf … | grep -q` under `set -o pipefail`. grep -q exits on its first match, tar takes SIGPIPE, the pipeline returns 141, and `if !` inverts that into "the file is missing" -- so a backup that DOES contain secret.key was refused with "Refusing to upgrade." MEASURED IN debian:bookworm-slim, and the mechanism is not what it first looked like. It is not inode order; it is the pipe buffer. Below roughly 200 entries tar finishes writing before grep exits and the guard passes. Above it, tar is still writing and takes the signal: 0 extra files: OLD=pass NEW=pass 10 extra files: OLD=pass NEW=pass 200 extra files: OLD=pass NEW=pass 2000 extra files: OLD=REFUSED NEW=pass So it is deterministic, not flaky, and it would have blocked the upgrade on essentially every install with a real recordings volume. Invisible on macOS, where bsdtar exits 0 on SIGPIPE, which is how it survived local testing -- and acceptance-install.sh only ever exercised the binary-mode script. Fixed by listing once into a variable and testing that. Herestrings rather than `printf | grep`, because printf takes SIGPIPE too: reordering the pipeline moves the bug rather than removing it. It also walks the archive once instead of twice. The guard was added in #348 to prevent data loss, and could block the upgrade it was written to protect. ## 3. A failed binaries job left container tags with no release behind them release.yml had no `needs:` anywhere, so `images` and `binaries` ran in parallel. A binaries failure -- syft, the SBOM guard, an upload -- left Docker Hub and GHCR already carrying :0.7.0, :0.7 and :latest with no GitHub Release, no checksums and no SBOM. That splits the two install modes: install.sh writes `image: <IMAGE>:latest` into the compose file, so docker operators take the new version immediately while binary installs resolve through releases/latest and stay put. Same tag, two populations, one running an artefact whose SBOM never passed its guard. `needs: binaries` costs a serialised wait and buys the property that a container tag never exists without the release it belongs to. ## 4. UPGRADING.md could not have told an operator any of this It had no 0.7.0 section, and grep for seal/encrypt/secret.key across all 196 lines returned nothing -- while 0.7.0 is the release that makes secret.key the difference between a restore and a silent outage. It also never mentioned update.sh, so operators were steered to the hand-rolled procedure rather than the guarded script install.sh writes. Added: the 0.7.0 note, the roll-back warning (0.6.0 reads every destination as having an empty key while still enabled, and there is no schema version for it to refuse on), the secret.key check in "Rolling back", and a keyUnreadable check in "Verifying an upgrade" -- because a restore that omits the key file looks completely successful until go-live. Verified: generated BOTH update.sh variants out of their heredocs and syntax checked them; go build, go vet, and go test -race on internal/supervisor; actionlint on release.yml unchanged at its one pre-existing finding. Claude-Session: https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX
|
There was a problem hiding this comment.
Pull request overview
This PR addresses several pre-v0.7.0 release blockers: preventing stream-key leakage in supervisor logs, fixing an upgrade guard that could falsely refuse valid backups, ensuring release workflow ordering avoids half-published container tags, and documenting the 0.7.0 secret.key implications for upgrades/restores.
Changes:
- Scrub stream keys on the supervisor “giving up on process” log line and add a regression test covering the give-up path.
- Fix the
secret.keyguard in the generatedupdate.shby avoidingtar | grep -qunderpipefail. - Serialize release publication so container images are only published after binaries/SBOM/release assets succeed, and document 0.7.0 upgrade/rollback risks.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 1 comment.
Show a summary per file
| File | Description |
|---|---|
| scripts/install.sh | Reworks backup verification logic to avoid SIGPIPE/pipefail false negatives; validates secret.key via captured listing. |
| internal/supervisor/supervisor.go | Scrubs the “giving up on process” error log to prevent stream key disclosure. |
| internal/supervisor/redact_test.go | Adds a regression test that reaches the give-up branch and asserts keys are masked (not removed). |
| docs/UPGRADING.md | Adds 0.7.0-specific guidance on secret.key, rollback hazards, and post-upgrade verification. |
| docs/notes/pre-tag-sweep-v0.7.0.md | Adds a pre-tag sweep checklist documenting verified blockers and findings. |
| .github/workflows/release.yml | Ensures image publishing waits for binaries job to complete to avoid half-published releases. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| # usable. Restoring without secret.key brings every destination back DISABLED | ||
| # -- correctly, since a key that will not open disables its destination rather | ||
| # than failing open -- and that reads as a successful restore until go-live. | ||
| if ! grep -q "secret\.key" <<<"\$listing"; then |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.



Batch A from the pre-tag sweep (
docs/notes/pre-tag-sweep-v0.7.0.md). Small changes, each blocking something.1. The give-up line leaked the stream key — and GHSA-7jqx says it doesn't
The advisory claims "The supervisor scrubs the
process exitedline." It does. #311 fixed that line and left this one below it in the same function, on the samemsg:msgisrunOnce's error, carrying FFmpeg's stderr and with it the publish URL and its key.This is the worse of the two to leak on. It fires only after
MaxRestartsconsecutive failures — after a destination has been refused over and over, which is exactly when an operator stops watching and starts copyingserver.loginto an issue. The retry line got attention because it fires often; this one fires when someone is about to ask a stranger for help.So this gates the advisories, not just the tag. Publishing GHSA-7jqx before this landed would have claimed a fix that was half done.
The existing test couldn't see it — it pins a supervisor that retries forever, so the give-up arm never runs. The new test needs
AutoRestart+ lowMaxRestarts+ short backoff to reach the branch.Mutation: reverting to
"err", msgfails the new test and leaves the old one passing. That second half is the point.2. The
secret.keyguard refused upgrades it should have allowedtar tzf … | grep -qunderset -o pipefail:grep -qexits on first match →tartakes SIGPIPE → pipeline returns 141 →if !inverts it into "the file is missing."The mechanism is not what the sweep reported. It attributed this to inode order. It's the pipe buffer, measured in
debian:bookworm-slim:So it is deterministic, not flaky, and would have blocked the upgrade on essentially every install with a real recordings volume. Invisible on macOS (bsdtar exits 0 on SIGPIPE) and
acceptance-install.shonly ever exercised the binary-mode script.Herestrings rather than
printf | grep, because printf takes SIGPIPE too — reordering the pipeline would move the bug, not remove it.This guard was added in #348 to prevent data loss, and could block the upgrade it was written to protect.
3. A failed
binariesjob left container tags with no releaserelease.ymlhad noneeds:anywhere. Abinariesfailure left Docker Hub and GHCR carrying:0.7.0/:latestwith no GitHub Release, no checksums, no SBOM.That splits the install modes:
install.sh:729writesimage: <IMAGE>:latest, so docker operators take it immediately while binary installs resolve viareleases/latestand stay put. Same tag, two populations, one running an artefact whose SBOM never passed its guard.4.
UPGRADING.mdcouldn't have warned anyoneNo 0.7.0 section, and
grep -in 'seal\|encrypt\|secret\.key'returned nothing across 196 lines — for the release that makessecret.keythe difference between a restore and a silent outage. It also never mentionedupdate.sh, steering operators to the hand-rolled procedure instead of the guarded script.Added the 0.7.0 note, the roll-back warning (0.6.0 reads every destination as having an empty key while still enabled, with no schema version to refuse on), the
secret.keycheck in Rolling back, and akeyUnreadablecheck in Verifying — because a restore that omits the key file looks completely successful until go-live.Verification
update.shvariants out of their heredocs and syntax-checked them —bash -n install.shdoes not check heredoc contents.go build ./...,go vet,go test -race ./internal/supervisor.actionlint release.ymlunchanged at its one pre-existing finding.Not in this PR
hevc_vaapi(offered and cannot start), capped VBR on NVENC, the advisory text corrections, and the docs sweep. Enhanced Broadcasting is being wired separately.https://claude.ai/code/session_01HeLrWaDmsNeeNSbHQfEofX