(Do not merge) Partially implement blocking as mixed content #16320

nox · 2017-04-09T15:45:10Z

This change is

highfive · 2017-04-09T15:45:13Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/dom/request.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/htmlscriptelement.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/document.rs, components/script/dom/workerglobalscope.rs, components/script/dom/htmlmediaelement.rs, components/script/stylesheet_loader.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/htmlimageelement.rs, components/script/script_thread.rs, components/script/dom/htmliframeelement.rs, components/script/dom/globalscope.rs, components/script/dom/bindings/trace.rs, components/script/dom/eventsource.rs, components/script/dom/serviceworkerglobalscope.rs
@KiChjang: components/script/dom/request.rs, components/script/dom/xmlhttprequest.rs, components/script/dom/htmlscriptelement.rs, components/net_traits/response.rs, components/net_traits/response.rs, components/script/dom/window.rs, components/script_traits/lib.rs, components/script_traits/lib.rs, components/script/dom/document.rs, components/net/http_loader.rs, components/script/dom/workerglobalscope.rs, components/net_traits/lib.rs, components/net_traits/lib.rs, components/script/dom/htmlmediaelement.rs, components/script/stylesheet_loader.rs, components/net/blob_loader.rs, components/script/dom/dedicatedworkerglobalscope.rs, components/script/dom/htmlimageelement.rs, components/script/script_thread.rs, components/script/dom/htmliframeelement.rs, components/net/fetch/methods.rs, components/script/dom/globalscope.rs, components/script/dom/bindings/trace.rs, components/script/dom/eventsource.rs, components/net_traits/request.rs, components/net_traits/request.rs, components/script/dom/serviceworkerglobalscope.rs

highfive · 2017-04-09T15:45:14Z

Warning

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

nox · 2017-04-09T15:45:34Z

@bors-servo try

bors-servo · 2017-04-09T15:45:38Z

⌛ Trying commit 93a5f07 with merge 887456f...


        Auto merge of #16320 - nox:mixed, r=<try>

(Do not merge) Partially implement blocking as mixed content

bors-servo · 2017-04-09T16:22:56Z

☀️ Test successful - android, arm32, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-css, mac-rel-wpt1, mac-rel-wpt2, windows-msvc-dev
State: approved= try=True

nox · 2017-04-09T16:43:49Z

This covers the script and style cases from #8582, AFAICT.

jdm · 2017-04-09T21:07:32Z

This should modify script/layout_image.rs as well.

jdm · 2017-04-09T23:50:49Z

I'm interested in having @avadacatavra review this.

avadacatavra · 2017-04-10T12:14:07Z

Reviewed 7 of 7 files at r1, 1 of 1 files at r2, 1 of 1 files at r3, 19 of 19 files at r4.
Review status: 23 of 204 files reviewed at latest revision, 1 unresolved discussion.

components/net/http_loader.rs, line 1178 at r2 (raw file):

        "https" => HttpsState::Modern,
        "http" => HttpsState::None,
        _ => panic!("The current URL's scheme should be an HTTP(S) scheme here."),

do we want to panic here or return an error?

Comments from Reviewable

avadacatavra · 2017-04-11T21:59:34Z

components/net/http_loader.rs

