708176 - [WebAPI] Allow privileged pages to access cross-origin properties in child iframes

Status: RESOLVED FIXED

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

Description

[Justin Lebar (not reading bugmail)]

The B2G browser needs to be able to create an iframe and look inside it, violating the cross-origin policy.

There's a lot of work here, but I'd like this bug to track a simple case, letting the parent frame read its child's window.location.  We might get other properties for free, but I'm not going to go out of my way for them.

Comment 1

[Justin Lebar (not reading bugmail)]

Wow, I really don't want to add a new docshell type if I don't have to.  That looks painful.

Comment 2

[Justin Lebar (not reading bugmail)]

Some terminology to keep us sane:

The HTMLBrowser is the webpage which acts like a browser.  The HTMLBrowserContent is the webpage loaded in an iframe by the HTMLBrowser.  The HTMLBrowser needs to peer into the HTMLBrowserContent.

Comment 3

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

What all properties are needed from HTMLBrowserContent?

Could there be some (possibly async) postMessage -like API to query certain properties.
The web page in HTMLBrowserContent wouldn't need to implement it, but it would be
builtin.

HTMLBrowser:

iframe.onbrowserstate = function(evt) {
  alert(evt.state + "=" + evt.data);
}
iframe.queryBrowserState("location");

Comment 4

[Justin Lebar (not reading bugmail)]

> What all properties are needed from HTMLBrowserContent?

Lots of things.  Read window.location and document.title, make bitmap screen captures of the rendered page (for thumbnails), receive events when the iframe navigates (so it can update the location bar)...

HTMLBrowser needs to be a Browser with a capital B.  So whatever Firefox chrome does, it's likely that HTMLBrowser needs to do.

Ben pointed out, for example, that we need to be able to peer into the DOM to implement sessionrestore.

Comment 5

[Justin Lebar (not reading bugmail)]

See https://wiki.mozilla.org/WebAPI/EmbeddedBrowserAPI for a (surely partial) list of things needed.

Comment 6

[Justin Lebar (not reading bugmail)]

> Could there be some (possibly async) postMessage -like API to query certain properties.

This would certainly be a lot easier than hacking wrappers, and it might get us 90% of the way there.

We'd still need something for registering events.

Comment 7

[Justin Lebar (not reading bugmail)]

Attachment: Proof of concept, v1

Comment 8

[Justin Lebar (not reading bugmail)]

Attachment: Test page: Read the location of a cross-origin iframe

Comment 9

[Justin Lebar (not reading bugmail)]

Attachment: Part 0: Add nsContentUtils::URIInPrefList

Comment 10

[Justin Lebar (not reading bugmail)]

Attachment: Part 1: Allow window.location to be queried cross-origin.

Comment 11

[Mounir Lamouri (:mounir)]

Comment on attachment 580335 [details] [diff] [review]
Part 0: Add nsContentUtils::URIInPrefList

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

You must have done too much C in your life to use |type *param| :)

r=me with the comments addressed

::: content/base/public/nsContentUtils.h
@@ +1862,5 @@
> +   *
> +   * Comparisons are case-insensitive, and whitespace between elements of the
> +   * comma-separated list is ignored.
> +   */
> +  static bool URIInPrefList(nsIURI *aURI, const char *aPref);

nit: I would have prefer "IsURIInPrefList", it is more readable.

