|
| 1 | +# Unworked Review Issues |
| 2 | + |
| 3 | +**Run:** 2026-05-06 00:00:00 |
| 4 | +**Task:** TASK-018 |
| 5 | +**Total:** 22 (0 critical, 1 major, 21 minor) |
| 6 | + |
| 7 | +## Major |
| 8 | + |
| 9 | +1. [ ] **security-reviewer** | `src/http_request.cpp:812` | data-integrity |
| 10 | + get_arg_flat() returns a string_view that aliases a pmr::string inside impl_->unescaped_args (line 818: 'return it->second[0];'). If unescaped_args is ever mutated after this call returns but before the caller finishes using the view (e.g. a concurrent request handler sharing the same http_request*, or test code that calls set_arg after get_arg_flat), the view dangles. The public header comment lists get_arg_flat() in the affected-getters list (CWE-416) but the comment is the only guard. The same applies to the string_views in get_args_flat() (line 849: 'ret[key] = val[0]') which aliases unescaped_args values directly. No compile-time or runtime enforcement prevents a caller from storing the view past the mutation point. |
| 11 | + *Recommendation:* No code change is strictly required for single-threaded handler use, which is the documented contract. However, add a [[nodiscard]] attribute to get_arg_flat() and get_args_flat() so callers are nudged to consume the result immediately. Also consider a debug-mode assertion that args_populated is true and no set_arg mutation can occur after population (i.e., once args_populated is set, lock the map). For the multi-value get_arg() path the design is safe because it copies into http_arg_value; document that divergence explicitly. |
| 12 | + |
| 13 | +## Minor |
| 14 | + |
| 15 | +1. [ ] **architecture-alignment-checker** | `src/httpserver/http_request.hpp:95` | pattern-violation |
| 16 | + The class-level Doxygen block (TASK-016 era) still says 'All std::string_view values returned by this class are only valid within the handler's call frame', while all TASK-018 per-method @note comments say 'valid for the lifetime of the request object'. The task spec action item explicitly says 'Document lifetime: the view is valid for the lifetime of the request object'. The two statements are not contradictory in practice (the handler invocation is the request lifetime on the live path), but the class-level wording is more restrictive than necessary and is now inconsistent with the per-method documentation added in this task. |
| 17 | + *Recommendation:* Update the class-level string_view lifetime contract block to use the same 'valid for the lifetime of the request object (typically the duration of the handler invocation)' phrasing used in the per-method @note comments, so the two levels of documentation agree. |
| 18 | + |
| 19 | +2. [ ] **code-quality-reviewer** | `src/httpserver/http_request.hpp:269` | code-readability |
| 20 | + get_cookie's doc block is minimal compared to every other per-key getter in this file. It has only a @note but no @param or @return line, while get_header (line 261) and get_footer (line 273) both document their parameter and return value. |
| 21 | + *Recommendation:* Add '@param key the specific cookie to get the value from' and '@return the value of the cookie.' lines to align with the surrounding getter docs. |
| 22 | + |
| 23 | +3. [ ] **code-quality-reviewer** | `src/http_request.cpp:862` | code-elegance |
| 24 | + get_querystring() is declared noexcept but its body dereferences impl_->querystring through a std::unique_ptr with a custom deleter. If impl_ is null (default-constructed http_request), this is UB rather than a thrown exception, so noexcept is technically correct but the safety guarantee is implicit. A brief comment noting why the impl_ deref is safe here would clarify intent. |
| 25 | + *Recommendation:* Add a one-line comment above the return such as '// impl_ is always non-null on live requests; default-ctor is private.' to make the safety contract explicit at the call site. |
| 26 | + |
| 27 | +4. [ ] **code-quality-reviewer** | `test/unit/create_test_request_test.cpp:338` | test-coverage |
| 28 | + The missing_key_does_not_insert test verifies non-insertion via the container-cache size, which is an indirect proxy. It doesn't exercise the case where get_arg_flat falls through to get_connection_value on a live-connection path. |
| 29 | + *Recommendation:* The current test is adequate for the test-request path. Consider a comment noting the MHD-path fallback in get_arg_flat is exercised by integration tests, or add a TODO tracking a future unit-test for that branch. |
| 30 | + |
| 31 | +5. [ ] **code-quality-reviewer** | `test/unit/http_request_const_getters_test.cpp:46` | code-readability |
| 32 | + The type alias 'cref' could be confused with std::cref from <functional>. In a type-traits-heavy file, slightly more descriptive aliases would improve legibility. |
| 33 | + *Recommendation:* Consider renaming 'cref' to 'const_req_ref' or 'req_const_ref' to avoid any confusion with std::cref. |
| 34 | + |
| 35 | +6. [ ] **code-simplifier** | `src/http_request.cpp:812` | code-structure |
| 36 | + get_arg_flat falls through to get_connection_value(key, MHD_GET_ARGUMENT_KIND) on a miss, but MHD_GET_ARGUMENT_KIND is not handled in the switch inside get_connection_value (only HEADER/FOOTER/COOKIE kinds are switched). On the live path MHD_lookup_connection_value is called with MHD_GET_ARGUMENT_KIND, which could return the raw (non-unescaped) string, creating a subtle behavioural inconsistency between the populated-args path and the fallback path. |
| 37 | + *Recommendation:* Simplify the fallback to just `return EMPTY;` (or `return std::string_view{};`) instead of delegating to get_connection_value with a kind that has no consistent meaning here. |
| 38 | + |
| 39 | +7. [ ] **code-simplifier** | `src/httpserver/http_request.hpp:434` | naming |
| 40 | + The free-function `operator<<` is declared twice with different signatures: once as a friend inside the class body (non-const reference) and once at namespace scope (const reference). These are two different overloads; the friend declaration does not match the namespace-scope declaration. This is a pre-existing issue but is in a changed file. |
| 41 | + *Recommendation:* Change the friend declaration to `friend std::ostream& operator<<(std::ostream& os, const http_request& r);` to match the namespace-scope declaration and the definition in http_request.cpp. |
| 42 | + |
| 43 | +8. [ ] **code-simplifier** | `src/httpserver/http_request.hpp:320` | code-structure |
| 44 | + The `@note (TASK-018)` tag in the get_querystring doc comment encodes a task-tracking reference into a user-facing API doc comment. Task numbers are internal implementation history, not part of the public API contract a downstream consumer needs to know. |
| 45 | + *Recommendation:* Remove the `@note (TASK-018)` prefix so the sentence reads simply: "The querystring is assembled eagerly at construction on the live MHD path, so the public reader is `noexcept`." |
| 46 | + |
| 47 | +9. [ ] **performance-reviewer** | `src/http_request.cpp:150` | memory-allocation |
| 48 | + get_connection_value constructs a temporary std::string(key) for map::find on the test-request path (connection_ == nullptr). On the live MHD path there is no allocation, but repeated per-key misses in tests will each pay one small-string allocation per call. |
| 49 | + *Recommendation:* Switch headers_local / footers_local / cookies_local to use a transparent-comparator variant so map::find(std::string_view) is heterogeneous and removes the temporary std::string construction on every test-path per-key lookup. |
| 50 | + |
| 51 | +10. [ ] **performance-reviewer** | `src/http_request.cpp:812` | missing-caching |
| 52 | + get_arg_flat falls through to get_connection_value(key, MHD_GET_ARGUMENT_KIND) when the key is absent from unescaped_args. MHD_GET_ARGUMENT_KIND is not one of the three kinds that ensure_headerlike_cache handles, so the call bypasses the one-time-fill cache and issues a raw MHD_lookup_connection_value call on every miss — an O(n) linear scan. |
| 53 | + *Recommendation:* Either extend ensure_headerlike_cache to cover MHD_GET_ARGUMENT_KIND, or simply remove the MHD fallback since populate_args() already iterates MHD_GET_ARGUMENT_KIND and stores all arguments in unescaped_args — if the key is absent after populate_args(), MHD will also not find it. |
| 54 | + |
| 55 | +11. [ ] **performance-reviewer** | `src/http_request.cpp:797` | memory-allocation |
| 56 | + get_arg copies the found pmr::vector<pmr::string> values element-by-element into a freshly heap-allocated http_arg_value::values on every call with a hit. The reserve is already present; mark as low priority. |
| 57 | + *Recommendation:* If get_arg is called frequently, consider returning const http_arg_value& by reference from a pre-built view in args_view_cached_. Low priority since the reserve is already present. |
| 58 | + |
| 59 | +12. [ ] **performance-reviewer** | `src/http_request.cpp:845` | algorithmic-complexity |
| 60 | + get_args_flat reconstructs a std::map<string_view, string_view> from scratch on every call, iterating all of unescaped_args. Unlike get_args() there is no cache for the flat view. |
| 61 | + *Recommendation:* Add a mutable flat_args_view_cached_ cache similar to args_view_cached_ in http_request_impl and populate it once on the first get_args_flat() call. Alternatively, document that callers should capture the result by value once and reuse it. |
| 62 | + |
| 63 | +13. [ ] **security-reviewer** | `src/http_request.cpp:205` | data-integrity (CWE-362) |
| 64 | + ensure_headerlike_cache() and populate_args() use a plain non-atomic bool guard to implement a lazy-fill pattern on mutable members accessed through a const pointer. If two threads invoke const getters concurrently on the same http_request, there is a data race on the bool and on the cache map itself. |
| 65 | + *Recommendation:* Document explicitly in the class-level Doxygen that http_request instances are not thread-safe and must not be accessed from multiple threads simultaneously. Extend the existing comment at lines 96-131 of the header with a thread-safety note. |
| 66 | + |
| 67 | +14. [ ] **security-reviewer** | `src/http_request.cpp:156` | input-validation |
| 68 | + get_connection_value() passes key.data() directly to MHD_lookup_connection_value(). std::string_view is not guaranteed to be null-terminated, so if a caller passes a non-terminated view, MHD will read past the intended end of the key. |
| 69 | + *Recommendation:* Convert key to a null-terminated string before passing to MHD: 'std::string key_str(key); MHD_lookup_connection_value(connection_, kind, key_str.c_str())'. This is a one-time allocation per lookup and preserves correctness. |
| 70 | + |
| 71 | +15. [ ] **security-reviewer** | `src/http_request.cpp:845` | data-integrity (CWE-416) |
| 72 | + get_args_flat() returns a by-value std::map<string_view, string_view>. The string_view keys and values alias pmr::string storage inside impl_->unescaped_args. The returned map is not listed in the header comment's 'affected getters' list, meaning callers may not know those views carry the same dangling risk. |
| 73 | + *Recommendation:* Add get_args_flat() to the list of affected getters in the class Doxygen block (http_request.hpp lines 109-111). Documentation-only fix. |
| 74 | + |
| 75 | +16. [ ] **security-reviewer** | `src/httpserver/detail/http_request_impl.hpp:284` | input-validation |
| 76 | + The arguments_accumulator DoS guard defaults (DEFAULT_MAX_ARGS_COUNT=64, DEFAULT_MAX_ARGS_BYTES=65536) are used on the test-request path without an explicit accumulator configuration. Until M5 lands, the limits are the global defaults with no per-webserver configurability. |
| 77 | + *Recommendation:* Known deferred item (TODO M5). No change required in this task. Verify the TODO is tracked and that the defaults are conservative enough for the deployed threat model. |
| 78 | + |
| 79 | +17. [ ] **spec-alignment-checker** | `src/httpserver/http_request.hpp:286` | action-item |
| 80 | + Action item states 'Document lifetime: the view is valid for the lifetime of the request object'. get_arg() returns http_arg_value (a struct with std::vector<std::string_view> values). The class-level doc block does not explicitly list 'get_arg()' even though the string_view values inside http_arg_value carry the same arena-lifetime constraint. |
| 81 | + *Recommendation:* Add a @note to get_arg()'s doxygen comment or to the class-level contract block making explicit that the string_view values inside the returned http_arg_value alias the request's arena storage and must not outlive the handler invocation. |
| 82 | + |
| 83 | +18. [ ] **spec-alignment-checker** | `src/httpserver/http_request.hpp:241` | specification-gap |
| 84 | + get_args_flat() is a container-level getter that returns const std::map<string_view, string_view> by value (not by const&). PRD-REQ-REQ-001 requires 'get_args' returns const&, and the existing get_args() correctly does so, but get_args_flat() still returns by value. This pre-dates TASK-018 and is outside the strict task scope. |
| 85 | + *Recommendation:* Confirm that get_args_flat() is intentionally out of scope for TASK-018 and tracked by a separate task. If so, add a TODO comment referencing the follow-up task. |
| 86 | + |
| 87 | +19. [ ] **test-quality-reviewer** | `test/unit/create_test_request_test.cpp:355` | logic-in-test |
| 88 | + The missing_key_does_not_insert test uses a for-loop (i = 0..4) to repeat five identical miss assertions. Loop logic in tests obscures intent and makes failures harder to attribute to a specific iteration. |
| 89 | + *Recommendation:* Either remove the loop and assert once (one miss is sufficient to prove the invariant), or use the littletest parameterisation pattern if available. |
| 90 | + |
| 91 | +20. [ ] **test-quality-reviewer** | `test/unit/create_test_request_test.cpp:112` | missing-test |
| 92 | + get_arg_flat is only tested against a single-value arg. The task's acceptance criterion for get_arg_flat is 'if the arg key has more than one value, only one is returned', but no test exercises a multi-value arg through get_arg_flat to confirm first-value semantics are preserved. |
| 93 | + *Recommendation:* Add a test case that builds a request with .arg("k","first").arg("k","second"), then asserts get_arg_flat("k") == "first". |
| 94 | + |
| 95 | +21. [ ] **test-quality-reviewer** | `test/unit/http_request_const_getters_test.cpp:130` | missing-test |
| 96 | + The compile-time sentinel does not assert that the per-key getters (get_header / get_footer / get_cookie / get_arg_flat) are declared const using a method-pointer form. std::is_invocable_v only verifies the function can be called with those argument types but does not distinguish const vs non-const overloads when both could theoretically exist. |
| 97 | + *Recommendation:* Add static_asserts of the form static_assert(std::is_same_v<decltype(&h::get_header), std::string_view (h::*)(std::string_view) const>, ...) for each per-key getter to lock the const-qualifier into the type signature. |
0 commit comments