#14514 Implement port-based blocking #14623

DominoTree · 2016-12-17T23:35:31Z

Add check for bad ports to http_fetch(), return NetworkError::Internal if bad port/schema combination is seen.

Test added

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

There are tests for these changes OR

This change is

highfive · 2016-12-17T23:35:36Z

Heads up! This PR modifies the following files:

@KiChjang: components/net/http_loader.rs

emilio

The rest looks good to me, thanks for doing that!

Probably @jdm wants to take a look. I'll do a try run to see if there are any wpt tests around testing this.

emilio · 2016-12-19T08:32:05Z

components/net/http_loader.rs

+    match request.url().port() {
+        Some(port) => {
+            let is_ftp: bool = request.url().scheme() == "ftp" && (port == 20 || port == 21);
+            let bad_ports: Vec<u16> = vec![1, 7, 9, 11, 13, 15, 17, 19, 20, 21, 22, 23, 25, 37, 42,


This can be a static array, and you can binary-search on it for speed.

emilio · 2016-12-19T08:32:05Z

components/net/http_loader.rs

+    // Bad port blocking - https://fetch.spec.whatwg.org/#block-bad-port
+    match request.url().port() {
+        Some(port) => {
+            let is_ftp: bool = request.url().scheme() == "ftp" && (port == 20 || port == 21);


I think the type annotation (: bool) is not needed, is it?

Just added this because it made things a bit clearer for me, but it's definitely not needed.

emilio · 2016-12-19T08:32:05Z

components/net/http_loader.rs

@@ -537,6 +537,23 @@ pub fn http_fetch(request: Rc<Request>,
                  done_chan: &mut DoneChannel,
                  context: &FetchContext)
                  -> Response {
+    // Bad port blocking - https://fetch.spec.whatwg.org/#block-bad-port
+    match request.url().port() {


Given the None arm is empty, you can do:

if let Some(port) = request.url.port() { // ... }

emilio · 2016-12-19T08:32:05Z

tests/unit/net/fetch.rs

@@ -60,6 +61,18 @@ fn test_fetch_response_is_not_network_error() {
 }

 #[test]
+fn test_fetch_on_bad_port_is_network_error() {


Any chance we can write this down also as a web platform test (if it doesn't exist)?

emilio · 2016-12-19T08:32:21Z

@bors-servo try

bors-servo · 2016-12-19T08:32:26Z

⌛ Trying commit dfdf0e9 with merge 166b4de...


        Auto merge of #14623 - DominoTree:master, r=<try>

Add check for bad ports to http_fetch(), return NetworkError::Internal if bad port/schema combination is seen. Test added ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14514 (github issue number if applicable).  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14623)

bors-servo · 2016-12-19T10:41:15Z

☀️ 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-dev

Ms2ger · 2016-12-19T11:40:10Z

components/net_traits/lib.rs

@@ -525,6 +525,8 @@ pub enum ConstellationMsg {
 /// Network errors that have to be exported out of the loaders
 #[derive(Clone, PartialEq, Eq, Debug, Deserialize, Serialize, HeapSizeOf)]
 pub enum NetworkError {
+    /// Request attempted on bad port - https://fetch.spec.whatwg.org/#block-bad-port
+    BadPort,


I don't think it's particularly useful to introduce a new variant here; Internal will be fine.

I was just using because it felt kinda hacky to compare strings in the unit test to confirm that the correct Internal error was returned from connecting to a bad port (and not simply a failed connection), but we can definitely remove it.

emilio · 2016-12-19T15:11:32Z

Please add a WPT test with this behaviour (since all existing tests passed).

You can find docs in how to do this in the file tests/wpt/README.md.

Thanks again! :)

Ms2ger · 2016-12-20T17:50:19Z

Thanks! I think using promise_rejects and promise_test will be necessary to make the test work.

DominoTree · 2016-12-20T17:53:38Z

@Ms2ger Cool, I will look into that

jdm · 2016-12-21T00:28:39Z

tests/wpt/web-platform-tests/fetch/api/request/request-bad-port.html

+    for (var i = 0; i < BLOCKED_PORTS_LIST.length; i++) {
+        var blockedPort = BLOCKED_PORTS_LIST[i];
+        promise_test(t => {
+            return Promise.all([


No need for Promise.all here; we can simply use return promise_rejects(...).

Thanks, I'm a bit out of my element with this JavaScript stuff. Still trying a couple of things to make this work properly.

DominoTree · 2016-12-21T01:42:36Z

Think that should do it!

Ms2ger · 2016-12-21T12:36:33Z

Test looks great, but I'm a little surprised you put the code in http_loader.rs, rather than in fn main_fetch(), as in the spec: https://fetch.spec.whatwg.org/#concept-main-fetch (step 5).

DominoTree · 2016-12-21T13:50:39Z

@Ms2ger My bad, I overlooked that in the spec. I've moved it into the proper place.


        Implement port-based blocking

emilio · 2016-12-23T11:54:55Z

@bors-servo r+

Thanks! :)

bors-servo · 2016-12-23T11:54:57Z

📌 Commit a56a7ba has been approved by emilio

bors-servo · 2016-12-23T12:26:15Z

⌛ Testing commit a56a7ba with merge 5fa80ae...


        Auto merge of #14623 - DominoTree:master, r=emilio

Add check for bad ports to http_fetch(), return NetworkError::Internal if bad port/schema combination is seen. Test added ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14514 (github issue number if applicable).  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14623)

bors-servo · 2016-12-23T12:26:24Z

💔 Test failed - mac-rel-wpt1

emilio · 2016-12-23T16:25:37Z

@bors-servo retry servo/saltfs#565

bors-servo · 2016-12-23T16:35:18Z

⌛ Testing commit a56a7ba with merge fd01e7a...


        Auto merge of #14623 - DominoTree:master, r=emilio

Add check for bad ports to http_fetch(), return NetworkError::Internal if bad port/schema combination is seen. Test added ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14514 (github issue number if applicable).  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14623)

bors-servo · 2016-12-23T16:35:44Z

💔 Test failed - mac-rel-css

KiChjang · 2016-12-23T18:42:02Z

@bors-servo retry

servo/saltfs#565

bors-servo · 2016-12-23T18:43:45Z

⚡ Previous build results for android, arm64, linux-dev, linux-rel-css, linux-rel-wpt, mac-dev-unit, mac-rel-wpt2, windows-gnu-dev are reusable. Rebuilding only arm32, mac-rel-css, mac-rel-wpt1...

Ms2ger · 2016-12-23T23:26:09Z

@bors-servo force

Ms2ger · 2016-12-23T23:26:43Z

Closing for just a moment

bors-servo · 2016-12-24T09:55:15Z

💥 Test timed out

KiChjang · 2016-12-24T09:56:29Z

@bors-servo retry

bors-servo · 2016-12-24T11:41:15Z

⌛ Testing commit a56a7ba with merge de7d73a...


        Auto merge of #14623 - DominoTree:master, r=emilio

Add check for bad ports to http_fetch(), return NetworkError::Internal if bad port/schema combination is seen. Test added ---  - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #14514 (github issue number if applicable).  - [x] There are tests for these changes OR   --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/14623)

bors-servo · 2016-12-24T12:37:09Z

☀️ 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-gnu-dev, windows-msvc-dev

highfive assigned mbrubeck Dec 17, 2016

highfive added the S-awaiting-review label Dec 17, 2016

DominoTree closed this Dec 18, 2016

DominoTree reopened this Dec 18, 2016

emilio reviewed Dec 19, 2016

View changes

Ms2ger reviewed Dec 19, 2016

View changes

jdm reviewed Dec 21, 2016

View changes

Implement port-based blocking

Loading status checks…

a56a7ba

highfive assigned emilio and unassigned mbrubeck Dec 23, 2016

highfive added S-awaiting-merge and removed S-awaiting-review labels Dec 23, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 23, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 23, 2016

highfive added S-tests-failed and removed S-awaiting-merge labels Dec 23, 2016

Ms2ger closed this Dec 23, 2016

Ms2ger reopened this Dec 23, 2016

highfive added S-awaiting-merge and removed S-tests-failed labels Dec 24, 2016

bors-servo merged commit a56a7ba into servo:master Dec 24, 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 Dec 24, 2016

bors-servo mentioned this pull request Dec 24, 2016

Implement browsing context discarding #14559

Merged

4 of 4 tasks complete

servo / servo

#14514 Implement port-based blocking #14623

#14514 Implement port-based blocking #14623

DominoTree commented Dec 17, 2016 •

edited

highfive commented Dec 17, 2016

emilio left a comment

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.

emilio commented Dec 19, 2016

bors-servo commented Dec 19, 2016

bors-servo commented Dec 19, 2016

This comment has been minimized.

This comment has been minimized.

emilio commented Dec 19, 2016

Ms2ger commented Dec 20, 2016

DominoTree commented Dec 20, 2016

This comment has been minimized.

This comment has been minimized.

DominoTree commented Dec 21, 2016

Ms2ger commented Dec 21, 2016

DominoTree commented Dec 21, 2016 •

edited

emilio commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

emilio commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

KiChjang commented Dec 23, 2016

bors-servo commented Dec 23, 2016

Ms2ger commented Dec 23, 2016

Ms2ger commented Dec 23, 2016

bors-servo commented Dec 24, 2016

KiChjang commented Dec 24, 2016

bors-servo commented Dec 24, 2016

bors-servo commented Dec 24, 2016

servo / servo

Join GitHub today

#14514 Implement port-based blocking #14623

#14514 Implement port-based blocking #14623

Conversation

DominoTree commented Dec 17, 2016 • edited

highfive commented Dec 17, 2016

emilio left a comment

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.

emilio commented Dec 19, 2016

bors-servo commented Dec 19, 2016

bors-servo commented Dec 19, 2016

This comment has been minimized.

This comment has been minimized.

DominoTree Dec 19, 2016 • edited

emilio commented Dec 19, 2016

Ms2ger commented Dec 20, 2016

DominoTree commented Dec 20, 2016

This comment has been minimized.

This comment has been minimized.

DominoTree commented Dec 21, 2016

Ms2ger commented Dec 21, 2016

DominoTree commented Dec 21, 2016 • edited

emilio commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

emilio commented Dec 23, 2016

bors-servo commented Dec 23, 2016

bors-servo commented Dec 23, 2016

KiChjang commented Dec 23, 2016

bors-servo commented Dec 23, 2016

Ms2ger commented Dec 23, 2016

Ms2ger commented Dec 23, 2016

bors-servo commented Dec 24, 2016

KiChjang commented Dec 24, 2016

bors-servo commented Dec 24, 2016

bors-servo commented Dec 24, 2016

DominoTree commented Dec 17, 2016 •

edited

DominoTree Dec 19, 2016 •

edited

DominoTree commented Dec 21, 2016 •

edited