This is an automated email from the ASF dual-hosted git repository.
yiguolei pushed a commit to branch master
in repository https://gitbox.apache.org/repos/asf/doris.git
The following commit(s) were added to refs/heads/master by this push:
new ba6ac67d021 [fix](be) Reject malformed percent escapes in url_decode
(#68454)
ba6ac67d021 is described below
commit ba6ac67d02100d5ba504880a7a2a1e3472d3f8c2
Author: Arpit Jain <[email protected]>
AuthorDate: Mon Sep 28 05:16:13 2026 -0400
[fix](be) Reject malformed percent escapes in url_decode (#68454)
### What problem does this PR solve?
Issue Number: none
Problem Summary:
`url_decode` reads the two characters after a `%` with an
`istringstream`, which is happy with a partial match. `a%1gb` decodes to
`a\x01b` and the `g` is dropped. The FE folds the same function through
`java.net.URLDecoder`, which raises on that input, so the two sides
disagree on it.
Stream load then ignores the bool entirely:
```cpp
url_decode(req->param(HTTP_DB_KEY), &ctx->db);
url_decode(req->param(HTTP_TABLE_KEY), &ctx->table);
```
`url_decode` clears its output before it starts, so on a malformed
escape the caller is left holding the prefix decoded so far. `PUT
/api/prod%zzbackup/t/_stream_load` loads into `prod`.
Now a `%` must be followed by two hex digits, and stream load fails the
request if either name does not decode. The SQL function already checked
the result, so it picks up the stricter parse for free.
While I was in there: `be/test/util/url_coding_test.cpp` was sitting in
the "todo: need fix those ut" list in `be/test/CMakeLists.txt`, still
calling the old `UrlDecode`/`Base64Encode`/`hive_compat` API. Ported it
to the current names and put it back in the build, with the malformed
cases added.
### Release note
`url_decode` rejects a `%` not followed by two hexadecimal digits, and
stream load fails the request rather than using a truncated database or
table name.
### Check List (For Author)
- Test: Unit Test
- `be/test/util/url_coding_test.cpp`, revived and extended.
- No BE build on this machine, so I compiled `url_coding.cpp` and that
test on their own against real gtest: 6 tests pass, and the
malformed-escape one fails on master. The stream load side is a source
change I have not run; CI and a reviewer's eye are the check there.
- Behavior changed: Yes, as described in the release note.
- Does this need documentation: No
---------
Signed-off-by: Arpit Jain <[email protected]>
---
be/src/service/http/action/stream_load.cpp | 17 +++-
be/src/util/path_trie.hpp | 4 +-
be/src/util/url_coding.cpp | 38 +++++---
be/test/CMakeLists.txt | 1 -
be/test/util/path_trie_test.cpp | 15 ++++
be/test/util/url_coding_test.cpp | 138 ++++++++++++++++++++---------
6 files changed, 155 insertions(+), 58 deletions(-)
diff --git a/be/src/service/http/action/stream_load.cpp
b/be/src/service/http/action/stream_load.cpp
index 8f47edaba8a..9b25eec6ec9 100644
--- a/be/src/service/http/action/stream_load.cpp
+++ b/be/src/service/http/action/stream_load.cpp
@@ -252,11 +252,22 @@ int StreamLoadAction::on_header(HttpRequest* req) {
ctx->load_type = TLoadType::MANUL_LOAD;
ctx->load_src_type = TLoadSourceType::RAW;
- url_decode(req->param(HTTP_DB_KEY), &ctx->db);
- url_decode(req->param(HTTP_TABLE_KEY), &ctx->table);
+ Status st = Status::OK();
+ if (!url_decode(req->param(HTTP_DB_KEY), &ctx->db) ||
+ !url_decode(req->param(HTTP_TABLE_KEY), &ctx->table)) {
+ // url_decode clears its output and then appends until it fails, so
whatever it
+ // managed to decode is still sitting in ctx. Drop it, or the log line
below and
+ // the failed load record both attribute the request to a truncated
name.
+ ctx->db.clear();
+ ctx->table.clear();
+ st = Status::InvalidArgument(
+ "Invalid percent-encoding in the database or table name of the
request path");
+ }
ctx->label = req->header(HTTP_LABEL_KEY);
ctx->two_phase_commit = req->header(HTTP_TWO_PHASE_COMMIT) == "true";
- Status st = _handle_group_commit(req, ctx);
+ if (st.ok()) {
+ st = _handle_group_commit(req, ctx);
+ }
if (!ctx->group_commit && ctx->label.empty()) {
ctx->label = generate_uuid_string();
}
diff --git a/be/src/util/path_trie.hpp b/be/src/util/path_trie.hpp
index c593b796ec6..75534ee751b 100644
--- a/be/src/util/path_trie.hpp
+++ b/be/src/util/path_trie.hpp
@@ -207,7 +207,9 @@ public:
void put(std::map<std::string, std::string>* params, TrieNode* node,
const std::string& token) {
if (params != nullptr && !node->_named_wildcard.empty()) {
- params->insert(std::make_pair(node->_named_wildcard, token));
+ // The query string is parsed into the same map before routing
runs, so
+ // insert() would silently keep a "?db=" over the {db} the
path matched.
+ (*params)[node->_named_wildcard] = token;
}
}
diff --git a/be/src/util/url_coding.cpp b/be/src/util/url_coding.cpp
index 468f80ea91a..7f40058cca4 100644
--- a/be/src/util/url_coding.cpp
+++ b/be/src/util/url_coding.cpp
@@ -53,25 +53,41 @@ void url_encode(const std::string_view& in, std::string*
out) {
// http://www.boost.org/doc/libs/1_40_0/doc/html/boost_asio/
// example/http/server3/request_handler.cpp
// See http://www.boost.org/LICENSE_1_0.txt for license for this method.
+// Value of a single hexadecimal digit, or -1 when c is not one.
+static int hex_digit(char c) {
+ if (c >= '0' && c <= '9') {
+ return c - '0';
+ }
+ if (c >= 'a' && c <= 'f') {
+ return c - 'a' + 10;
+ }
+ if (c >= 'A' && c <= 'F') {
+ return c - 'A' + 10;
+ }
+ return -1;
+}
+
bool url_decode(const std::string& in, std::string* out) {
out->clear();
out->reserve(in.size());
for (size_t i = 0; i < in.size(); ++i) {
if (in[i] == '%') {
- if (i + 3 <= in.size()) {
- int value = 0;
- std::istringstream is(in.substr(i + 1, 2));
-
- if (is >> std::hex >> value) {
- (*out) += static_cast<char>(value);
- i += 2;
- } else {
- return false;
- }
- } else {
+ if (i + 3 > in.size()) {
return false;
}
+
+ // A '%' must be followed by exactly two hexadecimal digits.
Parsing the pair
+ // with a stream accepts a partial match such as "%1g" and
silently drops the
+ // character that follows it.
+ const int high = hex_digit(in[i + 1]);
+ const int low = hex_digit(in[i + 2]);
+ if (high < 0 || low < 0) {
+ return false;
+ }
+
+ (*out) += static_cast<char>((high << 4) | low);
+ i += 2;
} else if (in[i] == '+') {
(*out) += ' ';
} else {
diff --git a/be/test/CMakeLists.txt b/be/test/CMakeLists.txt
index d89803ff7fe..f76ef151d0f 100644
--- a/be/test/CMakeLists.txt
+++ b/be/test/CMakeLists.txt
@@ -103,7 +103,6 @@ list(REMOVE_ITEM UT_FILES
${CMAKE_CURRENT_SOURCE_DIR}/storage/segment/segment_iterator_apply_index_expr_test.cpp
${CMAKE_CURRENT_SOURCE_DIR}/runtime/decimal_value_test.cpp
${CMAKE_CURRENT_SOURCE_DIR}/util/decompress_test.cpp
- ${CMAKE_CURRENT_SOURCE_DIR}/util/url_coding_test.cpp
${CMAKE_CURRENT_SOURCE_DIR}/io/fs/remote_file_system_test.cpp
${CMAKE_CURRENT_SOURCE_DIR}/storage/remote_rowset_gc_test.cpp
${CMAKE_CURRENT_SOURCE_DIR}/runtime/jsonb_value_test.cpp
diff --git a/be/test/util/path_trie_test.cpp b/be/test/util/path_trie_test.cpp
index 2f337b2a760..3112b65bd3c 100644
--- a/be/test/util/path_trie_test.cpp
+++ b/be/test/util/path_trie_test.cpp
@@ -72,6 +72,21 @@ TEST_F(PathTrieTest, TemplateTest) {
EXPECT_STREQ("c", params["rollup"].c_str());
}
+TEST_F(PathTrieTest, PathWinsOverQueryParamTest) {
+ // EvHttpServer parses the query string into the same map before it
routes, so a
+ // "?db=" must not survive into the {db} the path matched.
+ PathTrie<int> root;
+ EXPECT_TRUE(root.insert("/api/{db}/{table}/_stream_load", 1));
+
+ std::map<std::string, std::string> params;
+ params.emplace("db", "from_query_string");
+
+ int value;
+ EXPECT_TRUE(root.retrieve("/api/realdb/tbl/_stream_load", &value,
¶ms));
+ EXPECT_STREQ("realdb", params["db"].c_str());
+ EXPECT_STREQ("tbl", params["table"].c_str());
+}
+
TEST_F(PathTrieTest, ExactTest) {
PathTrie<int> root;
std::string path = "/db/table/rollup";
diff --git a/be/test/util/url_coding_test.cpp b/be/test/util/url_coding_test.cpp
index c9d0471abd8..b1dacbeb42f 100644
--- a/be/test/util/url_coding_test.cpp
+++ b/be/test/util/url_coding_test.cpp
@@ -18,77 +18,131 @@
#include "util/url_coding.h"
#include <gtest/gtest.h>
-#include <stdio.h>
-#include <stdlib.h>
-#include <iostream>
+#include <sstream>
+#include <string>
+#include <vector>
namespace doris {
-// Tests encoding/decoding of input. If expected_encoded is non-empty, the
-// encoded string is validated against it.
-void test_url(const string& input, const string& expected_encoded, bool
hive_compat) {
+// Encode the input, then decode it again and check we are back where we
started.
+void test_url(const std::string& input, const std::string& expected_encoded) {
std::string intermediate;
- url_encode(input, &intermediate, hive_compat);
- std::string output;
+ url_encode(input, &intermediate);
if (!expected_encoded.empty()) {
EXPECT_EQ(intermediate, expected_encoded);
}
- EXPECT_TRUE(UrlDecode(intermediate, &output, hive_compat));
+ std::string output;
+ EXPECT_TRUE(url_decode(intermediate, &output));
EXPECT_EQ(input, output);
-
- // Convert string to vector and try that also
- std::vector<uint8_t> input_vector;
- input_vector.resize(input.size());
- memcpy(&input_vector[0], input.c_str(), input.size());
- std::string intermediate2;
- url_encode(input_vector, &intermediate2, hive_compat);
- EXPECT_EQ(intermediate, intermediate2);
}
-void test_base64(const string& input, const string& expected_encoded) {
+void test_base64(const std::string& input, const std::string&
expected_encoded) {
std::string intermediate;
- Base64Encode(input, &intermediate);
- std::string output;
+ base64_encode(input, &intermediate);
if (!expected_encoded.empty()) {
EXPECT_EQ(intermediate, expected_encoded);
}
- EXPECT_TRUE(Base64Decode(intermediate, &output));
+ std::string output;
+ EXPECT_TRUE(base64_decode(intermediate, &output));
EXPECT_EQ(input, output);
-
- // Convert string to vector and try that also
- std::vector<uint8_t> input_vector;
- input_vector.resize(input.size());
- memcpy(&input_vector[0], input.c_str(), input.size());
- std::string intermediate2;
- Base64Encode(input_vector, &intermediate2);
- EXPECT_EQ(intermediate, intermediate2);
}
-// Test URL encoding. Check that the values that are put in are the
-// same that come out.
TEST(UrlCodingTest, Basic) {
std::string input =
"ABCDEFGHIJKLMNOPQRSTUWXYZ1234567890~!@#$%^&*()<>?,./:\";'{}|[]\\_+-=";
- test_url(input, "", false);
- test_url(input, "", true);
-}
-
-TEST(UrlCodingTest, HiveExceptions) {
- test_url(" +", " +", true);
+ test_url(input, "");
}
TEST(UrlCodingTest, BlankString) {
- test_url("", "", false);
- test_url("", "", true);
+ test_url("", "");
}
TEST(UrlCodingTest, PathSeparators) {
- test_url("/home/doris/directory/", "%2Fhome%2Fdoris%2Fdirectory%2F",
false);
- test_url("/home/doris/directory/", "%2Fhome%2Fdoris%2Fdirectory%2F", true);
+ test_url("/home/doris/directory/", "%2Fhome%2Fdoris%2Fdirectory%2F");
+}
+
+TEST(UrlCodingTest, Spaces) {
+ std::string output;
+ EXPECT_TRUE(url_decode("my+db", &output));
+ EXPECT_EQ(output, "my db");
+ EXPECT_TRUE(url_decode("my%20db", &output));
+ EXPECT_EQ(output, "my db");
+}
+
+TEST(UrlCodingTest, MalformedEscapeIsRejected) {
+ std::string output;
+
+ // Neither character after the '%' is a hexadecimal digit.
+ EXPECT_FALSE(url_decode("prod%zzbackup", &output));
+ EXPECT_FALSE(url_decode("a%%20b", &output));
+
+ // Only one of the two is. A stream parse accepts these and silently
swallows the
+ // character that follows, which is what this check exists to stop.
+ EXPECT_FALSE(url_decode("a%1gb", &output));
+ EXPECT_FALSE(url_decode("%z1", &output));
+ EXPECT_FALSE(url_decode("%1z", &output));
+
+ // Signs and spaces are what a stream parse is most willing to accept.
+ EXPECT_FALSE(url_decode("%+1", &output));
+ EXPECT_FALSE(url_decode("%-1", &output));
+ EXPECT_FALSE(url_decode("% 1", &output));
+ EXPECT_FALSE(url_decode("%1 ", &output));
+
+ // A '%' at, or one character from, the end of the input.
+ EXPECT_FALSE(url_decode("mydb%", &output));
+ EXPECT_FALSE(url_decode("mydb%2", &output));
+ EXPECT_FALSE(url_decode("100%", &output));
+}
+
+TEST(UrlCodingTest, WellFormedEscapesAreAccepted) {
+ std::string output;
+
+ // Both digit cases.
+ EXPECT_TRUE(url_decode("%2d%2D", &output));
+ EXPECT_EQ(output, "--");
+
+ // The whole hexadecimal alphabet, upper and lower.
+ EXPECT_TRUE(url_decode("%0a%0A%bf%BF%7e%7E", &output));
+ EXPECT_EQ(output, "\n\n\xbf\xbf~~");
+
+ // A NUL is a legal escape and must not end the string early.
+ EXPECT_TRUE(url_decode("a%00b", &output));
+ EXPECT_EQ(output, std::string("a\0b", 3));
+
+ // Multi byte UTF-8, one escape per byte.
+ EXPECT_TRUE(url_decode("%E4%B8%AD", &output));
+ EXPECT_EQ(output, "\xe4\xb8\xad");
+
+ // '+' still means a space, and an escaped space still means a space.
+ EXPECT_TRUE(url_decode("my+db", &output));
+ EXPECT_EQ(output, "my db");
+ EXPECT_TRUE(url_decode("my%20db", &output));
+ EXPECT_EQ(output, "my db");
+
+ // Nothing to decode.
+ EXPECT_TRUE(url_decode("mydb", &output));
+ EXPECT_EQ(output, "mydb");
+ EXPECT_TRUE(url_decode("", &output));
+ EXPECT_EQ(output, "");
+}
+
+TEST(UrlCodingTest, EncodeDecodeRoundTrip) {
+ // Every byte value survives a round trip through url_encode.
+ std::string all;
+ for (int i = 1; i < 256; ++i) {
+ all.push_back(static_cast<char>(i));
+ }
+
+ std::string encoded;
+ url_encode(all, &encoded);
+
+ std::string decoded;
+ EXPECT_TRUE(url_decode(encoded, &decoded));
+ EXPECT_EQ(all, decoded);
}
TEST(Base64Test, Basic) {
@@ -103,7 +157,7 @@ TEST(Base64Test, Basic) {
TEST(HtmlEscapingTest, Basic) {
std::string before = "<html><body>&";
std::stringstream after;
- EscapeForHtml(before, &after);
+ escape_for_html(before, &after);
EXPECT_EQ(after.str(), "<html><body>&amp");
}
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
