594999 - "Security Manager vetoed action" exception when accessing any property of InstallTrigger

Status: VERIFIED FIXED

(Core :: XPConnect, defect)

Description

[Tom Dale]

User-Agent:       Mozilla/5.0 (Macintosh; U; Intel Mac OS X 10_6_4; en-us) AppleWebKit/533.17.8 (KHTML, like Gecko) Version/5.0.1 Safari/533.17.8
Build Identifier: Mozilla/5.0 (Macintosh; Intel Mac OS X 10.6; rv:2.0b5) Gecko/20100101 Firefox/4.0b5

When accessing any undefined property of the window.InstallTrigger object, an exception is thrown.

Reproducible: Always

Steps to Reproduce:
1. Open the included test case in Firefox 4 Beta 5
2. Open the Error Console
Actual Results:  
Firefox throws an exception:
Error: uncaught exception: [Exception... "Security Manager vetoed action"  nsresult: "0x80570027 (NS_ERROR_XPC_SECURITY_MANAGER_VETO)"  location: "JS frame :: file:///Users/tomdale/Desktop/install_trigger_exception.html :: onclick :: line 1"  data: no]

Expected Results:  
Attempts to retrieve undefined properties do not create a security problem and should not throw an exception. I would expect undefined to be returned.

We consider this a high-priority bug because it causes almost all SproutCore applications to cease working. We generate string representations of classes partly by iterating over the window object and checking for a certain property on each object.

Because this happens at application initialization, this exception being thrown causes existing applications immediately cease functioning on page load. And any web application that iterates over the window object will likely be impacted by this as well.

Comment 1

[Tom Dale]

Attachment: Test case demonstrating the problem

Comment 2

[Alon Zakai (:azakai)]

Looks like an xpconnect issue, specifically with __exposedProps__.

Comment 3

[Michael Kraft [:morac]]

I tested the test case with the latest nightly Trunk load and the error message generated is currently:

Error: Permission denied to access property 'unknownProperty'
Source file: https://bug594999.bugzilla.mozilla.org/attachment.cgi?id=473805
Line: 4


That's pretty much the same error I'm seeing at me.com, which is broken in Firefox 4 beta and presumably what triggered this bug to be filed.

Comment 6

[Dave Townsend [:mossop]]

This is I guess sort of intentional, you aren't allowed to access anything other than the exposed properties. What is me.com expecting to be able to access that it can't?

Comment 7

[Alon Zakai (:azakai)]

The file is minified so it's hard to tell, but maybe they enumerate over all properties, |for (x in obj)|, maybe looking for some property they care about.

In any case, if we returned |undefined| instead of throwing a security exception, this would probably be fixed. But I don't know enough about the security manager to know if that doesn't introduce some other problem.

Comment 8

[Dave Townsend [:mossop]]

Blake, is there a way to have the __exposedProps__ thing just claim undefined for anything except that in the list?

Comment 9

[Andreas Gal :gal]

We should probably block on this.

Comment 10

[Juan Pinzon]

Alon is right.

We basically iterate through the window object using for (x in window). Then it finds the "InstallTrigger" property which I assume shouldn't be enumerable if it's not supposed to be accessed.

Comment 11

[Andreas Gal :gal]

No owner? Stealing.

Comment 12

[Andreas Gal :gal]

Attachment: patch

Comment 13

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

It appears that the "in" operator doesn't work with this patch.

I.e., for an object like:

x = {
  foo: 5,
  __exposedProps__: {},
};

|"foo" in x| returns true. Still writing tests.

Comment 14

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Comment on attachment 507787 [details] [diff] [review]
patch

It appears that with this patch, if a property is *not* exposed using __exposedProps__ the "in" operator always claims that the property exists. However in that case it should claim that the property does not exist, right?

If a property is listed in __exposedProps__, then the "in" operator works fine and properly reflects the existence of the property in the underlying object, whether it exists or not.


Enumeration appears to be working fine though.

Comment 15

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Tests

These were the tests I used to find the "in" bug.

