check http_state in determine_request_referrer #26546

splav · 2020-05-16T19:53:56Z

Check https status inside determine_request_referrer.

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #14506 (GitHub issue number if applicable)

There are tests for these changes OR
These changes do not require tests because ___

highfive · 2020-05-16T19:54:01Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/fetch.rs
@KiChjang: components/script/fetch.rs, components/net/fetch/methods.rs, components/net/tests/http_loader.rs, components/net_traits/request.rs, components/net/http_loader.rs

splav · 2020-05-16T19:54:21Z

Sadly, it seems there are no tests for HttpsState::Deprecated value. At least in enabled tests.

CYBAI · 2020-05-17T02:42:47Z

components/net/http_loader.rs

+/// <https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy>
+fn is_origin_trustworthy(url: ServoUrl) -> bool {
+    match url.origin() {
+        // Step 1
+        ImmutableOrigin::Opaque(_) => false,
+        ImmutableOrigin::Tuple(_, _, _) => {
+            // Step 3
+            if url.scheme() == "https" || url.scheme() == "wss" {
+                return true;
+            }
+            // Step 4-5 TODO
+            // Step 6
+            if url.scheme() == "file" {
+                return true;
+            }
+            // Step 7-8 TODO
+            // Step 9
+            false
+        },
+    }
 }


IIRC, we've had this function in urlhelper so you can just use UrlHelper::is_origin_trustworthy.

servo/components/script/dom/urlhelper.rs

Lines 75 to 94 in 0a00ea3

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

pub fn is_origin_trustworthy(url: &ServoUrl) -> bool {

// Step 1

if !url.origin().is_tuple() {

return false;

}

// Step 3

if url.scheme() == "https" || url.scheme() == "wss" {

true

// Step 4

} else if url.host().is_some() {

let host = url.host_str().unwrap();

host == "127.0.0.0/8" || host == "::1/128"

// Step 6

} else {

url.scheme() == "file"

}

}

}

splav · 2020-05-17T06:20:33Z

Thank you! Then I think: 1. It should be moved to somewhere in the net, may be there is something like net/url helpers? What is the best place? 2. The check for local ip is incorrect, I'll fix it using Rust std Ipv*Addr structs methods.

…

--------------------------------------------------------- Александров Сергей Васильевич вс, 17 мая 2020 г., 5:42 cybai <notifications@github.com>:

*@CYBAI* commented on this pull request. ------------------------------ In components/net/http_loader.rs <#26546 (comment)>: > +/// <https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy> +fn is_origin_trustworthy(url: ServoUrl) -> bool { + match url.origin() { + // Step 1 + ImmutableOrigin::Opaque(_) => false, + ImmutableOrigin::Tuple(_, _, _) => { + // Step 3 + if url.scheme() == "https" || url.scheme() == "wss" { + return true; + } + // Step 4-5 TODO + // Step 6 + if url.scheme() == "file" { + return true; + } + // Step 7-8 TODO + // Step 9 + false + }, + } } IIRC, we've had this function in urlhelper so you can just use UrlHelper::is_origin_trustworthy. https://github.com/servo/servo/blob/0a00ea3db3c2353c78435030040fd3f57f5f4497/components/script/dom/urlhelper.rs#L75-L94 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#26546 (review)>, or unsubscribe <https://github.com/notifications/unsubscribe-auth/AABMOIUCCWT535MQPG6W5Q3RR5FLFANCNFSM4NDBUZGQ> .

CYBAI · 2020-05-17T06:29:27Z

It should be moved to somewhere in the net, may be there is something like net/url helpers? What is the best place?

Yes, agree! @gterzian tried to move it as a method of ServoUrl in #26317 ; maybe that will be a good place?

See https://github.com/servo/servo/pull/26317/files#diff-cac78be29c466ffe8160ad29af53e214R173-R191

The check for local ip is incorrect, I'll fix it using Rust std Ipv*Addr structs methods.

👍

splav · 2020-05-17T10:52:50Z

Fixed is_origin_trustworthy + moved it to ServoUrl

jdm

We're missing initializing the request's https state in Document::fetch_async.

We should also make net_request_from_global in component/script/fetch.rs initialize the https_state member; this might require us to:

add a https_state method on GlobalScope
make load_whole_resource return the HTTPS state
update the global scope's https state based on the result of the load

Finally we should add the request's HTTPS state as an argument to load_whole_resource and set the Request's https_state member before starting the request.

jdm · 2020-05-17T15:07:23Z

components/net_traits/request.rs

