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

Properly set origin of fetch requests #16508

Merged
merged 1 commit into from Jul 17, 2017
Merged

Conversation

@fnune
Copy link
Contributor

@fnune fnune commented Apr 18, 2017

These changes aim to fix #15247


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes fix #15247 (github issue number if applicable).
  • There are tests for these changes
  • These changes do not require tests because cors is already tested with different origins

These changes require changes in tests, but I need help with that (see comments below).


This change is Reviewable

@highfive
Copy link

@highfive highfive commented Apr 18, 2017

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

@highfive
Copy link

@highfive highfive commented Apr 18, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/globalscope.rs, components/script/fetch.rs
  • @KiChjang: components/script/dom/globalscope.rs, components/script/fetch.rs
@highfive
Copy link

@highfive highfive commented Apr 18, 2017

warning Warning warning

  • These commits modify script code, but no tests are modified. Please consider adding a test!
@metajack
Copy link
Contributor

@metajack metajack commented Apr 18, 2017

@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented Apr 18, 2017

📌 Commit af91972 has been approved by metajack

@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented Apr 18, 2017

Testing commit af91972 with merge 43e4d17...

bors-servo added a commit that referenced this pull request Apr 18, 2017
Fetch set origin

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

These changes are a WIP, aiming to fix #15247

---
<!-- 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
- [ ] These changes fix #15247 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes OR
- [x] These changes do not require tests because cors is already tested with different origins

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/16508)
<!-- Reviewable:end -->
@fnune fnune changed the title Fetch set origin Properly set origin of fetch requests Apr 18, 2017
@fnune
Copy link
Contributor Author

@fnune fnune commented Apr 18, 2017

@metajack Hey! Thanks for the review but I think the PR needs some more work, I updated the description

@metajack
Copy link
Contributor

@metajack metajack commented Apr 18, 2017

@metajack
Copy link
Contributor

@metajack metajack commented Apr 18, 2017

I'm in Tokyo at the CSS WG meeting, so probably one of @jdm or @asajeffrey or @Manishearth should provide the feedback you are asking for.

@fnune
Copy link
Contributor Author

@fnune fnune commented Apr 18, 2017

@metajack That's fine, thanks! Have a nice one.

@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented Apr 18, 2017

💔 Test failed - mac-rel-wpt1

@metajack
Copy link
Contributor

@metajack metajack commented Apr 18, 2017

@brainlessdeveloper You'll also want to update the test results since presumably this made something pass now :)

@KiChjang KiChjang changed the title Properly set origin of fetch requests [WIP] Properly set origin of fetch requests Apr 18, 2017
@@ -263,6 +263,11 @@ impl GlobalScope {
unreachable!();
}

