Skip to content

Commit 2db1f91

Browse files
lkotuladahlerlend
authored andcommitted
Bug#37697750 - User provided URL redirection should block fragment
Description =========== "URL Handling Best Practices" point that when executing redirection provided by user should be blocked, overriden or prepared fragment should be attached. Fix === When redirecting, remove "fragment" part from the URL. (POST-PUSH FIX WL#15440) Change-Id: I46d96eb09eda62b127c567e07644705e7e30fdb6
1 parent 512e87e commit 2db1f91

File tree

7 files changed

+53
-16
lines changed

7 files changed

+53
-16
lines changed

mysql-test/suite/router/include/mrs/mrs_client.inc

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -118,6 +118,7 @@ if ($mrs_client_arg_expect_header)
118118
--die "mrs_client_arg_expect_header" requires mrs_client_arg_expect_header_value parameter.
119119
}
120120
--let $mrs_escape_query=$mrs_client_arg_expect_header_value
121+
--let $mrs_include_character_and=1
121122
--source escape_url_query.inc
122123
--let $mrs_client_cmd=$mrs_client_cmd --encoded-expected-header=$mrs_client_arg_expect_header=$mrs_escape_query
123124
}

mysql-test/suite/router/r/authentication_redirect.result

Lines changed: 10 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,10 @@ grant all on *.* to mrsuser@'%';
2727
# 7. path part contains control characters
2828
# 8. relative path, directory traversing
2929
#
30+
## III. Execute user redirection, but change the target URL
31+
#
32+
# 1. all fragments must be reseted
33+
#
3034
# Registred SERVICE at path: /svc_no_valid
3135
# Registred SERVICE at path: /svc_open_valid
3236
# Registred AUTH APP at path: NULL
@@ -221,6 +225,12 @@ OK
221225
# II.8
222226
GET /svc_open_valid/authentication/login?onCompletionRedirect=../../admin
223227

228+
OK
229+
230+
#
231+
# III.1
232+
GET /svc_no_valid/authentication/login?onCompletionRedirect=/path1/#fragment
233+
224234
OK
225235
drop user mrsuser@'%';
226236
DROP SCHEMA basic_schema;

mysql-test/suite/router/t/authentication_redirect.test

Lines changed: 15 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -34,6 +34,10 @@ grant all on *.* to mrsuser@'%';
3434
--echo # 7. path part contains control characters
3535
--echo # 8. relative path, directory traversing
3636
--echo #
37+
--echo ## III. Execute user redirection, but change the target URL
38+
--echo #
39+
--echo # 1. all fragments must be reseted
40+
--echo #
3741

3842

3943
--source ../include/mrs/start_object_definition.inc
@@ -62,6 +66,17 @@ grant all on *.* to mrsuser@'%';
6266
--let $test_service=svc_open_valid
6367
--source ../include/test/authentication_redirect.inc
6468

69+
--echo
70+
--echo #
71+
--echo # III.1
72+
--let $mrs_client_arg_path='/svc_no_valid/authentication/login?onCompletionRedirect=/path1/#fragment'
73+
--let $mrs_client_arg_authentication=basic
74+
--let $mrs_client_arg_user=mrsuser
75+
--let $mrs_client_arg_password=S3kre7
76+
--let $mrs_client_arg_expect_header=Location
77+
--let $mrs_client_arg_expect_header_value="/path1/?authApp=Internal&login=success"
78+
--source ../include/mrs/mrs_client.inc
79+
6580
# Cleanup
6681
drop user mrsuser@'%';
6782
--source ../include/mrs/cleanup.inc

router/src/mysql_rest_service/src/mrs/endpoint/handler/authentication/handler_authorize_login.cc

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -200,6 +200,11 @@ std::string HandlerAuthorizeLogin::append_status_parameters(
200200
Url::append_query_parameter(uri, "login",
201201
get_authentication_status(error.status));
202202

203+
// Best practices of handling URL redirection point that
204+
// fragment should be blocked in some way.
205+
// We are not forwarding it.
206+
uri.set_fragment({});
207+
203208
return uri.join();
204209
}
205210

