unworked validation sweep: fix all major findings for tasks 053-059

Address the 17 major-severity unworked review findings spanning the
five most recent task review files plus task-057. Findings that were
already addressed in code but never checked off in the review files
are marked as worked with a note pointing to the resolving commit.

Behavioural changes
-------------------
- .github/workflows/verify-build.yml: pin every `uses:` entry to a
  full 40-char commit SHA (actions/checkout@11bd719, actions/cache@
  0057852, msys2/setup-msys2@e989830, codecov/codecov-action@75cd116);
  pin the IWYU clone to clang_18 commit 377eaef + rev-parse assertion;
  add a sha256-pin (4d51346…) for the macOS curl tarball, with
  `curl -fsSL` and `set -euo pipefail`. Closes the SEC-04 supply-chain
  gaps flagged by cloud-infrastructure-reviewer on TASK-059.
- src/httpserver/detail/webserver_impl.hpp: swap exact_routes_ from
  std::unordered_map<std::string, route_entry> to std::map<std::string,
  route_entry, std::less<>>. Removes the hash-flooding (CWE-407/400)
  surface on the dispatch hot path, same posture TASK-056 applied to
  the radix-tree per-segment child container.
- src/http_request.cpp operator<<: collapse four symmetric if/else
  blocks over `expose` into a function-pointer dispatch plus a ternary
  for the pass field.
- test/unit/v2_dispatch_contract_test.cpp: renumber PORT from 8231 to
  8260 to break the EADDRINUSE race with
  hooks_handler_exception_user_handler_throws_continues_chain under
  `make check -j`.

Test changes
------------
- routing_regression_test.cpp: rename six `*_does_not_collide` tests
  to `*_throws_collision`; the bodies assert LT_CHECK(threw), the old
  names suggested the opposite.
- auth_handler_optional_signature_test.cpp: remove the redundant
  hook-count test (now owned by hooks_alias_count_test.cpp); add a
  throwing-handler pin (swallowed → request passes) and a 64 KB
  payload pin (large engaged optional arrives intact). Paired
  per-test PORT_N/PORT_N_STRING macros so curl URLs cannot silently
  drift from the constructor port.
- hooks_alias_count_test.cpp: add legacy_auth_handler_registers_one_
  before_handler under a TU-scoped #pragma so the deprecation warning
  doesn't break -Werror; matches the pattern in the legacy shim TU.
- auth_handler_legacy_shim_test.cpp: drop the moved-out hook-count
  assertion and the now-unused helper + include.

Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>

Author: etr

.github/workflows/verify-build.yml

@@ -354,7 +354,7 @@ jobs:
             shell: bash
     steps:
     - name: Checkout repository
-      uses: actions/checkout@v4
+      uses: actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683  # v4.2.2
       with:
         # We must fetch at least the immediate parents so that if this is
         # a pull request then we can checkout the head.
@@ -368,7 +368,7 @@ jobs:
 
     - name: Setup MSYS2
       if: ${{ matrix.os-type == 'windows' }}
-      uses: msys2/setup-msys2@v2
+      uses: msys2/setup-msys2@e9898307ac31d1a803454791be09ab9973336e1c  # v2.31.1
       with:
         msystem: ${{ matrix.msys-env }}
         update: true
@@ -468,30 +468,37 @@ jobs:
 
     - name: IWYU from cache (for testing)
       id: cache-IWYU
-      uses: actions/cache@v4
+      uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830  # v4.3.0
       with:
         path: include-what-you-use
         key: ${{ matrix.os }}-${{ matrix.c-compiler }}-include-what-you-use-pre-built
       if: ${{ matrix.build-type == 'iwyu' && matrix.os-type == 'ubuntu' }}
 
     # Installing iwyu manually because clang and iwyu paths won't match on Ubuntu otherwise.
