|
|
Created:
3 years, 10 months ago by jochen (gone - plz use gerrit) Modified:
3 years, 10 months ago CC:
blink-reviews, blink-reviews-bindings_chromium.org, chromium-reviews Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionAdd security checks to scheduled actions
Scheduled actions end up generating code from strings, so we should put
similar restrictions as for eval() in place
R=haraken@chromium.org,dcheng@chromium.org
BUG=693695, 694446
Review-Url: https://codereview.chromium.org/2702213004
Cr-Commit-Position: refs/heads/master@{#452016}
Committed: https://chromium.googlesource.com/chromium/src/+/0ac9c3a4fbf57ad4cc2f954fc452fa62a729e0e1
Patch Set 1 #Patch Set 2 : updates #Patch Set 3 : updates #
Total comments: 5
Patch Set 4 : updates #Patch Set 5 : updates #Patch Set 6 : win baseline #
Total comments: 1
Patch Set 7 : needs-rebaseline #Patch Set 8 : updates #
Messages
Total messages: 41 (24 generated)
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
hum, maybe a tad too aggressive
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
Description was changed from ========== Add security checks to scheduled actions Scheduled actions end up generating code from strings, so we should put similar restrictions as for eval() in place R=haraken@chromium.org,dcheng@chromium.org BUG=693695 ========== to ========== Add security checks to scheduled actions Scheduled actions end up generating code from strings, so we should put similar restrictions as for eval() in place R=haraken@chromium.org,dcheng@chromium.org BUG=693695,694446 ==========
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
green-ish bots (at least on mac) PTAL
https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right): https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:50: ExecutionContext* target, Is the target execution context different from scriptState->getExecutionContext()?
https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right): https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:50: ExecutionContext* target, On 2017/02/21 at 13:10:20, haraken wrote: > Is the target execution context different from scriptState->getExecutionContext()? possibly - you could invoke setTimeout on a same-origin window
yukishiino@chromium.org changed reviewers: + yukishiino@chromium.org
LGTM on my side. https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.h (right): https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScheduledAction.h:54: ExecutionContext*, I'd like to have |target| as the argument name so that it's clear that |target| may be different from the ScriptState.
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.h (right): https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScheduledAction.h:54: ExecutionContext*, On 2017/02/21 at 13:17:57, Yuki wrote: > I'd like to have |target| as the argument name so that it's clear that |target| may be different from the ScriptState. done
The CQ bit was checked by jochen@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
LGTM https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right): https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:50: ExecutionContext* target, On 2017/02/21 13:13:02, jochen wrote: > On 2017/02/21 at 13:10:20, haraken wrote: > > Is the target execution context different from > scriptState->getExecutionContext()? > > possibly - you could invoke setTimeout on a same-origin window Ah, right. Maybe would it be nicer to move the CHECK to ScheduledAction::execute(ExecutionContext* context)? Then you have the target execution context already. Also we already have a branch for the main thread.
On 2017/02/21 at 13:23:10, haraken wrote: > LGTM > > https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... > File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right): > > https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... > third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:50: ExecutionContext* target, > On 2017/02/21 13:13:02, jochen wrote: > > On 2017/02/21 at 13:10:20, haraken wrote: > > > Is the target execution context different from > > scriptState->getExecutionContext()? > > > > possibly - you could invoke setTimeout on a same-origin window > > Ah, right. > > Maybe would it be nicer to move the CHECK to ScheduledAction::execute(ExecutionContext* context)? Then you have the target execution context already. Also we already have a branch for the main thread. however, I'd have to store a reference to the entered context then, also not sure what we should do if that context navigated meanwhile and could then no longer schedule the action?
On 2017/02/21 13:26:28, jochen wrote: > On 2017/02/21 at 13:23:10, haraken wrote: > > LGTM > > > > > https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... > > File third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp (right): > > > > > https://codereview.chromium.org/2702213004/diff/40001/third_party/WebKit/Sour... > > third_party/WebKit/Source/bindings/core/v8/ScheduledAction.cpp:50: > ExecutionContext* target, > > On 2017/02/21 13:13:02, jochen wrote: > > > On 2017/02/21 at 13:10:20, haraken wrote: > > > > Is the target execution context different from > > > scriptState->getExecutionContext()? > > > > > > possibly - you could invoke setTimeout on a same-origin window > > > > Ah, right. > > > > Maybe would it be nicer to move the CHECK to > ScheduledAction::execute(ExecutionContext* context)? Then you have the target > execution context already. Also we already have a branch for the main thread. > > however, I'd have to store a reference to the entered context then, also not > sure what we should do if that context navigated meanwhile and could then no > longer schedule the action? Ah, you're right.
The CQ bit was checked by jochen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from yukishiino@chromium.org, haraken@chromium.org Link to the patchset: https://codereview.chromium.org/2702213004/#ps100001 (title: "win baseline")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_rel_ng on master.tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
https://codereview.chromium.org/2702213004/diff/100001/third_party/WebKit/Sou... File third_party/WebKit/Source/core/testing/Internals.cpp (right): https://codereview.chromium.org/2702213004/diff/100001/third_party/WebKit/Sou... third_party/WebKit/Source/core/testing/Internals.cpp:904: // We need to make the popup same origin so layout tests can access it. Uhhhh. Wow. Btw, should we be calling updateSecurityOrigin() here instead of setSecurityOrigin()?
The CQ bit was checked by jochen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2702213004/#ps120001 (title: "needs-rebaseline")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by jochen@chromium.org
The CQ bit was checked by jochen@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from haraken@chromium.org, yukishiino@chromium.org Link to the patchset: https://codereview.chromium.org/2702213004/#ps140001 (title: "updates")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch. Bot data: {"patchset_id": 140001, "attempt_start_ts": 1487761624336560, "parent_rev": "c688ec0f1e7fa2ba31238c5e316f738cf29bfc14", "commit_rev": "0ac9c3a4fbf57ad4cc2f954fc452fa62a729e0e1"}
Message was sent while issue was closed.
Description was changed from ========== Add security checks to scheduled actions Scheduled actions end up generating code from strings, so we should put similar restrictions as for eval() in place R=haraken@chromium.org,dcheng@chromium.org BUG=693695,694446 ========== to ========== Add security checks to scheduled actions Scheduled actions end up generating code from strings, so we should put similar restrictions as for eval() in place R=haraken@chromium.org,dcheng@chromium.org BUG=693695,694446 Review-Url: https://codereview.chromium.org/2702213004 Cr-Commit-Position: refs/heads/master@{#452016} Committed: https://chromium.googlesource.com/chromium/src/+/0ac9c3a4fbf57ad4cc2f954fc452... ==========
Message was sent while issue was closed.
Committed patchset #8 (id:140001) as https://chromium.googlesource.com/chromium/src/+/0ac9c3a4fbf57ad4cc2f954fc452...
Message was sent while issue was closed.
A revert of this CL (patchset #8 id:140001) has been created in https://codereview.chromium.org/2711163003/ by jochen@chromium.org. The reason for reverting is: Doesn't take microtasks into account - will reland with fix. |