TASK-058 validation iter1: address reviewer findings

- should_skip_auth (src/detail/webserver_request.cpp): switch the
  wildcard guard from size() > 2 to size() >= 2 so the global "/*"
  entry (size == 2) actually matches every path; also rewrite the
  prefix compare to use string_view to avoid a per-iteration substr
  allocation (security-reviewer-iter1-1).
- normalize_auth_skip_paths: reject entries containing '%' with
  std::invalid_argument.  Skip-path entries must be provided in
  decoded form -- a percent-encoded entry would never match MHD's
  decoded request path and silently bypass nothing, a quiet
  misconfiguration hazard (security-reviewer-iter1-3).  The
  wildcard-prefix branch grows a special case for the literal "/*"
  so canonicalisation does not collapse it to "//*" or drop it.
- get_allow_header (src/http_resource.cpp + .hpp): take the
  methods_allowed_ snapshot inside the mutex instead of before it
  to eliminate the TOCTOU data race (security-reviewer-iter1-2),
  and upgrade std::mutex to std::shared_mutex so concurrent warm-
  path readers no longer serialise -- only the cold fill path
  takes a unique_lock (performance-reviewer-iter1-1).  Double-
  checked lock pattern: re-read methods_allowed_ under the unique
  lock so a fill that races a refill is idempotent.
- bench_sizeof_http_resource: update the size-envelope algebra
  and comments for std::shared_mutex (~168 B on libc++, ~56 B on
  libstdc++).  The static_assert ceiling is recomputed.
- http_resource hpp / cpp comment blocks: refresh the cache
  contract description and the by-hand copy / move rationale now
  that the lock type is std::shared_mutex.
- New unit pins:
    auth_skip_normalize_test: global_wildcard_skip_path_matches_any_path
      covers the size() >= 2 fix; percent_encoded_skip_path_throws_invalid_argument
      covers the misconfiguration-rejection contract.
    http_resource_allow_cache_test: concurrent_reads_all_return_correct_value
      exercises 8 threads x 1000 iterations to surface a TSAN race if
      methods_allowed_ ever drifts back outside the lock or the warm
      path is taken without a shared_lock.

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

Author: etr

src/detail/webserver_request.cpp

