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
: WebKit
New Bugs
: 528+ (Nightly build)
: Unspecified Unspecified
: P2 Normal
Assigned To:
:
: WebExposed
: 106165
: 85558
  Show dependency treegraph
 
Reported: 2012-12-18 18:07 PST by
Modified: 2013-01-04 23:10 PST (History)


Attachments
Patch (2.09 KB, patch)
2012-12-18 18:08 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch (7.98 KB, patch)
2013-01-01 12:08 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch (8.29 KB, patch)
2013-01-04 00:09 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff
Patch for landing (8.61 KB, patch)
2013-01-04 11:02 PST, Mike West
no flags Review Patch | Details | Formatted Diff | Diff


Note

You need to log in before you can comment on or make changes to this bug.


Description From 2012-12-18 18:07:46 PST
CSP 1.1: Experiment with adding line numbers to violation reports.
------- Comment #1 From 2012-12-18 18:08:34 PST -------
Created an attachment (id=180070) [details]
Patch
------- Comment #2 From 2013-01-01 12:08:18 PST -------
Created an attachment (id=180995) [details]
Patch
------- Comment #3 From 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 From 2013-01-01 12:36:36 PST -------
(From update of attachment 180995 [details])
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 From 2013-01-02 10:58:16 PST -------
(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?

> 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 From 2013-01-02 11:44:54 PST -------
(In reply to comment #5)
> (From update of attachment 180995 [details] [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 From 2013-01-04 00:09:30 PST -------
Created an attachment (id=181277) [details]
Patch
------- Comment #8 From 2013-01-04 09:46:39 PST -------
(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.
------- Comment #9 From 2013-01-04 10:53:18 PST -------
(In reply to comment #8)
> (From update of attachment 181277 [details] [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 From 2013-01-04 11:02:31 PST -------
Created an attachment (id=181334) [details]
Patch for landing
------- Comment #11 From 2013-01-04 12:39:46 PST -------
(From update of attachment 181334 [details])
Clearing flags on attachment: 181334

Committed r138834: <http://trac.webkit.org/changeset/138834>
------- Comment #12 From 2013-01-04 12:39:50 PST -------
All reviewed patches have been landed.  Closing bug.
------- Comment #14 From 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.