TASK-085: residual test-smell sweep (auth fail-closed + smartptr fragility)

Item 2 (auth_handler swallow-and-pass): DECIDED fail-closed. A throwing
auth_handler now short-circuits with 500 instead of silently passing the
request through to the resource (a security fail-open, CWE-703). Fixed via
an auth-local try/catch guard in install_auth_alias_() that logs and returns
respond_with(500), matching the documented §5.2 / DR-009 contract. Test
renamed throwing_handler_is_swallowed_and_request_passes ->
throwing_auth_handler_fails_closed_with_500; contract documented in
specs/architecture/04-components/hooks.md.

Item 4 (smartptr parallel-runner fragility): fixed at the root. Replaced
fixed ports with create_webserver(0) + ws.get_bound_port(), and replaced the
shared static counted_resource::dtor_count with a per-test local
std::atomic<int> passed by pointer. Removed the fragility comments and the
set_up() reset. The TU is now parallel-runner safe.

Items 1 (Makefile.am XFAIL comment) and 3 (alias-slot test): already
satisfied by TASK-061 and TASK-066 respectively, per TASK-085's overlap
clauses; verified and dropped, no edits needed.

Full unit/integ suite 106/106 PASS. Pre-existing top-level check-doxygen
baseline failure (7 @security/@ref warnings in untouched headers) is out of
scope.

Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01Rkuh4aSmrD8m2f2vYqakb6

Author: etr

specs/architecture/04-components/hooks.md

@@ -99,6 +99,8 @@ hook_handle http_resource::add_hook(hook_phase, std::function<...>);
 
 **Alias mutability.** All v1 alias setters are **construction-time-only**. Their backing storage is wired during `create_webserver` → `webserver` construction and not mutated afterward. Two aliases (`log_access`, `internal_error_handler`) occupy dedicated single-slot members on `webserver_impl` (`log_access_alias_`, `handler_exception_alias_`); the other three (`auth_handler`, `method_not_allowed_handler`, `not_found_handler`) are seated into the regular per-phase hook vectors via `add_hook(...).detach()` at construction. In neither case is the slot mutable after `webserver::start()`. Users who need runtime registration or replacement of an extension point should use the hook bus directly: `webserver::add_hook(phase, callable)` returns a `hook_handle` that supports `remove()`. This is a deliberate v2.0 design choice (DR-012 / TASK-066): the hook bus IS the runtime extension surface; aliases are documented construction-time sugar. The semantics are pinned by `log_access_alias_is_immutable_after_construction` and `handler_exception_alias_is_immutable_after_construction` in `test/unit/hooks_log_access_alias_slot_test.cpp`.
 
+**`auth_handler` is fail-closed on exception.** The generic short-circuit firing path (`fire_short_circuit_hooks_for_phase`) catches an exception thrown by a `before_handler`/`request_received`/`body_chunk` hook, logs it, and treats it as `pass()` — i.e. the chain continues. The `auth_handler` alias is the one deliberate exception to that policy: because the auth seat is a security boundary, letting a throwing auth callable fall through to the resource would be a fail-open on authentication (CWE-703). The alias therefore wraps its own callable invocation in a local fail-closed guard: an exception is caught, logged via `log_dispatch_error`, and short-circuits the request with a **500** instead of `pass()`. This matches the §5.2 / DR-009 contract that "an exception thrown by a hook is routed through the same path as a throwing resource handler" (which terminates in a 500). Pinned by `throwing_auth_handler_fails_closed_with_500` in `test/unit/auth_handler_optional_signature_test.cpp` (TASK-085). A user `auth_handler` that wants a custom non-500 rejection on its own error path must catch internally and return an engaged `std::optional<http_response>`.
+
 **Related requirements:** PRD-HOOK-REQ-001..009.
 **Related decisions:** DR-012, §5.6.
 

specs/tasks/M7-v2-cleanup/TASK-085.md

@@ -8,10 +8,10 @@
 A cluster of small, independent test smells that the audit groups under "Other test smells". Each is too small to be its own task but together they erode confidence in the test contract.
 
 **Action Items:**