@@ -146,26 +146,47 @@ std::string normalize_path(const std::string& path) {
 // matched verbatim against a normalized request path, so non-
 // canonical inputs (e.g. "/public/", "/a/../b") silently never
 // matched -- the on-the-wire bug this normalization closes.
+//
+// security-reviewer-iter1-3: entries containing '%' are rejected with
+// std::invalid_argument.  Skip-path entries must be provided in
+// decoded form (the same form as the request path after MHD's
+// unescaper runs).  A '%'-encoded entry would never match a decoded
+// request path and would silently bypass auth for no route -- a
+// misconfiguration hazard caught early here.
 std::vector<std::string> normalize_auth_skip_paths(
         const std::vector<std::string>& raw) {
     std::vector<std::string> out;
     out.reserve(raw.size());
     for (const auto& entry : raw) {
+        // Reject percent-encoded entries: skip-path entries must be
+        // provided in decoded form.  A '%' in the entry indicates a
+        // URL-encoded sequence that would never match the decoded
+        // request path produced by MHD's unescaper.
+        if (entry.find('%') != std::string::npos) {
+            throw std::invalid_argument(
+                "auth_skip_paths entry contains a percent-encoded "
+                "sequence ('" + entry + "'). "
+                "Skip-path entries must be provided in decoded form "
+                "(e.g. '/public/test', not '/public%2Ftest').");
+        }
         // Wildcard suffix: strip the trailing "/*", normalize the
-        // prefix, then re-append "/*".  Empty prefix (just "/*")
-        // canonicalises to "/" + "/*" -> "//*", which would never
-        // have matched anything sensible pre-fix either, so we keep
-        // the literal "/*" unchanged (matches the v1 wildcard
-        // behaviour at the dispatch site).
-        if (entry.size() > 2 && entry.back() == '*' &&
+        // prefix, then re-append "/*".  The special case "/*" (size
+        // == 2) means "match every path" and is stored as-is so
+        // should_skip_auth can recognise it with the >= 2 guard.
+        if (entry.size() >= 2 && entry.back() == '*' &&
             entry[entry.size() - 2] == '/') {
-            std::string prefix = entry.substr(0, entry.size() - 2);
-            std::string normalized_prefix = normalize_path(prefix);
-            if (normalized_prefix == "/") {
-                // "/" + "/*" -> "/*" (keep literal).
+            if (entry.size() == 2) {
+                // "/*" -- global wildcard: matches every path.
                 out.push_back("/*");
             } else {
-                out.push_back(normalized_prefix + "/*");
+                std::string prefix = entry.substr(0, entry.size() - 2);
+                std::string normalized_prefix = normalize_path(prefix);
+                if (normalized_prefix == "/") {
+                    // Prefix collapsed to root -- treat as "/*".
+                    out.push_back("/*");
+                } else {
+                    out.push_back(normalized_prefix + "/*");
+                }
             }
             continue;
         }
@@ -193,11 +214,18 @@ bool webserver_impl::should_skip_auth(const std::string& path) const {
 
     for (const auto& skip_path : parent->auth_skip_paths_normalized) {
         if (skip_path == normalized) return true;
-        // Support wildcard suffix (e.g., "/public/*")
-        if (skip_path.size() > 2 && skip_path.back() == '*' &&
+        // Support wildcard suffix (e.g., "/public/*").
+        // security-reviewer-iter1-1: use >= 2 (not > 2) so the global
+        // wildcard "/*" (size == 2) is handled.  When skip_path is "/*"
+        // the prefix is "/" and every normalized path starts with "/",
+        // so we return true immediately for any request.
+        if (skip_path.size() >= 2 && skip_path.back() == '*' &&
             skip_path[skip_path.size() - 2] == '/') {
-            std::string prefix = skip_path.substr(0, skip_path.size() - 1);
-            if (normalized.compare(0, prefix.size(), prefix) == 0) return true;
+            std::string_view prefix(skip_path.data(), skip_path.size() - 1);
+            if (normalized.compare(0, prefix.size(), prefix.data(),
+                                   prefix.size()) == 0) {
+                return true;
+            }
         }
     }
     return false;

src/http_resource.cpp

@@ -28,6 +28,7 @@
 #include <atomic>
 #include <functional>
 #include <memory>
+#include <shared_mutex>
 #include <stdexcept>
 #include <string>
 #include <utility>
@@ -100,10 +101,10 @@ ensure_table(std::shared_ptr<detail::resource_hook_table>& slot) {
 
 // ---- copy / move special members ----------------------------------------
 //
-// TASK-058 step 3 added std::mutex cached_allow_mutex_, which has no
-// copy or move.  The defaulted special members on the public class
-// declaration would therefore be implicitly deleted.  Implement them
-// here by-hand, skipping the mutex.  The copy / move target starts
+// TASK-058 step 3 added std::shared_mutex cached_allow_mutex_, which
+// has no copy or move.  The defaulted special members on the public
+// class declaration would therefore be implicitly deleted.  Implement
+// them here by-hand, skipping the mutex.  The copy / move target starts
 // with a fresh, default-constructed mutex and an invalidated Allow-
 // header cache (cached_allow_valid_ = false), which the next call to
 // get_allow_header() repopulates lazily.
@@ -120,7 +121,7 @@ http_resource::http_resource(http_resource&& b) noexcept
     : methods_allowed_(b.methods_allowed_),
       hook_table_(std::move(b.hook_table_)) {
     // Same rationale as the copy constructor: cache state is per-
-    // instance and is not transferred.  std::mutex has no move.
+    // instance and is not transferred.  std::shared_mutex has no move.
 }
 
 http_resource& http_resource::operator=(const http_resource& b) noexcept {
@@ -129,7 +130,7 @@ http_resource& http_resource::operator=(const http_resource& b) noexcept {
         methods_allowed_ = b.methods_allowed_;
         // Invalidate the local cache; do NOT touch the mutex (still
         // owned by *this).
-        std::lock_guard<std::mutex> lock(cached_allow_mutex_);
+        std::unique_lock<std::shared_mutex> lock(cached_allow_mutex_);
         cached_allow_valid_ = false;
         cached_allow_header_.clear();
     }
@@ -140,7 +141,7 @@ http_resource& http_resource::operator=(http_resource&& b) noexcept {
     if (this != &b) {
         hook_table_ = std::move(b.hook_table_);
         methods_allowed_ = b.methods_allowed_;
-        std::lock_guard<std::mutex> lock(cached_allow_mutex_);
+        std::unique_lock<std::shared_mutex> lock(cached_allow_mutex_);
         cached_allow_valid_ = false;
         cached_allow_header_.clear();
     }
@@ -204,22 +205,42 @@ hook_handle http_resource::add_hook(hook_phase phase,
 // TASK-058 step 3: lazy Allow-header cache.  See the header-side
 // declaration for the contract.  Implementation:
 //
-//   1. Snapshot the live mask once -- subsequent comparisons run against
-//      this local copy (avoids re-loading methods_allowed_ inside the
-//      critical section).
-//   2. Take the per-resource mutex.  Under contention this serialises
-//      cache-fill; under the warm-path (hit) case the lock is held for
-//      a single integer compare + reference return.
-//   3. If the cached snapshot matches the live mask, return the cached
-//      string by reference.  Otherwise rebuild via detail::format_allow_header
-//      (the same routine the pre-TASK-058 path used) and update the snapshot.
+//   Warm path (cache hit, the common case after the first 405):
+//     1. Take a shared lock.
+//     2. Read methods_allowed_ INSIDE the lock (eliminates the TOCTOU
+//        data race flagged in security-reviewer-iter1-2).
+//     3. Compare with the cached snapshot; if they match, return the
+//        cached string by reference while still holding the shared lock.
+//
+//   Cold / stale path (cache miss or mask changed):
+//     1. Release the shared lock.
+//     2. Take an exclusive (unique) lock.
+//     3. Re-check (double-checked locking) because another thread may
+//        have filled the cache between the two lock acquisitions.
+//     4. Rebuild via detail::format_allow_header and update the snapshot.
+//     5. Return the cached string by reference.
+//
+// Using std::shared_mutex allows N concurrent warm-path readers without
+// serialisation; only the cold fill path serialises
+// (performance-reviewer-iter1-1).
 //
 // The returned reference is stable until the next mask mutation; the
 // caller (dispatch_resource_handler) consumes it synchronously within
 // the current dispatch, so the reference outlives use.
 const std::string& http_resource::get_allow_header() const {
-    const method_set live = methods_allowed_;
-    std::lock_guard<std::mutex> lock(cached_allow_mutex_);
+    // Warm path: shared (read) lock.
+    {
+        std::shared_lock<std::shared_mutex> rlock(cached_allow_mutex_);
+        const method_set live = methods_allowed_;  // read inside lock
+        if (cached_allow_valid_ && cached_allow_mask_ == live) {
+            return cached_allow_header_;
+        }
+    }
+    // Cold / stale path: exclusive (write) lock.
+    std::unique_lock<std::shared_mutex> wlock(cached_allow_mutex_);
+    // Double-checked: another thread may have filled the cache while we
+    // waited for the exclusive lock.
+    const method_set live = methods_allowed_;  // re-read inside write lock
     if (!cached_allow_valid_ || cached_allow_mask_ != live) {
         cached_allow_header_ = detail::format_allow_header(live);
         cached_allow_mask_ = live;

src/httpserver/http_resource.hpp

@@ -28,6 +28,7 @@
 #include <functional>
 #include <memory>
 #include <mutex>
+#include <shared_mutex>
 #include <string>
 
 // TASK-036: render_* virtuals now return http_response by value; the
@@ -234,9 +235,12 @@ class http_resource {
       * disallow_all, and allow_all all invalidate the cache implicitly,
       * without any explicit dependency on the mutation API.
       *
-      * Thread-safe: a per-resource std::mutex serialises the cache
-      * fill / read.  Once the cache is warm, the lock is uncontended
-      * on every subsequent call.
+      * Thread-safe: a per-resource std::shared_mutex allows concurrent
+      * warm-path reads (std::shared_lock) while the cache-fill path
+      * takes an exclusive std::unique_lock.  Under multi-threaded 405
+      * floods the warm path is fully concurrent; only a stale-mask
+      * fill serialises.  The methods_allowed_ snapshot is taken inside
+      * the lock to eliminate the TOCTOU data race.
       *
       * The returned reference is valid until the next mask mutation;
       * callers must not store it beyond a single dispatch invocation.
@@ -308,14 +312,15 @@ class http_resource {
       * are needed, register hooks on the copy separately after construction.
       *
       * TASK-058 step 3: cached_allow_mutex_ is non-copyable / non-movable
-      * (std::mutex has no copy or move).  The copy / move special members
-      * are therefore written by hand and skip the mutex member entirely --
-      * the copy / move target gets a freshly default-constructed mutex
-      * and an invalidated cache (cached_allow_valid_ = false), so the
-      * next get_allow_header() call on the target rebuilds the cache
-      * under its own (fresh) lock.  This is the correct semantic: copy /
-      * move is a structural operation; cache state is local to each
-      * instance and must not be shared by reference.
+      * (std::shared_mutex has no copy or move).  The copy / move special
+      * members are therefore written by hand and skip the mutex member
+      * entirely -- the copy / move target gets a freshly
+      * default-constructed mutex and an invalidated cache
+      * (cached_allow_valid_ = false), so the next get_allow_header()
+      * call on the target rebuilds the cache under its own (fresh) lock.
+      * This is the correct semantic: copy / move is a structural
+      * operation; cache state is local to each instance and must not be
+      * shared by reference.
      **/
      http_resource(const http_resource& b) noexcept;
      http_resource(http_resource&& b) noexcept;
@@ -361,14 +366,17 @@ class http_resource {
      //
      // Thread-safety: the dispatch path can call get_allow_header()
      // concurrently from multiple MHD worker threads against the same
-     // resource.  The mutex serialises the cache fill / read; once the
-     // cache is warm, the lock is uncontended on every subsequent 405.
+     // resource.  std::shared_mutex allows concurrent warm-path reads
+     // (std::shared_lock) while the cache-fill (miss/invalidate) path
+     // takes an exclusive std::unique_lock.  The methods_allowed_
+     // snapshot is taken INSIDE the lock (not before it) to eliminate
+     // the TOCTOU data race noted in security-reviewer-iter1-2.
      //
      // `mutable` for the same reason as hook_table_: the dispatch path
      // calls get_allow_header() on a `const http_resource&` (the cache
      // is logically const-observable -- the visible result is a stable
      // function of methods_allowed_).
-     mutable std::mutex cached_allow_mutex_;
+     mutable std::shared_mutex cached_allow_mutex_;
      mutable std::string cached_allow_header_;
      mutable method_set cached_allow_mask_{};
      mutable bool cached_allow_valid_ = false;
@@ -399,21 +407,21 @@ class http_resource {
 // table PIMPL (shared_ptr<resource_hook_table>) was added later, growing the
 // cap to vptr + shared_ptr + method_set + padding.
 //
-// TASK-058 step 3 added a lazy Allow-header cache: std::mutex +
-// std::string + method_set + bool.  std::mutex is a sizeof~64 storage on
-// pthread platforms (libc++/macOS) and ~40 on libstdc++/Linux; std::string
-// SBO adds ~24-32; the method_set / bool fields slot into existing padding.
-// The new ceiling is the old (3*void* + 2*method_set) plus the new cache
-// payload, padded for alignment.  See test/bench_sizeof_http_resource.cpp
-// for the v1-anchored algebra; the value here documents the
-// post-TASK-058 layout shape.
+// TASK-058 step 3 added a lazy Allow-header cache: std::shared_mutex +
+// std::string + method_set + bool.  std::shared_mutex is a sizeof~56
+// storage on pthread platforms (libc++/macOS) and ~56 on libstdc++/Linux
+// (similar to std::mutex); std::string SBO adds ~24-32; the method_set /
+// bool fields slot into existing padding.  The new ceiling is the old
+// (3*void* + 2*method_set) plus the new cache payload, padded for
+// alignment.  See test/bench_sizeof_http_resource.cpp for the v1-anchored
+// algebra; the value here documents the post-TASK-058 layout shape.
 static_assert(sizeof(http_resource) <=
                   sizeof(void*) * 3 + sizeof(method_set) * 2
-                  + sizeof(std::mutex) + sizeof(std::string)
+                  + sizeof(std::shared_mutex) + sizeof(std::string)
                   + sizeof(method_set) + sizeof(bool) * 8,
               "http_resource should be approximately vptr + shared_ptr + "
               "method_set (TASK-051) + Allow-header cache "
-              "(mutex + string + method_set + bool) after TASK-058 step 3");
+              "(shared_mutex + string + method_set + bool) after TASK-058 step 3");
 
 }  // namespace httpserver
 #endif  // SRC_HTTPSERVER_HTTP_RESOURCE_HPP_

test/bench_sizeof_http_resource.cpp

@@ -32,6 +32,7 @@
 
 #include <cstddef>
 #include <mutex>
+#include <shared_mutex>
 #include <string>
 
 #include "httpserver/http_method.hpp"
@@ -53,24 +54,29 @@ using httpserver::v1_baseline::V1_STD_MAP_STRING_BOOL_SIZEOF;
 //   reduction := V1_HTTP_RESOURCE_SIZEOF - sizeof(http_resource)
 // must satisfy
 //   reduction + sizeof(method_set) * 2 + sizeof(void*) * 2
-//             + sizeof(std::mutex) + sizeof(std::string)
+//             + sizeof(std::shared_mutex) + sizeof(std::string)
 //             + sizeof(method_set) + sizeof(bool) * 8
 //        >= V1_STD_MAP_STRING_BOOL_SIZEOF
 //
 // Terms:
-//   sizeof(method_set) * 2  -- own_size + worst-case alignment padding
-//                              for the methods_allowed_ member.
-//   sizeof(void*) * 2       -- shared_ptr<resource_hook_table> hook_table_
-//                              member (TASK-051 hook PIMPL slot).
-//   sizeof(std::mutex)      -- TASK-058 step 3: per-resource Allow-header
-//                              cache lock.  pthread_mutex_t on macOS
-//                              (libc++) is ~64 bytes; libstdc++ is ~40.
-//   sizeof(std::string)     -- TASK-058 step 3: cached Allow header
-//                              storage.  SBO header is ~24-32 bytes.
-//   sizeof(method_set)      -- TASK-058 step 3: cached mask snapshot.
-//   sizeof(bool) * 8        -- TASK-058 step 3: cached_allow_valid_ + worst-
-//                              case alignment padding (8 bytes; mutex
-//                              forces 8-byte alignment after a single bool).
+//   sizeof(method_set) * 2      -- own_size + worst-case alignment padding
+//                                  for the methods_allowed_ member.
+//   sizeof(void*) * 2           -- shared_ptr<resource_hook_table> hook_table_
+//                                  member (TASK-051 hook PIMPL slot).
+//   sizeof(std::shared_mutex)   -- TASK-058 step 3 / security-reviewer-iter1-2
+//                                  / performance-reviewer-iter1-1: per-resource
+//                                  Allow-header cache lock upgraded from
+//                                  std::mutex to std::shared_mutex to allow
+//                                  concurrent warm-path reads.
+//                                  macOS (libc++): ~168 bytes.
+//                                  Linux (libstdc++): ~56 bytes.
+//   sizeof(std::string)         -- TASK-058 step 3: cached Allow header
+//                                  storage.  SBO header is ~24-32 bytes.
+//   sizeof(method_set)          -- TASK-058 step 3: cached mask snapshot.
+//   sizeof(bool) * 8            -- TASK-058 step 3: cached_allow_valid_ +
+//                                  worst-case alignment padding (8 bytes;
+//                                  mutex forces 8-byte alignment after a
+//                                  single bool).
 //
 // The V1_* constants are selected at compile time by the detected
 // C++ standard library (libc++ vs. libstdc++) in v1_constants.hpp,
@@ -84,18 +90,19 @@ using httpserver::v1_baseline::V1_STD_MAP_STRING_BOOL_SIZEOF;
 static_assert(sizeof(http_resource) + V1_STD_MAP_STRING_BOOL_SIZEOF
               <= V1_HTTP_RESOURCE_SIZEOF + sizeof(method_set) * 2
                   + sizeof(void*) * 2
-                  + sizeof(std::mutex) + sizeof(std::string)
+                  + sizeof(std::shared_mutex) + sizeof(std::string)
                   + sizeof(method_set) + sizeof(bool) * 8,
               "http_resource grew beyond the documented PRD-REQ-REQ-003 / "
               "TASK-051 / TASK-058 step 3 envelope (method_set + per-route "
               "hook PIMPL slot + Allow header cache)");
 
 // Belt-and-suspenders: regardless of v1 anchoring, v2.0 must be no
 // larger than v1 + the documented hook PIMPL slot + the TASK-058 step 3
-// Allow-header cache payload.
+// Allow-header cache payload (upgraded to std::shared_mutex for
+// security-reviewer-iter1-2 / performance-reviewer-iter1-1 fix).
 static_assert(sizeof(http_resource) <=
                   V1_HTTP_RESOURCE_SIZEOF + sizeof(void*) * 2
-                  + sizeof(std::mutex) + sizeof(std::string)
+                  + sizeof(std::shared_mutex) + sizeof(std::string)
                   + sizeof(method_set) + sizeof(bool) * 8,
               "http_resource grew beyond v1 + TASK-051 hook PIMPL slot + "
               "TASK-058 step 3 Allow cache payload");