[webkit-reviews] review denied: [Bug 97398] Script run from an isolated world should bypass a page's CSP. : [Attachment 169401] Patch

[bugzilla-daemon at webkit.org]

Adam Barth <abarth at webkit.org> has denied Mike West <mkwst at chromium.org>'s
request for review:
Bug 97398: Script run from an isolated world should bypass a page's CSP.
https://bugs.webkit.org/show_bug.cgi?id=97398

Attachment 169401: Patch
https://bugs.webkit.org/attachment.cgi?id=169401&action=review

------- Additional Comments from Adam Barth <abarth at webkit.org>
View in context: https://bugs.webkit.org/attachment.cgi?id=169401&action=review


This looks great.  I'd keep the API as a String, but I would use a bool in
WebCore to represent this state.  We can always elaborate the WebCore state
if/when we want to support a full CSP policy.

Sorry for the delay in reviewing this patch.  For some reason I was hesitating
to review it, but now I see that my hesitation was misplaced.

> Source/WebCore/bindings/v8/DOMWrapperWorld.cpp:152
> +typedef HashMap<int, String> IsolatedWorldContentSecurityPolicyMap;
> +static IsolatedWorldContentSecurityPolicyMap&
isolatedWorldContentSecurityPolicies()
> +{
> +    ASSERT(isMainThread());
> +    DEFINE_STATIC_LOCAL(IsolatedWorldContentSecurityPolicyMap, map, ());
> +    return map;
> +}

I really dislike these maps keyed on the world ID.  I know you're just
following the pattern here, but this should really just be a bool on
DOMWrapperWord.

> Source/WebCore/bindings/v8/ScriptController.h:113
> +    // Returns true if the current world is isolated, and has its own
Content
> +    // Security Policy. In this case, the policy of the main world should be

> +    // ignored when evaluating resources injected into the DOM.
> +    bool shouldBypassMainWorldContentSecurityPolicy();

This API should take a DOMWrapperWorld* argument and the caller should call it
with currentWorld().  You can look at how the other functions that take
DOMWrapperWorld* arguments work in JSC.  We'd like to switch over to using an
approach more like JSC, but for now we do things a bit differently.  Given that
ScriptController is a shared interface with JSC, we should do things the JSC
way.

> Source/WebCore/loader/cache/CachedResourceLoader.cpp:307
> +    bool shouldBypassMainWorldContentSecurityPolicy = (document()->frame()
&&
document()->frame()->script()->shouldBypassMainWorldContentSecurityPolicy());

There's already a frame() accessor that you can use on CachedResourceLoader