983845 - Stop exposing BrowserFeedWriter to the Web

Status: RESOLVED FIXED

(Core :: DOM: Core & HTML, defect)

Comment 1

[(no longer active)]

Attachment: Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web; r=bzbarsky

Comment 2

[(no longer active)]

https://tbpl.mozilla.org/?tree=Try&rev=73884165c467

Comment 3

[(no longer active)]

OK, so the NS_SecurityCompareURIs change breaks all of the things, as can be seen on try.  Boris, what's the right way to do this?  Note that we need to do something which is compatible with the magic here: <http://mxr.mozilla.org/mozilla-central/source/browser/components/feeds/src/FeedConverter.js#248>

Comment 4

[Boris Zbarsky [:bzbarsky]]

Your proximate problem is that CheckSameOrigin() throws an exception on the JSContext if the check fails.

But even if we addressed that, I don't think we want to actually change SecurityComparURIs here.  Not without a lot of thinking about the attack avenues it opens up.

Instead, I'd probably go with checking that xpc::WindowGlobalOrNull(aGlobal) returns non-null, and then that the resulting principal has a URI which has SchemeIs("about") (to avoid getting long specs in common cases), and spec is "about:feeds".

Comment 5

[(no longer active)]

Attachment: Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web; r=bzbarsky

Comment 6

[(no longer active)]

Comment on attachment 8391759 [details] [diff] [review]
Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web; r=bzbarsky

https://tbpl.mozilla.org/?tree=Try&rev=7442d8aadeaa

Comment 7

[[e-mail]]

Comment on attachment 8391759 [details] [diff] [review]
Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web; r=bzbarsky

>+  void subscribe();
Nobody actually calls subscribe(). Although subscribe.js references it, nobody calls that anyway.

