Closed Bug 600584 Opened 12 years ago Closed 12 years ago

Add more detail to CSP inline script violation logging


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

Not set



Tracking Status
blocking2.0 --- -


(Reporter: bsterne, Assigned: bsterne)


(Depends on 1 open bug, Blocks 1 open bug)



(1 file, 3 obsolete files)

Attached patch starting point (obsolete) — Splinter Review
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.
blocking2.0: --- → ?
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.
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.
blocking2.0: ? → -
Attached patch work in progress (obsolete) — Splinter Review
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.
Attachment #479427 - Attachment is obsolete: true
Attachment #492503 - Flags: feedback?(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.
Attachment #492503 - Flags: feedback?(jst) → feedback+
(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.
Attached patch Fix (obsolete) — Splinter Review
New patch incorporates comment 4 feedback (gathers debugging info only after failing CSP check).
Attachment #492503 - Attachment is obsolete: true
Attachment #508320 - Flags: review?(jst)
Comment on attachment 508320 [details] [diff] [review]

- 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.
Attachment #508320 - Flags: review?(jst) → review+
Attached patch Final fixSplinter Review
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.
Attachment #508320 - Attachment is obsolete: true
Attachment #508432 - Flags: review+
Attachment #508432 - Flags: approval2.0?
Comment on attachment 508432 [details] [diff] [review]
Final fix

a=LegNeato for 2.0
Attachment #508432 - Flags: approval2.0? → approval2.0+
Pushed to m-c:
Closed: 12 years ago
Resolution: --- → FIXED
Depends on: 903968
You need to log in before you can comment on or make changes to this bug.