@@ -413,6 +418,7 @@ impl Request {
            redirect_count: 0,
            response_tainting: ResponseTainting::Basic,
            csp_list: None,
+            https_state: HttpsState::None,


We should make sure that the new https_state member is initialized in net_request_from_global in components/script/dom/request.rs.

jdm · 2020-05-17T15:07:23Z

components/script/fetch.rs

@@ -128,6 +129,7 @@ fn request_init_from_request(request: NetTraitsRequest) -> RequestBuilder {
        parser_metadata: request.parser_metadata,
        initiator: request.initiator,
        csp_list: None,
+        https_state: HttpsState::None,


This should be request.https_state, right?

Yes, missed that NetTraitsRequest means 'Request from net_traits'

splav · 2020-05-18T22:47:06Z

I want to believe that all requested changes are fixed now.

splav · 2020-05-18T23:11:37Z

Forgot unit tests, fixing.

jdm · 2020-05-19T15:11:48Z

components/script/dom/dedicatedworkerglobalscope.rs

@@ -395,6 +395,7 @@ impl DedicatedWorkerGlobalScope {
                    Ok((metadata, bytes)) => (metadata, bytes),
                };
                scope.set_url(metadata.final_url);
+                global_scope.set_https_state(metadata.https_state);


Since load_whole_resource now uses the default value of global's https state when we call it earlier in this file, please set global_scope's https_state to the https_state of current_global before calling load_whole_resource. This call to set_https_state should remain, however.

Done, essentially it makes the worker global scope inherit parent global scope https state.

jdm · 2020-05-19T17:03:59Z

components/script/dom/dedicatedworkerglobalscope.rs

@@ -375,6 +375,8 @@ impl DedicatedWorkerGlobalScope {
                let scope = global.upcast::<WorkerGlobalScope>();
                let global_scope = global.upcast::<GlobalScope>();

+                global_scope.set_https_state(current_global.get_https_state());


I'm 99% confident that this doesn't build, since current_global can't be moved between threads. We need to get the https state value before spawning the new thread instead.

Yes, fixed that. Strangely it did build. May be some caching... Also sometimes rustc crashes and builds fine after rerun.

jdm · 2020-05-19T17:13:08Z

@bors-servo try=wpt

bors-servo · 2020-05-19T17:13:13Z

⌛ Trying commit 357b486 with merge e82b9da...


        Auto merge of #26546 - splav:tls-protected-checks, r=<try>

check http_state in determine_request_referrer  Check https status inside determine_request_referrer. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14506 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-05-19T17:56:04Z

☀️ Test successful - status-taskcluster
State: approved= try=True

jdm · 2020-05-19T18:21:44Z

Huh, I'm surprised there are still no test result changes. I want to figure out what's missing before we merge this.

splav · 2020-05-19T18:25:59Z

Ok. If there are candidates - I can investigate concrete tests. I'll try to look for appropriate tests manually though.

jdm · 2020-05-19T20:05:35Z

I dug into it, and it turns out that there are no referrer-policy tests that start in an HTTPS context and make an HTTP request. This appears to be an oversight in the test suite generation, based on:

https://github.com/web-platform-tests/wpt/blob/b62bd4a22154756bc021bdad08aafe9b57bb17f6/referrer-policy/spec.src.json#L130-L143 - expected results for omitting the referrer when starting in HTTPS and making an HTTP request with no-referrer-when-downgrade
https://github.com/web-platform-tests/wpt/blob/b62bd4a22154756bc021bdad08aafe9b57bb17f6/referrer-policy/spec.src.json#L551- all L570 - HTTPS->HTTP tests are excluded from the set of tests to generate

It looks like either the mixed-content exclusions are over-enthusiastic, or the author thought that the mixed content standard subsumes this for some reason.

I'm inclined to merge these changes and investigate extending the test suite separately.

jdm · 2020-05-19T20:06:28Z

@bors-servo r+

bors-servo · 2020-05-19T20:06:29Z

📌 Commit 357b486 has been approved by jdm

splav · 2020-05-19T20:06:41Z

Random thought:
If the previous request was a modern https the it was handled correctly with checking url scheme.
Modern <=> "https" scheme; None <=> "http" scheme.
The only case not handled correctly previously I can imagine - Deprecated state from previous request.
I failed to find any uses of HttpsState::Deprecated in the code and is up to implementation what to consider deprecated. So, no fixed tests here does not seem strange.

The other thing I fixed - is_origin_trustworthy implementation. And here I do expect some test results changes.

jdm · 2020-05-19T20:15:43Z

@bors-servo r-
Let's answer the question about is_origin_trustworthy before merging.

jdm · 2020-05-19T20:34:02Z

Looks like the is_origin_trustworthy changes are difficult to test as part of the WPT harness, so let's merge these.
@bors-servo r+

bors-servo · 2020-05-19T20:34:03Z

📌 Commit 357b486 has been approved by jdm

bors-servo · 2020-05-19T20:53:27Z

⌛ Testing commit 357b486 with merge 4edf37a...


        Auto merge of #26546 - splav:tls-protected-checks, r=jdm

check http_state in determine_request_referrer  Check https status inside determine_request_referrer. ---  - [X] `./mach build -d` does not report any errors - [X] `./mach test-tidy` does not report any errors - [X] These changes fix #14506 (GitHub issue number if applicable)  - [ ] There are tests for these changes OR - [ ] These changes do not require tests because ___

bors-servo · 2020-05-19T21:28:27Z

💔 Test failed - status-taskcluster

jdm · 2020-05-19T21:32:37Z

@bors-servo retry

#26572

bors-servo · 2020-05-19T22:03:38Z

⌛ Testing commit 357b486 with merge e17b53e...

bors-servo · 2020-05-19T22:22:22Z

☀️ Test successful - status-taskcluster
Approved by: jdm
Pushing e17b53e to master...

highfive assigned Manishearth May 16, 2020

highfive added the S-awaiting-review label May 16, 2020

CYBAI reviewed May 17, 2020

View changes

jdm requested changes May 17, 2020

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels May 17, 2020

jdm assigned jdm and unassigned Manishearth May 17, 2020

splav force-pushed the splav:tls-protected-checks branch from 625c47b to 788ae3f May 18, 2020

highfive added S-awaiting-review and removed S-needs-code-changes labels May 18, 2020

splav force-pushed the splav:tls-protected-checks branch 2 times, most recently from b7d83d2 to 728fd0e May 18, 2020

splav force-pushed the splav:tls-protected-checks branch 2 times, most recently from cc83ffd to 0c42ae7 May 18, 2020

jdm reviewed May 19, 2020

View changes

jdm added S-needs-code-changes and removed S-awaiting-review labels May 19, 2020

splav force-pushed the splav:tls-protected-checks branch from 0c42ae7 to 0bf9a69 May 19, 2020

highfive added S-awaiting-review and removed S-needs-code-changes labels May 19, 2020

jdm reviewed May 19, 2020

View changes

splav force-pushed the splav:tls-protected-checks branch from 0bf9a69 to 8ca5ebb May 19, 2020

splav force-pushed the splav:tls-protected-checks branch from 8ca5ebb to 357b486 May 19, 2020

highfive added S-awaiting-merge and removed S-awaiting-review labels May 19, 2020

highfive added S-tests-failed and removed S-awaiting-merge labels May 19, 2020

highfive added S-awaiting-merge and removed S-tests-failed labels May 19, 2020

bors-servo merged commit e17b53e into servo:master May 19, 2020
2 checks passed

2 checks passed

Community-TC (pull_request) TaskGroup: success
Details

homu Test successful
Details

highfive removed the S-awaiting-merge label May 19, 2020

bors-servo mentioned this pull request May 19, 2020

ServiceWorker: restructure Job Queue, Register flow, to better match spec #26317

Merged

0 of 5 tasks complete

servo / servo

check http_state in determine_request_referrer #26546

check http_state in determine_request_referrer #26546

splav commented May 16, 2020

highfive commented May 16, 2020

splav commented May 16, 2020 •

edited

This comment has been minimized.

splav commented May 17, 2020

CYBAI commented May 17, 2020

splav commented May 17, 2020 •

edited

jdm left a comment •

edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

splav commented May 18, 2020

splav commented May 18, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented May 19, 2020

bors-servo commented May 19, 2020

bors-servo commented May 19, 2020

jdm commented May 19, 2020

splav commented May 19, 2020

jdm commented May 19, 2020

jdm commented May 19, 2020

bors-servo commented May 19, 2020

splav commented May 19, 2020

jdm commented May 19, 2020

jdm commented May 19, 2020

bors-servo commented May 19, 2020

bors-servo commented May 19, 2020

bors-servo commented May 19, 2020

jdm commented May 19, 2020

bors-servo commented May 19, 2020

bors-servo commented May 19, 2020

	// https://w3c.github.io/webappsec-secure-contexts/#is-origin-trustworthy
	pub fn is_origin_trustworthy(url: &ServoUrl) -> bool {
	// Step 1
	if !url.origin().is_tuple() {
	return false;
	}

	// Step 3
	if url.scheme() == "https" \|\| url.scheme() == "wss" {
	true
	// Step 4
	} else if url.host().is_some() {
	let host = url.host_str().unwrap();
	host == "127.0.0.0/8" \|\| host == "::1/128"
	// Step 6
	} else {
	url.scheme() == "file"
	}
	}
	}

servo / servo

Join GitHub today

check http_state in determine_request_referrer #26546

check http_state in determine_request_referrer #26546

Conversation

splav commented May 16, 2020

Check https status inside determine_request_referrer.

highfive commented May 16, 2020

splav commented May 16, 2020 • edited

This comment has been minimized.

splav commented May 17, 2020

CYBAI commented May 17, 2020

splav commented May 17, 2020 • edited

jdm left a comment • edited

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

splav commented May 18, 2020

splav commented May 18, 2020

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

jdm commented May 19, 2020

bors-servo commented May 19, 2020

bors-servo commented May 19, 2020

jdm commented May 19, 2020

splav commented May 19, 2020

jdm commented May 19, 2020

jdm commented May 19, 2020

bors-servo commented May 19, 2020

splav commented May 19, 2020

jdm commented May 19, 2020

jdm commented May 19, 2020

bors-servo commented May 19, 2020

bors-servo commented May 19, 2020

bors-servo commented May 19, 2020

jdm commented May 19, 2020

bors-servo commented May 19, 2020

bors-servo commented May 19, 2020

splav commented May 16, 2020 •

edited

splav commented May 17, 2020 •

edited

jdm left a comment •

edited