-    # Supply-chain note: we clone --depth 1 --single-branch to limit exposure.
-    # TODO(security): pin to an immutable commit SHA instead of the mutable
-    # clang_18 branch reference.  Obtain the verified SHA from the upstream
-    # release page and replace the `git log` step below with a hard-coded check:
-    #   git checkout <sha> ; git rev-parse --verify HEAD == <sha>
+    # Supply-chain: pinned to an immutable commit SHA on the upstream clang_18
+    # branch (latest commit as of 2024-02-10, "[ci] Pin to LLVM 18"). To rotate,
+    # obtain the new SHA from https://github.com/include-what-you-use/include-what-you-use/commits/clang_18
+    # and update IWYU_SHA below; the assertion will fail-fast on any mismatch.
     - name: Build IWYU if requested
       run: |
-        CLANG_ROOT_PATH=`llvm-config-18 --prefix` ;
-        CLANG_BIN_PATH=`llvm-config-18 --bindir` ;
-        git clone --depth 1 --single-branch --branch clang_18 https://github.com/include-what-you-use/include-what-you-use.git ;
-        cd include-what-you-use ;
-        echo "IWYU cloned at SHA: $(git rev-parse HEAD)" ;
-        mkdir build_iwyu ;
-        cd build_iwyu ;
-        cmake -G "Unix Makefiles" -DCMAKE_PREFIX_PATH=$CLANG_ROOT_PATH -DCMAKE_C_COMPILER=$CLANG_BIN_PATH/clang -DCMAKE_CXX_COMPILER=$CLANG_BIN_PATH/clang++ ../ ;
-        make ;
-        sudo make install ;
+        set -euo pipefail
+        IWYU_SHA=377eaef70cdda47368939f4d9beabfabe3f628f0
+        CLANG_ROOT_PATH=`llvm-config-18 --prefix`
+        CLANG_BIN_PATH=`llvm-config-18 --bindir`
+        git clone https://github.com/include-what-you-use/include-what-you-use.git
+        cd include-what-you-use
+        git checkout "${IWYU_SHA}"
+        HEAD_SHA="$(git rev-parse --verify HEAD)"
+        if [ "${HEAD_SHA}" != "${IWYU_SHA}" ]; then
+          echo "IWYU SHA mismatch: expected ${IWYU_SHA}, got ${HEAD_SHA}" >&2
+          exit 1
+        fi
+        echo "IWYU pinned at SHA: ${HEAD_SHA}"
+        mkdir build_iwyu
+        cd build_iwyu
+        cmake -G "Unix Makefiles" -DCMAKE_PREFIX_PATH=$CLANG_ROOT_PATH -DCMAKE_C_COMPILER=$CLANG_BIN_PATH/clang -DCMAKE_CXX_COMPILER=$CLANG_BIN_PATH/clang++ ../
+        make
+        sudo make install
       if: ${{ matrix.build-type == 'iwyu' && matrix.os-type == 'ubuntu' && steps.cache-IWYU.outputs.cache-hit != 'true' }}
 
     - name: Install IWYU if requested
@@ -502,25 +509,28 @@ jobs:
 
     - name: CURL from cache (for testing)
       id: cache-CURL
-      uses: actions/cache@v4
+      uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830  # v4.3.0
       with:
         path: curl-7.75.0
         key: ${{ matrix.os }}-CURL-pre-built-v3
       if: ${{ matrix.os == 'macos-latest' }}
 
     - name: Build CURL (for testing)
