Skip to content

ci: safer releases and download worker - #151

Merged
builtbyproxy merged 13 commits into
mainfrom
audit/l09-ci-worker
Oct 8, 2026
Merged

builtbyproxy merged 13 commits into
mainfrom
audit/l09-ci-worker

Conversation

@builtbyproxy

Copy link
Copy Markdown
Owner

Part of the audit fix stack, but independent of the code layers (#144-#149), so it can merge any time. Fixes audit findings CI-1, CI-3, CI-4, CI-5, CI-7, CI-8, CI-9, CI-10, CI-14, SEC-11, SEC-12 and DOC-9.

Non-shipping: only .github/, worker/, tests and docs change, so no version bump and no release.

After merging, the worker changes only go live once someone runs npx wrangler deploy from worker/.

What's broken

  • A release can drop out of the plugin catalog for good. Two merges a couple of minutes apart, or any failure after the tag is created, leave a GitHub release that manifest.json never lists. Rerunning cannot fix it, because the workflow stops as soon as it sees the tag.
  • The release job holds a write token in the same job that restores packages and runs the test suite, and every action is referenced by a movable tag.
  • The version gate only checks that the version changed. Two PRs bumping to the same number both pass, and the second ships silently under the first one's release.
  • The download worker's /dl/ route is an open redirect. Encoded slashes and dot segments send a link on the project's download domain to any GitHub path.
  • Anyone holding the public ingest key can fill the shared telemetry database with 256 KB log bundles.
  • Workers Logs may keep caller IPs for every manifest poll, and the README overstates how unlinkable the install hash is.
  • Smaller items:
    • four tests that cannot fail;
    • a test class racing others on static state;
    • Dependabot ignoring most dependencies;
    • template comments leaking into release notes;
    • a stale build.yaml.

Why it happens

  1. release.yml treats "tag exists" as "release is done", runs without a concurrency group, and pushes the manifest without rebasing.
  2. Build, test and publish all live in one job with contents: write.
  3. The worker decodes the path first and then checks it with a regex that allows ... It caps bundle size but not daily volume, and observability was left at its default.

What this PR does

  • Release workflow:
    • Runs one release at a time and decides by the manifest entry. If the manifest already lists the version, it does nothing. If the tag exists but the manifest entry does not, it repairs the release: it reuses the published asset and its checksum, rebuilding from the tag only when the asset is missing. If neither exists, it releases as before.
    • Tags the commit that was built, and retries the manifest push on a fresh main.
    • Takes the notes from the PR that bumped the version, cleans them to plain text and caps them at 4000 characters.
    • Is split into a read-only build job and a publish job with write access that runs no repository build code.
  • All workflows:
    • Every action is pinned to a commit SHA.
    • Dependabot now watches actions, the site's npm packages and the other NuGet packages, grouped and weekly. The Jellyfin SDK rule and its floor-policy note are unchanged.
  • Version gate: requires a version higher than main's that is not already tagged.
  • Worker (deploy by hand with npx wrangler deploy after merge):
    • /dl/ only accepts <tag>/jellyfin-plugin-letterboxd-<tag>.zip on the raw path.
    • Log bundles are capped at 200 a day and 20 MB a day.
    • Workers Logs is turned off.
  • Tests and docs:
    • Four tests get real assertions, and one test class joins the shared collection.
    • The README's install-count paragraph now says "pseudonymous", which matches what the fixed salt allows.
    • The stale build.yaml is removed.
  • Not touched: no package bumps (Dependabot will propose them). The 1.18/1.19 entries in release-notes.ts still say "cannot be linked across weeks" and need a separate wording fix.

How it was tested

  • dotnet test -c Release --filter "FullyQualifiedName!~Integration": 1062 passed.
    • The four strengthened tests fail when the code they cover is stubbed out.
  • node --experimental-strip-types --test worker/test/dl.test.mjs: 6 passed.
    • The test proves the old redirect payload is rejected and that every download URL in manifest.json still works.
    • Restoring the old path logic makes the rejection tests fail.
    • CI now runs this in a worker-tests job.
  • The release body and manifest push steps were run locally against a scratch repo with a stubbed gh:
    • picks the PR that bumped the version;
    • strips comments and HTML;
    • truncates long notes;
    • retries after main moves;
    • a rerun is a no-op.
  • The version gate script was run against lower, higher, numerically larger, already-tagged and non-shipping cases.
  • actionlint and a YAML parse are clean on every workflow.

@builtbyproxy
builtbyproxy merged commit d193709 into main Oct 8, 2026
6 checks passed
@builtbyproxy
builtbyproxy deleted the audit/l09-ci-worker branch October 8, 2026 03:30
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.

1 participant