Skip to content

Commit ecf7a0e

Browse files
etrclaude
andcommitted
unworked validation sweep: close task-053 + task-054 minor findings
TASK-053 (lookup_v2 dispatch cutover): - Merge the two split `namespace detail` blocks in webserver_dispatch.cpp into one continuous block with a section banner separator. - Replace bare __builtin_unreachable() in webserver_routes.cpp with assert(!"unreachable: ...") + __builtin_unreachable() so debug builds crash with a clear diagnostic; drop the dead trailing break. - Brace-initialize cache_value at the lookup_v2 insert site; condense the verbose move-vs-copy comment to a one-liner. - Trim invalidate_route_cache's comment — no more references to the removed v1 dispatch-cache field names. - Add an Invariant 5 contract test, unregistered_path_returns_404, to close the miss-path safety net in v2_dispatch_contract_test. - Correct the bench_route_lookup.cpp radix-tier comment: the 16 paths fit in the 256-entry LRU after warmup, so the bench measures a mix of cache-warm + radix latency, not pure radix. - Rewrite REGRESSION.md §"Why two surfaces" and §3 (custom-regex constraints) to reflect the post-cutover reality. - 05-cross-cutting.md §5.1 internal-locks table: route_table_mutex → route_table_mutex_; route_cache_mutex entry replaced with a note about the LRU mutex being owned by detail::route_cache. - v2-deferred-backlog-plan.md summary table now carries a Status column with TASK-053..059 marked Done. TASK-054 (auth_handler_ptr → optional<http_response>): - Gate the std::string path(...) allocation in the auth before_handler alias behind auth_skip_paths_normalized.empty() so production servers with no skip paths pay zero allocation per authenticated request. - Rename install_log_access_alias_ → install_log_access_alias (no trailing underscore on file-scope free functions in anonymous namespace; matches the codebase convention the comment cites). - Tidy the auth hook: drop the explicit type annotation, rename the local to `rejection`, switch to idiomatic `if (!rejection)`. - Simplify adapt_legacy_auth's lambda return to `std::move(*ptr)` (implicit conversion to optional handles the wrap). - Add paired PORT_N / PORT_N_STRING macros in auth_handler_legacy_shim_test.cpp and replace the two hardcoded localhost:8296 / 8297 strings (matches the iter1 fix for the optional-signature TU). - Simplify std::optional<http_response>(...) → direct returns in the example, the integration test, and the two unit-test sites. - Add a comment to the centralized auth example noting that production should read AUTH_USER/AUTH_PASS once at startup (per-request getenv is not thread-safe vs concurrent setenv). - create-webserver.md and RELEASE_NOTES.md now name v2.1 as the concrete removal target for the compat::auth_handler_v1_ptr alias. - v2-deferred-backlog-plan.md acceptance criteria rewritten so the grep AC explicitly excludes the intentional compat shim, and the heap-allocation criterion documents the by-inspection verification (citing webserver_aliases.cpp:221). TASK-055 (carry-over): - Add expose_exception_messages note to the create-webserver component doc to round out the security-opt-in documentation alongside expose_credentials_in_logs. Compacted both task-053 and task-054 unworked-review files: closed items collapsed into a single-line table with disposition; substantive deferred items grouped into named clusters for follow-up. Tested - libhttpserver.la rebuilds clean. - v2_dispatch_contract: 5 tests / 15 checks pass (was 4 / 12). - auth_handler_optional_signature: 9 successes. - auth_handler_legacy_shim: 5 successes. - routing_regression: clean. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent f252f55 commit ecf7a0e

17 files changed

Lines changed: 242 additions & 453 deletions

RELEASE_NOTES.md

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -178,9 +178,10 @@ and see the v2 replacement.
178178
(earlier v2 work-in-progress shipped
179179
`std::function<std::shared_ptr<http_response>(const http_request&)>`).
180180
Return `std::nullopt` to allow the request; return an `http_response`
181-
to reject. The v1 `shared_ptr` shape still compiles for one
182-
transitional build via `httpserver::compat::auth_handler_v1_ptr` and a
183-
`[[deprecated]]` setter overload; both emit a deprecation warning.
181+
to reject. The v1 `shared_ptr` shape still compiles via
182+
`httpserver::compat::auth_handler_v1_ptr` and a `[[deprecated]]`
183+
setter overload (both emit a deprecation warning); the compat alias
184+
and overload are scheduled for removal in v2.1.
184185
Removes one heap allocation per authenticated request.
185186
- **`http_request` getters return `const&` / `string_view`.** v1's
186187
`get_header(name)` (and `get_arg`, `get_cookie`, `get_footer`) returned

