Bug 155623 - Cleanup: Remove the need to pass reporting status to ContentSecurityPolicy functions
Summary: Cleanup: Remove the need to pass reporting status to ContentSecurityPolicy fu...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2016-03-17 21:04 PDT by Daniel Bates
Modified: 2016-03-17 23:32 PDT (History)
5 users (show)

See Also:


Attachments
Patch (55.83 KB, patch)
2016-03-17 21:08 PDT, Daniel Bates
aestes: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2016-03-17 21:04:35 PDT
ScriptController::initScript() is the only function that passes ContentSecurityPolicy::ReportingStatus::SuppressReport() following the removal of the SecurityPolicy script interface in <http://trac.webkit.org/changeset/197142> (bug #154694). We can refactor the logic in ScriptController::initScript() such that it delegates to the ContentSecurityPolicy object to enable/disable JavaScript eval()/operator eval in the newly initialized world.
Comment 1 Daniel Bates 2016-03-17 21:08:35 PDT
Created attachment 274367 [details]
Patch
Comment 2 Andy Estes 2016-03-17 22:43:59 PDT
Comment on attachment 274367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274367&action=review

r=me

Do you anticipate adding more uses of SuppressReport in later patches, or is ContentSecurityPolicyDirectiveList::allowEval() the only function that will ever be passed ReportingStatus::SuppressReport? I'm wondering if you considered further simplifying this by removing the reportingStatus argument and associated branch from the other allow functions in ContentSecurityPolicyDirectiveList.

> Source/WebCore/page/csp/ContentSecurityPolicy.h:44
> +class JSDOMWindowShell;

Should be listed after DOMStringList.
Comment 3 Alex Christensen 2016-03-17 22:59:18 PDT
Comment on attachment 274367 [details]
Patch

View in context: https://bugs.webkit.org/attachment.cgi?id=274367&action=review

> Source/WebCore/page/csp/ContentSecurityPolicy.cpp:97
> +    ASSERT(windowShell.window());
> +    ASSERT(windowShell.window()->scriptExecutionContext());
> +    ASSERT(windowShell.window()->scriptExecutionContext()->contentSecurityPolicy() == this);
> +    JSDOMWindow* window = windowShell.window();

It would have less redundancy if you put the assertions after the pointer declaration and used window in your assertions.
Comment 4 Daniel Bates 2016-03-17 23:26:06 PDT
(In reply to comment #2)
> Do you anticipate adding more uses of SuppressReport in later patches

No.

> , or is ContentSecurityPolicyDirectiveList::allowEval() the only function that will
> ever be passed ReportingStatus::SuppressReport? 

It is the only function.

> I'm wondering if you considered further simplifying this by removing the
> reportingStatus argument and associated branch from the other allow functions in
> ContentSecurityPolicyDirectiveList.
> 

Yes, I plan to completely remove the ReportingStatus enum and it use throughout ContentSecurityPolicyDirectiveList. This patch is the first step.

> > Source/WebCore/page/csp/ContentSecurityPolicy.h:44
> > +class JSDOMWindowShell;
> 
> Should be listed after DOMStringList.

Will fix before landing.
Comment 5 Daniel Bates 2016-03-17 23:28:06 PDT
(In reply to comment #3)
> Comment on attachment 274367 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=274367&action=review
> 
> > Source/WebCore/page/csp/ContentSecurityPolicy.cpp:97
> > +    ASSERT(windowShell.window());
> > +    ASSERT(windowShell.window()->scriptExecutionContext());
> > +    ASSERT(windowShell.window()->scriptExecutionContext()->contentSecurityPolicy() == this);
> > +    JSDOMWindow* window = windowShell.window();
> 
> It would have less redundancy if you put the assertions after the pointer
> declaration and used window in your assertions.

Will fix before landing.
Comment 6 Daniel Bates 2016-03-17 23:32:33 PDT
Committed r198379: <http://trac.webkit.org/changeset/198379>