IDL compiler: [CheckSecurity=Frame] interface + [DoNotCheckSecurity] members

IDL compiler: [CheckSecurity=Frame] interface + [DoNotCheckSecurity] members

This finishes [CheckSecurity=Frame] interface support.

This also refactors tests, moving [DoNotCheckSecurity] members into the
separate [CheckSecurity] test, as [DoNotCheckSecurity] on a member is
meaningless w/o [CheckSecurity] on the interface.

It also adds other tests for various combinations of extended attributes
that do appear in actual IDL files (Location.idl and Window.idl).

Also removes a gratuitous comment that lists all the extended attributes.

I've kept this simple; there are various special cases that are not necessary
in general, but may need support in Location.idl and Window.idl, which I'll
need to check individually anyway.

Followup to:
IDL compiler: [CheckSecurity=Frame] interfaces (initial)
https://codereview.chromium.org/61523003/

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

[Nils Barth (inactive), 2013-11-20 03:39:30]

CL 2/2 for [CheckSecurity=Frame].

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:289: ' |
'.join(method.property_attributes or ['v8::DontDelete']) %}
This |or ['v8::DontDelete']| replaces a *really* ugly hack in the Perl,
where it first adds 'v8::DontDelete' to the list of property attributes,
for [DoNotCheckSecurity] only (!),
then removes it if it's the only one!

[haraken, 2013-11-20 05:04:53]

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:101: {% block domain_safe_method_setter
%}

domain_safe_method_setter => attribute_setter_with_security_check

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:102: {% if has_domain_safe_method_setter
%}

has_domain_safe_method_setter => has_attribute_setter_with_security_check

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:103: static void
{{cpp_class_name}}DomainSafeFunctionSetter(v8::Local<v8::String> name,
v8::Local<v8::Value> jsValue, const v8::PropertyCallbackInfo<void>& info)

DomainSafeFunctionSetter => AttributeSetterWithSecurityCheck

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:280: underlying value, but instead
return a new copy of the original function.

I got confused with the name "DoNotCheckSecurity", since we allow access to
these methods with some security checks. Would you elaborate on it?

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:285:
'{0}V8Internal::{0}DomainSafeFunctionSetter'.format(cpp_class_name)

We want to have SetterCallback just like the GetterCallback. The SetterCallback
should have TRACE_EVENT_SET_SAMPLING_STATE etc. Would you implement it before
landing this CL? It will make this CL simpler.

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:290:
{{method.function_template}}->SetAccessor(v8::String::NewSymbol("{{method.name}}"),
{{callback_name}}, {{setter}}, v8Undefined(), v8::ALL_CAN_READ,
{{property_attribute}});

This should also be moved to V8DOMConfigratoin. A follow-up CL is fine.

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/methods.cpp:222: static void
{{method.name}}AttributeGetter{{world_suffix}}(const
v8::PropertyCallbackInfo<v8::Value>& info)

AttributeGetter => AttributeGetterWithSecurityCheck ?

[Nils Barth (inactive), 2013-11-20 06:11:03]

Replies, added FIXMEs to requests.

One Q: I'd rather add the SetterCallback in a followup,
as it's just added code (it makes this CL longer AFAICT),
and this CL is long enough as is.

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:101: {% block domain_safe_method_setter
%}
On 2013/11/20 05:04:54, haraken wrote:
> domain_safe_method_setter => attribute_setter_with_security_check

Got it; will do all the renames in a followup.

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:102: {% if has_domain_safe_method_setter
%}
On 2013/11/20 05:04:54, haraken wrote:
> has_domain_safe_method_setter => has_attribute_setter_with_security_check

Per above, in followup.

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:103: static void
{{cpp_class_name}}DomainSafeFunctionSetter(v8::Local<v8::String> name,
v8::Local<v8::Value> jsValue, const v8::PropertyCallbackInfo<void>& info)
On 2013/11/20 05:04:54, haraken wrote:
> DomainSafeFunctionSetter => AttributeSetterWithSecurityCheck

