TASK-064: fix partial-mutation in do_set_cookie and add semicolon guard to validate_attr_param

Two behavioral fixes driven by TDD:

1. Partial-mutation bug (code-simplifier-iter1-2): do_set_cookie now
   builds and validates the structured cookie first (with_name /
   with_value throw on invalid chars), then mirrors into the legacy
   cookies_ map only on success. Previously, insert_or_assign ran
   before with_name, leaving a dirty entry in the legacy map when
   with_name threw. Test: legacy_with_cookie_bad_key_leaves_maps_clean.

2. Attribute-injection guard (security-reviewer-iter1-1): validate_attr_param
   (used by with_domain and with_path) now rejects semicolons in addition
   to CR/LF/NUL. A semicolon in Domain or Path would be emitted verbatim
   by the renderer, injecting synthetic attributes into the Set-Cookie
   header (CWE-113). Tests: with_path_rejects_semicolon,
   with_domain_rejects_semicolon.

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

Author: etr

src/cookie.cpp

@@ -86,6 +86,16 @@ void validate_attr_param(std::string_view setter, std::string_view v) {
             ": attribute value contains forbidden control character "
             "(CR, LF, or NUL)");
     }
+    // CWE-113: a semicolon in a Domain or Path attribute value would
+    // be emitted verbatim by the renderer, injecting synthetic
+    // attributes into the Set-Cookie header (e.g. with_path("/; Secure")
+    // produces 'name=val; Path=/; Secure' even though the caller only
+    // meant to set a path).
+    if (v.find(';') != std::string_view::npos) {
+        throw std::invalid_argument(
+            std::string(setter) +
+            ": attribute value contains forbidden character (';')");
+    }
 }
 
 // IMF-fixdate formatter (RFC 7231 §7.1.1.1):

src/http_response.cpp

@@ -215,24 +215,24 @@ void http_response::do_set_footer(std::string key, std::string value) {
 }
 
 void http_response::do_set_cookie(std::string key, std::string value) {
-    // Legacy v1 string-blob entry point. Validates with the v1
-    // CR/LF/NUL guard (same as before TASK-064), mirrors into the
-    // legacy `cookies_` map for the deprecated `get_cookie`/
-    // `get_cookies` accessors, AND appends a structured cookie so the
-    // dispatch path has a single render source. Pre-TASK-064 callers
-    // that put a `;` in the value silently injected attributes;
-    // post-TASK-064 the structured renderer is the only render source
-    // and it bans `;` in cookie values -- so we keep the v1
-    // validate_http_field for the legacy shim's input check and
-    // simply rely on the structured renderer to enforce the stricter
-    // RFC 6265 rules on emit. (Action: a legacy caller passing `;` in
-    // the value will see the validation throw at to_set_cookie_header
-    // time -- that is the v2 promise the task asks for.)
-    validate_http_field("with_cookie", key, value);
-    cookies_.insert_or_assign(key, value);
-    cookie c;
-    c.with_name(std::move(key)).with_value(std::move(value));
-    structured_cookies_.push_back(std::move(c));
+    // Legacy v1 string-blob entry point. Build and validate the
+    // structured cookie FIRST — with_name / with_value throw
+    // std::invalid_argument for forbidden characters (CR, LF, NUL,
+    // ';', '=' in names). Only after the structured object is
+    // successfully constructed do we mirror name/value into the legacy
+    // `cookies_` map so the two stores never diverge on failure.
+    //
+    // Note: validate_http_field is intentionally NOT called here. The
+    // structured cookie::with_name / with_value setters enforce the
+    // stricter RFC 6265 rules (rejecting ';' in both name and value,
+    // among others) and give callers the correct error message.
+    // Calling validate_http_field first would give an incorrect
+    // earlier error citing 'forbidden control character' rather than
+    // the real reason.
+    cookie new_cookie;
+    new_cookie.with_name(key).with_value(value);  // throws before any map mutation
+    cookies_.insert_or_assign(std::move(key), std::move(value));
+    structured_cookies_.push_back(std::move(new_cookie));
 }
 
 void http_response::do_set_cookie_struct(cookie c) {

test/unit/cookie_render_test.cpp

@@ -153,6 +153,23 @@ LT_BEGIN_AUTO_TEST(cookie_render_suite, with_path_rejects_crlf)
     LT_CHECK_EQ(threw, true);
 LT_END_AUTO_TEST(with_path_rejects_crlf)
 
+LT_BEGIN_AUTO_TEST(cookie_render_suite, with_path_rejects_semicolon)
+    // CWE-113: a semicolon in Path would inject synthetic attributes
+    // into the Set-Cookie header.
+    bool threw = false;
+    try { cookie{}.with_path("/; Secure"); }
+    catch (const std::invalid_argument&) { threw = true; }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_path_rejects_semicolon)
+
+LT_BEGIN_AUTO_TEST(cookie_render_suite, with_domain_rejects_semicolon)
+    // CWE-113: a semicolon in Domain would inject synthetic attributes.
+    bool threw = false;
+    try { cookie{}.with_domain("example.com; Secure"); }
+    catch (const std::invalid_argument&) { threw = true; }
+    LT_CHECK_EQ(threw, true);
+LT_END_AUTO_TEST(with_domain_rejects_semicolon)
+
 // ---------------- Cycle 4: to_set_cookie_header() rendering ----------------
 
 LT_BEGIN_AUTO_TEST(cookie_render_suite, render_minimal_name_value)

test/unit/http_response_cookie_wire_test.cpp

@@ -220,6 +220,27 @@ LT_BEGIN_AUTO_TEST(http_response_cookie_wire_suite,
                 std::string("sid=abc; Secure; SameSite=Strict"));
 LT_END_AUTO_TEST(wire_renderer_emits_full_attribute_set)
 
+// ---------------- Cycle 9: partial-mutation guard (TASK-064 fix) ----
+
+LT_BEGIN_AUTO_TEST(http_response_cookie_wire_suite,
+                   legacy_with_cookie_bad_key_leaves_maps_clean)
+    // Regression: if with_name throws (key contains ';'), the legacy
+    // cookies_ map must NOT be mutated. Before the fix, insert_or_assign
+    // ran before with_name, so the map got a dirty entry on failure.
+    http_response r = http_response::string("body");
+    bool threw = false;
+    try {
+        r.with_cookie("bad;key", "value");
+    } catch (const std::invalid_argument&) {
+        threw = true;
+    }
+    LT_CHECK_EQ(threw, true);
+    // The bad key must NOT appear in the legacy map.
+    LT_CHECK_EQ(r.get_cookie("bad;key"), std::string_view(""));
+    // The structured vector must also be empty.
+    LT_CHECK_EQ(r.get_cookies_parsed().empty(), true);
+LT_END_AUTO_TEST(legacy_with_cookie_bad_key_leaves_maps_clean)
+
 LT_BEGIN_AUTO_TEST_ENV()
     AUTORUN_TESTS()
 LT_END_AUTO_TEST_ENV()