From 153059c55c7578c63d607f8f29a8ad6b00f0bb16 Mon Sep 17 00:00:00 2001 From: Daniel Robertson Date: Tue, 26 Apr 2016 22:36:59 -0400 Subject: [PATCH] Fix logic for cors cache match The current logic for a cors cache match does not consider "credentials is false and request's credentials mode is not "include" or credentials is true." --- components/net/fetch/cors_cache.rs | 8 ++-- components/net/fetch/methods.rs | 41 ++++++++++---------- components/net_traits/request.rs | 2 +- tests/unit/net/fetch.rs | 61 +++++++++++++++++++++++++++++- 4 files changed, 84 insertions(+), 28 deletions(-) diff --git a/components/net/fetch/cors_cache.rs b/components/net/fetch/cors_cache.rs index dc640ce4c375..55ad75455b7f 100644 --- a/components/net/fetch/cors_cache.rs +++ b/components/net/fetch/cors_cache.rs @@ -20,7 +20,7 @@ use url::Url; /// Union type for CORS cache entries /// /// Each entry might pertain to a header or method -#[derive(Clone)] +#[derive(Clone, Debug)] pub enum HeaderOrMethod { HeaderData(String), MethodData(Method) @@ -43,7 +43,7 @@ impl HeaderOrMethod { } /// An entry in the CORS cache -#[derive(Clone)] +#[derive(Clone, Debug)] pub struct CORSCacheEntry { pub origin: Origin, pub url: Url, @@ -112,7 +112,7 @@ pub struct BasicCORSCache(Vec); fn match_headers(cors_cache: &CORSCacheEntry, cors_req: &CacheRequestDetails) -> bool { cors_cache.origin == cors_req.origin && cors_cache.url == cors_req.destination && - cors_cache.credentials == cors_req.credentials + (cors_cache.credentials || !cors_req.credentials) } impl BasicCORSCache { @@ -150,7 +150,7 @@ impl CORSCache for BasicCORSCache { let BasicCORSCache(buf) = self.clone(); let now = time::now().to_timespec(); let new_buf: Vec = buf.into_iter() - .filter(|e| now.sec > e.created.sec + e.max_age as i64) + .filter(|e| now.sec < e.created.sec + e.max_age as i64) .collect(); *self = BasicCORSCache(new_buf); } diff --git a/components/net/fetch/methods.rs b/components/net/fetch/methods.rs index 479c9bc99510..bbf35fb02222 100644 --- a/components/net/fetch/methods.rs +++ b/components/net/fetch/methods.rs @@ -40,6 +40,10 @@ pub fn fetch_async(request: Request, listener: Box) { /// [Fetch](https://fetch.spec.whatwg.org#concept-fetch) pub fn fetch(request: Rc) -> Response { + fetch_with_cors_cache(request, &mut BasicCORSCache::new()) +} + +pub fn fetch_with_cors_cache(request: Rc, cache: &mut BasicCORSCache) -> Response { // Step 1 if request.window.get() == Window::Client { @@ -102,11 +106,11 @@ pub fn fetch(request: Rc) -> Response { // TODO: create a fetch record and append it to request's client's fetch group list } // Step 7 - main_fetch(request, false, false) + main_fetch(request, cache, false, false) } /// [Main fetch](https://fetch.spec.whatwg.org/#concept-main-fetch) -fn main_fetch(request: Rc, cors_flag: bool, recursive_flag: bool) -> Response { +fn main_fetch(request: Rc, cache: &mut BasicCORSCache, cors_flag: bool, recursive_flag: bool) -> Response { // TODO: Implement main fetch spec // Step 1 @@ -156,14 +160,14 @@ fn main_fetch(request: Rc, cors_flag: bool, recursive_flag: bool) -> Re current_url.scheme() == "about" || request.mode == RequestMode::Navigate { - basic_fetch(request.clone()) + basic_fetch(request.clone(), cache) } else if request.mode == RequestMode::SameOrigin { Response::network_error() } else if request.mode == RequestMode::NoCORS { request.response_tainting.set(ResponseTainting::Opaque); - basic_fetch(request.clone()) + basic_fetch(request.clone(), cache) } else if !matches!(current_url.scheme(), "http" | "https") { Response::network_error() @@ -175,7 +179,7 @@ fn main_fetch(request: Rc, cors_flag: bool, recursive_flag: bool) -> Re request.response_tainting.set(ResponseTainting::CORSTainting); request.redirect_mode.set(RedirectMode::Error); - let response = http_fetch(request.clone(), BasicCORSCache::new(), true, true, false); + let response = http_fetch(request.clone(), cache, true, true, false); if response.is_network_error() { // TODO clear cache entries using request } @@ -183,7 +187,7 @@ fn main_fetch(request: Rc, cors_flag: bool, recursive_flag: bool) -> Re } else { request.response_tainting.set(ResponseTainting::CORSTainting); - http_fetch(request.clone(), BasicCORSCache::new(), true, false, false) + http_fetch(request.clone(), cache, true, false, false) } } }; @@ -280,7 +284,7 @@ fn main_fetch(request: Rc, cors_flag: bool, recursive_flag: bool) -> Re } /// [Basic fetch](https://fetch.spec.whatwg.org#basic-fetch) -fn basic_fetch(request: Rc) -> Response { +fn basic_fetch(request: Rc, cache: &mut BasicCORSCache) -> Response { let url = request.current_url(); @@ -294,7 +298,7 @@ fn basic_fetch(request: Rc) -> Response { }, "http" | "https" => { - http_fetch(request.clone(), BasicCORSCache::new(), false, false, false) + http_fetch(request.clone(), cache, false, false, false) }, "data" => { @@ -324,7 +328,7 @@ fn basic_fetch(request: Rc) -> Response { /// [HTTP fetch](https://fetch.spec.whatwg.org#http-fetch) fn http_fetch(request: Rc, - mut cache: BasicCORSCache, + cache: &mut BasicCORSCache, cors_flag: bool, cors_preflight_flag: bool, authentication_fetch_flag: bool) -> Response { @@ -394,7 +398,7 @@ fn http_fetch(request: Rc, // Sub-substep 1 if method_mismatch || header_mismatch { - let preflight_result = cors_preflight_fetch(request.clone(), Some(cache)); + let preflight_result = cors_preflight_fetch(request.clone(), cache); // Sub-substep 2 if preflight_result.response_type == ResponseType::Error { return Response::network_error(); @@ -443,7 +447,7 @@ fn http_fetch(request: Rc, RedirectMode::Follow => { // set back to default response.return_internal.set(true); - http_redirect_fetch(request, Rc::new(response), cors_flag) + http_redirect_fetch(request, cache, Rc::new(response), cors_flag) } } }, @@ -466,7 +470,7 @@ fn http_fetch(request: Rc, } // Step 4 - return http_fetch(request, BasicCORSCache::new(), cors_flag, cors_preflight_flag, true); + return http_fetch(request, cache, cors_flag, cors_preflight_flag, true); } // Code 407 @@ -482,7 +486,7 @@ fn http_fetch(request: Rc, // TODO: Prompt the user for proxy authentication credentials // Step 4 - return http_fetch(request, BasicCORSCache::new(), + return http_fetch(request, cache, cors_flag, cors_preflight_flag, authentication_fetch_flag); } @@ -503,6 +507,7 @@ fn http_fetch(request: Rc, /// [HTTP redirect fetch](https://fetch.spec.whatwg.org#http-redirect-fetch) fn http_redirect_fetch(request: Rc, + cache: &mut BasicCORSCache, response: Rc, cors_flag: bool) -> Response { @@ -580,7 +585,7 @@ fn http_redirect_fetch(request: Rc, request.url_list.borrow_mut().push(location_url); // Step 15 - main_fetch(request, cors_flag, true) + main_fetch(request, cache, cors_flag, true) } /// [HTTP network or cache fetch](https://fetch.spec.whatwg.org#http-network-or-cache-fetch) @@ -917,7 +922,7 @@ fn http_network_fetch(request: Rc, } /// [CORS preflight fetch](https://fetch.spec.whatwg.org#cors-preflight-fetch) -fn cors_preflight_fetch(request: Rc, cache: Option) -> Response { +fn cors_preflight_fetch(request: Rc, cache: &mut BasicCORSCache) -> Response { // Step 1 let mut preflight = Request::new(request.current_url(), Some(request.origin.borrow().clone()), false); *preflight.method.borrow_mut() = Method::Options; @@ -995,12 +1000,6 @@ fn cors_preflight_fetch(request: Rc, cache: Option) -> // TODO: Substep 9 - Need to define what an imposed limit on max-age is - // Substep 10 - let mut cache = match cache { - Some(c) => c, - None => return response - }; - // Substep 11, 12 for method in &methods { cache.match_method_and_update(CacheRequestDetails { diff --git a/components/net_traits/request.rs b/components/net_traits/request.rs index e02529263c50..675a78fd788d 100644 --- a/components/net_traits/request.rs +++ b/components/net_traits/request.rs @@ -33,7 +33,7 @@ pub enum Destination { } /// A request [origin](https://fetch.spec.whatwg.org/#concept-request-origin) -#[derive(Clone, PartialEq)] +#[derive(Clone, PartialEq, Debug)] pub enum Origin { Client, Origin(UrlOrigin) diff --git a/tests/unit/net/fetch.rs b/tests/unit/net/fetch.rs index 8b95314c7266..88b0a14db679 100644 --- a/tests/unit/net/fetch.rs +++ b/tests/unit/net/fetch.rs @@ -3,7 +3,8 @@ * file, You can obtain one at http://mozilla.org/MPL/2.0/. */ use hyper::header::{AccessControlAllowCredentials, AccessControlAllowHeaders, AccessControlAllowOrigin}; -use hyper::header::{AccessControlAllowMethods, AccessControlRequestHeaders, AccessControlRequestMethod}; +use hyper::header::{AccessControlAllowMethods, AccessControlMaxAge}; +use hyper::header::{AccessControlRequestHeaders, AccessControlRequestMethod}; use hyper::header::{CacheControl, ContentLanguage, ContentType, Expires, LastModified}; use hyper::header::{Headers, HttpDate, Location, SetCookie, Pragma}; use hyper::method::Method; @@ -12,7 +13,8 @@ use hyper::server::{Handler, Listening, Server}; use hyper::server::{Request as HyperRequest, Response as HyperResponse}; use hyper::status::StatusCode; use hyper::uri::RequestUri; -use net::fetch::methods::{fetch, fetch_async}; +use net::fetch::cors_cache::{BasicCORSCache, CacheRequestDetails, CORSCache}; +use net::fetch::methods::{fetch, fetch_async, fetch_with_cors_cache}; use net_traits::AsyncFetchListener; use net_traits::request::{Origin, RedirectMode, Referer, Request, RequestMode}; use net_traits::response::{CacheState, Response, ResponseBody, ResponseType}; @@ -174,6 +176,61 @@ fn test_cors_preflight_fetch() { }; } +#[test] +fn test_cors_preflight_cache_fetch() { + static ACK: &'static [u8] = b"ACK"; + let state = Arc::new(AtomicUsize::new(0)); + let counter = state.clone(); + let mut cache = BasicCORSCache::new(); + let handler = move |request: HyperRequest, mut response: HyperResponse| { + if request.method == Method::Options && state.clone().fetch_add(1, Ordering::SeqCst) == 0 { + assert!(request.headers.has::()); + assert!(request.headers.has::()); + response.headers_mut().set(AccessControlAllowOrigin::Any); + response.headers_mut().set(AccessControlAllowCredentials); + response.headers_mut().set(AccessControlAllowMethods(vec![Method::Get])); + response.headers_mut().set(AccessControlMaxAge(6000)); + } else { + response.headers_mut().set(AccessControlAllowOrigin::Any); + response.send(ACK).unwrap(); + } + }; + let (mut server, url) = make_server(handler); + + let origin = Origin::Origin(UrlOrigin::new_opaque()); + let mut request = Request::new(url.clone(), Some(origin.clone()), false); + request.referer = Referer::NoReferer; + request.use_cors_preflight = true; + request.mode = RequestMode::CORSMode; + let wrapped_request0 = Rc::new(request.clone()); + let wrapped_request1 = Rc::new(request); + + let fetch_response0 = fetch_with_cors_cache(wrapped_request0, &mut cache); + let fetch_response1 = fetch_with_cors_cache(wrapped_request1, &mut cache); + let _ = server.close(); + + assert!(!fetch_response0.is_network_error() && !fetch_response1.is_network_error()); + + // The response from the CORS-preflight cache was used + assert_eq!(1, counter.load(Ordering::SeqCst)); + + // The entry exists in the CORS-preflight cache + assert_eq!(true, cache.match_method(CacheRequestDetails { + origin: origin, + destination: url, + credentials: false + }, Method::Get)); + + match *fetch_response0.body.lock().unwrap() { + ResponseBody::Done(ref body) => assert_eq!(&**body, ACK), + _ => panic!() + }; + match *fetch_response1.body.lock().unwrap() { + ResponseBody::Done(ref body) => assert_eq!(&**body, ACK), + _ => panic!() + }; +} + #[test] fn test_cors_preflight_fetch_network_error() { static ACK: &'static [u8] = b"ACK";