#14514 Implement port-based blocking by DominoTree · Pull Request #14623 · servo/servo

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 Implement port-based blocking #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:28:38Z

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:29:10Z

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:30:03Z

components/net/http_loader.rs

                  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:31:40Z

tests/unit/net/fetch.rs

    }
 }

+#[test]


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

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

 #[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.

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

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

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

"Actual commit differs from requested commit" error 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...

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 There is new code that needs to be reviewed. label Dec 17, 2016

DominoTree closed this Dec 18, 2016

DominoTree reopened this Dec 18, 2016

emilio reviewed Dec 19, 2016

View reviewed changes

Ms2ger reviewed Dec 19, 2016

View reviewed changes

jdm reviewed Dec 21, 2016

View reviewed changes

Implement port-based blocking

a56a7ba

highfive assigned emilio and unassigned mbrubeck Dec 23, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-awaiting-review There is new code that needs to be reviewed. labels Dec 23, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Dec 23, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 23, 2016

highfive added S-tests-failed The changes caused existing tests to fail. and removed S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. labels Dec 23, 2016

Ms2ger closed this Dec 23, 2016

Ms2ger reopened this Dec 23, 2016

highfive added S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. and removed S-tests-failed The changes caused existing tests to fail. labels Dec 24, 2016

bors-servo merged commit a56a7ba into servo:master Dec 24, 2016

highfive removed the S-awaiting-merge The PR is in the process of compiling and running tests on the automated CI. label Dec 24, 2016

bors-servo mentioned this pull request Dec 24, 2016

Implement browsing context discarding #14559

Merged

4 tasks

                   }
               }
+              #[test]

Uh oh!

Conversation

DominoTree commented Dec 17, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Dec 17, 2016

Uh oh!

emilio left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio commented Dec 19, 2016

Uh oh!

bors-servo commented Dec 19, 2016

Uh oh!

bors-servo commented Dec 19, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominoTree Dec 19, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

emilio commented Dec 19, 2016

Uh oh!

Ms2ger commented Dec 20, 2016

Uh oh!

DominoTree commented Dec 20, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DominoTree commented Dec 21, 2016

Uh oh!

Ms2ger commented Dec 21, 2016

Uh oh!

DominoTree commented Dec 21, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

emilio commented Dec 23, 2016

Uh oh!

bors-servo commented Dec 23, 2016

Uh oh!

bors-servo commented Dec 23, 2016

Uh oh!

bors-servo commented Dec 23, 2016

Uh oh!

emilio commented Dec 23, 2016

Uh oh!

bors-servo commented Dec 23, 2016

Uh oh!

bors-servo commented Dec 23, 2016

Uh oh!

KiChjang commented Dec 23, 2016

Uh oh!

bors-servo commented Dec 23, 2016

Uh oh!

Ms2ger commented Dec 23, 2016

Uh oh!

Ms2ger commented Dec 23, 2016

Uh oh!

bors-servo commented Dec 24, 2016

Uh oh!

KiChjang commented Dec 24, 2016

Uh oh!

bors-servo commented Dec 24, 2016

Uh oh!

bors-servo commented Dec 24, 2016

Uh oh!

Reviewers

DominoTree commented Dec 17, 2016 •

edited

Loading

DominoTree Dec 19, 2016 •

edited

Loading

DominoTree commented Dec 21, 2016 •

edited

Loading