Skip to content

Prevent re-encoding of path when following a redirect #2184

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
+36 −2
Open

Prevent re-encoding of path when following a redirect #2184

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
4 commits
Select commit Hold shift + click to select a range
9db1e5f
Prevent re-encoding of redirection paths
JoeMylinn Jul 19, 2025
f41e7eb
Add unit test for a redirect to an url with query parameters that con…
JoeMylinn Jul 20, 2025
b445576
Fix formatting style
JoeMylinn Jul 20, 2025
b32ef94
Fix formatting style again
JoeMylinn Jul 21, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 5 additions & 2 deletions httplib.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -8661,10 +8661,12 @@ inline bool ClientImpl::redirect(Request &req, Response &res, Error &error) {
if (next_host.empty()) { next_host = host_; }
if (next_path.empty()) { next_path = "/"; }

auto path = detail::decode_path(next_path, true) + next_query;
auto path = next_path + next_query;

// Same host redirect - use current client
if (next_scheme == scheme && next_host == host_ && next_port == port_) {
this->set_path_encode(
false); // Disable path encoding, redirects should already be encoded
Comment on lines +8668 to +8669
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Doesn't this set path_encode_ for all subsequent requests, overriding what the user configured?

Client cli(HOST, PORT);
cli.set_follow_location(true);
cli.set_path_encode(true); // encode paths
auto res = cli.Get("/a b"); // encoded as "/a%20b", redirects
// path_encode_ is now false
auto res = cli.Get("/c d"); // not encoded anymore, sent as "/c d"

Just a thought, I haven't tested this code.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah I believe you are right. That shouldn't be the case of course. Maybe we can just always create a new redirect client, even if it's the same host and never reuse the original one?

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Or you go back to your original suggestion (we talked about on discord) and decode the host+query so that it can be re-encoded in the client as normal. @JoeMylinn

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah maybe the best option is to always decode both the path and query, and re-encode it before passing the new path to the original or redirect client.

Copy link
Author

@JoeMylinn JoeMylinn Jul 22, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I still don't like the decode and re-encode thing. Especially since we'd always have to encode if we decode it first, or it could break URLs with encoded characters if path_encode_ is set to false.

We could just parse the query parameters and add them to the request, something like this:

 auto path = next_path;

 if (!next_query.empty())
 {
   detail::parse_query_text(next_query, req.params);
 }

@yhirose any thoughts on what solution you'd prefer?

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@JoeMylinn I still don't fully understand the whole situation, yet. Could you please try with v0.22.0 to see if the same issue happens?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@yhirose Yes, v0.22.0 has the same issue.

I'll try to explain the issue a bit better. We need several things for it to happen:

  1. follow_location_ set to true
  2. path_encode_ set to true
  3. Get redirected, with the query part of the Location containing a +.

For example, let's say the redirect location is https://example.com/test?say=Hello+World. The value of the parameter say should be Hello World, since '+' encodes space ' ' in a query. However, the current implementation just appends the query to the path, and then (if path_encode_ is set to true) encodes the whole path, including the query. This encodes it as Hello%2BWorld, instead of Hello+World or Hello%20World, changing the value of the of the parameter to Hello+World.

I've noticed this issue because it causes downloads from GitHub to fail, same as in #2185.

I originally fixed this by just setting path_encode_ to false when following a redirect.
However this is flawed, since it might override the user configured settings for the client, when the location is on the same host.

I think there are mainly two ways to fix this:

  1. Always use a new redirect client, even if the location is on the same host, with path_encode_ set to false.
  2. Parse the query properly, and pass the parameters to the new request.

Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The question is why is only the host decoded and not the entire query?

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure, but decoding just the path can already cause issues even if there isn't a query. If path_encode_ is false, and the path contains an encoded character, it will get decoded but not encoded again and the request will fail. So we'd have to always encode the path, regardless of the settings on the original client.

I just feel like it's simpler to never decode the path in the first place, and just parse the query. Unless there is a reason for it that I am missing.

return detail::redirect(*this, req, res, path, location, error);
}

Expand Down Expand Up @@ -8756,7 +8758,8 @@ inline void ClientImpl::setup_redirect_client(ClientType &client) {
client.set_keep_alive(keep_alive_);
client.set_follow_location(
true); // Enable redirects to handle multi-step redirects
client.set_path_encode(path_encode_);
client.set_path_encode(
false); // Disable encoding, redirects should already be encoded
client.set_compress(compress_);
client.set_decompress(decompress_);

Expand Down
31 changes: 31 additions & 0 deletions test/test.cc
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9942,6 +9942,37 @@ TEST(RedirectTest, RedirectToUrlWithQueryParameters) {
}
}

TEST(RedirectTest, RedirectToUrlWithPlusInQueryParameters) {
Server svr;

svr.Get("/", [](const Request & /*req*/, Response &res) {
res.set_redirect(R"(/hello?key=AByz09+~-._%20%26%3F%C3%BC%2B)");
});

svr.Get("/hello", [](const Request &req, Response &res) {
res.set_content(req.get_param_value("key"), "text/plain");
});

auto thread = std::thread([&]() { svr.listen(HOST, PORT); });
auto se = detail::scope_exit([&] {
svr.stop();
thread.join();
ASSERT_FALSE(svr.is_running());
});

svr.wait_until_ready();

{
Client cli(HOST, PORT);
cli.set_follow_location(true);

auto res = cli.Get("/");
ASSERT_TRUE(res);
EXPECT_EQ(StatusCode::OK_200, res->status);
EXPECT_EQ("AByz09 ~-._ &?ü+", res->body);
}
}

TEST(VulnerabilityTest, CRLFInjection) {
Server svr;

Expand Down
Loading