Copilot commented on code in PR #64935:
URL: https://github.com/apache/doris/pull/64935#discussion_r3489701711
##########
be/src/service/http/action/stream_load_forward_handler.cpp:
##########
@@ -31,6 +31,18 @@
namespace doris {
int StreamLoadForwardHandler::on_header(HttpRequest* req) {
+ const auto* params_for_auth = req->params();
+ auto token_it = params_for_auth->find("auth_token");
+ if (token_it != params_for_auth->end() &&
_exec_env->check_auth_token(token_it->second)) {
+ // fast path: FE passed internal auth token via query param
+ } else {
+ // fallback: validate via HttpHandlerWithAuth (Basic Auth -> FE RPC)
+ int auth_ret = HttpHandlerWithAuth::on_header(req);
+ if (auth_ret != 0) {
+ return auth_ret;
+ }
+ }
+
Review Comment:
The Basic Auth fallback currently depends on
`HttpHandlerWithAuth::on_header`, but that function returns success immediately
when `config::enable_all_http_auth` is false (default). That means requests
without a valid `auth_token` can still pass authentication and reach the
forward logic, which contradicts the PR goal of rejecting unauthenticated
access to `_stream_load_forward`.
##########
be/src/common/config.cpp:
##########
@@ -273,6 +273,7 @@ DEFINE_mInt32(download_low_speed_limit_kbps, "50");
DEFINE_mInt32(download_low_speed_time, "300");
// whether to download small files in batch
DEFINE_mBool(enable_batch_download, "true");
+DEFINE_Bool(enable_group_commit_streamload_be_forward, "false");
// whether to check md5sum when download
DEFINE_mBool(enable_download_md5sum_check, "false");
Review Comment:
This new config definition also lacks a descriptive comment (neighboring
configs document intent). Adding a short comment helps operators understand
what enabling it does.
##########
be/src/common/config.h:
##########
@@ -330,6 +330,7 @@ DECLARE_mInt32(download_low_speed_limit_kbps);
DECLARE_mInt32(download_low_speed_time);
// whether to download small files in batch.
DECLARE_mBool(enable_batch_download);
+DECLARE_Bool(enable_group_commit_streamload_be_forward);
// whether to check md5sum when download
Review Comment:
This new BE config is missing the one-line comment that nearby config
declarations consistently have, which makes `be.conf`/config docs harder to
maintain.
##########
fe/fe-core/src/main/java/org/apache/doris/httpv2/rest/LoadAction.java:
##########
@@ -800,11 +800,17 @@ private Backend selectBackendForGroupCommit(String
clusterName, HttpServletReque
* @return RedirectView configured for stream load forwarding
*/
private RedirectView redirectToStreamLoadForward(HttpServletRequest
request, TNetworkAddress addr,
- String forwardTarget) {
+ String forwardTarget) throws LoadException {
// Replace _stream_load with _stream_load_forward in the path.
String modifiedPath = request.getRequestURI().replace("/_stream_load",
"/_stream_load_forward");
String queryString = request.getQueryString();
- String redirectQuery = "forward_to=" + forwardTarget;
+ String authToken;
+ try {
+ authToken = Env.getCurrentEnv().getTokenManager().acquireToken();
+ } catch (Exception e) {
+ throw new LoadException("Failed to acquire auth token for stream
load forward", e);
+ }
+ String redirectQuery = "forward_to=" + forwardTarget + "&auth_token="
+ authToken;
if (!Strings.isNullOrEmpty(queryString)) {
redirectQuery = queryString + "&" + redirectQuery;
}
Review Comment:
`redirectToStreamLoadForward()` appends `auth_token` to the existing raw
query string without stripping any client-supplied `auth_token`. On BE, query
parsing uses `map.emplace(...)`, so the **first** occurrence of a duplicated
key wins; a client-provided `auth_token` can shadow the FE-generated token and
force the BE into the fallback path. Also, `authToken` is concatenated without
URL-encoding.
--
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]
