Bug 105374 - CSP 1.1: Experiment with adding line numbers to violation reports.
: CSP 1.1: Experiment with adding line numbers to violation reports.
Status: RESOLVED FIXED
Product: WebKit
Classification: Unclassified
Component: New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To: Mike West
: WebExposed
Depends on: 106165
Blocks: 85558
  Show dependency treegraph
 
Reported: 2012-12-18 18:07 PST by Mike West
Modified: 2013-01-04 23:10 PST (History)
4 users (show)

See Also:


Attachments
Patch (2.09 KB, patch)
2012-12-18 18:08 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (7.98 KB, patch)
2013-01-01 12:08 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch (8.29 KB, patch)
2013-01-04 00:09 PST, Mike West
no flags Details | Formatted Diff | Diff
Patch for landing (8.61 KB, patch)
2013-01-04 11:02 PST, 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 2012-12-18 18:07:46 PST
CSP 1.1: Experiment with adding line numbers to violation reports.
Comment 1 Mike West 2012-12-18 18:08:34 PST
Created attachment 180070 [details]
Patch
Comment 2 Mike West 2013-01-01 12:08:18 PST
Created attachment 180995 [details]
Patch
Comment 3 Mike West 2013-01-01 12:10:41 PST
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 4 Build Bot 2013-01-01 12:36:36 PST
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 5 Adam Barth 2013-01-02 10:58:16 PST
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?
Comment 6 Mike West 2013-01-02 11:44:54 PST
(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.
Comment 7 Mike West 2013-01-04 00:09:30 PST
Created attachment 181277 [details]
Patch
Comment 8 Adam Barth 2013-01-04 09:46:39 PST
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.
Comment 9 Mike West 2013-01-04 10:53:18 PST
(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!
Comment 10 Mike West 2013-01-04 11:02:31 PST
Created attachment 181334 [details]
Patch for landing
Comment 11 WebKit Review Bot 2013-01-04 12:39:46 PST
Comment on attachment 181334 [details]
Patch for landing

Clearing flags on attachment: 181334

Committed r138834: <http://trac.webkit.org/changeset/138834>
Comment 12 WebKit Review Bot 2013-01-04 12:39:50 PST
All reviewed patches have been landed.  Closing bug.
Comment 14 Mike West 2013-01-04 23:10:45 PST
(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.