JS security errors painfully uninformative

VERIFIED FIXED in mozilla0.9.4

Status

()

Core
Security: CAPS
P1
normal
VERIFIED FIXED
17 years ago
17 years ago

People

(Reporter: John Bandhauer, Assigned: Mitchell Stoltz (not reading bugmail))

Tracking

Trunk
mozilla0.9.4
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: patch, URL)

Attachments

(2 attachments)

(Reporter)

Description

17 years ago
For instance in bug 83107 we see:

  Error: uncaught exception: Permission denied to create wrapper for object

The only way I could get a clue about the real issue here was to run under the 
debugger, set a breakpoint where the the security error happens (lucky me, I 
know that), and then inspect the objects associated with the code on the stack.

I'm thinking that most DOM programmers are not going to be able to do this when 
they run into a problem.

I can think of three strategies to improve this...

a) Have the caps code format the info available to it when if builds its 
exception - at least the caller location.

b) Have xpconnect add extra info to the exception. Currently, the caps code sets 
a JSString as an expection. xpconnect could get that string exception and format 
a new exception that incorporates that string and adds the information about the 
call attempted. xpconnect would need to do something else if the pending 
exception turned out to not be a string.

c) We could give up on the idea that the caps code will give informative 
exception output and just change the interface contract such that the caps code 
is not even responsible for setting a JS expection and instead just returns some 
result code to the caller (xpconnect usually) that identifies the type of error 
and lets the caller set the exception of its choice. This will require changing 
all uses of nsIXPCSecurityManager.

We *do* have the potential security issue of exposing *too* much info. Maybe 
this does not matter. But it should be considered.

I say 'we' above, but I mean the plural 'you' :)
(Assignee)

Comment 1

17 years ago
Very colorful bug summary...

This is something I'd been meaning to do but ran out of time for 0.9.1. I think
we can do (a), which is what I was planning to do anyway, unless you think one
of the other options is simpler or makes for better organization. In the case of
canAccess and canCreateWrapper, I have a classInfo to get the class name from.
For the others I have only a CID. How do I look up a class name from a CID?
Performance isn't as much of an issue in this case, since this is an error
condition, so it's OK if the lookup is clunky.

Do we really need to be checking on canCreateWrapper? We discussed this before,
but I forget the conclusion. What can a script do with a wrapper on which they
can't call any properties or methods? 
Target Milestone: --- → mozilla0.9.2
(Assignee)

Updated

17 years ago
Status: NEW → ASSIGNED
Priority: -- → P4
Target Milestone: mozilla0.9.2 → mozilla0.9.3

Comment 2

17 years ago
*** Bug 87059 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 3

17 years ago
*** Bug 70162 has been marked as a duplicate of this bug. ***

Comment 4

17 years ago
Markintg INVALID
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago
Resolution: --- → INVALID

Comment 5

17 years ago
Okay, I realllllllly need to drink my coffee *before* looking at bugs.  As Jesse 
has very politely pointed out to me, *this* is the real bug and 70162 is the 
dup.


Ahem.
Status: RESOLVED → REOPENED
Resolution: INVALID → ---

Updated

17 years ago
Status: REOPENED → ASSIGNED
(Assignee)

Comment 6

17 years ago
*** Bug 91282 has been marked as a duplicate of this bug. ***
(Assignee)

Comment 7

17 years ago
Target is now 0.9.4, Priority P1.
Priority: P4 → P1
Target Milestone: mozilla0.9.3 → mozilla0.9.4
(Assignee)

Comment 8

17 years ago
Created attachment 45015 [details] [diff] [review]
Patch - more descriptive error messages and refactor the call to SetExceptionWasThrown
(Assignee)

Updated

17 years ago
Whiteboard: patch
- No need for the PRUnichar* cast at:

+  errorMsg.AppendWithConversion((PRUnichar*)JSValIDToString(aJSContext, aName));

- No need for rv2 here:

+            nsresult rv2;
+            nsCOMPtr<nsIXPConnect> xpc = do_GetService(nsIXPConnect::GetCID(),
&rv2);
+            if (NS_SUCCEEDED(rv2))

simply:

+            nsCOMPtr<nsIXPConnect> xpc = do_GetService(nsIXPConnect::GetCID());
+            if (xpc)

would do the same thing (since you don't return the error code anyway).

The code:

+        nsCAutoString errorMsg("Permission denied to create wrapper for object ");
...
+        if (aClassInfo)
+        {
...
+                errorMsg += "of class ";
+                errorMsg += className;
+            }
+        }

could maybe be changed to include "of class " in the initial value of errorMsg
here in stead of appending it separately?

This will leak the string from ToString() (in a few places):

+        errorMsg += aCID.ToString();

Other than that, sr=jst
(Assignee)

Comment 10

17 years ago
Created attachment 45443 [details] [diff] [review]
Patch with jst's changes (there are some unrelated changes in here too)
A few nits...

- I still see the extra PRUnichar* cast line ~200

- Be consistent with whitespace-after-if:

+            if (aClassInfo)
+                aClassInfo->GetClassDescription(getter_Copies(className));
+            if(className)
+                errorMsg += className;

- 3-space indentation at:
@@ -661,6 +712,9 @@
 nsresult
 nsScriptSecurityManager::GetBaseURIScheme(nsIURI* aURI, char** aScheme)
 {
+    if (!aURI)
+       return NS_ERROR_FAILURE;

sr=jst
(Assignee)

Comment 12

17 years ago
Fix checked in.
Status: ASSIGNED → RESOLVED
Last Resolved: 17 years ago17 years ago
Resolution: --- → FIXED

Comment 13

17 years ago
any reason why when I try to access "history.length" we write:
"Error: uncaught exception: Permission denied to get property History.length"

"history" -> "History"?
"history" == object name

"History" == class of object

Comment 15

17 years ago
Verified on 2001-09-19-03 build on WinNT

Ran the following code on the browser window:
javascript:alert(x.document.images[0]);

And got the following suitable error:
Error: uncaught exception: Permission denied to get property HTMLDocument.images
Status: RESOLVED → VERIFIED
You need to log in before you can comment on or make changes to this bug.