::: content/base/src/nsContentUtils.cpp
@@ +696,5 @@
> +
> +    if (whitelistItem.Length() == prePath.Length() &&
> +        CaseInsensitiveCompare(whitelistItem.Data(),
> +                               prePath.Data(),
> +                               whitelistItem.Length()) == 0) {

You can use nsAString::Equals(const PRUnichar*, const nsStringComparator&).

Comment 12

[Mounir Lamouri (:mounir)]

Comment on attachment 580337 [details] [diff] [review]
Part 1: Allow window.location to be queried cross-origin.

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

Couldn't you create a new interface that would apply to nsHTMLIFrameElement only if "dom.mozQueryInnerStateEnabled" is true? Seems cleaner to me.

Comment 13

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

Comment on attachment 580337 [details] [diff] [review]
Part 1: Allow window.location to be queried cross-origin.


>+  nsIPrincipal *principal = NodePrincipal();
>+  nsCOMPtr<nsIURI> principalURI;
>+  principal->GetURI(getter_AddRefs(principalURI));
>+  if (!nsContentUtils::URIInPrefList(principalURI, "dom.mozQueryInnerStateWhitelist")) {
>+    return NS_ERROR_FAILURE;
>+  }

Should chrome be able to query always? And, throw security error, not generic NS_ERROR_FAILURE

Comment 14

[Justin Lebar (not reading bugmail)]

How do you feel about Mounir's suggestion to move this code into a separate interface?

> Should chrome be able to query always?

I'm tempted to say YAGNI.  But I'd also be happy to put a chrome check into URIInPrefList.  What do you think?

Comment 15

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

Ah, yes, a separate interface would be good.

Oh, also, make sure also <frame> element supports the interface. So, could you push that implementation
to nsGenericHTMLFrameElement

Comment 16

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

It would be strange to add chrome check to URIInPrefList, since that has nothing to do with
chrome. There are other helpers which you could use.

Comment 17

[Justin Lebar (not reading bugmail)]

Maybe we should change the name, but URIInPrefList is going to be used all over the place until we get the much promised Permission Manager, so I'd rather have both the pref and chrome check in one place.

Comment 18

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v2: Add queryInnerState()

Comment 19

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v2: Add queryInnerState()

Comment 20

[Justin Lebar (not reading bugmail)]

Attachment: Part 3 - Renaming queryInnerState to getInnerState.

Comment 21

[Justin Lebar (not reading bugmail)]

Comment on attachment 583043 [details] [diff] [review]
Part 3 - Renaming queryInnerState to getInnerState.

I spent a while trying to come up with a better name for this.

I came up with

  <iframe inspectable>
  getInnerState()
  setInnerState()
  addInnerStateListener(topic, callback)
  removeInnerStateListener(topic, callback)

I feel like this is better than "queryInnerState", since it's not clear how the setter would be named.  But I'm not entirely satisfied with "inspectable".

Comment 22

[Ben Francis [:benfrancis]]

How about...

<iframe browser>
getContentProperty(name, listener)
setContentProperty(name)
addContentEventListener(type, listener)
removeContentEventListener(type, listener)

Unless there's some reason it's inaccurate, "*ContentProperty" seems more in-keeping with the current lexicon than "*InnerState" (e.g. window vs. contentWindow).

I'm not so sure about "browser" vs. "inspectable" but bare in mind that as well as getters, setters and event listeners there may need to be methods such as "reload" and "stop" so the iFrame isn't just inspectable, it can be controlled too. I can't think of a more accurate term to describe this succinctly than "browser".

Comment 23

[Justin Lebar (not reading bugmail)]

I'm OK with that naming.  Smaug?

I don't really like <iframe browser>, because that's sort of proscriptive rather than descriptive.  By which I mean, it's saying "you can make a browser out of this", rather than "you can peer into this iframe and modify it."  But we can always change it again if we come up with something better.

Comment 24

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

Comment on attachment 582069 [details] [diff] [review]
Part 1, v2: Add queryInnerState()

>+  nsCxPusher pusher;
>+  nsCOMPtr<nsIDOMEventTarget> eventTarget =
>+    do_QueryInterface(nsContentUtils::GetWindowFromCaller());
>+  NS_ENSURE_TRUE(eventTarget, NS_ERROR_FAILURE);
>+  pusher.Push(eventTarget);
One must always check the return value of Push

Comment 25

[Justin Lebar (not reading bugmail)]

Attachment: Part 2: Renaming to <iframe browser> and getContentState(). (will be folded into part 2)

Comment 26

[Justin Lebar (not reading bugmail)]

Attachment: Part 1, v3: Add {i,}frame.queryInnerState(), which allows privileged pages to peer into an iframe's state.

Check the return value of Push().

Comment 27

[Justin Lebar (not reading bugmail)]

Comment on attachment 583198 [details] [diff] [review]
Part 2: Renaming to <iframe browser> and getContentState(). (will be folded into part 2)

(Sorry to churn the attachment descriptions like this -- I'm trying out a git workflow and still working out the kinks.)

Comment 28

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

inspectable sounds strange. Hard for at least one non-native-English speaker.

What would add/RemoveContentEventListener do?

Will the iframe run always in the same process? If not, perhaps getContentProperty should be
requestContentProperty and the callback would be called async.

Comment 29

[Justin Lebar (not reading bugmail)]

I don't find it so confusing that "getContentState" is async, myself.  Since we have something called "setContentState" (which is also async), maybe it makes sense to have a corresponding "get" function?

Comment 30

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

Comment on attachment 583198 [details] [diff] [review]
Part 2: Renaming to <iframe browser> and getContentState(). (will be folded into part 2)


>-[scriptable, function, uuid(077660D6-43A1-4A83-B7D8-F49AC0B539B0)]
>-interface nsIDOMQueryInnerStateCallback : nsISupports
>+[scriptable, function, uuid(37687881-1801-489f-ad03-7af651a93448)]
>+interface nsIDOMInnerStateCallback : nsISupports
nsIDOMMoz...


>-[scriptable, uuid(655d6101-6b60-4a55-ba1c-cff5e17ec3f4)]
>-interface nsIDOMQueryableFrameElement : nsISupports
>+[scriptable, uuid(2ff0f421-64e4-4186-b0dd-619629f46048)]
>+interface nsIDOMBrowserFrameElement : nsISupports
nsIDOMMoz...

>-   * If the iframe is not mozQueryable, or if the calling window is not
>+   * If the iframe's mozBrowser is false, or if the calling window is not
>    * privileged, this function fails.
>    */
>-  void mozQueryInnerState(in DOMString property,
>-                          in nsIDOMQueryInnerStateCallback callback);
>+  void mozGetContentState(in DOMString property,
>+                          in nsIDOMInnerStateCallback callback);
Why do we need callback here. The implementation seems to work synchronously.
If the idea is that in the future iframe would be using a separate process, then
async API is needed. But please implement even the current API in an async way. (use runnables)

Comment 31

[Justin Lebar (not reading bugmail)]

> If the idea is that in the future iframe would be using a separate process, then
> async API is needed.

Yes, this is the idea.

> But please implement even the current API in an async way. (use runnables)

Since this API is exposed only to B2G at the moment and for the foreseeable future, will you let me get away with responding synchronously in this patch?  It'll be wasted work to rewrite all this, since the IPC code will be totally different.

Comment 32

[Justin Lebar (not reading bugmail)]

Smaug, ping re comment 31?

Comment 33

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

(In reply to Justin Lebar [:jlebar] from comment #31)
> Since this API is exposed only to B2G at the moment and for the foreseeable
> future, will you let me get away with responding synchronously in this
> patch?

No. Since if the API is first sync, all the code written using it will assume sync behavior.
It would be painful to change from sync to async later.

>  It'll be wasted work to rewrite all this, since the IPC code will be
> totally different.
Handling the callback in a runnable for now isn't difficult. That is enough to force
the user of the API to handle things async.

Comment 34

[Justin Lebar (not reading bugmail)]

Attachment: Address review comments.

Comment 35

[Justin Lebar (not reading bugmail)]

I can do a roll-up patch too, if you'd prefer.  But this seems simple enough to me.

Comment 36

[Justin Lebar (not reading bugmail)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/cc753e605073
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bdcd84ae727

Comment 37

[Justin Lebar (not reading bugmail)]

And a followup to fix a silly test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14385cbb4131

Comment 38

[Robert O'Callahan (:roc) (email my personal email if necessary)]

So we're going to have two kinds of privileged pages, each one with its own set of APIs for writing a browser?

Comment 39

[Justin Lebar (not reading bugmail)]

(In reply to Robert O'Callahan (:roc) (Mozilla Corporation) from comment #38)
> So we're going to have two kinds of privileged pages, each one with its own
> set of APIs for writing a browser?

Yes?

Note that a lot of the existing Chrome API (e.g. nsIWebProgress) is hardly a web API.

Comment 40

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/14385cbb4131

Comment 41

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/cc753e605073
https://hg.mozilla.org/mozilla-central/rev/1bdcd84ae727

Comment 42

[Justin Lebar (not reading bugmail)]

Attachment: Patch v1: Back out

Comment 43

[Justin Lebar (not reading bugmail)]

Comment on attachment 589970 [details] [diff] [review]
Patch v1: Back out

Oops; wrong bug.

Comment 44

[Justin Lebar (not reading bugmail)]

Note that this bug was backed out as part of bug 710231.  The functionality there subsumes the functionality added here, so I'm leaving this bug as FIXED.

Comment 45

[Justin Lebar (not reading bugmail)]

Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a262cdf67c82

Comment 46

[Ioana (away)]

Marking as [qa-] since this bug has been backed out and the bug where it has been backed out is [qa-].