Stop exposing BrowserFeedWriter to the Web

RESOLVED FIXED in mozilla31

Status

()

defect
RESOLVED FIXED
6 years ago
5 months ago

People

(Reporter: Ehsan, Assigned: Ehsan)

Tracking

(Blocks 2 bugs, {dev-doc-complete})

Trunk
mozilla31
x86
macOS
Points:
---
Dependency tree / graph

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

No description provided.
Blocks: 981845
Attachment #8391545 - Flags: review?(bzbarsky)
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>
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".
Attachment #8391545 - Attachment is obsolete: true
Attachment #8391545 - Flags: review?(bzbarsky)
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 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.
(In reply to neil@parkwaycc.co.uk 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 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!
Attachment #8391759 - Flags: review?(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.
Attachment #8391759 - Flags: review?(bzbarsky) → review+
https://hg.mozilla.org/mozilla-central/rev/e9ea1662020a
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla31
Keywords: dev-doc-needed
I don't think this really needs a dev-doc, this is not used externally and is just a gross feeds implementation detail.
Keywords: dev-doc-needed
I set the dev-doc-needed because this removes a Web-facing property from window.
Keywords: dev-doc-needed
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.
Err, actually CCing 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?
Can't you just build BrowserFeedWriter.webidl as part of browser/?
> 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...
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.
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?
Moved the header to content/base/src and re-enabled this interface for xulrunner:

https://hg.mozilla.org/integration/mozilla-inbound/rev/114fde4642e0
Flags: needinfo?(dtownsend+bugmail)
(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.
Sorry didn't mean to needinfo!
Flags: needinfo?(dtownsend+bugmail)
>     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.
Isn't SeaMonkey affected by this change?
Flags: needinfo?(neil)
(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.
(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.)
Flags: needinfo?(neil)
> 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?
(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.
Flags: needinfo?(neil)
> It exposes no API at all 

I'm a bit confused, then.  What is the purpose of SeaMonkey's BrowserFeedWriter?
Blocks: 991522
Blocks: 1010439
Component: DOM → DOM: Core & HTML
You need to log in before you can comment on or make changes to this bug.