Repository navigation
ci: centralize version pins and lane the PR suite - #1
Conversation
|
Warning Review limit reached
Next review available in: 26 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Plus Run ID: 📒 Files selected for processing (34)
WalkthroughChangesCentralized CI Version Pinning
Sequence Diagram(s)sequenceDiagram
participant CI orchestrator
participant reusable workflow
participant load-versions.sh
participant versions.env
participant ci-build.sh
participant fetch-verify.sh
CI orchestrator->>reusable workflow: Invoke CI lane
reusable workflow->>load-versions.sh: Load version pins
load-versions.sh->>versions.env: Validate manifest
load-versions.sh-->>reusable workflow: Export pins to GITHUB_ENV
reusable workflow->>ci-build.sh: Build selected flavor
ci-build.sh->>fetch-verify.sh: Download or recheck archive
fetch-verify.sh-->>ci-build.sh: Return verified archive
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches 💡 1📝 Generate docstrings 💡
🧪 Generate unit tests (beta)
✨ Simplify code
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 4482a373-5809-4eeb-83a0-64fb8bbb1b17
📒 Files selected for processing (13)
.github/scripts/compute-versions.sh.github/scripts/fetch-verify.sh.github/scripts/load-versions.sh.github/versions.env.github/workflows/asan.yml.github/workflows/build-test.yml.github/workflows/ci-deep.yml.github/workflows/codeql.yml.github/workflows/fuzzing.yml.github/workflows/security-scanners.yml.github/workflows/valgrind.ymlci/tools/bump-versions.shci/tools/ci-build.sh
eilandert
changed the title
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
ci/tools/bump-versions.sh (1)
51-54: 🎯 Functional Correctness | 🟡 Minor | ⚡ Quick winShow the generated manifest when bootstrapping.
When
.github/versions.envis missing or untracked,git diffanddiff -udo not show the newly generated file. The script reports a change, but operators cannot review the generated pins and digests. Use/dev/nullas the comparison baseline for missing or untracked files in both branches.Also applies to: 64-68
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 16e30c12-cd7b-4064-92a7-f0a0862e8efc
📒 Files selected for processing (13)
.github/scripts/compute-versions.sh.github/scripts/fetch-verify.sh.github/workflows/asan.yml.github/workflows/build-test.yml.github/workflows/ci-deep.yml.github/workflows/ci.yml.github/workflows/codeql.yml.github/workflows/fuzzing.yml.github/workflows/security-scanners.yml.github/workflows/valgrind.yml.pre-commit-config.yamlci/tools/bump-versions.shci/tools/ci-build.sh
| - id: actionlint | ||
| name: actionlint (workflow syntax) | ||
| entry: env SHELLCHECK_OPTS=-Swarning actionlint -ignore 'label ".+" is unknown' | ||
| language: system | ||
| files: ^\.github/workflows/.*\.ya?ml$ | ||
| pass_filenames: true |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
actionlint -config-file .github/actionlint.yaml .github/workflows/*.ymlRepository: myguard-labs/nginx-skeleton-module
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -eu
echo "== referenced config lines =="
sed -n '115,130p' .pre-commit-config.yaml
echo
echo "== actionlint config =="
if [ -f .github/actionlint.yaml ]; then
nl -ba .github/actionlint.yaml | sed -n '1,220p'
else
echo ".github/actionlint.yaml not found"
fi
echo
echo "== pre-commit actionlint occurrences =="
rg -n "actionlint|actionlint\.yaml|SHELLCHECK_OPTS|label \"\\.\\+\" is unknown" .pre-commit-config.yaml .github 2>/dev/null || true
echo
echo "== workflow runners / known labels snippets =="
for f in .github/workflows/*.yml .github/workflows/*.yaml; do
[ -e "$f" ] || continue
echo "--- $f"
rg -n "runs-on:|SECTIONS_LABEL|SECTIONS_LABEL_?|matrix:" "$f" || true
doneRepository: myguard-labs/nginx-skeleton-module
Length of output: 5403
🏁 Script executed:
#!/bin/bash
set -eu
actionlint -config-file .github/actionlint.yaml .github/workflows/*.ymlRepository: myguard-labs/nginx-skeleton-module
Length of output: 172
🏁 Script executed:
#!/bin/bash
set -eu
echo "== .github/actionlint.yaml =="
cat -n .github/actionlint.yaml
echo
echo "== local actionlint invocation in workflows =="
sed -n '70,90p' .github/workflows/build-test.yml
echo
echo "== actionlint help/config-file and ignore behavior =="
actionlint --help | grep -E "config-file|ignore|label" || true
echo
echo "== behavioral probe with temp workflow =="
tmpdir="$(mktemp -d)"
cat >/tmp/workflow-${RANDOM}.yml <<'YML'
on: push
jobs: foo:
runs-on: typoooo
steps:
- run: echo ok
YML
( actionlint -ignore 'label ".+" is unknown' /tmp/workflow-${RANDOM}.yml; echo "IGNORE_EXIT=$?" )
tmpf="/tmp/workflow-${RANDOM}.yml"
echo "on: push" > "$tmpf"
echo "jobs: foo:" >> "$tmpf"
echo ' runs-on: typoooo' >> "$tmpf"
echo " steps:" >> "$tmpf"
echo " - run: echo ok" >> "$tmpf"
( actionlint -config-file .github/actionlint.yaml "$tmpf"; echo "CONFIGFILE_EXIT=$?" )Repository: myguard-labs/nginx-skeleton-module
Length of output: 4768
Use actionlint.config-file for the hook instead of suppressing unknown labels.
This hook currently hides every unknown runner label, so a typo or unsupported runs-on value can pass locally while CI, which uses .github/actionlint.yaml, reports it. Since CI and the repo's workflow lint both use the config explicitly, align local-ci to the same behavior so local-green stays reliable.
369b700 to
1b6d094
Compare
Three changes that only make sense together, all in CI.
Version pins were spread over 7 workflow env: blocks, 3 hardcoded ci-deep
matrix cells, and sha256 digests in bash associative arrays in ci-build.sh.
Two lists with different writers meant a bump could carry a stale digest, and
an unpinned version still built with only a WARNING. They now live in
.github/versions.env with version and digest on adjacent lines and a single
writer, loaded into $GITHUB_ENV as the first step after checkout in all 13
nginx-building jobs. Verification is mandatory: an unpinned version is a hard
failure, and an already-present tarball is re-verified, so a poisoned build
cache is caught rather than reused.
Six workflows each carried their own push/pull_request trigger, so a PR asked
for all of them at once. Wall-clock on builder02 is queueing for a runner
slot, not compute. They are workflow_call members of one ci.yml now, in three
lanes under the 268s budget set by the longest single job:
A Fuzzing (268)
B A/UBSan (94) -> Valgrind (83) -> Security scanners (77) = 254
C Build&Test (~50)
Nothing is chained behind Fuzzing: work placed there is pure critical path,
work appended to a shorter lane is free. Lint, CodeQL and the changed-files
gate run hosted, take no self-hosted slot, and so are not laned at all. The
durations are measured, not estimated, and ci.yml's header carries the run ID
they came from plus the command to re-derive them.
A tracked pre-commit hook (core.hooksPath .githooks) runs ci/linter/, whose
thresholds mirror security-scanners.yml so local green predicts remote green.
A missing linter exits 2 and blocks the commit; it is never a silent skip.
Defects found by verifying rather than assuming:
- build-test.yml shared a concurrency group with its own orchestrator and
cancelled it. Lane C produced zero jobs and !cancelled() read that as a
partial pass.
- The asan changed-files gate failed open. A shallow checkout gave "no merge
base", the error was swallowed left of grep -q, the step exited 0, and the
sanitizer was skipped while the suite stayed green.
- fetch-verify.sh took the first line of a stream as the digest, so a spurious
progress line landed as NGINX_MAINLINE_SHA256. It is now shape-validated
against ^[0-9a-f]{64}$ rather than trusted by position.
- The Angie ci-deep cell had never built. -Wshadow in ci-build.sh's
--with-cc-opt is baked into CFLAGS for every object nginx/Angie compiles,
including Angie's own core, where Angie's -Werror made its shadow warnings
fatal. Neither flag alone does it. The same job hardcoded objs/nginx, but
Angie names its binary objs/angie, so TEST_NGINX_BINARY pointed at a file
that did not exist and only the .so was asserted. Angie now runs 55/55.
- semgrep defaults to one OCaml domain per core, each opening an io_uring ring
against an 8 MB RLIMIT_MEMLOCK shared with the six runner slots, and aborts
when they are busy: 3/3 crashed on a busy box, 0/3 on an idle one. Pinned to
--jobs=1 in both the hook and CI, so a neighbouring job cannot redden a clean
diff.
1b6d094 to
0a3ddcf
Compare
There was a problem hiding this comment.
Actionable comments posted: 21
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro Plus
Run ID: 7d59fe5d-9b45-47f3-b90d-3fd50704b701
📒 Files selected for processing (33)
.githooks/pre-commit.github/scripts/compute-versions.sh.github/scripts/fetch-verify.sh.github/scripts/load-versions.sh.github/versions.env.github/workflows/asan.yml.github/workflows/build-test.yml.github/workflows/ci-deep.yml.github/workflows/ci.yml.github/workflows/codeql.yml.github/workflows/fuzzing.yml.github/workflows/lint.yml.github/workflows/security-scanners.yml.github/workflows/valgrind.yml.perlcriticrc.pre-commit-config.yaml.yamllintREADME.mdci/PROMPT-standardize-module.mdci/linter/README.mdci/linter/install-linters.shci/linter/lib.shci/linter/lint-c.shci/linter/lint-nginx.shci/linter/lint-perl.shci/linter/lint-python.shci/linter/lint-sh.shci/linter/lint-yaml.shci/linter/run-all.shci/tools/bump-versions.shci/tools/ci-build.shsrc/ngx_http_skel_module.csrc/ngx_http_skel_scan.c
| # Enable once per clone: | ||
| # git config core.hooksPath .githooks | ||
| # Emergency bypass: | ||
| # git commit --no-verify | ||
| # | ||
| # It lints only the STAGED files, so a commit is never blocked by a finding in | ||
| # a file it does not touch. Run the whole tree yourself with: | ||
| # ci/linter/run-all.sh | ||
| # | ||
| # Exit 2 (a linter is not installed) blocks the commit on purpose: a gate that | ||
| # skips itself when its tool is missing reports green while checking nothing. | ||
| set -uo pipefail | ||
|
|
||
| ROOT="$(git rev-parse --show-toplevel)" | ||
| "$ROOT/ci/linter/run-all.sh" --staged |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -u
echo "== git files of interest =="
git ls-files -z | tr '\0' '\n' | grep -E '(^|/)(\.githooks/pre-commit|ci/linter/run-all\.sh|README|\.gitignore)$' || true
echo "== .githooks/pre-commit =="
if [ -f .githooks/pre-commit ]; then
cat -n .githooks/pre-commit
fi
echo "== ci/linter/run-all.sh outline/contents =="
if [ -f ci/linter/run-all.sh ]; then
wc -l ci/linter/run-all.sh
cat -n ci/linter/run-all.sh
fi
echo "== references to hooksPath / git hooks / linter =="
rg -n "hooksPath|\.githooks|run-all\.sh|core\.hooks" . --glob '!node_modules/**' --glob '!dist/**' --glob '!build/**' || true
echo "== executable and git tracked status =="
for f in .githooks/pre-commit ci/linter/run-all.sh; do
if [ -e "$f" ]; then
stat --format='%n mode=%a tracked=%s' "$f" "$(git ls-files --error-unmatch "$f" >/dev/null 2>&1 && echo yes || echo no)" | sed "s/tracked=yes/tracked=true/; s/tracked=no/tracked=false/"
fi
doneRepository: myguard-labs/nginx-skeleton-module
Length of output: 11197
🌐 Web query:
Git core.hooksPath working tree directory path relative to current branch checkout security
💡 Result:
The git configuration variable core.hooksPath allows users to specify a custom directory for Git hooks instead of the default.git/hooks directory [1][2]. When a relative path is provided, it is interpreted as relative to the root of the working tree in a non-bare repository, or relative to the $GIT_DIR in a bare repository [1][2]. Security Implications The ability to configure core.hooksPath, particularly when stored in repository-local configuration (.git/config), presents a significant security risk [3][4]. If an attacker can influence the repository configuration—for example, through an application that writes configuration entries without sanitization or via direct repo poisoning—they can redirect Git to execute malicious scripts placed within the working tree [3][5][6]. Because these hooks are executed in the context of the user running the Git command, this is a common vector for Remote Code Execution (RCE) [3][5][6]. Key Security Considerations: 1. Attack Vector: If an application allows users to manipulate Git configurations (e.g., setting author names, emails, or arbitrary config keys), an attacker may inject a newline character followed by a core.hooksPath entry into the configuration file to hijack hook execution [3][5]. 2. Defense-in-Depth: Git maintains security features like safe.directory to restrict operations on repositories owned by other users [7]. While there have been attempts to add protections against malicious core.hooksPath settings specifically during clone operations, these were complex to implement without breaking valid workflows and have faced revisions [8][9][10][4]. 3. Trusted Environment: Because core.hooksPath can redirect execution to arbitrary scripts, it should only be set in "protected" configuration scopes (system-wide or user-wide configuration) rather than in repository-local configuration, whenever possible, to prevent untrusted repositories from tampering with execution behavior [7][10]. Users should treat repository-local configurations as untrusted, especially in environments where multiple users or external processes interact with the same Git repositories [3][5]. Application developers should sanitize all inputs that are written to Git configuration files to prevent command or configuration injection [3][5][6].
Citations:
- 1: https://github.com/git/git/blob/6c18d03eb833bb76d0072373bd064e469a022d4f/Documentation/githooks.txt
- 2: https://git-scm.com/docs/githooks
- 3: GHSA-v87r-6q3f-2j67
- 4: git/git@20f3588
- 5: GHSA-v87r-6q3f-2j67
- 6: https://securelayer7.net/lab/cve-2025-65964-n8n-git-node-hooks-rce
- 7: https://public-inbox.org/git/69ee845be89/s/?b=Documentation%2Fconfig%2Fsafe.txt
- 8: http://public-inbox.org/git/20240521195659.870714-9-gitster@pobox.com/
- 9: http://public-inbox.org/git/7d5ef6db2a9c3c7a1b0ba78873d4202403768769.1715987756.git.gitgitgadget@gmail.com/
- 10: https://public-inbox.org/git/86d57213-e3b2-c985-6d69-71568c66fc9c@gmx.de/
Other (CWE-829): Inclusion of Functionality from Untrusted Control Sphere
Reachability: External
Exploitability: Moderate
Don’t point core.hooksPath at the mutable worktree
git config core.hooksPath .githooks makes Git execute hook scripts from the currently checked-out branch. If .githooks/pre-commit or ci/linter/run-all.sh are changed without explicit developer approval, Git will run that modified shell code with the developer’s privileges on commit/amend. Install a fixed trusted hook in .git/hooks or a user-controlled absolute hooks directory, or validate that both the hook and lint helper match an approved revision/cached installer before execution.
| - **Fork routing.** Every self-hosted job uses the reference's expression: | ||
| `runs-on: ${{ github.event.pull_request.head.repo.fork && 'ubuntu-latest' || fromJSON('["self-hosted","builder02","lxc"]') }}` | ||
| A fork PR never reaches the build host. | ||
| - **No `pull_request_target`**, ever, in a repo with self-hosted runners. It | ||
| runs with a writable token in the base-repo context; combined with a fork's | ||
| code it is a full compromise. If something seems to need it, it does not. | ||
| - **Least-privilege tokens.** `permissions: contents: read` at workflow level; | ||
| widen per-job only where genuinely needed (`security-events: write` for | ||
| CodeQL). Never `write-all`. | ||
| - `persist-credentials: false` on every checkout, so a later step cannot reuse | ||
| the token. | ||
| - **Pin every third-party action to a full commit SHA**, with the version in a | ||
| trailing comment. A tag is mutable. | ||
| - Pin every downloaded tool version (semgrep, actionlint, nginx tarballs) and | ||
| verify tarballs by sha256. This is code executing on a persistent host. | ||
| - Never expose secrets to a job that can run untrusted code. Prefer no secrets | ||
| at all in the PR lane; `bump.yml`-style writers run only from the default | ||
| branch on a schedule. | ||
| - Repo settings (check with `gh api`, fix or report): require approval for | ||
| first-time-contributor workflow runs, restrict which actions may run, | ||
| branch protection with required checks, and no self-hosted runner registered | ||
| at org level where a public repo can grab it. |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== file excerpt =="
sed -n '380,455p' ci/PROMPT-standardize-module.md || true
echo
echo "== references for fork routing/reusable jobs in repo =="
rg -n "self-hosted|builder02|pull_request\.head\.repo\.fork|pull_request_target|approve|approval|fork routing|fork PR|reusable workflow|uses:" ci .github 2>/dev/null | head -200
echo
echo "== relevant tracked/workflow/action files =="
git ls-files | grep -E '(^ci/|\.github/workflows/|\.github/*)' | head -200Repository: myguard-labs/nginx-skeleton-module
Length of output: 19341
🌐 Web query:
GitHub Actions fork pull requests workflow code can be modified approve required contributor external collaborators self-hosted runners
💡 Result:
GitHub Actions provides specific security mechanisms to manage the risk of running untrusted code from fork pull requests, especially when self-hosted runners or sensitive secrets are involved. When a pull request is opened from a fork, GitHub restricts workflows triggered by the pull_request event to use a read-only GITHUB_TOKEN and denies access to repository secrets by default [1][2]. However, the pull_request_target event is significantly more dangerous because it runs in the context of the base repository and has access to secrets [1][3]. To prevent "pwn request" attacks where malicious fork code is executed with elevated privileges, GitHub and the actions/checkout action have implemented specific protections [1][4][3]: 1. Approval Requirements: Repositories can be configured to require manual approval from a maintainer with write access before workflows can run on pull requests from external contributors [5][6]. This can be set at the repository, organization, or enterprise level to apply to all external contributors or only first-time contributors [5][6]. 2. Checkout Restrictions: As of June 2026, actions/checkout (v7+) by default refuses to fetch fork pull request code when used within pull_request_target or workflow_run workflows [4]. This prevents the common pattern of checking out untrusted head code in a privileged context [1][4]. 3. Opt-out Mechanism: If a workflow legitimately requires checking out fork code with elevated trust, users must explicitly add the allow-unsafe-pr-checkout: true input to the actions/checkout step [7][4]. This input is intentionally named to signal a deliberate security decision and facilitate static analysis [7][4]. Self-hosted runners pose an additional, high-consequence risk because they persist state and often have access to internal network resources [8][9]. GitHub explicitly recommends avoiding self-hosted runners for public repositories [10][9]. If they must be used, you should: - Enforce strict isolation and ensure runners are ephemeral [1][9]. - Use Workflow Execution Protections to restrict which events (like pull_request_target) can trigger workflows and who can trigger them [11]. - Implement a two-step workflow pattern: use an unprivileged workflow (triggered by pull_request) to test code and store artifacts, followed by a privileged workflow (triggered by workflow_run) to perform actions requiring secrets [3]. Always prioritize using GitHub-hosted runners for public repositories to ensure a fresh, sandboxed environment for every job [8][9].
Citations:
- 1: https://docs.github.com/en/actions/reference/security/securely-using-pull_request_target
- 2: https://microsoft.github.io/zerotrustassessment/docs/workshop-guidance/devsecops/DS_125
- 3: https://codeql.github.com/codeql-query-help/actions/actions-untrusted-checkout-medium/
- 4: https://github.blog/changelog/2026-06-18-safer-pull_request_target-defaults-for-github-actions-checkout/
- 5: https://docs.github.com/en/actions/how-tos/manage-workflow-runs/approve-runs-from-forks
- 6: https://docs.github.com/en/repositories/managing-your-repositorys-settings-and-features/enabling-features-for-your-repository/managing-github-actions-settings-for-a-repository
- 7: https://docs.github.com/en/enterprise-cloud@latest/actions/reference/security/securely-using-pull_request_target
- 8: https://safeguard.sh/resources/blog/securing-self-hosted-github-actions-runners
- 9: https://blog.gitguardian.com/github-actions-security-cheat-sheet/
- 10: https://docs.github.com/en/enterprise-cloud@latest/actions/how-tos/manage-runners/self-hosted-runners/add-runners
- 11: https://docs.github.com/en/enterprise-cloud@latest/admin/enforcing-policies/enforcing-policies-for-your-enterprise/actions-policies/workflow-execution-protections
Security Misconfiguration (CWE-284)
Reachability: External
Exploitability: Moderate
Don’t treat the fork-routing expression as a security boundary
The fork-routing condition lives in workflow code, even if in a reusable workflow. An external author can change the pull_request.head.repo.fork fork or label expression to select builder02/lxc directly, and approval for first-time contributors only lets trusted contributors later trigger malicious PR workflows without approval. The persistent self-hosted builders should not run fork-controlled workflow code; keep PR jobs on ubuntu-latest and reserve self-hosted runners for protected default-branch/scheduled jobs, enforced by organization-level runner restrictions if self-hosted execution is unavoidable.
🧰 Tools
🪛 LanguageTool
[uncategorized] ~416-~416: The official name of this software platform is spelled with a capital “H”.
Context: ...ed job uses the reference's expression: `runs-on: ${{ github.event.pull_request.head.repo.fork && 'u...
(GITHUB)
Two ways the lint gate could report clean without having checked anything. An unmatched LINT_ONLY left SEL empty, so both loops in run-all.sh iterated zero times and the script printed "all linters clean" and exited 0. lint.yml narrows the run with LINT_ONLY, so a renamed or mistyped checker would have made the whole job vacuous. An empty selection is now exit 2 -- the same "could not run" status as a missing linter -- naming the value and the known checkers. install-linters.sh printed sha256sum of the actionlint tarball and installed it regardless: a check-shaped no-op, worse than no check because it reads as done. The digest is now pinned beside the version and compared, matching the mandatory-verification policy the repo already applies in versions.env. Same fix in the README, which piped the tarball straight into tar. ci/linter/selftest.sh carries the negative controls for the selector and runs in lint.yml ahead of the linters; with the guard reverted, three of its four cases fail.
The lane map was measured from one green run. The next green run of the same tree put Security scanners at 94s against the 47s recorded here, and lane B finished 1s ahead of Fuzzing rather than 39s. builder02 runs the package builds too, so a single sample is not a duration. Header now carries the spread across both runs and drops the "~59s of headroom" claim: lane B is roughly at budget, so new work there needs a fourth lane.
lint_files() ate a failed git diff/ls-files as an empty file list -- 0 files reads as clean, not as an error. ci-build.sh's DEFAULT_VERSION lookup dereferenced NGINX_VERSION/ANGIE_VERSION unguarded, dying with an unbound-variable error under set -u instead of reaching the FATAL block that already exists for a missing pin.
…ify retry budget load-versions.sh dereferenced $GITHUB_ENV under set -u with no guard, so running it outside a workflow step died on a bare 'unbound variable' instead of a usable message. compute-versions.sh's api() and the nginx.org download-page fetch had no --retry/--connect-timeout/--max-time, unlike fetch-verify.sh -- a transient blip failed the weekly bump job or hung a runner instead of retrying.
The alternation was written twice, once to decide and once to log -- same fail-closed behaviour, same matched-paths logging, just one regex instead of two that could drift apart.
…plicitly Relied on actionlint's cwd-based auto-discovery of the config file; pre-commit hooks don't guarantee cwd the way an interactive shell invocation does. -config-file makes the self-hosted-runner-label allowlist explicit. The separate -ignore for unknown GitHub-hosted runner images stays -- there's no config-file section for that, only for self-hosted labels.
ruff floated to whatever PyPI served on each install, so local and CI findings could diverge the same way an unpinned semgrep would. zizmor stays unpinned on purpose -- its rule set is the point -- and the README explanation now says so against both pinned tools instead of just semgrep.
pre-commit applies the top-level exclude before any per-hook rule, so every suffix in that regex is invisible to every hook. .pem and .der were in it to keep the whitespace fixers from rewriting key material -- and took detect-private-key, whose whole job is finding a .pem, out with them. Probe, identical fake key in both files: probe-key.pem -> detect private key ... (no files to check) Skipped, exit 0 probe-key.txt -> detect private key ... Failed, Private key found Both suffixes move to per-hook excludes on the three fixers that rewrite files. After the change the .pem probe Fails and trailing-whitespace still skips it. Also drops two job-level comments that quoted the old single-sample durations the header no longer claims.
|
Working through the 2026-07-31 review. Verdicts below, each verified against the code rather than taken on the description. Fixed
Not changed
Deferred
|
The ci/feedback/ README's rule: acting on a finding means deleting it in the same commit, because a file that outlives the thing it describes reads as a live problem forever. These should have gone with the two commits that resolved them. nginx-http-shield-module-2026-08-05.md removed entirely -- all three findings are resolved and the file's header describes findings that no longer exist: - #1 (no gate ties the runner ban to the TRIGGER) is REFUTED, not ported. The claim was that a bare [self-hosted, builder02, lxc] on pull_request: passes check_runners; it does not. TRUST_SPLITS holds full fork-ternary strings from _trust_split(), so a bare list fails the membership test. The proposed replacement is also less trigger-aware than what is here: it greps a literal pull_request trigger and would go vacuous on every workflow_call member. - #2 (nothing enforces a member carries no push:) was ported in 2026-08-06 as lint-ci-cadence.sh and left undeleted. - #3 (fuzz.dict drift) acted on in 2b0f7b2 as a PROMPT.md step 27 instruction. nginx-label-autoconf-module-2026-08-05.md keeps its record of what was verified but loses #5, acted on in 2b0f7b2. The numbering keeps the gap deliberately -- the summary table and sibling findings cite these numbers. Finding 6 stays: it is a recommendation about what step 44 should require of an adopter claiming "no request surface", and still needs an owner call rather than a fix.
…t instruction) (#26) * ci: content-stamp the .github/ files an adopter copies An adopting module copies this repo's .github/ tree and then edits it. Neither side could answer "is that copy of build-test.yml still the one the skeleton ships, or did the adopter edit it / did the skeleton move?" without diffing every file by hand. sync-stamp.sh puts a `# sync-sha:` line in each shared file, excluded from its own digest so it is stable across re-stamping. `--list` on two repos, diff the output, and every drifted file names itself. lint-sync-stamp.sh runs --check so a stamp cannot rot into decoration: an edit without re-stamping goes red. Ported from nginx-error-abuse-module 8cdbfb2 rather than the earlier variant in nginx-label-autoconf-module. The differences are load-bearing: it rejects a duplicate stamp line (content_sha strips every one while stamp_of reads only the first, so a second line injected by a bad merge is invisible to both and --check would pass); it materialises the target list so a find that aborts is catchable rather than reaching the loop as a plain EOF and reporting "all N current" over fewer files; it sorts and matches both .yml/.yaml spellings; and it writes back through the original file, keeping the mode without GNU-only chmod --reference. Also adds ci-cadence to lint.yml's LINT_ONLY. It was added in 2026-08-06 and had never run remotely -- the allowlist is narrower than run-all.sh's glob, so a checker present locally can be absent from every PR. Verified: --check goes red on a stale stamp, a missing stamp, a duplicate placed after the genuine one, and a discovery failure (exit 2), and stamp mode repairs a duplicate back to exactly one line. * ci: require typed per-member secrets, and gate the fuzz-dictionary instruction Two adoption-feedback decisions, from ci/feedback/. Each is deleted from its feedback file in this commit, per that directory's README: a finding that outlives the thing it describes reads as a live problem forever. label-autoconf #5 -- every workflow_call member here declares `workflow_call: {}` with no secrets: stanza. That is not "needs no secrets", it is "can never receive one": an undeclared secret is not passed, so a member that starts needing a token sees an empty string and fails downstream of the cause -- a checkout that 404s, a curl that 401s -- with nothing naming the call boundary. Of the two available stances this takes (a), typed `required: true` per member, over (b) `secrets: inherit` at the caller. (b) is less boilerplate but widens every member's blast radius to the caller's full secret set, including secrets it never reads. A member's secret surface belongs at the member. This repo declares no secrets and should not: every job is contents: read over a public tree. The member half of the check is therefore vacuously green here by design, and its value is the moment an adopter adds the first one. The caller half is live today -- it goes red on inherit and on a secret passed to a member that never declared it. shield #3 -- PROMPT.md step 27 told an adopter to "use target tokens in the dictionary" without suggesting deriving them from the source of truth or gating the drift. A hand-listed fuzz.dict goes stale silently: adding a signature and forgetting the dictionary does not fail the fuzz gate, because a merely incomplete dictionary still produces a green crash-only run. Step 27 now says generate-and-drift-gate when the tokens live in a table, and records the measurement that makes the result readable: deriving it moved cov not at all (199 both arms) while signature reach went 23 -> 35 of 645 table literals. An adopter judging it by edge coverage concludes it did nothing. Each branch of the secrets check has a fixture that dies when the branch dies. Three earlier mutants survived and each exposed a real defect rather than a missing test: - `secrets: inherit` fell through to `set(passed)` and was iterated character by character -- red, for the wrong reason. Now an explicit PolicyError. - the missing-`required`-key branch and the value branch were redundant: every spec without the key yields None from .get(), and None is not True, so deleting the first changed no verdict. The split now runs on not-a-mapping vs mapping, which .get() genuinely cannot decide. - deleting the isinstance guard made the null-spec fixture die on None.get(). An AttributeError also exits 1, so an exit-status-only control stayed green through a crash. selftest.sh grows policy_msg_ for that case: red, and red with the stated finding. selftest.sh also now cross-checks lint.yml's LINT_ONLY against the ci/linter/ glob. That allowlist is narrower than what run-all.sh discovers, so a checker can run locally and in the hook while being absent from every PR -- which is how ci-cadence shipped without ever running remotely. * ci(feedback): delete the findings acted on in 8e81254 and 2b0f7b2 The ci/feedback/ README's rule: acting on a finding means deleting it in the same commit, because a file that outlives the thing it describes reads as a live problem forever. These should have gone with the two commits that resolved them. nginx-http-shield-module-2026-08-05.md removed entirely -- all three findings are resolved and the file's header describes findings that no longer exist: - #1 (no gate ties the runner ban to the TRIGGER) is REFUTED, not ported. The claim was that a bare [self-hosted, builder02, lxc] on pull_request: passes check_runners; it does not. TRUST_SPLITS holds full fork-ternary strings from _trust_split(), so a bare list fails the membership test. The proposed replacement is also less trigger-aware than what is here: it greps a literal pull_request trigger and would go vacuous on every workflow_call member. - #2 (nothing enforces a member carries no push:) was ported in 2026-08-06 as lint-ci-cadence.sh and left undeleted. - #3 (fuzz.dict drift) acted on in 2b0f7b2 as a PROMPT.md step 27 instruction. nginx-label-autoconf-module-2026-08-05.md keeps its record of what was verified but loses #5, acted on in 2b0f7b2. The numbering keeps the gap deliberately -- the summary table and sibling findings cite these numbers. Finding 6 stays: it is a recommendation about what step 44 should require of an adopter claiming "no request surface", and still needs an owner call rather than a fix. * ci(secrets): catch a required secret that no caller wires Self-review of the check found it green in the mirror direction of the case it was written for: a member declaring `required: true` with no caller passing it. Both halves read as correct alone -- the declaration is the endorsed shape, and a bare `uses:` with no secrets block is right for every other member here. GitHub does refuse to start that call, so this fails loudly rather than silently. That is what `required: true` buys. But it fails on the first run after merge, and the caller/member pairing is checkable at review time, which is the point of having the linter at all. `secrets-required-not-wired` fixture added as the red twin of `secrets-undeclared`; verified by mutation (disarming the branch turns it green). Also drops a dead `member` reassignment and computes `call` after the guard that validates it rather than before. * docs(prompt): correct the linting steps for the checkers added in this PR Answering "is there a step for zizmor and C/nginx linting": yes, steps 32-35 install the gate, 46 re-proves every checker still bites, 47 audits the drift classes. The C and nginx checkers have their own hazard covered at step 12 -- they pass green on an EMPTY selection, so the probe plants malloc/strcpy beside the module's real C and requires exit 1. What was wrong rather than missing: - Step 34 quoted the reference's LINT_ONLY as a literal string, which this PR had just invalidated. It now says to read the current value and warns that a string quoted in a document (including that one) is the thing that goes stale. - Step 34 and 47 both said nothing cross-checks LINT_ONLY against the scripts that exist. A selftest.sh case does now, so 34 says to port the case rather than the string and 47 says to re-run it. - "The three repo-policy checks" and the linter README's "last four" both miscounted after cadence and secrets landed. Replaced the positional wording with the names, so the next addition does not silently falsify it. Also adds the transfer conditions for the checks this PR introduced, since both have adoption caveats an adopter would otherwise hit blind: ci-secrets is vacuously green on a target with no secrets and only its caller half is live immediately; sync-stamp's value is in the reference-to-adopter direction, so an adopter running it in isolation gets a stamp file and no signal. * ci(secrets): reject `secrets: inherit` on external reusable workflows The caller loop filtered to local `./.github/workflows/` members before it judged `secrets: inherit`, so a call to `owner/repo/.github/workflows/x.yml@ref` was skipped entirely. That inverted the severity the check exists to enforce: a local `inherit` overshares within one repository, while the skipped case hands the caller's whole secret set to code maintained elsewhere, at a ref that can move. Judge `inherit` on the call site alone, before the local filter, and keep the rest of the loop local-only -- everything below compares against `declared`, which only knows local members. The new fixture is a real control, not a restatement of secrets-inherit: restoring the old ordering leaves secrets-inherit red and turns secrets-inherit-external green, so the local fixture alone could never have gated this path.
1 participant
TL;DR
To build nginx we download a source tarball. We want two guarantees: that we're building the version we meant to, and that the bytes we got are the bytes upstream published. The second one needs a checksum — a short fingerprint of the file — that we've written down in advance.
The problem was where we wrote things down. The version number lived in seven separate CI files. The fingerprint lived somewhere else entirely, in a lookup table inside a shell script. Two lists, two different people editing them, no mechanism tying them together. It's the recipe on the fridge and the shopping list in your pocket: fine right up until you update one of them.
Nothing was broken — the numbers happened to agree. But bump the version and forget the fingerprint, and the build either fails confusingly or, worse, skips the check entirely and builds whatever arrived. Miss one of the seven files and that job quietly keeps testing an old nginx while showing green.
Both now live on adjacent lines in one file, written by one script.
In code terms: all version and sha256 pins move to
.github/versions.env, loaded into$GITHUB_ENVby.github/scripts/load-versions.shas the first step after checkout in all 13 nginx-building jobs, and regenerated wholesale by.github/scripts/compute-versions.sh.Severity: None (CI maintainability — no module behaviour change; closes a latent stale-digest path)
What is going on
Version pins were spread across three kinds of location:
NGINX_VERSION:— a top-levelenv:in each ofbuild-test.yml,ci-deep.yml,codeql.yml,valgrind.yml,asan.yml,fuzzing.yml,security-scanners.yml. Seven copies of one number.build-flavorsmatrix —ci-deep.yml: three more hardcoded literals (version: "1.31.2","1.30.3","1.12.0").NGINX_SHA256/ANGIE_SHA256—ci/tools/ci-build.sh: bash associative arrays keyed by version.Relevant symbols:
bump_nginx_workflow_pin()—ci/tools/bump-versions.sh:seds the new version into a hardcoded list of seven workflow paths. Its own comment warned that the list must stay equal to whatgrep -l 'NGINX_VERSION:'returns, "or a bumped mainline leaves the omitted gate testing a stale nginx (silent + green)". The hazard was understood; it just had no enforcement behind it.bump_sha256_pin()—ci/tools/bump-versions.sh:seds a new digest line into the array inci-build.sh, a separate file from every version it corresponds to.matrix_version_for_label()—ci/tools/bump-versions.sh: parsesci-deep.ymlwithawkto recover the currently-pinned matrix versions.The digest lookup in
ci-build.shtreated a missing entry as non-fatal:That was a deliberate trade-off, and the old comment says why: the skeleton tracks a moving nginx release, so refusing to build an unpinned version "would break every future version bump until someone updates this table first". Given a hand-maintained table, that reasoning holds.
Why that is a problem
The failure needs no attacker — an ordinary bump reaches it:
bump-versions.shresolves a new mainline, say 1.31.2 → 1.31.3.bump_nginx_workflow_pin()rewrites the seven workflow files.bump_sha256_pin()computes and inserts the digest intoci-build.sh.Steps 2 and 3 edit different files through different code paths, and nothing checks that both landed. If step 3 is skipped, misses, or a human bumps a version by hand in the obvious place (the workflow) without touching the array,
ci-build.shfinds no entry for the new version, prints the warning, and builds the unverified tarball anyway. The supply-chain check silently switches itself off at exactly the moment the version changed — the one moment it was there to cover.The parallel failure is coverage: add a workflow with an
NGINX_VERSION:env and forget to add it to the list insidebump_nginx_workflow_pin(), and that job keeps building a pinned-forever old nginx. It stays green while testing something nobody intended.Both are deterministic given the triggering edit, not timing- or input-dependent. Neither is reachable through any request path — this is build-time plumbing, so the impact is CI trustworthiness, not runtime security.
Proposed fix
.github/versions.env, oneKEY=valueper line, with each*_SHA256written directly beneath the version it belongs to. One file, one writer, both facts visible together..github/scripts/load-versions.shvalidates each line matches^[A-Za-z_][A-Za-z0-9_]*=and appends it to$GITHUB_ENV, failing loudly on a malformed file rather than injecting garbage. Runs as the first step after checkout in all 13 jobs that build nginx..github/scripts/fetch-verify.shperforms download and verification together, with retries and connect/max timeouts. It re-hashes a tarball already present in.build/instead of trusting it, so a poisoned build cache is caught rather than assumed clean on a warm run..github/scripts/compute-versions.shresolves nginx mainline/stable fromnginx.org/en/download.htmland Angie from the GitHub releases API, hashes each archive, and rewritesversions.envwholesale. Version and digest are emitted in the same operation, so they cannot separate.ci/tools/bump-versions.shbecomes a wrapper over that script, keeping itsci/vendor/nginx-testssubmodule bump and--dry-run.bump_nginx_workflow_pin(),bump_matrix_pin(),bump_sha256_pin(),matrix_version_for_label()andsha256_for()are deleted — every structure they edited is gone.ci/tools/ci-build.shsourcesversions.envbefore defaulting$VERSION, so the default tracks the pin instead of being a literal (1.31.2) that rots silently.compute-versions.shalways emits both together, so that case can only arise from a hand-edit that half-did the job — which should fail, not build.$VERSIONis rejected before the digest lookup. Left unguarded, it would match an empty"${NGINX_STABLE:-}"pattern in thecaseand adopt the wrong digest.On Angie, one detail is easy to "simplify" wrongly: the version is resolved from the GitHub releases API (the only machine-readable index upstream offers) but hashed from
download.angie.software, which is whatci-build.shactually fetches. The two archives differ byte-for-byte. Hashing the GitHub tag archive would pin a digest that never matches at build time — a comment incompute-versions.shsays so, because the tempting cleanup here breaks the build.Matrix cells in
ci-deep.ymlnow carryversion_key:naming aversions.envkey rather than a literal, dereferenced by aResolve matrix versionstep. A matrix expands before any step runs, soload-versions.shcannot feed it directly; the indirection is what keeps the versions themselves in one file. The jobname:usesmatrix.labelfor the same reason — the resolved version isn't available at name-evaluation time.bump.ymlkeeps its existingBUMP_PR_TOKENaskpass,--force-with-lease, and open-PR check. The defaultGITHUB_TOKENcannot create PRs onmyguard-labsrepos, so the token handling is not interchangeable with the upstream version of this workflow.Testing
Run locally on builder02:
bash ci/tools/ci-build.sh nginx "" debug— builds 1.31.3 resolved fromversions.env; digest verified.bash ci/tools/ci-build.sh nginx 1.30.4 debug— stable cell builds; digest verified.bash ci/tools/bump-versions.sh --dry-run— reports up-to-date, working tree unmodified afterwards.bash .github/scripts/compute-versions.sh— regeneratesversions.envbyte-identical to the hand-written file (diffclean), which is what confirms the committed pins and the generator agree.Negative controls, each confirmed to fail closed:
NGINX_VERSION_SHA256set to a bogus digest: exits 1, prints expected vs actual, and deletes the rejected tarball so the next run cannot pick it up as a cache hit.ci-build.sh nginx 1.29.0: exits 1 listing the available pins. This is the case that previously warned and built.NGINX_VERSION=in a scratchversions.env: exits 1 at the empty-version guard.Static checks:
actionlint1.7.7 clean across all eight workflows;zizmor1.26.1 reports no findings;shellcheckclean apart from two SC2015 info notes onA && B || Cguards.bash -nclean on all four scripts.Two further controls added after review found real defects, both now fixed in
b5e3c07:fetch-verify.sh,compute-versions.shpreviously wroteNGINX_MAINLINE_SHA256=SPURIOUSintoversions.env. Progress text moved to stderr and the digest is now shape-validated; the same control exits 1. Regeneratingversions.envafterwards still reproduces the committed file byte-for-byte.bump-versions.shaborted underset -eon the pre-statecatbeforecompute-versions.shcould create the file. Now reportsCHANGED=1and generates the pins.CI: the full laned suite is green on head
df422a8— Build&Test (5 jobs), A/UBSan, Valgrind, CodeQL, Fuzzing, Security scanners, all under oneCIrun. A/UBSan running there is the positive control for the changed-files gate; it was skipped on the two prior heads until the fail-open bug above was fixed.Pre-existing failure, not introduced here: the Angie build fails under the module's
-Wshadow -Werrorflags, on Angie's own upstream sources (ngx_http_client_module.c:20shadowing a parameter,ngx_http_prometheus_module.c:844shadowing a local). Confirmed identical on the previously-pinned Angie 1.12.0, so it is not a 1.12.0 → 1.12.1 regression and not a consequence of this change. Theci-deepAngie cell is monthly-only, which is why it has gone unnoticed. Fixing it means changing warning flags or patching around upstream — a different concern with a different risk profile, so it is not folded in here.CI laning
Second concern, same six files. Each of those workflows carried its own
push/pull_requesttrigger, so a PR requested all of them at once. On the builder02 runners that is the wrong shape — wall-clock is dominated by jobs queueing for a label-matching slot, not by the jobs themselves.They become
workflow_callmembers of a single.github/workflows/ci.yml, paired longest-first into three lanes so each releases its slot to a shorter follow-up (durations measured on this branch):Follow-ups use
if: ${{ !cancelled() }}so a failing first check does not suppress an unrelated second one.codeql.ymlkeeps its independent monthlyschedulealongsideworkflow_call.The
push:triggers are also gone, which the merge gate requires: CI runs on the PR, and the merge commit is identical to the tested head. Note this leaves the skeleton diverging from all nine sibling modules, which still carrypush:triggers — that sweep is a separate decision, not folded in here.asan.yml'spaths:filter could not survive the conversion, because a reusable workflow cannot filter its own triggering. It becomes an explicit changed-files gate in the orchestrator.Two defects surfaced while verifying this, both fixed and both caught only by looking at what the run actually did:
build-test.ymlcancelled its own caller. Its concurrency group was byte-identical toci.yml's, and a called workflow inherits the caller'sgithub.workflow/github.ref— so withcancel-in-progressit killed the orchestrator before lane C started a single job. The run reported failure with no job to point at. Both sides now carry distinct prefixes.code=falseon this PR, which touches seven files it should match, so A/UBSan was skipped. The checkout was shallow,base...HEADdied with "no merge base", and that failure was invisible because it sat left of agrep -qpipeline — the step exited 0 and fell through to the else branch, disarming a sanitizer while reporting green. Nowfetch-depth: 0, the diff is captured before matching so a git failure exits 1, and the matched paths are logged.Compatibility
Version pins move to their current upstream releases as a side effect of regenerating the file: nginx mainline 1.31.2 → 1.31.3, stable 1.30.3 → 1.30.4, Angie 1.12.0 → 1.12.1. Both nginx builds pass locally on the new pins; Angie fails for the pre-existing reason above.
Anyone invoking
ci/tools/ci-build.shwith an explicit version outside the three pinned inversions.envnow gets a hard failure instead of an unverified build. That is the intended behaviour change, and the error message names the fix.