Bug 112783 – CSP 1.1: Fire a SecurityPolicyViolationEvent when violations occur.

• Status: RESOLVED
• Resolution: FIXED
• Component: WebCore Misc.

[Mike West]

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.

[Mike West]

Created attachment 194003 [details]
Patch

[Build Bot]

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

[Early Warning System Bot]

Comment on attachment 194003 [details]
Patch

Attachment 194003 [details] did not pass qt-ews (qt):
Output: http://webkit-commit-queue.appspot.com/results/17178585

[Mike West]

Created attachment 194005 [details]
Patch

[Mike West]

Bots are happy. Would you mind taking a look, Adam?

[Adam Barth]

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...

[Mike West]

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. :)

[Mike West]

Created attachment 194328 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 194328 [details]
Patch for landing

Clearing flags on attachment: 194328

Committed r146520: <http://trac.webkit.org/changeset/146520>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Mike West]

(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.