-      run: |
-        curl https://libhttpserver.s3.amazonaws.com/travis_stuff/curl-7.75.0.tar.gz -o curl-7.75.0.tar.gz ;
-        # TODO(security): pin to a known-good SHA-256 and replace this logging
-        # line with a hard assertion, e.g.:
-        #   echo "<expected-sha256>  curl-7.75.0.tar.gz" | shasum -a 256 -c
-        # Obtain the expected digest by running this step once and capturing
-        # the output below, then commit it as the authoritative value.
-        echo "curl-7.75.0.tar.gz SHA-256: $(shasum -a 256 curl-7.75.0.tar.gz | awk '{print $1}')" ;
-        tar -xzf curl-7.75.0.tar.gz ;
-        cd curl-7.75.0 ;
-        ./configure --with-darwinssl --without-ssl --without-nghttp2 --without-libidn2 --without-libssh2 --without-brotli --without-zstd --without-librtmp --without-libpsl --disable-ldap --disable-ldaps ;
-        make ;
+      # Supply-chain: curl-7.75.0.tar.gz is sha256-pinned. The expected digest
+      # is the official upstream value also published at https://curl.se/download/
+      # (cross-checked against the libhttpserver S3 mirror). To rotate the curl
+      # version, update both CURL_VERSION and CURL_SHA256 in the same PR.
+      run: |
+        set -euo pipefail
+        CURL_VERSION=7.75.0
+        CURL_SHA256=4d51346fe621624c3e4b9f86a8fd6f122a143820e17889f59c18f245d2d8e7a6
+        curl -fsSL -o "curl-${CURL_VERSION}.tar.gz" \
+          "https://libhttpserver.s3.amazonaws.com/travis_stuff/curl-${CURL_VERSION}.tar.gz"
+        echo "${CURL_SHA256}  curl-${CURL_VERSION}.tar.gz" | shasum -a 256 -c
+        tar -xzf "curl-${CURL_VERSION}.tar.gz"
+        cd "curl-${CURL_VERSION}"
+        ./configure --with-darwinssl --without-ssl --without-nghttp2 --without-libidn2 --without-libssh2 --without-brotli --without-zstd --without-librtmp --without-libpsl --disable-ldap --disable-ldaps
+        make
       if: ${{ matrix.os == 'macos-latest' && steps.cache-CURL.outputs.cache-hit != 'true' }}
 
     - name: Install CURL (for testing on mac only) 
@@ -556,7 +566,7 @@ jobs:
 
     - name: Fetch libmicrohttpd from cache
       id: cache-libmicrohttpd
-      uses: actions/cache@v4
+      uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830  # v4.3.0
       with:
         path: libmicrohttpd-1.0.3
         key: ${{ matrix.os }}-${{ matrix.c-compiler }}-libmicrohttpd-1.0.3-pre-built-v2
@@ -588,7 +598,7 @@ jobs:
 
     - name: Fetch libmicrohttpd from cache (flag-invariance-off lane)
       id: cache-libmicrohttpd-off
-      uses: actions/cache@v4
+      uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830  # v4.3.0
       with:
         path: libmicrohttpd-1.0.3
         key: ${{ matrix.os }}-${{ matrix.c-compiler }}-libmicrohttpd-1.0.3-flags-off-v1
@@ -660,7 +670,7 @@ jobs:
 
     - name: Fetch libmicrohttpd from cache (ARM cross-compile)
       id: cache-libmicrohttpd-arm
-      uses: actions/cache@v4
+      uses: actions/cache@0057852bfaa89a56745cba8c7296529d2fc39830  # v4.3.0
       with:
         path: libmicrohttpd-1.0.3-${{ matrix.build-type }}
         key: ${{ matrix.os }}-${{ matrix.build-type }}-libmicrohttpd-1.0.3-cross-compiled
@@ -949,7 +959,7 @@ jobs:
       if: ${{ matrix.os-type == 'ubuntu' && matrix.c-compiler == 'gcc' && matrix.debug == 'debug' && matrix.coverage == 'coverage' && success() }}
 
     - name: Upload coverage to Codecov
-      uses: codecov/codecov-action@v5
+      uses: codecov/codecov-action@75cd11691c0faa626561e295848008c8a7dddffe  # v5.5.4
       with:
         token: ${{ secrets.CODECOV_TOKEN }}
         files: build/coverage.xml

specs/unworked_review_issues/2026-05-29_112007_task-053.md

@@ -6,21 +6,21 @@
 
 ## Major
 
