Skip to content

Commit 9c2d2f1

Browse files
etrclaude
andcommitted
specs: M7 v2-cleanup milestone + audit (TASK-060..093)
Convert each issue surfaced in specs/tasks/v2-branch-gap-audit.md into a workable task under specs/tasks/M7-v2-cleanup/, mirroring the M1..M6 format so groundwork's plan / implement / validate pipeline can drive them. Also tracks the audit document itself. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent c26c373 commit 9c2d2f1

36 files changed

Lines changed: 1552 additions & 0 deletions
Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
### TASK-060: Scope or remove file-scoped `-Warray-bounds` suppressions
2+
3+
**Milestone:** M7 - v2 Cleanup
4+
**Component:** `src/http_utils.cpp`, `src/detail/ip_representation.cpp`
5+
**Estimate:** S
6+
7+
**Goal:**
8+
Eliminate the two unscoped `#pragma GCC diagnostic ignored "-Warray-bounds"` directives flagged as the worst-shaped suppressions in `specs/tasks/v2-branch-gap-audit.md` §1 "HIGH — Unscoped warning suppressions". Either localize each to the minimum offending line range with a `push`/`pop` pair plus a comment explaining the underlying false-positive, or investigate and remove the suppression entirely once the warning is silenced at the source.
9+
10+
**Action Items:**
11+
- [ ] Reproduce the `-Warray-bounds` warning at `src/http_utils.cpp:62` under the supported GCC versions used in CI (matrix lanes in `.github/workflows/verify-build.yml`). Capture the exact diagnostic text and offending construct in the commit message.
12+
- [ ] Reproduce the same at `src/detail/ip_representation.cpp:55`. Capture diagnostic text.
13+
- [ ] For each site: either (a) rewrite the source so the warning no longer fires and delete the pragma, or (b) replace the file-scoped pragma with a tightly scoped `#pragma GCC diagnostic push` / `#pragma GCC diagnostic ignored "-Warray-bounds"` / `#pragma GCC diagnostic pop` block around the minimum offending lines, with a comment naming the GCC version range and the upstream report (if any).
14+
- [ ] If the suppression is kept, add a `TODO(TASK-NNN-followup)` style comment only if a concrete follow-up exists; otherwise the scoping comment is sufficient.
15+
- [ ] Add a guard test or static-analysis spot-check that fails if either file regains a file-scoped suppression.
16+
17+
**Dependencies:**
18+
- Blocked by: None
19+
- Blocks: None
20+
21+
**Acceptance Criteria:**
22+
- `grep -nE '^#pragma GCC diagnostic ignored "-Warray-bounds"' src/http_utils.cpp src/detail/ip_representation.cpp` returns no matches at file scope (only inside `push`/`pop` blocks, if any).
23+
- The verify-build.yml GCC lanes still pass without `-Warray-bounds` warnings.
24+
- Each remaining suppression carries a comment naming the GCC version range and the reason.
25+
- Typecheck passes.
26+
- Tests pass.
27+
28+
**Related Requirements:** PRD §2 code quality NFR
29+
**Related Decisions:** None new
30+
31+
**Status:** Backlog
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
### TASK-061: Mechanical cleanup sweep — unfinished prose, orphan comments, stale doc references
2+
3+
**Milestone:** M7 - v2 Cleanup
4+
**Component:** Various (`src/`, `test/`, `scripts/`)
5+
**Estimate:** S
6+
7+
**Goal:**
8+
Clear the mechanical leftovers flagged in the audit's "Proposed disposition (next steps)" — short, low-risk edits that should land together so reviewers can scan one PR for stale prose drift.
9+
10+
**Action Items:**
11+
- [ ] Finish the truncated comment at `src/detail/webserver_register.cpp:344-349`. The TASK-029 block comment ends mid-sentence (`"…keeps working at the daemon level, but"` then closing brace). Either complete the sentence with the original intent (reconstruct from git history or surrounding code) or rewrite the comment to stand on its own.
12+
- [ ] Remove the two orphan comment fragments at `src/webserver.cpp:503-504` (leftover from removed logic).
13+
- [ ] Update the stale `XFAIL_TESTS` comment block at `test/Makefile.am:67-74` to reflect that `header_hygiene` was removed when TASK-020 landed (cross-referenced at lines 532-535 in the same file).
14+
- [ ] Drop the stale `RELEASE_NOTES.md) continue ;; # created by TASK-042, not yet present` line at `scripts/check-readme.sh:273` — TASK-042 has shipped.
15+
- [ ] Remove the decade-old `//TODO: personalized messages` at `test/littletest.hpp:21` only if we have a fork policy that lets us; otherwise leave (it's vendored).
16+
17+
**Dependencies:**
18+
- Blocked by: None
19+
- Blocks: None
20+
21+
**Acceptance Criteria:**
22+
- `git diff master..HEAD -- src/detail/webserver_register.cpp` shows a complete, well-formed comment.
23+
- `src/webserver.cpp:503-504` no longer contains orphan comment fragments.
24+
- `test/Makefile.am:67-74` no longer claims `header_hygiene` is in `XFAIL_TESTS`.
25+
- `scripts/check-readme.sh:273` no longer carries the "created by TASK-042, not yet present" line.
26+
- `make check` still green.
27+
- Typecheck passes.
28+
- Tests pass.
29+
30+
**Related Requirements:** PRD §2 code quality NFR
31+
**Related Decisions:** None new
32+
33+
**Status:** Backlog
Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
### TASK-062: RFC-7616-compliant Digest auth response factory
2+
3+
**Milestone:** M7 - v2 Cleanup
4+
**Component:** `http_response::unauthorized`, `webserver_impl::dauth`
5+
**Estimate:** L
6+
7+
**Goal:**
8+
Make `http_response::unauthorized(...)` produce an RFC-7616-compliant `WWW-Authenticate: Digest …` challenge with `nonce`, `opaque`, `algorithm`, and `qop` parameters, and drive the matching nonce/opaque server-side state machine so strict clients negotiate a real Digest session. The current implementation (`src/httpserver/http_response.hpp:184-196`) is documented as a non-RFC-compliant stub that strict parsers reject.
9+
10+
**Action Items:**
11+
- [ ] Audit current `http_response::unauthorized(...)` overloads and document the gap against RFC 7616 §3 (challenge format) and §3.4 (`Authorization` validation) in a short header comment.
12+
- [ ] Add a nonce/opaque generator to `webserver_impl` (CSPRNG-backed, with replay/expiry tracking). Reuse the existing `dauth` plumbing where possible.
13+
- [ ] Extend `http_response::unauthorized(...)` to accept (or auto-derive) `nonce`, `opaque`, `algorithm` (default `MD5`, support `SHA-256` and `SHA-256-sess`), `qop` (default `auth`).
14+
- [ ] Wire the dispatch path to validate incoming `Authorization: Digest …` against the issued nonce/opaque pair and route to `dauth` handlers.
15+
- [ ] Convert the six v2 digest placeholder integ tests (`test/integ/authentication.cpp:42-60, 245-613`) to drive the nonce/opaque state machine end-to-end. Coordinated with TASK-079 (test work).
16+
- [ ] Update Doxygen on `unauthorized(...)` to remove the "non-RFC-compliant stub" disclaimer and add RFC references.
17+
18+
**Dependencies:**
19+
- Blocked by: None
20+
- Blocks: TASK-079
21+
22+
**Acceptance Criteria:**
23+
- `curl --digest -u user:pass …` against an `unauthorized`-protected route negotiates successfully (no `400 Bad Request` from strict client parsers).
24+
- A new integ test pins the `WWW-Authenticate` challenge format against RFC 7616 §3.3 examples.
25+
- The six existing digest placeholder tests in `authentication.cpp` either drive real nonce/opaque flows (preferred) or are explicitly retired in favour of new tests.
26+
- libmicrohttpd's MD5/SHA-256 helpers remain the underlying primitive — we do not roll our own.
27+
- Typecheck passes.
28+
- Tests pass.
29+
30+
**Related Requirements:** PRD-RSP-REQ-005 (`unauthorized` factory completeness)
31+
**Related Decisions:** None new (RFC 7616)
32+
33+
**Status:** Backlog
Lines changed: 29 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,29 @@
1+
### TASK-063: Honor or remove `http_response::pipe` `size_hint` parameter
2+
3+
**Milestone:** M7 - v2 Cleanup
4+
**Component:** `http_response::pipe`
5+
**Estimate:** S
6+
7+
**Goal:**
8+
`pipe(int fd, std::size_t size_hint = 0)` exposes `size_hint` in the public API but the dispatch path ignores it; a test (`test/unit/http_response_factories_test.cpp:253-266`) pins "accepted-but-ignored" as the contract. Decide whether to honor the hint (sizing the streaming buffer, `Content-Length` synthesis, or kernel hint via `posix_fadvise`) or remove it from the signature.
9+
10+
**Action Items:**
11+
- [ ] Decide one of: (a) honor `size_hint` as the streaming chunk size and/or `Content-Length`, or (b) remove the parameter and the pinning test.
12+
- [ ] If (a): wire `size_hint` into `detail::body_pipe` (or wherever the pipe body is materialised) and add a `Content-Length` header when the hint is nonzero and finite. Add a real integration test that pins the byte-count emitted on the wire matches the hint when the underlying fd produces exactly that many bytes.
13+
- [ ] If (b): drop `size_hint` from the public `pipe(int fd)` signature in `src/httpserver/http_response.hpp:159-161`, drop the corresponding default-arg in `src/http_response.cpp:409`, and convert the "accepted-but-ignored" test in `test/unit/http_response_factories_test.cpp:253-266` into a compile-fail or call-site update.
14+
- [ ] Update RELEASE_NOTES.md "Migration notes" if the signature changed (binary/source incompatibility).
15+
16+
**Dependencies:**
17+
- Blocked by: None
18+
- Blocks: None
19+
20+
**Acceptance Criteria:**
21+
- `pipe(int fd, …)` has a documented, tested contract — either honors `size_hint` or does not expose it.
22+
- No accepted-but-ignored parameter survives on the public API.
23+
- Typecheck passes.
24+
- Tests pass.
25+
26+
**Related Requirements:** PRD-RSP-REQ-001 (factory by value), PRD §2 API minimalism
27+
**Related Decisions:** None new
28+
29+
**Status:** Backlog
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
### TASK-064: Structured cookie type
2+
3+
**Milestone:** M7 - v2 Cleanup
4+
**Component:** `http_response::with_cookie`, request cookie accessors
5+
**Estimate:** M
6+
7+
**Goal:**
8+
Replace the string-blob cookie surface on `http_response` with a structured `httpserver::cookie` value type carrying `name`, `value`, `domain`, `path`, `expires`, `max_age`, `secure`, `http_only`, `same_site`. The follow-up was explicitly deferred at `src/httpserver/http_response.hpp:304-313`.
9+
10+
**Action Items:**
11+
- [ ] Design the `httpserver::cookie` value type in a new public header `src/httpserver/cookie.hpp`. Default-construct empty; provide fluent `with_*` setters mirroring `http_response`'s style. Include enum `same_site_mode { unset, strict, lax, none }`.
12+
- [ ] Add `http_response::with_cookie(cookie)` and `http_response::with_cookie(std::string name, std::string value)` overloads. Internally render to the `Set-Cookie` header per RFC 6265 §4.1.
13+
- [ ] Provide `http_request::get_cookies()` returning a structured view (parsed once, cached on the request impl per TASK-016 arena pattern).
14+
- [ ] Document migration: legacy string-blob path remains as a `[[deprecated]]` thin shim for one transitional release.
15+
- [ ] Add a unit test pinning round-trip parsing/rendering against RFC 6265 examples.
16+
17+
**Dependencies:**
18+
- Blocked by: TASK-016 (arena), TASK-009 (http_response value type)
19+
- Blocks: None
20+
21+
**Acceptance Criteria:**
22+
- `http_response::ok().with_cookie(cookie{}.with_name("sid").with_secure(true).with_same_site(same_site_mode::strict))` compiles and emits a single, well-formed `Set-Cookie` header.
23+
- `http_request::get_cookies()` returns `const std::vector<cookie>&` (or `span<const cookie>`) for read-only access; never allocates after the first call.
24+
- RFC 6265 round-trip examples pass.
25+
- Deprecated string-blob path still compiles with a `[[deprecated]]` warning.
26+
- Typecheck passes.
27+
- Tests pass.
28+
29+
**Related Requirements:** PRD-RSP-REQ-004 (fluent return), PRD §2 API minimalism
30+
**Related Decisions:** None new (RFC 6265)
31+
32+
**Status:** Backlog
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
### TASK-065: RFC 5952 IPv6 zero-compression in `peer_address`
2+
3+
**Milestone:** M7 - v2 Cleanup
4+
**Component:** `src/peer_address.cpp`
5+
**Estimate:** S
6+
7+
**Goal:**
8+
`peer_address.cpp:49-50` skips RFC 5952 zero-compression (collapsing consecutive zero groups to `::`) so the textual IPv6 form we expose is non-canonical. Spec comment notes "TASK-046 can refine when telemetry firms up" — telemetry has firmed up; finish the canonicalization now.
9+
10+
**Action Items:**
11+
- [ ] Implement RFC 5952 §4 canonical form: lowercase, suppress leading zeros within groups, collapse the longest run of `2+` consecutive zero groups to `::` (ties broken by first occurrence), do not collapse a single zero group.
12+
- [ ] Reuse libc's `inet_ntop` where it already produces RFC 5952 output (glibc ≥ 2.28, musl, macOS recent) and only post-process when the platform falls short; or always post-process for determinism across platforms (preferred — document choice in commit message).
13+
- [ ] Add a unit test pinning the RFC 5952 §4.2.2 examples (`2001:db8::1`, `::1`, `::`, embedded IPv4 mappings).
14+
- [ ] Remove the "TASK-046 can refine when telemetry firms up" comment.
15+
16+
**Dependencies:**
17+
- Blocked by: None
18+
- Blocks: None
19+
20+
**Acceptance Criteria:**
21+
- `peer_address::to_string()` returns RFC 5952 canonical form for IPv6 inputs.
22+
- IPv4 path unchanged.
23+
- New unit test pins RFC 5952 §4.2.2 examples and the `::ffff:192.0.2.1` IPv4-mapped form.
24+
- Typecheck passes.
25+
- Tests pass.
26+
27+
**Related Requirements:** PRD §2 observability NFR
28+
**Related Decisions:** None new (RFC 5952)
29+
30+
**Status:** Backlog
Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
### TASK-066: Runtime setter for hook alias slots
2+
3+
**Milestone:** M7 - v2 Cleanup
4+
**Component:** `hook_handle`, `webserver_impl`
5+
**Estimate:** M
6+
7+
**Goal:**
8+
Five separate "if a future task adds a runtime setter for the alias slots, this MUST take the lock shared" comments cluster across `src/hook_handle.cpp:142, 449, 497` and `src/httpserver/detail/webserver_impl.hpp:336, 360`. All point at the same missing capability: runtime re-registration of internal alias slots (`log_access`, `not_found_handler`, `method_not_allowed_handler`, `internal_error_handler`, `auth_handler`). Land that capability or strike the speculation.
9+
10+
**Action Items:**
11+
- [ ] Decide: (a) add a `webserver::set_alias(hook_phase, alias_id, hook_action_fn)` runtime setter that re-points the relevant alias slot under the existing `shared_mutex`, with replacement semantics matching the construction-time alias (last-position, single-slot), or (b) remove the forward-debt comments and pin "aliases are immutable after `webserver::start()`" in the docs.
12+
- [ ] If (a): implement the setter, document it in the architecture doc (§4.10), add a hook-bus integration test covering concurrent alias replacement under load, and remove all five forward-debt comments.
13+
- [ ] If (b): remove the five comments and add a documentation note (in `hook_handle.hpp` and the §4.10 architecture page) that alias slots are construction-time-only.
14+
- [ ] Update `test/unit/hooks_log_access_alias_slot_test.cpp` to reflect the chosen semantics — coordinated with TASK-085.
15+
16+
**Dependencies:**
17+
- Blocked by: TASK-045 (hook bus skeleton, Done)
18+
- Blocks: TASK-085 (residual test smell on alias replacement)
19+
20+
**Acceptance Criteria:**
21+
- `grep -nE 'if a future task adds a runtime setter for the alias slots' src/` returns no matches.
22+
- The hook bus architecture doc (§4.10) explicitly states whether alias slots are mutable after start.
23+
- A test pins the chosen semantics (either runtime replacement, or rejection of replacement after start).
24+
- Typecheck passes.
25+
- Tests pass.
26+
27+
**Related Requirements:** PRD-HOOK-REQ-009 (v1 setters documented as aliases)
28+
**Related Decisions:** DR-012
29+
30+
**Status:** Backlog
Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
### TASK-067: Remove v1 `registered_resources*` maps and `namespace compat` shim
2+
3+
**Milestone:** M7 - v2 Cleanup
4+
**Component:** `webserver_impl`, `create_webserver.hpp`
5+
**Estimate:** L
6+
7+
**Goal:**
8+
Three transitional surfaces survive TASK-053's v1→v2 dispatch cutover:
9+
1. The v1 `registered_resources*` maps + mutex at `src/httpserver/detail/webserver_impl.hpp:152-164` (registration-time bookkeeping only, but still a divergence point).
10+
2. The `namespace compat` shim + `[[deprecated]]` overload + paired `-Wdeprecated-declarations` suppression at `src/httpserver/create_webserver.hpp:108-157, 549, 558-571` ("will be removed in the next release").
11+
3. The `prepare_or_create_lambda_shim` / WebSocket-path consumers that still read the v1 maps.
12+
13+
Cut them in a single, well-tested PR so the v2 dispatch path is the only one.
14+
15+
**Action Items:**
16+
- [ ] Identify every reader of `registered_resources*` (grep `src/`). For each: rewrite to consume the v2 route table or registration journal directly.
17+
- [ ] Delete `registered_resources*` maps + mutex from `webserver_impl`.
18+
- [ ] Delete `namespace compat` from `create_webserver.hpp`, the `[[deprecated]]` overload, and the paired `-Wdeprecated-declarations` push/pop.
19+
- [ ] Drop the same `-Wdeprecated-declarations` suppression in `src/http_resource.cpp:81-115` once the migration to `std::atomic<std::shared_ptr<T>>` (TASK-070) makes it unnecessary, or hand it off to TASK-070 explicitly if scope expands.
20+
- [ ] RELEASE_NOTES.md: document the binary/source incompatibility under "Removals".
21+
22+
**Dependencies:**
23+
- Blocked by: TASK-053 (Done)
24+
- Blocks: None
25+
26+
**Acceptance Criteria:**
27+
- `grep -nE 'registered_resources' src/httpserver/detail/webserver_impl.hpp` returns no matches.
28+
- `grep -nE 'namespace compat' src/httpserver/create_webserver.hpp` returns no matches.
29+
- `grep -nE 'Wdeprecated-declarations' src/httpserver/create_webserver.hpp` returns no matches.
30+
- All existing tests pass.
31+
- Typecheck passes.
32+
- Tests pass.
33+
34+
**Related Requirements:** PRD §2 API minimalism, PRD §1 release strategy
35+
**Related Decisions:** DR-007, DR-011
36+
37+
**Status:** Backlog
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
### TASK-068: `connection_state` hardening — CWE-226 arena overflow zeroing + CWE-14 `explicit_bzero`
2+
3+
**Milestone:** M7 - v2 Cleanup
4+
**Component:** `src/httpserver/detail/connection_state.hpp`
5+
**Estimate:** S
6+
7+
**Goal:**
8+
Two acknowledged residual gaps at `connection_state.hpp:122, 130`:
9+
1. CWE-226 — arena overflow bytes from the per-connection scratch region are not zeroed on connection reuse, so a later request can observe trailing bytes from a prior request's body.
10+
2. CWE-14 — the current clear path uses `memset(…, 0, …)`, which an optimizer may dead-code-eliminate. Replace with `explicit_bzero` (or a portable equivalent) so the zeroing is observable.
11+
12+
**Action Items:**
13+
- [ ] In `connection_state::reset()` (or whatever clears the arena between requests), zero the entire used-bytes prefix unconditionally. Document the trade-off (cycles vs. CWE-226 mitigation) in the header.
14+
- [ ] Replace the `memset` call with `explicit_bzero` on glibc/musl/macOS, `SecureZeroMemory` on Windows, or a hand-rolled `volatile`-pointer loop where neither is available. Centralize the helper in `src/httpserver/detail/secure_zero.hpp`.
15+
- [ ] Update the inline comments at lines 122 and 130 to reference the fix.
16+
- [ ] Add a unit test (compile-time + runtime) that pins the helper is not dead-code-eliminated under `-O2` (using a memory barrier and a `volatile` sink to observe the write).
17+
18+
**Dependencies:**
19+
- Blocked by: None
20+
- Blocks: None
21+
22+
**Acceptance Criteria:**
23+
- A request body containing sentinel bytes (e.g. `0xDEADBEEF` pattern) is not observable from a subsequent request on the same MHD connection, verified by an integ test.
24+
- The portable secure-zero helper compiles on every CI lane (Linux glibc, Linux musl, macOS, Windows).
25+
- ASan + valgrind continue to report clean.
26+
- Typecheck passes.
27+
- Tests pass.
28+
29+
**Related Requirements:** PRD §2 security NFR (CWE-226, CWE-14)
30+
**Related Decisions:** None new
31+
32+
**Status:** Backlog
Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
### TASK-069: Remove transitional two-arg `http_request_impl` constructor
2+
3+
**Milestone:** M7 - v2 Cleanup
4+
**Component:** `src/httpserver/detail/http_request_impl.hpp`
5+
**Estimate:** S
6+
7+
**Goal:**
8+
The two-arg `http_request_impl` constructor at `http_request_impl.hpp:95-99` is preserved "for source compatibility" with no removal date. Remove it now that the v2 cutover is complete, so the only constructor is the canonical one. Internal headers do not need a deprecation cycle.
9+
10+
**Action Items:**
11+
- [ ] Grep `src/` and `test/` for callers of the two-arg form. Migrate each to the canonical form.
12+
- [ ] Delete the two-arg overload and its comment block.
13+
- [ ] Verify `make check` is green across all build-flag matrix lanes (HAVE_BAUTH, HAVE_DAUTH, HAVE_GNUTLS, HAVE_WEBSOCKET).
14+
15+
**Dependencies:**
16+
- Blocked by: TASK-015 (request impl PIMPL split, Done)
17+
- Blocks: None
18+
19+
**Acceptance Criteria:**
20+
- The two-arg constructor and its forwarding/initialization no longer exist.
21+
- All callers use the canonical constructor.
22+
- Typecheck passes.
23+
- Tests pass.
24+
25+
**Related Requirements:** PRD §2 API minimalism
26+
**Related Decisions:** None new
27+
28+
**Status:** Backlog

0 commit comments

Comments
 (0)