examples/centralized_authentication.cpp

Lines changed: 9 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -48,22 +48,24 @@
4848
#include <httpserver.hpp>
4949

5050
// Returns std::nullopt to allow the request, or an engaged optional
51-
// carrying an http_response to reject it (TASK-054).
51+
// carrying an http_response to reject it (TASK-054). NOTE: a production
52+
// deployment should read AUTH_USER and AUTH_PASS once at startup and
53+
// stash them in immutable storage; calling std::getenv on every request
54+
// is not thread-safe with respect to concurrent setenv/putenv from
55+
// other threads in the same process.
5256
std::optional<httpserver::http_response> auth_handler(
5357
const httpserver::http_request& req) {
5458
const char* expected_user = std::getenv("AUTH_USER");
5559
const char* expected_pass = std::getenv("AUTH_PASS");
5660
if (!expected_user || !expected_pass) {
5761
std::cerr << "centralized_authentication: AUTH_USER and AUTH_PASS "
5862
"environment variables must be set.\n";
59-
return std::optional<httpserver::http_response>(
60-
httpserver::http_response::string("Server configuration error")
61-
.with_status(500));
63+
return httpserver::http_response::string("Server configuration error")
64+
.with_status(500);
6265
}
6366
if (req.get_user() != expected_user || req.get_pass() != expected_pass) {
64-
return std::optional<httpserver::http_response>(
65-
httpserver::http_response::unauthorized(
66-
"Basic", "MyRealm", "Unauthorized"));
67+
return httpserver::http_response::unauthorized(
68+
"Basic", "MyRealm", "Unauthorized");
6769
}
6870
return std::nullopt;
6971
}

specs/architecture/04-components/create-webserver.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -17,8 +17,8 @@ shape remains available for one transitional build via the
1717
`[[deprecated]]` setter overload (`auth_handler(compat::auth_handler_v1_ptr)`),
1818
both of which wrap the legacy callable via `compat::adapt_legacy_auth`
1919
into the canonical `auth_handler_ptr` shape and emit a deprecation
20-
diagnostic at the call site. The compat alias and overload will be
21-
removed after the next release. Rationale: completes the
20+
diagnostic at the call site. The compat alias and overload are scheduled
21+
for removal in v2.1. Rationale: completes the
2222
PRD-RSP-REQ-007 value-typed response cutover (DR-009) onto the auth
2323
path, removing the per-authenticated-request control-block allocation.
2424

specs/architecture/05-cross-cutting.md

Lines changed: 2 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,8 +9,8 @@
99
4. `http_response` is value-typed with exclusive ownership. Returning it transfers it.
1010

