Implement checks on Location setters #23670

braddunbar · 2019-07-01T02:54:19Z

So, this is my first pull to servo. I've tried to follow all the directions, but I may have missed or forgotten some of them. If so, please let me know - I'll be happy to fix things up!

As described in #23522, this implements various checks on the Location setters. In particular, for the following components.

protocol
host
hostname
port
pathname

I'd also like to add code for throwing a SyntaxError when appropriate, but I may do that separately.

I've removed the expectations for the protocol setter tests, but I haven't yet found any failing tests for the other components. Should I write tests for those? If so, should I add them here or is there an upstream that I should contribute to?

Let me know what you think!

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #23522
There are tests for these changes

This change is


        only allow http/https protocols


        host or hostname: abort if url cannot be a base


        port: abort if url cannot have username/password/port


        pathname: terminate if cannot-be-a-base


        remove failure expectations

highfive · 2019-07-01T02:54:26Z

Thanks for the pull request, and welcome! The Servo team is excited to review your changes, and you should hear from @asajeffrey (or someone else) soon.

highfive · 2019-07-01T02:54:27Z

Heads up! This PR modifies the following files:

@asajeffrey: components/script/dom/location.rs
@KiChjang: components/script/dom/location.rs

gterzian

Looks great, just a few questions...

I'll remove my commit from #23368, since this is better :)

gterzian · 2019-07-01T11:04:32Z

components/script/dom/location.rs

