Skip to content

Commit f252f55

Browse files
etrclaude
andcommitted
unworked validation sweep: close task-055 + task-056 minor findings
TASK-055 (DR-009 Revision 1 — default 500 body sanitization): - Document expose_exception_messages on the create-webserver and webserver component specs (§4.9, §4.1) mirroring the shape used for expose_credentials_in_logs. - Clarify constants.hpp: GENERIC_ERROR is v1-API-parity only; INTERNAL_SERVER_ERROR is the live dispatch constant. - Update the internal_error_page declaration comment (Doxygen) to describe the post-revision dual-branch body (fixed string by default; e.what() only when expose_exception_messages is set). - Extract the 500 status into a local in webserver_error_pages.cpp so both return paths share one binding for the must-not-diverge dimension. - Replace the README examples that echoed `what` verbatim to the HTTP client (CWE-209 anti-pattern); show the safe pattern with an internal log call placeholder. - Refresh stale test-name references in basic.cpp comment blocks (post-TASK-055 rename: dr009_default_body_is_fixed_string). - Remove redundant unknown-exception substring check from dr009_default_body_is_fixed_string_for_non_std_exception (the preceding LT_CHECK_EQ on the full body already covers it). TASK-056 (hash-DoS hardening + prefix collision): - Correct the has_terminus_at comment in radix_tree.hpp: it DOES descend the wildcard child when the pattern segment is wildcard-shaped (same shape rule as remove()). - Correct the tokenize() comment: tokenize_url takes a const std::string&, not by-value. - Add a /EXA/ path-normalisation comment in basic.cpp's family_endpoints test so the trailing-slash hit is not misread as prefix fall-through. - Trim the verbose zero-floor guard comment in threadsafety_stress.cpp to a single sentence; the full rationale is in the test-level block. - Drop the std::string(...) wrappers in the collision error-message concatenation in webserver_routes.cpp; operator+ accepts const char* directly. - Update the route-table.md sentence to drop the stale "to be enforced once TASK-053 lands" qualifier — bench_route_lookup exists now. - Rewrite TASK-056 action item 2 in v2-deferred-backlog-plan.md to reference reject_terminus_collision() at the actual call sites. Compacted the unworked-review files for both tasks: closed items collapsed into a single-line table with disposition; substantive deferred items kept in full as a follow-up backlog. Tested - libhttpserver.la rebuilds clean. - routing_regression: 26 tests / 99 checks pass. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent d4ad72d commit f252f55

14 files changed

Lines changed: 157 additions & 323 deletions

README.md

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -664,8 +664,11 @@ auto cfg = httpserver::create_webserver(8080)
664664
return httpserver::http_response::empty().with_status(405);
665665
})
666666
.internal_error_handler([](const httpserver::http_request&, std::string_view what) {
667-
// In production, log 'what' internally and return a generic message to the client.
668-
return httpserver::http_response::string(std::string{what}).with_status(500);
667+
// CWE-209: 'what' may contain file paths, SQL fragments, or
668+
// attacker-influenced input. Log it internally; do NOT echo
669+
// it to the HTTP client.
670+
(void)what; // e.g. logger->error(what);
671+
return httpserver::http_response::string("Internal Server Error").with_status(500);
669672
});
670673
httpserver::webserver ws{cfg};
671674
```
@@ -1715,8 +1718,11 @@ auto cfg = httpserver::create_webserver(8080)
17151718
return httpserver::http_response::empty().with_status(405);
17161719
})
17171720
.internal_error_handler([](const httpserver::http_request&, std::string_view what) {
1718-
// In production, log 'what' internally and return a generic message to the client.
1719-
return httpserver::http_response::string(std::string{what}).with_status(500);
1721+
// CWE-209: 'what' may contain file paths, SQL fragments, or
1722+
// attacker-influenced input. Log it internally; do NOT echo
1723+
// it to the HTTP client.
1724+
(void)what; // e.g. logger->error(what);
1725+
return httpserver::http_response::string("Internal Server Error").with_status(500);
17201726
});
17211727
httpserver::webserver ws{cfg};
17221728
```

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

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,14 @@ removed after the next release. 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

25+
**`expose_exception_messages` (TASK-055 / DR-009 Revision 1).**
26+
Security-opt-in builder setter. Default `false` makes the
27+
no-handler-set 500 path return the fixed string `"Internal Server Error"`
28+
as the response body — `e.what()` is logged via `log_error` but never
29+
crosses a process boundary on the wire (CWE-209). Setting `true`
30+
restores the v1 verbose body that surfaces the message. Intended
31+
for development only; must not be set in production.
32+
2533
**`expose_credentials_in_logs` (TASK-057).** Security-opt-in builder
2634
setter that propagates to every `http_request` the webserver dispatches.
2735
Default `false` suppresses credential surfaces in `http_request::operator<<`

