Bug 112783 - CSP 1.1: Fire a SecurityPolicyViolationEvent when violations occur.
Summary: CSP 1.1: Fire a SecurityPolicyViolationEvent when violations occur.
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebCore Misc. (show other bugs)
Version: 528+ (Nightly build)
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Mike West
URL:
Keywords:
Depends on:
Blocks: 85558
  Show dependency treegraph
 
Reported: 2013-03-20 01:57 PDT by Mike West
Modified: 2013-03-21 15:03 PDT (History)
6 users (show)

See Also:


Attachments
Patch (9.40 KB, patch)
2013-03-20 02:05 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch (9.43 KB, patch)
2013-03-20 02:24 PDT, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (9.63 KB, patch)
2013-03-21 14:00 PDT, Mike West
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Mike West 2013-03-20 01:57:20 PDT
Following up on bug 112681, we should wire up the newly created event so that it actually fires off on the Document when violations occur.
Comment 1 Mike West 2013-03-20 02:05:59 PDT
Created attachment 194003 [details]
Patch
Comment 2 Build Bot 2013-03-20 02:10:06 PDT
Comment on attachment 194003 [details]
Patch

Attachment 194003 [details] did not pass mac-wk2-ews (mac-wk2):
Output: http://webkit-commit-queue.appspot.com/results/17191579
Comment 3 Early Warning System Bot 2013-03-20 02:14:35 PDT
Comment on attachment 194003 [details]
Patch

Attachment 194003 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17178585
Comment 4 Mike West 2013-03-20 02:24:25 PDT
Created attachment 194005 [details]
Patch
Comment 5 Mike West 2013-03-21 01:26:29 PDT
Bots are happy. Would you mind taking a look, Adam?
Comment 6 Adam Barth 2013-03-21 09:57:10 PDT
Comment on attachment 194005 [details]
Patch

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

> Source/WebCore/page/ContentSecurityPolicy.cpp:1688
> +    if (stack) {

prefer early return

> Source/WebCore/page/ContentSecurityPolicy.cpp:1692
> +            KURL source = KURL(KURL(), callFrame.sourceURL());

This should probably use the ParsedURLString version of the constructor.

> Source/WebCore/page/ContentSecurityPolicy.cpp:1718
> +        document->enqueueDocumentEvent(SecurityPolicyViolationEvent::create(eventNames().securitypolicyviolationEvent, init));

Do, we always fire the event at the document?  I guess that's ok.  It seems like it would be better to fire the event at the element that caused the violation, but I guess there isn't always an element...
Comment 7 Mike West 2013-03-21 11:27:11 PDT
Thanks, Adam!

(In reply to comment #6)
> (From update of attachment 194005 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=194005&action=review
> 
> > Source/WebCore/page/ContentSecurityPolicy.cpp:1688
> > +    if (stack) {
> 
> prefer early return

Good call. I'll fix this and CQ the patch in the morning.

> > Source/WebCore/page/ContentSecurityPolicy.cpp:1692
> > +            KURL source = KURL(KURL(), callFrame.sourceURL());
> 
> This should probably use the ParsedURLString version of the constructor.

I'll fix this in a separate patch, as I can change the 'report-uri' version I copy/pasted at the same time.

> > Source/WebCore/page/ContentSecurityPolicy.cpp:1718
> > +        document->enqueueDocumentEvent(SecurityPolicyViolationEvent::create(eventNames().securitypolicyviolationEvent, init));
> 
> Do, we always fire the event at the document?  I guess that's ok.  It seems like it would be better to fire the event at the element that caused the violation, but I guess there isn't always an element...

As a first pass, I think targeting the document makes sense. It has the advantage of capturing all events related to a page, regardless of whether they occur on attached elements or XHR or whatever.

When it's relevant, though, I do think it'd be a good idea to either target the elements directly, or include a reference to the element in the event. *shrug* It's a good discussion to have on the list. :)
Comment 8 Mike West 2013-03-21 14:00:41 PDT
Created attachment 194328 [details]
Patch for landing
Comment 9 WebKit Review Bot 2013-03-21 14:28:01 PDT
Comment on attachment 194328 [details]
Patch for landing

Clearing flags on attachment: 194328

Committed r146520: <http://trac.webkit.org/changeset/146520>
Comment 10 WebKit Review Bot 2013-03-21 14:28:05 PDT
All reviewed patches have been landed.  Closing bug.
Comment 11 Mike West 2013-03-21 15:03:59 PDT
(In reply to comment #7)
> > > Source/WebCore/page/ContentSecurityPolicy.cpp:1692
> > > +            KURL source = KURL(KURL(), callFrame.sourceURL());
> > 
> > This should probably use the ParsedURLString version of the constructor.
> 
> I'll fix this in a separate patch, as I can change the 'report-uri' version I copy/pasted at the same time.

Split out into bug 112965; thanks again for pointing it out.