Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1973363004:
Expose a way to make a "same-origin" realm (Closed)
|
Created:
4 years, 7 months ago by Dan Ehrenberg Modified:
4 years, 6 months ago CC:
v8-reviews_googlegroups.com Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
DescriptionExpose a way to make a same-origin realm
Some tests, e.g. in test262, want to create a new same-origin
realm. This patch exposes a new function,
Realm.createAllowCrossRealmAccess(), which vends a new realm with
the same security token as the currently executing one.
Committed: https://crrev.com/9778f2efad6100402d6435aa169037e3f9331439
Cr-Commit-Position: refs/heads/master@{#36561}
Patch Set 1 #Patch Set 2 : Add a test #
Total comments: 2
Patch Set 3 : Take review suggestion #Patch Set 4 : fix typo #
Total comments: 7
Patch Set 5 : Eliminate code duplication #Patch Set 6 : Add simple additional test #
Total comments: 4
Patch Set 7 : Style fixes #
Messages
Total messages: 41 (19 generated)
Description was changed from ========== Make same origin BUG= ========== to ========== Expose setting a realm's security token Some tests, e.g. in test262, want to create a new same-origin realm. This patch exposes a new function, Realm.makeSameOrigin(), to set the security token to the running context's token. ==========
littledan@chromium.org changed reviewers: + adamk@chromium.org
The CQ bit was checked by littledan@chromium.org
The CQ bit was unchecked by littledan@chromium.org
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973363004/20001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973363004/20001
adamk@chromium.org changed reviewers: + rossberg@chromium.org
+rossberg as he was the original author of this API, and in that patch included this comment: // (Note that properties of global objects cannot be read/written cross-realm.) I'd like to understand if there's some deep reason for that, or if it was simply what was needed for the tests of the time. Clearly there's no real security issue here, both because such properties can easily be read by calling Realm.eval(). https://codereview.chromium.org/1973363004/diff/20001/src/d8.h File src/d8.h (right): https://codereview.chromium.org/1973363004/diff/20001/src/d8.h#newcode378 src/d8.h:378: static void RealmMakeSameOrigin( Taking a different-origin realm and making it same-origin seems a little funny to me. What if you instead add a new kind of creation? RealmCreateSameOrigin, perhaps? Or an extra argument to Create? Naming: "Origin" is unfortunately a web concept and not a JS concept. It's mentioned in passing in the API, but I wonder if there's something better we could use. "RealmCreateAllowCrossRealmAccess" seems really long, but I think we should think on this. Whatever name we use here is likely to be treated as part of our API.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Ping, rossberg PTAL
I think the comment was just descriptive. With this CL, it should probably be reworded to say that they are only accessible if the global has the right security token. https://codereview.chromium.org/1973363004/diff/20001/src/d8.h File src/d8.h (right): https://codereview.chromium.org/1973363004/diff/20001/src/d8.h#newcode378 src/d8.h:378: static void RealmMakeSameOrigin( On 2016/05/13 23:09:59, adamk wrote: > Taking a different-origin realm and making it same-origin seems a little funny > to me. What if you instead add a new kind of creation? RealmCreateSameOrigin, > perhaps? Or an extra argument to Create? > > Naming: "Origin" is unfortunately a web concept and not a JS concept. It's > mentioned in passing in the API, but I wonder if there's something better we > could use. "RealmCreateAllowCrossRealmAccess" seems really long, but I think we > should think on this. Whatever name we use here is likely to be treated as part > of our API. +1
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973363004/40001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973363004/40001
Description was changed from ========== Expose setting a realm's security token Some tests, e.g. in test262, want to create a new same-origin realm. This patch exposes a new function, Realm.makeSameOrigin(), to set the security token to the running context's token. ========== to ========== Expose a way to make a same-origin realm Some tests, e.g. in test262, want to create a new same-origin realm. This patch exposes a new function, Realm.createAllowCrossRealmAccess(), which vends a new realm with the same security token as the currently executing one. ==========
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973363004/60001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973363004/60001
Changed the naming to what Adam suggested suggested, WDYT?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm https://codereview.chromium.org/1973363004/diff/60001/src/d8.cc File src/d8.cc (right): https://codereview.chromium.org/1973363004/diff/60001/src/d8.cc#newcode553 src/d8.cc:553: void Shell::RealmCreateAllowCrossRealmAccess( I wouldn't mind factoring out the commonalities between this and and the previous method.
https://codereview.chromium.org/1973363004/diff/60001/src/d8.cc File src/d8.cc (right): https://codereview.chromium.org/1973363004/diff/60001/src/d8.cc#newcode551 src/d8.cc:551: // Realm.createAllowCrossRealmAccess(i) jreates a new realm with the same s/jreates/creates/ https://codereview.chromium.org/1973363004/diff/60001/src/d8.cc#newcode553 src/d8.cc:553: void Shell::RealmCreateAllowCrossRealmAccess( On 2016/05/19 06:07:00, rossberg wrote: > I wouldn't mind factoring out the commonalities between this and and the > previous method. Agreed, the fewer places we have to write 'delete[]' the better :) https://codereview.chromium.org/1973363004/diff/60001/test/mjsunit/realm-prop... File test/mjsunit/realm-property-access.js (right): https://codereview.chromium.org/1973363004/diff/60001/test/mjsunit/realm-prop... test/mjsunit/realm-property-access.js:15: // Same-origin property access doesn't throw Please also add a test that accessing properties of Realm.global(r2) works too (realize it's the same thing, but just to cover the API surface of Realm).
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973363004/80001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973363004/80001
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973363004/100001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973363004/100001
PTAL https://codereview.chromium.org/1973363004/diff/60001/src/d8.cc File src/d8.cc (right): https://codereview.chromium.org/1973363004/diff/60001/src/d8.cc#newcode551 src/d8.cc:551: // Realm.createAllowCrossRealmAccess(i) jreates a new realm with the same On 2016/05/19 at 19:23:20, adamk wrote: > s/jreates/creates/ Done https://codereview.chromium.org/1973363004/diff/60001/src/d8.cc#newcode553 src/d8.cc:553: void Shell::RealmCreateAllowCrossRealmAccess( On 2016/05/19 at 19:23:20, adamk wrote: > On 2016/05/19 06:07:00, rossberg wrote: > > I wouldn't mind factoring out the commonalities between this and and the > > previous method. > > Agreed, the fewer places we have to write 'delete[]' the better :) Done https://codereview.chromium.org/1973363004/diff/60001/test/mjsunit/realm-prop... File test/mjsunit/realm-property-access.js (right): https://codereview.chromium.org/1973363004/diff/60001/test/mjsunit/realm-prop... test/mjsunit/realm-property-access.js:15: // Same-origin property access doesn't throw On 2016/05/19 at 19:23:20, adamk wrote: > Please also add a test that accessing properties of Realm.global(r2) works too (realize it's the same thing, but just to cover the API surface of Realm). A new test checks that f{,2} equals the output of Realm.global to be sure that it's the same thing.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
lgtm % nits (which don't necessarily require action, use your judgment). https://codereview.chromium.org/1973363004/diff/100001/src/d8.cc File src/d8.cc (right): https://codereview.chromium.org/1973363004/diff/100001/src/d8.cc#newcode548 src/d8.cc:548: return MaybeLocal<Context>(context); Nit: doesn't a Local<Context> silently coerce to a MaybeLocal<Context>? I would have expected just "return context" to work. https://codereview.chromium.org/1973363004/diff/100001/src/d8.h File src/d8.h (right): https://codereview.chromium.org/1973363004/diff/100001/src/d8.h#newcode374 src/d8.h:374: static MaybeLocal<Context> CreateRealm( Nit: Seems like this could be private, but so little in this class is private anyway that it may not be worthwhile.
The CQ bit was checked by littledan@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973363004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973363004/120001
The CQ bit was unchecked by littledan@chromium.org
The CQ bit was checked by littledan@chromium.org
https://codereview.chromium.org/1973363004/diff/100001/src/d8.cc File src/d8.cc (right): https://codereview.chromium.org/1973363004/diff/100001/src/d8.cc#newcode548 src/d8.cc:548: return MaybeLocal<Context>(context); On 2016/05/27 at 18:10:51, adamk wrote: > Nit: doesn't a Local<Context> silently coerce to a MaybeLocal<Context>? I would have expected just "return context" to work. Done https://codereview.chromium.org/1973363004/diff/100001/src/d8.h File src/d8.h (right): https://codereview.chromium.org/1973363004/diff/100001/src/d8.h#newcode374 src/d8.h:374: static MaybeLocal<Context> CreateRealm( On 2016/05/27 at 18:10:51, adamk wrote: > Nit: Seems like this could be private, but so little in this class is private anyway that it may not be worthwhile. Done
The patchset sent to the CQ was uploaded after l-g-t-m from rossberg@chromium.org, adamk@chromium.org Link to the patchset: https://codereview.chromium.org/1973363004/#ps120001 (title: "Style fixes")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1973363004/120001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1973363004/120001
Message was sent while issue was closed.
Committed patchset #7 (id:120001)
Message was sent while issue was closed.
Description was changed from ========== Expose a way to make a same-origin realm Some tests, e.g. in test262, want to create a new same-origin realm. This patch exposes a new function, Realm.createAllowCrossRealmAccess(), which vends a new realm with the same security token as the currently executing one. ========== to ========== Expose a way to make a same-origin realm Some tests, e.g. in test262, want to create a new same-origin realm. This patch exposes a new function, Realm.createAllowCrossRealmAccess(), which vends a new realm with the same security token as the currently executing one. Committed: https://crrev.com/9778f2efad6100402d6435aa169037e3f9331439 Cr-Commit-Position: refs/heads/master@{#36561} ==========
Message was sent while issue was closed.
Patchset 7 (id:??) landed as https://crrev.com/9778f2efad6100402d6435aa169037e3f9331439 Cr-Commit-Position: refs/heads/master@{#36561} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
