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

RESOLVED FIXED in Firefox 12

Status

()

Core
DOM
RESOLVED FIXED
6 years ago
5 years ago

People

(Reporter: Justin Lebar (not reading bugmail), Assigned: Justin Lebar (not reading bugmail))

Tracking

(Blocks: 1 bug, {dev-doc-needed, testcase})

Trunk
mozilla12
dev-doc-needed, testcase
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite +

Firefox Tracking Flags

(firefox12 fixed)

Details

(Whiteboard: [qa-])

Attachments

(5 attachments, 6 obsolete attachments)

373 bytes, text/html
Details
3.26 KB, patch
mounir
: review+
Details | Diff | Splinter Review
17.48 KB, patch
smaug
: review-
Details | Diff | Splinter Review
17.20 KB, patch
Details | Diff | Splinter Review
11.08 KB, patch
smaug
: review+
Details | Diff | Splinter Review
(Assignee)

Description

6 years ago
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.
(Assignee)

Updated

6 years ago
Blocks: 693515
(Assignee)

Comment 1

6 years ago
Wow, I really don't want to add a new docshell type if I don't have to.  That looks painful.
(Assignee)

Comment 2

6 years ago
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

6 years ago
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");
(Assignee)

Comment 4

6 years ago
> 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.
(Assignee)

Comment 5

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

Comment 6

6 years ago
> 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.
(Assignee)

Comment 7

6 years ago
Created attachment 579991 [details] [diff] [review]
Proof of concept, v1
(Assignee)

Comment 8

6 years ago
Created attachment 579992 [details]
Test page: Read the location of a cross-origin iframe
(Assignee)

Comment 9

6 years ago
Created attachment 580335 [details] [diff] [review]
Part 0: Add nsContentUtils::URIInPrefList
Attachment #580335 - Flags: review?(mounir)
(Assignee)

Comment 10

6 years ago
Created attachment 580337 [details] [diff] [review]
Part 1: Allow window.location to be queried cross-origin.
Attachment #580337 - Flags: review?(bugs)
(Assignee)

Updated

6 years ago
Attachment #579991 - Attachment is obsolete: true
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&).
Attachment #580335 - Flags: review?(mounir) → review+
OS: Mac OS X → All
Hardware: x86 → All
Version: unspecified → Trunk
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 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
Attachment #580337 - Flags: review?(bugs) → review+
(Assignee)

Comment 14

5 years ago
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?
(Assignee)

Updated

5 years ago
Whiteboard: [needs landing]
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
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.
(Assignee)

Comment 17

5 years ago
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.
(Assignee)

Comment 18

5 years ago
Created attachment 582069 [details] [diff] [review]
Part 1, v2: Add queryInnerState()
Attachment #582069 - Flags: review?
(Assignee)

Comment 19

5 years ago
Created attachment 582071 [details] [diff] [review]
Part 1, v2: Add queryInnerState()
Attachment #582071 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #582071 - Attachment is obsolete: true
Attachment #582071 - Flags: review?
(Assignee)

Updated

5 years ago
Attachment #582069 - Flags: review? → review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #580337 - Attachment is obsolete: true
(Assignee)

Updated

5 years ago
Whiteboard: [needs landing] → [needs review]
(Assignee)

Comment 20

5 years ago
Created attachment 583043 [details] [diff] [review]
Part 3 - Renaming queryInnerState to getInnerState.
(Assignee)

Comment 21

5 years ago
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".
Attachment #583043 - Attachment description: Renaming queryInnerState to getInnerState. → Part 3 - Renaming queryInnerState to getInnerState.
Attachment #583043 - Flags: review?(bugs)
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".
(Assignee)

Comment 23

5 years ago
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 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
(Assignee)

Comment 25

5 years ago
Created attachment 583198 [details] [diff] [review]
Part 2: Renaming to <iframe browser> and getContentState(). (will be folded into part 2)
(Assignee)

Updated

