[DoNotCheckSecurity=Getter] => [DoNotCheckSecurity]

[DoNotCheckSecurity=Getter] => [DoNotCheckSecurity]

[DoNotCheckSecurity=Getter] is used only one place (Window.opener), and can be eliminated, as the Setter is custom, and checks security there (V8WindowCustom.cpp).

Note that this makes the access control list misleading.

[DoNotCheckSecurity=Setter] is only used one place (Location.href), but this is a real exception, so we have to keep it.
Adding a comment to explain why (changes page, so ok to set).

Updated docs:
https://sites.google.com/a/chromium.org/dev/blink/webidl/blink-idl-extended-attributes#TOC-DoNotCheckSecurity-m-a-

Committed: https://src.chromium.org/viewvc/blink?view=rev&revision=162355

[Nils Barth (inactive), 2013-11-20 08:00:43]

Test CL to check what happens if we remove the identifiers.
Alternatively, for href may want to turn *on* security checks for both?

[haraken, 2013-11-20 08:02:59]

What's the difference in generated code?

If we have any difference in generated code, we have to be very careful about
making the change. This is about security.

[Nils Barth (inactive), 2013-11-20 08:13:29]

On 2013/11/20 08:02:59, haraken wrote:
> What's the difference in generated code?
> 
> If we have any difference in generated code, we have to be very careful about
> making the change. This is about security.

There is a difference, specifically the access control list, as noted in other
CL:

diff -r gen-orig/blink/bindings/V8Location.cpp gen/blink/bindings/V8Location.cpp
614c614
<     {"href", LocationV8Internal::hrefAttributeGetterCallback,
LocationV8Internal::hrefAttributeSetterCallback, 0, 0, 0,
static_cast<v8::AccessControl>(v8::ALL_CAN_WRITE | v8::PROHIBITS_OVERWRITING),
static_cast<v8::PropertyAttribute>(v8::DontDelete), 0 /* on instance */},
---
>     {"href", LocationV8Internal::hrefAttributeGetterCallback,
LocationV8Internal::hrefAttributeSetterCallback, 0, 0, 0,
static_cast<v8::AccessControl>(v8::ALL_CAN_READ | v8::ALL_CAN_WRITE |
v8::PROHIBITS_OVERWRITING), static_cast<v8::PropertyAttribute>(v8::DontDelete),
0 /* on instance */},
diff -r gen-orig/blink/bindings/V8Window.cpp gen/blink/bindings/V8Window.cpp
6961c6961
<     {"opener", DOMWindowV8Internal::openerAttributeGetterCallback,
DOMWindowV8Internal::openerAttributeSetterCallback, 0, 0, 0,
static_cast<v8::AccessControl>(v8::ALL_CAN_READ),
static_cast<v8::PropertyAttribute>(v8::None), 0 /* on instance */},
---
>     {"opener", DOMWindowV8Internal::openerAttributeGetterCallback,
DOMWindowV8Internal::openerAttributeSetterCallback, 0, 0, 0,
static_cast<v8::AccessControl>(v8::ALL_CAN_READ | v8::ALL_CAN_WRITE),
static_cast<v8::PropertyAttribute>(v8::None), 0 /* on instance */},

[haraken, 2013-11-20 08:18:39]

> There is a difference, specifically the access control list, as noted in other
> CL:
> 
> diff -r gen-orig/blink/bindings/V8Location.cpp
gen/blink/bindings/V8Location.cpp
> 614c614
> <     {"href", LocationV8Internal::hrefAttributeGetterCallback,
> LocationV8Internal::hrefAttributeSetterCallback, 0, 0, 0,
> static_cast<v8::AccessControl>(v8::ALL_CAN_WRITE | v8::PROHIBITS_OVERWRITING),
> static_cast<v8::PropertyAttribute>(v8::DontDelete), 0 /* on instance */},
> ---
> >     {"href", LocationV8Internal::hrefAttributeGetterCallback,
> LocationV8Internal::hrefAttributeSetterCallback, 0, 0, 0,
> static_cast<v8::AccessControl>(v8::ALL_CAN_READ | v8::ALL_CAN_WRITE |
> v8::PROHIBITS_OVERWRITING),
static_cast<v8::PropertyAttribute>(v8::DontDelete),
> 0 /* on instance */},
> diff -r gen-orig/blink/bindings/V8Window.cpp gen/blink/bindings/V8Window.cpp
> 6961c6961
> <     {"opener", DOMWindowV8Internal::openerAttributeGetterCallback,
> DOMWindowV8Internal::openerAttributeSetterCallback, 0, 0, 0,
> static_cast<v8::AccessControl>(v8::ALL_CAN_READ),
> static_cast<v8::PropertyAttribute>(v8::None), 0 /* on instance */},
> ---
> >     {"opener", DOMWindowV8Internal::openerAttributeGetterCallback,
> DOMWindowV8Internal::openerAttributeSetterCallback, 0, 0, 0,
> static_cast<v8::AccessControl>(v8::ALL_CAN_READ | v8::ALL_CAN_WRITE),
> static_cast<v8::PropertyAttribute>(v8::None), 0 /* on instance */},