Per above, in followup.

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:280: underlying value, but instead
return a new copy of the original function.
On 2013/11/20 05:04:54, haraken wrote:
> I got confused with the name "DoNotCheckSecurity", since we allow access to
> these methods with some security checks. Would you elaborate on it?

[DoNotCheckSecurity] is only used in Location.idl and Window.idl.

The main thing it does is disable this check in the generated C++ method:

{% if method.is_check_security_for_frame %}
if (!BindingSecurity::shouldAllowAccessToFrame(imp->frame(), exceptionState)) {
    exceptionState.throwIfNeeded();
    return;
}
{% endif %}

I.e., the following 
[CheckSecurity=Frame] interface Foo { void bar(); };
...has a security check, but
[CheckSecurity=Frame] interface Foo { [DoNotCheckSecurity] void bar(); };
...does not.

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:285:
'{0}V8Internal::{0}DomainSafeFunctionSetter'.format(cpp_class_name)
On 2013/11/20 05:04:54, haraken wrote:
> We want to have SetterCallback just like the GetterCallback. The
SetterCallback
> should have TRACE_EVENT_SET_SAMPLING_STATE etc. Would you implement it before
> landing this CL? It will make this CL simpler.

I'm happy to do this, but I don't see how it would make this CL easier
(it would make it longer b/c I'd need the callback as well);
can I do it in a followup?

(We've identified 3 additional CLs for this one -- rename, move to
V8DOMConfiguration, and add SetterCallback -- and this is a pretty long CL as
is, so I'd rather do followups after landing.)

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:290:
{{method.function_template}}->SetAccessor(v8::String::NewSymbol("{{method.name}}"),
{{callback_name}}, {{setter}}, v8Undefined(), v8::ALL_CAN_READ,
{{property_attribute}});
On 2013/11/20 05:04:54, haraken wrote:
> This should also be moved to V8DOMConfigratoin. A follow-up CL is fine.

Added FIXME, will do in followup.

[haraken, 2013-11-20 06:35:57]

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:280: underlying value, but instead
return a new copy of the original function.
On 2013/11/20 06:11:03, Nils Barth wrote:
> On 2013/11/20 05:04:54, haraken wrote:
> > I got confused with the name "DoNotCheckSecurity", since we allow access to
> > these methods with some security checks. Would you elaborate on it?
> 
> [DoNotCheckSecurity] is only used in Location.idl and Window.idl.
> 
> The main thing it does is disable this check in the generated C++ method:
> 
> {% if method.is_check_security_for_frame %}
> if (!BindingSecurity::shouldAllowAccessToFrame(imp->frame(), exceptionState))
{
>     exceptionState.throwIfNeeded();
>     return;
> }
> {% endif %}
> 
> I.e., the following 
> [CheckSecurity=Frame] interface Foo { void bar(); };
> ...has a security check, but
> [CheckSecurity=Frame] interface Foo { [DoNotCheckSecurity] void bar(); };
> ...does not.

On the other hand, you're doing the security check in the generated code for
[DoNotCheckSecurity]. That's why the name "DoNotCheckSecurity" is confusing.

As far as I read the code, we're doing different things for attributes with
[DoNotCheckSecurity] and methods with [DoNotCheckSecurity].

(a) Attributes with [DoNotCheckSecurity] => We just don't generate the security
check. Thus the name "DoNotCheckSecurity" makes sense.

(a') For readonly attributes, we sometimes specify [DoNotCheckSecurity=Getter]
sometimes specify [DoNotCheckSecurity]. Would you unify them to
[DoNotCheckSecurity].

(b) Methods with [DoNotCheckSecurity] => We don't generate the security check
for method callbacks but generate xxxAttributeGetter and
domainSafeFunctionSetter that do the security checks. Thus the name
"DoNotCheckSecurity" doesn't make sense.

- We can rename xxxAttributeGetter to xxxDomainSafeMethodGetter
- We can rename domainSafeFunctionSetter to domainSafeMethodSetter
- We can rename [DoNotCheckSecurity] to [DoNotCheckSecurityInSameDomain]

WDYT?

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:285:
'{0}V8Internal::{0}DomainSafeFunctionSetter'.format(cpp_class_name)
On 2013/11/20 06:11:03, Nils Barth wrote:
> On 2013/11/20 05:04:54, haraken wrote:
> > We want to have SetterCallback just like the GetterCallback. The
> SetterCallback
> > should have TRACE_EVENT_SET_SAMPLING_STATE etc. Would you implement it
before
> > landing this CL? It will make this CL simpler.
> 
> I'm happy to do this, but I don't see how it would make this CL easier
> (it would make it longer b/c I'd need the callback as well);
> can I do it in a followup?
> 
> (We've identified 3 additional CLs for this one -- rename, move to
> V8DOMConfiguration, and add SetterCallback -- and this is a pretty long CL as
> is, so I'd rather do followups after landing.)

I might want to fix the naming and SetterCallback before landing this CL (not in
this CL but in preparation CLs), since this CL is a bit messy to land. You can
fix the DOMConfiguration thing in a follow-up since it will require to refactor
code widely.

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

Replies, CL refs.

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:280: underlying value, but instead
return a new copy of the original function.
On 2013/11/20 06:35:58, haraken wrote:
> As far as I read the code, we're doing different things for attributes with
> [DoNotCheckSecurity] and methods with [DoNotCheckSecurity].
> 
> (a') For readonly attributes, we sometimes specify [DoNotCheckSecurity=Getter]
> sometimes specify [DoNotCheckSecurity]. Would you unify them to
> [DoNotCheckSecurity].

Done in:
[DoNotCheckSecurity=Getter] => [DoNotCheckSecurity] for readonly attribute
https://codereview.chromium.org/77693002/

> (b) Methods with [DoNotCheckSecurity] => We don't generate the security check
> for method callbacks but generate xxxAttributeGetter and
> domainSafeFunctionSetter that do the security checks. Thus the name
> "DoNotCheckSecurity" doesn't make sense.
> 
> - We can rename xxxAttributeGetter to xxxDomainSafeMethodGetter
> - We can rename domainSafeFunctionSetter to domainSafeMethodSetter

Done in:
Rename *AttributeGetter => *DomainSafeMethodGetter and DomainSafeFunctionSetter
=> DomainSafeMethod…
https://codereview.chromium.org/77743002/

> - We can rename [DoNotCheckSecurity] to [DoNotCheckSecurityInSameDomain]
How about: [DoNotCheckSecurity] => [DoNotCheckSecurity=Domain] ?
...so we don't add another extended attribute?

https://codereview.chromium.org/77453002/diff/30001/Source/bindings/templates...
Source/bindings/templates/interface.cpp:285:
'{0}V8Internal::{0}DomainSafeFunctionSetter'.format(cpp_class_name)
On 2013/11/20 06:35:58, haraken wrote:
> I might want to fix the naming and SetterCallback before landing this CL (not
in
> this CL but in preparation CLs), since this CL is a bit messy to land.

Prep CLs will certainly make the history a bit cleaner (^.-)v

[haraken, 2013-11-20 07:47:27]

>> - We can rename [DoNotCheckSecurity] to [DoNotCheckSecurityInSameDomain]
> How about: [DoNotCheckSecurity] => [DoNotCheckSecurity=Domain] ?
> ...so we don't add another extended attribute?

First of all, I'm confused about having [DoNotCheckSecurity=Getter|Setter]. How
does it make sense to have the security check only for getter/setter? More
confusing thing is:

- We have [DoNotCheckSecurity=Setter] for Location.href, but we don't generate
the security checks in href's getter as well. Why?
- We can drop [DoNotCheckSecurity=Getter] from window.opener, since the setter
is written in custom.

Would you mind checking if we can drop [DoNotCheckSecurity=Getter|Setter] ?

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

On 2013/11/20 07:47:27, haraken wrote:
> First of all, I'm confused about having [DoNotCheckSecurity=Getter|Setter].
> How does it make sense to have the security check only for getter/setter?

I don't know. There's only one use of each.
There may be a subtle reason, or it may be cruft.

> More confusing thing is:

In fact, the only 2 cases:

> - We have [DoNotCheckSecurity=Setter] for Location.href, but we don't generate
> the security checks in href's getter as well. Why?

You mean we *do* generate the security checks for the getter, but not for the
setter,
which seems weird b/c presumably setting is sensitive, right?

Presumably we should just have security checks for both.

The Blink code is a very simple "return/set URL", only checking frame existence:
https://code.google.com/p/chromium/codesearch#chromium/src/third_party/WebKit...

> - We can drop [DoNotCheckSecurity=Getter] from window.opener, since the setter
> is written in custom.

This change does change the access control list in the attribute configuration,
in case that makes a difference.
(We check readonly, but we can't tell what should happen for [Custom].)

> Would you mind checking if we can drop [DoNotCheckSecurity=Getter|Setter] ?

Test CL posted at:
Test CL: [DoNotCheckSecurity=Getter|Setter] => [DoNotCheckSecurity]
https://codereview.chromium.org/77723003/

[Nils Barth (inactive), 2013-11-21 01:38:29]

Now that prep CLs are done, updated to include fixes/renames.
How does it look?

[Nils Barth (inactive), 2013-11-21 09:27:22]

On 2013/11/21 01:38:29, Nils Barth wrote:
> Now that prep CLs are done, updated to include fixes/renames.
> How does it look?

*ping*

[haraken, 2013-11-21 09:35:39]

On 2013/11/21 09:27:22, Nils Barth wrote:
> On 2013/11/21 01:38:29, Nils Barth wrote:
> > Now that prep CLs are done, updated to include fixes/renames.
> > How does it look?
> 
> *ping*

sorry, I'm overloaded with other things today. Let me take a look in hours :)

[haraken, 2013-11-21 10:13:05]

LGTM

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/scripts/...
File Source/bindings/scripts/unstable/v8_interface.py (right):

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_interface.py:112:
'has_domain_safe_method_setter': any(

has_domain_safe_method_setter => has_origin_safe_method_setter

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:101: {% block domain_safe_method_setter
%}

domain_safe_method_setter => origin_safe_method_setter

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:109: ExceptionState
exceptionState(info.Holder(), info.GetIsolate());

Nit: You want to avoid calling info.GetIsolate() three times.

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:287: changed and then accessed on a
different domain, we do not return the

domain => origin

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:290: {% set callback_name =
'%sV8Internal::%sOriginSafeMethodGetterCallback%s' %

callback_name => getter_callback

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:292: {% set setter =

setter => setter_callback

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
File Source/bindings/templates/interface_base.cpp (right):

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface_base.cpp:110:
{{domain_safe_method_getter(method, world_suffix)}}

domain_safe_method_getter => origin_safe_method_getter

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/methods.cpp:227: V8PerIsolateData* data =
V8PerIsolateData::from(info.GetIsolate());

Nit: We want to avoid calling info.GetIsolate() a lot of times.

[Nils Barth (inactive), 2013-11-22 03:17:52]

Revised, landing.
2 questions:
1. Should I add local |isolate| variables?
2. Naming question:
Should we get rid of '_name' suffix in templates,
since these are all strings anyway (so "name" is redundant)?

Detailed comments below.

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/scripts/...
File Source/bindings/scripts/unstable/v8_interface.py (right):

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/scripts/...
Source/bindings/scripts/unstable/v8_interface.py:112:
'has_domain_safe_method_setter': any(
On 2013/11/21 10:13:05, haraken wrote:
> has_domain_safe_method_setter => has_origin_safe_method_setter

fixed passim

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:109: ExceptionState
exceptionState(info.Holder(), info.GetIsolate());
On 2013/11/21 10:13:05, haraken wrote:
> 
> Nit: You want to avoid calling info.GetIsolate() three times.

Can I just add:
v8::Isolate isolate = info.GetIsolate();
...and use |isolate|?
(Is so, I'll do in followup.)

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:290: {% set callback_name =
'%sV8Internal::%sOriginSafeMethodGetterCallback%s' %
On 2013/11/21 10:13:05, haraken wrote:
> callback_name => getter_callback

Done.
Also renamed callback_name => callback throughout.

Question:
There are 2 other uses of _name which seem unnecessary:
runtime_enabled_function_name / per_context_enabled_function_name
...which is used a couple places, and is a bit long, and
cpp_class_name and v8_class_name
...which are used everywhere.
(It's clearly a name, since it's in the template!)

Should I remove _name (in a followup CL)?

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:292: {% set setter =
On 2013/11/21 10:13:05, haraken wrote:
> 
> setter => setter_callback

Done.

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
File Source/bindings/templates/methods.cpp (right):

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/methods.cpp:227: V8PerIsolateData* data =
V8PerIsolateData::from(info.GetIsolate());
On 2013/11/21 10:13:05, haraken wrote:
> Nit: We want to avoid calling info.GetIsolate() a lot of times.

As above: should I just add a local |isolate| variable?

[commit-bot: I haz the power, 2013-11-22 03:18:21]

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

[commit-bot: I haz the power, 2013-11-22 04:20:49]

Change committed as 162510

[haraken, 2013-11-22 05:53:25]

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:109: ExceptionState
exceptionState(info.Holder(), info.GetIsolate());
On 2013/11/22 03:17:52, Nils Barth wrote:
> On 2013/11/21 10:13:05, haraken wrote:
> > 
> > Nit: You want to avoid calling info.GetIsolate() three times.
> 
> Can I just add:
> v8::Isolate isolate = info.GetIsolate();
> ...and use |isolate|?
> (Is so, I'll do in followup.)

Yes. We're doing it in a lot of places.

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:290: {% set callback_name =
'%sV8Internal::%sOriginSafeMethodGetterCallback%s' %
On 2013/11/22 03:17:52, Nils Barth wrote:
> On 2013/11/21 10:13:05, haraken wrote:
> > callback_name => getter_callback
> 
> Done.
> Also renamed callback_name => callback throughout.
> 
> Question:
> There are 2 other uses of _name which seem unnecessary:
> runtime_enabled_function_name / per_context_enabled_function_name
> ...which is used a couple places, and is a bit long, and
> cpp_class_name and v8_class_name
> ...which are used everywhere.
> (It's clearly a name, since it's in the template!)
> 
> Should I remove _name (in a followup CL)?

Either is fine with me as long as it's consistent through the compiler :)

[Nils Barth (inactive), 2013-11-22 06:04:05]

Thanks for replies, will do in followups!

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
File Source/bindings/templates/interface.cpp (right):

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:109: ExceptionState
exceptionState(info.Holder(), info.GetIsolate());
On 2013/11/22 05:53:26, haraken wrote:
> On 2013/11/22 03:17:52, Nils Barth wrote:
> > Can I just add:
> > v8::Isolate isolate = info.GetIsolate();
> > ...and use |isolate|?
> > (Is so, I'll do in followup.)
> 
> Yes. We're doing it in a lot of places.

Got it; will throughout in one followup.

https://codereview.chromium.org/77453002/diff/170001/Source/bindings/template...
Source/bindings/templates/interface.cpp:290: {% set callback_name =
'%sV8Internal::%sOriginSafeMethodGetterCallback%s' %
On 2013/11/22 05:53:26, haraken wrote:
> On 2013/11/22 03:17:52, Nils Barth wrote:
> > Should I remove _name (in a followup CL)?
> 
> Either is fine with me as long as it's consistent through the compiler :)

Got it; will do in followup.