Last Comment Bug 842372 - Make getUserData and setUserData ChromeOnly
: Make getUserData and setUserData ChromeOnly
Status: RESOLVED FIXED
: addon-compat, dev-doc-complete, site-compat
Product: Core
Classification: Components
Component: DOM: Core & HTML (show other bugs)
: unspecified
: All All
: -- normal (vote)
: mozilla22
Assigned To: Masatoshi Kimura [:emk]
:
: Andrew Overholt [:overholt]
Mentors:
Depends on: 834697 838691 876228
Blocks: 749981
  Show dependency treegraph
 
Reported: 2013-02-18 12:38 PST by Masatoshi Kimura [:emk]
Modified: 2013-05-28 16:17 PDT (History)
9 users (show)
VYV03354: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
patch (4.74 KB, patch)
2013-02-18 12:38 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
mochitest (2.18 KB, patch)
2013-02-19 12:59 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Hide getUserData and setUserData from content (8.81 KB, patch)
2013-02-20 15:32 PST, Masatoshi Kimura [:emk]
bzbarsky: review+
Details | Diff | Splinter Review
Part 1: Implement nsINode::IsChromeOrXBL (2.71 KB, patch)
2013-02-22 04:42 PST, Masatoshi Kimura [:emk]
bobbyholley: review-
Details | Diff | Splinter Review
Part 2: Hide getUserData and setUserData from content (7.69 KB, patch)
2013-02-22 04:43 PST, Masatoshi Kimura [:emk]
no flags Details | Diff | Splinter Review
Part 1: Implement nsINode::ShouldExposeUserData (2.73 KB, patch)
2013-02-22 10:25 PST, Masatoshi Kimura [:emk]
bobbyholley: review+
Details | Diff | Splinter Review
Part 2: Hide getUserData and setUserData from content (7.70 KB, patch)
2013-02-22 10:25 PST, Masatoshi Kimura [:emk]
bzbarsky: review+
Details | Diff | Splinter Review

Description Masatoshi Kimura [:emk] 2013-02-18 12:38:14 PST
Created attachment 715218 [details] [diff] [review]
patch

test_bug548463.html essentially requires get/setUserData() as long as it's supported. So I've SpecialPower.wrap()ed the node to have an access to get/setUserData().
test_treewalker_nextsibling.xml used get/setUserData() only to test the object identity. So I've rewritten it using WeakMap.
test_bug384632.html did no longer test what is supposed to be tested because Node migrated to WebIDL. Given that all DOM interfaces would migrate to WebIDL sooner or later, I've rewritten the test using nsIWritablePropertyBag.
Comment 1 Masatoshi Kimura [:emk] 2013-02-18 14:53:52 PST
https://tbpl.mozilla.org/?tree=Try&rev=5ef9fdc9d43f
Comment 2 Boris Zbarsky [:bz] (still a bit busy) 2013-02-19 06:07:30 PST
Doesn't this break addons that use get/setUserData from XBL bindings, if any?
Comment 3 Masatoshi Kimura [:emk] 2013-02-19 12:59:54 PST
Created attachment 715661 [details] [diff] [review]
mochitest

