From 566147dab3d15dac82533668de2f92026ad52ff5 Mon Sep 17 00:00:00 2001 From: Bastien Orivel Date: Sat, 21 Dec 2019 13:40:39 +0100 Subject: [PATCH 1/5] Strip request-body-header when redirecting from a POST to GET This doesn't change any expectation because we're not setting response.redirected properly so all the tests fail later on when it's asserted to be true. Fixes #25257 --- components/net/http_loader.rs | 15 ++++++++++++++- components/script/dom/response.rs | 3 +-- 2 files changed, 15 insertions(+), 3 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 18dff8ca0ef..10c2d7faaec 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -25,7 +25,10 @@ use headers::{ use headers::{AccessControlAllowOrigin, AccessControlMaxAge}; use headers::{CacheControl, ContentEncoding, ContentLength}; use headers::{IfModifiedSince, LastModified, Origin as HyperOrigin, Pragma, Referer, UserAgent}; -use http::header::{self, HeaderName, HeaderValue}; +use http::header::{ + self, HeaderName, HeaderValue, CONTENT_ENCODING, CONTENT_LANGUAGE, CONTENT_LOCATION, + CONTENT_TYPE, +}; use http::{HeaderMap, Request as HyperRequest}; use hyper::{Body, Client, Method, Response as HyperResponse, StatusCode}; use hyper_serde::Serde; @@ -804,8 +807,18 @@ pub fn http_redirect_fetch( (*code == StatusCode::SEE_OTHER && request.method != Method::HEAD) }) { + // Step 11.1 request.method = Method::GET; request.body = None; + // Step 11.2 + for name in &[ + CONTENT_ENCODING, + CONTENT_LANGUAGE, + CONTENT_LOCATION, + CONTENT_TYPE, + ] { + request.headers.remove(name); + } } // Step 12 diff --git a/components/script/dom/response.rs b/components/script/dom/response.rs index 636acc05ff5..e605edeac2c 100644 --- a/components/script/dom/response.rs +++ b/components/script/dom/response.rs @@ -287,8 +287,7 @@ impl ResponseMethods for Response { // https://fetch.spec.whatwg.org/#dom-response-redirected fn Redirected(&self) -> bool { - let url_list_len = self.url_list.borrow().len(); - url_list_len > 1 + return *self.redirected.borrow(); } // https://fetch.spec.whatwg.org/#dom-response-status From aa43ce8cf32b2f44d58145df9198c36091aac5e1 Mon Sep 17 00:00:00 2001 From: Bastien Orivel Date: Sat, 21 Dec 2019 15:09:53 +0100 Subject: [PATCH 2/5] Fix the redirected attribute for Response --- components/net_traits/lib.rs | 3 + components/net_traits/response.rs | 1 + components/script/dom/response.rs | 6 ++ components/script/fetch.rs | 1 + .../api/redirect/redirect-method.any.js.ini | 66 ------------------- 5 files changed, 11 insertions(+), 66 deletions(-) diff --git a/components/net_traits/lib.rs b/components/net_traits/lib.rs index 2de544c31fe..a7100e0ef44 100644 --- a/components/net_traits/lib.rs +++ b/components/net_traits/lib.rs @@ -638,6 +638,8 @@ pub struct Metadata { pub referrer_policy: Option, /// Performance information for navigation events pub timing: Option, + /// True if the request comes from a redirection + pub redirected: bool, } impl Metadata { @@ -655,6 +657,7 @@ impl Metadata { referrer: None, referrer_policy: None, timing: None, + redirected: false, } } diff --git a/components/net_traits/response.rs b/components/net_traits/response.rs index 34e46ebc6a3..f509801678f 100644 --- a/components/net_traits/response.rs +++ b/components/net_traits/response.rs @@ -310,6 +310,7 @@ impl Response { metadata.https_state = response.https_state; metadata.referrer = response.referrer.clone(); metadata.referrer_policy = response.referrer_policy.clone(); + metadata.redirected = response.actual_response().url_list.len() > 1; metadata }; diff --git a/components/script/dom/response.rs b/components/script/dom/response.rs index e605edeac2c..d04cde07755 100644 --- a/components/script/dom/response.rs +++ b/components/script/dom/response.rs @@ -51,6 +51,7 @@ pub struct Response { body_promise: DomRefCell, BodyType)>>, #[ignore_malloc_size_of = "StreamConsumer"] stream_consumer: DomRefCell>, + redirected: DomRefCell, } #[allow(non_snake_case)] @@ -69,6 +70,7 @@ impl Response { body: DomRefCell::new(NetTraitsResponseBody::Empty), body_promise: DomRefCell::new(None), stream_consumer: DomRefCell::new(None), + redirected: DomRefCell::new(false), } } @@ -414,6 +416,10 @@ impl Response { *self.url.borrow_mut() = Some(final_url); } + pub fn set_redirected(&self, is_redirected: bool) { + *self.redirected.borrow_mut() = is_redirected; + } + fn set_response_members_by_type(&self, response_type: DOMResponseType) { match response_type { DOMResponseType::Error => { diff --git a/components/script/fetch.rs b/components/script/fetch.rs index 66b5ee3c6ef..fe0792d857f 100644 --- a/components/script/fetch.rs +++ b/components/script/fetch.rs @@ -306,6 +306,7 @@ fn fill_headers_with_metadata(r: DomRoot, m: Metadata) { r.set_headers(m.headers); r.set_raw_status(m.status); r.set_final_url(m.final_url); + r.set_redirected(m.redirected); } /// Convenience function for synchronously loading a whole resource. diff --git a/tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini b/tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini index 0b6787710cf..e2d28b96134 100644 --- a/tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini +++ b/tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini @@ -1,87 +1,21 @@ [redirect-method.any.worker.html] - [Redirect 301 with GET] - expected: FAIL - - [Redirect 301 with POST] - expected: FAIL - - [Redirect 301 with HEAD] - expected: FAIL - - [Redirect 302 with GET] - expected: FAIL - - [Redirect 302 with POST] - expected: FAIL - - [Redirect 302 with HEAD] - expected: FAIL - [Redirect 303 with GET] expected: FAIL - [Redirect 303 with POST] - expected: FAIL - - [Redirect 303 with HEAD] - expected: FAIL - - [Redirect 307 with GET] - expected: FAIL - [Redirect 307 with POST (string body)] expected: FAIL [Redirect 307 with POST (blob body)] expected: FAIL - [Redirect 307 with HEAD] - expected: FAIL - - [Redirect 303 with TESTING] - expected: FAIL - [redirect-method.any.html] - [Redirect 301 with GET] - expected: FAIL - - [Redirect 301 with POST] - expected: FAIL - - [Redirect 301 with HEAD] - expected: FAIL - - [Redirect 302 with GET] - expected: FAIL - - [Redirect 302 with POST] - expected: FAIL - - [Redirect 302 with HEAD] - expected: FAIL - [Redirect 303 with GET] expected: FAIL - [Redirect 303 with POST] - expected: FAIL - - [Redirect 303 with HEAD] - expected: FAIL - - [Redirect 307 with GET] - expected: FAIL - [Redirect 307 with POST (string body)] expected: FAIL [Redirect 307 with POST (blob body)] expected: FAIL - [Redirect 307 with HEAD] - expected: FAIL - - [Redirect 303 with TESTING] - expected: FAIL - From 2b2804244616d13203ea2d9d7f8ec6efbf0a0b94 Mon Sep 17 00:00:00 2001 From: Bastien Orivel Date: Sat, 21 Dec 2019 15:12:07 +0100 Subject: [PATCH 3/5] Fix a mistake in the redirect fetch code The spec says to ignore both HEAD and GET in step 11 --- components/net/http_loader.rs | 4 +++- .../metadata/fetch/api/redirect/redirect-method.any.js.ini | 6 ------ 2 files changed, 3 insertions(+), 7 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 10c2d7faaec..74262db3f1c 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -804,7 +804,9 @@ pub fn http_redirect_fetch( .map_or(false, |(code, _)| { ((*code == StatusCode::MOVED_PERMANENTLY || *code == StatusCode::FOUND) && request.method == Method::POST) || - (*code == StatusCode::SEE_OTHER && request.method != Method::HEAD) + (*code == StatusCode::SEE_OTHER && + request.method != Method::HEAD && + request.method != Method::GET) }) { // Step 11.1 diff --git a/tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini b/tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini index e2d28b96134..8f719427a8d 100644 --- a/tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini +++ b/tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini @@ -1,7 +1,4 @@ [redirect-method.any.worker.html] - [Redirect 303 with GET] - expected: FAIL - [Redirect 307 with POST (string body)] expected: FAIL @@ -10,9 +7,6 @@ [redirect-method.any.html] - [Redirect 303 with GET] - expected: FAIL - [Redirect 307 with POST (string body)] expected: FAIL From 4a1732a761da92a68b5929b1e889e087535be953 Mon Sep 17 00:00:00 2001 From: Josh Matthews Date: Wed, 22 Apr 2020 13:53:49 -0400 Subject: [PATCH 4/5] Remove outdated HTTP redirection handling code. --- components/net/http_loader.rs | 30 +++--------------------------- 1 file changed, 3 insertions(+), 27 deletions(-) diff --git a/components/net/http_loader.rs b/components/net/http_loader.rs index 74262db3f1c..c7002c4c9b4 100644 --- a/components/net/http_loader.rs +++ b/components/net/http_loader.rs @@ -360,11 +360,9 @@ fn obtain_response( client: &Client, url: &ServoUrl, method: &Method, - request_headers: &HeaderMap, + headers: &HeaderMap, data: &Option>, - load_data_method: &Method, pipeline_id: &Option, - iters: u32, request_id: Option<&str>, is_xhr: bool, context: &FetchContext, @@ -374,28 +372,7 @@ fn obtain_response( Error = NetworkError, >, > { - let mut headers = request_headers.clone(); - - // Avoid automatically sending request body if a redirect has occurred. - // - // TODO - This is the wrong behaviour according to the RFC. However, I'm not - // sure how much "correctness" vs. real-world is important in this case. - // - // https://tools.ietf.org/html/rfc7231#section-6.4 - let is_redirected_request = iters != 1; - let request_body; - match data { - &Some(ref d) if !is_redirected_request => { - headers.typed_insert(ContentLength(d.len() as u64)); - request_body = d.clone(); - }, - _ => { - if *load_data_method != Method::GET && *load_data_method != Method::HEAD { - headers.typed_insert(ContentLength(0)) - } - request_body = vec![]; - }, - } + let request_body = data.as_ref().cloned().unwrap_or(vec![]); context .timing @@ -455,6 +432,7 @@ fn obtain_response( let method = method.clone(); let send_start = precise_time_ms(); + let headers = headers.clone(); Box::new( client .request(request) @@ -1413,9 +1391,7 @@ fn http_network_fetch( &request.method, &request.headers, &request.body, - &request.method, &request.pipeline_id, - request.redirect_count + 1, request_id.as_ref().map(Deref::deref), is_xhr, context, From d86a429bd586e31d3a412b23a633da9935cc389a Mon Sep 17 00:00:00 2001 From: Bastien Orivel Date: Wed, 22 Apr 2020 20:45:38 +0200 Subject: [PATCH 5/5] Update test expectations --- .../api/basic/request-headers.any.js.ini | 5 --- .../api/redirect/redirect-method.any.js.ini | 15 -------- .../xhr/send-redirect-post-upload.htm.ini | 15 -------- .../xhr/send-redirect-to-cors.htm.ini | 35 ------------------- 4 files changed, 70 deletions(-) delete mode 100644 tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini delete mode 100644 tests/wpt/metadata/xhr/send-redirect-to-cors.htm.ini diff --git a/tests/wpt/metadata/fetch/api/basic/request-headers.any.js.ini b/tests/wpt/metadata/fetch/api/basic/request-headers.any.js.ini index 34647dbc095..1e27b18f095 100644 --- a/tests/wpt/metadata/fetch/api/basic/request-headers.any.js.ini +++ b/tests/wpt/metadata/fetch/api/basic/request-headers.any.js.ini @@ -3,14 +3,9 @@ [Fetch with POST with FormData body] expected: FAIL - [Fetch with Chicken] - expected: FAIL [request-headers.any.worker.html] type: testharness [Fetch with POST with FormData body] expected: FAIL - [Fetch with Chicken] - expected: FAIL - diff --git a/tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini b/tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini deleted file mode 100644 index 8f719427a8d..00000000000 --- a/tests/wpt/metadata/fetch/api/redirect/redirect-method.any.js.ini +++ /dev/null @@ -1,15 +0,0 @@ -[redirect-method.any.worker.html] - [Redirect 307 with POST (string body)] - expected: FAIL - - [Redirect 307 with POST (blob body)] - expected: FAIL - - -[redirect-method.any.html] - [Redirect 307 with POST (string body)] - expected: FAIL - - [Redirect 307 with POST (blob body)] - expected: FAIL - diff --git a/tests/wpt/metadata/xhr/send-redirect-post-upload.htm.ini b/tests/wpt/metadata/xhr/send-redirect-post-upload.htm.ini index 343465b443c..0ede2741b88 100644 --- a/tests/wpt/metadata/xhr/send-redirect-post-upload.htm.ini +++ b/tests/wpt/metadata/xhr/send-redirect-post-upload.htm.ini @@ -1,20 +1,5 @@ [send-redirect-post-upload.htm] type: testharness - [XMLHttpRequest: The send() method: POSTing to URL that redirects (301)] - expected: FAIL - - [XMLHttpRequest: The send() method: POSTing to URL that redirects (302)] - expected: FAIL - - [XMLHttpRequest: The send() method: POSTing to URL that redirects (303)] - expected: FAIL - [XMLHttpRequest: The send() method: POSTing to URL that redirects (307)] expected: FAIL - [XMLHttpRequest: The send() method: POSTing to URL that redirects (307 (string))] - expected: FAIL - - [XMLHttpRequest: The send() method: POSTing to URL that redirects (307 (blob))] - expected: FAIL - diff --git a/tests/wpt/metadata/xhr/send-redirect-to-cors.htm.ini b/tests/wpt/metadata/xhr/send-redirect-to-cors.htm.ini deleted file mode 100644 index fcc36e00a53..00000000000 --- a/tests/wpt/metadata/xhr/send-redirect-to-cors.htm.ini +++ /dev/null @@ -1,35 +0,0 @@ -[send-redirect-to-cors.htm] - type: testharness - [XMLHttpRequest: send() - Redirect to CORS-enabled resource (307 post with string)] - expected: FAIL - - [XMLHttpRequest: send() - Redirect to CORS-enabled resource (307 post with typed array)] - expected: FAIL - - [XMLHttpRequest: send() - Redirect to CORS-enabled resource (308 FOO with string and explicit Content-Type text/plain)] - expected: FAIL - - [XMLHttpRequest: send() - Redirect to CORS-enabled resource (308 FOO with string and explicit Content-Type multipart/form-data)] - expected: FAIL - - [XMLHttpRequest: send() - Redirect to CORS-enabled resource (308 FOO with string and explicit Content-Type safelisted)] - expected: FAIL - - [XMLHttpRequest: send() - Redirect to CORS-enabled resource (307 POST with string and explicit Content-Type safelisted)] - expected: FAIL - - [XMLHttpRequest: send() - Redirect to CORS-enabled resource (302 FOO with string and explicit Content-Type safelisted)] - expected: FAIL - - [XMLHttpRequest: send() - Redirect to CORS-enabled resource (301 POST with string and explicit Content-Type safelisted)] - expected: FAIL - - [XMLHttpRequest: send() - Redirect to CORS-enabled resource (303 FOO with string and explicit Content-Type safelisted)] - expected: FAIL - - [XMLHttpRequest: send() - Redirect to CORS-enabled resource (302 POST with string and explicit Content-Type)] - expected: FAIL - - [XMLHttpRequest: send() - Redirect to CORS-enabled resource (301 POST with string and explicit Content-Type)] - expected: FAIL -