[webkit-reviews] review denied: [Bug 94331] JSC should throw a more descriptive exception when blocking 'eval' via CSP. : [Attachment 159125] Resetting tests.

[bugzilla-daemon at webkit.org]

Geoffrey Garen <ggaren at apple.com> has denied Mike West <mkwst at chromium.org>'s
request for review:
Bug 94331: JSC should throw a more descriptive exception when blocking 'eval'
via CSP.
https://bugs.webkit.org/show_bug.cgi?id=94331

Attachment 159125: Resetting tests.
https://bugs.webkit.org/attachment.cgi?id=159125&action=review

------- Additional Comments from Geoffrey Garen <ggaren at apple.com>
View in context: https://bugs.webkit.org/attachment.cgi?id=159125&action=review


Patch looks generally good. I have some small style comments, and a question
about security policy policy.

> Source/WebCore/page/ContentSecurityPolicy.cpp:807
> +	   String message = makeString("Refused to evaluate a string as
JavaScript because it violates the following Content Security Policy directive:
\"", directives->operativeDirective(directives->m_scriptSrc.get())->text(),
"\".\n");

The proper tense here is "violated", not "violates", since "Refused" is in the
past tense.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1353
> +String ContentSecurityPolicy::evalDisabledErrorMessage() const
> +{
> +    for (size_t i = 0; i < m_policies.size(); ++i) {

This is a linear search each time we make a new document. Is m_policies
guaranteed to be very small?

Why did you choose the message from the first non-eval policy? What if there
are more non-eval policies?

> Source/WebCore/page/ContentSecurityPolicy.cpp:1354
> +	   if (!m_policies[i].get()->allowEval(0, SuppressReport))

No need for .get() on a smart pointer: operator-> does the right thing.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1355
> +	       return m_policies[i].get()->evalDisabledErrorMessage();

Ditto.