(In reply to Boris Zbarsky (:bz) from comment #2)
> Doesn't this break addons that use get/setUserData from XBL bindings, if any?

Yes, it breaks.
Is it possible to add an extended attribute something like [ChromeOrXBLOnly]?
Comment 4 Boris Zbarsky [:bz] (still a bit busy) 2013-02-19 13:24:18 PST
Hmm.

Once Bobby flips the separate-scope pref, won't XBL always see content through Xrays?  If so, it'll automatically get treated as chrome for [ChromeOnly] purposes, I'd think.
Comment 5 Masatoshi Kimura [:emk] 2013-02-19 13:57:57 PST
(In reply to Boris Zbarsky (:bz) from comment #4)
> Hmm.
> 
> Once Bobby flips the separate-scope pref, won't XBL always see content
> through Xrays?  If so, it'll automatically get treated as chrome for
> [ChromeOnly] purposes, I'd think.

Does it mean bug 834697? It is already fixed and I confirmed "dom.xbl_scopes" was enabled on my local build.
Comment 6 Boris Zbarsky [:bz] (still a bit busy) 2013-02-19 14:06:32 PST
Hmm.  Yes, that's teh one I was thinking of.

Bobby, any idea why this doesn't work with the pref flipped?
Comment 7 Masatoshi Kimura [:emk] 2013-02-19 14:15:34 PST
I think if ChromeOnly properties are visible from the content XBL, it is a security hole because content easily bind a XBL as my mochitest shown.
At least I closed bug 814254 assuming that ChromeOnly properties are invisible from the content.
Comment 8 Boris Zbarsky [:bz] (still a bit busy) 2013-02-19 14:17:37 PST
Mochitests flip a pref that allows random XBL.  Normal content can only apply chrome:// bindings, and even then only script inside the binding would be able to read [ChromeOnly] properties.

Assuming that it works at all, of course...  I'm still trying to understand why it doesn't, before we try to make it work for this case.
Comment 9 Masatoshi Kimura [:emk] 2013-02-19 14:35:11 PST
Do XBL compartments have the system principal?
https://mxr.mozilla.org/mozilla-central/source/js/xpconnect/wrappers/AccessCheck.cpp?rev=1c2e7ae47afc#90
Comment 10 Olli Pettay [:smaug] 2013-02-19 14:44:43 PST
No.
Comment 12 Boris Zbarsky [:bz] (still a bit busy) 2013-02-19 16:51:08 PST
> Do XBL compartments have the system principal?

No, but I thought WebIDL Xrays always got the "chrome" view.  And I thought that XBL compartments got the Xray view.  Which one of those assumptions is wrong? 

The code quoted in comment 11 is for direct prototype setup, not Xrays.
Comment 13 Bobby Holley (:bholley) (busy with Stylo) 2013-02-19 16:53:45 PST
(In reply to Boris Zbarsky (:bz) from comment #12)
> > Do XBL compartments have the system principal?
> 
> No, but I thought WebIDL Xrays always got the "chrome" view.

I sure hope not. After all, content can always get an Xray view to its own objects via Components.lookupMethod, so that would be a major security bug if that were the case. Marking this comment private until someone confirms that's not the case.

> And I thought
> that XBL compartments got the Xray view.

Yes, on account of having nsExpandedPrincipal.

> Which one of those assumptions is wrong? 

I sure hope the former.
Comment 14 Boris Zbarsky [:bz] (still a bit busy) 2013-02-19 17:12:28 PST
OK, it's the former.  XrayResolveNativeProperty only looks at nativeProperties.chromeOnly when xpc::AccessCheck::isChrome(js::GetObjectCompartment(wrapper)).  Unmarking comment 13 private bit.

So given that, I suppose we could add another level of access that's always available Xrays, even unprivileged ones...  Or make it XBL-specific somehow.  Or something.
Comment 15 Masatoshi Kimura [:emk] 2013-02-19 18:15:00 PST
Will [Func="IsCallerChromeOrXBL"] work once bug 838691 has been landed?
Comment 16 Boris Zbarsky [:bz] (still a bit busy) 2013-02-19 18:49:22 PST
Oh, very nice.

Assuming we can tell from the cx (presumably from it's compartment) that we're "chrome or xbl", then yes, I think it would.
Comment 17 Masatoshi Kimura [:emk] 2013-02-20 15:32:26 PST
Created attachment 716269 [details] [diff] [review]
Hide getUserData and setUserData from content
Comment 18 Boris Zbarsky [:bz] (still a bit busy) 2013-02-20 19:15:03 PST
Comment on attachment 716269 [details] [diff] [review]
Hide getUserData and setUserData from content

Very nice!
Comment 19 Bobby Holley (:bholley) (busy with Stylo) 2013-02-21 07:49:18 PST
Given that this depends on XBL scopes, we'll have to back it out if we ever flip off dom.xbl_scopes. Marking the dep so that we make sure to remember.
Comment 20 Boris Zbarsky [:bz] (still a bit busy) 2013-02-21 07:50:16 PST
We can just check the pref in the function, and claim the properties are enabled if the pref is flipped off, no?  That might be a good idea.
Comment 21 Bobby Holley (:bholley) (busy with Stylo) 2013-02-21 08:05:57 PST
(In reply to Boris Zbarsky (:bz) from comment #20)
> We can just check the pref in the function, and claim the properties are
> enabled if the pref is flipped off, no?  That might be a good idea.

Yes, let's do that.
Comment 22 Masatoshi Kimura [:emk] 2013-02-22 04:42:42 PST
Created attachment 717066 [details] [diff] [review]
Part 1: Implement nsINode::IsChromeOrXBL
Comment 23 Masatoshi Kimura [:emk] 2013-02-22 04:43:20 PST
Created attachment 717067 [details] [diff] [review]
Part 2: Hide getUserData and setUserData from content
Comment 24 Bobby Holley (:bholley) (busy with Stylo) 2013-02-22 09:37:18 PST
Comment on attachment 717066 [details] [diff] [review]
Part 1: Implement nsINode::IsChromeOrXBL

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

I have some concerns with this patch:

* Why do we take an unused JSObject parameter? Is it some artifact of what the CodeGen needs?
* Why is this a function on nsINode? CodeGen again?
* The pref-disabled behavior here is dangerous given what the function is named. I understand that it's ok for this use-case to just say "true" if xbl scopes are disabled, but if anyone else starts using it it might not be. I'd prefer to give this a very specific name like "ShouldExposeGetUserData" or something.
Comment 25 Boris Zbarsky [:bz] (still a bit busy) 2013-02-22 09:48:21 PST
> Is it some artifact of what the CodeGen needs?

Yes.  The codegen passes in the JSContext and the JSObject that the prop will be defined on so that callees can introspect those as needed.

> * Why is this a function on nsINode? CodeGen again?

It just needs to be a static function somewhere, for codegen purposes.
Comment 26 Masatoshi Kimura [:emk] 2013-02-22 10:25:18 PST
Created attachment 717194 [details] [diff] [review]
Part 1: Implement nsINode::ShouldExposeUserData

(In reply to Bobby Holley (:bholley) from comment #24)
> * Why do we take an unused JSObject parameter? Is it some artifact of what
> the CodeGen needs?

Yes, the function signature must be something like |FuncName(JSContext*, JSOnject*)|.

> * Why is this a function on nsINode? CodeGen again?

I put it on nsINode because it is not a general-puopose function as you pointed out in the next bullet.

> * The pref-disabled behavior here is dangerous given what the function is
> named. I understand that it's ok for this use-case to just say "true" if xbl
> scopes are disabled, but if anyone else starts using it it might not be. I'd
> prefer to give this a very specific name like "ShouldExposeGetUserData" or
> something.

Renamed.
Comment 27 Masatoshi Kimura [:emk] 2013-02-22 10:25:59 PST
Created attachment 717195 [details] [diff] [review]
Part 2: Hide getUserData and setUserData from content
Comment 28 Bobby Holley (:bholley) (busy with Stylo) 2013-02-22 11:16:12 PST
Comment on attachment 717194 [details] [diff] [review]
Part 1: Implement nsINode::ShouldExposeUserData

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

::: content/base/public/nsINode.h
@@ +1134,5 @@
>    
>      return NS_OK;
>    }
>  
> +  static bool ShouldExposeUserData(JSContext* aCx, JSObject* /* unused */);

Please add some comments here stating that this refers to the getUserData/setUserData accessors. When I read this, I it sounds like a privacy issue. ;-)
Comment 29 Jonas Sicking (:sicking) No longer reading bugmail consistently 2013-02-22 17:09:44 PST
Comment on attachment 717195 [details] [diff] [review]
Part 2: Hide getUserData and setUserData from content

I won't have time to look at this. Looks like bz has been involved here.
Comment 30 Boris Zbarsky [:bz] (still a bit busy) 2013-02-22 20:09:33 PST
Comment on attachment 717195 [details] [diff] [review]
Part 2: Hide getUserData and setUserData from content

r=me
Comment 33 Kohei Yoshino [:kohei] 2013-03-29 09:52:52 PDT
I've added this bug to the compatibility doc. Please correct the info if wrong.
https://developer.mozilla.org/en-US/docs/Site_Compatibility_for_Firefox_22

Also, please update the following docs:
https://developer.mozilla.org/en-US/docs/DOM/Node.getUserData
https://developer.mozilla.org/en-US/docs/DOM/Node.setUserData

Note You need to log in before you can comment on or make changes to this bug.