It's actually possible to refactor the code so that you don't actually have to call anything from content at all; see bug 846763. (Unfortunately because nsIXPCScriptable is itself not actually scriptable you can't hide the nsIDOMGlobalObjectConstructor interface from explicit discovery via QueryInterface, and SeaMonkey doesn't hide the other three interfaces it uses, although that's achievable through use of a wrapper object. Also webidl doesn't support nsIDOMGlobalObjectConstructor but if you're able to limit the use of BrowserFeedWriter to about:feeds itself then you could probably switch to nsIDOMGlobalPropertyInitializer).

Of course, there may be a better way for about:feeds to notify chrome; I'm open to suggestions.

Comment 8

[(no longer active)]

(In reply to [e-mail] from comment #7)
> Comment on attachment 8391759 [details] [diff] [review]
> Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web;
> r=bzbarsky
> 
> >+  void subscribe();
> Nobody actually calls subscribe(). Although subscribe.js references it,
> nobody calls that anyway.
> 
> It's actually possible to refactor the code so that you don't actually have
> to call anything from content at all; see bug 846763. (Unfortunately because
> nsIXPCScriptable is itself not actually scriptable you can't hide the
> nsIDOMGlobalObjectConstructor interface from explicit discovery via
> QueryInterface, and SeaMonkey doesn't hide the other three interfaces it
> uses, although that's achievable through use of a wrapper object. Also
> webidl doesn't support nsIDOMGlobalObjectConstructor but if you're able to
> limit the use of BrowserFeedWriter to about:feeds itself then you could
> probably switch to nsIDOMGlobalPropertyInitializer).

Please file a follow-up, but I won't probably take that.  My only interest here is to stop expose BrowserFeedWriter to the Web, which this patch accomplishes.

Comment 9

[(no longer active)]

Comment on attachment 8391759 [details] [diff] [review]
Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web; r=bzbarsky

Seems like I forgot to ask for review here or something!

Comment 10

[Boris Zbarsky [:bzbarsky]]

Comment on attachment 8391759 [details] [diff] [review]
Port nsIFeedWriter to WebIDL and stop exposing BrowserFeedWriter to the Web; r=bzbarsky

>+++ b/browser/components/feeds/src/FeedWriterEnabled.h
>+  NS_ENSURE_TRUE(uri, false);

Please don't warn if !uri.  That just means system principal, which is a totally normal thing to have around.  Just return false.

>+  bool isAbout = false;
>+  uri->SchemeIs("about", &isAbout);
>+  if (!isAbout) {

Document that this is an optimization to avoid getting long specs?

>+++ b/dom/bindings/Bindings.conf
>+'BrowserFeedWriter': {
>+    'headerFile': 'mozilla/FeedWriterEnabled.h',

This seems fishy to me.  In particular, that's not where BrowserFeedWriter is defined...

It may be simpler to do Func="mozilla::FeedWriterEnabled::IsEnabled", which will automatically make us include "mozilla/FeedWriterEnabled.h" for the function.  And then you can just make FeedWriterEnabled a namespace, or a class with a static method.

>+++ b/dom/webidl/BrowserFeedWriter.webidl
>+  void init(WindowProxy aWindow);

We could drop this by implementing QI to nsIDOMGlobalPropertyInitializer on the component (in which case the WebIDL machinery will automatically call init() with a window and we could remove the explicit call).  Either way.

>+  void subscribe();

This is unused, right?  Why add it?

>+++ b/dom/webidl/moz.build
>+if CONFIG['MOZ_BUILD_APP'] == 'browser':

Any Firefox-on-xulrunner worries here?  Seems like there should be.

r=me with the above addressed.

Comment 11

[(no longer active)]

Addressed all of the comments: https://hg.mozilla.org/integration/mozilla-inbound/rev/e9ea1662020a

Comment 12

[Carsten Book [:Tomcat]]

https://hg.mozilla.org/mozilla-central/rev/e9ea1662020a

Comment 13

[:Gavin Sharp [email: [e-mail]]]

I don't think this really needs a dev-doc, this is not used externally and is just a gross feeds implementation detail.

Comment 14

[(no longer active)]

I set the dev-doc-needed because this removes a Web-facing property from window.

Comment 15

[(no longer active)]

This broke XULRunner builds: <https://tbpl.mozilla.org/php/getParsedLog.php?id=36495684&tree=Mozilla-Central#error0>

I took out the bit which was supposed to not break xulrunner <http://hg.mozilla.org/integration/mozilla-inbound/rev/c46d3d35b106>.  I guess Firefox over XULRunner won't have feed support, or something.  CCing Mossop to tell me what I should do, if anything.

Comment 16

[(no longer active)]

Err, actually CCing Mossop.

Comment 17

[Dave Townsend [:mossop]]

(In reply to :Ehsan Akhgari (lagging on bugmail, needinfo? me!) from comment #15)
> This broke XULRunner builds:
> <https://tbpl.mozilla.org/php/getParsedLog.php?id=36495684&tree=Mozilla-
> Central#error0>
> 
> I took out the bit which was supposed to not break xulrunner
> <http://hg.mozilla.org/integration/mozilla-inbound/rev/c46d3d35b106>.  I
> guess Firefox over XULRunner won't have feed support, or something.  CCing
> Mossop to tell me what I should do, if anything.

That's probably going to break some linux distro's. I don't know webidl enough to know what the right answer is. Is there a way to have the webidl and related bits be defined in the browser/ tree?

Comment 18

[:Gavin Sharp [email: [e-mail]]]

Can't you just build BrowserFeedWriter.webidl as part of browser/?

Comment 19

[Boris Zbarsky [:bzbarsky]]

> Is there a way to have the webidl and related bits be defined in the browser/ tree?

Not really, no, because WebIDL is designed for the web platform, which doesn't have concepts like independent pieces or ordering.

We can just move the header to somewhere that's built in xulrunner builds, like content/base or whatnot...

Comment 20

[:Gavin Sharp [email: [e-mail]]]

We should just get rid of the friggin' thing and reimplementing feed support in some other way, really, but that doesn't really help Ehsan at the moment.

Comment 21

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/c46d3d35b106

Comment 22

[Mike Hommey [:glandium]]

Sounds to me you could either move FeedWriterEnabled.h to somewhere in toolkit, or add its source path as INCLUDES in dom/bindings. That being said, how does webidl react to an implementation not being there? For example, what happens in the webapp runtime, which doesn't have browser data?

Comment 23

[(no longer active)]

Moved the header to content/base/src and re-enabled this interface for xulrunner:

https://hg.mozilla.org/integration/mozilla-inbound/rev/114fde4642e0

Comment 24

[(no longer active)]

(In reply to Mike Hommey [:glandium] from comment #22)
> Sounds to me you could either move FeedWriterEnabled.h to somewhere in
> toolkit, or add its source path as INCLUDES in dom/bindings. That being
> said, how does webidl react to an implementation not being there? For
> example, what happens in the webapp runtime, which doesn't have browser data?

You will get a runtime js exception when trying to call into the API.

Comment 25

[(no longer active)]

Sorry didn't mean to needinfo!

Comment 26

[Phil Ringnalda (:philor)]

https://hg.mozilla.org/mozilla-central/rev/114fde4642e0

Comment 27

[Masatoshi Kimura [:emk]]

>     1.2 --- a/browser/components/feeds/src/FeedWriterEnabled.h
>     1.3 +++ /dev/null
>     3.2 --- /dev/null
>     3.3 +++ b/content/base/src/FeedWriterEnabled.h

You should have used hg mv.

Comment 28

[Masatoshi Kimura [:emk]]

Isn't SeaMonkey affected by this change?

Comment 29

[(no longer active)]

(In reply to comment #27)
> >     1.2 --- a/browser/components/feeds/src/FeedWriterEnabled.h
> >     1.3 +++ /dev/null
> >     3.2 --- /dev/null
> >     3.3 +++ b/content/base/src/FeedWriterEnabled.h
> 
> You should have used hg mv.

Sorry, I use git locally and sometimes mess things up when extracting a patch out of it to land into hg.  Fortunately this file did not have a lot of history.

Comment 30

[[e-mail]]

(In reply to Masatoshi Kimura from comment #28)
> Isn't SeaMonkey affected by this change?

Not yet; because we're not a listed app the WebIDL doesn't get compiled in. (Although for people building SeaMonkey over XULRunner I'm not sure whether the existing category overrides WebIDL or not.)

Comment 31

[Boris Zbarsky [:bzbarsky]]

> I'm not sure whether the existing category overrides WebIDL or not.

WebIDL bits take precedence over the existing category if both are registered for the same name, as far as I can tell.

Does seamonkey use the same contractid for this name?  If so, does it expose the same API?

Comment 32

[[e-mail]]

(In reply to Boris Zbarsky from comment #31)
> Does seamonkey use the same contractid for this name?
Currently it doesn't, but that could be changed.

> If so, does it expose the same API?

It exposes no API at all (except for the limitation that nsIXPCScriptable is itself not scriptable and therefore content can always call QueryInterface(nsIDOMGlobalObjectConstructor) on the object) so that's not a problem here.

Comment 33

[Boris Zbarsky [:bzbarsky]]

> It exposes no API at all 

I'm a bit confused, then.  What is the purpose of SeaMonkey's BrowserFeedWriter?

Comment 34

[Kohei Yoshino [:kohei]]

This won't affect content, but for reference:

https://developer.mozilla.org/en-US/Firefox/Releases/31/Site_Compatibility