Copilot commented on code in PR #65376:
URL: https://github.com/apache/doris/pull/65376#discussion_r3543074231
##########
be/src/http/action/stream_load_forward_handler.cpp:
##########
@@ -32,6 +32,18 @@ namespace doris {
#include "common/compile_check_begin.h"
int StreamLoadForwardHandler::on_header(HttpRequest* req) {
+ if (!config::enable_group_commit_streamload_be_forward) {
+ HttpChannel::send_reply(req, HttpStatus::FORBIDDEN,
+ "Stream load forward is disabled. "
+ "Set
enable_group_commit_streamload_be_forward=true to enable.");
+ return -1;
+ }
+
+ int auth_ret = HttpHandlerWithAuth::on_header(req);
+ if (auth_ret != 0) {
+ return auth_ret;
+ }
Review Comment:
`HttpHandlerWithAuth::on_header()` only enforces authentication when
`config::enable_all_http_auth` is true (it returns 0 otherwise). With the
current change, enabling `enable_group_commit_streamload_be_forward` while
leaving `enable_all_http_auth` at its default `false` still leaves
`_stream_load_forward` unauthenticated, which defeats the intent of adding auth
for this security-sensitive load endpoint. Consider failing closed (or doing
explicit Basic/token auth like normal stream load) when the forward endpoint is
enabled.
##########
be/src/http/action/stream_load_forward_handler.h:
##########
@@ -98,9 +99,10 @@ class StreamLoadForwardContext {
// Stream Load request forward handler
// Forwards Stream Load requests to other BE nodes
// Supports streaming forward, maintains original request path format:
/api/{db}/{table}/_stream_load_forward
-class StreamLoadForwardHandler : public HttpHandler {
+class StreamLoadForwardHandler : public HttpHandlerWithAuth {
public:
- StreamLoadForwardHandler() = default;
+ explicit StreamLoadForwardHandler(ExecEnv* exec_env)
+ : HttpHandlerWithAuth(exec_env, TPrivilegeHier::GLOBAL,
TPrivilegeType::LOAD) {}
~StreamLoadForwardHandler() override = default;
Review Comment:
The forward endpoint uses `TPrivilegeHier::GLOBAL` + `TPrivilegeType::LOAD`,
but FE stream load authorization is checked at table scope
(`PrivPredicate.LOAD` on db+table). This mismatch can reject users who
legitimately have LOAD on the target table but not global LOAD, causing
unexpected 403s when clients follow the redirect to `_stream_load_forward`. The
BE-side check should align with FE by authenticating at TABLE hierarchy and
populating `TPrivilegeCtrl.db/tbl` from the request path params.
##########
be/src/http/action/stream_load_forward_handler.cpp:
##########
@@ -32,6 +32,18 @@ namespace doris {
#include "common/compile_check_begin.h"
int StreamLoadForwardHandler::on_header(HttpRequest* req) {
+ if (!config::enable_group_commit_streamload_be_forward) {
+ HttpChannel::send_reply(req, HttpStatus::FORBIDDEN,
+ "Stream load forward is disabled. "
+ "Set
enable_group_commit_streamload_be_forward=true to enable.");
+ return -1;
+ }
Review Comment:
`HttpHandler::on_header()` is documented to return 0 on success and -1 on
failure (after sending the reply). In this handler, the function returns
`HttpStatus::*` codes (e.g., `BAD_REQUEST`, `INTERNAL_SERVER_ERROR`, `OK`) in
later branches, which are positive values and therefore treated as success by
`EvHttpServer::on_header` (it only checks `res < 0`). That means error paths
can still proceed to body reading + `handle()`, potentially sending multiple
replies and wasting resources. The function should return 0 for success and -1
for all failure paths.
##########
regression-test/suites/load_p0/stream_load/test_group_commit_redirect.groovy:
##########
@@ -93,6 +93,7 @@ suite('test_group_commit_redirect', 'docker') {
options.beNum = 3
options.cloudMode = true
options.beConfigs.add('enable_java_support=false')
+ options.beConfigs.add('enable_group_commit_streamload_be_forward=true')
options.feConfigs.add('enable_group_commit_streamload_be_forward=true')
Review Comment:
This suite enables `enable_group_commit_streamload_be_forward`, but does not
explicitly enable BE-side HTTP auth. Since `HttpHandlerWithAuth` only enforces
auth when `enable_all_http_auth=true`, the test may not actually exercise the
newly added auth path for `_stream_load_forward` and could pass even if auth is
bypassed. Consider setting `enable_all_http_auth=true` in BE configs here so
the regression test covers the security behavior.
--
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]