/// Get the origin URL for the current global object, shorthand for GlobalScope::current().get_url()
pub fn origin() -> ServoUrl {

This comment has been minimized.

@KiChjang

KiChjang Apr 18, 2017
Member

This function is a misnomer then -- it's not really getting the origin of an environment settings object, since it returns a ServoUrl. In view of this, I think you should disregard my earlier suggestion and remove this function altogether.

This comment has been minimized.

@fnune

fnune Apr 18, 2017
Author Contributor

You're right. Won't save that much typing anyway either. Can you help me with my feedback requests in the PR description?

@fnune
Copy link
Contributor Author

@fnune fnune commented Apr 19, 2017

After carefully reading the spec, I have two concerns to present to you. Maybe I am just extremely confused, but it seems to me that we can't just pass a ServoUrl to RequestInit.

I drew this little flow chart, I hope it doesn't confuse you:
servo_fetch

When setting origin here, we instantiate a RequestInit from a Request. About this step, the spec says:

A request has an associated origin, which is "client" or an origin. Unless stated otherwise it is "client". "client" is changed to an origin during fetching. It provides a convenient way for standards to not have to set request’s origin. Request’s origin can be changed during redirects too.

Changed to an origin during fetching means we need to pass the enum ImmutableOrigin to RequestInit. As per a part of the spec, an origin can be either an opaque origin, or a tuple of a schema, a host, and a port (and a domain that's always null). This is reflected neatly by the ImmutableOrigin tuple. As far as I've understood, the origin will be an opaque origin if it's generated from a file path. The spec says that an opaque origin will always be a string containing null.

The ImmutableOrigin struct contains a couple of handy methods that deal with the logic to figure out if the origin is an opaque origin or if it's a tuple. Another method, the unicode_serialization method, returns a string from an ImmutableOrigin. This string will be either "null" if it's an opaque origin or contain the schema, host, and port if it's a tuple.

In my doodle, there's Q1 (in blue), representing my first concern: I can instantiate a Url struct from a tuple origin, and then create a ServoUrl from it, and pass it to RequestInit's origin field. But I can't do this with an opaque origin, because neither Url or ServoUrl will take a string "null" to be instantiated.

About my second concern (Q2 in blue in the doodle): the spec says a request's "client" is changed to an origin during fetching. But there's still the possibility that the Request from which we're trying to instantiate a RequestInit contains an Origin of type Client.

I think RequestInit should not take an origin of type ServoUrl, I think it should take... an origin of type Origin! And it should deal with transforming it to a string afterwards. The problem is, if you grep for RequestInit { in the project, it appears 105 times, and I'm quite sure all of them serve an origin in a different way, and none of them account for the fact that the origin should be "null" in some cases. So if I'm right, there's quite a bit of refactoring work.

Please, tell me if I've missed something. To me it seems just passing GlobalScope::current().get_url() blindly does not comply with the spec, and will lead to a lot of tears in the future.

@jdm
Copy link
Member

@jdm jdm commented Apr 20, 2017

You are correct, and that's what the TODO in request_init_from_request is referencing. Going back to my suggestion of adding an origin method to GlobalScope, we can have the Window implementation return the associated document's immutable origin, and for workers return an origin based on get_url().origin(). I suspect the 105 uses you see are overcounting; here's the list of the important ones:

components/gfx/font_cache_thread.rs
215:                let request = RequestInit {

components/net_traits/request.rs
164:    fn default() -> RequestInit {

components/script/dom/dedicatedworkerglobalscope.rs
176:            let request = RequestInit {

components/script/dom/eventsource.rs
333:    pub fn request(&self) -> RequestInit {
358:        let mut request = RequestInit {

components/script/dom/htmlimageelement.rs
261:        let request = RequestInit {

components/script/dom/htmlmediaelement.rs
542:            let request = RequestInit {

components/script/dom/htmlscriptelement.rs
240:    let request = RequestInit {

components/script/dom/serviceworkerglobalscope.rs
161:            let request = RequestInit {

components/script/dom/workerglobalscope.rs
206:            let request = NetRequestInit {

components/script/dom/xmlhttprequest.rs
582:        let mut request = RequestInit {

components/script/fetch.rs
44:    NetTraitsRequestInit {

components/script/layout_image.rs
71:    let request = FetchRequestInit {

components/script/script_thread.rs
2125:        let request = RequestInit {

components/script/stylesheet_loader.rs
240:        let request = RequestInit {

Then there are some in tests/unit/net/http_loader.rs as well, but those shouldn't require as much care.

@fnune
Copy link
Contributor Author

@fnune fnune commented Apr 20, 2017

So in summary I should change RequestInit to take only an Origin in the origin field, and then, in the uses that already there, instantiate an Origin from the URL each use case has implemented. The problem is the following: in some cases, that request may have been created from the browser console, in which case its origin field should be the string "null". If I instantiate an Origin from the URLs that are currently there, the case where the origin has to be "null" will be omitted.

Edit: I was wrong about that, read the following comments.

@jdm
Copy link
Member

@jdm jdm commented Apr 20, 2017

I'm not sure I understand the connection to the browser console here. If such a request occurred, I would expect it to use the origin of the page associated with the console, not null.

@fnune
Copy link
Contributor Author

@fnune fnune commented Apr 20, 2017

I'm sorry, I was wrong about the browser console, and you're right: if I open my console right now and go fetch('http://somewebsite.com'), the request's origin will be set to https://github.com.

What I was trying to explain was the possibility of the request origin being "null". It can happen in (AFAIK) two situations:

  • The request was not done from a hosted website but from a file in the user's computer.
  • If a cross-origin resource redirects to another resource at a new origin, (the value of origin should be set to "null" after redirecting).

Currently, all the uses of RequestInit neglect this possibility.

@jdm
Copy link
Member

@jdm jdm commented Apr 20, 2017

The redirection happens as part of the fetch implementation, so the value for RequestInit is not affected by that. file:// URLs are the easiest example of null origins, but it could also happen from blob:// URLs too I believe. If we encapsulate the global origin within the origin() method, we can fix those for free later on.

@fnune
Copy link
Contributor Author

@fnune fnune commented Apr 21, 2017

Seems good, I'll be working on it tonight. Thanks for your help!

@fnune
Copy link
Contributor Author

@fnune fnune commented Apr 21, 2017

Should I use the origin method from the document (this line), or should I implement a new origin method for GlobalScope ?

I suppose in the case of implementing a new method for GlobalScope, I would actually return the document's origin, something like this: window.Document().origin

...which makes it sort of futile to implement it, because I could just get the document and use its origin. What do you think?

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jul 16, 2017

Good point, it looks like what's happening is that the same-origin check is failing because the request origin is an opaque origin, rather than the origin that originated the request. Tracking this down, it looks like the culprit is https://github.com/brainlessdeveloper/servo/blob/5dc8fc99a89478a4827677fc256664b689a024fa/components/script/script_thread.rs#L2289:

    fn pre_page_load(&self, incomplete: InProgressLoad, load_data: LoadData) {
        let id = incomplete.pipeline_id.clone();
        let mut req_init = RequestInit {
            url: load_data.url.clone(),
            method: load_data.method,
            destination: Destination::Document,
            credentials_mode: CredentialsMode::Include,
            use_url_credentials: true,
            pipeline_id: Some(id),
            referrer_url: load_data.referrer_url,
            referrer_policy: load_data.referrer_policy,
            headers: load_data.headers,
            body: load_data.data,
            redirect_mode: RedirectMode::Manual,
            // TODO: Add a proper origin to pre_page_load
            // https://github.com/servo/servo/issues/17238
            .. RequestInit::default()
        };
    ...

Adding:

            origin: incomplete.origin.immutable().clone(),

Seems to do the trick.

@brainlessdeveloper: do you want to make this change and we'll see if this fixes everything?

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jul 16, 2017

I ran the wpt test suite locally. Results at https://gist.github.com/asajeffrey/fec16fc305de171b758eafe42c111333.

Eyeballing it, that looks like quite a few PASS [expected FAIL], the usual intermittents, and the issue discussed above in #16508 (comment).

So this is all looking good with that one-line change. (Usual story: days of debugging to find one line to change, sigh.)

@fnune
Copy link
Contributor Author

@fnune fnune commented Jul 16, 2017

@asajeffrey on it right now! :)

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jul 16, 2017

Thanks!

@fnune
Copy link
Contributor Author

@fnune fnune commented Jul 16, 2017

So, all the PASS, expected FAIL tests I see are in the quirks mode module (presumably nothing to do with this) and then the intermittent tests we dug up some days ago. The only new thing I see is this:

  ▶ Unexpected subtest result in /cors/allow-headers.htm:
  └ PASS [expected FAIL] Allow origin: _http://web-platform.test:8000___[tab]_

I guess I should change the expectation for this one?

@fnune fnune force-pushed the fnune:fetch-set-origin branch from 5dc8fc9 to 6032940 Jul 16, 2017
@fnune
Copy link
Contributor Author

@fnune fnune commented Jul 16, 2017

Ok, the cors tests pass with no unexpected results, as well as referrer-policy and fetch/api (in this last one, excluding the intermittent ones ofc). So I think we should be good to go, hopefully.

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jul 16, 2017

@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented Jul 16, 2017

Trying commit 6032940 with merge 006e36e...

bors-servo added a commit that referenced this pull request Jul 16, 2017
Properly set origin of fetch requests

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

These changes aim to fix #15247

---
<!-- 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 fix #15247 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes
- [x] These changes do not require tests because cors is already tested with different origins

These changes require changes in tests, but I need help with that (see comments below).

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/16508)
<!-- Reviewable:end -->
@fnune
Copy link
Contributor Author

@fnune fnune commented Jul 16, 2017

Finally! :D

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jul 16, 2017

I'll have a last look at it when I'm in front of a computer, I'm not expecting any issues, we should be good to merge! Thanks for your patience.

@fnune
Copy link
Contributor Author

@fnune fnune commented Jul 16, 2017

Thanks for your patience too, especially considering the last failures were caused by things I typed, and I didn't manage to debug it 🙈

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jul 17, 2017

OK, we are good to go! @bors-servo r+

@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented Jul 17, 2017

📌 Commit 6032940 has been approved by asajeffrey

@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented Jul 17, 2017

Testing commit 6032940 with merge 2bb4f65...

bors-servo added a commit that referenced this pull request Jul 17, 2017
Properly set origin of fetch requests

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

These changes aim to fix #15247

---
<!-- 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 fix #15247 (github issue number if applicable).

<!-- Either: -->
- [ ] There are tests for these changes
- [x] These changes do not require tests because cors is already tested with different origins

These changes require changes in tests, but I need help with that (see comments below).

<!-- Also, please make sure that "Allow edits from maintainers" checkbox is checked, so that we can help you if you get stuck somewhere along the way.-->

<!-- 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/16508)
<!-- Reviewable:end -->
@bors-servo
Copy link
Contributor

@bors-servo bors-servo commented Jul 17, 2017

@bors-servo bors-servo merged commit 6032940 into servo:master Jul 17, 2017
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
@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jul 17, 2017

Woo hoo! Just shy of the three month anniversary. Thanks @brainlessdeveloper!

@fnune
Copy link
Contributor Author

@fnune fnune commented Jul 17, 2017

Thank you for your help! :)

@fnune fnune deleted the fnune:fetch-set-origin branch Jul 17, 2017
@KiChjang
Copy link
Member

@KiChjang KiChjang commented Jul 17, 2017

I feel a bit guilty about the last one here because I've mentioned it in the linked issue here but I did not speak up about the probability of the root cause being that. Sorry.

@asajeffrey
Copy link
Member

@asajeffrey asajeffrey commented Jul 17, 2017

@KiChjang np, it was a good exercise in figuring out how our fetch code works!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

7 participants
You can’t perform that action at this time.