-1. [ ] **architecture-alignment-checker** | `src/httpserver/detail/webserver_impl.hpp:248` | interface-contract
-   The NOTE comment on lookup_v2's declaration states 'lookup_v2 is a SHADOW TABLE only. The live HTTP dispatch path (finalize_answer -> resolve_resource_for_request) still uses the v1 registered_resources* maps. lookup_v2 is used by tests to pin the v2 tier pipeline and will be wired into finalize_answer at Cycle K dispatch cutover. Until then, any access control registered via the v2 table has NO effect on live traffic.' This is factually wrong post-TASK-053: resolve_resource_for_request in src/detail/webserver_dispatch.cpp (line 272) now calls lookup_v2() directly and the v1 resolver was deleted. A CWE-284 security note saying 'any access control registered via the v2 table has NO effect on live traffic' is misleading at best and a security audit hazard at worst — auditors reading this comment would conclude the v2 table is inert.
-   *Recommendation:* Replace lines 248-253 of webserver_impl.hpp with a comment reflecting the post-cutover reality: 'Walk the v2 route table for (method, path) per the lookup pipeline documented above. This is the sole route resolver used by resolve_resource_for_request after TASK-053 retired the v1 dispatch path. Returns lookup_result; populates tier even on miss (tier_hit::none) so callers can branch deterministically.'
+1. [x] **architecture-alignment-checker** | `src/httpserver/detail/webserver_impl.hpp:248` | interface-contract
+   The NOTE comment on lookup_v2's declaration states 'lookup_v2 is a SHADOW TABLE only…' — factually wrong post-TASK-053. CWE-284 security audit hazard.
+   *Fixed:* Already corrected in `feature/v2.0` prior to this sweep — the current comment now reads "Walk the v2 route table for (method, path) per the lookup pipeline…" and explicitly notes "lookup_v2 is the live dispatch path. As of TASK-053 step 3, resolve_resource_for_request() calls lookup_v2() exclusively; all v1 helper functions were deleted." Finding pre-dates the fix.
 
-2. [ ] **code-simplifier** | `src/httpserver/detail/webserver_impl.hpp:246` | comments
-   The NOTE in the lookup_v2 declaration doc-comment (lines 249-254) says 'The live HTTP dispatch path still uses the v1 registered_resources* maps. lookup_v2 is used by tests ... and will be wired into finalize_answer at Cycle K dispatch cutover.' This is the pre-cutover warning comment; the cutover has already happened in TASK-053 step 3. The comment is now factually wrong and will mislead future readers into thinking lookup_v2 is still shadow-only.
-   *Recommendation:* Remove or replace the NOTE block (lines 249-254) with a brief forward reference to TASK-058 (the canonicalize optimization) now that lookup_v2 IS the live dispatch path. A minimal replacement: '// NOTE: lookup_v2 is the sole dispatch-path resolver as of TASK-053. See TASK-058 for the planned canonicalize_lookup_path optimization.'
+2. [x] **code-simplifier** | `src/httpserver/detail/webserver_impl.hpp:246` | comments
+   The NOTE doc-comment is factually wrong post-cutover.
+   *Fixed:* See item 1 — comment already reflects post-cutover reality.
 