@@ -204,7 +213,11 @@ impl LocationMethods for Location {
    // https://html.spec.whatwg.org/multipage/#dom-location-port
    fn SetPort(&self, value: USVString) -> ErrorResult {
        self.check_same_origin_domain()?;
-        self.set_url_component(value, UrlHelper::SetPort);
+        let url = self.get_url();
+        // If copyURL cannot have a username/password/port, then return.


Here you could add a link to the spec: https://url.spec.whatwg.org/#cannot-have-a-username-password-port

Sure! 05f7547

gterzian · 2019-07-01T11:04:32Z

components/script/dom/location.rs

-        self.set_url_component(value, UrlHelper::SetProtocol);
+        // If copyURL's scheme is not an HTTP(S) scheme, then terminate these steps.
+        let scheme = value.split(':').next().unwrap();
+        if scheme.eq_ignore_ascii_case("http") || scheme.eq_ignore_ascii_case("https") {


The spec contains this note:

Because the URL parser ignores multiple consecutive colons, providing a value of "https:" (or even "https::::") is the same as providing a value of "https".

Does this allow for multiple consecutive colons?

If this is currently not tested in the existing tests, you could also consider adding such a testcase.

Ok, I'll write some tests!

gterzian · 2019-07-01T11:06:20Z

Should I write tests for those?

@bors-servo try=wpt (let's see if we find any other updates)

If so, should I add them here or is there an upstream that I should contribute to?

If you find a relevant test to write, you can do it here and it will be upstreamed automatically.


        Auto merge of #23670 - braddunbar:https-scheme, r=<try>

Implement checks on Location setters So, this is my first pull to servo. I've tried to follow all the directions, but I may have missed or forgotten some of them. If so, please let me know - I'll be happy to fix things up! As described in #23522, this implements various checks on the Location setters. In particular, for the following components. * protocol * host * hostname * port * pathname I'd also like to add code for throwing a `SyntaxError` when appropriate, but I may do that separately. I've removed the expectations for the protocol setter tests, but I haven't yet found any failing tests for the other components. Should I write tests for those? If so, should I add them here or is there an upstream that I should contribute to? Let me know what you think! --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [ ] There are tests for these changes  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/23670)

bors-servo · 2019-07-01T11:06:25Z

⌛ Trying commit 6c31813 with merge 7be2770...

bors-servo · 2019-07-01T11:52:45Z

☀️ Test successful - linux-rel-css, linux-rel-wpt, status-taskcluster
State: approved= try=True


        link to cannot have a username password spec

gterzian · 2019-07-02T07:20:38Z

Ok so since the tests were successful(no unexpected PASS), it means you got all the metadata update covered...

gterzian · 2019-07-09T06:13:42Z

Re those tests for the protocol setter, I think they are covered by the url test suite, so on second-thoughts I don't think it's necessary to add those. Sorry if you have already started working on it...

I'd say if you squash your commits into one, it's ready to go.

@jdm @asajeffrey r?

jdm · 2019-07-12T21:19:23Z

@asajeffrey Are you available to review this?

asajeffrey · 2019-07-17T14:56:54Z

components/script/dom/location.rs

@@ -140,7 +140,10 @@ impl LocationMethods for Location {
    // https://html.spec.whatwg.org/multipage/#dom-location-host
    fn SetHost(&self, value: USVString) -> ErrorResult {
        self.check_same_origin_domain()?;
-        self.set_url_component(value, UrlHelper::SetHost);
+        // If copyURL's cannot-be-a-base-URL flag is set, terminate these steps.


Can you add a comment "Step 4" to this, to make it easy to follow the code wrt the spec? (This applies to the other setters too.) Apart from that, looks great!

asajeffrey · 2019-07-17T14:58:17Z

This looks great, just needs some comments linking to steps of the spec. Might be a good idea to squash all the commits into one?

asajeffrey · 2019-08-19T12:31:31Z

@braddunbar Are you still working on this?

asajeffrey · 2019-09-27T15:09:36Z

@braddunbar Ping?

bors-servo · 2019-10-22T10:08:48Z

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

asajeffrey · 2020-01-06T17:09:21Z

Closing as dormant. Feel free to reopen it!


        Auto merge of #26231 - utsavoza:ugo/issue-23522/16-04-2020, r=gterzian

@UxioAndrade

Implement various checks on the Location setters Since there are no existing open PRs for the issue, I am raising one based on the work done by @UxioAndrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively. - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [x] There are tests for these changes


        Auto merge of #26231 - utsavoza:ugo/issue-23522/16-04-2020, r=gterzian

@UxioAndrade

Implement various checks on the Location setters Since there are no existing open PRs for the issue, I am raising one based on the work done by @UxioAndrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively. - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [x] There are tests for these changes


        Auto merge of #26231 - utsavoza:ugo/issue-23522/16-04-2020, r=gterzian

@UxioAndrade

Implement various checks on the Location setters Since there are no existing open PRs for the issue, I am raising one based on the work done by @UxioAndrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively. - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [x] There are tests for these changes


        Auto merge of #26231 - utsavoza:ugo/issue-23522/16-04-2020, r=gterzian

@UxioAndrade

Implement various checks on the Location setters Since there are no existing open PRs for the issue, I am raising one based on the work done by @UxioAndrade and @braddunbar in previous two PRs for the issue (#25714 and #23670) respectively. - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #23522 - [x] There are tests for these changes

braddunbar added 5 commits Jun 29, 2019

only allow http/https protocols

Verified

This commit was signed with a verified signature.

braddunbar Brad Dunbar

GPG key ID: C32E3CC9B201C719 Learn about signing commits

9845249

host or hostname: abort if url cannot be a base

Verified

This commit was signed with a verified signature.

braddunbar Brad Dunbar

GPG key ID: C32E3CC9B201C719 Learn about signing commits

3c8df69

port: abort if url cannot have username/password/port

Verified

This commit was signed with a verified signature.

braddunbar Brad Dunbar

GPG key ID: C32E3CC9B201C719 Learn about signing commits

97e967c

pathname: terminate if cannot-be-a-base

Verified

This commit was signed with a verified signature.

braddunbar Brad Dunbar

GPG key ID: C32E3CC9B201C719 Learn about signing commits

3f2abcc

remove failure expectations

Verified

This commit was signed with a verified signature.

braddunbar Brad Dunbar

GPG key ID: C32E3CC9B201C719 Learn about signing commits

Loading status checks…

6c31813

highfive assigned asajeffrey Jul 1, 2019

highfive added the S-awaiting-review label Jul 1, 2019

gterzian reviewed Jul 1, 2019

View changes

link to cannot have a username password spec

Verified

This commit was signed with a verified signature.

braddunbar Brad Dunbar

GPG key ID: C32E3CC9B201C719 Learn about signing commits

Loading status checks…

05f7547

gterzian mentioned this pull request Jul 2, 2019

Clean-up navigation #23368

Merged

0 of 5 tasks complete

asajeffrey reviewed Jul 17, 2019

View changes

jdm added S-needs-code-changes S-needs-squash and removed S-awaiting-review labels Jul 24, 2019

highfive added the S-needs-rebase label Oct 22, 2019

asajeffrey closed this Jan 6, 2020

jdm mentioned this pull request Jan 6, 2020

Implement various checks on the Location setters #23522

Closed

UxioAndrade mentioned this pull request Feb 9, 2020

Add comments linking to the steps of the specs in the checks on Location setters #25714

Closed

4 of 4 tasks complete

utsavoza mentioned this pull request Apr 20, 2020

Implement various checks on the Location setters #26231

Merged

4 of 4 tasks complete

servo / servo

Implement checks on Location setters #23670

Implement checks on Location setters #23670

braddunbar commented Jul 1, 2019 •

edited by SimonSapin

highfive commented Jul 1, 2019

highfive commented Jul 1, 2019

gterzian left a comment

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

This comment has been minimized.

gterzian commented Jul 1, 2019

bors-servo commented Jul 1, 2019

bors-servo commented Jul 1, 2019

gterzian commented Jul 2, 2019

gterzian commented Jul 9, 2019

jdm commented Jul 12, 2019

This comment has been minimized.

asajeffrey commented Jul 17, 2019

asajeffrey commented Aug 19, 2019

asajeffrey commented Sep 27, 2019

bors-servo commented Oct 22, 2019

asajeffrey commented Jan 6, 2020

servo / servo

Join GitHub today

Implement checks on Location setters #23670

Implement checks on Location setters #23670

Conversation

braddunbar commented Jul 1, 2019 • edited by SimonSapin

highfive commented Jul 1, 2019

highfive commented Jul 1, 2019

gterzian left a comment

This comment has been minimized.

gterzian Jul 1, 2019

This comment has been minimized.

braddunbar Jul 1, 2019

This comment has been minimized.

gterzian Jul 1, 2019

This comment has been minimized.

braddunbar Jul 1, 2019

gterzian commented Jul 1, 2019

bors-servo commented Jul 1, 2019

bors-servo commented Jul 1, 2019

gterzian commented Jul 2, 2019

gterzian commented Jul 9, 2019

jdm commented Jul 12, 2019

This comment has been minimized.

asajeffrey Jul 17, 2019

asajeffrey commented Jul 17, 2019

asajeffrey commented Aug 19, 2019

asajeffrey commented Sep 27, 2019

bors-servo commented Oct 22, 2019

asajeffrey commented Jan 6, 2020

braddunbar commented Jul 1, 2019 •

edited by SimonSapin