specs/architecture/04-components/route-table.md

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -21,7 +21,7 @@ A `route_entry` carries:
2121

2222
**Prefix-vs-exact collision detection:** registering an exact route at a path that already has a prefix terminus (or vice versa) throws `std::invalid_argument` at registration time rather than silently double-registering. The cache key `(method, path)` cannot distinguish the two kinds at lookup time, so the conflict is rejected at the source. The guard probes both storage locations (`exact_routes_` and the radix tree's `exact_terminus_` / `prefix_terminus_`) before any mutation, so the atomicity contract — "a failed registration leaves the table exactly as it was" — still holds.
2323

24-
**Hash-flooding immunity (CWE-407):** the radix tree's per-segment children are kept in `std::map` rather than `std::unordered_map`. URL path segments are attacker-controlled, and neither libc++ nor libstdc++ seed `std::hash<std::string>` by default — a crafted sibling-key corpus can degrade `std::unordered_map::find` from O(1) amortized to O(n) per probe, opening an algorithmic-complexity DoS vector. The `std::map` (red-black tree) gives O(log n) worst case with no hashing in the loop. The transparent comparator `std::less<>` lets the hot-path lookup pass `std::string_view` keys directly without constructing a temporary `std::string` per probe. Typical URL trees branch shallowly (< 10 children per node), so the constant-factor difference from hashing is dominated by the per-segment string compare either way; the cache-miss radix path stays well under the 5 µs ceiling (to be enforced by `test/bench_route_lookup.cpp` once TASK-053 lands and wires `lookup_v2` into dispatch).
24+
**Hash-flooding immunity (CWE-407):** the radix tree's per-segment children are kept in `std::map` rather than `std::unordered_map`. URL path segments are attacker-controlled, and neither libc++ nor libstdc++ seed `std::hash<std::string>` by default — a crafted sibling-key corpus can degrade `std::unordered_map::find` from O(1) amortized to O(n) per probe, opening an algorithmic-complexity DoS vector. The `std::map` (red-black tree) gives O(log n) worst case with no hashing in the loop. The transparent comparator `std::less<>` lets the hot-path lookup pass `std::string_view` keys directly without constructing a temporary `std::string` per probe. Typical URL trees branch shallowly (< 10 children per node), so the constant-factor difference from hashing is dominated by the per-segment string compare either way; the cache-miss radix path stays well under the 5 µs ceiling enforced by `test/bench_route_lookup.cpp`.
2525

2626
**Future evolution:** if `std::map` probe cost dominates measured lookup time at high fanout, switching to an in-tree small-vector flat_map remains an internal-only optimization. v2.0 commits only to the *outer shape* (three-tier with cache), not the per-node container choice.
2727

specs/architecture/04-components/webserver.md

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@
2121
- Route registration grabs a writer lock; route lookup grabs a reader lock. The LRU cache (256 entries) is checked before the locks on the lookup path.
2222
- `~webserver()` joins MHD's internal threads before returning. Users who call `stop()` themselves still receive the same join behavior on destruction.
2323
- The constructor `webserver(const create_webserver&)` is `explicit` (PRD-NAM-REQ-004).
24+
- Default 500 body (no `internal_error_handler` configured) is the fixed string `"Internal Server Error"` (DR-009 Revision 1 / CWE-209). The originating `e.what()` is still surfaced via the `log_error` callback. The verbose body is opt-in via `create_webserver::expose_exception_messages(true)`. See §5.2 for the full contract.
2425

2526
**Related requirements:** PRD-HDR-REQ-001..004, PRD-FLG-REQ-001..005, PRD-CFG-REQ-001..004, PRD-HDL-REQ-001..006, PRD-NAM-REQ-001..005.
2627

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

Lines changed: 4 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -251,9 +251,10 @@ TASK-027 cleanup:
251251
flat_map) in the radix node child container. Benchmark the impact:
252252
acceptable if cache-miss path stays under 5 µs on the worst-case
253253
realistic tree depth.
254-
- [x] Add the `is_prefix_match` guard at the call site of
255-
`upsert_v2_param_route` so prefix-vs-exact terminus collisions are
256-
detected at registration time, not at lookup time.
254+
- [x] Add the prefix-vs-exact terminus collision guard at the
255+
registration call sites. Landed as `webserver_impl::reject_terminus_collision()`
256+
invoked from `register_v2_route()` and `upsert_v2_radix_route()` so
257+
collisions are detected at registration time, not at lookup time.
257258
- [x] Add a regression test
258259
`routing_regression_test.cpp::register_exact_after_prefix_does_not_collide`
259260
pinning the registration-time error.

0 commit comments

Comments
 (0)