Bug 54381 - Sketch script-src for Content Security Policy
Summary: Sketch script-src for Content Security Policy
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Other OS X 10.5
: P2 Normal
Assignee: Adam Barth
URL:
Keywords:
Depends on:
Blocks: 53572
  Show dependency treegraph
 
Reported: 2011-02-14 02:08 PST by Adam Barth
Modified: 2011-02-15 08:48 PST (History)
3 users (show)

See Also:


Attachments
work-in-progress (needs test) (4.44 KB, patch)
2011-02-14 02:09 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (10.86 KB, patch)
2011-02-14 17:40 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (12.08 KB, patch)
2011-02-14 18:19 PST, Adam Barth
no flags Details | Formatted Diff | Diff
Patch (12.22 KB, patch)
2011-02-14 18:50 PST, Adam Barth
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Adam Barth 2011-02-14 02:08:32 PST
Sketch script-src for Content Security Policy
Comment 1 Adam Barth 2011-02-14 02:09:30 PST
Created attachment 82299 [details]
work-in-progress (needs test)
Comment 2 Eric Seidel (no email) 2011-02-14 02:16:36 PST
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?
Comment 3 Adam Barth 2011-02-14 02:26:47 PST
(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.
Comment 4 Adam Barth 2011-02-14 17:40:33 PST
Created attachment 82395 [details]
Patch
Comment 5 Eric Seidel (no email) 2011-02-14 18:00:38 PST
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".
Comment 6 Adam Barth 2011-02-14 18:19:29 PST
Created attachment 82402 [details]
Patch
Comment 7 Eric Seidel (no email) 2011-02-14 18:27:51 PST
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.
Comment 8 Eric Seidel (no email) 2011-02-14 18:29:41 PST
Comment on attachment 82402 [details]
Patch

Please consider my alternate design when landing.
Comment 9 Adam Barth 2011-02-14 18:50:43 PST
Created attachment 82403 [details]
Patch
Comment 10 Eric Seidel (no email) 2011-02-14 18:54:23 PST
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.
Comment 11 Adam Barth 2011-02-14 18:57:25 PST
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.
Comment 12 WebKit Commit Bot 2011-02-15 08:47:57 PST
Comment on attachment 82403 [details]
Patch

Clearing flags on attachment: 82403

Committed r78569: <http://trac.webkit.org/changeset/78569>
Comment 13 WebKit Commit Bot 2011-02-15 08:48:01 PST
All reviewed patches have been landed.  Closing bug.