|
| 1 | +# Unworked Review Issues |
| 2 | + |
| 3 | +**Run:** 2026-06-02 11:53:20 |
| 4 | +**Task:** TASK-059 |
| 5 | +**Total:** 15 (0 critical, 3 major, 12 minor) |
| 6 | + |
| 7 | +## Major |
| 8 | + |
| 9 | +1. [ ] **cloud-infrastructure-reviewer** | `.github/workflows/verify-build.yml:357` | iac-anti-pattern |
| 10 | + [SEC-04 Supply-Chain Integrity] GitHub Actions are pinned to mutable tag references (e.g. `actions/checkout@v4`, `actions/cache@v4`, `msys2/setup-msys2@v2`, `codecov/codecov-action@v5`) rather than immutable commit SHAs. A tag can be force-pushed by the upstream maintainer — or compromised — to silently inject malicious code into every CI run. TASK-059 correctly pins PMD by digest but the same discipline is not applied to the Actions themselves, which execute with full runner permissions. |
| 11 | + *Recommendation:* Pin every `uses:` reference to the full 40-character commit SHA, e.g. `actions/checkout@11bd71901bbe5b1630ceea73d27597364c9af683` (v4.2.2). Maintain a renovate.json or Dependabot `github-actions` ecosystem config to automate SHA bump PRs. See https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions and https://docs.github.com/en/code-security/supply-chain-security/keeping-your-dependencies-updated-automatically/configuration-options-for-the-dependabot.yml-file#package-ecosystem |
| 12 | + |
| 13 | +2. [ ] **cloud-infrastructure-reviewer** | `.github/workflows/verify-build.yml:483` | iac-anti-pattern |
| 14 | + [SEC-04 Supply-Chain Integrity] The IWYU build clones `--branch clang_18` from GitHub (`include-what-you-use/include-what-you-use.git`), which is a mutable branch reference. The existing TODO comment on lines 476-478 correctly identifies this risk but the fix has not been applied: the clone has no pinned commit SHA and no post-clone integrity check. Any adversarial push to that branch would be compiled and installed with `sudo make install` on the runner. |
| 15 | + *Recommendation:* Replace the mutable branch clone with `git clone` followed by `git checkout <immutable-SHA>` and a `git rev-parse --verify HEAD` assertion. Obtain the verified SHA from the upstream release/tag page at https://github.com/include-what-you-use/include-what-you-use/tags and commit it as a comment alongside the SHA. This converts the mutable reference into an immutable one consistent with the PMD pin just added. See https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions |
| 16 | + |
| 17 | +3. [ ] **cloud-infrastructure-reviewer** | `.github/workflows/verify-build.yml:509` | iac-anti-pattern |
| 18 | + [SEC-04 Supply-Chain Integrity] The macOS curl tarball (`curl-7.75.0.tar.gz`) is downloaded from an S3 bucket (`libhttpserver.s3.amazonaws.com`) over HTTPS but is NOT verified against a pinned sha256 digest before it is compiled and installed. Lines 514-515 only log the observed hash; they do not fail the step on mismatch. A compromised or tampered S3 object would be installed silently. This is a pre-existing gap that TASK-059's PMD work did not address, leaving an inconsistent trust model in the same workflow. |
| 19 | + *Recommendation:* Replace the `echo` log line with a hard assertion identical to the PMD and libmicrohttpd patterns already used elsewhere in this file: `echo "<expected-sha256> curl-7.75.0.tar.gz" | shasum -a 256 -c`. Record the expected digest by downloading the file once from a trusted context, running `shasum -a 256`, and committing the result. Also add `-f` / `--fail` to the bare `curl` invocation on line 509 (it currently lacks these flags, unlike the PMD step). See https://docs.github.com/en/actions/security-guides/security-hardening-for-github-actions#using-third-party-actions |
| 20 | + |
| 21 | +## Minor |
| 22 | + |
| 23 | +4. [ ] **code-quality-reviewer** | `.github/workflows/verify-build.yml:439` | test-coverage |
| 24 | + The acceptance criteria state integrity was 'proven locally' by a wrong-hash rehearsal but there is no automated test or CI artifact that captures this proof. Future reviewers have no way to verify the claim after the fact. |
| 25 | + *Recommendation:* Consider adding a comment or a separate CI job step (even a dry-run with a known-bad hash in a workflow_dispatch path) to make the tamper-detection property automatically verifiable, or at minimum note the exact rehearsal commands used in the commit message for traceability. |
| 26 | + |
| 27 | +5. [ ] **code-quality-reviewer** | `.github/workflows/verify-build.yml:451` | error-handling |
| 28 | + The sha256 digest string (line 451) has 63 hex characters; a valid SHA-256 digest is 64 hex characters. If this is a transcription error the hash will always mismatch and the lint lane will go red. If it is intentional (truncated for brevity) it weakens the integrity guarantee. |
| 29 | + *Recommendation:* Verify the exact digest of pmd-dist-7.24.0-bin.zip from the PMD GitHub release page and replace the value with the full 64-character hex string. |
| 30 | + |
| 31 | +6. [ ] **code-simplifier** | `.github/workflows/verify-build.yml:444` | naming |
| 32 | + The comment says 'Obtain the new digest from the GitHub release page's pmd-dist-<ver>-bin.zip asset (or `sha256sum` the freshly downloaded zip and commit the resulting value here).' The phrase 'commit the resulting value here' is slightly ambiguous — it could mean a git commit or the action of writing the value. Consider rewording to 'record the resulting value in PMD_SHA256 in this file' for precision. |
| 33 | + *Recommendation:* Replace: `the resulting value here` with `the resulting value in PMD_SHA256 in this file`. This is a documentation-only change with no functional impact. |
| 34 | + |
| 35 | +7. [ ] **code-simplifier** | `.github/workflows/verify-build.yml:448` | code-structure |
| 36 | + The rotation comment's last two sentences form a single run-on that conflates two distinct verification paths (sha256 sidecar vs Maven Central attestation) in one conditional clause, making it harder to scan at a glance. |
| 37 | + *Recommendation:* Split into two explicit bullet-style sentences: 'Cross-check against the pmd-dist-<ver>-bin.zip.sha256 sidecar on the same release page. Alternatively, verify the Maven Central / OSSRH attestation for the same version if the GitHub release page itself may be compromised.' This keeps both verification paths visible without the nested parenthetical. |
| 38 | + |
| 39 | +8. [ ] **code-simplifier** | `.github/workflows/verify-build.yml:449` | code-structure |
| 40 | + The run block does not set `set -euo pipefail`. In a GitHub Actions `run: |` block the shell defaults to `bash -e`, which catches simple command failures but not pipeline failures. The `curl ... | sha256sum` pattern is not present here, but a future maintainer could introduce one; and the curl itself would benefit from the `pipefail` guarantee being explicit. More concretely: if `curl` silently truncates the download (e.g. network interruption after exit 0), sha256sum would still catch it, but the intent is unclear without explicit fail-fast settings. |
| 41 | + *Recommendation:* Add `set -euo pipefail` as the first line of the run block, which is the idiomatic GitHub Actions shell script header for safe scripting: |
| 42 | +```yaml |
| 43 | + run: | |
| 44 | + set -euo pipefail |
| 45 | + PMD_VERSION=7.24.0 |
| 46 | + ... |
| 47 | +``` |
| 48 | +This is a low-risk, one-line change that makes the safety posture explicit. |
| 49 | +
|
| 50 | +9. [ ] **code-simplifier** | `.github/workflows/verify-build.yml:454` | code-structure |
| 51 | + The sha256sum invocation uses a trailing bare `-` to read from stdin (`sha256sum -c -`). The `-` is redundant here: `sha256sum -c` reads from stdin by default when piped. While technically harmless and portable, the extra `-` adds a small amount of visual noise for readers unfamiliar with the convention. |
| 52 | + *Recommendation:* Drop the trailing `-`: `echo "${PMD_SHA256} /tmp/pmd.zip" | sha256sum -c`. Both forms are POSIX-correct; the shorter form is cleaner and matches common usage in other CI scripts. |
| 53 | + |
| 54 | +10. [ ] **code-simplifier** | `specs/unworked_review_issues/2026-06-02_113040_task-059.md:6` | naming |
| 55 | + The header line 'Total: 3 major (all out of scope for TASK-059)' is accurate and concise — no issue. The file overall is appropriately lean: three deferred findings plus two checked-off in-scope fixes, each with enough detail to act on. No simplification needed. |
| 56 | + *Recommendation:* No change required. |
| 57 | + |
| 58 | +11. [ ] **housekeeper** | `specs/unworked_review_issues/2026-06-02_113040_task-059.md:1` | documentation-stale |
| 59 | + The new unworked entry header uses 'Source: cloud-infrastructure-reviewer iter1' which is slightly inconsistent with the existing convention in 2026-05-21_121150_manual-validation.md, where the source/reviewer name appears inline per finding (e.g. '**code-quality-reviewer**') rather than as a top-level header field. The 'Total' field also introduces a parenthetical '(all out of scope for TASK-059)' that does not appear in the reference entry. These are cosmetic deviations — the entry is fully readable and the intent is clear. |
| 60 | + *Recommendation:* No action required; the entry is functional and the added context aids future readers. Consider aligning the header format with the existing convention in a future cleanup pass if strict uniformity is desired. |
| 61 | + |
| 62 | +12. [ ] **security-reviewer** | `.github/workflows/verify-build.yml:483` | supply-chain |
| 63 | + IWYU clone (pre-existing) tracks a mutable branch reference (`--branch clang_18`) rather than a pinned commit SHA. A force-push to the upstream branch could silently substitute a different revision. This is a pre-existing issue with a TODO comment and was not introduced by this PR; it is noted for completeness. |
| 64 | + *Recommendation:* Replace the branch reference with an immutable commit SHA: `git clone --depth 1 https://... ; git checkout <sha>` and verify with `git rev-parse HEAD`. |
| 65 | + |
| 66 | +13. [ ] **security-reviewer** | `.github/workflows/verify-build.yml:509` | supply-chain |
| 67 | + macOS curl-7.75.0.tar.gz download (pre-existing) still logs the SHA256 instead of asserting it. A tampered tarball on the S3 bucket would not be caught. This is a pre-existing issue with a TODO comment and was not introduced by this PR; it is noted for completeness. |
| 68 | + *Recommendation:* Replace the sha256 logging line with a hard assertion: `echo "<expected-sha256> curl-7.75.0.tar.gz" | shasum -a 256 -c`. Obtain the expected digest from a known-good download. |
| 69 | + |
| 70 | +14. [ ] **spec-alignment-checker** | `/Users/etr/progs/libhttpserver/.worktrees/TASK-059/.github/workflows/verify-build.yml:475` | specification-gap |
| 71 | + Two unrelated TODO(security) comments remain in the same workflow file (lines 475 and 510) — one for IWYU (pin to immutable commit SHA) and one for CURL (pin to known-good SHA-256). These are out of scope for TASK-059, which targets only the PMD step. However, the task spec text says 'A TODO comment was added in a prior commit but the actual sha256sum check is still missing', implying the original task author believed a TODO existed on the PMD step. Inspection of feature/v2.0 confirms no such TODO was on the PMD step; the description was inaccurate. The implementation correctly treated the 'Remove TODO' item as vacuously satisfied. |
| 72 | + *Recommendation:* Consider filing follow-up tasks for the IWYU and CURL TODO(security) comments (lines 475 and 510) to maintain consistency with the PMD fix. Update the task spec's Goal paragraph to correct the stale claim about a TODO comment on the PMD step. |
| 73 | + |
| 74 | +15. [ ] **test-quality-reviewer** | `.github/workflows/verify-build.yml:454` | missing-test |
| 75 | + The hash-mismatch failure path is verified only by a one-time local rehearsal, not by a repeatable automated test. If the sha256sum line is accidentally deleted or the variable reference is broken in a future edit, no CI gate would catch the regression until a human notices. |
| 76 | + *Recommendation:* Consider adding a small dedicated CI job (e.g. a separate workflow or a matrix entry) that intentionally supplies a wrong hash and asserts the step exits non-zero. This could be a single 5-line shell script job. Given the 'S' task scope, deferring this to a future hardening task is acceptable, but it should be tracked as a known gap. |
0 commit comments