router/tests/mrs_client/client/authentication.cc

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -306,7 +306,7 @@ Result Authentication::do_basic_flow(
306306
"Response doesn't contains Set-Cookie with proper value.");
307307
}
308308

309-
return {HttpStatusCode::Ok, {}, {}};
309+
return {HttpStatusCode::Ok, request->get_input_headers(), {}};
310310
}
311311

312312
Result Authentication::do_basic_json_flow(
@@ -363,7 +363,7 @@ Result Authentication::do_basic_json_flow(
363363
// The session_id - cookie name is service dependent, thus we would also need
364364
// to transfer service_id.
365365

366-
return {HttpStatusCode::Ok, {}, {}};
366+
return {HttpStatusCode::Ok, request->get_input_headers(), {}};
367367
}
368368

369369
class Scram {
@@ -556,7 +556,7 @@ Result Authentication::do_scram_post_flow(
556556
request->get_session()->add_header(&header[0]);
557557
}
558558

559-
return {HttpStatusCode::Ok, {}, {}};
559+
return {HttpStatusCode::Ok, request->get_input_headers(), {}};
560560
}
561561

562562
Result Authentication::do_scram_get_flow(
@@ -619,7 +619,7 @@ Result Authentication::do_scram_get_flow(
619619
request->get_session()->add_header(&header[0]);
620620
}
621621

622-
return {HttpStatusCode::Ok, {}, {}};
622+
return {HttpStatusCode::Ok, request->get_input_headers(), {}};
623623
}
624624

625625
} // namespace mrs_client

router/tests/mrs_client/client/http_client_request.cc

Lines changed: 16 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -32,16 +32,6 @@
3232

3333
namespace mrs_client {
3434

35-
static Headers get_headers(const ::http::base::Request *request) {
36-
Headers result;
37-
auto &headers = request->get_input_headers();
38-
for (const auto &h : headers) {
39-
result.push_back(h);
40-
}
41-
42-
return result;
43-
}
44-
4535
HttpClientRequest::HttpClientRequest(net::io_context *context,
4636
HttpClientSession *session,
4737
const http::base::Uri &uri)
@@ -56,6 +46,20 @@ void HttpClientRequest::add_header(const char *name, const char *value) {
5646
one_shot_headers_.emplace_back(name, value);
5747
}
5848

49+
static Headers get_headers(const ::http::base::Request *request) {
50+
Headers result;
51+
auto &headers = request->get_input_headers();
52+
for (const auto &h : headers) {
53+
result.push_back(h);
54+
}
55+
56+
return result;
57+
}
58+
59+
const Headers &HttpClientRequest::get_input_headers() const {
60+
return input_headers_;
61+
}
62+
5963
Result HttpClientRequest::do_request(http::base::method::key_type method,
6064
const std::string &path,
6165
const std::string &body,
@@ -96,8 +100,8 @@ Result HttpClientRequest::do_request(http::base::method::key_type method,
96100
auto &ib = request.get_input_buffer();
97101
auto array = ib.pop_front(ib.length());
98102

99-
return Result{code, get_headers(&request),
100-
std::string(array.begin(), array.end())};
103+
input_headers_ = get_headers(&request);
104+
return Result{code, input_headers_, std::string(array.begin(), array.end())};
101105
}
102106

103107
} // namespace mrs_client

router/tests/mrs_client/client/http_client_request.h

Lines changed: 2 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -78,6 +78,7 @@ class HttpClientRequest {
7878
const http::base::Uri &uri);
7979

8080
void add_header(const char *name, const char *value);
81+
const Headers &get_input_headers() const;
8182
Result do_request(http::base::method::key_type type, const std::string &path,
8283
const std::string &body, bool set_new_cookies = true);
8384

@@ -91,6 +92,7 @@ class HttpClientRequest {
9192
std::unique_ptr<http::client::Client> client_;
9293
std::string response_;
9394
Headers one_shot_headers_;
95+
Headers input_headers_;
9496
};
9597

9698
} // namespace mrs_client

0 commit comments

Comments
 (0)