Apply path encoding to open_stream()

ClientImpl::open_stream() passed the caller-supplied path straight to the request line, so it ignored path_encode_ entirely and always behaved as if set_path_encode(false) had been called. The same path therefore produced different bytes on the wire depending on which API was used: Get() "/a b" -> GET /a%20b HTTP/1.1 open_stream() "/a b" -> GET /a b HTTP/1.1 A space is the request-target delimiter, so the streaming form is not merely inconsistent: an RFC 9112 conformant server reads the target as "/a" and the version as "b". Non-ASCII bytes and '+' diverged the same way, the latter changing the value a server that decodes '+' as space sees. Extract the path/query splitting and encoding out of ClientImpl::write_request into detail::encode_request_target() and call it from both paths, so the encoding rule lives in one place. open_stream() appends Params before encoding, matching ClientImpl::Get(path, params), which builds its target the same way. Note a behavior change: with path encoding enabled, CR/LF in the target is now percent-encoded and sent rather than rejected with Error::Write, matching Get(). This is not a weakening of the CR/LF guard in write_request_line() -- that check is independent of path_encode_ and still backstops set_path_encode(false), where encode_path() does nothing.
2026-09-30 12:42:26 +07:00 · 2026-07-25 13:05:48 -04:00
parent d66d9a9599
commit 5b9d1495ff
2 changed files with 262 additions and 41 deletions
@@ -7947,6 +7947,43 @@ inline std::string normalize_query_string(const std::string &query) {
  return result;
 }

