Bug 105374 – CSP 1.1: Experiment with adding line numbers to violation reports.

• Component: New Bugs
• Status: RESOLVED FIXED

[Mike West]

CSP 1.1: Experiment with adding line numbers to violation reports.

[Mike West]

Created attachment 180070 [details]
Patch

[Mike West]

Created attachment 180995 [details]
Patch

[Mike West]

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

[Build Bot]

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

[Adam Barth]

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?

[Mike West]

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

[Mike West]

Created attachment 181277 [details]
Patch

[Adam Barth]

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.

[Mike West]

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

[Mike West]

Created attachment 181334 [details]
Patch for landing

[WebKit Review Bot]

Comment on attachment 181334 [details]
Patch for landing

Clearing flags on attachment: 181334

Committed r138834: <http://trac.webkit.org/changeset/138834>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.

[Ryosuke Niwa]

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

[Mike West]

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