Duplicated CSP directives should throw a more accurate console error.

• Component: WebCore Misc.
• Status: RESOLVED FIXED

[Mike West]

Accidentally duplicating a CSP directive (by, for example, typoing `style-src` into `script-src`) leads to an error that reads "Unrecognized Content-Security-Policy directive 'script-src'." If, like me, you are slow witted, it might take you a half hour or so to figure out why things aren't working the way you expect them to, especially if the policy you expected to see active is in the second of the two. :)

I'd like to add an error for this condition, something along the lines of "Ignoring duplicate Content-Security-Policy directive: `script-src`."

WDYT?

Seems reasonable

[Mike West]

Created attachment 145490 [details]
Patch

[Mike West]

Based on sifting through http://www.webkit.org/coding/RefPtr.html, I'm fairly certain I'm doing something wrong by passing the `OwnPtr<CSPDirective>` around without converting to `PassOwnPtr`. It's not clear to me how I ought to do it correctly in this scenario. Feedback would be appreciated. :)

[Adam Barth]

Comment on attachment 145490 [details]
Patch

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

This looks fine.  A bunch of style nits below.

> Source/WebCore/page/ContentSecurityPolicy.cpp:520
> -    PassOwnPtr<CSPDirective> createCSPDirective(const String& name, const String& value);
> +    void setCSPDirective(OwnPtr<CSPDirective>&, const String& name, const String& value);

This isn't a common pattern in WebKit, but it's fine.  In this case, the OwnPtr is an in/out parameter for this function.  We usually put output parameters at the end of the argument list.

> Source/WebCore/page/ContentSecurityPolicy.cpp:830
> +      logDuplicateDirective(name);
> +      return;

Four-space indent.

> Source/WebCore/page/ContentSecurityPolicy.cpp:852
> +    else

Prefer early return.

> Source/WebCore/page/ContentSecurityPolicy.cpp:859
> +      logDuplicateDirective(name);

Four-space indent.

> Source/WebCore/page/ContentSecurityPolicy.cpp:860
> +    else {

Prefer early return.

[Adam Barth]

PassOwnPtr is useful when you want to pass around ownership of an object (e.g., as a return value of a function).  Here you're passing around an l-value into which you're going to store the result.  Another idiom you could use is the following:

m_scriptSrc = createDirective(m_scriptSrc.get(), ...,  ...)

where the m_scriptSrc argument would be purely an in-parameter and the result would be returned as a return value.  What you've done instead is to have an in/out parameter, which can be trickier to read but removes the need to repeat m_scriptSrc twice (which can lead to an error if the two aren't the same).

Given that I'd expect folks to copy/paste this code when adding new directives, using the in/out parameter makes sense so we don't have the error case of not editing both m_blah values when copy/pasting.

(In reply to comment #5)
> PassOwnPtr is useful when you want to pass around ownership of an object (e.g., as a return value of a function).  Here you're passing around an l-value into which you're going to store the result.  Another idiom you could use is the following:
> 
> m_scriptSrc = createDirective(m_scriptSrc.get(), ...,  ...)
> 
> where the m_scriptSrc argument would be purely an in-parameter and the result would be returned as a return value.  What you've done instead is to have an in/out parameter, which can be trickier to read but removes the need to repeat m_scriptSrc twice (which can lead to an error if the two aren't the same).
> 
> Given that I'd expect folks to copy/paste this code when adding new directives, using the in/out parameter makes sense so we don't have the error case of not editing both m_blah values when copy/pasting.

an alternative would be to return nullptr when you don't create an object to return

[Mike West]

Created attachment 145492 [details]
Patch

[Mike West]

(In reply to comment #5)
> Given that I'd expect folks to copy/paste this code when adding new directives, using the in/out parameter makes sense so we don't have the error case of not editing both m_blah values when copy/pasting.

Makes sense. That was certainly the idiom I was going for, but I'd originally tried to pass a pointer to the pointer (which would have made it slightly more clear what was going on, or at least that *something* interesting was happening). That ran afoul of the style bot, which is understandable, as it was ugly. :)

I've addressed the style nits, and uploaded a new patch. Thanks for the detailed feedback.

[Adam Barth]

Comment on attachment 145492 [details]
Patch

Thanks.  Let's give Jochen a chance to provide feedback before landing this patch.

Comment on attachment 145492 [details]
Patch

To prove that I also read the patch, here's a nit

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

> LayoutTests/http/tests/security/contentSecurityPolicy/duplicate-directive.html:3
> +  <head>

can you please also indent 4 spaces in html/js

[Mike West]

Created attachment 145537 [details]
Patch

[Mike West]

Comment on attachment 145537 [details]
Patch

Thanks for the nits. What are the odds that I can convince the entire webkit community to use two spaces instead of four? :)

[WebKit Review Bot]

Comment on attachment 145537 [details]
Patch

Clearing flags on attachment: 145537

Committed r119380: <http://trac.webkit.org/changeset/119380>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Adam Barth]

> What are the odds that I can convince the entire webkit community to use two spaces instead of four? :)

Similar to the odds of convincing the community to abandon ChangeLogs.  ;)