diff --git a/.github/workflows/release.yml b/.github/workflows/release.yml index 6db0611e..65225519 100644 --- a/.github/workflows/release.yml +++ b/.github/workflows/release.yml @@ -64,6 +64,23 @@ jobs: images: name: container images runs-on: ubuntu-latest + # PUBLISHED LAST, BECAUSE A CONTAINER TAG IS THE ONE THING HERE THAT CANNOT + # BE UNPUBLISHED QUIETLY. These two jobs used to run in parallel, so a + # `binaries` failure -- a syft download, the SBOM guard, an upload -- left + # Docker Hub and GHCR already carrying :0.7.0, :0.7 and :latest with no + # GitHub Release, no binaries, no checksums and no SBOM behind them. + # + # That is worse than a half-finished release looks, because the two install + # modes diverge: install.sh writes `image: :latest` into the compose + # file, so every docker operator running update.sh takes the new version + # immediately, while binary installs resolve through releases/latest and + # stay on the old one. Same tag, two populations, one of them running an + # artefact whose SBOM never passed its guard. + # + # Re-pushing identical tags is idempotent, so this costs a serialised wait + # and buys the property that a container tag never exists without the + # release it belongs to. + needs: binaries permissions: contents: read # checkout only; this job never writes to the repository packages: write # ghcr.io, via GITHUB_TOKEN diff --git a/docs/UPGRADING.md b/docs/UPGRADING.md index a3e87fc7..fe494ffc 100644 --- a/docs/UPGRADING.md +++ b/docs/UPGRADING.md @@ -5,8 +5,18 @@ polyemesis migrates its own database on startup. In the normal case, upgrading is: stop, replace the binary or pull the image, start. -**Back up `` first.** Migrations run forward only — there is no -downgrade path, and a backup is the only way back. +**Back up `` first, and check that the backup contains `secret.key`.** +Migrations run forward only — there is no downgrade path, and a backup is the +only way back. From 0.7.0 a backup without that one file is not a backup; see +[Upgrading to 0.7.0](#upgrading-to-070-sealed-stream-keys--breaking-to-roll-back) +before you start. + +**`install.sh` writes a guarded `update.sh` that does all of this for you** — it +takes the backup, refuses to proceed if the archive is empty or missing +`secret.key`, and only then pulls. If you installed with `install.sh`, run +`/update.sh` rather than the manual steps below. Operators who +installed before 0.7.0 do not have it: re-run `install.sh` to regenerate it, or +follow the manual procedure and do the `secret.key` check by hand. ```sh # Binary @@ -68,6 +78,42 @@ instead. ## Version-specific notes +### Upgrading to 0.7.0: sealed stream keys — **breaking to roll back** + +0.7.0 encrypts every destination stream key at rest. The key that opens them is +`secret.key`, in ``, and it is generated on first start. + +**A restore without `secret.key` looks completely successful and is not.** The +server starts, the database opens, every destination is listed — and each one +comes back **disabled**, because a key that will not decrypt disables its +destination rather than failing open with a wrong key. Nothing is wrong until +you go live, which is the worst moment to find out. + +It is easy to get wrong, because `secret.key` is generated silently when it is +absent. Restore the database without it and the server mints a fresh one, so +there is no error to notice — just a new key that cannot open the old rows. + +```sh +# Check your backup before you rely on it. +tar tzf backup-.tar.gz | grep secret.key +``` + +**Rolling back to 0.6.0 blanks every stream key.** The sealing migration clears +the plaintext column, and 0.6.0 has no concept of the encrypted one — so the +older binary reads every destination as having an empty key while still marked +enabled. There is no schema version for it to refuse on. Once you have started +0.7.0 against a database, treat the upgrade as one-way and go back via the +backup rather than by reinstalling the old version. + +If you already have destinations showing as disabled after a restore, the +`keyUnreadable` field on `GET /api/v1/destinations` says which, and re-entering +the key on each one fixes it. + +Also in 0.7.0: **a stream key containing a control character is now refused +when you save it** rather than being silently truncated. A destination carrying +such a key — most often from a terminal paste that appended an escape sequence +— must have its key re-entered before it can be saved again. + ### Upgrading to multi-source The existing configuration becomes the **default source**, automatically. All @@ -181,6 +227,11 @@ you can stop. Restoring the data directory is not optional. The database will have been migrated, and the older binary will not understand it. +**Restore the whole directory, including `secret.key`.** Restoring only +`polyemesis.db` is the mistake this section exists to prevent: from 0.7.0 the +database alone is not enough to publish, and the failure is silent until you go +live. See [Upgrading to 0.7.0](#upgrading-to-070-sealed-stream-keys--breaking-to-roll-back). + ## Verifying an upgrade ```sh @@ -188,6 +239,18 @@ polyemesis -version curl -s localhost:8080/api/v1/health ``` +**Check no destination came back disabled.** From 0.7.0 this is the first thing +to look at after an upgrade or a restore, because it is the one failure that +looks like success: + +```sh +curl -s localhost:8080/api/v1/destinations | grep -c keyUnreadable +``` + +Anything above zero means those destinations could not decrypt their stream key +— almost always a restore that omitted `secret.key`. Re-enter the key on each, +or restore the file and restart. + Then, in the UI: the ingest goes live, each destination reports running, and the **Meters** page shows loudness after routing. That last one is the real check — it is measured from what the platforms actually receive, so if it looks right, diff --git a/docs/notes/pre-tag-sweep-v0.7.0.md b/docs/notes/pre-tag-sweep-v0.7.0.md new file mode 100644 index 00000000..3ade4c60 --- /dev/null +++ b/docs/notes/pre-tag-sweep-v0.7.0.md @@ -0,0 +1,180 @@ +# Pre-tag sweep for v0.7.0 + +Five parallel read-only sweeps against `main` @ `f62fb74`, 2026-08-14, before +tagging v0.7.0. Every row marked **verified** was reproduced against the source +or by running something; **reported** means a sweep asserted it and it has not +been independently checked here. + +All five sweeps have reported. The two external reviewers (codex and agy) both +returned the verdict **"do not tag"**, independently and for different reasons. + +--- + +## The two that matter most + +**B0a — GHSA-7jqx would be published claiming a fix that is partial.** The +advisory says *"The supervisor scrubs the `process exited` line."* True. But +`supervisor.go:703`, in the same function and on the same `msg` variable, logs +`"err", msg` **unscrubbed** on the give-up path: + +```go +:669 p.log.Warn("process exited", "err", p.scrub(msg), …) // scrubbed +:703 p.log.Error("giving up on process", …, "err", msg) // NOT scrubbed +``` + +`msg` is `runOnce`'s error carrying FFmpeg stderr, including the publish URL and +key. A refused destination with `MaxRestarts` set writes its key to `server.log` +after N retries — the exact leak the advisory claims closed. No test covers it +(`grep 'giving up' *_test.go` → nothing). Found by the sweep agent itself; +neither external reviewer caught it. **Verified.** + +**B0b — Twitch Enhanced Broadcasting is a phantom feature.** +`grep -rn "polyemesis/internal/multitrack" --include='*.go'` returns **nothing** +outside the package's own tests. `multitrack.Negotiate` has no non-test caller. +`Destination.Multitrack` is persisted and **never read** by engine or API. + +Yet: `DestinationDialog.tsx:1865` ships a toggle promising *"Asks Twitch at +go-live … and says so once"*; `features.astro:29` says it *"negotiates"*; five +docs describe it as working; and `WEBSITE-COPY-PROPOSAL.md:25` proposes public +marketing copy asserting it end to end. `CHANGELOG.md:282` is the only honest +statement in the tree — *"Nothing publishes through it yet."* + +The generic two-mix egress **is** genuinely wired (`routing.CompilePair` → +`engine/destinations.go:310` → `ffmpeg.secondAudioMap`), so "two mixes to one +destination" is defensible. The *Twitch* framing is not. **Verified.** + +--- + +## Blockers — do not tag until resolved + +| # | Finding | Evidence | Status | +|---|---|---|---| +| B0a | Supervisor give-up path logs the stream key unscrubbed — see above | `internal/supervisor/supervisor.go:669` vs `:703` | verified | +| B0c | **The VOD second audio track pushes two tracks to a one-track ingest, silently.** The engine compiles the pair on `row.VODProfile != nil` alone and never consults `row.Multitrack`; `Validate()` mentions neither field. The field comment claims *"Nothing here enforces that pairing — **the engine reports it**"*. It does not. Combined with B0b (no negotiation ever happens), enabling the VOD mix on Twitch pushes two audio tracks to the ordinary RTMP ingest, which by this codebase's own documentation takes one. Twitch drops or rejects it and nothing warns. **This is the headline #141 feature failing on its primary platform.** | `internal/engine/destinations.go:90`; `internal/db/destinations.go:164-168` | verified | +| B0b | Enhanced Broadcasting inert; UI toggle and marketing claim it works — see above | `internal/multitrack` imported by nothing | verified | +| B10 | **`docs/UPGRADING.md` has no 0.7.0 section and no `secret.key` check.** Following it can disable every destination. `secrets.LoadOrCreate` *silently generates a fresh key* when the file is missing, so the server starts clean and the failure only shows at go-live. `install.sh:1038` names this gap in its own comment. Compounding it, **no markdown file anywhere mentions `update.sh`** — the guarded path is undocumented, so operators are steered to the unguarded hand-rolled procedure. | `docs/UPGRADING.md`; `internal/secrets/secrets.go:41-69` | reported | +| B11 | **`docs/PLATFORMS.md:348` states `moderation:ban` is "deliberately not requested". It is requested** — `internal/oauth/kick.go:102`. A false statement about what a consent screen will ask for. | `internal/oauth/kick.go:102` | reported | +| B12 | **`docs/HARDWARE.md:71` states the opposite of the code.** It says libx264 stays the default on a machine with a working GPU and "hardware is an opt-in". `Tools.DefaultVideoEncoder` returns the first *probe-passing* encoder from a preference list that puts hardware first. `RENDITIONS.md:295` has it right — internal contradiction. | `internal/ffmpeg/detect.go:406-434` | reported | +| B1 | **The `secret.key` upgrade guard falsely refuses legitimate upgrades.** `install.sh:1049` sets `pipefail`; `:1082` runs `tar tzf … \| grep -q "secret\.key"`. `grep -q` exits on first match, `tar` takes SIGPIPE, the pipeline returns 141, and `if !` inverts that into the error branch — so a backup that *does* contain the file is rejected. Fires or not depending on `readdir` order, i.e. inode order on ext4. Reproduced in `debian:bookworm-slim`: entry 1 of 4002 → false refusal, entry 4001 → pass. Invisible on macOS (bsdtar returns 0 on SIGPIPE) and `acceptance-install.sh` only exercises the binary-mode path. **Fix:** `listing="$(tar tzf "$f")"` once, then test the listing. | `scripts/install.sh:1049,1082` | verified | +| B2 | **`hevc_vaapi` is selectable and cannot start.** `encoderProfiles` contains only `h264_vaapi` with `vaapi:true`. `hevc_vaapi` takes the unknown branch, so `prof.vaapi` is false and the argv gets neither `-vaapi_device` nor `format=nv12,hwupload`. Never probed, so never greyed out; the start gate only refuses on *measured* failures. Same root cause leaves `libx265` without `-profile:v high -pix_fmt yuv420p`. | `internal/ffmpeg/rendition.go:204,241-257,467-471`; `internal/db/renditions.go:36` | verified | +| B3 | **Capped VBR — the flagship #341 feature — does not work on NVENC.** `EncoderNVENC.rateControl` is `{"-rc","cbr","-profile:v","high"}`, appended at `:296` immediately before `-b:v/-maxrate/-bufsize`. An `h264_nvenc` rendition with a ceiling above target still runs CBR. `ENCODING.md:68` claims capped VBR works, unqualified. | `internal/ffmpeg/rendition.go:188-192,296-301`; `docs/ENCODING.md:68` | verified | +| B4 | **GHSA-7jqx's version range is false.** Defect 5 (partially-masked minted Twitch key) was introduced by `c7db212` on 2026-08-13, four days *after* v0.6.0; `git tag --contains c7db212` is empty. So `< 0.7.0` overclaims and the advisory sentence *"The latest release, v0.6.0, carries all five defects"* is wrong — v0.6.0 carries four. | `git tag --contains c7db212` (empty) | verified | +| B5 | **GHSA-7jqx mis-describes defect 4.** It says an API response carried *a destination's stream key*. `281821c` shows it is the automod model provider's own third-party key (`?api_key=sk-…`) in `ModelStats.LastError`. No destination key involved. | `git show 281821c` | reported | +| B6 | **GHSA-7jqx claims two fixes the CHANGELOG never mentions** — the database/data-dir permission fix (#299/#300) and the `server.log` stream-key leak (#310/#311). An operator reading the notes is never told the database was world-readable, so is never prompted to assume prior exposure. The existing first Security bullet covers `process.log` (#306), a *different* defect. | CHANGELOG scan: `0644`, `0600`, `umask`, `#299`, `#310` all absent | reported | +| B7 | **Two false claims about a competitor.** `comparison.astro:38` says Restreamer has REST-only metrics — its README advertises Prometheus, and it exposes GraphQL. `comparison.astro:53` claims no per-destination loudness — it has `FilterSelect` per publication with `Loudnorm.js` among the filters. Both rows are mirrored in `docs/COMPARISON.md` and a build check enforces agreement, so each needs changing in two places. | `web/src/pages/comparison.astro:38,53`; upstream sources | reported | +| B8 | **`ENCODING.md:51` — "Twelve hardware encoders … each probed with a real test encode."** Ten are hardware (12 constants include libx264/libx265); only **six** are probed (`probeCandidates` = 5 hardware + x264). HEVC verdicts are inferred from the H.264 sibling. Restated wrongly at `RENDITIONS.md:259`. | `internal/ffmpeg/probe_encoders.go:82`; `internal/ffmpeg/rendition.go:26`; `internal/db/renditions.go:23-39` | verified | +| B9 | **`CONTRIBUTING.md:118` claims PR coverage a docs-only PR does not get.** Says "eleven host acceptance suites"; there are twelve, and since #351/#357 a documentation-only PR runs **zero** of them — the matrix reports green having executed nothing. Documented in none of the four testing docs. | `.github/workflows/ci.yml:1205-1225,1245` | verified | + +--- + +## Should fix before tagging + +| # | Finding | Evidence | Status | +|---|---|---|---| +| S1 | **A failed `binaries` job leaves a half-published release.** `release.yml` has no `needs:` anywhere — `images` and `binaries` run in parallel. If `binaries` fails, Docker Hub and GHCR already carry `:0.7.0`/`:latest` with no GitHub Release. Since `install.sh:729` pins compose to `:latest`, docker operators upgrade immediately while binary installs stay on 0.6.0. **Fix:** `needs: binaries` on `images`. | `grep -n 'needs:' .github/workflows/release.yml` → no output | verified | +| S2 | **Existing 0.6.0 installs never receive the guard.** `update.sh` is generated at install time and written to disk, so a 0.6.0 operator runs the *old, unguarded* script for the 0.6.0 → 0.7.0 upgrade. `git show v0.6.0:scripts/install.sh` has no `secret.key` check at all. Release notes must tell them to re-run `install.sh` first. | `git show v0.6.0:scripts/install.sh` | reported | +| S3 | **`docs/UPGRADING.md` says nothing about key sealing.** `grep -in 'seal\|encrypt\|secret\.key'` returns nothing across 196 lines. Line 23 still teaches the weaker check the generated script outgrew; "Rolling back" never mentions the key file; there is no 0.7.0 entry — despite this being the most consequential change for restore safety in the release. | `docs/UPGRADING.md` | reported | +| S4 | **Enhanced Broadcasting ships without its GPU precondition** on the home page and in the comparison table, where the cell reads a bare "Yes". `negotiate.go:128` refuses outright when `len(a.Hardware.GPU) == 0`. The project's own `COPY-CONSTRAINTS.md:50` requires stating it "in the same block that announces the feature, not in a footnote," because the audience self-hosts on VPSes. `/features` gets it right. | `web/src/pages/index.astro:204`, `comparison.astro:58`; `internal/multitrack/negotiate.go:128` | reported | +| S5 | **All three advisories are `severity: low` with null CVSS.** GHSA-7jqx covers at-rest disclosure of every destination stream key, the admin bcrypt hash and session secrets in a world-readable file — and proves it with `sudo -u nobody` succeeding. Its workaround also under-scopes rotation: "rotate every key that has ever been refused" is right for defect 2, but defect 1 exposed *all* of them. | GHSA API | reported | +| S6 | **GHSA-wv59's only workaround assumes a reverse proxy.** polyemesis terminates TLS itself in a supported mode; those operators have no remedy offered. | GHSA API | reported | +| S7 | **`ENCODING.md:30` overclaims the GPU requirement.** The "Required for" cell reads as universal, but a second audio track needs no GPU on any non-Twitch target. Knock-on: `ENCODING.md:20-22` and `RENDITIONS.md:51` ("audio encoded once, never twice") are false for a destination with a VOD track. | `internal/db/destinations.go:150-170`; `internal/routing/pair.go:18-56` | reported | +| S8 | **Test-doc counts are broadly stale.** 23 acceptance suites exist, `live-test-coverage-gaps.md:11` and `TESTING.md:443` say 17. `TEST-STRATEGY.md:95` says OAuth has "no integration test at all" (46 checks exist); `:94` says four chat adapters never tested against a real server (five, and `acceptance-chat.sh` hits real hosts); `:20` records browser E2E as absent, contradicting `:62` of the same file. `CONTRIBUTING.md:42` states a Go floor of 1.26.5 against `go.mod`'s 1.26.6, which hard-fails. | as cited | verified | +| S9 | **`features.astro:29` illustrates the flagship feature with a screenshot that does not contain it**, and its `alt` text is copy-pasted from the section above. | `web/src/pages/features.astro:29-35` | reported | +| S10 | **`features.astro:95` caption says "four platforms" under a heading saying five.** Rumble is the fifth. | `web/src/pages/features.astro:89,95` | reported | +| S11 | **`index.astro:42` misattributes a limit to OBS** — "up to 32 OBS split audio tracks". 32 is our ceiling (`MaxTracks`); OBS offers 6, which the page's own H1 says. | `web/src/pages/index.astro:42`; `internal/routing/profile.go:27` | reported | +| S12 | **Footer version has no `--match`.** `git describe --tags --abbrev=0` returns v0.6.0 today and could pick up `backup-pre-github` or `rescue-main-*`. Also `pages.yml` checks out at `fetch-depth: 1` (no tags), so the footer is permanently blank, and tagging does not redeploy the site. | `web/src/components/Footer.astro:15`; `.github/workflows/pages.yml:70` | reported | + +--- + +## From the external reviewers, verified against source + +| # | Finding | Severity | Raised by | +|---|---|---|---| +| X1 | **Advisory #1's "cleartext at rest" wording is broader than the fix.** `sources.token`/`prev_token`, the ingest JSON (SRT passphrases, legacy RTMP keys, pull URL credentials) and the whole Settings JSON are still plaintext — deliberately, per `schema.sql:61-67`. But `schema.sql:67`'s comment *"stream_key on destinations is stored the same way"* is now **false**. Either scope the advisory to destination keys or disclose the residual. | SHOULD-FIX | codex | +| X2 | **Plaintext v0.6 keys survive an upgrade in freed pages and WAL frames.** The sealing migration only `UPDATE`s rows; there is no checkpoint or `VACUUM` (zero hits in `internal/db/*.go`). Mitigated by the 0600 fix, but real for anyone restoring a backup taken across the upgrade. | SHOULD-FIX | codex | +| X3 | **Key-in-URL bypasses the sealed column.** A custom RTMP destination with the key embedded in `URL` and `StreamKey` empty is stored plaintext. | SHOULD-FIX | codex | +| X4 | **Tests overclaim.** `destination_secrets_test.go` asserts logical SQL values, never raw db/`-wal` bytes — so it could not have caught X2. `mqtt_broker_disclosure_test.go:112` calls `Validate()` while claiming "real sink, end to end"; it never observes an actual HTTP 400 body. | NICE-TO-HAVE | codex | +| X5 | `internal/multitrack/live_test.go:36` passes **vacuously** when Twitch is unreachable — `unreachable()` returns rather than skipping, and `POLYEMESIS_REQUIRE_NET` is set in no workflow. Loudly logged, so weaker than framed, but it proves nothing in default CI. | NICE-TO-HAVE | agy | +| X6 | `MigratePlatformAccountScopeVer` error path skips `sqldb.Close()` and error wrapping; all seven sibling migrations do both. The comment above it also describes the wrong migration. | NICE-TO-HAVE | agy | + +**Refuted, and worth recording so it is not re-raised:** agy claimed `update.sh` +locks out v0.6.0 installs lacking `secret.key`. Wrong premise — v0.6.0's +`main.go:205` calls `secrets.LoadOrCreate` unconditionally at boot, so every +v0.6.0 install that ever started has the file. + +**Not verified, do not treat as vetted:** agy's ggmlMagic and mocked-transport +items describe defects *this diff already fixed* (historical narrative, not open +issues); its `hlsHandler` auth-wrapping claim was not traced. codex's line +numbers were approximate (the code was confirmed at nearby lines). + +## From the Go sweep + +| # | Finding | Severity | +|---|---|---| +| G1 | **Rolling back to v0.6.0 silently blanks every stream key.** Verified empirically against real DBs seeded from each tag: the forward upgrade is clean and idempotent, but the v0.6.0 binary against an upgraded DB yields `streamKey="" enabled=true` for every destination — the backfill blanks the plaintext column and v0.6.0 has no concept of `stream_key_enc`. There is no `PRAGMA user_version` for a downgrade guard to read. **Needs a release note that the upgrade is one-way once a key file exists.** (The agent also tested whether the blanked plaintext survives as freelist residue — it does not. Hypothesis disproved, recorded so it is not re-raised.) | SHOULD-FIX | +| G2 | **14 media tests bypass the repo's own anti-skip guard.** `testenv.FFmpegBinary` + `POLYEMESIS_REQUIRE_FFMPEG` exist precisely so a missing ffmpeg *fails* rather than skips (#187). Tests added this cycle call `exec.LookPath` + `t.Skip` directly. Verified by stripping ffmpeg from PATH with the guard set: **41 tests fail (guard working), 49 skip while their packages print `ok`**. If the ffmpeg install step breaks, audio-copy bit-exactness, VFR timing and multitrack ordering regressions all go green. `routing/pair_test.go:309` does it correctly — inconsistency, not ignorance. | SHOULD-FIX | +| G3 | **`AutomodModel.TimeoutForBan` is settable and never read** — declared live-reloadable, exposed in the UI, but `ApplyAutomod` maps 8 of 9 fields and skips it. Set `timeoutForBan: 600`; chatters are silenced for 300s while the settings page shows 600. Another #341-class defect; predates v0.6.0. | SHOULD-FIX | +| G4 | `destSecrets(row, extra ...string)` documents `extra` as "the Twitch Enhanced Broadcasting minted key". **All three non-test call sites pass no `extra`.** `TestTheMintedKeyIsMaskedWholeAndNotJustItsTail` passes — pinning redaction of a credential production never mints. Corroborates B0b: the seam was built and never connected. | NICE-TO-HAVE | +| G5 | `NewSecretSet` logs refusals at `log.Debug` while its own doc insists *"Refusals are LOGGED, never silently dropped."* At the default Info level they are silently dropped. | NICE-TO-HAVE | +| G6 | `internal/api/upgrade.go:420` builds the artefact filename from the feed's `tag_name` unsanitised while `:465` carefully escapes the same variable; `tag='../../../../etc'` → `filepath.Dir` = `/`, which `:285` `RemoveAll`s. Requires a compromised `api.github.com` *and* matching `SHA256SUMS` over TLS, so it does not widen blast radius — but it is a one-line `filepath.Base()` fix. | NICE-TO-HAVE | +| G7 | `internal/api/media.go:74` — an oversized upload returns 500 instead of 413. `internal/web/web.go:132` — `stat, _ :=` then `stat.ModTime()` nil-derefs; unreachable with `embed.FS`, but `HandlerFor` is exported and takes an arbitrary `fs.FS`. | NICE-TO-HAVE | + +**Verified clean by the Go sweep, with the work actually run:** full suite passes, +`go build`/`go vet` clean, `go test -race` green across ten packages including +supervisor ×3. No panics from user input, no goroutine or resource leaks in the +diff, migrations forward-only and idempotent with no data-loss operation, no +vacuous `-run` filters or excluded packages. It also **refuted** a raised +finding — an "unguarded state mutation" at `supervisor.go:918` does not hold; +the fields are under `p.cmdMu`/`p.mu` and the race detector is clean. + +> **The sweeps disagree on one point, and it matters.** The Go sweep marked +> advisory (a) **FIXED** after checking the sealing, the backfill and the +> `readSafe*` paths. The external-review sweep found `supervisor.go:703`, and I +> confirmed it myself. Both are right about what they looked at; (a) is fixed at +> rest and in the `process exited` line, and still leaks on the give-up line. +> A "verified fixed" from one angle is not a clearance. + +## More docs findings + +| # | Finding | +|---|---| +| D1 | `docs/CONFIGURATION.md:148` says of environment variables *"There are none."* `RUMBLE_CHAT_API_KEY` and `RUMBLE_CHAT_CHANNEL` exist, and `PLATFORMS.md:181` says the key goes there "and nowhere else". | +| D2 | **`secret.key` described as OAuth-only in five places** — `SECURITY.md:120`, `CONFIGURATION.md:133`, `FAQ.md:140`, `INSTALL.md:298`, `PLATFORMS.md:360`. All now incomplete, and none states the disable consequence. | +| D3 | **Go floor wrong in five docs** — `go.mod` is 1.26.6 (a hard floor since Go 1.21); README, CONTRIBUTING, MODULES and INSTALL all say 1.26.5+. | +| D4 | `docs/COMPARISON.md:96` — *"Every destination decodes and re-encodes its audio, always."* False: `AudioEncoding.Copy` stream-copies, refused only on RTMP and Icecast, so SRT and file destinations can copy. **Understates the product in its own honest-loss section.** | +| D5 | `docs/API.md` documents none of 0.7.0's new surface — `vodProfile`, `keyUnreadable` (a field an operator hits after a bad restore), `maxrateKbps`/`bufsizeKbps`. `TROUBLESHOOTING.md` has no entry for a destination disabled by an unreadable key. | +| D6 | `docs/ARCHITECTURE.md:590` names four `internal/ffmpeg` files that do not exist; omits `internal/multitrack/` and `routing/pair.go`. | +| D7 | `docs/AUDIO-ROUTING.md:60` — `vodProfile` has "its own sample rate". False: `-ar` is unqualified and binds both streams from the primary's rate; `-b:a` is shared too. | +| D8 | `docs/MODULES.md:170` says `Dockerfile.vaapi` is `ubuntu:24.04`; it is `26.04`. `HARDWARE.md:114` has it right. | +| D9 | `PLATFORMS.md:251` omits three Twitch scopes actually requested; `:55` says "the other twenty-five entries" against 33 presets / 19 non-matrix. | +| D10 | `CHANGES-SINCE-v0.6.0.md:3` says 132 commits; actual is 148. Dependency versions stale in both inventory docs. `WEBSITE-COPY-PROPOSAL.md:4` cites `copy-constraints.md` — resolves on macOS, breaks on Linux. | + +## Nice to have + +| # | Finding | +|---|---| +| N1 | `go install …@v0.7.0` compiles but ships a **blank dashboard** — `.gitignore:12` excludes `internal/web/dist/*`. Not advertised anywhere, but the tag makes it fetchable via the module proxy. | +| N2 | Missing CWEs: GHSA-wv59 → CWE-548; GHSA-jqc3 → CWE-532 + CWE-209. | +| N3 | A botched merge severed a doc comment in the security-critical sealing path — `go doc ./internal/db Warning` renders "sealStreamKey splits a stream key into the pair of columns that store it: Warning is one advisory finding…". `internal/db/destinations.go:738,781`. | +| N4 | v0.4.0–v0.6.0 are annotated, unsigned tags. Use `git tag -a v0.7.0 -m "…"` to match. | +| N5 | Undocumented: the `fps=` filter inserted before scaling (+17% measured on 4K60→1080p30); bufsize upper bound of 400 000 kbps; dimension bounds 128–7680 (even only); FPS ≤ 240. | +| N6 | `comparison.astro:75` obs-multi-rtmp "around 4,975 stars" — actual 4,992. `:46` restream.io "2–8 by plan" is true for named tiers but Enterprise is custom. | +| N7 | `AuthScreen.tsx:173` says destinations "will not open", which reads as transient; the code's word is **disabled**, a state the operator must fix. | +| N8 | `index.astro:332` names Castr as an alternative and links to `/comparison`, which never mentions it. | +| N9 | `features.astro:258` says "All eight capabilities" against a table rendering nine rows. | + +--- + +## Verified clean — recorded so nobody re-derives it + +- **SBOM guard**: 9/9, and fails correctly on each individual ecosystem loss. +- **Version embedding**: built with real release ldflags, `-version` prints `polyemesis v0.7.0`; windows/arm64 cross-compiles clean. +- **All four release secrets present**; checksums resolve via `filepath.Base`. +- **0.6.0 → 0.7.0 needs no flag day**: `openStreamKey` prefers ciphertext and falls back to the plaintext column, so pre-upgrade rows work from the first read. +- **Binary-mode update guard** verified against four staged directories: empty → refused; db without `secret.key` → refused; healthy → pass; pre-existing backup dir → refused. +- **No stale `0.6.0`** to bump outside test fixtures and historical docs. +- **VFR**: `-fps_mode`/`-vsync` appear in no non-test code; the regression test asserts frame count and PTS span. +- **Docker Hub description sync** covered twice; tag/prerelease logic consistent across both jobs. +- **~15 other security entries** in the 0.7.0 notes were each checked against the tags: every one fixes a never-released regression, so none needs its own advisory. +- **Marketing site links**: all internal anchors resolve; all 17 linked `docs/*.md` exist; all 7 screenshots present and none orphaned. +- **Competitor claims other than B7**: verified true against primary sources, including exact quoted source lines from obs-multi-rtmp, Aitum and MistServer. diff --git a/internal/supervisor/redact_test.go b/internal/supervisor/redact_test.go index 99b1d7ec..83676742 100644 --- a/internal/supervisor/redact_test.go +++ b/internal/supervisor/redact_test.go @@ -338,6 +338,77 @@ func TestTheProcessExitedLogLineCarriesNoStreamKey(t *testing.T) { } } +// The SECOND line that carries the same error, which the fix above missed. +// +// #311 scrubbed "process exited" and left "giving up on process" four lines +// below it in the same function, reading the same `msg`. The test above could +// not see it: it asserts on a supervisor that retries for ever, so the give-up +// branch never ran. A leak was fixed, a test was written to pin the fix, and +// the sibling path stayed open -- which is the shape this file already warns +// about at the top of the test above ("the leak survived a check that only +// looked at the sink that had already been fixed"). +// +// This one is the worse of the two to leak on. It fires only after MaxRestarts +// consecutive failures, so it marks the moment a destination has been refused +// over and over -- which is precisely when an operator stops watching and +// starts copying server.log into an issue. +// +// Proven able to fail against the committed tree by changing the Error call in +// supervisor.go back to `"err", msg`: the key reappeared and the first +// assertion below failed. +func TestTheGivingUpLogLineCarriesNoStreamKey(t *testing.T) { + refusal := "[out#0/flv @ 0x65431867e200] Error opening output rtmps://" + + fbHost + ":443/rtmp/" + mainStreamKey + ": Connection refused" + + var buf syncBuffer + f := fakeExitSaying(251, refusal) + spec := Spec{ + Name: "dest:5", + Kind: "destination", + Secrets: []string{mainStreamKey}, + Bin: f.bin, + Args: f.args, + // The three that make the give-up branch reachable at all. Without + // AutoRestart the process never retries and never gives up; without a + // low MaxRestarts and a short backoff the test outlives its deadline + // before the branch is entered. + AutoRestart: true, + MaxRestarts: 2, + MinBackoff: 10 * time.Millisecond, + MaxBackoff: 20 * time.Millisecond, + } + p := New(slog.New(slog.NewTextHandler(&buf, nil)), spec) + t.Cleanup(func() { + ctx, cancel := context.WithTimeout(context.Background(), 3*time.Second) + defer cancel() + _ = p.Stop(ctx) + }) + + p.Start() + waitFor(t, "the supervisor to give up", func() bool { + return strings.Contains(buf.String(), "giving up on process") + }) + + got := buf.String() + if strings.Contains(got, mainStreamKey) { + t.Errorf("the \"giving up on process\" log line carries the stream key.\n" + + "This fires after a destination has been refused repeatedly, which\n" + + "is exactly when server.log gets copied into a bug report.") + } + if !strings.Contains(got, alerts.Mask) { + t.Errorf("the key was neither present nor masked -- the error text has\n"+ + "gone missing rather than been scrubbed.\ngot: %s", got) + } + // The diagnostic has to survive the scrub, or the fix has traded a leak + // for an unreadable failure. + for _, want := range []string{"Error opening output", fbHost, "Connection refused"} { + if !strings.Contains(got, want) { + t.Errorf("the give-up line has lost %q, which is what makes it worth\n"+ + "logging at all.\ngot: %s", want, got) + } + } +} + // syncBuffer is a bytes.Buffer the run goroutine writes and the test reads. type syncBuffer struct { mu sync.Mutex diff --git a/internal/supervisor/supervisor.go b/internal/supervisor/supervisor.go index fe114993..b45429bc 100644 --- a/internal/supervisor/supervisor.go +++ b/internal/supervisor/supervisor.go @@ -700,7 +700,19 @@ func (p *Process) supervise(ctx context.Context, done chan struct{}) { if msg != "" { give += ": " + msg } - p.log.Error("giving up on process", "restarts", consecutive-1, "err", msg) + // SCRUBBED, for the reason the "process exited" line above is, and + // this line is the one that was missed when that one was fixed. + // #311 scrubbed the retry line and left this one carrying the same + // `msg` -- runOnce's error, which holds 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 looking at when they copy server.log into an + // issue. The retry line got the attention because it fires often; + // this one fires when someone is about to ask for help. + p.log.Error("giving up on process", "restarts", consecutive-1, "err", p.scrub(msg)) p.appendLog(give, "error") p.setState(StateFailed, give) return diff --git a/scripts/install.sh b/scripts/install.sh index ce5141b0..e98af013 100755 --- a/scripts/install.sh +++ b/scripts/install.sh @@ -1066,24 +1066,40 @@ docker volume inspect polyemesis-data >/dev/null 2>&1 || { docker run --rm -v polyemesis-data:/data -v "$INSTALL_DIR:/backup" alpine \\ tar czf "/backup/backup-\${stamp}.tar.gz" -C /data . +# LIST ONCE, THEN TEST THE LISTING. Never pipe tar into a reader that can exit +# early. \`tar tzf … | grep -q\` looks obviously correct and is not: grep -q +# exits on its FIRST match, tar then takes SIGPIPE, and under the \`set -o +# pipefail\` above the pipeline returns 141 -- which \`if !\` inverts into "the +# file is missing". A backup that DOES contain secret.key was refused, and +# whether it happened depended on where the file landed in readdir order, i.e. +# inode order. Reproduced at entry 1 of 4002 (false refusal) and entry 4001 +# (pass), same archive contents. Invisible on macOS, where bsdtar exits 0 on +# SIGPIPE, which is why this survived local testing. +# +# Herestrings rather than \`printf | grep\`, because printf takes SIGPIPE too -- +# reordering the pipeline would move the bug, not remove it. This also walks the +# archive once instead of twice. +listing="\$(tar tzf "$INSTALL_DIR/backup-\${stamp}.tar.gz")" + # A backup that exists but holds nothing is worse than no backup, because it # reads as success. tar always writes the './' entry, so anything under two # entries means the volume was empty. -entries="\$(tar tzf "$INSTALL_DIR/backup-\${stamp}.tar.gz" | wc -l)" +entries="\$(wc -l <<<"\$listing")" if [ "\$entries" -lt 2 ]; then echo "ERROR: backup archive is empty (\${entries} entries). Refusing to upgrade." >&2 exit 1 fi - # AND THE ONE FILE THE COUNT CANNOT VOUCH FOR. A non-empty archive proves the - # volume held something, not that it held the file that makes the database - # 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 ! tar tzf "$INSTALL_DIR/backup-\${stamp}.tar.gz" | grep -q "secret\.key"; then - echo "ERROR: the backup contains no secret.key. Restoring without it leaves" >&2 - echo "every destination disabled. Refusing to upgrade." >&2 - exit 1 - fi + +# AND THE ONE FILE THE COUNT CANNOT VOUCH FOR. A non-empty archive proves the +# volume held something, not that it held the file that makes the database +# 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 + echo "ERROR: the backup contains no secret.key. Restoring without it leaves" >&2 + echo "every destination disabled. Refusing to upgrade." >&2 + exit 1 +fi echo "backup verified: \${entries} entries" $COMPOSE_CMD pull $COMPOSE_CMD up -d