The patch as-is should be passing with just the patch in this bug. The commented out tests test the behavior in the patch in bug 628410, hence they are commented out.

Comment 16

[Andreas Gal :gal]

Trivial fix. Uploading a new patch. Thanks for writing the tests! Andreas

Comment 17

[Andreas Gal :gal]

Attachment: patch

Comment 18

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Merged patch

This merges on top of bug 628410

Comment 19

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Attachment: Test fixes

Tests for this bug and bug 628410

Comment 20

[Blake Kaplan (:mrbkap) (inactive)]

Comment on attachment 508067 [details] [diff] [review]
Merged patch

>+    desc->obj= NULL; // default result if we refuse to perform this action

Nit: (here and below). Need a space before the = sign.

Comment 21

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

This is breaking Mochitests :(

Specifically

js/src/xpconnect/tests/mochitest/test_bug428021.html is failing with

Permission denied to access property 'document' at http://mochi.test:8888/tests/js/src/xpconnect/tests/mochitest/test_bug396851.html:17


Though it's really weird that the test files aren't lining up here...

Comment 22

[Andreas Gal :gal]

How do I run that one test only? I can try to reproduce.

Comment 23

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Even weirder is that that line is *supposed* to throw! But there's a try/catch around it.

Are we somehow resurfacing an old exception?

Comment 24

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

go to objdir/_test/testing/mochitest
run: python runtests.py --test-path=js/src/xpconnect/tests/mochitest/ --autorun

There seems to be some weird interaction between tests here, so you likely need to run the whole directory.

Comment 25

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

Make that

python runtests.py --test-path=js/src/xpconnect/tests/mochitest/ --autorun --debugger=gdb

if you want to run inside gdb.

Comment 26

[Andreas Gal :gal]

Its not failing for me.

Comment 27

[Mounir Lamouri (:mounir)]

(In reply to comment #25)
> Make that
> 
> python runtests.py --test-path=js/src/xpconnect/tests/mochitest/ --autorun
> --debugger=gdb
> 
> if you want to run inside gdb.

FWIW, `TEST_PATH="js/src/xpconnect/tests/mochitest/test_bug396851.html" make mochitest-plain` (to run in $OBJDIR) is probably an easier way. And add EXTRA_TEST_ARGS='--debugger=gdb' to run in in a debugger.

Comment 28

[Jonas Sicking (:sicking) No longer reading bugmail consistently]

The patch is failing in all sorts of weird ways, and it's not consistent across platforms. Even some of the trivial tests in test_cows are failing in some runs. Maybe there's some uninitialized variable in there?

http://tbpl.mozilla.org/?tree=MozillaTry&rev=ab66e36aa42c

Comment 29

[Andreas Gal :gal]

Yeah, I think I found something. New patch in a sec.

Comment 30

[Andreas Gal :gal]

Attachment: patch

Comment 31

[Andreas Gal :gal]

Attachment: patch

Comment 32

[Andreas Gal :gal]

I pushed this to try again, with the uninitialized variable fix.

Comment 33

[Andreas Gal :gal]

With the fix we seem to pass try.

Comment 34

[Andreas Gal :gal]

http://hg.mozilla.org/tracemonkey/rev/2fb3475b3036

Comment 35

[Andreas Gal :gal]

And the tests:

http://hg.mozilla.org/tracemonkey/rev/59d84fe951ba

Comment 36

[Olli Pettay [:smaug][[e-mail]]]

Does this fix Bug 627305? 
(I don't have access to me.com, so can't test)

Comment 38

[Chris Leary [:cdleary] (not checking bugmail)]

cdleary-bot mozilla-central merge info:
http://hg.mozilla.org/mozilla-central/rev/2fb3475b3036
http://hg.mozilla.org/mozilla-central/rev/59d84fe951ba

Comment 39

[Steve Scott (pxbugz)]

Verified fixed on Mozilla/5.0 (Windows NT 6.1; WOW64; rv:2.0b12pre) Gecko/20110204 Firefox/4.0b12pre