-3. [ ] **security-reviewer** | `src/httpserver/detail/webserver_impl.hpp:201` | A04-insecure-design
-   exact_routes_ is std::unordered_map<std::string, route_entry> using the default std::hash<std::string>. On most libstdc++ and libc++ implementations std::hash<std::string> is NOT hash-randomized per process startup (unlike Python's SipHash or Rust's RandomState). An attacker who can register many routes whose URL keys hash-collide (trivially achievable offline by finding strings with the same modular hash for any fixed bucket count) can degrade exact_routes_.find() from O(1) to O(n) per bucket probe. Every HTTP request dispatched via lookup_v2 hits the exact tier first (webserver_dispatch.cpp line 167), so a maximally-colliding route table turns every exact-tier lookup into a linear scan. This is worse than the v1 registered_resources_str path it replaced, which used std::map (ordered, O(log n) worst case, collision-free). The issue is tracked as TASK-056 but this PR promotes it from 'planned for TASK-056' to 'now live on every request'. The impact is proportional to the number of registered routes (typically small in practice) but is a genuine DoS surface for deployments with many routes or adversarially-influenced route registration. CWE-400.
-   *Recommendation:* Track as TASK-056 per existing plan. As an interim measure, note explicitly in the PR and in the webserver_impl.hpp comment that exact_routes_ is not hash-randomized and is therefore vulnerable to Hash-DoS if an attacker can influence route registration. Consider using absl::flat_hash_map with a randomized seed, or std::map as a safe (if slower) interim. Do not block this PR on this finding alone since TASK-056 is the planned fix, but ensure TASK-056 is scheduled before GA. CWE-400.
+3. [x] **security-reviewer** | `src/httpserver/detail/webserver_impl.hpp:201` | A04-insecure-design
+   `exact_routes_` is std::unordered_map<std::string, route_entry> with default std::hash<std::string>; not seeded per-process on libstdc++/libc++. Hash-DoS surface on the dispatch hot path (CWE-400 / CWE-407).
+   *Fixed:* Swapped `exact_routes_` to `std::map<std::string, route_entry, std::less<>>` for hash-free O(log n) lookup with transparent string_view comparators. Same posture TASK-056 applied to the radix-tree per-segment child container. Comment block extended with the rationale so future maintainers don't regress to unordered_map.
 
