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
Created attachment 145490 [details] Patch
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. :)
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.
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
Created attachment 145492 [details] Patch
(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.
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
Created attachment 145537 [details] Patch
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? :)
Comment on attachment 145537 [details] Patch Clearing flags on attachment: 145537 Committed r119380: <http://trac.webkit.org/changeset/119380>
All reviewed patches have been landed. Closing bug.
> 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. ;)