-- [ ] `test/Makefile.am:67-74`: orphaned comment claiming `header_hygiene` is in `XFAIL_TESTS`; lines 532-535 admit it was removed when TASK-020 landed. Update the comment. (Mechanical — overlaps with TASK-061; do whichever lands first and drop from the other.)
-- [ ] `test/unit/auth_handler_optional_signature_test.cpp:176-192`: `throwing_handler_is_swallowed_and_request_passes` ratifies a questionable swallow-and-pass behaviour as the pin. Decide: is this the intended contract? If yes, update the test name to make the contract explicit (e.g., `auth_handler_exception_is_logged_and_request_proceeds_without_auth`) and document in `specs/architecture/` why. If no, change the behaviour to fail-closed (reject the request with 500 or equivalent) and update the test accordingly.
-- [ ] `test/unit/hooks_log_access_alias_slot_test.cpp:166-205`: "second registration replaces first" only simulates re-registration via two webservers. Once TASK-066 ships the runtime setter (or pins the immutable-after-start contract), update this test to exercise the real path.
-- [ ] `test/unit/webserver_register_smartptr_test.cpp:60-64, 148-156`: documented parallel-runner fragility. Diagnose root cause (most likely a shared static or filesystem path collision) and fix, or convert the affected sub-tests to serial-only with an explicit littletest annotation.
+- [x] `test/Makefile.am:67-74`: orphaned comment claiming `header_hygiene` is in `XFAIL_TESTS`; lines 532-535 admit it was removed when TASK-020 landed. Update the comment. (Mechanical — overlaps with TASK-061; do whichever lands first and drop from the other.) — **Satisfied by TASK-061** (status Done; lines 67-74 are now the cookie-test block, the accurate historical XFAIL note lives at the `header_hygiene_SOURCES` block ~103-115). No orphaned comment remains; dropped per the overlap clause.
+- [x] `test/unit/auth_handler_optional_signature_test.cpp:176-192`: `throwing_handler_is_swallowed_and_request_passes` ratifies a questionable swallow-and-pass behaviour as the pin. Decide: is this the intended contract? If yes, update the test name to make the contract explicit (e.g., `auth_handler_exception_is_logged_and_request_proceeds_without_auth`) and document in `specs/architecture/` why. If no, change the behaviour to fail-closed (reject the request with 500 or equivalent) and update the test accordingly. — **DECIDED: fail-closed.** A throwing auth handler now returns 500 (auth-local guard in `src/detail/webserver_aliases.cpp`); test renamed to `throwing_auth_handler_fails_closed_with_500`; contract documented in `specs/architecture/04-components/hooks.md` ("`auth_handler` is fail-closed on exception"), removing the spec/impl contradiction with §5.2/DR-009.
+- [x] `test/unit/hooks_log_access_alias_slot_test.cpp:166-205`: "second registration replaces first" only simulates re-registration via two webservers. Once TASK-066 ships the runtime setter (or pins the immutable-after-start contract), update this test to exercise the real path. — **Satisfied by TASK-066** (status Done). TASK-066 chose option (b): aliases are immutable-after-construction, NO runtime setter. The misleading test was replaced by `log_access_alias_is_immutable_after_construction` + `handler_exception_alias_is_immutable_after_construction`, which exercise the real (immutability) contract.
+- [x] `test/unit/webserver_register_smartptr_test.cpp:60-64, 148-156`: documented parallel-runner fragility. Diagnose root cause (most likely a shared static or filesystem path collision) and fix, or convert the affected sub-tests to serial-only with an explicit littletest annotation. — **Fixed (root cause).** Replaced fixed ports with `create_webserver(0)` + `ws.get_bound_port()` (no cross-test port collision) and replaced the shared static `counted_resource::dtor_count` with a per-test local `std::atomic<int>` passed by pointer (no cross-test contamination). Fragility comments and the `set_up()` reset removed.
 
 **Dependencies:**
 - Blocked by: TASK-066 (for the alias-slot test work)
@@ -26,4 +26,4 @@ A cluster of small, independent test smells that the audit groups under "Other t
 **Related Requirements:** PRD §2 test reliability NFR
 **Related Decisions:** None new
 
-**Status:** Backlog
+**Status:** Done

specs/tasks/M7-v2-cleanup/_index.md

@@ -49,7 +49,7 @@ TASK-093).
 | TASK-082 | Tighten static-size bounds in `http_resource_test` and `webserver_pimpl_test` | MED | S | Done |
 | TASK-083 | Wire real CI gates into benchmarks | MED | M | Done |
 | TASK-084 | Re-measure libstdc++/Linux v1 baseline for `get_headers` ns/call | MED | S | Done |
