Merge TASK-036 minors (13/43 marked; 30 still need review)

Author: etr

.gitignore

@@ -1,5 +1,6 @@
 *.sw*
 *.*~
+*~
 *.in
 *.php
 *.pm

specs/architecture/04-components/http-resource.md

@@ -2,12 +2,12 @@
 
 **Responsibility:** Stateful handler base for cases where state is shared across HTTP methods of one resource (counter, cache, DB handle, auth context).
 
-**Implementation:** Public abstract base. Subclasses override one of `render_get / render_post / render_put / render_delete / render_patch / render_options / render_head` (renamed from v1's `render_GET` etc., to comply with PRD-NAM-REQ-001 snake_case). The default `render(...)` falls back when the method-specific override is not provided.
+**Implementation:** Public abstract base. Subclasses override one of `render_get / render_post / render_put / render_delete / render_patch / render_options / render_head` (renamed from v1's `render_GET` etc., to comply with PRD-NAM-REQ-001 snake_case). Each `render_*` override returns `http_response` **by value** (DR-004 / PRD-RSP-REQ-007); the webserver moves the returned value into the per-connection `modded_request::response` anchor. The default `render(...)` falls back when the method-specific override is not provided; it returns a default-constructed `http_response` whose `status_code_ == -1` sentinel routes through `internal_error_page`.
 
 The allow-mask (formerly `std::map<std::string, bool> method_state`) becomes `method_set methods_allowed_;` — a `uint32_t` bitmask wrapper (DR-6). `is_allowed(http_method)` and `get_allowed_methods()` are `const` and return without allocation.
 
 **Lifetime:** owned by the `webserver` via `unique_ptr` or `shared_ptr` (PRD-HDL-REQ-003). Raw-pointer registration is gone (PRD-HDL-REQ-005).
 
-**Related requirements:** PRD-HDL-REQ-003, PRD-HDL-REQ-005, PRD-REQ-REQ-002, PRD-REQ-REQ-003.
+**Related requirements:** DR-004, PRD-HDL-REQ-003, PRD-HDL-REQ-005, PRD-REQ-REQ-002, PRD-REQ-REQ-003, PRD-RSP-REQ-007.
 
 ---

specs/unworked_review_issues/2026-05-18_142351_task-036.md

@@ -73,17 +73,20 @@ then use entry (or std::move(entry) for the regex push_back). This removes ~6 du
 
 ## Minor
 
-13. [ ] **architecture-alignment-checker** | `src/httpserver/create_webserver.hpp:67` | adr-violation
+13. [x] **architecture-alignment-checker** | `src/httpserver/create_webserver.hpp:67` | adr-violation
    auth_handler_ptr is still typed as std::function<std::shared_ptr<http_response>(const http_request&)> — a v1 shared_ptr return pattern — while DR-004 mandates http_response by value for all handler signatures. The dispatch site in webserver.cpp (line 2227-2229) bridges the gap with a manual std::move(*auth_response) into mr->response_, with a comment explicitly marking this as out of scope for TASK-036.
    *Recommendation:* Track in a follow-up task: change auth_handler_ptr to std::function<std::optional<http_response>(const http_request&)> (optional because null means 'pass through') and remove the bridging code. This would complete the DR-004 rollout to the auth hook.
+   *Status:* already addressed — TODO comment added on auth_handler_ptr in create_webserver.hpp (commit a11e5ee). The deferred migration is tracked.
 
-14. [ ] **architecture-alignment-checker** | `src/httpserver/detail/modded_request.hpp:51` | pattern-violation
+14. [x] **architecture-alignment-checker** | `src/httpserver/detail/modded_request.hpp:51` | pattern-violation
    The callback pointer-to-member (http_response (http_resource::*callback)(...)) has no default member initializer. If an unrecognized HTTP method is received and a resource is found, mr->callback is indeterminate. The code comment at webserver.cpp:2382 acknowledges this as a pre-existing latent bug from TASK-027, and the 405 guard (is_allowed(count_) == false) prevents the indeterminate pointer from being dereferenced in practice. However, TASK-036 introduced the member with its current return-by-value signature, making it the right time to add a null initializer or a static_assert.
    *Recommendation:* Add a default member initializer: http_response (httpserver::http_resource::*callback)(const httpserver::http_request&) = nullptr; and guard the dispatch call site with an assert or explicit null check to eliminate the latent UB.
+   *Status:* already addressed — `= nullptr` default initializer added to `callback` field (commit 22d8f07). Comment in resolve_method_callback documents the nullptr-for-unrecognized-method invariant.
 
-15. [ ] **architecture-alignment-checker** | `src/webserver.cpp:2307` | interface-contract
+15. [x] **architecture-alignment-checker** | `src/webserver.cpp:2307` | interface-contract
    For a deferred (streaming) response, finalize_answer calls MHD_destroy_response(raw_response) immediately after MHD_queue_response. The deferred_body trampoline passes this (deferred_body*) as the cls pointer to MHD_create_response_from_callback. The body is safe because MHD's own reference counting keeps the MHD_Response alive until streaming completes, and request_completed (which destroys the modded_request and its embedded response_) only fires after MHD is done. However the code comment at the call site does not document this MHD ref-counting assumption, leaving a subtle lifetime dependency undocumented.
    *Recommendation:* Add a comment near MHD_destroy_response in finalize_answer explaining that for MHD_create_response_from_callback responses, MHD increments its own refcount during MHD_queue_response, so the immediate MHD_destroy_response only releases the caller's reference — the trampoline's cls pointer (deferred_body*) remains valid until request_completed fires.
+   *Status:* fixed in this batch (commit 72614c0 — added MHD refcounting comment near MHD_destroy_response in materialize_and_queue_response in webserver_request.cpp).
 
 16. [ ] **code-quality-reviewer** | `src/Makefile.am:25` | code-elegance
    Removal of http_resource.cpp from sources is correct and clean. No issue, noted for completeness that the build system correctly reflects the header-only migration.

src/detail/webserver_aliases.cpp

@@ -178,6 +178,15 @@ void webserver::install_default_alias_hooks_() {
     // webserver_impl::apply_auth_short_circuit inline path: it respects
     // auth_skip_paths, calls the user-supplied auth_handler, and
     // short-circuits with the returned response when auth fails.
+    //
+    // Design note (security-reviewer-iter1-7 / CWE-200): auth_handler
+    // is registered as a before_handler hook, which fires only for
+    // matched routes (found==true). Requests to unregistered paths
+    // receive a 404 without consulting auth_handler. This means 401 vs
+    // 404 distinguishes registered from unregistered routes (auth oracle).
+    // Callers who need uniform authentication on all requests — including
+    // 404s — should add a catch-all fallback route or register a
+    // not_found_handler that applies equivalent auth logic.
     // ----------------------------------------------------------------
     if (auth_handler != nullptr) {
         // Capture both the webserver* (for auth_handler callable) and the

src/detail/webserver_callbacks.cpp

@@ -309,7 +309,7 @@ void webserver_impl::error_log(void* cls, const char* fmt, va_list ap) {
     webserver* dws = static_cast<webserver*>(cls);
 
     std::string msg;
-    msg.resize(80);  // Asssume one line will be enought most of the time.
+    msg.resize(80);  // Assume one line will be enough most of the time.
 
     va_list va;
     va_copy(va, ap);  // Stash a copy in case we need to try again.

src/detail/webserver_error_pages.cpp

@@ -98,6 +98,12 @@ void webserver_impl::log_dispatch_error(std::string_view msg) const {
     if (parent->log_error == nullptr) {
         return;
     }
+    // Framework contract (CWE-532): msg may contain e.what() text from
+    // a handler exception, which could include sensitive data (DB connection
+    // strings, file paths, user-supplied input that triggered the exception).
+    // Application code should sanitize or wrap exceptions that might expose
+    // sensitive information before re-throwing them.
+    //
     // A misbehaving user logger must not poison the catch from inside
     // the catch. Swallow any exception it throws; we have no recovery
     // beyond dropping the log line.

src/detail/webserver_request.cpp

@@ -194,41 +194,44 @@ struct MHD_Response* webserver_impl::get_raw_response_with_fallback(detail::modd
     // emplace(std::move(...)); the optional owns the value and the
     // deferred-body trampoline keeps a pointer into it for the lifetime
     // of the modded_request.
-    auto materialize_current = [&]() -> struct MHD_Response* {
+    auto try_materialize = [&]() -> struct MHD_Response* {
         return materialize_response(mr->response ? &*mr->response : nullptr);
     };
+    // Helper: emplace a new response and immediately materialize it.
+    // Each catch arm sets mr->response then returns the raw MHD handle.
+    auto emplace_and_materialize = [&](http_response r) -> struct MHD_Response* {
+        mr->response.emplace(std::move(r));
+        return try_materialize();
+    };
     try {
-        struct MHD_Response* raw = materialize_current();
+        struct MHD_Response* raw = try_materialize();
         if (raw == nullptr) {
             // TASK-031: no exception was thrown, but the body materializer
             // returned null. Route through the safe internal-error path so
             // a misbehaving user handler can't escape.
-            mr->response.emplace(run_internal_error_handler_safely(
+            return emplace_and_materialize(run_internal_error_handler_safely(
                 mr, "materialize_response returned null"));
-            raw = materialize_current();
         }
         return raw;
     } catch(const std::invalid_argument&) {
         try {
-            mr->response.emplace(not_found_page(mr));
-            return materialize_current();
+            return emplace_and_materialize(not_found_page(mr));
         } catch(...) {
             return nullptr;
         }
     } catch(const std::exception& e) {
         log_dispatch_error(std::string("materialize threw: ") + e.what());
         try {
-            mr->response.emplace(run_internal_error_handler_safely(mr, e.what()));
-            return materialize_current();
+            return emplace_and_materialize(
+                run_internal_error_handler_safely(mr, e.what()));
         } catch(...) {
             return nullptr;
         }
     } catch(...) {
         log_dispatch_error("materialize threw unknown exception");
         try {
-            mr->response.emplace(run_internal_error_handler_safely(mr,
-                                                         "unknown exception"));
-            return materialize_current();
+            return emplace_and_materialize(run_internal_error_handler_safely(
+                mr, "unknown exception"));
         } catch(...) {
             return nullptr;
         }
@@ -260,6 +263,14 @@ MHD_Result webserver_impl::materialize_and_queue_response(MHD_Connection* connec
     // queued bytes -- only the http_response in mr->response matters
     // for the hook ctx pointer.
     fire_response_sent_gated(mr);
+    // MHD reference-counting note: for MHD_create_response_from_callback
+    // responses (deferred/streaming), MHD increments its own internal
+    // refcount during MHD_queue_response, so this MHD_destroy_response only
+    // releases the *caller's* reference. MHD keeps the streaming callback
+    // (deferred_body::trampoline) alive — and therefore the cls pointer into
+    // mr->response — until request_completed fires and MHD releases its own
+    // reference. The modded_request (and mr->response) are destroyed only in
+    // the request_completed callback, which is after MHD is done streaming.
     MHD_destroy_response(raw_response);
     return (MHD_Result) to_ret;
 }

src/detail/webserver_routes.cpp

@@ -170,8 +170,11 @@ void webserver_impl::commit_handlers_to_shim(detail::lambda_resource& shim,
         method_set methods,
         const std::function<::httpserver::http_response(
             const ::httpserver::http_request&)>& handler) {
-    // The shared std::function copies cheaply (type-erased callable),
-    // so each slot owns its own copy.
+    // Each slot gets its own copy of the std::function. For small captures
+    // (within SBO, ~16-32 bytes on libstdc++/libc++) the copies are cheap.
+    // For large-capture handlers registered across many methods, each copy
+    // allocates once. Registration-time only; not a hot-path concern.
+    // (performance-reviewer-iter1-3: document the per-slot copy behaviour.)
     for_each_requested_method(methods, [&](http_method m) {
         shim.set_slot(m, handler);
     });

src/detail/webserver_websocket.cpp

@@ -212,11 +212,15 @@ webserver_impl::complete_websocket_upgrade(MHD_Connection* connection,
     std::shared_ptr<websocket_handler> handler_sp = ws_it->second;
     lock.unlock();
 
-    auto* data = new ws_upgrade_data{this, std::move(handler_sp)};
+    // CWE-401: RAII guard so data is freed if MHD_create_response_for_upgrade
+    // returns null. Ownership is transferred to MHD (via release()) only after
+    // the queue call — upgrade_handler receives data_guard.get() as cls and
+    // wraps it in unique_ptr for cleanup (see upgrade_handler below).
+    std::unique_ptr<ws_upgrade_data> data_guard(
+        new ws_upgrade_data{this, std::move(handler_sp)});
     struct MHD_Response* response = MHD_create_response_for_upgrade(
-        &webserver_impl::upgrade_handler, data);
+        &webserver_impl::upgrade_handler, data_guard.get());
     if (response == nullptr) {
-        delete data;
         return std::nullopt;
     }
     MHD_add_response_header(response, MHD_HTTP_HEADER_UPGRADE, "websocket");
@@ -231,6 +235,7 @@ webserver_impl::complete_websocket_upgrade(MHD_Connection* connection,
                                                        MHD_HTTP_SWITCHING_PROTOCOLS,
                                                        response);
     MHD_destroy_response(response);
+    data_guard.release();  // MHD now owns; upgrade_handler will delete.
     return to_ret;
 }
 

src/httpserver/detail/lambda_resource.hpp

@@ -64,7 +64,10 @@ namespace detail {
 
 // Tiny adapter that wraps a single lambda_handler as an http_resource
 // virtual override. We keep one slot per method enum and dispatch in
-// each render_* override.
+// each render_* override. Each slot holds a
+//   std::function<http_response(const http_request&)>
+// (see route_entry.hpp::lambda_handler). Slots are installed via
+// set_slot() and invoked by the render_* overrides below.
 class lambda_resource final : public ::httpserver::http_resource {
  public:
     lambda_resource() {
@@ -90,6 +93,9 @@ class lambda_resource final : public ::httpserver::http_resource {
     // Each differs only by the http_method enum constant forwarded.
     // They are required by the http_resource base-class interface and
     // cannot be collapsed further without changing that interface.
+    // render_connect and render_trace intentionally fall back to the
+    // base-class default (no slot wired, no set_allowing call); they
+    // return the -1 sentinel and route through internal_error_page.
     ::httpserver::http_response
     render_get(const ::httpserver::http_request& r) override {
         return invoke_(http_method::get, r);
@@ -120,6 +126,9 @@ class lambda_resource final : public ::httpserver::http_resource {
     }
 
  private:
+    // Dispatch to the registered slot for method `m`, returning the
+    // handler's http_response by value. Callers must ensure is_allowed(m)
+    // is true before calling (the 405 guard in finalize_answer enforces this).
     ::httpserver::http_response
     invoke_(http_method m, const ::httpserver::http_request& r) {
         auto& slot = slots_[static_cast<std::size_t>(m)];