[WIP] Cross origin wrappers by avadacatavra · Pull Request #16501 · servo/servo

avadacatavra · 2017-04-17T20:38:28Z

This PR adds some infrastructure required for XOWs.

Addresses #16243 and #2382

./mach build -d does not report any errors
./mach test-tidy does not report any errors
These changes fix #__ (github issue number if applicable).

There are tests for these changes OR
These changes do not require tests because _____

This change is

highfive · 2017-04-17T20:38:33Z

Heads up! This PR modifies the following files:

@fitzgen: components/script/script_thread.rs, components/script/dom/window.rs, components/script/script_runtime.rs, components/script/dom/bindings/interface.rs, components/script/dom/bindings/utils.rs
@KiChjang: components/script/script_thread.rs, components/script/dom/window.rs, components/script/script_runtime.rs, components/script/dom/bindings/interface.rs, components/script/dom/bindings/utils.rs

highfive · 2017-04-17T20:38:34Z

Warning

These commits modify unsafe code. Please review it carefully!
These commits modify script code, but no tests are modified. Please consider adding a test!

asajeffrey · 2017-04-17T22:30:32Z

Yay, security policy!

@jdm

added servojsprincipal skeleton Cross origin wrappers (servo/servo#16501) require JSPrincipals. This PR adds the skeleton and basic functionality required. CC: @jdm  --- This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/servo/rust-mozjs/353)

bors-servo · 2017-04-24T14:46:59Z

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

jdm · 2017-05-01T16:43:58Z

components/script/dom/bindings/utils.rs


+// avadacatavra: destroy will need to retrieved the boxed origin pointer, turn it back into a box and allow it to be freed
+pub unsafe fn destroy_servo_jsprincipal(principal: &mut ServoJSPrincipal) {
+    let origin = GetPrincipalOrigin(principal.0) as *mut Box<MutableOrigin>;


Casting to a *mut Box is incorrect; we need *mut MutableOrigin here so we can use from_raw and cause its destructor to run.

jdm · 2017-05-01T16:44:11Z

components/script/dom/bindings/utils.rs

    }
 }

+// avadacatavra: destroy will need to retrieved the boxed origin pointer, turn it back into a box and allow it to be freed


We'll want to remove this.

jdm · 2017-05-01T16:44:25Z

components/script/dom/bindings/utils.rs

 }

-struct ServoJSPrincipal(*mut JSPrincipals);
+//TODO make principal.rs


jdm · 2017-05-01T16:48:34Z

components/script/dom/bindings/utils.rs

