Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
134 changes: 134 additions & 0 deletions .github/workflows/build-test.yml
Original file line number Diff line number Diff line change
Expand Up @@ -928,4 +928,138 @@ jobs:
cd "$GITHUB_WORKSPACE"
bash tools/test_reload_leak.sh "$TEST_NGINX_BINARY" 5

# ── gcov coverage — separate --coverage build so the sanitizer/strict
# builds above stay unpolluted by instrumentation overhead. Informational
# (not a hard gate on absolute %, since most of the remainder is
# alloc-failure/ZSTD-API-error defensive paths not worth fault-injecting)
# but DOES fail if line coverage regresses below the last-known floor, so
# a future change can't silently drop tested surface. ─────────────────
coverage:
name: Coverage (gcov/lcov)
runs-on: [self-hosted, builder02, lxc]
timeout-minutes: 20
needs: resolve
env:
NGINX_VERSION: ${{ needs.resolve.outputs.nginx_version }}

steps:
- name: Checkout module
uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
Comment on lines +946 to +947

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🔒 Security & Privacy | 🟠 Major | ⚡ Quick win

Disable persisted checkout credentials.

Repository-controlled tests run after checkout and can read the persisted token from Git configuration, which is especially risky on the self-hosted runner.

Proposed fix
       - name: Checkout module
         uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
+        with:
+          persist-credentials: false
📝 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
- name: Checkout module
uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
- name: Checkout module
uses: actions/checkout@93cb6efe18208431cddfb8368fd83d5badbf9bfd # v5
with:
persist-credentials: false
🧰 Tools
🪛 zizmor (1.26.1)

[warning] 946-947: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false

(artipacked)

🤖 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/build-test.yml around lines 946 - 947, Update the
“Checkout module” actions/checkout step to disable persisted checkout
credentials by setting its persist-credentials option to false, while preserving
the existing pinned action version and other checkout behavior.

Source: Linters/SAST tools


- name: Install dependencies
run: |
sudo apt-get update
sudo apt-get install -y build-essential pkg-config libzstd-dev \
libpcre2-dev zlib1g-dev libssl-dev libperl-dev cpanminus \
python3 python3-requests zstd lcov wget

- name: Cache nginx source tarball
id: nginx-tarball
uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830 # v4
with:
path: nginx-${{ env.NGINX_VERSION }}.tar.gz
key: nginx-src-${{ env.NGINX_VERSION }}

- name: Download nginx source tarball
if: steps.nginx-tarball.outputs.cache-hit != 'true'
run: wget -q "https://nginx.org/download/nginx-${NGINX_VERSION}.tar.gz"