5 years ago
Attachment #583043 - Attachment is obsolete: true
Attachment #583043 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #583198 - Attachment description: Bug 708176 - Part 2b: Renaming to <iframe browser> and getContentState(). (will be folded into part 2) → Part 3: Renaming to <iframe browser> and getContentState(). (will be folded into part 2)
Attachment #583198 - Flags: review?(bugs)
(Assignee)

Comment 26

5 years ago
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().
(Assignee)

Comment 27

5 years ago
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.)
Attachment #583198 - Attachment description: Part 3: Renaming to <iframe browser> and getContentState(). (will be folded into part 2) → Part 2: Renaming to <iframe browser> and getContentState(). (will be folded into part 2)
(Assignee)

Updated

5 years ago
Attachment #583199 - Attachment description: Part 2, v2: Add {i,}frame.queryInnerState(), which allows privileged pages to peer into an iframe's state. → Part 1, v2: Add {i,}frame.queryInnerState(), which allows privileged pages to peer into an iframe's state.
Attachment #583199 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #582069 - Attachment is obsolete: true
Attachment #582069 - Flags: review?(bugs)
(Assignee)

Updated

5 years ago
Attachment #583199 - Attachment description: Part 1, v2: Add {i,}frame.queryInnerState(), which allows privileged pages to peer into an iframe's state. → Part 1, v3: Add {i,}frame.queryInnerState(), which allows privileged pages to peer into an iframe's state.
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.
(Assignee)

Comment 29

5 years ago
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?
(Assignee)

Updated

5 years ago
Blocks: 710231
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)
Attachment #583198 - Flags: review?(bugs) → review-

Updated

5 years ago
Attachment #583199 - Flags: review?(bugs)
(Assignee)

Comment 31

5 years ago
> 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.
(Assignee)

Comment 32

5 years ago
Smaug, ping re comment 31?
(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.
(Assignee)

Comment 34

5 years ago
Created attachment 587286 [details] [diff] [review]
Address review comments.
Attachment #587286 - Flags: review?(bugs)
(Assignee)

Comment 35

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

Updated

5 years ago
Attachment #587286 - Flags: review?(bugs) → review+
(Assignee)

Comment 36

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/cc753e605073
https://hg.mozilla.org/integration/mozilla-inbound/rev/1bdcd84ae727
status-firefox12: --- → fixed
Flags: in-testsuite+
(Assignee)

Updated

5 years ago
Whiteboard: [needs review]
(Assignee)

Comment 37

5 years ago
And a followup to fix a silly test:
https://hg.mozilla.org/integration/mozilla-inbound/rev/14385cbb4131
(Assignee)

Updated

5 years ago
Depends on: 717019
So we're going to have two kinds of privileged pages, each one with its own set of APIs for writing a browser?
(Assignee)

Comment 39

5 years ago
(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.
https://hg.mozilla.org/mozilla-central/rev/14385cbb4131
Assignee: nobody → justin.lebar+bug
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla12
https://hg.mozilla.org/mozilla-central/rev/cc753e605073
https://hg.mozilla.org/mozilla-central/rev/1bdcd84ae727
(Assignee)

Comment 42

5 years ago
Created attachment 589970 [details] [diff] [review]
Patch v1: Back out
Attachment #589970 - Flags: review?
(Assignee)

Comment 43

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

Oops; wrong bug.
Attachment #589970 - Attachment is obsolete: true
Attachment #589970 - Flags: review?
(Assignee)

Comment 44

5 years ago
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.
(Assignee)

Comment 45

5 years ago
Backed out in https://hg.mozilla.org/integration/mozilla-inbound/rev/a262cdf67c82
Keywords: testcase
Whiteboard: [qa+]

Comment 46

5 years ago
Marking as [qa-] since this bug has been backed out and the bug where it has been backed out is [qa-].
Whiteboard: [qa+] → [qa-]
Keywords: dev-doc-needed
You need to log in before you can comment on or make changes to this bug.