TASK-012: review-pass fixes (security: header injection / simplify: & overload helper)

- Add CRLF/NUL validation to with_header, with_footer, with_cookie:
  reject key or value containing \r, \n, or \0 via std::invalid_argument
  (CWE-113 header injection, security-reviewer-iter1-1/2/3)
- Add range validation to with_status: reject codes outside [100,599]
  per RFC 9110 §15 (security-reviewer-iter1-4)
- Refactor 8 overload pairs to delegate to private do_set_* helpers,
  eliminating duplicated mutation logic (code-simplifier-iter1-1)
- Add 16 new unit tests covering CRLF/NUL rejection and status bounds,
  following the TDD RED→GREEN cycle

Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>

Author: etr

src/http_response.cpp

@@ -192,53 +192,113 @@ void http_response::shoutCAST() {
 // -----------------------------------------------------------------------
 // Fluent with_* setters (TASK-012, PRD-RSP-REQ-004).
 //
-// Each setter has two ref-qualified overloads. The bodies are a one-line
-// mutation (insert_or_assign for the maps, plain assignment for status),
-// followed by `return *this` (& overload) or `return std::move(*this)`
-// (&& overload). insert_or_assign — rather than `m[k] = v` — is used so
-// the by-value `std::string` parameters can be moved into the map slot
-// directly, avoiding an extra string copy at the insertion site when
-// the caller hands the setter an rvalue.
+// Each setter has two ref-qualified overloads that delegate to a private
+// do_set_*() helper containing the validation + mutation logic. The
+// overloads differ only in their return statement: `& overload` returns
+// *this by lvalue reference; `&& overload` returns std::move(*this).
+// Centralising the mutation in a single helper means validation and
+// insert_or_assign are in exactly one place per setter, not duplicated
+// across every overload pair.
+//
+// Validation (security, TASK-012 review-pass):
+//   * with_header / with_footer: reject key or value containing CR,
+//     LF, or NUL — these characters can split an HTTP response and
+//     inject additional headers (CWE-113).
+//   * with_cookie: same CRLF/NUL rejection on name and value.
+//   * with_status: code must be in [100, 599] per RFC 9110 §15.
+//
+// insert_or_assign — rather than `m[k] = v` — is used so the by-value
+// `std::string` parameters can be moved into the map slot directly.
 // -----------------------------------------------------------------------
+
+// Shared forbidden-character set for header/footer/cookie field names
+// and values. The string_view spans all three bytes including the
+// embedded NUL.
+namespace {
+constexpr std::string_view kForbiddenFieldChars("\r\n\0", 3);
+
+void validate_header_field(std::string_view context,
+                           std::string_view key,
+                           std::string_view value) {
+    if (key.find_first_of(kForbiddenFieldChars) != std::string_view::npos) {
+        throw std::invalid_argument(
+            std::string(context) +
+            ": key contains forbidden control character (CR, LF, or NUL)");
+    }
+    if (value.find_first_of(kForbiddenFieldChars) != std::string_view::npos) {
+        throw std::invalid_argument(
+            std::string(context) +
+            ": value contains forbidden control character (CR, LF, or NUL)");
+    }
+}
+}  // namespace
+
+void http_response::do_set_header(std::string key, std::string value) {
+    validate_header_field("with_header", key, value);
+    headers_.insert_or_assign(std::move(key), std::move(value));
+}
+
+void http_response::do_set_footer(std::string key, std::string value) {
+    validate_header_field("with_footer", key, value);
+    footers_.insert_or_assign(std::move(key), std::move(value));
+}
+
+void http_response::do_set_cookie(std::string key, std::string value) {
+    validate_header_field("with_cookie", key, value);
+    cookies_.insert_or_assign(std::move(key), std::move(value));
+}
+
+void http_response::do_set_status(int code) {
+    if (code < 100 || code > 599) {
+        throw std::invalid_argument(
+            "with_status: HTTP status code out of range [100, 599]");
+    }
+    status_code_ = code;
+}
+
 http_response& http_response::with_header(std::string key,
                                           std::string value) & {
-    headers_.insert_or_assign(std::move(key), std::move(value));
+    do_set_header(std::move(key), std::move(value));
     return *this;
 }
+
 http_response&& http_response::with_header(std::string key,
                                            std::string value) && {
-    headers_.insert_or_assign(std::move(key), std::move(value));
+    do_set_header(std::move(key), std::move(value));
     return std::move(*this);
 }
 
 http_response& http_response::with_footer(std::string key,
                                           std::string value) & {
-    footers_.insert_or_assign(std::move(key), std::move(value));
+    do_set_footer(std::move(key), std::move(value));
     return *this;
 }
+
 http_response&& http_response::with_footer(std::string key,
                                            std::string value) && {
-    footers_.insert_or_assign(std::move(key), std::move(value));
+    do_set_footer(std::move(key), std::move(value));
     return std::move(*this);
 }
 
 http_response& http_response::with_cookie(std::string key,
                                           std::string value) & {
-    cookies_.insert_or_assign(std::move(key), std::move(value));
+    do_set_cookie(std::move(key), std::move(value));
     return *this;
 }
+
 http_response&& http_response::with_cookie(std::string key,
                                            std::string value) && {
-    cookies_.insert_or_assign(std::move(key), std::move(value));
+    do_set_cookie(std::move(key), std::move(value));
     return std::move(*this);
 }
 
 http_response& http_response::with_status(int code) & {
-    status_code_ = code;
+    do_set_status(code);
     return *this;
 }
+
 http_response&& http_response::with_status(int code) && {
-    status_code_ = code;
+    do_set_status(code);
     return std::move(*this);
 }
 

src/httpserver/http_response.hpp

@@ -353,6 +353,16 @@ class http_response {
      void destroy_body() noexcept;
      void adopt_body_from(http_response& o) noexcept;
 
+     // Shared mutation helpers for the fluent setters (TASK-012
+     // review-pass). Each helper validates its inputs, then performs the
+     // map mutation or scalar assignment.  Centralising the logic here
+     // means the & and && overloads only differ in their return
+     // statement; the mutation + validation is in exactly one place.
+     void do_set_header(std::string key, std::string value);
+     void do_set_footer(std::string key, std::string value);
+     void do_set_cookie(std::string key, std::string value);
+     void do_set_status(int code);
+
      // Placement-new a concrete detail::body subclass into the SBO
      // buffer (or, if T does not fit, onto the heap via the matched
      // ::operator new(sizeof(T))/::operator delete pairing the

test/unit/http_response_test.cpp

@@ -601,6 +601,195 @@ LT_BEGIN_AUTO_TEST(http_response_suite, with_header_moves_string_args)
                 std::string_view(std::string(64, 'v')));
 LT_END_AUTO_TEST(with_header_moves_string_args)
 
+// -----------------------------------------------------------------------
+// TASK-012 review-pass: security validation on fluent setters.
+//
+// with_header, with_footer, with_cookie must reject keys/values that
+// contain CR (\r), LF (\n), or NUL (\0) — these characters allow
+// HTTP response-header injection (CWE-113). with_status must reject
+// codes outside [100, 599] per RFC 9110 §15.
+// -----------------------------------------------------------------------
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_header_rejects_crlf_in_value)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_header("X-Foo", "bar\r\nSet-Cookie: evil=1");
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_header_rejects_crlf_in_value)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_header_rejects_lf_in_value)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_header("X-Foo", "bar\ninjected");
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_header_rejects_lf_in_value)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_header_rejects_nul_in_value)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_header("X-Foo", std::string("bar\0baz", 7));
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_header_rejects_nul_in_value)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_header_rejects_crlf_in_key)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_header("X-Foo\r\nEvil", "value");
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_header_rejects_crlf_in_key)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_footer_rejects_crlf_in_value)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_footer("X-Footer", "val\r\ninjected");
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_footer_rejects_crlf_in_value)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_footer_rejects_lf_in_key)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_footer("X-Footer\nEvil", "value");
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_footer_rejects_lf_in_key)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_cookie_rejects_crlf_in_value)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_cookie("sid", "abc\r\nSet-Cookie: evil=1");
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_cookie_rejects_crlf_in_value)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_cookie_rejects_lf_in_name)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_cookie("sid\nevil", "value");
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_cookie_rejects_lf_in_name)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_cookie_rejects_nul_in_value)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_cookie("sid", std::string("abc\0def", 7));
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_cookie_rejects_nul_in_value)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_status_rejects_below_100)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_status(99);
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_status_rejects_below_100)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_status_rejects_above_599)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_status(600);
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_status_rejects_above_599)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_status_rejects_negative)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_status(-1);
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_status_rejects_negative)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_status_rejects_zero)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_status(0);
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_status_rejects_zero)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_status_accepts_boundary_100)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_status(100);
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, false);
+    LT_CHECK_EQ(resp.get_status(), 100);
+LT_END_AUTO_TEST(with_status_accepts_boundary_100)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_status_accepts_boundary_599)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_status(599);
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, false);
+    LT_CHECK_EQ(resp.get_status(), 599);
+LT_END_AUTO_TEST(with_status_accepts_boundary_599)
+
+LT_BEGIN_AUTO_TEST(http_response_suite, with_header_accepts_valid_value)
+    http_response resp = http_response::string("body");
+    bool threw = false;
+    try {
+        resp.with_header("X-Foo", "valid value with spaces and colons: ok");
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, false);
+    LT_CHECK_EQ(resp.get_header("X-Foo"),
+                std::string_view("valid value with spaces and colons: ok"));
+LT_END_AUTO_TEST(with_header_accepts_valid_value)
+
 LT_BEGIN_AUTO_TEST_ENV()
     AUTORUN_TESTS()
 LT_END_AUTO_TEST_ENV()