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

Source/bindings/templates/interface.cpp

Index: Source/bindings/templates/interface.cpp
diff --git a/Source/bindings/templates/interface.cpp b/Source/bindings/templates/interface.cpp
index c398adde5d3e88bd9c1c4b3a4379337a24753b3e..ded9555eac139980b0dff99590fbbe7cfa494054 100644
--- a/Source/bindings/templates/interface.cpp
+++ b/Source/bindings/templates/interface.cpp
@@ -98,6 +98,28 @@ bool namedSecurityCheck(v8::Local<v8::Object> host, v8::Local<v8::Value> key, v8
 
 
 {##############################################################################}
+{% block domain_safe_method_setter %}

[haraken, 2013/11/20 05:04:54]

domain_safe_method_setter => attribute_setter_with_security_check

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

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

+{% if has_domain_safe_method_setter %}

[haraken, 2013/11/20 05:04:54]

has_domain_safe_method_setter => has_attribute_setter_with_security_check

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

Per above, in followup.

+static void {{cpp_class_name}}DomainSafeFunctionSetter(v8::Local<v8::String> name, v8::Local<v8::Value> jsValue, const v8::PropertyCallbackInfo<void>& info)

[haraken, 2013/11/20 05:04:54]

DomainSafeFunctionSetter => AttributeSetterWithSecurityCheck

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

Per above, in followup.

+{
+    v8::Handle<v8::Object> holder = info.This()->FindInstanceInPrototypeChain({{v8_class_name}}::GetTemplate(info.GetIsolate(), worldType(info.GetIsolate())));
+    if (holder.IsEmpty())
+        return;
+    {{cpp_class_name}}* imp = {{v8_class_name}}::toNative(holder);
+    ExceptionState exceptionState(info.Holder(), info.GetIsolate());
+    if (!BindingSecurity::shouldAllowAccessToFrame(imp->frame(), exceptionState)) {
+        exceptionState.throwIfNeeded();
+        return;
+    }
+
+    info.This()->SetHiddenValue(name, jsValue);
+}
+
+{% endif %}
+{% endblock %}
+
+
+{##############################################################################}
 {% block class_attributes %}
 {# FIXME: rename to install_attributes and put into configure_class_template #}
 {% if attributes %}
@@ -204,6 +226,17 @@ static v8::Handle<v8::FunctionTemplate> Configure{{v8_class_name}}Template(v8::H
     {% if not method.overload_index or method.overload_index == 1 %}
     {# For overloaded methods, only generate one accessor #}
     {% filter conditional(method.conditional_string) %}
+    {% if method.is_do_not_check_security %}
+    {% if method.is_per_world_bindings %}
+    if (currentWorldType == MainWorld) {
+        {{install_do_not_check_security_signature(method, 'ForMainWorld')}}
+    } else {
+        {{install_do_not_check_security_signature(method)}}
+    }
+    {% else %}
+    {{install_do_not_check_security_signature(method)}}
+    {% endif %}
+    {% else %}{# is_do_not_check_security #}
     {% if method.is_per_world_bindings %}
     if (currentWorldType == MainWorld) {
         {% filter runtime_enabled(method.runtime_enabled_function_name) %}
@@ -219,6 +252,7 @@ static v8::Handle<v8::FunctionTemplate> Configure{{v8_class_name}}Template(v8::H
     {{install_custom_signature(method)}}
     {% endfilter %}
     {% endif %}
+    {% endif %}{# is_do_not_check_security #}
     {% endfilter %}
     {% endif %}{# install_custom_signature #}
     {% endfor %}
@@ -240,6 +274,24 @@ static v8::Handle<v8::FunctionTemplate> Configure{{v8_class_name}}Template(v8::H
 
 
 {######################################}
+{% macro install_do_not_check_security_signature(method, world_suffix) %}
+{# Methods that are [DoNotCheckSecurity] are always readable, but if they are
+   changed and then accessed on a different domain, we do not return the
+   underlying value, but instead return a new copy of the original function.

[haraken, 2013/11/20 05:04:54]

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

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

[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.

[haraken, 2013/11/20 06:35:58]

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?

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

Done in:
[DoNotCheckSecurity=Getter] => [DoNotCheckSecurity] for readonly attribute
https://codereview.chromium.org/77693002/
Done in:
Rename *AttributeGetter => *DomainSafeMethodGetter and DomainSafeFunctionSetter
=> DomainSafeMethod…
https://codereview.chromium.org/77743002/
How about: [DoNotCheckSecurity] => [DoNotCheckSecurity=Domain] ?
...so we don't add another extended attribute?

+   This is achieved by storing the changed value as a hidden property. #}
+{% set callback_name = '%sV8Internal::%sAttributeGetterCallback%s' %
+                       (cpp_class_name, method.name, world_suffix) %}
+{% set setter =
+    '{0}V8Internal::{0}DomainSafeFunctionSetter'.format(cpp_class_name)

[haraken, 2013/11/20 05:04:54]

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.

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

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.)

[haraken, 2013/11/20 06:35:58]

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]

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

+    if not method.is_read_only else '0' %}
+{% set property_attribute =
+    'static_cast<v8::PropertyAttribute>(%s)' %
+    ' | '.join(method.property_attributes or ['v8::DontDelete']) %}

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

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!

+{{method.function_template}}->SetAccessor(v8::String::NewSymbol("{{method.name}}"), {{callback_name}}, {{setter}}, v8Undefined(), v8::ALL_CAN_READ, {{property_attribute}});

[haraken, 2013/11/20 05:04:54]

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

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

Added FIXME, will do in followup.

+{%- endmacro %}
+
+
+{######################################}
 {% macro install_custom_signature(method, world_suffix) %}
 {# FIXME: move to V8DOMConfiguration::installDOMCallbacksWithCustomSignature #}
 {% set callback_name = '%sV8Internal::%sMethodCallback%s' %