@@ -1102,6 +1105,18 @@ fn http_network_fetch(request: &Request,
    response.headers = res.response.headers.clone();
    response.referrer = request.referrer.to_url().cloned();

+    {
+        let http_message = res.response.get_ref().downcast_ref::<Http11Message>().unwrap();


all of this is to figure out if the response is served over https, right?

Yes. I could more simply just check that the request's URL scheme was "https", but this checks that the very stream that handles the bits over the wire actually uses an HttpsStream, so I think that is better. And we will need that code when we will finally look at the actual cypher anyway.

avadacatavra · 2017-04-11T22:24:32Z

components/net/Cargo.toml

@@ -15,9 +15,9 @@ brotli = "1.0.6"
 cookie = "0.6"
 devtools_traits = {path = "../devtools_traits"}
 flate2 = "0.2.0"
-hyper = "0.10"
+hyper = "0.10.8"


why do we need to specify 0.10.8 instead of just 0.10?

Good practice, the get_ref functions are in hyper 0.10.8.

avadacatavra · 2017-04-11T22:24:32Z

components/net/Cargo.toml

 hyper_serde = "0.6"
-hyper-openssl = "0.2.2"
+hyper-openssl = "0.2.6"


same question--why do we need to specify past 0.2?

Same reason, though I ended up not using the lock function I introduced in 0.2.6, given we don't need to look at the cipher yet.

jdm · 2017-04-11T22:46:11Z

components/net/fetch/methods.rs

@@ -255,7 +271,12 @@ pub fn main_fetch(request: &mut Request,
        // TODO: handle blocking by content security policy.
        let blocked_error_response;
        let internal_response =
-            if should_replace_with_nosniff_error {
+            if should_replace_with_mixed_content_error {


Please remove the TODO comment about mixed content above this line.

jdm · 2017-04-11T22:46:11Z

components/net/fetch/methods.rs

+    is_potentially_trustworthy_url(url.as_url()) || url.scheme() == "data"
+}
+
+/// https://www.w3.org/TR/secure-contexts/#is-url-trustworthy


https://w3c.github.io/webappsec-secure-contexts/#is-url-trustworthy

jdm · 2017-04-11T22:46:11Z

components/net/fetch/methods.rs

+    is_potentially_trustworthy_origin(url.origin())
+}
+
+/// https://www.w3.org/TR/secure-contexts/#is-origin-trustworthy


https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy


        Fix enumeration of steps in HTMLScriptElement::prepare


        Move HttpsState to top of net_traits crate


        Kill WrappedHttpResponse


        Kill StreamedResponse


        Update hyper to 0.10.8 and hyper-openssl to 0.2.6


        Set response's HTTPS state as modern if served over https


        Pass HTTPS state through Fetch with net_traits::request::Client


        Enable mixed-content tests


        Partially implement blocking as mixed content (fixes #14513)

nox · 2017-05-01T11:58:33Z

r? @jdm

jdm · 2017-05-01T15:36:37Z

components/script/dom/xmlhttprequest.rs

            unsafe_request: true,
            // XXXManishearth figure out how to avoid this clone
            body: extracted_or_serialized.as_ref().map(|e| e.0.clone()),
+            // TODO: handle request's client.


This is handled now.

jdm · 2017-05-01T15:36:37Z

components/net/fetch/methods.rs

+
+    // Steps 3-4.
+    // TODO: handle whether response is an opaque filtered response.
+    // https://github.com/servo/servo/issues/16355


This hasn't been addressed yet.

jdm · 2017-05-01T15:36:37Z

components/script/dom/window.rs

+
+    /// https://w3c.github.io/webappsec-mixed-content/#categorize-settings-object
+    pub fn target_browsing_context_has_parent_browsing_context(&self) -> bool {
+        self.is_top_level()


This check is backwards.

jdm · 2017-05-01T15:36:37Z

components/script/dom/window.rs

+            if responsible_document.https_state() != HttpsState::None {
+                return true;
+            }
+            match responsible_document.window().parent_info() {


This is broken for cross-origin iframes. We will need to talk to the constellation to make this work correctly.

bors-servo · 2017-05-01T19:22:06Z

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

avadacatavra · 2017-11-14T11:13:01Z

@nox what's the status on this?

jdm · 2017-11-15T22:36:01Z

Closing due to inactivity; tracking in #19243.

highfive assigned mbrubeck Apr 9, 2017

highfive added the S-awaiting-review label Apr 9, 2017

bors-servo added a commit that referenced this pull request Apr 9, 2017

Auto merge of #16320 - nox:mixed, r=<try>

Loading status checks…

887456f

(Do not merge) Partially implement blocking as mixed content

nox force-pushed the nox:mixed branch from 93a5f07 to b1a49a0 Apr 9, 2017

mbrubeck removed their assignment Apr 9, 2017

nox force-pushed the nox:mixed branch from b1a49a0 to 6d86380 Apr 9, 2017

nox force-pushed the nox:mixed branch from 6d86380 to addd811 Apr 10, 2017

nox assigned avadacatavra Apr 10, 2017

nox force-pushed the nox:mixed branch from addd811 to fc92cbe Apr 11, 2017

nox force-pushed the nox:mixed branch from fc92cbe to 6b58fa0 Apr 11, 2017

avadacatavra reviewed Apr 11, 2017

View changes

avadacatavra removed the S-awaiting-review label Apr 11, 2017

nox force-pushed the nox:mixed branch from 6b58fa0 to 31cb651 Apr 11, 2017

highfive added the S-awaiting-review label Apr 11, 2017

avadacatavra reviewed Apr 11, 2017

View changes

jdm requested changes Apr 11, 2017

View changes

Fix enumeration of steps in HTMLScriptElement::prepare

d8d52c6

nox added 7 commits Apr 8, 2017

Move HttpsState to top of net_traits crate

e1b4be6

Kill WrappedHttpResponse

e1a2b96

Kill StreamedResponse

d0659d8

Update hyper to 0.10.8 and hyper-openssl to 0.2.6

bc2d820

Set response's HTTPS state as modern if served over https

70ea1a7

Pass HTTPS state through Fetch with net_traits::request::Client

2a22574

Enable mixed-content tests

6f4babd

nox force-pushed the nox:mixed branch from 31cb651 to 3c7f3f8 Apr 13, 2017

Partially implement blocking as mixed content (fixes #14513)

Loading status checks…

fe2aee2

nox force-pushed the nox:mixed branch from 3c7f3f8 to fe2aee2 Apr 13, 2017

highfive assigned jdm and unassigned avadacatavra May 1, 2017

jdm requested changes May 1, 2017

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels May 1, 2017

highfive added the S-needs-rebase label May 1, 2017

jdm mentioned this pull request Nov 15, 2017

Implement blocking of mixed content #19243

Closed

jdm closed this Nov 15, 2017

jdm mentioned this pull request Nov 15, 2017

Implement mixed content blocking #14513

Open

servo / servo

(Do not merge) Partially implement blocking as mixed content #16320

(Do not merge) Partially implement blocking as mixed content #16320

nox commented Apr 9, 2017 •

edited by larsbergstrom

highfive commented Apr 9, 2017

highfive commented Apr 9, 2017

nox commented Apr 9, 2017

bors-servo commented Apr 9, 2017

bors-servo commented Apr 9, 2017

nox commented Apr 9, 2017

jdm commented Apr 9, 2017

jdm commented Apr 9, 2017

avadacatavra commented Apr 10, 2017

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented May 1, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented May 1, 2017

avadacatavra commented Nov 14, 2017

jdm commented Nov 15, 2017

servo / servo

Join GitHub today

(Do not merge) Partially implement blocking as mixed content #16320

(Do not merge) Partially implement blocking as mixed content #16320

Conversation

nox commented Apr 9, 2017 • edited by larsbergstrom

highfive commented Apr 9, 2017

highfive commented Apr 9, 2017

nox commented Apr 9, 2017

bors-servo commented Apr 9, 2017

bors-servo commented Apr 9, 2017

nox commented Apr 9, 2017

jdm commented Apr 9, 2017

jdm commented Apr 9, 2017

avadacatavra commented Apr 10, 2017

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.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

nox commented May 1, 2017

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

bors-servo commented May 1, 2017

avadacatavra commented Nov 14, 2017

jdm commented Nov 15, 2017

nox commented Apr 9, 2017 •

edited by larsbergstrom