+// Build the request target that goes on the wire from a caller-supplied path.
+// Shared by the buffered send path and the streaming API so that both put the
+// same bytes in the request line for the same input.
+inline std::string encode_request_target(const std::string &target,
+                                         bool path_encode) {
+  // `substr(0, npos)` yields the whole string, which is what the no-query
+  // case needs.
+  auto query_pos = target.find('?');
+  auto path_part = target.substr(0, query_pos);
+  std::string query_part;
+  if (query_pos != std::string::npos) {
+    query_part = target.substr(query_pos + 1);
+  }
+
+  auto result = path_encode ? encode_path(path_part) : std::move(path_part);
+
+  if (!query_part.empty()) {
+    // When path encoding is disabled the caller has supplied an already-encoded
+    // target and expects the exact bytes to be sent on the wire, so skip
+    // normalization for the query too. Normalizing would decode-then-re-encode
+    // it and corrupt pre-encoded binary payloads (e.g. turning `%20` into `+`,
+    // which a strict RFC 3986 server decodes back as `+`, not a space).
+    if (path_encode) {
+      auto normalized = normalize_query_string(query_part);
+      if (!normalized.empty()) {
+        result += '?';
+        result += normalized;
+      }
+    } else {
+      result += '?';
+      result += query_part;
+    }
+  }
+
+  return result;
+}
+
 inline bool parse_multipart_boundary(const std::string &content_type,
                                     std::string &boundary) {
  std::map<std::string, std::string> params;
@@ -13240,7 +13277,12 @@ ClientImpl::open_stream(const std::string &method, const std::string &path,
  handle.response = detail::make_unique<Response>();
  handle.error = Error::Success;

-  auto query_path = params.empty() ? path : append_query_params(path, params);
+  // Encode the target exactly like the buffered send path does, so that the
+  // same `path` produces the same request line through either API.
+  auto raw_query_path =
+      params.empty() ? path : append_query_params(path, params);
+  auto query_path = detail::encode_request_target(raw_query_path, path_encode_);
+
  handle.connection_ = detail::make_unique<ClientConnection>();

  {
@@ -13885,52 +13927,26 @@ inline bool ClientImpl::write_request(Stream &strm, Request &req,
  {
    detail::BufferStream bstrm;

-    // Extract path and query from req.path
-    std::string path_part, query_part;
+    // Extract the query from req.path. The encoding itself is delegated to
+    // `encode_request_target`; the raw query is still needed here to decide
+    // between populating `req.params` from it and falling back to building a
+    // query out of caller-supplied `req.params`.
    auto query_pos = req.path.find('?');
-    if (query_pos != std::string::npos) {
-      path_part = req.path.substr(0, query_pos);
-      query_part = req.path.substr(query_pos + 1);
-    } else {
-      path_part = req.path;
-      query_part = "";
-    }
+    auto query_part = query_pos == std::string::npos
+                          ? std::string()
+                          : req.path.substr(query_pos + 1);

-    // Encode path part. If the original `req.path` already contained a
-    // query component, preserve its raw query string (including parameter
-    // order) instead of reparsing and reassembling it which may reorder
-    // parameters due to container ordering (e.g. `Params` uses
-    // `std::multimap`). When there is no query in `req.path`, fall back to
-    // building a query from `req.params` so existing callers that pass
-    // `Params` continue to work.
    auto path_with_query =
-        path_encode_ ? detail::encode_path(path_part) : path_part;
+        detail::encode_request_target(req.path, path_encode_);

    if (!query_part.empty()) {
-      // Normalize the query string (decode then re-encode) while preserving
-      // the original parameter order. When path encoding is disabled the
-      // caller has supplied an already-encoded target and expects the exact
-      // bytes to be sent on the wire, so skip normalization for the query
-      // too. Normalizing here would decode-then-re-encode the query and
-      // corrupt pre-encoded binary payloads (e.g. turning `%20` into `+`,
-      // which a strict RFC 3986 server decodes back as `+`, not a space).
-      if (path_encode_) {
-        auto normalized = detail::normalize_query_string(query_part);
-        if (!normalized.empty()) { path_with_query += '?' + normalized; }
-      } else {
-        path_with_query += '?' + query_part;
-      }
-
-      // Still populate req.params for handlers/users who read them.
+      // The query already came in through `req.path`; still populate
+      // `req.params` for handlers/users who read them.
      detail::parse_query_text(query_part, req.params);
-    } else {
-      // No query in path; parse any query_part (empty) and append params
-      // from `req.params` when present (preserves prior behavior for
-      // callers who provide Params separately).
-      detail::parse_query_text(query_part, req.params);
-      if (!req.params.empty()) {
-        path_with_query = append_query_params(path_with_query, req.params);
-      }
+    } else if (!req.params.empty()) {
+      // No query in `req.path`; build one from `req.params` so existing
+      // callers that pass `Params` separately continue to work.
+      path_with_query = append_query_params(path_with_query, req.params);
    }

    // Write request line and headers
@@ -3140,6 +3140,211 @@ TEST(PathUrlEncodeTest, PreEncodedQueryNotReencoded) {
  }
 }

+TEST(PathUrlEncodeTest, StreamingMatchesBufferedTarget) {
+  // `open_stream()` used to skip the path encoding that the buffered send
+  // path applies, so the same `path` produced different bytes in the request
+  // line depending on which API was used. Assert the two agree.
+  Server svr;
+
+  std::string target;
+  svr.Get(".*", [&](const Request &req, Response &res) {
+    target = req.target;
+    res.status = StatusCode::OK_200;
+  });
+
+  auto port = svr.bind_to_any_port(HOST);
+  auto thread = std::thread([&]() { svr.listen_after_bind(); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    thread.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+
+  svr.wait_until_ready();
+
+  struct {
+    const char *path;
+    const char *expected;
+  } cases[] = {
+      {"/a b?x=1", "/a%20b?x=1"},
+      {"/\xE6\x97\xA5\xE6\x9C\xAC", "/%E6%97%A5%E6%9C%AC"},
+      {"/a,b;c+d", "/a%2Cb%3Bc%2Bd"},
+  };
+
+  for (const auto &c : cases) {
+    Client cli(HOST, port);
+
+    target.clear();
+    auto res = cli.Get(c.path);
+    ASSERT_TRUE(res) << "Error: " << to_string(res.error());
+    EXPECT_EQ(c.expected, target) << "buffered path: " << c.path;
+    auto buffered_target = target;
+
+    target.clear();
+    auto handle = cli.open_stream("GET", c.path);
+    EXPECT_TRUE(handle.is_valid())
+        << "streaming path: " << c.path << ", " << to_string(handle.error);
+    EXPECT_EQ(buffered_target, target) << "streaming path: " << c.path;
+  }
+}
+
+TEST(PathUrlEncodeTest, StreamingPreEncodedQueryNotReencoded) {
+  // The `set_path_encode(false)` contract — transmit the supplied target
+  // verbatim — must hold for the streaming API too.
+  Server svr;
+
+  const std::string expected_target = "/foo?q=a%20b%2Cc%24d%3Bx&a=%00%FF";
+
+  std::string target;
+  svr.Get("/foo", [&](const Request &req, Response &res) {
+    target = req.target;
+    res.status = StatusCode::OK_200;
+  });
+
+  auto port = svr.bind_to_any_port(HOST);
+  auto thread = std::thread([&]() { svr.listen_after_bind(); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    thread.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+
+  svr.wait_until_ready();
+
+  {
+    Client cli(HOST, port);
+    cli.set_path_encode(false);
+
+    auto handle = cli.open_stream("GET", expected_target);
+    EXPECT_TRUE(handle.is_valid()) << to_string(handle.error);
+    EXPECT_EQ(expected_target, target);
+  }
+}
+
+TEST(PathUrlEncodeTest, StreamingCRLFInTargetIsEncoded) {
+  // With path encoding enabled a CR/LF in the target is percent-encoded
+  // rather than rejected, matching the buffered send path. The CR/LF guard in
+  // write_request_line still backstops `set_path_encode(false)`, which is
+  // covered by StreamingCRLFRejectedWhenPathEncodeDisabled.
+  Server svr;
+
+  // Captured before routing: the decoded path contains a newline, which a
+  // `.*` handler pattern would not match (`.` excludes `\n` in std::regex).
+  std::string target;
+  svr.set_pre_routing_handler([&](const Request &req, Response &res) {
+    target = req.target;
+    res.status = StatusCode::OK_200;
+    return Server::HandlerResponse::Handled;
+  });
+
+  auto port = svr.bind_to_any_port(HOST);
+  auto thread = std::thread([&]() { svr.listen_after_bind(); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    thread.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+
+  svr.wait_until_ready();
+
+  {
+    Client cli(HOST, port);
+
+    auto handle = cli.open_stream("GET", "/a\r\nX-Injected: 1");
+    EXPECT_TRUE(handle.is_valid()) << to_string(handle.error);
+    EXPECT_EQ("/a%0D%0AX-Injected:%201", target);
+  }
+}
+
+TEST(PathUrlEncodeTest, StreamingCRLFRejectedWhenPathEncodeDisabled) {
+  // Nothing may reach the wire: a raw CR/LF target would split the request
+  // line and inject headers.
+  Server svr;
+
+  // Pre-routing so that "not called" means nothing reached the server at all,
+  // rather than merely failing to match a handler pattern.
+  auto handler_called = false;
+  svr.set_pre_routing_handler([&](const Request & /*req*/, Response &res) {
+    handler_called = true;
+    res.status = StatusCode::OK_200;
+    return Server::HandlerResponse::Handled;
+  });
+
+  auto port = svr.bind_to_any_port(HOST);
+  auto thread = std::thread([&]() { svr.listen_after_bind(); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    thread.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+
+  svr.wait_until_ready();
+
+  {
+    Client cli(HOST, port);
+    cli.set_path_encode(false);
+
+    auto handle = cli.open_stream("GET", "/a\r\nX-Injected: 1");
+    EXPECT_FALSE(handle.is_valid());
+    EXPECT_EQ(Error::Write, handle.error);
+    EXPECT_FALSE(handler_called);
+  }
+}
+
+TEST(PathUrlEncodeTest, RequestParamsBranchSelection) {
+  // Which of the two branches runs is decided by whether the query component
+  // is empty, not by whether `req.path` contains a `?`: a trailing `?` yields
+  // an empty query and must still fall back to building one from
+  // `req.params`, while a non-empty query must win over `req.params`.
+  Server svr;
+
+  std::string target;
+  svr.Get("/foo", [&](const Request &req, Response &res) {
+    target = req.target;
+    res.status = StatusCode::OK_200;
+  });
+
+  auto port = svr.bind_to_any_port(HOST);
+  auto thread = std::thread([&]() { svr.listen_after_bind(); });
+  auto se = detail::scope_exit([&] {
+    svr.stop();
+    thread.join();
+    ASSERT_FALSE(svr.is_running());
+  });
+
+  svr.wait_until_ready();
+
+  {
+    // Trailing `?` -> empty query component -> `req.params` is used.
+    Client cli(HOST, port);
+
+    Request req;
+    req.method = "GET";
+    req.path = "/foo?";
+    req.params.emplace("a", "1");
+
+    target.clear();
+    auto res = cli.send(req);
+    ASSERT_TRUE(res) << "Error: " << to_string(res.error());
+    EXPECT_EQ("/foo?a=1", target);
+  }
+
+  {
+    // Non-empty query in `req.path` -> `req.params` is not appended.
+    Client cli(HOST, port);
+
+    Request req;
+    req.method = "GET";
+    req.path = "/foo?b=2";
+    req.params.emplace("a", "1");
+
+    target.clear();
+    auto res = cli.send(req);
+    ASSERT_TRUE(res) << "Error: " << to_string(res.error());
+    EXPECT_EQ("/foo?b=2", target);
+  }
+}
+
 TEST(PathUrlEncodeTest, IncludePercentEncodingLF) {
  Server svr;