Write unit tests for cookie isolation

[jxs]

closes #7624

[jdm]

Good start!
-S-awaiting-review +S-needs-code-changes

---

Reviewed 1 of 1 files at r1.
Review status: all files reviewed at latest revision, 7 unresolved discussions, all commit checks successful.

---

^{tests/unit/net/http_loader.rs, line 464 [r1] (raw file):}
s/set/send/

---

^{tests/unit/net/http_loader.rs, line 467 [r1] (raw file):}
Let's use the url variable here instead.

---

^{tests/unit/net/http_loader.rs, line 473 [r1] (raw file):}
Let's add an AssertRequestMustNotHaveHeaders for clarity (and because this won't actually test what we want until #7646).

---

^{tests/unit/net/http_loader.rs, line 482 [r1] (raw file):}
No need for the httpsonly bit in this name.

---

^{tests/unit/net/http_loader.rs, line 484 [r1] (raw file):}
This is a good start, but we can be more thorough. Instead of setting cookies manually by sending a SetCookiesForUrl for this test, we should be making a request that produces a response that includes a Set-Cookie header. Does that make sense?

---

^{tests/unit/net/http_loader.rs, line 495 [r1] (raw file):}
This should be https://, and we should assert that the cookie was set properly after doing this.

---

^{tests/unit/net/http_loader.rs, line 501 [r1] (raw file):}
Let's use the AssertRequestMustNotHaveHeaders from earlier.

---

Comments from the review on Reviewable.io

[jxs]

thanks @jdm, updated it, but still have 2 issues:

• getting this :

http_loader::test_when_cookie_set_marked_httpsonly_secure_isnt_sent_on_http_request stdout ----
    thread 'http_loader::test_when_cookie_set_marked_httpsonly_secure_isnt_sent_on_http_request' panicked at 'assertion failed: `(left == right)` (left: `"mozillaIs=theBest; HttpOnly; Secure;"`, right: `"mozillaIs=theBest"`)', /Volumes/data/dev/jxs/servo/tests/unit/net/http_loader.rs:227

on making a request that produces a response, should i use MockRequest/MockResponse ?
and how do i integrate it with a ResourceTask ?

[jdm]

That's the correct output; the code that fetches cookies for a given URL only returns data in the form "name=value" (see http://mxr.mozilla.org/servo/source/components/net/cookie_storage.rs#100), so the expected output needs to be corrected.

[jdm]

As for making a response, yes, we want a MockRequest that uses ResponseType::WithHeaders like http://mxr.mozilla.org/servo/source/tests/unit/net/http_loader.rs#426 .

[jdm]

-S-awaiting-review +S-needs-code-changes

---

Reviewed 1 of 1 files at r2.
Review status: all files reviewed at latest revision, 4 unresolved discussions, all commit checks successful.

---

^{tests/unit/net/http_loader.rs, line 178 [r2] (raw file):}
Since we're not checking the header values, let's just store Vec<String> instead.

---

^{tests/unit/net/http_loader.rs, line 197 [r2] (raw file):}
I think this can be for header in &self.headers_not_expected {

---

Comments from the review on Reviewable.io

[jxs]

thanks! updated
but still having one failing, now on:

http_loader::test_load_doesnt_send_cookie_if_nonhttp stdout ----
    thread 'http_loader::test_load_doesnt_send_cookie_if_nonhttp' panicked at 'assertion failed: self.request_headers.get_raw(header).is_none()', /Volumes/data/dev/jxs/servo/tests/unit/net/http_loader.rs:198

[jdm]

Oh! That's because the premise of the test is incorrect; my apologies! We do actually want to send NonHTTP cookies with HTTP requests, so let's make this test for that instead by using AssertRequestMustHaveHeaders :)

My original request was confused - what I was thinking of is that cookies that are set with httponly should not be available when using GetCookiesForUrl and passing CookieSource::NonHttp.

[jxs]

no problem :)
updated it, do you want me to also test for that, cookies set with httponly should not be available when using GetCookiesForUrl and passing CookieSource::NonHttp?

[jdm]

Yes, it looks like we don't have any for that. Let's have the test use a request that uses Set-Cookie, rather than setting it directly, too.

[jxs]

ok, added test_cookie_set_with_httponly_should_not_be_available_using_getcookiesforurl

[bors-servo]

☔ The latest upstream changes (presumably #7654) made this pull request unmergeable. Please resolve the merge conflicts.

[jxs]

@jdm r+ ?

[jdm]

@bors-servo: retry

• #8416

[bors-servo]

⌛ Testing commit 0c685ab with merge 13f5136...

[bors-servo]

💔 Test failed - mac-dev-ref-unit

[jdm]

Uh oh:

---- http_loader::test_when_cookie_set_marked_httpsonly_secure_isnt_sent_on_http_request stdout ----
    thread 'http_loader::test_when_cookie_set_marked_httpsonly_secure_isnt_sent_on_http_request' panicked at 'assertion failed: self.request_headers.get_raw(header).is_none()', /Users/servo/buildbot/slave/mac-dev-ref-unit/build/tests/unit/net/http_loader.rs:265

[jdm]

Ah, I misread the last change; my mistake. https://github.com/servo/servo/pull/7646/files#diff-c007a862cc2c468a1b2ae7aa7707e35dR876 says we're setting a secure cookie for https://mozilla.com as expected, but we're also making a request to https://mozilla.com, which explains why the test is failing. We should be requesting http://mozilla.com instead, and making sure that the previously stored cookie is not present in the request.

[jxs]

updated, and makes sense! test still failing though

[jdm]

Reviewed 1 of 1 files at r7.
Review status: all files reviewed at latest revision, 1 unresolved discussion.

---

^{tests/unit/net/http_loader.rs, line 877 [r7] (raw file):}
I have a theory - what if we use example.com instead of mozilla.com? I think the insecure request is being upgraded due to the HSTS preload entry: http://mxr.mozilla.org/servo/source/resources/hsts_preload.json#1856

---

Comments from the review on Reviewable.io

[jdm]

@bors-servo: r+

---

Reviewed 1 of 1 files at r8, 1 of 1 files at r9.
Review status: all files reviewed at latest revision, all discussions resolved.

---

Comments from the review on Reviewable.io

[bors-servo]

📌 Commit 79fcd1d has been approved by jdm

[bors-servo]

⌛ Testing commit 79fcd1d with merge e394c0d...

[bors-servo]

☀️ Test successful - android, gonk, linux-dev, linux-rel, mac-dev-ref-unit, mac-rel-css, mac-rel-wpt