1111
**Internal locks:**
12-
- `route_table_mutex` (`std::shared_mutex`) — registration vs lookup.
13-
- `route_cache_mutex` (`std::mutex`) — LRU cache promotion.
12+
- `route_table_mutex_` (`std::shared_mutex` on `webserver_impl`) — registration vs lookup.
13+
- `route_lru_cache`'s internal `std::mutex` (owned by `detail::route_cache`; not a named top-level field on `webserver_impl`) — LRU promotion.
1414
- `bans_mutex` (`std::shared_mutex`) — block list.
1515
- `mutexwait` / `mutexcond` (`pthread_mutex_t` / `pthread_cond_t`) — start/stop handshake (kept as POSIX primitives because MHD's start path expects them).
1616

specs/tasks/v2-deferred-backlog-plan.md

Lines changed: 19 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -21,15 +21,15 @@ can be split out into individual task files when work starts.
2121

2222
## Summary
2323

24-
| Task ID (proposed) | Name | Severity | Milestone | Estimate | GA-blocker? |
25-
|---|---|---|---|---|---|
26-
| TASK-053 | Wire `lookup_v2()` into dispatch hot path | Major | M5 | L | **Yes** — TASK-027 work is dead code today |
27-
| TASK-054 | Migrate `auth_handler_ptr` to `optional<http_response>` | Major | M4/M5 | M | **Yes** — last `shared_ptr<http_response>` on public API |
28-
| TASK-055 | DR-009 revision: default error body must not surface `e.what()` | Major | M6 | M | **Yes** — CWE-209 information disclosure |
29-
| TASK-056 | Hash-DoS hardening + prefix-route disambiguation in radix tree | Major | M5 | M | **Yes** — security hardening |
30-
| TASK-057 | Redact credentials in `http_request::operator<<` | Minor (sec) | M3 | S | **Yes** — A09:2021 logging failure |
31-
| TASK-058 | Hot-path allocation pass: canonicalize/normalize/serialize_allow_methods | Minor | post-v2.0 | L | No — perf polish |
32-
| TASK-059 | Supply-chain: sha256-pin PMD analyzer download in CI | Minor (sec) | M6 | S | Yes — quick win |
24+
| Task ID (proposed) | Name | Severity | Milestone | Estimate | GA-blocker? | Status |
25+
|---|---|---|---|---|---|---|
26+
| TASK-053 | Wire `lookup_v2()` into dispatch hot path | Major | M5 | L | **Yes** — TASK-027 work is dead code today | Done |
27+
| TASK-054 | Migrate `auth_handler_ptr` to `optional<http_response>` | Major | M4/M5 | M | **Yes** — last `shared_ptr<http_response>` on public API | Done |
28+
| TASK-055 | DR-009 revision: default error body must not surface `e.what()` | Major | M6 | M | **Yes** — CWE-209 information disclosure | Done |
29+
| TASK-056 | Hash-DoS hardening + prefix-route disambiguation in radix tree | Major | M5 | M | **Yes** — security hardening | Done |
30+
| TASK-057 | Redact credentials in `http_request::operator<<` | Minor (sec) | M3 | S | **Yes** — A09:2021 logging failure | Done |
31+
| TASK-058 | Hot-path allocation pass: canonicalize/normalize/serialize_allow_methods | Minor | post-v2.0 | L | No — perf polish | Done |
32+
| TASK-059 | Supply-chain: sha256-pin PMD analyzer download in CI | Minor (sec) | M6 | S | Yes — quick win | Done |
3333

3434
GA-blockers (six of seven) should land before v2.0 cuts a release tag.
3535
TASK-058 is a post-v2.0 polish but the prep work (string_view return type
@@ -154,14 +154,19 @@ TODO comment marking this migration already lives at
154154
- Blocks: None
155155

156156
**Acceptance Criteria:**
157-
- `grep -rE 'shared_ptr<.*http_response' src/httpserver/` returns no
158-
results in public headers.
157+
- `grep -rE 'shared_ptr<.*http_response' src/httpserver/ | grep -v compat`
158+
returns no results in public headers. (The deprecated v1 shim in
159+
`httpserver::compat` is intentionally retained for one transitional
160+
build per Action Item 1.)
159161
- `examples/centralized_authentication.cpp` and the auth integration
160162
test compile against the new signature.
161163
- The v1 shim adapter compiles with `-Werror=deprecated-declarations`
162-
off, and emits a deprecation warning when on.
163-
- One heap allocation removed per authenticated request (verifiable via
164-
`bench_auth.cpp` or by inspection).
164+
off, and emits a deprecation warning when on. The compat alias and
165+
setter overload are scheduled for removal in v2.1.
166+
- One heap allocation removed per authenticated request — verified by
167+
inspection: the `std::optional<http_response>` return path eliminates
168+
the `std::shared_ptr<http_response>` control-block allocation that the
169+
v1 shape required. See `src/detail/webserver_aliases.cpp:221`.
165170

166171
**Related Findings:** task-036 #38, task-030 #18
167172
**Related Decisions:** DR-009, PRD-RSP-REQ-007

0 commit comments

Comments
 (0)