1042436 - Add more visible warnings when property access silently fails due to XrayToJS or Opaque Xrays

Status: RESOLVED FIXED

(Core :: XPConnect, defect)

Description

[Bobby Holley (:bholley)]

Bug 856067 and bug 987111 were major milestones in securing interaction between content and privileged JS. The downside, of course, is that the filtering behavior may cause addons to fail somewhat inexplicably.

There's currently a mechanism to dump to the console in debug builds when this happens, but I've come to the conclusion that we should report something to the browser console as well.

Comment 1

[Bobby Holley (:bholley)]

Sorry I've been slow on this - Some other Mozilla stuff came up, and I've also been distracted with a family medical emergency. I'll try to take a crack at this today.

Comment 2

[Gabor Krizsanits (INACTIVE)]

(In reply to Bobby Holley (:bholley) from comment #1)
> Sorry I've been slow on this - Some other Mozilla stuff came up, and I've
> also been distracted with a family medical emergency. I'll try to take a
> crack at this today.

I hope everything is alright, and thanks for the feedback.

Comment 3

[Bobby Holley (:bholley)]

Attachment: Part 1 - Always enter the wrapper's compartment before invoking SilentFailure. v1

Comment 4

[Bobby Holley (:bholley)]

Attachment: Part 2 - Warn once to the console when XrayWrappers deny access to an object. v1

Gabor for review, smaug to make sure I'm using the console correctly, and
jorge to confirm that this is the right thing for addon devs.

Comment 5

[Gabor Krizsanits (INACTIVE)]

Comment on attachment 8468214 [details] [diff] [review]
Part 2 - Warn once to the console when XrayWrappers deny access to an object. v1

Review of attachment 8468214 [details] [diff] [review]:
-----------------------------------------------------------------

r=me with a few minor changes:

::: js/xpconnect/wrappers/XrayWrapper.cpp
@@ +583,5 @@
> +
> +    // The browser console warning is only emitted for the first violation,
> +    // whereas the (debug-only) NS_WARNING is emitted for each violation.
> +#ifndef DEBUG
> +    if (alreadyWarnedOnce)

How about adding a pref flag for non-debug mode here that can turn on the debug behavior?

@@ +602,5 @@
> +
> +    // If this isn't the first warning on this topic for this global, we've
> +    // already bailed out in opt builds. Now that the NS_WARNING is done, bail
> +    // out in debug builds as well.
> +    if (alreadyWarnedOnce)

Shouldn't the debug build bail out after NS_WARNING always? It seems to me that in debug build for the first error we are going to warn twice, and the second one with the message "only the first ... will be reported" which is not true...

@@ +609,5 @@
> +    //
> +    // Log a message to the console service.
> +    //
> +
> +    // Grab the pieces.

Can we do this whole console code bellow in some common place, like nsContentUtils or xpcprivate? I find it useful as a utility function, and does not look like some code I wanted to write myself / duplicate very often...

Comment 6

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

nsContentUtils has almost similar code. Almost, but not exactly, so reusing that here wouldn't quite work.

gabor, NS_WARNING goes to the terminal, so I'm not sure why that reporting is relevant here.

Comment 7

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

Comment on attachment 8468214 [details] [diff] [review]
Part 2 - Warn once to the console when XrayWrappers deny access to an object. v1

I think the comment about XRayWrapper denying access isn't clear enough.
The comment should tell why the access was denied, not what denied the access.

Comment 8

[Gabor Krizsanits (INACTIVE)]

(In reply to Olli Pettay [:smaug] from comment #6)
> gabor, NS_WARNING goes to the terminal, so I'm not sure why that reporting
> is relevant here.

Ah right, I've got a bit confused there... But wouldn't the console warning show on the
terminal as well? Anyway, it does not matter, even if does we still want to keep both,
so just ignore my comment.

Comment 9

[Bobby Holley (:bholley)]

(In reply to Gabor Krizsanits [:krizsa :gabor] from comment #8)
> Ah right, I've got a bit confused there... But wouldn't the console warning
> show on the
> terminal as well?

I don't believe that it does.

Comment 10

[Bobby Holley (:bholley)]

Attachment: Part 2 - Lowercase the "reason" param to SilentFailure. v1 r=me

Comment 11

[Bobby Holley (:bholley)]

Attachment: Part 3 - Warn once to the console when XrayWrappers deny access to an object. v3 r=gabor

Comment 12

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/0cafdd33a6f8
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/59cd444c129b
remote:   https://hg.mozilla.org/integration/mozilla-inbound/rev/36d685698443

Comment 13

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/0cafdd33a6f8
https://hg.mozilla.org/mozilla-central/rev/59cd444c129b
https://hg.mozilla.org/mozilla-central/rev/36d685698443

Comment 14

[Bobby Holley (:bholley)]

This should be in tomorrow's Nightly. Jorge, can you play around with it tomorrow and let me know if the behavior is satisfactory? Assuming it is, I'd like to get this backported as soon as possible.

Comment 15

[Jorge Villalobos [:jorgev] (he/him)]

The message looks good, thanks!

Comment 16

[Bobby Holley (:bholley)]

Comment on attachment 8468686 [details] [diff] [review]
Part 3 - Warn once to the console when XrayWrappers deny access to an object. v3 r=gabor

Great! Flagging for uplift.

Bug 987111 is now on beta. We had some addon-compat concerns, and thankfully we have received scant reports of any breakage, which is great. Regardless though, Jorge would like to be able to point developers to the browser console for information about whether the new security wrappers interfered with a given addon. This information will drastically reduce confusion about the cause of any breakage and the time needed to track down what line of code to fix.

Approval Request Comment
[Feature/regressing bug #]: bug 987111, in a sense. 
[User impact if declined]: Potential pain for addon developers adapting to bug 987111.
[Describe test coverage new/current, TBPL]: Landed on m-c. Automated test coverage.
[Risks and why]: Low risk 
[String/UUID change made/needed]: The message is non-localized, so no string changes were made.

Comment 17

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/releases/mozilla-aurora/rev/2449111f679e
https://hg.mozilla.org/releases/mozilla-aurora/rev/704ef45176bd
https://hg.mozilla.org/releases/mozilla-aurora/rev/98d705a709c7

https://hg.mozilla.org/releases/mozilla-beta/rev/6037db66624b
https://hg.mozilla.org/releases/mozilla-beta/rev/4cb373d9ce33
https://hg.mozilla.org/releases/mozilla-beta/rev/1cf7b5810eb5

Comment 18

[Ryan VanderMeulen [:RyanVM]]

Backed out from beta for bustage. This'll need rebasing.
https://hg.mozilla.org/releases/mozilla-beta/rev/53c7aceaf1a8

Comment 19

[Bobby Holley (:bholley)]

remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/08c5b02a125e
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/84bf42618416
remote:   https://hg.mozilla.org/releases/mozilla-beta/rev/c405f720f587

Comment 20

[[SV Manager] Florin Mezei, QA (:FlorinMezei)]

https://hg.mozilla.org/releases/mozilla-beta/file/c405f720f587/js/xpconnect/tests/chrome/test_bug1042436.xul