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

Added some same-origin-domain checks #15478

Closed
wants to merge 5 commits into from

Conversation

@asajeffrey
Copy link
Member

asajeffrey commented Feb 9, 2017

This PR adds some same-origin-domain checks, replacing some same-origin checks. This will make a difference once we support setting document.domain.

This PR builds on #15438, only the last commit is part of this PR.


  • ./mach build -d does not report any errors
  • ./mach test-tidy does not report any errors
  • These changes do not require tests because we don't support setting document.domain yet

This change is Reviewable

@highfive
Copy link

highfive commented Feb 9, 2017

Heads up! This PR modifies the following files:

  • @fitzgen: components/script/dom/servoparser/mod.rs, components/script/dom/window.rs, components/script/dom/location.rs, components/script/dom/document.rs, components/script/dom/xmlhttprequest.rs, components/script/devtools.rs, components/script/dom/history.rs, components/script/webdriver_handlers.rs, components/script/lib.rs, components/script/dom/domparser.rs, components/script/script_thread.rs, components/script/dom/domimplementation.rs, components/script/dom/htmliframeelement.rs, components/script/dom/webidls/Location.webidl, components/script/dom/node.rs, components/script/dom/xmldocument.rs, components/script/dom/bindings/trace.rs, components/script/origin.rs
  • @KiChjang: components/script/dom/servoparser/mod.rs, components/script/dom/window.rs, components/script/dom/location.rs, components/script/dom/document.rs, components/net/http_loader.rs, components/script/dom/xmlhttprequest.rs, components/script/devtools.rs, components/script/dom/history.rs, components/script/webdriver_handlers.rs, components/script/lib.rs, components/script/dom/domparser.rs, components/script/script_thread.rs, components/script/dom/domimplementation.rs, components/script/dom/htmliframeelement.rs, components/script/dom/webidls/Location.webidl, components/script/dom/node.rs, components/script/dom/xmldocument.rs, components/script/dom/bindings/trace.rs, components/script/origin.rs, components/net_traits/request.rs, components/net_traits/request.rs
@highfive
Copy link

highfive commented Feb 9, 2017

warning Warning warning

  • These commits modify unsafe code. Please review it carefully!
@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 9, 2017

@asajeffrey asajeffrey changed the title Script same origin domain Added some same-origin-domain checks Feb 9, 2017
Copy link
Contributor

Ms2ger left a comment

I'm not convinced it's a good idea to land this until we actually have document.domain, and I can tell you to write tests for each and every change here.

fn GetContentDocument(&self) -> Option<Root<Document>> {
self.get_content_window().map(|window| window.Document())
// Step 1.
if let Some(pipeline_id) = self.pipeline_id.get() {

This comment has been minimized.

Copy link
@Ms2ger

Ms2ger Feb 10, 2017

Contributor

Early returns would be better here.

This comment has been minimized.

Copy link
@asajeffrey

asajeffrey Feb 10, 2017

Author Member

Done.

@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 10, 2017

I updated the test expectations (d6e2792). Summary:

  • Some tests now PASS, hooray!
/html/browsers/history/the-location-interface/security_location_0.htm
/html/browsers/the-window-object/security-window/window-security.html
/html/semantics/embedded-content/the-iframe-element/iframe-load-event.html
/navigation-timing/test_unique_performance_objects.html 
/html/semantics/scripting-1/the-template-element/...
/html/syntax/parsing/template/...
  • Some tests which were FAIL are now TIMEOUT. The timeouts are caused by postMessage, in particular event.source being undefined.
  • I bugfixed tests/wpt/mozilla/tests/mozilla/referrer-policy/generic/common.js, which was stringifying iframe.contentWindow.location even for cross-origin windows, which now generates a security error.

@Ms2ger: with these updates to the test expectations, are you okay with this PR going ahead without document.domain?

@asajeffrey asajeffrey force-pushed the asajeffrey:script-same-origin-domain branch from d7d00a2 to 583a7dd Feb 13, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 13, 2017

Updated #15438.

@asajeffrey asajeffrey mentioned this pull request Feb 13, 2017
4 of 4 tasks complete
@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 13, 2017

@Ms2ger I added the setter for document.domain over in #15536, which includes a test for same-origin-domain security checks.

@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 15, 2017

@Ms2ger ping?

@asajeffrey asajeffrey force-pushed the asajeffrey:script-same-origin-domain branch from 583a7dd to 92d220b Feb 15, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 15, 2017

Rebased.

@bors-servo
Copy link
Contributor

bors-servo commented Feb 18, 2017

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

@asajeffrey asajeffrey force-pushed the asajeffrey:script-same-origin-domain branch from 92d220b to b07d2dc Feb 21, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 21, 2017

Fixed a bug which was causing blob://http://foo.com/ to be considered dissimilar-origin to http://foo.com/. Also rebased.

@asajeffrey asajeffrey mentioned this pull request Feb 21, 2017
3 of 3 tasks complete
@bors-servo
Copy link
Contributor

bors-servo commented Feb 22, 2017

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

@asajeffrey asajeffrey force-pushed the asajeffrey:script-same-origin-domain branch 2 times, most recently from d91b63a to 26ea806 Feb 22, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 22, 2017

#15438 landed, so this can now land.

@Ms2ger: are you still wanting more tests? If so, we probably need to merge this with #15536.

@bors-servo
Copy link
Contributor

bors-servo commented Feb 23, 2017

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

@asajeffrey asajeffrey force-pushed the asajeffrey:script-same-origin-domain branch from 26ea806 to 1d8d6ac Feb 23, 2017
@asajeffrey
Copy link
Member Author

asajeffrey commented Feb 27, 2017

Conversation with @jdm on irc: http://logs.glob.uno/?c=mozilla%23servo&s=24+Feb+2017&e=24+Feb+2017#c619500

TL;DR: merge this PR with #15536.

@asajeffrey asajeffrey closed this Feb 27, 2017
bors-servo added a commit that referenced this pull request Mar 15, 2017
Implement setter for document.domain

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

This PR implements the setter for `document.domain`.

It builds on #15438 and #15478, only the last commit is part of this PR.

It includes tests for similar-origin security checks.

---
<!-- 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 #934.
- [X] There are tests for these changes.

<!-- 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/15536)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Mar 15, 2017
Implement setter for document.domain

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

This PR implements the setter for `document.domain`.

It builds on #15438 and #15478, only the last commit is part of this PR.

It includes tests for similar-origin security checks.

---
<!-- 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 #934.
- [X] There are tests for these changes.

<!-- 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/15536)
<!-- Reviewable:end -->
bors-servo added a commit that referenced this pull request Mar 15, 2017
Implement cross-thread postMessage

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

This PR implements cross-thread postMessage,

It builds on #15438 and #15478, only the last commit is part of this PR.

---
<!-- 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] There are tests for these changes

<!-- 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/15679)
<!-- Reviewable:end -->
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.

None yet

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