All users were logged out of Bugzilla on October 13th, 2018

Add more detail to CSP inline script violation logging

RESOLVED FIXED

Status

()

RESOLVED FIXED
8 years ago
5 years ago

People

(Reporter: bsterne, Assigned: bsterne)

Tracking

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

Trunk
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(blocking2.0 -)

Details

Attachments

(1 attachment, 3 obsolete attachments)

(Assignee)

Description

8 years ago
Created attachment 479427 [details] [diff] [review]
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.
(Assignee)

Updated

8 years ago
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.
(Assignee)

Comment 2

8 years ago
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.

Updated

8 years ago
blocking2.0: ? → -
(Assignee)

Comment 3

8 years ago
Created attachment 492503 [details] [diff] [review]
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.
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+
(Assignee)

Comment 5

8 years ago
(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.
(Assignee)

Comment 6

8 years ago
Created attachment 508320 [details] [diff] [review]
Fix

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]
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.
Attachment #508320 - Flags: review?(jst) → review+
(Assignee)

Comment 8

8 years ago
Created attachment 508432 [details] [diff] [review]
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.
Attachment #508320 - Attachment is obsolete: true
Attachment #508432 - Flags: review+
Attachment #508432 - Flags: approval2.0?

Comment 9

8 years ago
Comment on attachment 508432 [details] [diff] [review]
Final fix

a=LegNeato for 2.0
Attachment #508432 - Flags: approval2.0? → approval2.0+
(Assignee)

Comment 10

8 years ago
Pushed to m-c:
http://hg.mozilla.org/mozilla-central/rev/c71dc0ee1973
Status: NEW → RESOLVED
Last Resolved: 8 years ago
Resolution: --- → FIXED
Depends on: 903968
You need to log in before you can comment on or make changes to this bug.