Bug 54381 – Sketch script-src for Content Security Policy

• Component: New Bugs
• Status: RESOLVED FIXED

[Adam Barth]

Sketch script-src for Content Security Policy

[Adam Barth]

Created attachment 82299 [details]
work-in-progress (needs test)

[Eric Seidel (no email)]

Comment on attachment 82299 [details]
work-in-progress (needs test)

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

I think i'm too tired/sick to review this for real.  But I look forward to the tested patch.

> Source/WebCore/page/ContentSecurityPolicy.cpp:151
> +        m_scriptSrc = adoptPtr(new ScriptSrcDirective(value));

Should we have a default case?  Like logging to the console?

[Adam Barth]

(In reply to comment #2)
> (From update of attachment 82299 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=82299&action=review
>
> > Source/WebCore/page/ContentSecurityPolicy.cpp:151
> > +        m_scriptSrc = adoptPtr(new ScriptSrcDirective(value));
> 
> Should we have a default case?  Like logging to the console?

Maybe...  I haven't really thought about what to do for unsupported directives.  My plan was just to ignore them.

[Adam Barth]

Created attachment 82395 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 82395 [details]
Patch

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:130
> +        emitDirective(String(name), String(value));
> +        name.clear();
> +        value.clear();

This still feels strange.  Seems you should have an emitAndClear function to wrap this copy/paste.

> Source/WebCore/page/ContentSecurityPolicy.h:49
>      bool m_isEnabled;

This name doesn't make much sense.  Seems you could have parsed already and not have "enabled".

[Adam Barth]

Created attachment 82402 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 82402 [details]
Patch

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:83
> +        pos = parseDirective(pos, end);

I think I would have designed this to return the name and value from this function and then call emitDirective from within this loop.  That would get rid of the goto, and might make it more clear who is doing what.

I think this is OK though.  It's not immediately clear from reading that calling "parseDirective" will end up calling emitDirective.

Actually, in what I proposed then the Vectors are managed by the outer loop and they auto-clear then the loop loops.

[Eric Seidel (no email)]

Comment on attachment 82402 [details]
Patch

Please consider my alternate design when landing.

[Adam Barth]

Created attachment 82403 [details]
Patch

[Eric Seidel (no email)]

Comment on attachment 82403 [details]
Patch

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:87
> +        parseDirective(pos, end, name, value);
> +        if (name.isEmpty())
> +            continue;

I would have probably had this return true/false for success.  But this is good too.

[Adam Barth]

Comment on attachment 82403 [details]
Patch

Yeah, it's a bit philosophical.  Parsing can never "fail" per se.  It's just that unnamed directives don't have any meaning, e.g., ";;;;" parses fine by has a bunch of unnamed directives.

[WebKit Commit Bot]

Comment on attachment 82403 [details]
Patch

Clearing flags on attachment: 82403

Committed r78569: <http://trac.webkit.org/changeset/78569>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.