Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Setting the scheme to "file" should fail if there's a host or port #278

Closed
asajeffrey opened this issue Feb 15, 2017 · 11 comments
Closed

Setting the scheme to "file" should fail if there's a host or port #278

asajeffrey opened this issue Feb 15, 2017 · 11 comments

Comments

@asajeffrey
Copy link
Member

The test case:

    let mut url: Url = "http://localhost:6767/foo/bar".parse().unwrap();
    let result = url.set_scheme("file");
    assert_eq!(url.to_string(), "http://localhost:6767/foo/bar");
    assert_eq!(result, Err(()));

should pass, instead it fails with:

   (left == right)` (left: `"file://localhost:6767/foo/bar"`, right: `"http://localhost:6767/foo/bar"`)

The resulting URL has a file scheme, and a host/port, so serialization fails.

@asajeffrey
Copy link
Member Author

This is the root cause of servo/servo#15435

bors-servo pushed a commit to servo/servo that referenced this issue Feb 20, 2017
…ishearth

Prevent the constellation from panicking if there is a deserialization error

<!-- Please describe your changes on the following line: -->

At the moment, the constellation panics if there is a deserialization error on an ipc channel. This happens in `/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html` due to the URL deserialization errors caused by servo/rust-url#278.

This PR stops the constellation from panicking, so we can get a crash report for deserialization errors.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because we don't test panic recovery

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/servo/15618)
<!-- Reviewable:end -->
moz-v2v-gh pushed a commit to mozilla/gecko-dev that referenced this issue Feb 21, 2017
…re is a deserialization error (from asajeffrey:constellation-ipc-dont-panic); r=Manishearth

<!-- Please describe your changes on the following line: -->

At the moment, the constellation panics if there is a deserialization error on an ipc channel. This happens in `/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html` due to the URL deserialization errors caused by servo/rust-url#278.

This PR stops the constellation from panicking, so we can get a crash report for deserialization errors.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because we don't test panic recovery

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 524a117ff6a5c3bda23e5bc702ef3f67f8df3fd1

--HG--
extra : subtree_source : https%3A//hg.mozilla.org/projects/converted-servo-linear
extra : subtree_revision : 7f6bb6ccd9b821b2db5571ce87712c91ea326918
JerryShih pushed a commit to JerryShih/gecko-dev that referenced this issue Feb 27, 2017
…re is a deserialization error (from asajeffrey:constellation-ipc-dont-panic); r=Manishearth

<!-- Please describe your changes on the following line: -->

At the moment, the constellation panics if there is a deserialization error on an ipc channel. This happens in `/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html` due to the URL deserialization errors caused by servo/rust-url#278.

This PR stops the constellation from panicking, so we can get a crash report for deserialization errors.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because we don't test panic recovery

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 524a117ff6a5c3bda23e5bc702ef3f67f8df3fd1
Manishearth pushed a commit to Manishearth/gecko-dev that referenced this issue Mar 9, 2017
…re is a deserialization error (from asajeffrey:constellation-ipc-dont-panic); r=Manishearth

<!-- Please describe your changes on the following line: -->

At the moment, the constellation panics if there is a deserialization error on an ipc channel. This happens in `/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html` due to the URL deserialization errors caused by servo/rust-url#278.

This PR stops the constellation from panicking, so we can get a crash report for deserialization errors.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because we don't test panic recovery

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 524a117ff6a5c3bda23e5bc702ef3f67f8df3fd1
tmccombs added a commit to tmccombs/rust-url that referenced this issue Jun 18, 2017
tmccombs added a commit to tmccombs/rust-url that referenced this issue Jun 18, 2017
tmccombs added a commit to tmccombs/rust-url that referenced this issue Jun 18, 2017
@SimonSapin
Copy link
Member

Should it really fail? This doesn’t match the spec at https://url.spec.whatwg.org/#dom-url-protocol

In this test case:

<a id=a href="http://example.net:8080/foo"></a>
<script>
var a = document.getElementById("a");
a.protocol = "file";
document.write(a.href)
</script>

Firefox prints file:///foo, Chromium prints file://example.net:8080/foo. I think we probably want to do like Firefox and remove the port implicitly.

@annevk Do you think the spec should be changed to remove the port in this case and avoid leaving the URL in an inconsistent state?

@annevk
Copy link

annevk commented Jun 22, 2017

If you look at scheme state 2.1.3 it seems pretty clear there is a special case. It's not quite failure, but it shouldn't succeed.

@tmccombs
Copy link
Contributor

tmccombs commented Jun 24, 2017

https://url.spec.whatwg.org/#scheme-state

If url includes credentials or has a non-null port, and buffer is "file", then return.

If I understand this correctly, it means that setting the scheme to file for a URL that has credentials or a port specified should fail.

@annevk
Copy link

annevk commented Jun 26, 2017

It's a no-op from the perspective of JavaScript, it doesn't throw. That's what I meant with not quite failure.

@tmccombs
Copy link
Contributor

You're right @annevk.

@sanmai-NL
Copy link

@SimonSapin: Similarly, this library behaves counterintuitively when parsing a data URL.
See https://play.rust-lang.org/?gist=032d9d4e7724b68d74efec2daf07df35&version=stable
My reading of this discussion is that @tmccombs's #366 isn't an unreasonable approach in the end.

In the JavaScript console in the latest Firefox:

>> var parser = document.createElement('a');
>> parser.href = "data://localhost:80";
>> console.log(parser.protocol);
 http:
 undefined 

>> console.log(parser.host); 
 undefined

>> console.log(parser.hostname);
 undefined

>> console.log(parser.port); 
 undefined

Some weirdness with the protocol component/scheme, but no hostname or port number extracted from a data URL for sure.

@jdm
Copy link
Member

jdm commented Nov 6, 2017

@sanmai-NL Data URLs do not include the //.

@sanmai-NL
Copy link

@jdm: I know, a valid example is the counterexample in the Rust playground test case.

@annevk
Copy link

annevk commented Nov 7, 2017

For a data URL that includes // Firefox is incorrect there as the URL parser doesn't special case data URLs.

gecko-dev-updater pushed a commit to marco-c/gecko-dev-comments-removed that referenced this issue Oct 1, 2019
…re is a deserialization error (from asajeffrey:constellation-ipc-dont-panic); r=Manishearth

<!-- Please describe your changes on the following line: -->

At the moment, the constellation panics if there is a deserialization error on an ipc channel. This happens in `/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html` due to the URL deserialization errors caused by servo/rust-url#278.

This PR stops the constellation from panicking, so we can get a crash report for deserialization errors.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because we don't test panic recovery

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 524a117ff6a5c3bda23e5bc702ef3f67f8df3fd1

UltraBlame original commit: e838276dea72f9d0bf23a8cb9cce051b3285bad7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified that referenced this issue Oct 1, 2019
…re is a deserialization error (from asajeffrey:constellation-ipc-dont-panic); r=Manishearth

<!-- Please describe your changes on the following line: -->

At the moment, the constellation panics if there is a deserialization error on an ipc channel. This happens in `/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html` due to the URL deserialization errors caused by servo/rust-url#278.

This PR stops the constellation from panicking, so we can get a crash report for deserialization errors.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because we don't test panic recovery

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 524a117ff6a5c3bda23e5bc702ef3f67f8df3fd1

UltraBlame original commit: e838276dea72f9d0bf23a8cb9cce051b3285bad7
gecko-dev-updater pushed a commit to marco-c/gecko-dev-wordified-and-comments-removed that referenced this issue Oct 1, 2019
…re is a deserialization error (from asajeffrey:constellation-ipc-dont-panic); r=Manishearth

<!-- Please describe your changes on the following line: -->

At the moment, the constellation panics if there is a deserialization error on an ipc channel. This happens in `/html/browsers/history/the-location-interface/location-protocol-setter-non-broken.html` due to the URL deserialization errors caused by servo/rust-url#278.

This PR stops the constellation from panicking, so we can get a crash report for deserialization errors.

---
<!-- Thank you for contributing to Servo! Please replace each `[ ]` by `[X]` when the step is complete, and replace `__` with appropriate data: -->
- [X] `./mach build -d` does not report any errors
- [X] `./mach test-tidy` does not report any errors
- [X] These changes do not require tests because we don't test panic recovery

<!-- Pull requests that do not address these steps are welcome, but they will require additional verification as part of the review process. -->

Source-Repo: https://github.com/servo/servo
Source-Revision: 524a117ff6a5c3bda23e5bc702ef3f67f8df3fd1

UltraBlame original commit: e838276dea72f9d0bf23a8cb9cce051b3285bad7
@djc
Copy link
Contributor

djc commented Aug 25, 2020

This passes the proposed test, so I'm guessing it can be closed. I added the test in 7b8b841 just to make sure.

@djc djc closed this as completed Aug 25, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants