Summary: | CSP 1.1: Fire a SecurityPolicyViolationEvent when violations occur. | ||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike West <mkwst> | ||||||||
Component: | WebCore Misc. | Assignee: | Mike West <mkwst> | ||||||||
Status: | RESOLVED FIXED | ||||||||||
Severity: | Normal | CC: | abarth, buildbot, mkwst+watchlist, rniwa, webkit-ews, webkit.review.bot | ||||||||
Priority: | P2 | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||
Hardware: | Unspecified | ||||||||||
OS: | Unspecified | ||||||||||
Bug Depends on: | |||||||||||
Bug Blocks: | 85558 | ||||||||||
Attachments: |
|
Description
Mike West
2013-03-20 01:57:20 PDT
Created attachment 194003 [details]
Patch
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 on attachment 194003 [details] Patch Attachment 194003 [details] did not pass qt-ews (qt): Output: http://webkit-commit-queue.appspot.com/results/17178585 Created attachment 194005 [details]
Patch
Bots are happy. Would you mind taking a look, Adam? 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... 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. :) Created attachment 194328 [details]
Patch for landing
Comment on attachment 194328 [details] Patch for landing Clearing flags on attachment: 194328 Committed r146520: <http://trac.webkit.org/changeset/146520> All reviewed patches have been landed. Closing bug. (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. |