- name: Verify nginx tarball PGP signature
run: |
set -euo pipefail
wget -q "https://nginx.org/download/nginx-${NGINX_VERSION}.tar.gz.asc" \
-O "nginx-${NGINX_VERSION}.tar.gz.asc"
gnupghome="$(mktemp -d)"
export GNUPGHOME="$gnupghome"
chmod 700 "$gnupghome"
for keyfile in tools/keys/*.key; do
gpg --quiet --import "$keyfile" 2>/dev/null
done
gpg --quiet --verify "nginx-${NGINX_VERSION}.tar.gz.asc" "nginx-${NGINX_VERSION}.tar.gz"
rm -rf "$gnupghome"

- name: Configure & build nginx with --coverage instrumentation
run: |
tar -xzf "nginx-${NGINX_VERSION}.tar.gz"
cd "nginx-${NGINX_VERSION}"
./configure \
--with-compat --with-debug --with-threads --with-file-aio \
--with-http_ssl_module --with-http_v2_module --with-http_v3_module \
--with-http_gzip_static_module --with-http_realip_module \
--with-http_sub_module --with-http_addition_module \
--with-http_stub_status_module --with-http_auth_request_module \
--with-cc-opt="-DZSTD_STATIC_LINKING_ONLY --coverage -O0 -g $(pkg-config --cflags libzstd zlib libpcre2-8 openssl)" \
--with-ld-opt="--coverage $(pkg-config --libs libzstd zlib libpcre2-8 openssl)" \
--add-dynamic-module="$GITHUB_WORKSPACE"
make -j"$(nproc)"
echo "TEST_NGINX_BINARY=$PWD/objs/nginx" >> "$GITHUB_ENV"
echo "COV_OBJDIR=$PWD/objs/addon" >> "$GITHUB_ENV"

- name: Install Test::Nginx::Socket
run: |
cpanm -l ~/perl5 --notest Test::Nginx::Socket
echo "PERL5LIB=$HOME/perl5/lib/perl5:$PERL5LIB" >> "$GITHUB_ENV"

- name: Run TAP suite (drives filter/static/conf-warn paths)
run: |
export PERL5LIB="$HOME/perl5/lib/perl5:${PERL5LIB:-}"
cd "$GITHUB_WORKSPACE"
# 01-static.t asserts on a fixed ETag derived from fixture mtime;
# pin it or every run gets a fresh checkout mtime and fails.
touch -d @1541504307 "$GITHUB_WORKSPACE/t/suite/test" "$GITHUB_WORKSPACE/t/suite/test.zst"
prove t/*.t
Comment on lines +1003 to +1010

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

🎯 Functional Correctness | 🟠 Major | ⚡ Quick win

Configure the servroot required by the static TAP suite.

prove t/*.t includes t/01-static.t, but this step only pins timestamps. Arrange the servroot so its root ../../t/suite fixture path resolves, as the ASAN job already does.

Proposed fix
           export PERL5LIB="$HOME/perl5/lib/perl5:${PERL5LIB:-}"
           cd "$GITHUB_WORKSPACE"
+          export TEST_NGINX_SERVROOT="$GITHUB_WORKSPACE/t/servroot-static"
+          mkdir -p "$TEST_NGINX_SERVROOT"
           # 01-static.t asserts on a fixed ETag derived from fixture mtime;

As per coding guidelines, static-suite tests using relative roots must arrange the servroot so root ../../t/suite resolves correctly.

📝 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
- name: Run TAP suite (drives filter/static/conf-warn paths)
run: |
export PERL5LIB="$HOME/perl5/lib/perl5:${PERL5LIB:-}"
cd "$GITHUB_WORKSPACE"
# 01-static.t asserts on a fixed ETag derived from fixture mtime;
# pin it or every run gets a fresh checkout mtime and fails.
touch -d @1541504307 "$GITHUB_WORKSPACE/t/suite/test" "$GITHUB_WORKSPACE/t/suite/test.zst"
prove t/*.t
- name: Run TAP suite (drives filter/static/conf-warn paths)
run: |
export PERL5LIB="$HOME/perl5/lib/perl5:${PERL5LIB:-}"
cd "$GITHUB_WORKSPACE"
export TEST_NGINX_SERVROOT="$GITHUB_WORKSPACE/t/servroot-static"
mkdir -p "$TEST_NGINX_SERVROOT"
# 01-static.t asserts on a fixed ETag derived from fixture mtime;
# pin it or every run gets a fresh checkout mtime and fails.
touch -d `@1541504307` "$GITHUB_WORKSPACE/t/suite/test" "$GITHUB_WORKSPACE/t/suite/test.zst"
prove t/*.t
🤖 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/build-test.yml around lines 1003 - 1010, Update the “Run
TAP suite” step to configure the required servroot before invoking prove,
matching the ASAN job’s setup so the relative root ../../t/suite resolves to the
t/suite fixtures. Preserve the existing timestamp pinning and prove t/*.t
execution.

Source: Coding guidelines


- name: Run correctness/regression tool suite (drives filter edge cases)
run: |
cd "$GITHUB_WORKSPACE"
python3 tools/test_encoding.py --nginx-binary "$TEST_NGINX_BINARY" --port 19080
python3 tools/test_compression_matrix.py \
--nginx-binary "$TEST_NGINX_BINARY" --port 19096 --backend-port 19097
python3 tools/test_concurrent_cctx_isolation.py \
--nginx-binary "$TEST_NGINX_BINARY" --port 19098 --backend-port 19099
python3 tools/test_proxy_unbuffered_truncation.py \
--nginx-binary "$TEST_NGINX_BINARY" --port 19094 --backend-port 19095
python3 tools/test_terminal_frame.py --nginx-binary "$TEST_NGINX_BINARY" --port 19093
python3 tools/test_window_cap.py \
--nginx-binary "$TEST_NGINX_BINARY" --port 19104 --backend-port 19105
python3 tools/test_zstd_long_ldm.py \
--nginx-binary "$TEST_NGINX_BINARY" --port 19102 --backend-port 19103

- name: Capture coverage (lcov) and enforce line-coverage floor
run: |
cd "$GITHUB_WORKSPACE"
lcov --capture --directory "$COV_OBJDIR" --output-file zstd-coverage.info \
--rc lcov_branch_coverage=1 --ignore-errors inconsistent
lcov --summary zstd-coverage.info --rc lcov_branch_coverage=1 | tee coverage-summary.txt
# Floor set from the 2026-07-17 coverage push (79.2% -> 80.2% line).
# Bump this only when a deliberate coverage improvement raises it —
# never lower it to make a regression pass.
floor_pct=80
line_pct=$(grep "lines" coverage-summary.txt | grep -oE '[0-9]+\.[0-9]+%' | head -1 | tr -d '%')
line_pct_int=${line_pct%.*}
echo "Line coverage: ${line_pct}% (floor: ${floor_pct}%)"
if [ "$line_pct_int" -lt "$floor_pct" ]; then
echo "❌ Line coverage ${line_pct}% dropped below floor ${floor_pct}%"
exit 1
fi
echo "✓ Line coverage floor held"

- name: Generate HTML coverage report
if: always()
run: |
cd "$GITHUB_WORKSPACE"
genhtml zstd-coverage.info --output-directory coverage-html \
--branch-coverage --ignore-errors inconsistent || true

- name: Upload coverage report
if: always()
uses: actions/upload-artifact@330a01c490aca151604b8cf639adc76d48f6c5d4 # v5
with:
name: coverage-report
path: |
${{ github.workspace }}/zstd-coverage.info
${{ github.workspace }}/coverage-summary.txt
${{ github.workspace }}/coverage-html
retention-days: 14


108 changes: 108 additions & 0 deletions t/00-filter.t
Original file line number Diff line number Diff line change
Expand Up @@ -2019,3 +2019,111 @@ Content-Length: 59738
!Content-Encoding
--- no_error_log
[error]



=== TEST 80: repeated "q" parameter is malformed (RFC 9110 permits at most one)
--- config
location /filter {
zstd on;
zstd_types text/plain;
proxy_pass http://127.0.0.1:$TEST_NGINX_SERVER_PORT/test;
}
location /test {
root $TEST_NGINX_PERL_PATH/suite/;
}
--- request
GET /filter
--- more_headers
Accept-Encoding: zstd;q=0.5;q=0.9
--- response_headers
Content-Length: 59738
!Content-Encoding
--- no_error_log
[error]



=== TEST 81: "q" parameter present without "=value" is malformed
--- config
location /filter {
zstd on;
zstd_types text/plain;
proxy_pass http://127.0.0.1:$TEST_NGINX_SERVER_PORT/test;
}
location /test {
root $TEST_NGINX_PERL_PATH/suite/;
}
--- request
GET /filter
--- more_headers
Accept-Encoding: zstd;q
--- response_headers
Content-Length: 59738
!Content-Encoding
--- no_error_log
[error]



=== TEST 82: qvalue leading digit not 0 or 1 is malformed (q=2)
--- config
location /filter {
zstd on;
zstd_types text/plain;
proxy_pass http://127.0.0.1:$TEST_NGINX_SERVER_PORT/test;
}
location /test {
root $TEST_NGINX_PERL_PATH/suite/;
}
--- request
GET /filter
--- more_headers
Accept-Encoding: zstd;q=2
--- response_headers
Content-Length: 59738
!Content-Encoding
--- no_error_log
[error]



=== TEST 83: non-"q" parameter with an escaped quoted-pair does not confuse the delimiter scan
--- config
location /filter {
zstd on;
zstd_types text/plain;
proxy_pass http://127.0.0.1:$TEST_NGINX_SERVER_PORT/test;
}
location /test {
root $TEST_NGINX_PERL_PATH/suite/;
}
--- request
GET /filter
--- more_headers
Accept-Encoding: zstd;foo="a\"b,c";q=0.8, gzip;q=0.1
--- response_headers_like
Content-Encoding: zstd
--- no_error_log
[error]



=== TEST 84: unterminated quoted-string in a non-"q" parameter runs to end without OOB read, defaults q=1
--- config
location /filter {
zstd on;
zstd_types text/plain;
proxy_pass http://127.0.0.1:$TEST_NGINX_SERVER_PORT/test;
}
location /test {
root $TEST_NGINX_PERL_PATH/suite/;
}
--- request
GET /filter
--- more_headers
Accept-Encoding: zstd;foo="unterminated
--- response_headers_like
Content-Encoding: zstd
--- no_error_log
[error]
Comment on lines +2025 to +2129

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

📐 Maintainability & Code Quality | 🟠 Major | ⚡ Quick win

Restore the suite’s fixed assertion plan.

Adding T80–84 while Line 28 remains plan 'no_plan' removes protection against premature test termination. Restore the required repeat_each() * (blocks() * 3) + N plan and recalculate N.

As per coding guidelines, t/00-filter.t: “Keep the filter test plan constant (plan tests => repeat_each() * (blocks() * 3) + N;) in sync with the actual number of assertions.”

🤖 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 `@t/00-filter.t` around lines 2025 - 2129, Replace the no_plan setup in the
filter test suite with the fixed repeat_each() * (blocks() * 3) + N assertion
plan, recalculating N to include tests 80–84 and match the suite’s total
assertions.

Source: Coding guidelines

Loading