TASK-055: capture unworked review issues report

17 minor items deferred from the multi-agent validation pass on
TASK-055 (sanitize default 500 body / DR-009 Revision 1). All
critical and major findings were addressed in the implementation
commit; remaining items are documentation/cosmetic polish that
fits a future hygiene sweep.

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

Author: etr

specs/unworked_review_issues/2026-05-29_183434_task-055.md

@@ -0,0 +1,75 @@
+# Unworked Review Issues
+
+**Run:** 2026-05-29 18:34:34
+**Task:** TASK-055
+**Total:** 17 (0 critical, 0 major, 17 minor)
+
+## Minor
+
+1. [ ] **architecture-alignment-checker** | `specs/architecture/04-components/create-webserver.md:1` | interface-contract
+   The create-webserver component spec does not document the new expose_exception_messages(bool) builder flag introduced by DR-009 Revision 1. The task verification criteria explicitly listed this component spec as a place that should reflect the new flag. The flag is well documented in the Doxygen, README, RELEASE_NOTES, and the DR-009 record itself, but the canonical component responsibility doc is silent on it.
+   *Recommendation:* Add a short note to the 'create_webserver' component spec (§4.9) listing expose_exception_messages(bool = true) alongside the other builder flags, cross-referencing DR-009 Revision 1 and the CWE-209 rationale. One or two sentences suffice — the full contract is already in DR-009.
+
+2. [ ] **architecture-alignment-checker** | `specs/architecture/04-components/webserver.md:1` | interface-contract
+   The webserver component spec (§4.1) does not reference the revised default error-body contract from DR-009 Revision 1. The webserver.hpp Doxygen block (the handler error-propagation contract section) is fully updated (point 2 now documents 'Internal Server Error' fixed body and the expose_exception_messages opt-in), but the component spec that is the structured reference for maintainers does not reflect this. The task criteria asked that webserver.md 'reflects the new error-body contract if applicable.'
+   *Recommendation:* Add a bullet or sentence to the webserver component spec's 'Key design notes' or 'Related requirements' section noting that the default 500 body is the fixed string 'Internal Server Error' (DR-009 Revision 1 / CWE-209) and that expose_exception_messages on the builder controls the opt-in verbose path.
+
+3. [ ] **code-quality-reviewer** | `src/httpserver/constants.hpp:62` | code-elegance
+   GENERIC_ERROR ('Internal Error') is now a dead constant in the error-page path. The implementation uses INTERNAL_SERVER_ERROR ('Internal Server Error') for the sanitized default body, and the msg-passthrough path constructs a string directly from msg. GENERIC_ERROR is retained for API-compat but its only production usage was in the old unsanitized internal_error_page path.
+   *Recommendation:* Add a comment to GENERIC_ERROR clarifying that it is retained for v1 API compatibility and is no longer used by the internal_error_page dispatch path (which now uses INTERNAL_SERVER_ERROR). This prevents future confusion about which constant governs the 500 default body.
+
+4. [ ] **code-quality-reviewer** | `src/httpserver/detail/webserver_impl_dispatch.hpp:63` | code-readability
+   The internal_error_page declaration comment says the no-handler path 'return[s] a default 500 whose body surfaces @p msg, so the unset-handler default is informative', which was true before this revision. Post-TASK-055, the default body is 'Internal Server Error' and @p msg is only surfaced when expose_exception_messages is true.
+   *Recommendation:* Update the bullet at lines 63-65 to reflect the new dual-branch behaviour: e.g., '- otherwise (no handler set, !force_our): return a default 500 whose body is the fixed string "Internal Server Error" unless expose_exception_messages is set, in which case @p msg is forwarded.'
+
+5. [ ] **code-quality-reviewer** | `test/integ/basic.cpp:2073` | code-readability
+   The comment at lines 2073-2074 references two old test names that were renamed/replaced by this task: 'dr009_runtime_error_message_surfaces_in_default_body' and 'dr009_non_std_exception_yields_unknown_exception_in_default_body'. These names no longer exist in the file; they were renamed to the 'fixed string' variants in TASK-055.
+   *Recommendation:* Update the comment to reference the current test names: 'dr009_default_body_is_fixed_string' and 'dr009_default_body_is_fixed_string_for_non_std_exception' (the verbose-opt-in counterparts are dr009_verbose_body_surfaces_message_when_opted_in and dr009_verbose_body_surfaces_unknown_exception_when_opted_in).
+
+6. [ ] **code-simplifier** | `src/detail/webserver_error_pages.cpp:95` | code-structure
+   The expose_exception_messages branch in internal_error_page duplicates the .with_status(http_utils::http_internal_server_error) call that also appears on the immediately following default-body path. The two return statements are three lines apart, so this is low risk, but the status code is the one dimension that must always be identical between the two branches.
+   *Recommendation:* Extract the status code into a local variable before the if-branch: `const auto status = http_utils::http_internal_server_error;` and use it in both return expressions, making it obvious that only the body text differs between the two paths.
+
+7. [ ] **code-simplifier** | `src/httpserver/constants.hpp:62` | naming
+   constants::GENERIC_ERROR ("Internal Error") is retained alongside the new constants::INTERNAL_SERVER_ERROR ("Internal Server Error") but is no longer used anywhere in the dispatch path. Its only references are in the constants unit-test fixture, which treats it as a v1 API-parity value. The two constants are confusingly similar in name and purpose, differing only in the body text they hold, making the file harder to reason about.
+   *Recommendation:* Add a comment to GENERIC_ERROR that explicitly marks it as a v1 API-parity constant kept for external consumer compatibility, and distinguishes it from INTERNAL_SERVER_ERROR which is the live dispatch constant. Alternatively, if no public consumer depends on the exact string "Internal Error", consider removing GENERIC_ERROR or aliasing it to INTERNAL_SERVER_ERROR in a follow-up task.
+
+8. [ ] **code-simplifier** | `test/integ/basic.cpp:3227` | code-structure
+   The commit removed the named constant PORT_DR009_BASE = 80 (which used to express that all DR-009 port offsets were relative to base 80), replacing it with direct PORT + 80 through PORT + 89 literals spread across eight test functions. The named constant was inlined into the comment block but not into the code. The magic numeric offsets (PORT + 80, PORT + 83, PORT + 87, PORT + 88, PORT + 89) repeat the comment's intent without a single source of truth, so a future renumbering requires editing both the comment table and each test body.
+   *Recommendation:* Restore small named offset constants for each test, e.g. `static constexpr int PORT_DR009_FIXED_BODY = PORT + 80;`, or keep PORT_DR009_BASE and express each test as `PORT_DR009_BASE + <relative_offset>`. Either approach keeps the port-allocation table and the actual port numbers in sync mechanically.
+
+9. [ ] **performance-reviewer** | `src/detail/webserver_error_pages.cpp:100` | memory-allocation
+   The sanitized default body path constructs `std::string{constants::INTERNAL_SERVER_ERROR}` by converting a compile-time `string_view` literal (21 bytes) into a heap-allocated std::string on every 500 response that reaches this branch. The identical pattern already existed for NOT_FOUND_ERROR (line 58) and METHOD_ERROR (line 66), so this is consistent with the existing convention, but it is a redundant heap allocation on each call. Under an attack that drives a high volume of 500s (e.g. a fuzz campaign against an endpoint that always throws) the allocator will see one extra ~32-byte allocation + deallocation per error response.
+   *Recommendation:* If `http_response::string` can accept a `std::string_view` overload (or a `std::string_view`-constructible body type), prefer that to avoid converting the compile-time literal to a heap string. If adding such an overload is out of scope for this task, the existing approach is acceptable — the 5xx path is cold relative to 2xx traffic and the allocation is bounded.
+
+10. [ ] **performance-reviewer** | `src/detail/webserver_error_pages.cpp:123` | memory-allocation
+   `log_dispatch_error` already existed, but the comment added in this diff clarifies that `msg` (a `std::string_view` over `e.what()`) is copied into a `std::string` at line 123 (`parent->log_error(std::string(msg))`) on every invocation. This is a pre-existing pattern (not introduced by TASK-055), but under attack-volume error floods the per-call heap allocation for the log string adds up. The `log_error` callback signature (`log_error_ptr`) accepts `std::string`; changing to `std::string_view` would require an API change.
+   *Recommendation:* This is a pre-existing allocation, not introduced by TASK-055. No action required for this task; flag for a future API cleanup pass if `log_error_ptr` can be widened to accept `std::string_view` without breaking callers.
+
+11. [ ] **security-reviewer** | `README.md:668` | insecure-design
+   Both README code examples (lines 668 and 1719) show `return httpserver::http_response::string(std::string{what}).with_status(500)` — i.e. echoing e.what() verbatim to the HTTP client through a custom internal_error_handler. The only guard is an inline comment `// In production, log 'what' internally and return a generic message to the client.` that directly contradicts the body of the lambda shown. A developer copy-pasting this example will ship a CWE-209 surface via the explicit-handler path, which is not protected by the new sanitization. The second occurrence at line 1719 has slightly more context in the surrounding prose (lines 1725-1731) but the code itself is identical and the comment is still misleading. The fix for the default path (this task) does not close this vector.
+   *Recommendation:* Replace the example lambda body with a safe pattern, e.g. `(void)what; return httpserver::http_response::string("Internal Server Error").with_status(500);` and add a commented-out log call for `what`. Alternatively add a bold warning block immediately above the code snippet making clear the shown body intentionally leaks exception text and must be replaced before production use.
+
+12. [ ] **security-reviewer** | `src/detail/webserver_error_pages.cpp:104` | logging-failures
+   log_dispatch_error forwards e.what() verbatim to the user log_error callback regardless of the expose_exception_messages flag. The function comment correctly documents this (CWE-532 note), but there is no runtime protection against sensitive data reaching an insufficiently-protected log sink. If e.what() contains attacker-influenced content (e.g. a SQL injection attempt that surfaced in a DB error message, or a path traversal fragment from a file-open failure), it will appear in server logs unconditionally. This is a known and documented trade-off, not a bug introduced by this task, but the revision did not add any mitigation (e.g. log-level gating, truncation, or sanitization hint API).
+   *Recommendation:* This is a pre-existing documented limitation. For this revision, the comment at line 108-117 is adequate. A future task could add a log_sanitizer callback slot on create_webserver that preprocesses the message before forwarding to log_error. No action required to approve this PR.
+
+13. [ ] **security-reviewer** | `src/httpserver/create_webserver.hpp:282` | insecure-design
+   The builder method is declared `create_webserver& expose_exception_messages(bool enable = true)`. The default-argument form means that calling `.expose_exception_messages()` with no argument silently enables the insecure (verbose) path. This is consistent with the existing builder API convention (debug(), pedantic(), use_ssl() all use the same pattern), but those flags are not security-sensitive. A developer following the existing pattern may write `.expose_exception_messages()` believing it is toggling/querying the feature rather than enabling it, accidentally shipping with CWE-209 exposure. The unit test at create_webserver_test.cpp line 123 exercises exactly this no-argument form without asserting that its effect is 'enabled', which could mask the risk during code review.
+   *Recommendation:* Consider removing the default argument so the call site must be explicit: `expose_exception_messages(true)` or `expose_exception_messages(false)`. If the default-arg convention must be kept for API consistency, add a `[[deprecated("Prefer expose_exception_messages(true) — explicit is safer for a security-sensitive flag")]]` annotation on the no-arg overload, or at minimum add a `// CWE-209: this enables the insecure verbose path` comment at the call site in the unit test.
+
+14. [ ] **spec-alignment-checker** | `src/httpserver/detail/webserver_impl_dispatch.hpp:63` | specification-gap
+   The Doxygen/comment block on the `internal_error_page` declaration still says: '- otherwise (no handler set, !force_our): return a default 500 whose body surfaces @p msg, so the unset-handler default is informative.' This description matches the pre-TASK-055 behaviour. After DR-009 Revision 1 the default (no handler, !force_our, expose_exception_messages==false) is the fixed string 'Internal Server Error', not @p msg. The actual implementation in webserver_error_pages.cpp is correct; only this declaration comment was not updated.
+   *Recommendation:* Update the third bullet in the internal_error_page declaration comment (lines 63-65) to read: '- otherwise (no handler set, !force_our): return a default 500 whose body is the fixed string "Internal Server Error" (DR-009 Revision 1 / CWE-209); the body surfaces @p msg only when parent->expose_exception_messages is true (development opt-in).'
+
+15. [ ] **test-quality-reviewer** | `test/integ/basic.cpp:3413` | redundant-test
+   dr009_default_body_is_fixed_string_for_non_std_exception contains LT_CHECK_EQ(s.find('unknown exception'), std::string::npos) which is redundant given the preceding LT_CHECK_EQ(s, std::string('Internal Server Error')). If the body is the exact fixed string 'Internal Server Error', the substring search for 'unknown exception' is guaranteed to return npos and adds no independent protective value.
+   *Recommendation:* Remove the redundant substring assertion; the equality check already provides full coverage of the body content.
+
+16. [ ] **test-quality-reviewer** | `test/integ/basic.cpp:3531` | naming-convention
+   dr009_feature_unavailable_lands_as_generic_500 now silently depends on expose_exception_messages(true) to keep its existing body assertions passing, but the test name gives no hint of this opt-in. A reader relying on the name would expect default-webserver behaviour.
+   *Recommendation:* Rename to dr009_feature_unavailable_verbose_body_surfaces_message or add a brief comment in the test name area explaining that the opt-in is part of the contract being tested, mirroring the pattern used for dr009_verbose_body_surfaces_message_when_opted_in.
+
+17. [ ] **test-quality-reviewer** | `test/unit/create_webserver_test.cpp:122` | implementation-coupling
+   builder_expose_exception_messages_toggle only asserts that the builder methods do not throw (LT_CHECK_NOTHROW); it never verifies that the flag actually propagates to the webserver and affects dispatch behaviour. A builder that silently ignores the flag would pass this test.
+   *Recommendation:* Add a second test (or extend the integration suite) that constructs a webserver with expose_exception_messages(false) (the default), issues a request that triggers an exception, and asserts the body equals 'Internal Server Error'. The existing dr009_default_body_is_fixed_string integration test already provides this coverage, so the unit test could be left as-is if the integration tests are considered authoritative; but the current unit test is strictly a compile/no-crash check rather than a behavioral check.