600584 - Add more detail to CSP inline script violation logging

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Description

[Brandon Sterne (:bsterne)]

Attachment: starting point

It would be nice to add more information to the current warnings we send when an inline script violation occurs.  They currently say:

Warning: CSP: Directive "violated base restriction: Inline Scripts will not execute" violated

We should provide the script fragment and line number if possible to help developers track these down.  I started experimenting with this last night and it doesn't look very hard to do.

Comment 1

[Jeff Balogh (:jbalogh)]

Is this going to send a huge line of text if there's a violation in a minified javascript file?  Otherwise, I am looking forward to this change.

Comment 2

[Brandon Sterne (:bsterne)]

We definitely want to avoid that situation if possible.  I haven't dug deep enough yet to know for certain, but I should know very soon if it's feasible to send just the violating script fragment even if it's part of one long line.

Comment 3

[Brandon Sterne (:bsterne)]

Attachment: work in progress

Makes some changes to nsIContentSecurityPolicy to accept additional context from content callers regarding inline script violations.  Propagates the information down to the warnings placed on the console.  Basically we make a best effort to determine the source file and line number of the violation, but that information is not always available.

Still to do: send this information with CSP reports (not just console) and complete the same pattern for CSP-blocks-eval case.

Comment 4

[Johnny Stenback (:jst)]

Comment on attachment 492503 [details] [diff] [review]
work in progress

I think this generally looks good but I do have one suggestion. Rather than collecting the script sample, uri, line number, etc up front, maybe change this so that you do a check w/o supplying any context, and only if that fails you go through the effort of collecting the context data and calling into a logging facility or somesuch on the policy. That way you'd put the cost of reporting the error only into the cases where there actually is an error. A bit less convenient to code up, but less overhead.

Comment 5

[Brandon Sterne (:bsterne)]

(In reply to comment #1)
> Is this going to send a huge line of text if there's a violation in a minified
> javascript file?  Otherwise, I am looking forward to this change.

For those following along, the approach I'm taking sends (up to) the first 40 bytes of the violating code only. None of the preceding non-violation-causing JS code will be sent in the report or logged on the Error Console.  I'm sure this is preferable for most users.

40 bytes was a fairly arbitrary choice on my part, however.  If someone has a strong reason why it should be a different value, I could be persuaded.

Comment 6

[Brandon Sterne (:bsterne)]

Attachment: Fix

New patch incorporates comment 4 feedback (gathers debugging info only after failing CSP check).

Comment 7

[Johnny Stenback (:jst)]

Comment on attachment 508320 [details] [diff] [review]
Fix

- In nsScriptSecurityManager::ContentSecurity...():

+        JSStackFrame *fp = nsnull;
+        JSScript *script = nsnull;
+        nsAutoString fileName;
+        PRUint32 lineNum = 0;
+        nsAutoString scriptSample(NS_LITERAL_STRING("call to eval() or related function blocked by CSP"));

Use NS_NAMED_LITERAL_STRING(scriptSample, "...") here to avoid extra work.

+        fp = JS_FrameIterator(cx, &fp);
+        if (fp) {
+            script = JS_GetFrameScript(cx, fp);

You can move the declaration of the script variable here, as it's only used inside this if block.

- In nsScriptLoader::ProcessScriptElement():

+      nsAutoString scriptText;
+      aElement->GetScriptText(scriptText);
+
+      // cap the length of the script sample at 40 chars
+      if (scriptText.Length() > 40) {
+        scriptText = Substring(scriptText, 0, 40);

Use scriptText.Truncate(40) here.

- In nsEventListenerManager::AddScriptEventListener():

+        nsIURI* uri = doc->GetDocumentURI();
+        nsCAutoString asciiSpec;
+        uri->GetAsciiSpec(asciiSpec);

There's no guarantee that GetDocumentURI() returns a non-null pointer, so null check here before using uri.

r=jst with those issues fixed.

Comment 8

[Brandon Sterne (:bsterne)]

Attachment: Final fix

Addresses jst's review comments.  We have a Firefox 4 launch partner that really wants this feature ASAP.  If we can get it in for beta 11 that would be fantastic.

Comment 9

[LegNeato]

Comment on attachment 508432 [details] [diff] [review]
Final fix

a=LegNeato for 2.0

Comment 10

[Brandon Sterne (:bsterne)]

Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/c71dc0ee1973