Summary: | CSP 1.1: Experiment with adding line numbers to violation reports. | ||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Mike West <mkwst> | ||||||||||
Component: | New Bugs | Assignee: | Mike West <mkwst> | ||||||||||
Status: | RESOLVED FIXED | ||||||||||||
Severity: | Normal | CC: | abarth, mkwst+watchlist, rniwa, webkit.review.bot | ||||||||||
Priority: | P2 | Keywords: | WebExposed | ||||||||||
Version: | 528+ (Nightly build) | ||||||||||||
Hardware: | Unspecified | ||||||||||||
OS: | Unspecified | ||||||||||||
Bug Depends on: | 106165 | ||||||||||||
Bug Blocks: | 85558 | ||||||||||||
Attachments: |
|
Description
Mike West
2012-12-18 18:07:46 PST
Created attachment 180070 [details]
Patch
Created attachment 180995 [details]
Patch
Hey Adam! It'd be nice if you could take a look at this, but it's certainly not a priority. Just throw it on your (massive?) post-holiday pile. :) Comment on attachment 180995 [details] Patch Attachment 180995 [details] did not pass mac-ews (mac): Output: http://queues.webkit.org/results/15630290 New failing tests: http/tests/security/contentSecurityPolicy/report-uri-from-javascript.html Comment on attachment 180995 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=180995&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:1610 > + RefPtr<ScriptCallStack> stack = createScriptCallStack(1, false); What does the 1 mean in in the first position here? > Source/WebCore/page/ContentSecurityPolicy.cpp:1615 > + cspReport->setString("source-file", callFrame.sourceURL()); > + cspReport->setNumber("line-number", callFrame.lineNumber()); I guess this can be used to extract information from cross-origin scripts. Should we check whether the script is same-origin (or used CORS) the same way we do for window.onerror? (In reply to comment #5) > (From update of attachment 180995 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=180995&action=review > > > Source/WebCore/page/ContentSecurityPolicy.cpp:1610 > > + RefPtr<ScriptCallStack> stack = createScriptCallStack(1, false); > > What does the 1 mean in in the first position here? The number of call stack frames to generate. Fewer is faster, and in this case, we only need the top frame so that we can point to the last line that executed. This is also, sadly, the cause of the Mac failure. Its top frame is 'native code', which isn't what we want. I'll probably grab two frames and check the second if a) it exists, and b) it has a line number. > > > Source/WebCore/page/ContentSecurityPolicy.cpp:1615 > > + cspReport->setString("source-file", callFrame.sourceURL()); > > + cspReport->setNumber("line-number", callFrame.lineNumber()); > > I guess this can be used to extract information from cross-origin scripts. Should we check whether the script is same-origin (or used CORS) the same way we do for window.onerror? Probably, yes. Created attachment 181277 [details]
Patch
Comment on attachment 181277 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=181277&action=review > Source/WebCore/page/ContentSecurityPolicy.cpp:1616 > + int frameNumber = 0; > + if (!stack->at(0).lineNumber() && stack->size() > 1 && stack->at(1).lineNumber()) > + frameNumber = 1; > + > + const ScriptCallFrame& callFrame = stack->at(frameNumber); I might have made this a helper function with a descriptive frame. (In reply to comment #8) > (From update of attachment 181277 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=181277&action=review > > > Source/WebCore/page/ContentSecurityPolicy.cpp:1616 > > + int frameNumber = 0; > > + if (!stack->at(0).lineNumber() && stack->size() > 1 && stack->at(1).lineNumber()) > > + frameNumber = 1; > > + > > + const ScriptCallFrame& callFrame = stack->at(frameNumber); > > I might have made this a helper function with a descriptive frame. That makes sense. I'll spin a new patch with this bit extracted, and throw it into the CQ. Thanks! Created attachment 181334 [details]
Patch for landing
Comment on attachment 181334 [details] Patch for landing Clearing flags on attachment: 181334 Committed r138834: <http://trac.webkit.org/changeset/138834> All reviewed patches have been landed. Closing bug. The test added by this patch has been flaky on all bots: http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2Freport-uri-from-javascript.html (In reply to comment #13) > The test added by this patch has been flaky on all bots: > http://test-results.appspot.com/dashboards/flakiness_dashboard.html#group=%40ToT%20-%20webkit.org&tests=http%2Ftests%2Fsecurity%2FcontentSecurityPolicy%2Freport-uri-from-javascript.html Thanks for filing https://bugs.webkit.org/show_bug.cgi?id=106165. I'll take care of the flake there. |