Thanks for digging into this.

hmm, it's not clear to me how the difference affects web-exposed behavior. Would
you mind running http/tests/ and see what tests fail?

[Nils Barth (inactive), 2013-11-20 08:39:28]

On 2013/11/20 08:18:39, haraken wrote:
> Thanks for digging into this.

n/p!

> hmm, it's not clear to me how the difference affects web-exposed behavior.
Would
> you mind running http/tests/ and see what tests fail?

The specific directory of interest is (unsurprisingly):
http/tests/security

Changing this line in Window.idl causes no problems:
[DoNotCheckSecurity=Getter, Custom=Setter] attribute Window opener;
[DoNotCheckSecurity, Custom=Setter] attribute Window opener;

Changing this line (no security on getter):
[SetterCallWith=ActiveWindow&FirstWindow, DoNotCheckSecurity=Setter,
Unforgeable] attribute DOMString href;
[SetterCallWith=ActiveWindow&FirstWindow, DoNotCheckSecurity, Unforgeable]
attribute DOMString href;
...causes 30 test failures.

Uniformly turning on security checking:
[SetterCallWith=ActiveWindow&FirstWindow, Unforgeable] attribute DOMString href;
...causes 1 failure:
http/tests/security/xss-DENIED-assign-location-href-javascript.html

[haraken, 2013-11-20 08:54:55]

On 2013/11/20 08:39:28, Nils Barth wrote:
> On 2013/11/20 08:18:39, haraken wrote:
> > Thanks for digging into this.
> 
> n/p!
> 
> > hmm, it's not clear to me how the difference affects web-exposed behavior.
> Would
> > you mind running http/tests/ and see what tests fail?
> 
> The specific directory of interest is (unsurprisingly):
> http/tests/security
> 
> Changing this line in Window.idl causes no problems:
> [DoNotCheckSecurity=Getter, Custom=Setter] attribute Window opener;
> [DoNotCheckSecurity, Custom=Setter] attribute Window opener;
> 
> Changing this line (no security on getter):
> [SetterCallWith=ActiveWindow&FirstWindow, DoNotCheckSecurity=Setter,
> Unforgeable] attribute DOMString href;
> [SetterCallWith=ActiveWindow&FirstWindow, DoNotCheckSecurity, Unforgeable]
> attribute DOMString href;
> ...causes 30 test failures.
> 
> Uniformly turning on security checking:
> [SetterCallWith=ActiveWindow&FirstWindow, Unforgeable] attribute DOMString
href;
> ...causes 1 failure:
> http/tests/security/xss-DENIED-assign-location-href-javascript.html

Thanks, let me take a close look later today.

[Nils Barth (inactive), 2013-11-20 09:10:20]

On 2013/11/20 08:54:55, haraken wrote:
> On 2013/11/20 08:39:28, Nils Barth wrote:
> > On 2013/11/20 08:18:39, haraken wrote:
> > > Thanks for digging into this.
> > 
> > n/p!
> > 
> > > hmm, it's not clear to me how the difference affects web-exposed behavior.
> > Would
> > > you mind running http/tests/ and see what tests fail?
> > 
> > The specific directory of interest is (unsurprisingly):
> > http/tests/security
> > 
> > Changing this line in Window.idl causes no problems:
> > [DoNotCheckSecurity=Getter, Custom=Setter] attribute Window opener;
> > [DoNotCheckSecurity, Custom=Setter] attribute Window opener;
> > 
> > Changing this line (no security on getter):
> > [SetterCallWith=ActiveWindow&FirstWindow, DoNotCheckSecurity=Setter,
> > Unforgeable] attribute DOMString href;
> > [SetterCallWith=ActiveWindow&FirstWindow, DoNotCheckSecurity, Unforgeable]
> > attribute DOMString href;
> > ...causes 30 test failures.
> > 
> > Uniformly turning on security checking:
> > [SetterCallWith=ActiveWindow&FirstWindow, Unforgeable] attribute DOMString
> href;
> > ...causes 1 failure:
> > http/tests/security/xss-DENIED-assign-location-href-javascript.html
> 
> Thanks, let me take a close look later today.