-4. [ ] **test-quality-reviewer** | `test/unit/v2_dispatch_contract_test.cpp:88` | test-isolation
-   Port 8231 is hardcoded in both test/unit/v2_dispatch_contract_test.cpp (#define PORT 8231) and test/integ/hooks_handler_exception_user_handler_throws_continues_chain.cpp (#define PORT 8231). When `make check` is run with -j2 or higher parallelism these two test binaries race on the same port, causing intermittent EADDRINUSE failures. Under serial execution (the default automake TESTS runner) the collision is latent but will surface on any CI configuration that sets MAKEFLAGS=-j or uses `make check -j`.
-   *Recommendation:* Assign v2_dispatch_contract a unique port not shared with any other test binary. The current allocation pattern uses 8180 (webserver_register_path_prefix), 8190 (webserver_on_methods), 8230 (webserver_route), 8231 (two binaries — collision). Renumber v2_dispatch_contract to 8232 (the next free slot after webserver_route's 8230). Update #define PORT and the wait_for_server_ready call-site comment.
+4. [x] **test-quality-reviewer** | `test/unit/v2_dispatch_contract_test.cpp:88` | test-isolation
+   Port 8231 is shared with `test/integ/hooks_handler_exception_user_handler_throws_continues_chain.cpp`; race under `make check -j`.
+   *Fixed:* Renumbered v2_dispatch_contract_test PORT from 8231 to 8260 (verified unused elsewhere in test/ after also checking the recently-added auth_handler tests at 8290–8295). Comment added explaining why the move was made.
 
 ## Minor
 

specs/unworked_review_issues/2026-05-29_165618_task-054.md

@@ -6,21 +6,25 @@
 
 ## Major
 
-1. [ ] **test-quality-reviewer** | `test/unit/auth_handler_legacy_shim_test.cpp:160` | redundant-test
+1. [x] **test-quality-reviewer** | `test/unit/auth_handler_legacy_shim_test.cpp:160` | redundant-test
    legacy_setter_registers_one_before_handler (line 160) asserts that the deprecated setter installs one hook via the same webserver_test_access bridge. hooks_alias_count_test.cpp does not cover the legacy setter path, so this assertion is not a strict duplicate — but the intent of the hooks_alias_count suite is to be the single location for hook-count assertions. The legacy shim TU should focus on compile-type pinning and wire behavior, not hook counts.
    *Recommendation:* Consider moving the hook-count assertion for the legacy setter into hooks_alias_count_test.cpp alongside the canonical-setter count test, so all hook-count contracts live in one file. This is a minor structural concern; the test itself is useful.
+   *Fixed:* Moved legacy hook-count assertion to hooks_alias_count_test.cpp as `legacy_auth_handler_registers_one_before_handler` (with a TU-scoped `#pragma` to suppress the deprecation warning, same pattern as the legacy shim TU). Removed the assertion and now-unused helper/include from the shim TU.
 
-2. [ ] **test-quality-reviewer** | `test/unit/auth_handler_optional_signature_test.cpp:1` | missing-test
+2. [x] **test-quality-reviewer** | `test/unit/auth_handler_optional_signature_test.cpp:1` | missing-test
    The acceptance criteria call for a negative case where the auth handler throws an exception. Neither new unit file contains a test where the handler callable throws — there is no coverage of what the dispatch path does when auth_handler itself throws (does it propagate, swallow, or invoke internal_error_handler?).
    *Recommendation:* Add a test in auth_handler_optional_signature_test.cpp where the handler throws std::runtime_error and assert the observable outcome (e.g., 500 returned, or exception propagated). This is the only way to pin the contract for error paths through the hook-invocation machinery.
+   *Fixed:* Added `throwing_handler_is_swallowed_and_request_passes` which pins the observed contract: exceptions thrown from the alias hook are swallowed and the request passes (response 200 SUCCESS). A docstring explains the design rationale (the hook framework's handler-exception slot is the named extension point; the alias hook stays non-fatal so a buggy callable cannot DoS the server). Any future change to short-circuit must update the pin.
 
-3. [ ] **test-quality-reviewer** | `test/unit/auth_handler_optional_signature_test.cpp:1` | missing-test
+3. [x] **test-quality-reviewer** | `test/unit/auth_handler_optional_signature_test.cpp:1` | missing-test
    The acceptance criteria include a negative case: 'handler returning moved-from optional, large response payload not corrupted in transit'. There is no test verifying that a large response body (e.g., 64 KB) set on the auth rejection optional arrives on the wire uncorrupted. The SBO-path claim in the production code comments ('small responses ride the http_response SBO') makes the large-payload case a meaningful regression target.
    *Recommendation:* Add a test where the auth handler returns an engaged optional whose body is a large string (e.g., 64 KB). Assert the response_code and that the received body length and content match exactly, confirming that the MOVE from optional into hook_action::respond_with does not truncate or corrupt it.
+   *Fixed:* Added `large_engaged_optional_payload_arrives_intact` (64 KB body, status 403); asserts body.size() and full equality against the constructed string.
 
-4. [ ] **test-quality-reviewer** | `test/unit/auth_handler_optional_signature_test.cpp:122` | redundant-test
+4. [x] **test-quality-reviewer** | `test/unit/auth_handler_optional_signature_test.cpp:122` | redundant-test
    accepting_optional_lambda_registers_one_before_handler (line 122) checks that the optional-signature setter installs exactly one before_handler hook. The same assertion — that auth_handler(fn) installs one hook — is already pinned in hooks_alias_count_test.cpp:auth_handler_registers_one_before_handler (line 65). Both tests use the same webserver_test_access::impl bridge, exercise the identical code path, and assert the identical value. One of them is redundant.
    *Recommendation:* Remove accepting_optional_lambda_registers_one_before_handler from auth_handler_optional_signature_test.cpp and let hooks_alias_count_test.cpp own the hook-count contract. The optional-signature TU can then focus exclusively on compile-time type pinning and end-to-end wire behavior.
+   *Fixed:* Removed the redundant test and the now-unused `before_handler_count` helper + webserver_impl include + `hook_phase` using-declaration from the optional-signature TU. Hook-count contract now lives only in `hooks_alias_count_test.cpp`. Also paired per-test PORT_N/PORT_N_STRING macros (item 9 fix) so URL strings cannot silently drift from the constructor port.
 
 ## Minor