Repository navigation
fix(release): security review fixes for the release pipeline and CI - #28
Merged
Merged
Conversation
Security review C-02. A git signature covers the tag object, and the object names the tag it was signed as, but verify-tag.sh only looked at the ref's name. So the release manager's signed v1.0.0-rc.1 object, pushed again as refs/tags/v1.0.0, verified as a final release (prerelease=false). verify-tag.sh now compares the "tag" line inside the signed object with the tag's name and stops otherwise. tag-release.sh checks the same before it pushes. The release workflow passes the commit verify-tag checked to the build job, which fails if the commit it checked out is a different one. docs/verify.md adds the same name check to the manual steps. The new test signs v9.9.9-rc.1 with a throwaway key and points refs/tags/v9.9.9 at it: verify-tag.sh exited 0 before this change and exits 1 now. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Security review C-05. check-security-gate.mjs passed when any line of a review-v*.md started with "Gate: closed", wherever it was. That included the reviewers' reports appended below the lead's text, and notes such as "**Gate:** closed? No". The gate is now closed only by one line of exactly this form, above the report's "## Appendix" heading: Gate: closed YYYY-MM-DD by <lead>, fixes confirmed on vX.Y.Z-rc.N The date must be a real one, and the candidate must be one of the reviewed version's. Above the appendix there must be no other line starting "Gate:", so a "Gate: open" line keeps the gate open. Anything below the appendix counts for nothing. The release workflow now passes the tag's version, and a review opened for that version has to be the closed one; a version with no review of its own is still gated by the earlier ones. Slice 54's spec and docs/stores.md describe the format. The new tests use reviewer C's three texts, which all opened the gate before, and they now leave it closed. A valid report opens it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Security review C-06. The Firefox job unpacked the release zip and ran `web-ext sign --source-dir`, which always builds a new zip with new timestamps. So the package AMO reviewed couldn't be matched to SHA256SUMS or the release's attestation, while docs/verify.md and docs/reproducible-builds.md said the stores get the exact packages. scripts/amo-submit.mjs checks the zip against its SHA256SUMS line, then hands that exact file to web-ext's own AMO client (web-ext/util/submit-addon, exported by web-ext since mozilla/web-ext#3250; the lockfile pins 10.7.0). That client uses the AMO v5 API: upload, validation, then a new listed version. It reads the file's bytes and sends them unchanged. It uses the same JWT credentials as before and doesn't wait for review. The `web-ext sign` CLI can't do this; its upstream issue (mozilla/web-ext#3242) is still open. The docs now say the stores get the release zips unchanged and then sign them: AMO adds its META-INF signature files, and the Chrome Web Store repackages the zip as a signed CRX. The new test runs a fake AMO on 127.0.0.1. The bytes it receives hash to the release zip's SHA-256. A zip that doesn't match SHA256SUMS never reaches it. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Security review C-08 (the release half). The release build ran the property tests with a new random seed on every run, so a tag's release could fail by chance, and a rerun tested different inputs. v1.0.0-rc.1's release run failed this way. The build job now sets FC_SEED for fast-check and passes `mix test --seed` (StreamData takes ExUnit's seed), so a release, and a rerun of it, runs the same inputs. CI keeps a new seed on every pull request and push to main, so it keeps exploring. The timing-sensitive tests themselves are fixed separately. docs/stores.md tells the release manager how to rerun the failed jobs of a tag's run (`gh run rerun <run-id> --failed`) without making a new tag. Checked: `FC_SEED=20261006 npm run coverage` passes (1989 tests, 1 skipped, coverage gates met), and `mix test --seed 20261006` gives 23 properties, 1106 tests, 0 failures. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Security review C-01 (the CI half). The required `secrets` and `osv-scanner` checks read their allowlists (.gitleaks.toml, .gitleaksignore, osv-scanner.toml) and their check scripts from the pull request's own tree. So a pull request could add a token and allow it in .gitleaks.toml in the same change, and the scan passed. On a pull request these files now come from the base commit: - scripts/base-file.sh copies them out of the base commit, and CI runs the base's own copy of that helper. - The checks run with those copies: gitleaks' config and ignore file, the forbidden-files and DCO scripts, and osv-scanner.toml. - The tree's copies of .gitleaks.toml and .gitleaksignore are removed before the scan, because gitleaks always reads the .gitleaksignore of the folder it scans, whatever --gitleaks-ignore-path says. - `--ignore-gitleaks-allow` stops an inline "gitleaks:allow" comment from hiding a token. The history has none; a full scan with the flag finds no leaks. A pull request's edits to these files take effect once it is merged. On main, in the weekly scan and in a release, the tree's copies are used. A pull request can still change the workflow files themselves. Only review, or an organization-level required workflow, covers that. The regression test builds a throwaway repository. A pull request there adds a token and edits .gitleaks.toml, .gitleaksignore and both check scripts to let it through. With gitleaks 8.30.1 (CI's pinned version), the old command finds no leak (exit 0); the new one fails (exit 1, "leaks found: 1"). The secrets job runs this test with its own gitleaks. Running the job's steps, taken out of ci.yml, against the same pull request: before this change every step passed, and now the scan step fails. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Security review C-01 (docs half) and C-03. The assurance case and docs/stores.md claimed review by a maintainer on every pull request, and spoke of a `release` environment that didn't exist yet. They also told maintainers to delete a wrong release tag, which the "tags are permanent" ruleset refuses. They now say what is enforced: - the `main` ruleset requires the CI checks, and on a pull request the scans take their settings from the base commit; - the `release` environment, with the maintainer as required reviewer and `v*` tags only, gates every store upload; - only the release manager (the repository admin) can create a release tag or delete a wrong one; other maintainers can't; - review by a second maintainer is the aim but isn't enforced, an accepted risk while one maintainer does nearly all the work (CODE_REVIEW.md), now listed among the assurance case's residual risks. A pull request can also still change a workflow itself. The settings are the lead's changes on GitHub; these docs describe them. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
Security review B-08. SECURITY.md said "Kotiko is before 1.0" and docs/security/requirements.md said "There are no releases yet". Both texts ship in 1.0.0. SECURITY.md now says only the latest release gets security fixes: from 1.0.0 on, the latest 1.x. Release candidates and older versions get none. The requirements say the pipeline is in place, release candidates are tagged for slice 54's review, and v1.0.0, the first store release, is pending. check-versions.mjs (CI, and tag-release.sh) now fails once the version is 1.0.0 or later while SECURITY.md still calls Kotiko pre-1.0. The new test fails without this check. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
…irefox
Security review C-11. The Chrome form ticks "Authentication information:
Yes", for the learner's own AI-service key and their server's token. The
Firefox manifest declared no data collection
(`data_collection_permissions.required: ["none"]`).
Mozilla's add-on policy (§6) defines data transmission as "any data
collected, used, transferred, shared, or handled outside the add-on or the
local browser". It makes no exception for a service the user chose.
`authenticationInfo` covers passwords, PINs and similar credentials
(extensionworkshop.com, built-in data consent). The key and token leave
the browser in an Authorization header, so "none" misstated them.
The manifest now declares `required: ["authenticationInfo"]`. Optional
would fit better, since Kotiko works without a key, but an optional
permission only counts once the extension asks with
`permissions.request({ data_collection: [...] })` in a click handler.
Kotiko doesn't ask yet, so declaring it optional would claim a consent it
never collects. store/firefox-amo.md explains this and the follow-up.
Nothing goes to ScriptKittyOS.
A new store-readiness test checks that the Chrome form and the Firefox
declaration agree on every category that leaves the browser. It fails on
the old manifest. web-ext lint still reports no errors.
Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
… review's changes allowed_signers claimed a key change is a reviewed pull request; second- maintainer review isn't enforced yet, which the assurance case records. The blank line that made ssh-keygen print "missing key" is gone. Slices 30 and 28 get dated notes for the AMO upload (C-06), the tag-name and gate checks (C-02, C-05) and the Firefox data declaration (C-11). Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com> Signed-off-by: Ayla Croft <dev@scriptkittyos.com>
This was referenced Oct 7, 2026
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.
Fixes from the pre-release security review (slice 54, SCR-476) in the release pipeline, CI and store docs. Each fix has a regression test that failed before it.
verify-tag.shandtag-release.shcheck the name inside the signed tag object, so a signedv1.0.0-rc.1re-pointed asv1.0.0is refused. The release build checks out exactly the verified commit.Gate: closed YYYY-MM-DD by <lead>, fixes confirmed on vX.Y.Z-rc.N, placed above## Appendix. Reviewer C's three false positives now leave it closed.SHA256SUMS, through web-ext's AMO client (scripts/amo-submit.mjs) instead of a repack.docs/stores.mdexplains rerunning a failed release withgh run rerun --failed.gitleaks:allowcomments are no longer honoured. The docs now say what is enforced, and second-maintainer review goes in as a residual risk.SECURITY.mdandrequirements.mdreflect 1.0.0, andcheck-versions.mjscatches "before 1.0" text from 1.0.0 onwards.authenticationInfo(the learner's own key, sent to the service they chose), matching the Chrome form. A test checks that the two stores' declarations agree..github/allowed_signersno longer overclaims review and loses the blank line that madessh-keygenprint "missing key". Slices 28 and 30 get dated notes.GitHub settings applied separately: the
release,store-statusandevalenvironments, required SHA pinning, and the v* tag-creation ruleset.🤖 Generated with Claude Code