I looked into this further: Location.href is a real exception.
So we can't remove [DoNotCheckSecurity=Setter], but we should add a comment.
We *may* be able to remove [DoNotCheckSecurity=Getter].

Here are some docs:
http://javascript.info/tutorial/same-origin-security-policy
http://stackoverflow.com/questions/952242/why-do-frame-breakers-work-cross-do...

In fact, any use of [DoNotCheckSecurity] is suspicious, so it really requires a
comment.
I've commented this as follows (the 3 uses in Location.idl):
    // |assign|, |replace|, and *writing* |href| do not require a security
    // check, as they *change* the page, and thus these do not change any
    // property of an *existing* document at a different origin.
    // However, *reading* |href|, or accessing any component, is a security
    // problem, since that allows tracking navigation.

[haraken, 2013-11-20 09:15:53]

Great, thanks for the comment. That makes sense. LGTM.

[haraken, 2013-11-20 09:17:11]

Also it would be great if you add similar comments to Window.idl about why
[DoNotCheckSecurity] is written on some attributes/methods.

[Nils Barth (inactive), 2013-11-20 09:29:30]

On 2013/11/20 09:17:11, haraken wrote:
> Also it would be great if you add similar comments to Window.idl about why
> [DoNotCheckSecurity] is written on some attributes/methods.

Shall I do this when I'm doing all the custom support for Window.idl?
I can't understand all the reasons at a glance, and I'll need to do lots of
work for Window.idl-specific bindings, so it would be easier then.

I've also removed [DoNotCheckSecurity=Getter] from the test cases and Python
compiler.

[haraken, 2013-11-20 09:30:26]

On 2013/11/20 09:29:30, Nils Barth wrote:
> On 2013/11/20 09:17:11, haraken wrote:
> > Also it would be great if you add similar comments to Window.idl about why
> > [DoNotCheckSecurity] is written on some attributes/methods.
> 
> Shall I do this when I'm doing all the custom support for Window.idl?
> I can't understand all the reasons at a glance, and I'll need to do lots of
> work for Window.idl-specific bindings, so it would be easier then.
> 
> I've also removed [DoNotCheckSecurity=Getter] from the test cases and Python
> compiler.

Sure.

[commit-bot: I haz the power, 2013-11-20 09:30:32]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/77723003/160001

[commit-bot: I haz the power, 2013-11-20 09:31:13]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/77723003/160001

[commit-bot: I haz the power, 2013-11-20 09:33:57]

CQ is trying da patch. Follow status at
https://chromium-status.appspot.com/cq/nbarth@chromium.org/77723003/160001

[Nils Barth (inactive), 2013-11-20 09:34:25]

On 2013/11/20 09:30:26, haraken wrote:
> On 2013/11/20 09:29:30, Nils Barth wrote:
> > Shall I do this when I'm doing all the custom support for Window.idl?
> 
> Sure.

Thanks! Added a FIXME to remind me.

[haraken, 2013-11-20 09:35:56]

On 2013/11/20 09:34:25, Nils Barth wrote:
> On 2013/11/20 09:30:26, haraken wrote:
> > On 2013/11/20 09:29:30, Nils Barth wrote:
> > > Shall I do this when I'm doing all the custom support for Window.idl?
> > 
> > Sure.
> 
> Thanks! Added a FIXME to remind me.

One good news is that the security binding code has good test coverage. You can
rely on http/tests/security/.

[Nils Barth (inactive), 2013-11-20 09:38:44]

On 2013/11/20 09:35:56, haraken wrote:
> One good news is that the security binding code has good test coverage. You
can
> rely on http/tests/security/.

That's good to know, thanks!

[commit-bot: I haz the power, 2013-11-20 10:58:41]

Change committed as 162355