github-actions[bot] commented on code in PR #68454:
URL: https://github.com/apache/doris/pull/68454#discussion_r4089142904
##########
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) {
Review Comment:
[P2] Keep FE folding aligned with the stricter decoder
This check newly rejects `%+1` because `+` is not a hex digit, but FE
constant folding still delegates to Java `URLDecoder`, which accepts `%+1` and
produces byte `0x01`. As a result, `HEX(URL_DECODE('%+1'))` can fold to `01`,
while the same value supplied through a nonconstant expression reaches
`FunctionUrlDecode` and fails with `Decode url failed`. Please make the FE
executable implementation enforce the same two-ASCII-hex-digit grammar (or
decline folding for it) and add paired constant/runtime coverage.
##########
be/src/service/http/action/stream_load.cpp:
##########
@@ -252,11 +252,17 @@ 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) ||
Review Comment:
[P2] Validate the wildcard path values, not query overrides
`req->param` reads the combined parameter map: query keys are inserted
first, then `PathTrie::put` uses non-overwriting `map::insert` for `{db}` and
`{table}`. Thus
`/api/prod%zzbackup/orders/_stream_load?db=safe_db&table=safe_table` reaches
this line with the query values, passes validation, and starts a load without
ever checking the malformed path token. Please make route captures
authoritative or keep them separate from query parameters, and cover duplicate
path/query keys in the handler regression.
##########
be/test/util/url_coding_test.cpp:
##########
@@ -18,77 +18,73 @@
#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;
+ // A '%' must be followed by exactly two hexadecimal digits.
+ EXPECT_FALSE(url_decode("prod%zzbackup", &output));
Review Comment:
[P2] Cover the stream-load rejection at the handler boundary
These malformed cases call `url_decode` directly, so they still pass if
`StreamLoadAction::on_header` goes back to ignoring the returned `false`.
Please add a handler-level regression for malformed database and table route
parameters that verifies the `InvalidArgument` response and that
group-commit/transaction/planning setup is not entered. That covers the
wrong-target behavior this PR is intended to fix, including the new
reply/cleanup path.
##########
be/src/service/http/action/stream_load.cpp:
##########
@@ -252,11 +252,17 @@ 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)) {
Review Comment:
[P2] Do not retain decoded prefixes for rejected route names
`url_decode` appends into its output before returning `false`, so
`prod%zzbackup` leaves `ctx->db == "prod"` (and the short-circuit leaves the
table empty). Although load setup is now stopped, the unconditional request log
and failed stream-load record still attribute the rejected request to that
valid-looking prefix. Decode both names into temporaries and assign the context
only after both succeed, or otherwise preserve the raw rejected names, so
failed requests cannot be recorded against the wrong database/table.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
---------------------------------------------------------------------
To unsubscribe, e-mail: [email protected]
For additional commands, e-mail: [email protected]
