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

Source/bindings/templates/interface.cpp

 {% extends 'interface_base.cpp' %}
 
 
 {##############################################################################}
 {% macro attribute_configuration(attribute) %}
 {% set getter_callback_name =
        '%sV8Internal::%sAttributeGetterCallback' %
             (interface_name, attribute.name)
        if not attribute.constructor_type else
        '{0}V8Internal::{0}ConstructorGetter'.format(interface_name) %}
@@ skipping 80 matching lines @@
 {
     {{cpp_class_name}}* imp =  {{v8_class_name}}::toNative(host);
     return BindingSecurity::shouldAllowAccessToFrame(imp->frame(), DoNotReportSecurityError);
 }
 
 {% endif %}
 {% endblock %}
 
 
 {##############################################################################}
+{% 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 %}
 static const V8DOMConfiguration::AttributeConfiguration {{v8_class_name}}Attributes[] = {
     {% for attribute in attributes
        if not (attribute.is_expose_js_accessors or
                attribute.is_static or
                attribute.runtime_enabled_function_name or
                attribute.per_context_enabled_function_name) %}
     {% filter conditional(attribute.conditional_string) %}
@@ skipping 86 matching lines @@
 
     // Custom Signature '{{method.name}}'
     const int {{method.name}}Argc = {{method.arguments | length}};
     v8::Handle<v8::FunctionTemplate> {{method.name}}Argv[{{method.name}}Argc] = { {{method.custom_signature}} };
     v8::Handle<v8::Signature> {{method.name}}Signature = v8::Signature::New(functionTemplate, {{method.name}}Argc, {{method.name}}Argv);
     {% endif %}
     {# install_custom_signature #}
     {% 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) %}
         {{install_custom_signature(method, 'ForMainWorld')}}
         {% endfilter %}
     } else {
         {% filter runtime_enabled(method.runtime_enabled_function_name) %}
         {{install_custom_signature(method)}}
         {% endfilter %}
     }
     {% else %}
     {% filter runtime_enabled(method.runtime_enabled_function_name) %}
     {{install_custom_signature(method)}}
     {% endfilter %}
     {% endif %}
+    {% endif %}{# is_do_not_check_security #}
     {% endfilter %}
     {% endif %}{# install_custom_signature #}
     {% endfor %}
     {% for attribute in attributes if attribute.is_static %}
     {% set getter_callback_name = '%sV8Internal::%sAttributeGetterCallback' %
            (interface_name, attribute.name) %}
     functionTemplate->SetNativeDataProperty(v8::String::NewSymbol("{{attribute.name}}"), {{getter_callback_name}}, {{attribute.setter_callback_name}}, v8::External::New(isolate, 0), static_cast<v8::PropertyAttribute>(v8::None), v8::Handle<v8::AccessorSignature>(), static_cast<v8::AccessControl>(v8::DEFAULT));
     {% endfor %}
     {% if constants %}
     {{install_constants() | indent}}
     {% endif %}
 
     // Custom toString template
     functionTemplate->Set(v8::String::NewSymbol("toString"), V8PerIsolateData::current()->toStringTemplate());
     return functionTemplate;
 }
 
 {% endblock %}
 
 
 {######################################}
+{% 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' %
                        (interface_name, method.name, world_suffix) %}
 {% set property_attribute = 'static_cast<v8::PropertyAttribute>(%s)' %
                             ' | '.join(method.property_attributes) %}
 {{method.function_template}}->Set(v8::String::NewSymbol("{{method.name}}"), v8::FunctionTemplate::New({{callback_name}}, v8Undefined(), {{method.signature}}, {{method.number_of_required_or_variadic_arguments}}){% if method.property_attributes %}, {{property_attribute}}{% endif %});
 {%- endmacro %}
 
 
@@ skipping 137 matching lines @@
     V8DOMWrapper::associateObjectWithWrapper<{{v8_class_name}}>(impl, &wrapperTypeInfo, wrapper, isolate, {{wrapper_configuration}});
     return wrapper;
 }
 
 void {{v8_class_name}}::derefObject(void* object)
 {
     fromInternalPointer(object)->deref();
 }
 
 {% endblock %}