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

Fetch is not setting origin correctly. #15247

Closed
asajeffrey opened this issue Jan 26, 2017 · 9 comments · Fixed by #16508
Closed

Fetch is not setting origin correctly. #15247

asajeffrey opened this issue Jan 26, 2017 · 9 comments · Fixed by #16508
Assignees
Labels
A-content/script Related to the script thread A-network A-security C-assigned There is someone working on resolving the issue

Comments

@asajeffrey
Copy link
Member

In https://github.com/servo/servo/blob/master/components/script/fetch.rs#L60 we have:

fn request_init_from_request(request: NetTraitsRequest) -> NetTraitsRequestInit {
    NetTraitsRequestInit {
        ...
        // TODO: NetTraitsRequestInit and NetTraitsRequest have different "origin"
        // ... NetTraitsRequestInit.origin: Url
        // ... NetTraitsRequest.origin: RefCell<Origin>
        origin: request.url(),
        ...
    }
}

which sets the origin of the request to be the origin of the request url (that is, the resource being fetched) rather than the origin of the requester. This means that when we come to do the same-origin test https://github.com/servo/servo/blob/master/components/net/fetch/methods.rs#L211:

/// [Main fetch](https://fetch.spec.whatwg.org/#concept-main-fetch)
pub fn main_fetch(request: Rc<Request>,
                  cache: &mut CorsCache,
                  cors_flag: bool,
                  recursive_flag: bool,
                  target: Target,
                  done_chan: &mut DoneChannel,
                  context: &FetchContext)
                  -> Response {
    ...
    // Step 11
    let response = match response {
        Some(response) => response,
        None => {
            let current_url = request.current_url();
            let same_origin = if let Origin::Origin(ref origin) = *request.origin.borrow() {
                *origin == current_url.origin()
            } else {
                false
            };
    ...

all fetches are considered to be same-origin. This is not a good security feature!

IRC chat with @Manishearth and @KiChjang: http://logs.glob.uno/?c=mozilla%23servo&s=26+Jan+2017&e=26+Jan+2017#c600706

@asajeffrey asajeffrey added A-content/script Related to the script thread A-network A-security labels Jan 26, 2017
@asajeffrey asajeffrey self-assigned this Jan 26, 2017
@asajeffrey
Copy link
Member Author

This is probably the root cause of the test failure at #15232 (comment)

@fnune
Copy link
Contributor

fnune commented Apr 13, 2017

I would like to work on this.

@asajeffrey
Copy link
Member Author

@brainlessdeveloper cool, thanks! (I was on vacation, sorry about the delay.) Let me know if there's any help you need.

@fnune
Copy link
Contributor

fnune commented Apr 17, 2017

Oh, I'd forgotten about this one haha. Would you mind adding the assigned label to it?

@cbrewster cbrewster added the C-assigned There is someone working on resolving the issue label Apr 17, 2017
@asajeffrey
Copy link
Member Author

@cbrewster beat me to it :)

@KiChjang
Copy link
Contributor

@brainlessdeveloper The following conversation snippet would light your path forward:

19:36	jdm	KiChjang: "Set request's client to the element's node document's Window object's environment settings object and type to "image"."
19:36	jdm	(for example)
19:36	jdm	the environment settings object is derived from the request's client
19:37	jdm	although new Request(...) uses the current global
19:37	jdm	(ie. GlobalScope::current())
19:37	KiChjang	"element's node document's window object's"
19:38	KiChjang	too many apostrophes
19:42	KiChjang	okay, looks like the request origin should come from GlobalScope::curent().get_url()
19:43	KiChjang	though ideally we should also have a origin() method on GlobalScope

@fnune
Copy link
Contributor

fnune commented Apr 17, 2017

@KiChjang Awesome! I read the whole chat but on a first read I didn't figure out much. Thanks!

@fnune
Copy link
Contributor

fnune commented Apr 17, 2017

So in short I should fix origin in fn request_init_from_request here and also figure out if we need an origin method on GlobalScope. If so, implement it, and it should return the origin url. Did I get it all right?

@KiChjang
Copy link
Contributor

The origin method on GlobalScope would just be a shorthand for GlobalScope::current().get_url(), so that method would just be there to save some typing, but it's not strictly necessary. You may also want to grep places where we instantiate a new RequestInit struct and read the relevant section of the spec to see whether it is initializing the origin correctly.

bors-servo pushed a commit that referenced this issue 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 -->
bors-servo pushed a commit that referenced this issue Jun 8, 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 pushed a commit that referenced this issue Jul 5, 2017
…y,jdm

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 pushed a commit that referenced this issue Jul 11, 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 pushed a commit that referenced this issue Jul 11, 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 pushed a commit that referenced this issue 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 -->
bors-servo pushed a commit that referenced this issue 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 -->
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-content/script Related to the script thread A-network A-security C-assigned There is someone working on resolving the issue
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants