|
| 1 | +# Unworked Review Issues |
| 2 | + |
| 3 | +**Run:** 2026-05-07 00:00:00 |
| 4 | +**Task:** TASK-019 |
| 5 | +**Total:** 20 (0 critical, 1 major deferred, 19 minor) |
| 6 | + |
| 7 | +## Major Deferred (acknowledged scope deferrals) |
| 8 | + |
| 9 | +1. [ ] **code-simplifier** | `src/http_request.cpp:518` | code-structure |
| 10 | + The Subject DN and Issuer DN extraction in populate_all_cert_fields (lines 518-540) are structurally identical two-call GnuTLS blocks: size-probe, allocate, fill, strip trailing NUL, assign to pmr::string. This pattern repeats twice verbatim with only the GnuTLS function name and destination field differing. The Common Name block (lines 542-553) is a third near-copy. |
| 11 | + *Recommendation:* Extract a static file-scope helper inside the #ifdef HAVE_GNUTLS block: `static std::string extract_dn(gnutls_x509_crt_t cert, int(*getter)(gnutls_x509_crt_t, char*, size_t*))`. The code-simplifier agent returned `approve` — this is a non-blocking simplification opportunity deferred to a future task. |
| 12 | + |
| 13 | +## Minor |
| 14 | + |
| 15 | +2. [ ] **code-simplifier** | `src/http_request.cpp:964` | code-structure |
| 16 | + The four string_view accessor bodies (get_client_cert_dn, get_client_cert_issuer_dn, get_client_cert_cn, get_client_cert_fingerprint_sha256) are structurally identical: `#ifdef HAVE_GNUTLS`, call populate_all_cert_fields(), return `std::string_view(impl_->field.data(), impl_->field.size())`, `#else` return `std::string_view{}`, `#endif`. |
| 17 | + *Recommendation:* A small private helper or macro (e.g. `CERT_STRING_VIEW_ACCESSOR(name, field)`) would collapse all four bodies to a one-liner, matching the existing COMPARATOR macro style in http_utils.hpp. |
| 18 | + |
| 19 | +3. [ ] **code-simplifier** | `src/http_request.cpp:494` | code-structure |
| 20 | + In populate_all_cert_fields, the code first calls has_tls_session() then separately calls get_tls_session(). But get_tls_session() already checks connection_ == nullptr and returns nullptr on miss, making the outer has_tls_session() guard partially redundant. |
| 21 | + *Recommendation:* Simplify to `gnutls_session_t session = get_tls_session();` directly, removing the has_tls_session() call from populate_all_cert_fields entirely. |
| 22 | + |
| 23 | +4. [ ] **code-simplifier** | `src/http_request.cpp:557` | naming |
| 24 | + The local array `unsigned char fingerprint[32]` uses a magic number 32 for the SHA-256 digest size. |
| 25 | + *Recommendation:* Replace the literal 32 with a named constant such as `static constexpr size_t SHA256_DIGEST_BYTES = 32;` inside the anonymous namespace. |
| 26 | + |
| 27 | +5. [ ] **code-quality-reviewer** | `src/http_request.cpp:519` | code-elegance |
| 28 | + The GnuTLS two-call DN/CN extraction pattern is repeated three times (DN, issuer DN, CN) with nearly identical code. |
| 29 | + *Recommendation:* Extract a named lambda or private helper (e.g., `read_gnutls_string`) that takes the GnuTLS getter function pointer and the destination pmr::string&. |
| 30 | + |
| 31 | +6. [ ] **code-quality-reviewer** | `src/httpserver/http_utils.hpp:372` | code-readability |
| 32 | + In `arg_comparator::operator()(const std::string& x, std::string_view y)` the call constructs a heap-allocated `std::string` from a `string_view` unnecessarily; other three overloads delegate to `(string_view, string_view)`. Pre-existing issue from a prior task. |
| 33 | + *Recommendation:* Change to delegate to `operator()(std::string_view(x), y)` rather than `operator()(std::string_view(x), std::string(y))`. |
| 34 | + |
| 35 | +7. [ ] **code-quality-reviewer** | `test/unit/http_request_tls_accessors_test.cpp:1` | test-coverage |
| 36 | + No runtime test covers the noexcept try/catch wrappers in `is_client_cert_verified`, `get_client_cert_not_before`, and `get_client_cert_not_after` under an allocation-failure scenario. |
| 37 | + *Recommendation:* Add a comment noting that the try/catch sentinel path is not covered by automated tests since injecting bad_alloc in GnuTLS context is impractical in CI. |
| 38 | + |
| 39 | +8. [ ] **code-quality-reviewer** | `src/http_request.cpp:948` | code-readability |
| 40 | + The comment block preceding the TASK-019 public accessor implementations is verbose (12 lines) and restates design decisions already captured in the task spec and architecture doc. |
| 41 | + *Recommendation:* Condense to 3-4 lines covering only the non-obvious decisions (unconditional public symbols, noexcept try/catch rationale). |
| 42 | + |
| 43 | +9. [ ] **security-reviewer** | `src/http_request.cpp:492` | insecure-design |
| 44 | + In populate_all_cert_fields(), client_cert_fields_cached is set to true before any cert field is populated. If std::bad_alloc is thrown during string allocation, the cached flag is already true, creating an asymmetric exception contract between noexcept and string_view accessors. |
| 45 | + *Recommendation:* Move client_cert_fields_cached = true to after all field population is complete, or catch per-field allocation failures individually. Alternatively, add try/catch in the four string_view accessors to match the noexcept accessors' contract. |
| 46 | + |
| 47 | +10. [ ] **security-reviewer** | `examples/client_cert_auth.cpp:103` | insecure-design |
| 48 | + The time validity check is correctly guarded by has_client_certificate() and is_client_cert_verified() checks earlier, but this guard dependency is not documented. A developer copying only the time-checking snippet would be vulnerable: when no cert is present not_after is -1, and any non-negative now would satisfy now > not_after. |
| 49 | + *Recommendation:* Add an inline comment noting that the time checks must only be reached after has_client_certificate() returns true. |
| 50 | + |
| 51 | +11. [ ] **security-reviewer** | `src/http_request.cpp:1108` | logging-failures |
| 52 | + The operator<< diagnostic formatter logs the Basic Auth password in plaintext. Pre-existing behavior not introduced by TASK-019. New cert fields (DN, CN, fingerprint) were correctly excluded from the formatter. |
| 53 | + *Recommendation:* Pre-existing issue outside TASK-019 scope. |
| 54 | + |
| 55 | +12. [ ] **performance-reviewer** | `src/http_request.cpp:467` | missing-caching |
| 56 | + has_client_certificate() calls gnutls_certificate_get_peers() on every invocation with no caching. If a handler calls this predicate more than once per request, the peer-certificate list is fetched from GnuTLS multiple times. |
| 57 | + *Recommendation:* Add a mutable tri-state cache field (e.g. `mutable std::optional<bool> client_cert_present_cached_`) to http_request_impl. |
| 58 | + |
| 59 | +13. [ ] **performance-reviewer** | `src/http_request.cpp:561` | memory-allocation |
| 60 | + The SHA-256 fingerprint is assembled into a default-heap std::string (hex_fingerprint) then .assign()'d into the arena-backed pmr::string, causing one extra heap allocation and a byte-by-byte copy. |
| 61 | + *Recommendation:* Reserve client_cert_fingerprint_sha256 to 64 chars upfront and write hex digits directly into it using a scratch buffer, bypassing the intermediate std::string. |
| 62 | + |
| 63 | +14. [ ] **performance-reviewer** | `src/http_request.cpp:495` | resource-leak |
| 64 | + populate_all_cert_fields() calls has_tls_session() then get_tls_session() as two separate MHD_get_connection_info lookups inside what is already a once-cached function. |
| 65 | + *Recommendation:* Call get_tls_session() once, assign to a local, and check for null to infer the has_tls_session() condition. |
| 66 | + |
| 67 | +15. [ ] **architecture-alignment-checker** | `src/httpserver/http_utils.hpp:50` | pattern-violation |
| 68 | + http_utils.hpp includes <microhttpd.h> unconditionally at the public-header level. Pre-existing issue not introduced by TASK-019; MHD types are supposed to appear only in detail/ headers and .cpp files per §4.1 and §4.2. |
| 69 | + *Recommendation:* Track as a separate cleanup task to move the <microhttpd.h> dependency behind HTTPSERVER_COMPILATION. TASK-019 itself correctly excludes <gnutls/gnutls.h>. |
| 70 | + |
| 71 | +16. [ ] **spec-alignment-checker** | `test/integ/ws_start_stop.cpp:1057` | acceptance-criteria |
| 72 | + The `client_cert_with_certificate` test checks only that `CN:Test Client` appears and that a fingerprint field is present, not a specific known fingerprint value. The `client_cert_fingerprint` test verifies length is 64 hex chars but not exact value. |
| 73 | + *Recommendation:* Document that length+format verification against a known cert is sufficient, or add a test fixture that embeds the expected SHA-256 fingerprint. |
| 74 | + |
| 75 | +17. [ ] **spec-alignment-checker** | `src/httpserver/create_test_request.hpp:115` | ears-requirement |
| 76 | + The `tls_enabled()` builder method is still gated on `#ifdef HAVE_GNUTLS`, violating PRD-FLG-REQ-001 which requires no HAVE_* gates in public headers. |
| 77 | + *Recommendation:* Make `tls_enabled()` unconditional (the impl guard in http_request.cpp is sufficient), or track this as deferred to the API-FLG milestone task. |
| 78 | + |
| 79 | +18. [ ] **spec-alignment-checker** | `src/httpserver/http_request.hpp:126` | specification-gap |
| 80 | + `http_request.hpp` still has `#ifdef HAVE_BAUTH` and `#ifdef HAVE_DAUTH` gates around auth accessors, violating PRD-FLG-REQ-001. TASK-019 correctly removed the HAVE_GNUTLS gate (its direct scope) but did not address these remaining gates. |
| 81 | + *Recommendation:* Add a TODO comment cross-referencing the API-FLG milestone task so the remaining PRD-FLG-REQ-001 violation is tracked. |
| 82 | + |
| 83 | +19. [ ] **test-quality-reviewer** | `test/integ/ws_start_stop.cpp:0` | logic-in-test |
| 84 | + The fingerprint validation loop `for (char c : fp)` with an if-based validity check introduces control flow logic in test code. |
| 85 | + *Recommendation:* Replace with `std::all_of` and a lambda, or a regex match against `^[0-9a-f]{64}$`. |
| 86 | + |
| 87 | +20. [ ] **test-quality-reviewer** | `test/unit/create_test_request_test.cpp:198` | redundant-test |
| 88 | + `build_tls_enabled_no_peer_cert` and `build_no_client_cert_returns_sentinels` both exercise the same code path with the same sentinel assertions. |
| 89 | + *Recommendation:* Collapse into a single test: keep `build_no_client_cert_returns_sentinels` (ungated) and expand it with a `tls_enabled()` builder block. |
| 90 | + |
| 91 | +21. [ ] **test-quality-reviewer** | `test/unit/http_request_tls_accessors_test.cpp:0` | missing-test |
| 92 | + The compile-time test file has no corresponding no-GNUTLS compile target in test/Makefile.am. The HAVE_GNUTLS=off sentinel path is contractually guaranteed but not compile-tested. |
| 93 | + *Recommendation:* Add a second Makefile.am entry compiled with -UHAVE_GNUTLS, or document that the no-GNUTLS path is covered by the unconditional runtime sentinels in create_test_request_test.cpp. |
0 commit comments