Fix logic for cors cache match #10867

dlrobertson · 2016-04-27T02:45:27Z

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."

I could have missed something, but CacheRequestDetails::credentials is set to true if credentials mode is "include", and false otherwise. So (!cors_cache.credentials && !cors_req.credentials) || cors_cache.credentials would be directly following the spec, but unless I'm mistaken cors_cache.credentials || !cors_req.credentials is logically the same.

Fixes: #10525

This change is

highfive · 2016-04-27T02:45:30Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/fetch/cors_cache.rs

highfive · 2016-04-27T02:45:31Z

Warning

These commits modify net code, but no tests are modified. Please consider adding a test!

KiChjang · 2016-04-27T02:53:58Z

components/net/fetch/cors_cache.rs

@@ -112,7 +112,7 @@ pub struct BasicCORSCache(Vec<CORSCacheEntry>);
 fn match_headers(cors_cache: &CORSCacheEntry, cors_req: &CacheRequestDetails) -> bool {
    cors_cache.origin == cors_req.origin &&
        cors_cache.url == cors_req.destination &&


Hmm... spec says "url is request's current url", so I'd expect to see something like cors_cache.url == cors_req.current_url() instead.

Great point. Didn't think to question that since the Issue was focused on the later logic. After looking at CacheRequestDetails, destination is the only Url member. Is destination used as the "current URL" in here?

Wait, sorry. I didn't notice that cors_req is an instance of CacheRequestDetails, so the code as it is is fine.

No worries. Thanks for taking a peek at the code!

KiChjang · 2016-04-27T03:23:20Z

@bors-servo r+

bors-servo · 2016-04-27T03:23:21Z

📌 Commit c01f8af has been approved by KiChjang

jdm · 2016-04-27T03:40:24Z

@bors-servo: r-
Before we merge these changes, I'd like to either have a test for it or be explicit that we can't write one.

dlrobertson · 2016-04-27T03:43:57Z

No problem. Where are the cors tests?

frewsxcv · 2016-04-27T03:46:44Z

https://github.com/servo/servo/tree/master/tests/wpt/web-platform-tests/cors

jdm · 2016-04-27T11:26:58Z

For the fetch code, it's tests/unit/net/fetch.rs. That code isn't hooked up to real WPT tests yet.

dlrobertson · 2016-04-27T18:12:16Z

Review status: 0 of 4 files reviewed at latest revision, 2 unresolved discussions.

components/net/fetch/cors_cache.rs, line 153 [r2] (raw file):
> will find the entries that need to be dropped. < to find the ones to keep.

Comments from Reviewable

KiChjang · 2016-04-27T19:36:43Z

components/net/fetch/methods.rs

@@ -40,6 +40,10 @@ pub fn fetch_async(request: Request, listener: Box<AsyncFetchListener + Send>) {

 /// [Fetch](https://fetch.spec.whatwg.org#concept-fetch)
 pub fn fetch(request: Rc<Request>) -> Response {
+    fetch_with_cors_cache(request, &mut BasicCORSCache::new())


This is not behaviour documented in the fetch spec. @jdm does this look ok?

Related to http://logs.glob.uno/?c=mozilla%23servo#c416083

KiChjang · 2016-04-27T19:49:41Z

components/net/fetch/methods.rs

+    fetch_with_cors_cache(request, &mut BasicCORSCache::new())
+}
+
+pub fn fetch_with_cors_cache<C: CORSCache>(request: Rc<Request>, cache: &mut C) -> Response {


I'm not sure why cache: &mut CORSCache wouldn't work here.

CORSCache is a trait, so you'll get the following error.

error: the trait bound `<trait>: std::marker::Sized` is not satisfied

I couldn't think of another way around it


        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."


        Cleanup CORSCache

Remove the CORSCache trait, CORSCacheSender, CORSCacheThreadMsg, and CORSCacheThread. Rename BasicCORSCache to CORSCache and keep its old implementation of CORSCache.

KiChjang · 2016-04-28T19:18:34Z

components/net/fetch/cors_cache.rs

@@ -74,51 +73,20 @@ pub struct CacheRequestDetails {
    pub credentials: bool


I'm wondering whether we even need this CacheRequestDetails struct at all... it feels like we can just do away this altogether and use Request instead. I think I'll file a follow-up issue to remove this.

Filed #10904.

KiChjang · 2016-04-28T19:19:11Z

@bors-servo r+

Thanks!

bors-servo · 2016-04-28T19:19:12Z

📌 Commit 483f07c has been approved by KiChjang

bors-servo · 2016-04-28T19:19:13Z

📌 Commit 483f07c has been approved by KiChjang

bors-servo · 2016-04-28T20:36:06Z

⌛ Testing commit 483f07c with merge 3d38a60...


        Auto merge of #10867 - danlrobertson:sandbox, r=KiChjang

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." I could have missed something, but `CacheRequestDetails::credentials` is set to true if credentials mode is "include", and false otherwise. So `(!cors_cache.credentials && !cors_req.credentials) || cors_cache.credentials` would be directly following the spec, but unless I'm mistaken `cors_cache.credentials || !cors_req.credentials` is logically the same. Fixes: #10525  --- This change is [<img src="https://reviewable.io/review_button.svg" height="35" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/10867)

bors-servo · 2016-04-28T21:30:02Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel, mac-dev-unit, mac-rel-css, mac-rel-wpt

highfive assigned jdm Apr 27, 2016

highfive added the S-awaiting-review label Apr 27, 2016

KiChjang reviewed Apr 27, 2016
View changes

KiChjang assigned KiChjang and unassigned jdm Apr 27, 2016

KiChjang added S-needs-code-changes and removed S-awaiting-review labels Apr 27, 2016

highfive added S-awaiting-merge and removed S-needs-code-changes labels Apr 27, 2016

jdm added S-needs-tests and removed S-awaiting-merge labels Apr 27, 2016

dlrobertson force-pushed the dlrobertson:sandbox branch from c01f8af to bc32e8e Apr 27, 2016

highfive added the S-awaiting-review label Apr 27, 2016

KiChjang reviewed Apr 27, 2016
View changes

KiChjang added S-needs-code-changes S-awaiting-answer and removed S-awaiting-review labels Apr 27, 2016

Fix logic for cors cache match

Unverified

The committer email address is not verified.

Learn about signing commits

153059c

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."

dlrobertson force-pushed the dlrobertson:sandbox branch from bc32e8e to 9d1b9da Apr 27, 2016

highfive removed the S-needs-code-changes label Apr 27, 2016

highfive added the S-awaiting-review label Apr 27, 2016

dlrobertson force-pushed the dlrobertson:sandbox branch from 9d1b9da to 483f07c Apr 27, 2016

KiChjang reviewed Apr 28, 2016
View changes

highfive added S-awaiting-merge and removed S-awaiting-review S-awaiting-answer labels Apr 28, 2016

KiChjang removed the S-needs-tests label Apr 28, 2016

bors-servo merged commit 483f07c into servo:master Apr 28, 2016
3 checks passed

3 checks passed

continuous-integration/appveyor/pr AppVeyor build succeeded
Details

continuous-integration/travis-ci/pr The Travis CI build passed
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label Apr 28, 2016

servo / servo

Fix logic for cors cache match #10867

Fix logic for cors cache match #10867

dlrobertson commented Apr 27, 2016 •

edited by larsbergstrom

highfive commented Apr 27, 2016

highfive commented Apr 27, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Apr 27, 2016

bors-servo commented Apr 27, 2016

jdm commented Apr 27, 2016

dlrobertson commented Apr 27, 2016

frewsxcv commented Apr 27, 2016

jdm commented Apr 27, 2016

dlrobertson commented Apr 27, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Apr 28, 2016 •

edited by larsbergstrom

bors-servo commented Apr 28, 2016

bors-servo commented Apr 28, 2016

bors-servo commented Apr 28, 2016

bors-servo commented Apr 28, 2016

		@@ -74,51 +73,20 @@ pub struct CacheRequestDetails {
		pub credentials: bool

servo / servo

Join GitHub today

Fix logic for cors cache match #10867

Fix logic for cors cache match #10867

Conversation

dlrobertson commented Apr 27, 2016 • edited by larsbergstrom

highfive commented Apr 27, 2016

highfive commented Apr 27, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

KiChjang commented Apr 27, 2016

bors-servo commented Apr 27, 2016

jdm commented Apr 27, 2016

dlrobertson commented Apr 27, 2016

frewsxcv commented Apr 27, 2016

jdm commented Apr 27, 2016

dlrobertson commented Apr 27, 2016

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

dlrobertson Apr 27, 2016 • edited by larsbergstrom

This comment has been minimized.

This comment has been minimized.

KiChjang commented Apr 28, 2016 • edited by larsbergstrom

bors-servo commented Apr 28, 2016

bors-servo commented Apr 28, 2016

bors-servo commented Apr 28, 2016

bors-servo commented Apr 28, 2016

dlrobertson commented Apr 27, 2016 •

edited by larsbergstrom

dlrobertson Apr 27, 2016 •

edited by larsbergstrom

KiChjang commented Apr 28, 2016 •

edited by larsbergstrom