-| TASK-085 | Residual test-smell sweep | MED | S | Backlog |
+| TASK-085 | Residual test-smell sweep | MED | S | Done |
 | TASK-086 | Execute file-size split roadmap (FILE_LOC_MAX 750 → 500) | HIGH | L | Backlog |
 | TASK-087 | Restore msan CI lane | HIGH | M | Backlog |
 | TASK-088 | Re-enable helgrind / drd / sgcheck in valgrind CI lane | HIGH | M | Backlog |

src/detail/webserver_aliases.cpp

@@ -56,6 +56,7 @@
 #include "httpserver/detail/webserver_impl.hpp"
 
 #include <cassert>
+#include <exception>
 #include <functional>
 #include <memory>
 #include <optional>
@@ -227,7 +228,32 @@ void webserver::install_auth_alias_() {
                 // (one heap alloc removed per request that runs through
                 // this hook), and small responses ride the http_response
                 // SBO with zero further allocs.
-                auto rejection = ws_ptr->auth_handler(*ctx.request);
+                // Fail-closed (TASK-085): the auth seat is a security
+                // boundary, so a throwing auth callable must NOT fall
+                // through to the resource. The generic short-circuit
+                // firing path (fire_short_circuit_hooks_for_phase) treats
+                // a throwing hook as pass(), which would be a security
+                // fail-open here (CWE-703). Wrap the callable in a local
+                // try/catch that short-circuits with a 500 instead,
+                // matching the documented §5.2 / DR-009 contract that an
+                // exception thrown by a hook is routed through the same
+                // path as a throwing resource handler (which yields 500).
+                std::optional<http_response> rejection;
+                try {
+                    rejection = ws_ptr->auth_handler(*ctx.request);
+                } catch (const std::exception& e) {
+                    impl_ptr->log_dispatch_error(
+                        std::string("auth_handler threw: ").append(e.what())
+                            .append("; failing closed with 500"));
+                    return hook_action::respond_with(
+                        http_response::empty().with_status(500));
+                } catch (...) {
+                    impl_ptr->log_dispatch_error(
+                        "auth_handler threw unknown exception; "
+                        "failing closed with 500");
+                    return hook_action::respond_with(
+                        http_response::empty().with_status(500));
+                }
                 if (!rejection) {
                     return hook_action::pass();
                 }

test/unit/auth_handler_optional_signature_test.cpp

@@ -165,16 +165,21 @@ LT_BEGIN_AUTO_TEST(auth_handler_optional_signature_suite,
     ws.stop();
 LT_END_AUTO_TEST(engaged_optional_rejects_request)
 
-// 3. Auth handler throwing a std::runtime_error: pin the observable
-//    contract for error paths through the hook-invocation machinery.
-//    The current dispatch path swallows the exception and lets the
-//    request progress (equivalent to nullopt). The hook framework's
-//    handler-exception slot is the named extension point for converting
-//    an exception into a custom response; the alias hook itself stays
-//    non-fatal so a buggy auth callable cannot DoS the whole server.
-//    A future change that short-circuits with 500 must update this pin.
+// 3. Auth handler throwing a std::exception: the auth alias is
+//    FAIL-CLOSED (TASK-085). A throwing auth callable must NOT let the
+//    request through to the resource — that would be a security
+//    fail-open on the authentication boundary (CWE-703). Instead the
+//    alias hook catches the exception, logs it, and short-circuits with
+//    a 500, matching the documented §5.2 / DR-009 contract that an
+//    exception thrown by a hook is "routed through the same path as a
+//    throwing resource handler" (which terminates in a 500). The
+//    contrast with the generic short-circuit phases (which still treat
+//    a throwing hook as pass()) is deliberate: the auth seat is the one
+//    place where fail-open is most dangerous, so the alias wraps its own
+//    callable invocation in a fail-closed guard. See the auth_handler
+//    row in specs/architecture/04-components/hooks.md.
 LT_BEGIN_AUTO_TEST(auth_handler_optional_signature_suite,
-                   throwing_handler_is_swallowed_and_request_passes)
+                   throwing_auth_handler_fails_closed_with_500)
     webserver ws{create_webserver(PORT_3)
         .auth_handler([](const http_request&)
                           -> std::optional<http_response> {
@@ -185,11 +190,12 @@ LT_BEGIN_AUTO_TEST(auth_handler_optional_signature_suite,
     ws.start(false);
 
     fetch_result fr = fetch("localhost:" PORT_3_STRING "/protected");
-    LT_CHECK_EQ(fr.response_code, 200);
-    LT_CHECK_EQ(fr.body, std::string("SUCCESS"));
+    // Fail-closed: the request must NOT reach the resource.
+    LT_CHECK_EQ(fr.response_code, 500);
+    LT_CHECK(fr.body != std::string("SUCCESS"));
 
     ws.stop();
-LT_END_AUTO_TEST(throwing_handler_is_swallowed_and_request_passes)
+LT_END_AUTO_TEST(throwing_auth_handler_fails_closed_with_500)
 
 // 4. Engaged optional carrying a 64 KB body arrives uncorrupted on the
 //    wire. Regression target for the MOVE from the optional into

test/unit/webserver_register_smartptr_test.cpp

@@ -55,13 +55,16 @@ using httpserver::http_resource;
 using httpserver::http_response;
 using httpserver::webserver;
 
-// PORT and the PORT+N offsets below are fixed numeric values shared by all
-// tests in this TU. The tests run sequentially (single-process), so the
-// offsets (PORT+0 through PORT+5) do not conflict with each other.
-// If a future parallel test runner binds the same ports concurrently, switch
-// to create_webserver(0) and retrieve the OS-assigned port via
-// ws.get_bound_port() after ws.start().
-#define PORT 8080
+// TASK-085: this TU is parallel-runner safe.
+//   - The one test that starts a server and does a curl round-trip
+//     (unique_ptr_overload_compiles_and_serves) binds an OS-assigned
+//     ephemeral port via create_webserver(0) and reads it back with
+//     ws.get_bound_port() — no fixed port, no cross-test collision.
+//   - Every other test constructs create_webserver(0) but never calls
+//     start(), so no port is ever bound; the ephemeral 0 is harmless.
+//   - The destructor-counting tests use a per-test local std::atomic<int>
+//     passed to counted_resource by pointer, not a shared static, so two
+//     tests running concurrently cannot contaminate each other's count.
 
 namespace {
 
@@ -77,21 +80,24 @@ class ok_resource : public http_resource {
      }
 };
 
-// Resource whose destructor increments a static counter, so tests can
-// observe ownership-driven destruction.
+// Resource whose destructor increments a caller-supplied counter, so
+// tests can observe ownership-driven destruction. The counter is a
+// per-test local std::atomic<int> passed by pointer (TASK-085) rather
+// than a shared static, so concurrent tests cannot contaminate each
+// other's count under a parallel runner.
 class counted_resource : public http_resource {
  public:
-     static std::atomic<int> dtor_count;
-
-     counted_resource() = default;
-     ~counted_resource() override { ++dtor_count; }
+     explicit counted_resource(std::atomic<int>* counter)
+         : counter_(counter) {}
+     ~counted_resource() override { ++(*counter_); }
 
      http_response render_get(const http_request&) override {
          return http_response::string("OK");
      }
-};
 
-std::atomic<int> counted_resource::dtor_count{0};
+ private:
+     std::atomic<int>* counter_;
+};
 
 }  // namespace
 
@@ -145,15 +151,9 @@ static_assert(!has_raw_register_resource<webserver>::value,
 // ---- Runtime ownership tests ------------------------------------------
 
 LT_BEGIN_SUITE(webserver_register_smartptr_suite)
-    void set_up() {
-        // Reset the static dtor_count before every test so tests do not
-        // accumulate each other's destructor calls. This relies on the
-        // current runner executing tests sequentially; if a parallel
-        // runner is ever introduced, replace dtor_count with a per-test
-        // local atomic passed by reference.
-        counted_resource::dtor_count = 0;
-    }
-
+    // No shared mutable state to reset: the destructor-counting tests
+    // each own a local std::atomic<int> (TASK-085).
+    void set_up() {}
     void tear_down() {}
 LT_END_SUITE(webserver_register_smartptr_suite)
 
@@ -168,15 +168,19 @@ LT_END_SUITE(webserver_register_smartptr_suite)
 // explicit "compiles AND serves" requirement.
 LT_BEGIN_AUTO_TEST(webserver_register_smartptr_suite,
                    unique_ptr_overload_compiles_and_serves)
-    webserver ws{create_webserver(PORT)};
+    webserver ws{create_webserver(0)};
     auto r = std::make_unique<ok_resource>();
     ws.register_resource("/foo", std::move(r));
     ws.start(false);
+    // OS-assigned ephemeral port: no fixed port to collide under a
+    // parallel runner (TASK-085).
+    const std::string url =
+        "localhost:" + std::to_string(ws.get_bound_port()) + "/foo";
 
     curl_global_init(CURL_GLOBAL_ALL);
     std::string s;
     CURL* curl = curl_easy_init();
-    curl_easy_setopt(curl, CURLOPT_URL, "localhost:8080/foo");
+    curl_easy_setopt(curl, CURLOPT_URL, url.c_str());
     curl_easy_setopt(curl, CURLOPT_HTTPGET, 1L);
     curl_easy_setopt(curl, CURLOPT_WRITEFUNCTION, writefunc);
     curl_easy_setopt(curl, CURLOPT_WRITEDATA, &s);
@@ -192,35 +196,38 @@ LT_END_AUTO_TEST(unique_ptr_overload_compiles_and_serves)
 // when the webserver is destroyed."
 LT_BEGIN_AUTO_TEST(webserver_register_smartptr_suite,
                    unique_ptr_dtor_runs_on_webserver_destruction)
-    LT_CHECK_EQ(counted_resource::dtor_count.load(), 0);
+    std::atomic<int> dtor_count{0};
+    LT_CHECK_EQ(dtor_count.load(), 0);
     {
-        webserver ws{create_webserver(PORT + 1)};
-        ws.register_resource("/x", std::make_unique<counted_resource>());
+        webserver ws{create_webserver(0)};
+        ws.register_resource(
+            "/x", std::make_unique<counted_resource>(&dtor_count));
         // No start/stop needed — registration alone must transfer
         // ownership; webserver destruction must run the dtor.
     }
-    LT_CHECK_EQ(counted_resource::dtor_count.load(), 1);
+    LT_CHECK_EQ(dtor_count.load(), 1);
 LT_END_AUTO_TEST(unique_ptr_dtor_runs_on_webserver_destruction)
 
 // shared_ptr semantics: caller's shared_ptr keeps the resource alive
 // past webserver destruction. The dtor runs only once both refs drop.
 LT_BEGIN_AUTO_TEST(webserver_register_smartptr_suite,
                    shared_ptr_caller_keeps_resource_alive)
-    LT_CHECK_EQ(counted_resource::dtor_count.load(), 0);
-    auto sp = std::make_shared<counted_resource>();
+    std::atomic<int> dtor_count{0};
+    LT_CHECK_EQ(dtor_count.load(), 0);
+    auto sp = std::make_shared<counted_resource>(&dtor_count);
     {
-        webserver ws{create_webserver(PORT + 2)};
+        webserver ws{create_webserver(0)};
         ws.register_resource("/x", sp);
     }
     // Webserver destroyed; caller still holds a ref.
-    LT_CHECK_EQ(counted_resource::dtor_count.load(), 0);
+    LT_CHECK_EQ(dtor_count.load(), 0);
     sp.reset();
-    LT_CHECK_EQ(counted_resource::dtor_count.load(), 1);
+    LT_CHECK_EQ(dtor_count.load(), 1);
 LT_END_AUTO_TEST(shared_ptr_caller_keeps_resource_alive)
 
 LT_BEGIN_AUTO_TEST(webserver_register_smartptr_suite,
                    null_unique_ptr_throws)
-    webserver ws{create_webserver(PORT + 3)};
+    webserver ws{create_webserver(0)};
     bool caught_invalid_argument = false;
     try {
         ws.register_resource("/x", std::unique_ptr<http_resource>{});
@@ -234,7 +241,7 @@ LT_END_AUTO_TEST(null_unique_ptr_throws)
 
 LT_BEGIN_AUTO_TEST(webserver_register_smartptr_suite,
                    null_shared_ptr_throws)
-    webserver ws{create_webserver(PORT + 4)};
+    webserver ws{create_webserver(0)};
     bool caught_invalid_argument = false;
     try {
         ws.register_resource("/x", std::shared_ptr<http_resource>{});
@@ -249,7 +256,7 @@ LT_END_AUTO_TEST(null_shared_ptr_throws)
 // throw-on-null behavior.
 LT_BEGIN_AUTO_TEST(webserver_register_smartptr_suite,
                    duplicate_registration_throws)
-    webserver ws{create_webserver(PORT + 5)};
+    webserver ws{create_webserver(0)};
     ws.register_resource("/dup", std::make_shared<ok_resource>());
     bool caught_invalid_argument = false;
     try {