ci+parser+test: fix all 7 audit findings (F1-F7, sha e289021)#82
Merged
Conversation
… input
fuzz_seconds was spliced directly into a run: shell command via
${{ }} expansion, so a dispatch value like 3600"; id; # became
executable shell on the self-hosted builder02 runner (zizmor
high-confidence finding, audit sha e289021 F1).
Fix: pass the raw input through env: (not interpolated into command
text), validate ^[0-9]+$ and a bounded range (<=14400s) before use,
write with printf instead of echo of an unvalidated value. zizmor
no longer reports a template-injection finding on this file.
DIR ("<flavor>-<version>") and SRCDIR ("$ROOT/<flavor>-<version>")
are the same path, so the tarball already extracts to $SRCDIR and the
subsequent mv failed with "cannot move to a subdirectory of itself"
on every clean-root / NO_CACHE=1 build -- meaning ci-deep's new
nginx-stable and angie build-flavors coverage never actually ran
(audit sha e289021 F2).
Guard the mv on the paths actually differing. Verified with a clean
BUILD_ROOT: both nginx 1.31.2 and angie 1.11.5 now configure, compile,
and produce the filter/static .so through to a full build success.
master carries a required-pull-request ruleset with no bypass actor, so the weekly bump job's git push origin HEAD:master was always rejected in practice -- pins/digests silently stopped updating despite a green-looking scheduled job (audit sha e289021 F4). Push a dated branch and open a PR via a classic PAT (secrets.BUMP_PR_TOKEN, set as a repo secret) instead -- the default GITHUB_TOKEN can't create PRs on myguard-labs repos. Normal review/required-checks still gate the merge. Also dropped the already-stale nginx-tests-submodule wording from the header comment and tightened permissions: contents: read (git ops now go through the PAT env, not the default token).
…fy on cache-hit too Three related gaps in tarball provenance (audit sha e289021 F3): 1. Signer keys were fetched from nginx.org at CI time -- the same origin serving the tarball and signature, so an origin compromise could substitute all three. Keys now live vendored in tools/keys/ (committed), imported only from there; rotation is a reviewed PR. 2. nginx-stable (statically pinned in ci-deep.yml's build matrix, unlike floating mainline) had no sha256 pin, only PGP. Added NGINX_SHA256 next to ANGIE_SHA256, checked the same way. Floating mainline still relies on PGP alone (no static pin is possible for it by design). 3. A cache-hit tarball skipped verification entirely -- only a fresh download was checked. Verification now runs unconditionally after the download-or-cache-hit branch, so a corrupted/tampered cached tarball is caught on the next run. tools/bump-versions.sh's nginx-stable path now fetches+verifies the new version's PGP signature (against the same vendored keyring) before recording its sha256 pin -- a bump script that pins a digest without checking provenance first would just relocate the trust gap. Verified: nginx mainline (1.31.2, PGP-only), nginx-stable (1.30.3, PGP+sha256), angie (1.11.5, sha256) all build clean from a fresh BUILD_ROOT. Corrupted a cached nginx-stable tarball and confirmed the now-unconditional verification step catches it on cache-hit (previously would have silently reused it). bump-versions.sh's sha256_for_nginx_stable() independently reproduces the same digest pinned in ci-build.sh for 1.30.3.
…at downloads it F3's scope wasn't limited to ci-build.sh -- codeql.yml, security-scanners.yml, valgrind.yml, and two jobs in build-test.yml (old-libzstd, ASAN/UBSAN, and the matrix-driven full-debug build) each did their own raw wget+tar of an nginx tarball with ZERO verification (not even the PGP check ci-build.sh had before this fix). Added the same vendored-keyring PGP verify step (tools/keys/) to each, running unconditionally after the cache-hit-or-download branch so a cached tarball gets re-checked too, not just a fresh download. Left build-test.yml's 'Download nginx binary (mainline, from build matrix)' step alone -- that's an actions/download-artifact pull of this same workflow run's own already-verified build job output, not a raw nginx.org fetch.
After a valid qvalue, the trailing-junk check only looked at the byte immediately following the qvalue digits (catching "q=1x"), but a subsequent OWS-skip loop consumed any space/tab BEFORE the outer parameter loop re-checked for ';'/',' -- landing on an arbitrary non-delimiter byte silently ended the loop as if there were no more parameters, instead of rejecting. "zstd;q=1 garbage" and "zstd;q=0.5\tjunk" both negotiated zstd despite trailing invalid text (audit sha e289021 F5; the module's own comment and the independent fuzz oracle already assumed this was rejected). Added the same delimiter check after the general trailing-OWS skip that already exists after the q-specific check. Verified: reproduced the bug with a standalone harness built via fuzz/extract_parser.sh against ngx_shim.h, confirmed the two new TAP cases (T78/T79) fail against pre-fix code and pass after the fix, full t/00-filter.t (936 subtests) + t/01-static.t + t/02-conf-warn.t (332 more) all green, and re-ran the accept_encoding fuzz target 1.3M+ execs with no crash/divergence.
… load Two related gaps (audit sha e289021 F6): the readiness retry loop never asserted success after its 10s window (a backend serving only 500s/connection-refused would silently 'pass' readiness and proceed straight to load), and each worker's curl -f failure was caught by the if/then with no else -- a failing request was silently discarded rather than counted, so a live nginx returning nothing but errors could finish a clean soak with zero successful requests recorded. Readiness now exits 1 (dumping error.log) if no request ever succeeds. Each worker now tracks ok/bad counts, logs a failed request instead of dropping it, and requires zero bad + at least one ok to pass; wait already captures the aggregate via fail=1 per worker. Verified: a real 15s/4-worker soak against a static --add-module build passes clean (4000+ successful requests, 0 bad) with the new accounting -- confirming the stricter checks don't regress the happy path. Manually confirmed curl -f returns nonzero against an all-500 backend, which is exactly what the fixed readiness loop now treats as a hard failure instead of silent success.
F7: README/SECURITY/CONTRIBUTING materially disagreed with live CI (audit sha e289021). Fixed: - README claimed 'nginx 1.31.0 mainline' as CI-verified and that every build exercises nginx+Angie together -- Build & Test never touches Angie at all (only ci-deep.yml does, monthly); mainline floats and isn't pinned to any single version. Corrected the Compatibility table and Testing & CI section, added the missing CodeQL row, added a Bump row/mention, and updated stale TAP counts (46/21 -> current 79/28/4). - SECURITY.md linked a removed AGENTS.md and omitted valgrind/ci-deep/ bump from its workflow list. - CONTRIBUTING.md claimed the fuzz harness builds with -Werror; it doesn't (only the module's own C sources do, in build-test.yml). - ci-deep.yml's header said 'codeql.yml is dropped entirely' -- it exists, runs on every push/PR, and is documented in the README. - Trailing whitespace in README.md. NITs: ruff --fix on two Python test files (f-string-without-placeholder, unused import). shfmt -i 4 -ci (matching this repo's existing 4-space style, not shfmt's tab default) on the five scripts the audit flagged for formatting drift; re-verified bump-versions.sh --dry-run, a full ci-build.sh angie build, and a soak.sh run all still work correctly after the reformat (whitespace-only diff, no behavior change intended or observed).
📝 WalkthroughWalkthroughThe changes add detached PGP verification for NGINX downloads, verified SHA256 pinning for stable releases, parser regression coverage, pull-request-based version bumps, validated fuzz durations, updated CI documentation, and revised test-harness reporting. ChangesArtifact integrity
Accept-Encoding parser correctness
CI automation and documentation
Test harness maintenance
Estimated code review effort: 4 (Complex) | ~60 minutes Sequence Diagram(s)sequenceDiagram
participant Workflow as GitHub Actions workflow
participant Nginx as nginx.org
participant Keys as tools/keys/*.key
participant GPG as Temporary GNUPGHOME
Workflow->>Nginx: Download tarball and .asc signature
Workflow->>Keys: Read vendored public keys
Workflow->>GPG: Import keys
Workflow->>GPG: Verify tarball signature
GPG-->>Workflow: Verification result
Workflow->>Workflow: Continue build or fail
Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 6
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In @.github/workflows/bump.yml:
- Around line 58-71: Update the branch naming and PR creation flow around the
branch variable, git checkout, git push, and gh pr create so rerunning the
workflow on the same UTC day cannot collide with an existing branch or pull
request. Use a run-unique suffix for the bump branch, or explicitly detect and
update the existing branch and PR before creation, while preserving the current
automated commit and PR metadata.
- Around line 39-43: Update the bump workflow’s authentication setup after
GH_TOKEN is initialized to configure Git credentials before the git push, using
gh auth setup-git or an equivalent mechanism. Ensure the BUMP_PR_TOKEN has
repository write permission, while preserving the checkout step’s disabled
credential persistence.
In @.github/workflows/ci-deep.yml:
- Around line 136-146: Update the RAW_FUZZ_SECONDS validation before the numeric
-gt comparison to reject values exceeding the allowed range even when they
overflow shell integer parsing. After validating digits, trim leading zeroes and
enforce a bounded length or equivalent safe parse, while preserving valid
zero-padded values and the existing 14400-second maximum before writing
FUZZ_SECS to GITHUB_ENV.
In @.github/workflows/security-scanners.yml:
- Around line 57-64: In the GnuPG setup around gnupghome in
.github/workflows/security-scanners.yml lines 57-64, add an EXIT trap
immediately after creating the temporary directory and retain the existing
cleanup as appropriate. Apply the same trap in .github/workflows/valgrind.yml
lines 61-68, and remove that workflow’s success-only rm -rf cleanup so both
workflows clean up on success and failure.
- Around line 56-61: Install the gpg package in the dependency setup for both
verification workflows before their gpg usage: update
.github/workflows/security-scanners.yml at lines 56-61 and
.github/workflows/valgrind.yml at lines 59-65. Ensure the existing gpg --import
and gpg --verify steps can run without changing their verification behavior.
In `@tools/bump-versions.sh`:
- Around line 164-169: Replace the duplicated bump_nginx_sha256_pin and
bump_angie_sha256_pin helpers with a parameterized bump_sha256_pin accepting the
target array name, new version, and digest. Use the array parameter in the
existing grep and sed operations, then update both callers to pass ANGIE_SHA256
or NGINX_SHA256 accordingly while preserving the already-pinned check and
CHANGED behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 03644a72-b6c0-45cc-8706-70930afd039b
📒 Files selected for processing (24)
.github/workflows/build-test.yml.github/workflows/bump.yml.github/workflows/ci-deep.yml.github/workflows/codeql.yml.github/workflows/security-scanners.yml.github/workflows/valgrind.ymlCONTRIBUTING.mdREADME.mdSECURITY.mdfuzz/extract_parser.shngx_http_zstd_common.ht/00-filter.ttools/bump-versions.shtools/ci-build.shtools/keys/arut.keytools/keys/maxim.keytools/keys/nginx_signing.keytools/keys/pluknet.keytools/keys/sb.keytools/keys/thresh.keytools/soak.shtools/test_reload_leak.shtools/test_test_package_artifact.pytools/test_window_cap.py
💤 Files with no reviewable changes (1)
- tools/test_window_cap.py
Comment on lines
39
to
43
| - name: Checkout | ||
| uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5 | ||
| with: | ||
| persist-credentials: true | ||
| submodules: true | ||
| persist-credentials: false | ||
| fetch-depth: 0 |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
sed -n '1,220p' .github/workflows/bump.ymlRepository: myguard-labs/nginx-zstd-module
Length of output: 2744
🏁 Script executed:
python3 - <<'PY'
from pathlib import Path
p = Path('.github/workflows/bump.yml')
text = p.read_text()
for i, line in enumerate(text.splitlines(), 1):
if 1 <= i <= 220:
print(f"{i:4}: {line}")
PYRepository: myguard-labs/nginx-zstd-module
Length of output: 3170
🏁 Script executed:
sed -n '1,220p' tools/bump-versions.shRepository: myguard-labs/nginx-zstd-module
Length of output: 7599
Authenticate git before the push. persist-credentials: false removes checkout’s credential helper, so git push origin "$branch" needs a git auth step after GH_TOKEN is set (gh auth setup-git or equivalent). Ensure BUMP_PR_TOKEN can write to the repo.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/bump.yml around lines 39 - 43, Update the bump workflow’s
authentication setup after GH_TOKEN is initialized to configure Git credentials
before the git push, using gh auth setup-git or an equivalent mechanism. Ensure
the BUMP_PR_TOKEN has repository write permission, while preserving the checkout
step’s disabled credential persistence.
Comment on lines
+58
to
+71
| branch="bump/versions-$(date -u +%Y%m%d)" | ||
| git config user.name "myguard-bump-bot" | ||
| git config user.email "actions@users.noreply.github.com" | ||
| git checkout -b "$branch" | ||
| git add -A | ||
| git commit -m "chore: bump nginx-stable/angie/nginx-tests pins | ||
| git commit -m "chore: bump nginx-stable/angie pins | ||
|
|
||
| Automated weekly version check (tools/bump-versions.sh)." | ||
| git push origin HEAD:master | ||
| git push origin "$branch" | ||
| gh pr create \ | ||
| --title "chore: bump nginx-stable/angie pins ($(date -u +%Y-%m-%d))" \ | ||
| --body "Automated weekly version check (tools/bump-versions.sh). Review the diff and let required CI checks gate the merge." \ | ||
| --base master \ | ||
| --head "$branch" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Make the bump branch and PR flow idempotent.
The date-only branch name collides whenever the workflow is rerun on the same UTC day. git checkout -b or the subsequent push will then fail if that branch or its PR already exists. Use a run-unique branch suffix or detect and update an existing branch/PR before creating a new one.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/bump.yml around lines 58 - 71, Update the branch naming
and PR creation flow around the branch variable, git checkout, git push, and gh
pr create so rerunning the workflow on the same UTC day cannot collide with an
existing branch or pull request. Use a run-unique suffix for the bump branch, or
explicitly detect and update the existing branch and PR before creation, while
preserving the current automated commit and PR metadata.
Comment on lines
+136
to
+146
| case "$RAW_FUZZ_SECONDS" in | ||
| ''|*[!0-9]*) | ||
| echo "::error::fuzz_seconds must be a non-negative integer, got: $RAW_FUZZ_SECONDS" | ||
| exit 1 | ||
| ;; | ||
| esac | ||
| if [ "$RAW_FUZZ_SECONDS" -gt 14400 ]; then | ||
| echo "::error::fuzz_seconds exceeds the 14400s (4h) budget: $RAW_FUZZ_SECONDS" | ||
| exit 1 | ||
| fi | ||
| printf 'FUZZ_SECS=%s\n' "$RAW_FUZZ_SECONDS" >> "$GITHUB_ENV" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Reject oversized numeric inputs before the -gt comparison.
The digit-only check still accepts arbitrarily long values; once they overflow the shell’s integer comparison, the -gt 14400 guard can fail open and the invalid value gets written to GITHUB_ENV. Use a bounded parse or a string-length check after trimming leading zeroes.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/ci-deep.yml around lines 136 - 146, Update the
RAW_FUZZ_SECONDS validation before the numeric -gt comparison to reject values
exceeding the allowed range even when they overflow shell integer parsing. After
validating digits, trim leading zeroes and enforce a bounded length or
equivalent safe parse, while preserving valid zero-padded values and the
existing 14400-second maximum before writing FUZZ_SECS to GITHUB_ENV.
Comment on lines
+56
to
+61
| wget -q "https://nginx.org/download/nginx-${ver}.tar.gz.asc" -O "nginx-${ver}.tar.gz.asc" | ||
| gnupghome="$(mktemp -d)" | ||
| export GNUPGHOME="$gnupghome" | ||
| chmod 700 "$gnupghome" | ||
| for keyfile in "$GITHUB_WORKSPACE"/tools/keys/*.key; do | ||
| gpg --quiet --import "$keyfile" 2>/dev/null |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo '--- security-scanners.yml ---'
nl -ba .github/workflows/security-scanners.yml | sed -n '1,180p'
echo
echo '--- valgrind.yml ---'
nl -ba .github/workflows/valgrind.yml | sed -n '1,180p'Repository: myguard-labs/nginx-zstd-module
Length of output: 238
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('.github/workflows/security-scanners.yml'), Path('.github/workflows/valgrind.yml')]:
print(f'--- {path} ---')
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 1 <= i <= 220:
print(f'{i:4}: {line}')
print()
PYRepository: myguard-labs/nginx-zstd-module
Length of output: 8976
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('.github/workflows/security-scanners.yml'), Path('.github/workflows/valgrind.yml')]:
print(f'=== {path} ===')
text = path.read_text().splitlines()
for start in [1, 41, 81, 121, 161]:
if start > len(text):
continue
end = min(start + 59, len(text))
print(f'--- lines {start}-{end} ---')
for i in range(start, end + 1):
print(f'{i:4}: {text[i-1]}')
print()
PYRepository: myguard-labs/nginx-zstd-module
Length of output: 12578
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('.github/workflows/security-scanners.yml'), Path('.github/workflows/valgrind.yml')]:
print(f'=== {path} ===')
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if 'gpg' in line or 'apt' in line or 'apk' in line or 'yum' in line or 'dnf' in line or 'brew' in line or 'pacman' in line or 'zypper' in line or 'apt-get' in line:
lo = max(1, i - 8)
hi = min(len(lines), i + 8)
print(f'--- context around line {i} ---')
for j in range(lo, hi + 1):
print(f'{j:4}: {lines[j-1]}')
print()
PYRepository: myguard-labs/nginx-zstd-module
Length of output: 7933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in [Path('.github/workflows/security-scanners.yml'), Path('.github/workflows/valgrind.yml')]:
print(f'=== {path} ===')
lines = path.read_text().splitlines()
for i, line in enumerate(lines, 1):
if any(tok in line for tok in ['gpg', 'apt', 'apk', 'yum', 'dnf', 'brew', 'pacman', 'zypper', 'apt-get']):
lo = max(1, i - 8)
hi = min(len(lines), i + 8)
print(f'--- context around line {i} ---')
for j in range(lo, hi + 1):
print(f'{j:4}: {lines[j-1]}')
print()
PYRepository: myguard-labs/nginx-zstd-module
Length of output: 7933
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in Path('.github/workflows').glob('*.yml'):
text = path.read_text()
if 'gpg' in text or 'gnupg' in text:
print(f'=== {path} ===')
for i, line in enumerate(text.splitlines(), 1):
if 'gpg' in line or 'gnupg' in line:
print(f'{i:4}: {line}')
print()
PYRepository: myguard-labs/nginx-zstd-module
Length of output: 2281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
for path in Path('.').rglob('*'):
if path.is_file() and path.suffix in {'.yml', '.yaml', '.md', '.txt'}:
try:
text = path.read_text()
except Exception:
continue
if 'gpg' in text or 'gnupg' in text:
print(f'=== {path} ===')
for i, line in enumerate(text.splitlines(), 1):
if 'gpg' in line or 'gnupg' in line:
print(f'{i:4}: {line}')
print()
PYRepository: myguard-labs/nginx-zstd-module
Length of output: 2281
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
patterns = ['gnupg', 'gpg', 'builder02', 'lxc', 'self-hosted', 'apt-get install -y', 'apt install -y']
for path in Path('.').rglob('*'):
if not path.is_file():
continue
if path.suffix.lower() not in {'.yml', '.yaml', '.md', '.txt', '.adoc'}:
continue
try:
lines = path.read_text(errors='ignore').splitlines()
except Exception:
continue
hits = []
for i, line in enumerate(lines, 1):
if any(p in line for p in patterns):
hits.append((i, line))
if hits:
print(f'=== {path} ===')
for i, line in hits[:80]:
print(f'{i:4}: {line}')
print()
PYRepository: myguard-labs/nginx-zstd-module
Length of output: 4408
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from pathlib import Path
targets = [Path('README.md'), Path('docs'), Path('.github')]
for root in targets:
if root.is_file():
paths = [root]
elif root.is_dir():
paths = [p for p in root.rglob('*') if p.is_file()]
else:
continue
for path in paths:
try:
text = path.read_text(errors='ignore')
except Exception:
continue
if any(k in text.lower() for k in ['gpg', 'gnupg', 'builder02', 'lxc', 'self-hosted runner', 'runner image', 'preinstalled']):
print(f'=== {path} ===')
for i, line in enumerate(text.splitlines(), 1):
if any(k in line.lower() for k in ['gpg', 'gnupg', 'builder02', 'lxc', 'self-hosted runner', 'runner image', 'preinstalled']):
print(f'{i:4}: {line}')
print()
PYRepository: myguard-labs/nginx-zstd-module
Length of output: 3490
Install gpg before the verification steps. Both workflows call gpg --import/--verify, but neither job’s dependency list includes it.
📍 Affects 2 files
.github/workflows/security-scanners.yml#L56-L61(this comment).github/workflows/valgrind.yml#L59-L65
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/security-scanners.yml around lines 56 - 61, Install the
gpg package in the dependency setup for both verification workflows before their
gpg usage: update .github/workflows/security-scanners.yml at lines 56-61 and
.github/workflows/valgrind.yml at lines 59-65. Ensure the existing gpg --import
and gpg --verify steps can run without changing their verification behavior.
Comment on lines
+57
to
+64
| gnupghome="$(mktemp -d)" | ||
| export GNUPGHOME="$gnupghome" | ||
| chmod 700 "$gnupghome" | ||
| for keyfile in "$GITHUB_WORKSPACE"/tools/keys/*.key; do | ||
| gpg --quiet --import "$keyfile" 2>/dev/null | ||
| done | ||
| gpg --quiet --verify "nginx-${ver}.tar.gz.asc" "nginx-${ver}.tar.gz" | ||
| rm -rf "$gnupghome" |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n== security-scanners.yml ==\n'
sed -n '1,180p' .github/workflows/security-scanners.yml
printf '\n== valgrind.yml ==\n'
sed -n '1,180p' .github/workflows/valgrind.ymlRepository: myguard-labs/nginx-zstd-module
Length of output: 7796
Install an EXIT trap for GNUPGHOME.
A failed key import or signature check skips the final rm -rf, leaving temp GnuPG directories behind on self-hosted runners.
.github/workflows/security-scanners.yml#L57-L64: addtrap 'rm -rf "$gnupghome"' EXITright after creatinggnupghome..github/workflows/valgrind.yml#L61-L68: add the same trap and drop the success-only cleanup.
📍 Affects 2 files
.github/workflows/security-scanners.yml#L57-L64(this comment).github/workflows/valgrind.yml#L61-L68
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In @.github/workflows/security-scanners.yml around lines 57 - 64, In the GnuPG
setup around gnupghome in .github/workflows/security-scanners.yml lines 57-64,
add an EXIT trap immediately after creating the temporary directory and retain
the existing cleanup as appropriate. Apply the same trap in
.github/workflows/valgrind.yml lines 61-68, and remove that workflow’s
success-only rm -rf cleanup so both workflows clean up on success and failure.
Comment on lines
+164
to
+169
| bump_nginx_sha256_pin() { | ||
| local old="$1" new="$2" digest="$3" | ||
| grep -q "\[\"${new}\"\]" tools/ci-build.sh && return 0 # already pinned | ||
| sed -i "/declare -A NGINX_SHA256=(/a\\ [\"${new}\"]=\"${digest}\"" tools/ci-build.sh | ||
| CHANGED=1 | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick win
Unify bump_nginx_sha256_pin with bump_angie_sha256_pin.
The two functions are identical except for the target array name (NGINX_SHA256 vs ANGIE_SHA256). Parameterize one helper instead of maintaining two copies.
♻️ Proposed refactor
-bump_angie_sha256_pin() {
- local old="$1" new="$2" digest="$3"
- grep -q "\[\"${new}\"\]" tools/ci-build.sh && return 0 # already pinned
- sed -i "/declare -A ANGIE_SHA256=(/a\\ [\"${new}\"]=\"${digest}\"" tools/ci-build.sh
- CHANGED=1
-}
-
-bump_nginx_sha256_pin() {
- local old="$1" new="$2" digest="$3"
- grep -q "\[\"${new}\"\]" tools/ci-build.sh && return 0 # already pinned
- sed -i "/declare -A NGINX_SHA256=(/a\\ [\"${new}\"]=\"${digest}\"" tools/ci-build.sh
- CHANGED=1
-}
+bump_sha256_pin() {
+ local array_name="$1" new="$2" digest="$3"
+ grep -q "\[\"${new}\"\]" tools/ci-build.sh && return 0 # already pinned
+ sed -i "/declare -A ${array_name}=(/a\\ [\"${new}\"]=\"${digest}\"" tools/ci-build.sh
+ CHANGED=1
+}Callers become bump_sha256_pin ANGIE_SHA256 "$NEW" "$DIGEST" / bump_sha256_pin NGINX_SHA256 "$NEW" "$DIGEST".
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| bump_nginx_sha256_pin() { | |
| local old="$1" new="$2" digest="$3" | |
| grep -q "\[\"${new}\"\]" tools/ci-build.sh && return 0 # already pinned | |
| sed -i "/declare -A NGINX_SHA256=(/a\\ [\"${new}\"]=\"${digest}\"" tools/ci-build.sh | |
| CHANGED=1 | |
| } | |
| bump_sha256_pin() { | |
| local array_name="$1" new="$2" digest="$3" | |
| grep -q "\[\"${new}\"\]" tools/ci-build.sh && return 0 # already pinned | |
| sed -i "/declare -A ${array_name}=(/a\\ [\"${new}\"]=\"${digest}\"" tools/ci-build.sh | |
| CHANGED=1 | |
| } |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tools/bump-versions.sh` around lines 164 - 169, Replace the duplicated
bump_nginx_sha256_pin and bump_angie_sha256_pin helpers with a parameterized
bump_sha256_pin accepting the target array name, new version, and digest. Use
the array parameter in the existing grep and sed operations, then update both
callers to pass ANGIE_SHA256 or NGINX_SHA256 accordingly while preserving the
already-pinned check and CHANGED behavior.
3 tasks
eilandert
added a commit
that referenced
this pull request
Jul 15, 2026
PR #82 fixed all 7 audit findings but only F5 shipped with an automated regression test (TAP T78/T79); F1/F2/F3/F4/F6/F7 relied on one-time manual verification described in commit messages, leaving them free to silently regress on the next change to ci-deep.yml/ci-build.sh/README.md. Adds three new tools/test_*.sh scripts, wired into build-test.yml's validation job (no nginx build required): - test_fuzz_seconds_validation.sh (F1): drives the exact validation shape from ci-deep.yml's "Select fuzz duration" step against injection payloads, malformed input, and the 14400s boundary. - test_ci_build_provenance.sh (F2+F3): reproduces ci-build.sh's extract+guarded-mv sequence against a tarball whose top-level dir name collides with SRCDIR (the pre-fix failure shape), plus wrong-sha256 and corrupted-cache-hit rejection cases. Includes one live clean-root angie build (skipped offline). - test_docs_ci_drift.sh (F7): checks every .github/workflows/*.yml filename is referenced in README.md and vice versa, that SECURITY.md doesn't link the removed AGENTS.md, and that CONTRIBUTING.md's fuzz -Werror claim matches reality. Caught a real drift on first run -- bump.yml existed but was never referenced by filename in README.md; fixed by adding a Bump row to the workflow table. F4 (bump.yml's PR-open path) and F6 (soak.sh's failure detection) are left as accepted gaps: F4 needs a live protected-branch fixture repo to test meaningfully, and F6's own fix already runs as the regression guard in every existing soak invocation. All three scripts pass shellcheck/shfmt -i 4 -ci/actionlint clean.
5 tasks
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
1 participant
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.
Summary
Fixes all 7 findings (4 MAJOR, 3 MINOR) plus the noted NITs from the 2026-07-15
/audit-codepass (shae289021, HIGH RISK verdict), one commit per finding:ci-deep.yml'sfuzz_secondsdispatch input was spliced into arun:shell command (zizmor high-confidence template injection) → runner RCE. Fixed viaenv:passthrough + numeric/range validation.ci-build.shalwaysmv'd the extracted tarball dir onto itself on a clean root, breaking every fresh nginx-stable/Angie build. Guarded themvon the paths actually differing.tools/keys/, added anNGINX_SHA256pin, made verification run unconditionally (cache-hit or fresh download), and extended the same PGP check to every other workflow that raw-downloads nginx (codeql, security-scanners, valgrind, build-test's two other jobs).bump.ymlpushed straight tomaster, which has a required-PR ruleset with no bypass actor — always rejected in practice. Now opens a PR via a classic PAT repo secret.zstd;q=1 garbagenegotiated zstd). Added the missing post-OWS delimiter check; two new TAP cases (T78/T79) reproduce it against pre-fix code.soak.shnever asserted readiness and silently discarded curl failures, so an all-500 backend could "pass" a clean soak. Added a hard readiness-failure exit and per-worker ok/bad accounting.AGENTS.mdlink, missing CodeQL/valgrind/bump documentation, false-Werrorfuzz claim). Corrected.-i 4 -ci, matching this repo's style) on five scripts with formatting drift, correctedci-deep.yml's stale "CodeQL is dropped" rationale.Test plan
zizmorno longer reports a template-injection finding onci-deep.ymlBUILD_ROOT,NO_CACHE=1— nginx mainline (1.31.2), nginx-stable (1.30.3), and Angie (1.11.5) all build to completionbump-versions.sh's new nginx-stable digest helper reproduces the pinned hash independentlybump.ymlYAML validated, zizmor clean, dry-run ofbump-versions.shconfirmed logic unchangedfuzz/extract_parser.sh+ngx_shim.h) reproduced the bug pre-fix; T78/T79 fail on pre-fix code, pass post-fix; fullt/00-filter.t(936 subtests) +t/01-static.t+t/02-conf-warn.t(332 more) green; fuzz target re-run 1.3M+ execs with no crash--add-modulebuild passes clean (4000+ requests, 0 bad); confirmedcurl -freturns nonzero against an induced all-500 backend, which the fixed readiness loop now treats as a hard failureactionlint,shellcheck,gitleaks(no findings in tracked files) all clean across the diffSummary by CodeRabbit
Security
Bug Fixes
Accept-Encodingvalues containing trailing junk after quality parameters.CI Improvements
Documentation