Last Comment Bug 708176 - [WebAPI] Allow privileged pages to access cross-origin properties in child iframes
: [WebAPI] Allow privileged pages to access cross-origin properties in child if...
Status: RESOLVED FIXED
[qa-]
: dev-doc-needed, testcase
Product: Core
Classification: Components
Component: DOM (show other bugs)
: Trunk
: All All
: -- normal (vote)
: mozilla12
Assigned To: Justin Lebar (not reading bugmail)
:
Mentors:
Depends on: 717019
Blocks: browser-api 710231
  Show dependency treegraph
 
Reported: 2011-12-06 20:34 PST by Justin Lebar (not reading bugmail)
Modified: 2012-06-12 18:01 PDT (History)
14 users (show)
justin.lebar+bug: in‑testsuite+
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---
fixed


Attachments
Proof of concept, v1 (4.63 KB, patch)
2011-12-08 02:36 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review
Test page: Read the location of a cross-origin iframe (373 bytes, text/html)
2011-12-08 02:40 PST, Justin Lebar (not reading bugmail)
no flags Details
Part 0: Add nsContentUtils::URIInPrefList (3.26 KB, patch)
2011-12-09 00:22 PST, Justin Lebar (not reading bugmail)
mounir: review+
Details | Diff | Review
Part 1: Allow window.location to be queried cross-origin. (11.18 KB, patch)
2011-12-09 00:23 PST, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Review
Part 1, v2: Add queryInnerState() (16.90 KB, patch)
2011-12-15 12:47 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review
Part 1, v2: Add queryInnerState() (16.90 KB, patch)
2011-12-15 12:54 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review
Part 3 - Renaming queryInnerState to getInnerState. (18.63 KB, patch)
2011-12-19 18:36 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review
Part 2: Renaming to <iframe browser> and getContentState(). (will be folded into part 2) (17.48 KB, patch)
2011-12-20 09:22 PST, Justin Lebar (not reading bugmail)
bugs: review-
Details | Diff | Review
Part 1, v3: Add {i,}frame.queryInnerState(), which allows privileged pages to peer into an iframe's state. (17.20 KB, patch)
2011-12-20 09:26 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review
Address review comments. (11.08 KB, patch)
2012-01-10 06:29 PST, Justin Lebar (not reading bugmail)
bugs: review+
Details | Diff | Review
Patch v1: Back out (18.45 KB, patch)
2012-01-19 13:24 PST, Justin Lebar (not reading bugmail)
no flags Details | Diff | Review

Description Justin Lebar (not reading bugmail) 2011-12-06 20:34:48 PST
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) 2011-12-06 22:21:19 PST
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) 2011-12-06 23:23:47 PST
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] 2011-12-07 01:51:22 PST
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) 2011-12-07 02:05:26 PST
> 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) 2011-12-07 02:07:31 PST
See https://wiki.mozilla.org/WebAPI/EmbeddedBrowserAPI for a (surely partial) list of things needed.
Comment 6 Justin Lebar (not reading bugmail) 2011-12-07 18:15:13 PST
> 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) 2011-12-08 02:36:39 PST
Created attachment 579991 [details] [diff] [review]
Proof of concept, v1
Comment 8 Justin Lebar (not reading bugmail) 2011-12-08 02:40:36 PST
Created attachment 579992 [details]
Test page: Read the location of a cross-origin iframe
Comment 9 Justin Lebar (not reading bugmail) 2011-12-09 00:22:06 PST
Created attachment 580335 [details] [diff] [review]
Part 0: Add nsContentUtils::URIInPrefList
Comment 10 Justin Lebar (not reading bugmail) 2011-12-09 00:23:43 PST
Created attachment 580337 [details] [diff] [review]
Part 1: Allow window.location to be queried cross-origin.
Comment 11 Mounir Lamouri (:mounir) 2011-12-09 00:40:29 PST
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) 2011-12-09 00:43:59 PST
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] 2011-12-14 05:43:15 PST
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) 2011-12-14 06:52:45 PST
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] 2011-12-14 08:00:10 PST
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] 2011-12-14 08:02:19 PST
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) 2011-12-14 08:11:54 PST
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) 2011-12-15 12:47:31 PST
Created attachment 582069 [details] [diff] [review]
Part 1, v2: Add queryInnerState()
Comment 19 Justin Lebar (not reading bugmail) 2011-12-15 12:54:09 PST
Created attachment 582071 [details] [diff] [review]
Part 1, v2: Add queryInnerState()
Comment 20 Justin Lebar (not reading bugmail) 2011-12-19 18:36:01 PST
Created attachment 583043 [details] [diff] [review]
Part 3 - Renaming queryInnerState to getInnerState.
Comment 21 Justin Lebar (not reading bugmail) 2011-12-19 18:38:57 PST
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] 2011-12-20 03:34:53 PST
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) 2011-12-20 07:09:07 PST
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] 2011-12-20 09:19:51 PST
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) 2011-12-20 09:22:33 PST
Created attachment 583198 [details] [diff] [review]
Part 2: Renaming to <iframe browser> and getContentState(). (will be folded into part 2)
Comment 26 Justin Lebar (not reading bugmail) 2011-12-20 09:26:54 PST
Created attachment 583199 [details] [diff] [review]
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) 2011-12-20 09:27:46 PST
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] 2011-12-20 09:36:56 PST
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) 2011-12-20 09:42:16 PST
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] 2011-12-29 11:29:20 PST
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) 2012-01-03 07:19:44 PST
> 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) 2012-01-04 19:03:00 PST
Smaug, ping re comment 31?
Comment 33 Olli Pettay [:smaug] 2012-01-05 07:17:02 PST
(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) 2012-01-10 06:29:52 PST
Created attachment 587286 [details] [diff] [review]
Address review comments.
Comment 35 Justin Lebar (not reading bugmail) 2012-01-10 06:31:36 PST
I can do a roll-up patch too, if you'd prefer.  But this seems simple enough to me.
Comment 37 Justin Lebar (not reading bugmail) 2012-01-10 13:19:37 PST
And a followup to fix a silly test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14385cbb4131
Comment 38 Robert O'Callahan (:roc) (Exited; email my personal email if necessary) 2012-01-10 14:22:45 PST
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) 2012-01-10 16:06:58 PST
(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] 2012-01-10 18:48:50 PST
https://hg.mozilla.org/mozilla-central/rev/14385cbb4131
Comment 42 Justin Lebar (not reading bugmail) 2012-01-19 13:24:24 PST
Created attachment 589970 [details] [diff] [review]
Patch v1: Back out
Comment 43 Justin Lebar (not reading bugmail) 2012-01-19 13:26:11 PST
Comment on attachment 589970 [details] [diff] [review]
Patch v1: Back out

Oops; wrong bug.
Comment 44 Justin Lebar (not reading bugmail) 2012-01-20 09:11:03 PST
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) 2012-01-20 12:27:42 PST
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a262cdf67c82
Comment 46 Ioana (away) 2012-04-20 04:54:03 PDT
Marking as [qa-] since this bug has been backed out and the bug where it has been backed out is [qa-].

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