+    match &*name {
+        "Location" => CrossOriginObjectType::CrossOriginLocation,
+        "Window" => CrossOriginObjectType::CrossOriginWindow,
+        _ => CrossOriginObjectType::CrossOriginOpaque,


let name = CStr::from_ptr((*obj_class).name).to_bytes(); match name { b"Location" => ..., b"Window" => ..., _ => ..., }

jdm · 2017-05-01T16:49:32Z

components/script/dom/bindings/utils.rs

+    }
+}
+
+unsafe fn target_subsumes_obj(cx: *mut JSContext, obj: HandleObject) -> bool {


Let's call this obj_subsumes_current_compartment.

jdm · 2017-05-01T16:50:31Z

components/script/dom/bindings/codegen/CodegenRust.py

        values["members"] = "\n".join(members)

        return CGGeneric("""\
+let origin = object.origin().clone();


Do we need this clone if we pass a reference to create_global_object?

jdm · 2017-05-01T16:51:52Z

components/script/dom/bindings/utils.rs

+//TODO is same_origin_domain equivalent to subsumes for our purposes 
+pub unsafe extern fn subsumes(obj: *mut JSPrincipals, other: *mut JSPrincipals) -> bool {
+    let obj = &ServoJSPrincipal(obj);
+    let other = &ServoJSPrincipal(other);


The &s aren't necessary here.

jdm · 2017-05-01T16:52:32Z

components/script/dom/bindings/utils.rs

+    //step 7 compare hosts
+
+    //step 8 compare ports
+    //false


Since these are all covered by subsumes afaict, we don't need to leave references to them here.

jdm · 2017-05-01T16:53:39Z

components/script/dom/bindings/utils.rs

+
+unsafe fn select_wrapper(cx: *mut JSContext, obj: HandleObject) -> *const libc::c_void {
+    let security_wrapper = !target_subsumes_obj(cx, obj);
+    if !security_wrapper {


This will be easier to reason about as if target_subsumes_obj(cx, obj) {.

jdm · 2017-05-01T16:54:06Z

components/script/dom/bindings/utils.rs

@@ -387,7 +478,8 @@ unsafe extern "C" fn wrap(cx: *mut JSContext,
                          -> *mut JSObject {
    // FIXME terrible idea. need security wrappers
    // https://github.com/servo/servo/issues/2382


These comments can go away.

bors-servo · 2017-05-13T13:13:28Z

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

jdm · 2017-11-15T22:37:06Z

We're waiting to return to this work until we have a more recent spidermonkey.

atouchet · 2018-09-10T21:51:47Z

@jdm @avadacatavra should this be re-opened now that SpiderMonkey has been updated?

jdm · 2018-09-10T23:05:00Z

No need to do that until someone is actually working on it.

…ts of servo#16501

Implement `Location`'s custom internal methods This PR partly resurrects #16501 and introduces the use of principals object to associate objects and Realms with origins. Using this infrastructure, this PR implements [the custom internal methods][1] of the `Location` interface, which is "maybe-cross-origin". Other maybe-cross-origin interfaces, namely `WindowProxy` and `DissimilarWindowLocation`, aren't implemented correctly yet (causing most test cases of `tests/wpt/web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects.html` to fail). [The "perform a security check" operation][2] and `Location`'s non-cross-origin properties' relevant `Document` origin checks aren't implemented either (not sure if they are covered by the existing tests). Finally, there are a slight deviation from the standard and inefficiency in `CrossOriginGetOwnPropertyHelper`'s current implementation. [1]: https://html.spec.whatwg.org/multipage/#the-location-interface [2]: https://html.spec.whatwg.org/multipage/browsers.html#integration-with-idl --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #16243 and make some progress in #2382 --- - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___

Implement `Location`'s custom internal methods This PR partly resurrects #16501 and introduces the use of principals object to associate objects and Realms with origins. Using this infrastructure, this PR implements [the custom internal methods][1] of the `Location` interface, which is "maybe-cross-origin". Unimplemented/incomplete things: - Other maybe-cross-origin interfaces, namely `WindowProxy` and `DissimilarWindowLocation`, aren't implemented correctly yet (causing most test cases of `tests/wpt/web-platform-tests/html/browsers/origin/cross-origin-objects/cross-origin-objects.html` to fail). - `WindowProxy`: #28556 - [The "perform a security check" operation][2] and `Location`'s non-cross-origin properties' relevant `Document` origin checks aren't implemented either (not sure if they are covered by the existing tests). - There are a slight deviation from the standard and inefficiency in `CrossOriginGetOwnPropertyHelper`'s current implementation. - #28557 [1]: https://html.spec.whatwg.org/multipage/#the-location-interface [2]: https://html.spec.whatwg.org/multipage/browsers.html#integration-with-idl --- - [x] `./mach build -d` does not report any errors - [x] `./mach test-tidy` does not report any errors - [x] These changes fix #16243 and make some progress in #2382 --- - [x] There are tests for these changes OR - [ ] These changes do not require tests because ___

avadacatavra requested a review from jdm April 17, 2017 20:38

highfive assigned cbrewster Apr 17, 2017

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Apr 17, 2017

avadacatavra assigned jdm Apr 17, 2017

cbrewster removed their assignment Apr 17, 2017

avadacatavra mentioned this pull request Apr 18, 2017

added servojsprincipal skeleton servo/rust-mozjs#353

Merged

avadacatavra force-pushed the xow branch from 57fbf54 to 3451b28 Compare April 18, 2017 14:58

highfive added the S-needs-rebase There are merge conflict errors. label Apr 24, 2017

avadacatavra force-pushed the xow branch from 3f379c5 to f7ffc79 Compare April 25, 2017 17:24

avadacatavra added 3 commits April 25, 2017 21:10

added JSPrincipal bindings

ef053db

added xow security callbacks

9151b4b

added wrapper selection and subsumes logic

3577d1f

avadacatavra force-pushed the xow branch 2 times, most recently from c8ed375 to 39a524c Compare April 25, 2017 22:05

changed to use globalscope origin

aa7b4f6

avadacatavra force-pushed the xow branch from 39a524c to aa7b4f6 Compare April 25, 2017 23:42

jdm requested changes May 1, 2017

View reviewed changes

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 1, 2017

investigating failures after filtering wrapper integration with mozjs

3acac9d

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels May 9, 2017

jdm added S-needs-code-changes Changes have not yet been made that were requested by a reviewer. and removed S-awaiting-review There is new code that needs to be reviewed. labels May 9, 2017

debugging

bb283d9

added dom exception callback stuff

af3940f

highfive added S-awaiting-review There is new code that needs to be reviewed. and removed S-needs-code-changes Changes have not yet been made that were requested by a reviewer. labels May 12, 2017

jdm removed the S-awaiting-review There is new code that needs to be reviewed. label May 12, 2017

highfive added the S-needs-rebase There are merge conflict errors. label May 13, 2017

added in cross origin type checks. assert_throws isn't working yet

0316a0d

highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 15, 2017

jdm removed the S-awaiting-review There is new code that needs to be reviewed. label May 16, 2017

frames override is the worst

0d9d5b3

highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 17, 2017

jdm removed the S-awaiting-review There is new code that needs to be reviewed. label May 18, 2017

show which tests work

8a682cf

highfive added the S-awaiting-review There is new code that needs to be reviewed. label May 30, 2017

jdm removed the S-awaiting-review There is new code that needs to be reviewed. label May 30, 2017

xow tests reflect current state

9a7978e

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 12, 2017

jdm removed the S-awaiting-review There is new code that needs to be reviewed. label Jun 12, 2017

debugging

2346420

highfive added the S-awaiting-review There is new code that needs to be reviewed. label Jun 15, 2017

enumerate should work. but is a dirty hack

ff8d2cd

jdm removed the S-awaiting-review There is new code that needs to be reviewed. label Jun 16, 2017

jdm closed this Nov 15, 2017

yvt added a commit to yvt/servo that referenced this pull request Jul 13, 2021

fix(script): apply some of the changes requested in the review commen…

e786627

…ts of servo#16501

yvt mentioned this pull request Jul 17, 2021

Implement Location's custom internal methods #28546

Merged

5 tasks

Uh oh!

Conversation

avadacatavra commented Apr 17, 2017 • edited by larsbergstrom Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

highfive commented Apr 17, 2017

Uh oh!

highfive commented Apr 17, 2017

Uh oh!

asajeffrey commented Apr 17, 2017

Uh oh!

bors-servo commented Apr 24, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bors-servo commented May 13, 2017

Uh oh!

jdm commented Nov 15, 2017

Uh oh!

atouchet commented Sep 10, 2018

Uh oh!

jdm commented Sep 10, 2018

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

7 participants

avadacatavra commented Apr 17, 2017 •

edited by larsbergstrom

Loading