Closed Bug 785310 Opened 12 years ago Closed 10 years ago

html5 sandboxed iframe should not be able to perform top navigation with scripts allowed

Categories

(Core :: Security, defect)

Product:
Core
Component:
Security
Version:
17 Branch
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

()

Status:
RESOLVED FIXED
Milestone:
mozilla29
Tracking Flags:
Tracking Status
firefox16 --- unaffected
firefox17 + wontfix
firefox18 - affected

People

(Reporter: freddy, Assigned: bobowen)

References

(Blocks 3 open bugs)

Details

(Keywords: sec-high)

Attachments

(5 files, 21 obsolete files)

test main file, contains the sandboxed iframe element
335 bytes, text/html
Details
sandboxed file that tries to navigate the top window away
584 bytes, text/html
Details
the website states that the test has failed
238 bytes, text/html
Details
Bug 785310 fix - Add source browsing context and use GetIncumbentGlobal when navigating using Location.
37.78 KB, patch
bobowen
: review+
Details | Diff | Splinter Review
Bug 785310 - Tests v11
38.46 KB, patch
bobowen
: review+
Details | Diff | Splinter Review 
When sandboxing an iframe with the allow-scripts directive, said iframe's content is allowed to change the location of the current website using a top.location assignment.

This is *not* in conjunction what IE and Chrome do, however I failed to pull out the relevant part of the specification for this case. However, since there is a designated "allow-top-navigation" keyword, it is probably wise to deny this feature unless explicitly stated in the sandbox attribute anyway. 

I tested this on Nightly and attach my three simple html pages as follows:

test_topnav.html: Contains the sandbox iframe element that has src=topnav.html, the only sandbox attribute is allow-scripts.
topnav.html: This is supposed to be sandboxed and tries moving the outer window away to topnav2.html
topnav2.html: This website just says that the test case failed (i.e. top navigation was possible).
Oh, to be fair: This is all just a stripped down version of the first test in the Microsoft test suite over at http://ie.microsoft.com/testdrive/HTML5/sandbox/Default.html

Going over there with nightly might just suffice, the attachments are just more concise.
Making this security sensitive.
Group: core-security
I'm gonna say this is sec-high
Keywords: sec-high
The existing top navigation tests use <a target='_top' ...>, I'll add a test case for window.top.location.
Adding dveditz to ask if he agrees with sec-high
It looks like in Chrome you need allow-same-origin AND allow-top-navigation to be able to set window.top.location from a sandboxed iframe - however, you can read the value of window.top.location in script with only allow-same-origin. 

In Gecko, currently, the value of window.top.location can be changed from a sandboxed iframe without 'allow-same-origin', regardless of the value of 'allow-top-navigation' and can be read in script. The behavior around 'allow-same-origin' is consistent with how a non-same-origin document in a non-sandboxed iframe behaves.
from dom/base/nsLocation.cpp nsLocation::setHref

  JSContext *cx;

  if (NS_FAILED(GetContextFromStack(stack, &cx)))
    return NS_ERROR_FAILURE;

  if (cx) {
    rv = SetHrefWithContext(cx, aHref, false);
  } else {
    rv = GetHref(oldHref);

when will we not have a JSContext here ? when we're called from chrome ?
in my simple test case, which sets window.location from a document in an iframe with sandbox='allow-scripts', LocationSetterGuts<nsIDOMWindow>() ends up calling nsLocation::SetHrefWithContext - the context is then used to get the base URL of the caller via nsIDocument. 

So, in LocationSetterGuts<nsIDOMWindow>, it seems we could use nsLocation::GetSourceDocument and the JSContext to check the sandbox flags of the caller to see if it has 'allow-top-navigation' - when the templated interface is nsIDOMWindow, we could possibly use that to check the window whose location is being set is top - i'm not sure how to handle the cases when it ISN'T nsIDOMWindow (or what these cases are)
seems like I can use if nsIDOMWindow->GetScriptableTop == (nsIDOMWindow whose location is being set) to see if we're top and need to check the calling document's sandbox flags. 

Need to look and see what other interfaces are used with LocationSetterGuts<>
Looks like sometimes Interface can be nsIDOMDocument, ie LocationSetterGuts<nsIDOMDocument> 

This corresponds to the case where a sandboxed iframe does 'window.top.document.location = <some url>' - note that the sandboxed iframe needs 'allow-scripts' AND 'allow-same-origin' to be able to write to 'window.top.document.location'. 

Writing to 'window.top.document.location' with 'allow-scripts 'allow-same-origin' should also not be allowed unless 'allow-top-navigation' is also present. 

I think I can give a patch a go now (including tests of course). I'll take a pass through the MS test drive once I get a patch working as well.
> when will we not have a JSContext here ?

When you're not called from script.

> it seems we could use nsLocation::GetSourceDocument and the JSContext to check the
> sandbox flags of the caller

Which "caller" do you want?  The script entry point, or the script that actually did the location set?
(In reply to Boris Zbarsky (:bz) from comment #14)
> > when will we not have a JSContext here ?
> 
> When you're not called from script.

ok, seems like that's a case we don't need to handle to fix this then. 
 
> > it seems we could use nsLocation::GetSourceDocument and the JSContext to check the
> > sandbox flags of the caller
> 
> Which "caller" do you want?  The script entry point, or the script that
> actually did the location set?

i'm not sure exactly what you mean by script entry point - what we want here is the sandbox flags of the document containing the script that's trying to change the location of top, which sounds like the latter ?
"script entry point" is the scenario where code running in window A has a reference to window B and looks like this:

  B.someFunction();

and someFunction (defined in window B) does a location set on some third window C.

In that case, the script entry point (which is what the window.location code uses to get the base URI in the code you were looking at) is window A, but the location set is being done by window B.
(In reply to Boris Zbarsky (:bz) from comment #16)
> "script entry point" is the scenario where code running in window A has a
> reference to window B and looks like this:
> 
>   B.someFunction();
> 
> and someFunction (defined in window B) does a location set on some third
> window C.
> 
> In that case, the script entry point (which is what the window.location code
> uses to get the base URI in the code you were looking at) is window A, but
> the location set is being done by window B.

ok, i'm pretty sure that in your case we want to check the document in window B's sandbox flags - so nsLocation::GetSourceDocument is not the one I want... how do I get window B's document in this case ?
Good question.  bholley, what's the right way to go from a compartment (or more precisely a cx in a particular compartment) to the corresponding nsIDocument?
Adding Bobby, but he's apparently on vacation 8/24 - 9/3

i'm a little confused - the JSContext in your case above will be in window B's compartment ? but GetSourceDocument returns the document for window A from this context ?
> the JSContext in your case above will be in window B's compartment 

Yes.  The compartment indicates which code is running on the context right now.

> but GetSourceDocument returns the document for window A from this context

Yes; it doesn't go via the compartment, but instead via the context private.
(In reply to Boris Zbarsky (:bz) from comment #20)
> > the JSContext in your case above will be in window B's compartment 
> 
> Yes.  The compartment indicates which code is running on the context right
> now.
> 
> > but GetSourceDocument returns the document for window A from this context
> 
> Yes; it doesn't go via the compartment, but instead via the context private.

thanks for the explanation ! so ok, once i learn how i can get to the nsIDocument from the JSContext, i think i can write this.
i can see how to get principals from compartments and how to get the compartment from the context.. but not sure there's a way back to the document via the compartment.
Adding Blake in case he knows a good answer to this question, I know he's off at a work week though :)
(In reply to Boris Zbarsky (:bz) from comment #18)
> Good question.  bholley, what's the right way to go from a compartment (or
> more precisely a cx in a particular compartment) to the corresponding
> nsIDocument?

You should be able to just use JS_GetGlobalForScopeChain(cx) which gives you a JS object representing the inner window. You can get the principal from there or use GetExtantDoc(ument) to grab the document.
(In reply to Blake Kaplan (:mrbkap) from comment #24)
> (In reply to Boris Zbarsky (:bz) from comment #18)
> > Good question.  bholley, what's the right way to go from a compartment (or
> > more precisely a cx in a particular compartment) to the corresponding
> > nsIDocument?
> 
> You should be able to just use JS_GetGlobalForScopeChain(cx) which gives you
> a JS object representing the inner window. You can get the principal from
> there or use GetExtantDoc(ument) to grab the document.

Thanks Blake ! I'll try to get a first pass patch up soon.
It appears that a sandbox with the attribute 'allow-scripts' only has access to top.location (or at least to top.location.toString().
However, top.location.href does not work.

Example:

<iframe src="data:text/html,<h1>HI</h1><script>alert(top.location);</script>" sandbox="allow-scripts"></iframe>

This does example not work in Chrome.
(In reply to Frederik Braun [:freddyb] from comment #26)
> It appears that a sandbox with the attribute 'allow-scripts' only has access
> to top.location (or at least to top.location.toString().
> However, top.location.href does not work.
> 
> Example:
> 
> <iframe
> src="data:text/html,<h1>HI</h1><script>alert(top.location);</script>"
> sandbox="allow-scripts"></iframe>
> 
> This does example not work in Chrome.

Filed a separate bug: https://bugzilla.mozilla.org/show_bug.cgi?id=788295
(In reply to Devdatta Akhawe from comment #27)
> (In reply to Frederik Braun [:freddyb] from comment #26)
> > It appears that a sandbox with the attribute 'allow-scripts' only has access
> > to top.location (or at least to top.location.toString().
> > However, top.location.href does not work.
> > 
> > Example:
> > 
> > <iframe
> > src="data:text/html,<h1>HI</h1><script>alert(top.location);</script>"
> > sandbox="allow-scripts"></iframe>
> > 
> > This does example not work in Chrome.
> 
> Filed a separate bug: https://bugzilla.mozilla.org/show_bug.cgi?id=788295

ah ok - not requiring allow-same-origin is a separate bug, not a conscious choice on our part - see my comment 9 which describes this behaviour

note also that 788295 is a dupe of bug 720619 or so it seems.
Should I be CCed on either bug 788295 or 720619? For me, access is restricted to either of them.
(In reply to Frederik Braun [:freddyb] from comment #29)
> Should I be CCed on either bug 788295 or 720619? For me, access is
> restricted to either of them.

i cc'd you on 720619 :)
I tried doing this :

    JSObject* glob = JS_GetGlobalForScopeChain(cx);
    NS_ENSURE_TRUE(glob, NS_ERROR_UNEXPECTED);

    nsCOMPtr<nsPIDOMWindow> callerWin = do_QueryInterface(
      nsContentUtils::XPConnect()->
        GetNativeOfWrapper(cx, glob));

    NS_ENSURE_TRUE(callerWin, NS_ERROR_UNEXPECTED);

    nsIDocument* doc = callerWin->GetExtantDoc();
    NS_ENSURE_TRUE(doc, NS_ERROR_UNEXPECTED);

but the nsIDocument I get here is the document for the window whose location is being set, not the sandboxed document inside the iframe :(

I have code that correctly (or seems to) detect if the obj whose location is being set is top though.
(In reply to Ian Melven :imelven from comment #31)
> I tried doing this :
> 
>     JSObject* glob = JS_GetGlobalForScopeChain(cx);
>     NS_ENSURE_TRUE(glob, NS_ERROR_UNEXPECTED);
> 
>     nsCOMPtr<nsPIDOMWindow> callerWin = do_QueryInterface(
>       nsContentUtils::XPConnect()->
>         GetNativeOfWrapper(cx, glob));
> 
>     NS_ENSURE_TRUE(callerWin, NS_ERROR_UNEXPECTED);
> 
>     nsIDocument* doc = callerWin->GetExtantDoc();
>     NS_ENSURE_TRUE(doc, NS_ERROR_UNEXPECTED);

This looks roughly right, assuming that |cx| is in the compartment of the iframe when this code is running. Is it?
> This looks roughly right, assuming that |cx| is in the compartment of the iframe 

Given that the call is happening on a cross-compartment wrapper, there's a good chance cx is not in fact in that compartment.
(In reply to Boris Zbarsky (:bz) from comment #33)
> > This looks roughly right, assuming that |cx| is in the compartment of the iframe 
> 
> Given that the call is happening on a cross-compartment wrapper, there's a
> good chance cx is not in fact in that compartment.

That's how it looked with my test case. The URI for the document I got from |cx| was the URI of the parent/top document. The sandboxed document in an iframe contained within that parent/top document calls "window.top.location = 'http://mozilla.org';" in an onclick() handler for a button. I used <iframe sandbox='allow-scripts allow-same-origin'>.
So I'm back to the question from comment 17 :

is there a way to get to the document which contains the script that called window.top.location = .. from what we have inside LocationSetterGuts<nsIDOMWindow> or LocationSetterGuts<nsIDOMDocument> ? 

i could also try to move the check somewhere else..
(In reply to Ian Melven :imelven from comment #35)
> So I'm back to the question from comment 17 :
> 
> is there a way to get to the document which contains the script that called
> window.top.location = .. from what we have inside
> LocationSetterGuts<nsIDOMWindow> or LocationSetterGuts<nsIDOMDocument> ?

So, assuming everything is working correctly here, I don't think we should actually be entering |top|'s compartment. allow-same-origin is false, so we should have a cross-origin wrapper (XOW), which has Xray delegation, meaning we stay in our own compartment and do native calls. So if gdb says that cx->compartment->principals->dump() is anything other than a NullPrincipal at this point (since we use NullPrincipal to implement allow-same-origin=false sandboxes), it'd be surprising and worth looking into.

As for the implementation of our final security check. Our modern security model involves putting all (or as many as possible) security checks in our cross-compartment security wrappers, especially when it comes to cross-origin-accessible properties. See js/xpconnect/wrappers/AccessCheck.cpp:IsPermitted for our implementation of the same origin policy.

Adding a dynamic check in IsPermitted might be unacceptable performance-wise, so we'll probably want to compute a separate kind of wrapper (in WrapperFactory::ReWrap) that denies access to History.* and Location.* for the relevant windows.
(In reply to Ian Melven :imelven from comment #8)
> Adding dveditz to ask if he agrees with sec-high

Sorry I missed this in my deluge of vacation bugmail. I don't see how allowing top navigation is a "sec-high" kind of bug.

From https://wiki.mozilla.org/Security_Severity_Ratings

  Obtain confidential data from other sites the user is visiting
  or the local machine, or inject data or code into those sites,
  requiring no more than normal browsing actions. Indefinite DoS
  of the user's system, requiring OS reinstallation or extensive
  cleanup. Exploitable web vulnerabilities that can lead to the
  targeted compromise of a small number of users.

Unless there's some really ugly compartment compromise that's underlying the symptoms here, simply not complying with this aspect of the spec is a relatively minor problem. Malicious framed content could maybe DOS the framing page or do a spoof for users who don't check the URL bar but that's about all I can think of off the top of my head.
Keywords: sec-high
(In reply to Daniel Veditz [:dveditz] from comment #41)
> ....simply not complying with this aspect of the spec is a
> relatively minor problem. Malicious framed content could maybe DOS the
> framing page or do a spoof for users who don't check the URL bar but that's
> about all I can think of off the top of my head.

An evil frame can elevate its rights from "no forms, no plugins, no ..." to everything, by busting out of the frame, i.e. `top.location = location.href`.
(In reply to Frederik Braun [:freddyb] from comment #42)
> (In reply to Daniel Veditz [:dveditz] from comment #41)
> > ....simply not complying with this aspect of the spec is a
> > relatively minor problem. Malicious framed content could maybe DOS the
> > framing page or do a spoof for users who don't check the URL bar but that's
> > about all I can think of off the top of my head.
> 
> An evil frame can elevate its rights from "no forms, no plugins, no ..." to
> everything, by busting out of the frame, i.e. `top.location = location.href`.

It needs 'allow-same-origin' and 'allow-scripts' to be able to do this (modulo the bug mentioned in comment 28 when top.location can be accessed cross domain) - it can just remove its sandbox attribute and reload itself in this case as well.

The security issue here is that even with allow-same-origin and allow-scripts, changing window.top.location should not be allowed (per the HTML5 spec). As Dan said, it seems like it's mostly limited to a DOS or a spoof/phish. With the caveat above about being able to remove the sandbox attribute (which requires a navigation to take effect), this does seem a little strange though...
(In reply to Ian Melven :imelven from comment #43)
> (In reply to Frederik Braun [:freddyb] from comment #42)
> > (In reply to Daniel Veditz [:dveditz] from comment #41)
> > > ....simply not complying with this aspect of the spec is a
> > > relatively minor problem. Malicious framed content could maybe DOS the
> > > framing page or do a spoof for users who don't check the URL bar but that's
> > > about all I can think of off the top of my head.
> > 
> > An evil frame can elevate its rights from "no forms, no plugins, no ..." to
> > everything, by busting out of the frame, i.e. `top.location = location.href`.
> 
> It needs 'allow-same-origin' and 'allow-scripts' to be able to do this
> (modulo the bug mentioned in comment 28 when top.location can be accessed
> cross domain) - it can just remove its sandbox attribute and reload itself
> in this case as well.
> 
> The security issue here is that even with allow-same-origin and
> allow-scripts, changing window.top.location should not be allowed (per the
> HTML5 spec).

Er, really? I believe it's quite explicitly allowed under the same-origin policy. window.top gives you a ref to the top-level Window, and cross origin sets to Window.location are allowed.
Right. *Setting* window.location across origin is always allowed. Except within sandboxed iframes...(modulo *this* bug :))
(In reply to Bobby Holley (:bholley) from comment #45)
> See
> http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.
> html#security-window
> and
> http://www.whatwg.org/specs/web-apps/current-work/multipage/history.
> html#security-location

also see http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#sandboxLinks :

<portion about if the browsing context being navigated is not a top-level browsing context omitted>

"Otherwise, if the browsing context being navigated is a top-level browsing context, and is one of the ancestor browsing contexts of the source browsing context, and the source browsing context's Document's active sandboxing flag set has its sandboxed top-level navigation browsing context flag set, then abort these steps."

The sandboxed top-level navigation flag is set for an iframe with a sandbox attribute unless the attribute contains 'allow-top-navigation'.
Oh, I see. I think Ian was referring to sandboxed iframes in comment 43.
(In reply to Bobby Holley (:bholley) from comment #48)
> Oh, I see. I think Ian was referring to sandboxed iframes in comment 43.

yes, that's right, sorry for being unclear !
Keywords: sec-low
Discussed with dveditz, we agree this is more properly a sec-low.
(mostly a question for dveditz)

bug 793255 has been filed as a public dupe of this, can I open this up and dupe 793255 to this bug ?
(In reply to Ian Melven :imelven from comment #51)
> (mostly a question for dveditz)
> 
> bug 793255 has been filed as a public dupe of this, can I open this up and
> dupe 793255 to this bug ?

dveditz says yes :)
Group: core-security
Since this is a new feature in 17 it'd be nice to get this correct right out of the gate, but if the fix requires any tricky compartment munging it's not worth the risk.
status-firefox16: --- → unaffected
status-firefox17: --- → affected
status-firefox18: --- → affected
tracking-firefox17: --- → ?
tracking-firefox18: --- → +
Maybe you should delay the release of this feature if you're going to not follow the spec correctly. This doesn't even fix the issue for people trying to sandbox ad iFrames that hijack the top location and redirect users. Since you're saying you support the feature, people are going to expect it works correctly as other browsers already do.
Or we could ship what we have to get more eyes on it and fix this issue...
So currently, and presumably at launch, having "allow-scripts" will still allow the iframe itself to change the parent's location. The "allow-top-navigation" will only block anchor tags with target="_top", correct?

Also, why was this changed from sec-high to sec-low? There was a discussion earlier in this thread about how an iframe with "allow-scripts" can just change the "sandbox" attribute, but if the parent and the iframe have different origins this is not possible. Example being an ad iframe that wants to redirect the parent page (to a possibly malicious URL; controllable by the parent page and possibly not even user-initiated) as already mentioned.
James, please read the bug. See comments 41 through 47.
Okay, re-read the comments again and was misunderstood the first few times I read it. So the understanding of the spec is correct, it just might not make it in the 17 release if its a pain to implement. Instead of allowing all iframes to still set window.top.location, could sandboxed iframes just not allow any window.top.location changes until the "allow-top-navigation" is correctly fixed?
(In reply to James Hartig from comment #59)
> Okay, re-read the comments again and was misunderstood the first few times I
> read it. So the understanding of the spec is correct, it just might not make
> it in the 17 release if its a pain to implement. Instead of allowing all
> iframes to still set window.top.location, could sandboxed iframes just not
> allow any window.top.location changes until the "allow-top-navigation" is
> correctly fixed?

to block script changing window.top.location still requires a fix along the lines of that discussed previously in this bug - this is at the top of my list to fix, and when it's fixed, i'll pursue backporting if it looks feasible.
Assignee: nobody → imelven
tracking-firefox17: ? → +
(In reply to Frederik Braun [:freddyb] from comment #42)
> An evil frame can elevate its rights from "no forms, no plugins, no ..." to
> everything, by busting out of the frame, i.e. `top.location = location.href`.

Sure, that's status quo on the web. sandbox iframes are an attempt to mitigate that but sites can never rely solely on new mechanisms for their security because of legacy clients. sandbox iframes, x-frame-options, content-security-policy headers -- all provide defense in depth but can't be a site's primary security mechanism. Sites will still have to try their best to strip injected content off their sites and not frame untrusted 3rd party content on sensitive/spoofable pages.

In any case forms and plugins are generally accepted on the web; there's no problem with a top-level site using those things. They can be denied to an embedded frame because users might assume they are part of the top-level site they can see in the URL bar and be fooled into interacting with them in unsafe ways, not because they are inherently dangerous (in theory; in practice plugins must die--but that's for another bug).
A note that bholley and I have discussed that window.top.history should also be protected when a sandboxed document doesn't have 'allow-top-navigation'. This is the behavior in Webkit as well.
Based on feedback from Ian and the sec-low rating as well as more time needed to work on a fix for this, we're wontfixing for 17 and will look for a fix ready in time for 18 on Beta.
status-firefox17: affected → wontfix
Hey Ian,

What's the status on this?

(In reply to Ian Melven :imelven from comment #62)
> A note that bholley and I have discussed that window.top.history should also
> be protected when a sandboxed document doesn't have 'allow-top-navigation'.
> This is the behavior in Webkit as well.

Note - window.history is no longer cross-origin accessible. See bug 801576.
(In reply to Bobby Holley (:bholley) from comment #64)
> Hey Ian,
> 
> What's the status on this?
> 

thanks for checking. I've been on PTO on and off for the last several weeks but hope to get cracking on this now that's all finished. I've spent a fair bit looking at the code and reviewing our previous discussions and just need to sit down and crank out a first try asap. This and CSP 1.0 stuff are my top priorities at the moment and I really want to wrap them both up as soon as I can.

Thanks for the reminder on window.history, I was following bug 801576 :)
Attached patch an incomplete WIP (obsolete) — — Splinter Review 
This is a very incomplete WIP. 

Bobby, I got a little way through the approach we discussed. It seems like the compartment private flag and using it to select the new wrapper work the way they're intended to.

I've been focusing on the top navigation case first (since navigation is pretty much the same policy, just with a different decision on whether to use the SBXOW or not).

I'm very unsure about WrapperFactory::IsTopLevel - it seems like WrapperFactory may not be the right place to stick this and additionally it seems like i'm passing in the wrong JSContext ? (but the function is correctly saying that yes, this is a top level thing we're wrapping and it should use the SBXOW..)

Mostly I'm wondering how to implement the 'extra' check the SBXOW should do - in general, whether top navigation or regular navigation, we want to block setting window.location (or location.href or location.hash). One way to do that is to duplicate the code from AccessCheck::isCrossOriginPermitted in AccessCheck::isSandboxedCrossOriginAccessPermitted and add checks against the name, id, and action there. Another way would be to have a duplicate, slightly modified version of AccessCheck::IsPermitted that change window.location to only be R instead of RW, and remove location.href and location.hash, which would then be used by isSandboxedCrossOriginPermitted. Either way, there's a fair bit of duplicate code, I'm curious if you have a preference here ?

Another weird quirk is that with this patch, when I try my test case which does window.top.location = 'http://mozilla.com' in a button click handler, it navigates as expected (since the sandboxed cross origin policy is the same as the regular cross origin policy in this WIP), but when I then click back and go back to my test case, clicking the button does nothing...

Thoughts/feedback very welcome !
Attachment #688527 - Flags: feedback?(bobbyholley+bmo)
(In reply to Ian Melven :imelven from comment #66)

> additionally it seems like i'm passing in the wrong JSContext ? (but the 
> function is correctly saying that yes, this is a top level thing we're wrapping and it should use the SBXOW..)

Ms2ger helped me understand this a bit better by explaining there isn't a JSContext per compartment, but a JSContext whose .compartment changes.
Comment on attachment 688527 [details] [diff] [review]
an incomplete WIP

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

::: dom/base/nsGlobalWindow.cpp
@@ +224,5 @@
>  #include "nsSandboxFlags.h"
>  #include "TimeChangeObserver.h"
>  #include "nsPISocketTransportService.h"
>  #include "mozilla/dom/AudioContext.h"
> +#include "xpcprivate.h"

No! Bad! :-)

@@ +2023,5 @@
> +    
> +    // REVIEW : what if we don't have a private yet ?
> +    if (priv) {
> +      priv->isSandboxedNavigation = true;
> +    }

You should use EnsureCompartmentPrivate here. You might need to expose it in xpcpublic.h.

@@ +2032,5 @@
> +    // REVIEW : what if we don't have a private yet ?
> +    if (priv) {
> +      priv->isSandboxedTopNavigation = true;
> +    }
> +  }

Same here. You can just get the priv once, unconditionally, and then do:
priv->isX = flags & MASK_X;
priv->isY = flags & MASK_Y;

::: js/xpconnect/src/xpcprivate.h
@@ +4291,5 @@
>          : wantXrays(false)
>          , universalXPConnectEnabled(false)
>          , scope(nullptr)
> +        , isSandboxedNavigation(false)
> +        , isSandboxedTopNavigation(false)

Oh right, the struct itself is defined in xpcprivate.


I think we should do all access to the compartment private stuff in non-inline functions exposed via xpcpublic. EnableUniversalXPConnect and all that stuff should also be uninlined, because I'm sick of CAPS breaking mysteriously whenever someone updates the layout of compartment private. Can you do that as a separate patch underneath your other work?

::: js/xpconnect/wrappers/AccessCheck.h
@@ +70,5 @@
> +// document's sandboxing flags into account when deciding whether to allow access
> +// to certain properties.
> +struct SandboxedCrossOriginAccessiblePropertiesOnly : public Policy {
> +    static bool check(JSContext *cx, JSObject *wrapper, jsid id, js::Wrapper::Action act) {
> +        // TODO : the special sandbox handling

I think the simplest approach here is just to filter out the extra stuff that's not allowed in the sandboxed case, and fall through to AccessCheck::isCrossOriginAccessPermitted.

Effectively, we want to deny access to all the cross-origin-accessible Location properties, right? In that case, we can just determine whether js::GetObjectClaass(js::Wrapper::wrappedObject(wrapper))->name is "Location", and if so, deny access.

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +449,5 @@
>                                          CrossOriginAccessiblePropertiesOnly>::singleton;
>          } else if (type == XrayForDOMObject) {
>              wrapper = &FilteringWrapper<SecurityXrayDOM,
>                                          CrossOriginAccessiblePropertiesOnly>::singleton;
>          } else {

Can we just do EnsureCompartmentPrivate for targetData and get rid of all the checks for null targetData?

@@ +450,5 @@
>          } else if (type == XrayForDOMObject) {
>              wrapper = &FilteringWrapper<SecurityXrayDOM,
>                                          CrossOriginAccessiblePropertiesOnly>::singleton;
>          } else {
> +            // If we are wrapping the object to the compartment of a sandboxed document,

"for the compartment"

@@ +457,5 @@
> +            if (targetdata && targetdata->isSandboxedTopNavigation && IsTopLevel(cx, obj)) {
> +              // Sandboxed with respect to top level navigation implies the target is
> +              // also sandboxed with respect to navigation in general, since the sandboxed
> +              // navigation flag can never be opted out of and is always set for a sandboxed
> +              // document.

Then why do we check IsTopLevel here? And we do we not check IsDescendant like we do below?

@@ +460,5 @@
> +              // navigation flag can never be opted out of and is always set for a sandboxed
> +              // document.
> +              wrapper = &FilteringWrapper<SecurityXrayXPCWN,
> +                                          SandboxedCrossOriginAccessiblePropertiesOnly>::singleton;
> +              

Whitespace noise here. Set up your editor to display trailing whitespace?

@@ +658,5 @@
>      return newSameCompartmentWrapper;
>  }
>  
> +bool
> +WrapperFactory::IsTopLevel(JSContext* cx, JSObject* obj)

What is |obj| supposed to be, exactly? In the earlier code it seems to be whatever object we're wrapping, but here it seems assumed that it's a document or window.

IMO, the best thing to do is to get the global for the compartment, QI that to nsPIDOMWindow, get the docshell, and ask whether the docshell has a parent.

@@ +692,5 @@
>  }
> +
> +bool
> +WrapperFactory::IsDescendant(JSContext * cx, JSCompartment* origin, JSCompartment* target)
> +{

Let's talk on IRC tomorrow about the precise semantics of what we're trying to do here.
Attachment #688527 - Flags: feedback?(bobbyholley+bmo)
Un-inline some stuff as Bobby asked and add sandbox flag getters to xpcpublic.h

The previous WIP will be part 2, building on top of this.
Attachment #698203 - Flags: review?(bobbyholley+bmo)
Comment on attachment 698203 [details] [diff] [review]
part 1 - xpcpublic/xpcprivate changes

Stuff in headers is always effectively inline (at least per compilation unit), since the compilation model of C/C++ doesn't really allow for anything more sane. As such, stripping the |inline| keyword here doesn't solve the problem of files in caps having an out-of-date layout of CompartmentPrivate.

As a matter of fact though, I recently fixed this myself in some patches that are waiting for Blake's review, in bug 823348. I also refactored the hell out of all this code, so I'd suggest basing your patches on that. ;-)
Attachment #698203 - Flags: review?(bobbyholley+bmo) → review-
(In reply to Bobby Holley (:bholley) from comment #70)
> Comment on attachment 698203 [details] [diff] [review]
> part 1 - xpcpublic/xpcprivate changes
> 
> Stuff in headers is always effectively inline (at least per compilation
> unit), since the compilation model of C/C++ doesn't really allow for
> anything more sane. As such, stripping the |inline| keyword here doesn't
> solve the problem of files in caps having an out-of-date layout of
> CompartmentPrivate.

> As a matter of fact though, I recently fixed this myself in some patches
> that are waiting for Blake's review, in bug 823348. I also refactored the
> hell out of all this code, so I'd suggest basing your patches on that. ;-)

sounds good, I just imported those patches :) I took a look at your changes there wrt CompartmentPrivate and see what you mean about the inlining, sorry about that.
Status: NEW → ASSIGNED
Depends on: 823348
Attached patch Patch v1 (obsolete) — — Splinter Review 
Base the work for this bug on Bobby's patches in bug 823348 

As it says in ForbiddenBySandbox:
  // Navigation of a sandboxed document document is blocked.
  // Explicitly, this means setting location.href, location.hash,
  // or calling location.replace() are forbidden.

I've been thinking more about auxiliary browsing contexts and allow-popups as Bob Owen has been working on supporting allow-popups in bug 766282 - what we will need to do for that I think is track that a document was opened by a sandboxed document and should only be navigated via its opener - so maybe we need a new sandbox flag for that case : 'opened by a sandboxed document' that we can then set on the document's associated compartment and then check that and apply a SBXOW if necessary. I don't think we need to address that in this bug right now necessarily though.
Attachment #688527 - Attachment is obsolete: true
Attachment #698203 - Attachment is obsolete: true
Attachment #701398 - Flags: review?(bobbyholley+bmo)
This needs tests of course - I'll start working on those next in a separate patch.
(In reply to Ian Melven :imelven from comment #72)
> Created attachment 701398 [details] [diff] [review]
> Patch v1
> 
> Base the work for this bug on Bobby's patches in bug 823348 
> 
> As it says in ForbiddenBySandbox:
>   // Navigation of a sandboxed document document is blocked.

I just saw the doubled word there.. :)
Comment on attachment 701398 [details] [diff] [review]
Patch v1

Nice job overall - just some small stuff. :-) Sorry for the review delay.

In general, the name "Sandbox" in XPConnect refers to XPC sandboxes. Can you add
comments to all of the functions and objects with "Sandbox" in them to indicate
that they explicitly refer to an HTML5 sandboxed iframe?

This needs thorough tests.

diff --git a/dom/base/nsGlobalWindow.cpp b/dom/base/nsGlobalWindow.cpp
--- a/dom/base/nsGlobalWindow.cpp
+++ b/dom/base/nsGlobalWindow.cpp
+  // If the new document is sandboxed with respect to navigation or top navigation,
+  // we need to propagate this to the compartment's CompartmentPrivate.
+  uint32_t flags = mDoc->GetSandboxFlags();
+  if (flags) {
+    xpc::SetCompartmentSandboxedNavigation(mJSObject, flags & SANDBOXED_NAVIGATION);
+    xpc::SetCompartmentSandboxedTopNavigation(mJSObject, flags & SANDBOXED_TOPLEVEL_NAVIGATION);
+  }

This is sketchy. SetNewDocument is invoked on the outer nsGlobalWindow, rather than the inner, so mJSObject here is actually an outer window proxy. At this point it should be transplanted into the correct scope, but it would be better to use newInnerWindow->mJSObject here instead, which is a bonafide global.

diff --git a/js/xpconnect/wrappers/AccessCheck.cpp b/js/xpconnect/wrappers/AccessCheck.cpp
--- a/js/xpconnect/wrappers/AccessCheck.cpp
+++ b/js/xpconnect/wrappers/AccessCheck.cpp
@@ -195,19 +195,49 @@ IsFrameId(JSContext *cx, JSObject *obj, 
 }

+static bool
+ForbiddenBySandbox(const char *name, JSFlatString *prop, bool set)

IsForbiddenBySandbox.

+{
+  // Navigation of a sandboxed document document is blocked.
+  // Explicitly, this means setting location.href, location.hash,
+  // or calling location.replace() are forbidden.
+  size_t propLength;
+  const jschar *propChars =
+      JS_GetInternedStringCharsAndLength(JS_FORGET_STRING_FLATNESS(prop), &propLength);
+  if (!propLength)
+      return false;
+
+  if (set) {
+      if (IsWindow(name) && JS_FlatStringEqualsAscii(prop, "location"))
+		      return true;
+	    else if (IsLocation(name) && (JS_FlatStringEqualsAscii(prop, "hash") || JS_FlatStringEqualsAscii(prop, "href")))
+		      return true;
+  }
+  else if (IsLocation(name) && JS_FlatStringEqualsAscii(prop, "replace"))
+      return true;
+  else
+    return false;
+}

Can we just put this right underneath IsPermitted and use the same machinery,
rather than hand-rolling all this logic? That would also get rid of IsLocation.

+
 bool
 AccessCheck::isCrossOriginAccessPermitted(JSContext *cx, JSObject *wrapper, jsid id,
-                                          Wrapper::Action act)
+                                          Wrapper::Action act, bool sandboxed)
 {

How about embedding factoring out the calculation of |name| into a helper, moving
the IsForbiddenBySandbox directly into the Policy, and dropping the bool param
here? We should avoid nameless custom bool params whenever possible.

diff --git a/js/xpconnect/wrappers/WrapperFactory.cpp b/js/xpconnect/wrappers/WrapperFactory.cpp

     // This is a security wrapper. Use the security versions and filter.
-    if (xrayType == XrayForWrappedNative)
+    if (xrayType == XrayForWrappedNative) {
+        // If we are wrapping the object for the compartment of a sandboxed document,
+        // we need to use a special security wrapper that applies a
+        // more restrictive cross-origin policy.
+        if (sandboxed) {
+            return &FilteringWrapper<SecurityXrayXPCWN,
+                                     SandboxedCrossOriginAccessiblePropertiesOnly>::singleton;
+        }

This is enough of a special case that it doesn't belong in SelectWrapper, IMO.
Can we hoist it to the "special cases" section?

@@ -419,21 +430,29 @@ WrapperFactory::Rewrap(JSContext *cx, JS
         // caller and privacy to the callee.
         bool wantXrays = !(sameOrigin && !targetdata->wantXrays);
 
         // If Xrays are warranted, the caller may waive them for non-security
         // wrappers.
         bool waiveXrays = wantXrays && !securityWrapper &&
                           (flags & WAIVE_XRAY_WRAPPER_FLAG);
 
-        wrapper = SelectWrapper(securityWrapper, wantXrays, xrayType, waiveXrays);
+        // If we are wrapping the object for the compartment of a sandboxed document,
+        // in certain cases, we need to use a special wrapper that will account
+        // for the sandboxing.
+        bool sandboxed = ((targetdata->isSandboxedNavigation &&
+                           !IsTopLevel(cx,obj) &&

Nit: (cx, obj)

+                           !IsDescendant(cx, obj, target)) ||
+                          (targetdata->isSandboxedTopNavigation &&
+                           IsTopLevel(cx, obj)));
+
+        wrapper = SelectWrapper(securityWrapper, wantXrays, xrayType, waiveXrays,
+                                sandboxed);
     }
 
-
-
     // If the prototype of a chrome object being wrapped in content is a prototype
     // for a standard class, use the one from the content compartment so
     // that we can safely take advantage of things like .forEach().
     //
     // If the prototype chain of chrome object |obj| looks like this:
     //
     // obj => foo => bar => chromeWin.StandardClass.prototype
     //
@@ -594,16 +613,84 @@ WrapperFactory::WrapForSameCompartmentXr
 
 bool
 WrapperFactory::XrayWrapperNotShadowing(JSObject *wrapper, jsid id)
 {
     ResolvingId *rid = ResolvingId::getResolvingIdFromWrapper(wrapper);
     return rid->isXrayShadowing(id);
 }
 
+bool
+WrapperFactory::IsTopLevel(JSContext *cx, JSObject *obj)
+{
+// REVIEW : what to do here if any of these calls return nullptr
+  JSAutoCompartment ac(cx, obj);
+  JSObject * global = JS_GetGlobalForObject(cx, obj);

This will never return null.

+
+  if (global) {
+    nsISupports * xpcomObj = nsXPConnect::GetXPConnect()->GetNativeOfWrapper(cx, global);
+

This will return null if the global isn't backed by a DOM object, which can
happen for xpc sandboxes and such. Just returning false in that case is fine.
Avoid the extra indentation.

+    if (xpcomObj) {
+      nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(xpcomObj);

ditto.

+      if (window) {
+        nsCOMPtr <nsIDocShell> docShell = window->GetDocShell();
+        if (docShell) {

This _maybe_ could happen, but only in exceptional cases. Use
NS_ENSURE_TRUE(docShell, false) so that we get a warning if it happens.

+          nsCOMPtr<nsIDocShellTreeItem> treeItem = do_QueryInterface(docShell);
+          nsCOMPtr<nsIDocShellTreeItem> parent;
+          treeItem->GetSameTypeParent(getter_AddRefs(parent));
+          return (parent == nullptr);

Just |return !parent;|

+bool
+WrapperFactory::IsDescendant(JSContext *cx, JSObject *obj, JSCompartment *target)
+{
+  // REVIEW : what to do here if any of these calls return nullptr, particularly
+  // JS_GetGlobalForCompartmentOrNull

It seems like that shouldn't be able to happen, because otherwise the |parent|
arg to Rewrap would be nonsensical. But a quick scan doesn't reveal any reason
why it can't, so just NS_ENSURE_TRUE on the resulting global.


+  // use js::GetContextCompartment(cx) here, since that's how ::Rewrap gets target
+  // and passes it to this method anyways...

Explicit is better, I think.

+
+  // This checks whether compartment origin's window is a descendant of compartment
+  // target's window.
+  JSObject *globalForTarget = JS_GetGlobalForCompartmentOrNull(cx, target);
+  nsISupports * xpcomObjTarget = nsXPConnect::GetXPConnect()->GetNativeOfWrapper(cx, globalForTarget);
+
+  JSAutoCompartment ac(cx, obj);
+  JSObject *globalForOrigin = JS_GetGlobalForObject(cx, obj);
+
+  nsISupports * xpcomObjOrigin = nsXPConnect::GetXPConnect()->GetNativeOfWrapper(cx, globalForOrigin);
+
+  nsCOMPtr<nsPIDOMWindow> windowTarget = do_QueryInterface(xpcomObjTarget);
+  nsCOMPtr<nsPIDOMWindow> windowOrigin = do_QueryInterface(xpcomObjOrigin);

return false if any of these are null.


+  nsCOMPtr <nsIDocShell> docShellTarget = windowTarget->GetDocShell();
+  nsCOMPtr <nsIDocShell> docShellOrigin = windowOrigin->GetDocShell();

NS_ENSURE_TRUE for these.

+
+  nsCOMPtr<nsIDocShellTreeItem> treeItemTarget = do_QueryInterface(docShellTarget);
+  nsCOMPtr<nsIDocShellTreeItem> treeItemOrigin = do_QueryInterface(docShellOrigin);

These should never fail.


+void SetCompartmentSandboxedNavigation(JSObject *obj, bool isSandboxed)
+{
+    JSCompartment *c = js::GetObjectCompartment(obj);
+    xpc::CompartmentPrivate *priv = xpc::EnsureCompartmentPrivate(c);
+    priv->isSandboxedNavigation = isSandboxed;
 }
+
+void SetCompartmentSandboxedTopNavigation(JSObject *obj, bool isSandboxed)
+{
+    JSCompartment *c = js::GetObjectCompartment(obj);
+    xpc::CompartmentPrivate *priv = xpc::EnsureCompartmentPrivate(c);
+    priv->isSandboxedTopNavigation = isSandboxed;
+}
+}
+
Lets put these in XPCJSRuntime with the rest of the compartment private flag routines.

diff --git a/js/xpconnect/wrappers/XrayWrapper.h b/js/xpconnect/wrappers/XrayWrapper.h

+#define SBSecurityXrayXPCWN xpc::XrayWrapper<js::CrossCompartmentSecurityWrapper, xpc::XPCWrappedNativeXrayTraits>

You don't need to add this.
Attachment #701398 - Flags: review?(bobbyholley+bmo) → review-
(In reply to Bobby Holley (:bholley) from comment #76)
> Ian, any progress here?

I'm trying to finish bug 763879 so we can turn on our 1.0 spec compliant CSP stuff and then will be back to this to address your review comments, sorry this is taking so long ! :/

> Also, see https://bugzilla.mozilla.org/show_bug.cgi?id=754029

Thanks for the heads up.
Attached patch Patch v2 (obsolete) — — Splinter Review 
Address bholley's review comments.

No rush to look at this right now, Bobby - I'm working on the tests right now in a separate patch.
Attachment #701398 - Attachment is obsolete: true
Attachment #716173 - Flags: review?(bobbyholley+bmo)
(In reply to Ian Melven :imelven from comment #77)
> > Also, see https://bugzilla.mozilla.org/show_bug.cgi?id=754029
> 
> Thanks for the heads up.

Hm, I think I meant to post a different link: https://www.w3.org/Bugs/Public/show_bug.cgi?id=19893
(In reply to Bobby Holley (:bholley) from comment #79)
> (In reply to Ian Melven :imelven from comment #77)
> > > Also, see https://bugzilla.mozilla.org/show_bug.cgi?id=754029
> > 
> > Thanks for the heads up.
> 
> Hm, I think I meant to post a different link:
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=19893

ah ok, the other one did make some sense as well as it was at least partially about location.assign() vs location.replace() - i'll look at that w3 bug, thanks !
Comment on attachment 716173 [details] [diff] [review]
Patch v2

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

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +291,5 @@
> +void SetCompartmentSandboxedNavigation(JSObject *obj, bool isSandboxed)
> +{
> +    JSCompartment *c = js::GetObjectCompartment(obj);
> +    xpc::CompartmentPrivate *priv = xpc::EnsureCompartmentPrivate(c);
> +    priv->isSandboxedNavigation = isSandboxed;

This file has |using namespace xpc;|, so no need to qualify. Just do this:

EnsureCompartmentPrivate(c)->isSandboxedNavigation = isSandboxed;

@@ +298,5 @@
> +void SetCompartmentSandboxedTopNavigation(JSObject *obj, bool isSandboxed)
> +{
> +    JSCompartment *c = js::GetObjectCompartment(obj);
> +    xpc::CompartmentPrivate *priv = xpc::EnsureCompartmentPrivate(c);
> +    priv->isSandboxedTopNavigation = isSandboxed;

ditto.

::: js/xpconnect/src/xpcprivate.h
@@ +4299,5 @@
> +    // Does this compartment belong to a document that is sandboxed (by
> +    // HTML5 iframe sandbox or CSP sandbox) with respect to navigation
> +    // and/or top navigation?
> +    bool isSandboxedNavigation;
> +    bool isSandboxedTopNavigation;

Move these next to the other bools so that they're more likely to pack nicely?

::: js/xpconnect/src/xpcpublic.h
@@ +337,5 @@
>  
> +// Used for when a compartment is sandboxed with respect to (top)
> +// navigation by HTML5 iframe sandbox or CSP sandbox.
> +void SetCompartmentSandboxedNavigation(JSObject *obj, bool isSandboxed);
> +void SetCompartmentSandboxedTopNavigation(JSObject *obj, bool isSandboxed);

Let's call these "SetSandboxedNavigation" and similar, and rename the variable |obj| to |scope|. That should be descriptive enough.

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +184,5 @@
> +             PROP('t', R("top"))
> +             PROP('w', R("window")))
> +    }
> +    return false;
> +}

I don't think we want any duplication of our implementation of the same-origin policy - it's a total footgun if we ever make policy changes (which we've been doing recently).

In my last review comments, I was suggesting using the same machinery to implement the extra sandbox checks, rather than reimplementing the entire policy check.

@@ +228,5 @@
>      return name[0] == 'W' && !strcmp(name, "Window");
>  }
>  
> +static const char*
> +GetNameForObjectClass(JSObject *obj)

DOMClassName(JSObject *obj)

@@ +240,5 @@
> +}
> +
> +bool
> +AccessCheck::isPermittedBySandbox(JSContext *cx, JSObject *wrapper, jsid id,
> +                                  Wrapper::Action act)

My suggestion was to create an IsForbiddenBySandbox that just checks against the extra properties that are inaccessible under a sandbox, and to move that check into the Sandbox policy. So the regular policy just consists of isCrossOriginAccessPermitted, whereas the sandbox policy would be:

return isCrossOriginAccessPermitted && !isForbiddenBySandbox.
Attachment #716173 - Flags: review?(bobbyholley+bmo) → review-
Attached patch Tests v1 (obsolete) — — Splinter Review 
A first cut at some tests.

These follow the pattern Bob is using in bug 838692, which tries to reduce any possible raciness.

I'm sure there are more cases I should add to these, suggestions very welcome.
Attachment #717398 - Flags: review?(bobbyholley+bmo)
Attached patch Tests v2 (obsolete) — — Splinter Review 
Clean up some silliness (line endings and whitespace)
Attachment #717398 - Attachment is obsolete: true
Attachment #717398 - Flags: review?(bobbyholley+bmo)
Attachment #717400 - Flags: review?(bobbyholley+bmo)
Attached patch Patch v3 (obsolete) — — Splinter Review 
Address review comments.

This should be closer, Bobby. Your points from the last review made a lot of sense especially the bit about duplicating the whole policy, thank you !
Attachment #716173 - Attachment is obsolete: true
Attachment #717431 - Flags: review?(bobbyholley+bmo)
Comment on attachment 717400 [details] [diff] [review]
Tests v2

Asking Bob to take a look at these as well, as he's been deep in iframe sandbox tests recently :)
Attachment #717400 - Flags: feedback?(bobowencode)
Comment on attachment 717431 [details] [diff] [review]
Patch v3

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

Getting there :-)

::: js/xpconnect/src/XPCJSRuntime.cpp
@@ +290,5 @@
> +// HTML5 iframe sandbox or CSP sandbox.
> +void SetSandboxedNavigation(JSObject *scope, bool isSandboxed)
> +{
> +    JSCompartment *c = js::GetObjectCompartment(scope);
> +    EnsureCompartmentPrivate(c)->isSandboxedNavigation = isSandboxed;

Actually, you can just do EnsureCompartmentPrivate(scope)->isSandboxedNavigation = isSandboxed;

@@ +296,5 @@
> +
> +void SetSandboxedTopNavigation(JSObject *scope, bool isSandboxed)
> +{
> +    JSCompartment *c = js::GetObjectCompartment(scope);
> +    EnsureCompartmentPrivate(c)->isSandboxedTopNavigation = isSandboxed;

same here.

::: js/xpconnect/wrappers/AccessCheck.cpp
@@ +172,5 @@
> +bool
> +AccessCheck::isForbiddenBySandbox(JSObject *wrapper, jsid id, Wrapper::Action act)
> +{
> +    if (!XPCWrapper::GetSecurityManager())
> +        return true;

I don't think this is necessary.

@@ +175,5 @@
> +    if (!XPCWrapper::GetSecurityManager())
> +        return true;
> +
> +    if (act == Wrapper::CALL)
> +        return true;

This is backwards

@@ +181,5 @@
> +    JSObject *obj = Wrapper::wrappedObject(wrapper);
> +
> +    const char *name = DOMClassName(obj);
> +
> +    bool set = act == Wrapper::SET;

Nit: compute these both (|name| and |set|) under the if (), without linebreaks, in the order that they're passed as parameters to IsPermitted.

@@ +190,5 @@
> +        size_t propLength;
> +        const jschar *propChars =
> +            JS_GetInternedStringCharsAndLength(JS_FORGET_STRING_FLATNESS(prop), &propLength);
> +        if (!propLength)
> +            return false;

I'd return true here.

::: js/xpconnect/wrappers/AccessCheck.h
@@ +99,5 @@
> +        return AccessCheck::isCrossOriginAccessPermitted(cx, wrapper, id, act) &&
> +               !AccessCheck::isForbiddenBySandbox(wrapper, id, act);
> +    }
> +    static bool deny(JSContext *cx, jsid id, js::Wrapper::Action act) {
> +        AccessCheck::deny(cx, id);

This line needs to go away since bug 836301 landed. See how the other policies look now.

::: js/xpconnect/wrappers/WrapperFactory.cpp
@@ +393,5 @@
> +    // we need to use a special wrapper that will account for the sandboxing
> +    // by using a more restrictive cross domain policy.
> +    // flags set on the document.
> +    } else if ((targetdata->isSandboxedNavigation &&
> +                !IsTopLevel(cx, obj) && IsDescendant(cx, obj, target)) ||

Shouldn't this be !IsDescendant? Convince me.

@@ +394,5 @@
> +    // by using a more restrictive cross domain policy.
> +    // flags set on the document.
> +    } else if ((targetdata->isSandboxedNavigation &&
> +                !IsTopLevel(cx, obj) && IsDescendant(cx, obj, target)) ||
> +               (targetdata->isSandboxedTopNavigation && IsTopLevel(cx, obj))) {

In general, this logic doesn't make a lot of sense to me. Here's how I see it.

sandboxed = false;
if (isSandboxedNavigation && !isDescendant(cx, obj, target)) {
  if (IsTopLevel(cx, obj) && (target descends from obj))
    sandboxed = !isSandboxedTopNavigation;
  else
    sandboxed = true;
}

Does that seem right?

@@ +396,5 @@
> +    } else if ((targetdata->isSandboxedNavigation &&
> +                !IsTopLevel(cx, obj) && IsDescendant(cx, obj, target)) ||
> +               (targetdata->isSandboxedTopNavigation && IsTopLevel(cx, obj))) {
> +        wrapper = &FilteringWrapper<SecurityXrayXPCWN,
> +                                   SandboxedCrossOriginAccessiblePropertiesOnly>::singleton;

So this is going to break as soon as you have a non-XPCWN DOM object wrapped in a sandboxed situation.

I think the best strategy is to compute a |sandboxedPolicy| boolean right where we compute |wantXrays| and everything, and then pass that boolean to SelectWrapper. This will involve making both DOM and XPCWN versions of SBXOW btw.

(And yes, I'm aware that I originally said to hoist this out of SelectWrapper. But that was more about the computation of IsDescendant and such).

@@ +609,5 @@
> +bool
> +WrapperFactory::IsTopLevel(JSContext *cx, JSObject *obj)
> +{
> +  JSAutoCompartment ac(cx, obj);
> +  JSObject * global = JS_GetGlobalForObject(cx, obj);

Nit: Don't use the wishy-washy star. Bind it right for XPConnect, which uses js-style.

@@ +612,5 @@
> +  JSAutoCompartment ac(cx, obj);
> +  JSObject * global = JS_GetGlobalForObject(cx, obj);
> +
> +  nsISupports * xpcomObj = nsXPConnect::GetXPConnect()->GetNativeOfWrapper(cx, global);
> +

Nit: no space between the function call and the error check. This happens in a few places. Also, when using XPCOM-y things, NS_ENSURE_* is preferred.

@@ +616,5 @@
> +
> +  if (!xpcomObj)
> +    return false;
> +
> +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(xpcomObj);

You can fold this all into one call:

nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(nsXPCOnnect::GetXPConnect()->GetNativeOfWrapper(cx, global));
NS_ENSURE_TRUE(window, false).

@@ +644,5 @@
> +  JSAutoCompartment ac(cx, obj);
> +  JSObject *globalForOrigin = JS_GetGlobalForObject(cx, obj);
> +  if (!globalForOrigin)
> +    return false;
> +

JS_GetGlobalForObject is infallible.

@@ +650,5 @@
> +  if (!xpcomObjOrigin)
> +    return false;
> +
> +  nsCOMPtr<nsPIDOMWindow> windowTarget = do_QueryInterface(xpcomObjTarget);
> +  nsCOMPtr<nsPIDOMWindow> windowOrigin = do_QueryInterface(xpcomObjOrigin);

As above, lets fold these QIs into the same line as the GetNativeOfWrapper call. Might be worth doing nsIXPConnect *xpc = nsXPConnect::GetXPConnect() at the top of the function to make that stuff a little shorter.

@@ +662,5 @@
> +  NS_ENSURE_TRUE(docShellTarget, false);
> +  NS_ENSURE_TRUE(docShellOrigin, false);
> +
> +  nsCOMPtr<nsIDocShellTreeItem> treeItemTarget = do_QueryInterface(docShellTarget);
> +  nsCOMPtr<nsIDocShellTreeItem> treeItemOrigin = do_QueryInterface(docShellOrigin);

Just do treeItemTarget = do_QueryInterface(windowTarget->GetDocShell());

etc. The idea is to coalesce null checks whenever possible to make the code more readable. And you can always pass null to QI.

@@ +672,5 @@
> +
> +    if (parent == treeItemTarget)
> +      return true;
> +
> +    current = parent;

I would nix both of these newlines.

@@ +745,5 @@
>      return newSameCompartmentWrapper;
>  }
>  
>  }
> +

Nit - unnecessary whitespace change here.
Attachment #717431 - Flags: review?(bobbyholley+bmo) → review-
As discussed in [1], this is going to require a different approach :-(

https://www.w3.org/Bugs/Public/show_bug.cgi?id=19662
Comment on attachment 717400 [details] [diff] [review]
Tests v2

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

This seems to cover all the blocking tests well.

Over suggestions for more tests, maybe:
* check that location.replace() works with allow-top-navigation
* checks that things are not blocked on child browsing contexts
* checks that properties allowed when allow-same-origin specified are not blocked
* check that location.assign() and location.reload() are blocked if allow-same-origin specified

Also, I noticed that the sandboxed iframe can use window.top.close() to close the window without allow-top-navigation, which doesn't feel right.

Cheers,
Bob

::: content/html/content/test/file_iframe_sandbox_h_if1a.html
@@ +7,5 @@
> +</head>
> +<script>
> +window.addEventListener("message", receiveMessage, false);
> +function receiveMessage(msg) {
> +  window.opener.postMessage(msg.data, "*");

Just a thought: As the "b" files are unlikely to be used in other tests they could just use window.top.opener.postMessage().
It wouldn't fall foul of the test suite problem as these run in their own window.

::: content/html/content/test/file_iframe_sandbox_h_if1b.html
@@ +6,5 @@
> +  <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +</head>
> +<script>
> +function go() {
> +  window.parent.postMessage({type: "attempted"}, "*");

It probably doesn't really matter, but I think it makes more sense to call this after the test - repeated in other tests.

@@ +7,5 @@
> +</head>
> +<script>
> +function go() {
> +  window.parent.postMessage({type: "attempted"}, "*");
> +  window.top.location = 'file_iframe_sandbox_top_navigation_fail.html';

You could use try...catch here to create a positively passing test.
You'll need to make sure you pass "addToAttempted: false" to the "ok" postMessage, to prevent double counting.
Again applies to most of the other tests.

::: content/html/content/test/file_iframe_sandbox_h_if5b.html
@@ +10,5 @@
> +  window.parent.postMessage({type: "attempted"}, "*");
> +  window.top.location.hash = 'sausages';
> +  // The navigation should have been blocked.
> +  var result = window.top.location.hash != 'sausages';
> +  window.parent.postMessage({type: "ok", ok: result, desc: "navigating a top level window from a sandboxed document using window.top.location.hash should not be allowed" +

The test doesn't get this far as the attempted access to hash throws an error.
As before, a try...catch will work.

::: content/html/content/test/file_iframe_sandbox_top_navigation_fail.html
@@ +6,5 @@
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>  </head>
>  <script>
>  function doStuff() {
> +	window.opener.postMessage({type:"ok", ok: false, desc:  "top navigation should NOT be allowed by a document sandboxed without 'allow-top-navigation'"}, "*");

Nit: existing stray tab.
Also, this needs to have ", addToAttempted: false" added to it to prevent double counting.

@@ -7,5 @@
>  </head>
>  <script>
>  function doStuff() {
> -	window.opener.postMessage({ok: false, desc:  "top navigation should NOT be allowed by a document sandboxed without 'allow-top-navigation'"}, "*");
> -	window.close();

I think the close can stay.  In case it is being used in a test that isn't using the same window closing pattern.

::: content/html/content/test/file_iframe_sandbox_top_navigation_pass.html
@@ +6,5 @@
>    <link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
>  </head>
>  <script>
>  function doStuff() {
> +	window.opener.postMessage({type: 'ok', ok: true, desc:  "top navigation should be allowed by a document sandboxed with 'allow-top-navigation'"}, "*");

Nit: existing stray tab.

::: content/html/content/test/test_iframe_sandbox_navigation_script.html
@@ +8,5 @@
> +<head>
> +  <meta charset="utf-8">
> +  <title>Test for Bug 785310 - iframe sandbox script navigation</title>
> +  <script type="application/javascript" src="/tests/SimpleTest/SimpleTest.js"></script>
> +	<script type="text/javascript" src="/tests/SimpleTest/EventUtils.js"></script>

Nit: stray tab.

@@ +16,5 @@
> +/** Test for Bug 341604 - html5 sandboxed iframe should not be able to perform **/
> +/** top navigation with scripts allowed **/
> +
> +SimpleTest.waitForExplicitFinish();
> +// a postMessage handler that is used by sandboxed iframes to 

Nit: trailing whitespace.

@@ +25,5 @@
> +
> +var testPassesReceived = 0;
> +
> +function receiveMessage(event) {
> +  if (event.data.type == "ok") 

Nit: trailing whitespace.

@@ +26,5 @@
> +var testPassesReceived = 0;
> +
> +function receiveMessage(event) {
> +  if (event.data.type == "ok") 
> +    ok_wrapper(event.data.ok, event.data.desc);

This needs to pass event.data.addToAttempted as well.
In the other tests, I'm actually starting to always try and call testAttempted separately and not do it via ok_wrapper.
It makes it easier to stop it from being called twice on a test and things ending too soon.
I only really added it to ok_wrapper, so I didn't have to change all the existing navigation tests.
I've pinched the message type idea in the changes for 766282 ... much nicer than the way I was doing it.

@@ +56,5 @@
> +  attemptedTests++;
> +  if (attemptedTests == totalTestsToAttempt) {
> +    // Make sure all tests have had a chance to complete.
> +    setTimeout(function() {finish();}, 1000);
> +  } 

Nit: trailing whitespace.

@@ +77,5 @@
> +function doTest() {
> +  // 1) A new window is opened, the document contains a sandboxed iframe with 'allow-scripts',
> +  // the sandboxed iframe tries to navigate its top level parent via setting window.top.location, which should be blocked
> +  windowsToClose.push(window.open("file_iframe_sandbox_h_if1a.html"));
> +  

Nit: trailing whitespace.

@@ +81,5 @@
> +  
> +  // 2) A new window is opened, the document contains a sandboxed iframe with 'allow-scripts',
> +  // the sandboxed iframe tries to navigate its top level parent via setting window.top.location.href, which should be blocked
> +  windowsToClose.push(window.open("file_iframe_sandbox_h_if2a.html"));
> +  

Nit: trailing whitespace.

@@ +85,5 @@
> +  
> +  // 3) A new window is opened, the document contains a sandboxed iframe with 'allow-scripts',
> +  // the sandboxed iframe tries to navigate its top level parent via setting window.top.location.assign, which should be blocked
> +  windowsToClose.push(window.open("file_iframe_sandbox_h_if3a.html"));
> +  

Nit: trailing whitespace.

@@ +93,5 @@
> +
> +  // 5) A new window is opened, the document contains a sandboxed iframe with 'allow-scripts',
> +  // the sandboxed iframe tries to navigate its top level parent via setting window.top.location.hash, which should be blocked
> +  windowsToClose.push(window.open("file_iframe_sandbox_h_if5a.html"));
> +  

Nit: trailing whitespace.

@@ +95,5 @@
> +  // the sandboxed iframe tries to navigate its top level parent via setting window.top.location.hash, which should be blocked
> +  windowsToClose.push(window.open("file_iframe_sandbox_h_if5a.html"));
> +  
> +  // 6) A new window is opened, the document contains a sandboxed iframe with 'allow-scripts' and
> +  // 'allow-top-navigation', the sandboxed iframe tries to navigate its top level parent via setting window.top.location.hash, 

Nit: trailing whitespace.
Attachment #717400 - Flags: feedback?(bobowencode) → feedback+
Comment on attachment 717400 [details] [diff] [review]
Tests v2

Cancelling this review until we get the rest of this stuff sorted out.
Attachment #717400 - Flags: review?(bobbyholley+bmo)
Depends on: 859454
(In reply to Bob Owen from comment #89)
>
> Also, I noticed that the sandboxed iframe can use window.top.close() to
> close the window without allow-top-navigation, which doesn't feel right.

a note that Bob followed up on window.top.close, details are here https://www.w3.org/Bugs/Public/show_bug.cgi?id=20939#c6

and suggests we handle that in a separate bug (both for the spec and Gecko), which sounds good to me.

Discussion about which document's sandboxing flags to use for checks on script initiated things like navigating and opening new windows etc. is ongoing here : https://www.w3.org/Bugs/Public/show_bug.cgi?id=19662
rforbes-bugspam-for-setting-that-bounty-flag-20130719
Flags: sec-bounty+
Is there any status on this? This affects B2G.
(In reply to Matt Basta [:basta] from comment #93)
> Is there any status on this? This affects B2G.

No updates. I plan to look at this again when I'm done moving. I mentioned to Bob Owen that he should feel free to look at it if he likes now he's done with bug 766282 as well.
Attached patch Patch v4 (obsolete) — — Splinter Review 
(In reply to Ian Melven :imelven from comment #94)
> (In reply to Matt Basta [:basta] from comment #93)
> > Is there any status on this? This affects B2G.
> 
> No updates. I plan to look at this again when I'm done moving. I mentioned
> to Bob Owen that he should feel free to look at it if he likes now he's done
> with bug 766282 as well.

I'm in between contracts at the moment, so while I'm not clearing up in the garden for winter or doing DIY, I've had some time to look at this.


(In reply to Bobby Holley (:bholley) from comment #88)
> As discussed in [1], this is going to require a different approach :-(
> 
> https://www.w3.org/Bugs/Public/show_bug.cgi?id=19662

I'm hoping that this no longer holds since it was decided to go with incumbent script in 19662.

So, I've merged with latest changes and started to try and address review from comment 86, see below.

It needs more work, but I thought I'd ask for feedback before I go any further.

(In reply to Bobby Holley (:bholley) from comment #86)
> Comment on attachment 717431 [details] [diff] [review]
> Patch v3
> 
> Review of attachment 717431 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Getting there :-)
> 
> ::: js/xpconnect/src/XPCJSRuntime.cpp
> @@ +290,5 @@
> > +// HTML5 iframe sandbox or CSP sandbox.
> > +void SetSandboxedNavigation(JSObject *scope, bool isSandboxed)
> > +{
> > +    JSCompartment *c = js::GetObjectCompartment(scope);
> > +    EnsureCompartmentPrivate(c)->isSandboxedNavigation = isSandboxed;
> 
> Actually, you can just do
> EnsureCompartmentPrivate(scope)->isSandboxedNavigation = isSandboxed;

Done.
 
> @@ +296,5 @@
> > +
> > +void SetSandboxedTopNavigation(JSObject *scope, bool isSandboxed)
> > +{
> > +    JSCompartment *c = js::GetObjectCompartment(scope);
> > +    EnsureCompartmentPrivate(c)->isSandboxedTopNavigation = isSandboxed;
> 
> same here.

I've removed this as I'm just using the isSandboxedNavigation to see if I need to check at all.

> ::: js/xpconnect/wrappers/AccessCheck.cpp
> @@ +172,5 @@
> > +bool
> > +AccessCheck::isForbiddenBySandbox(JSObject *wrapper, jsid id, Wrapper::Action act)
> > +{
> > +    if (!XPCWrapper::GetSecurityManager())
> > +        return true;
> 
> I don't think this is necessary.

Removed.
 
> @@ +175,5 @@
> > +    if (!XPCWrapper::GetSecurityManager())
> > +        return true;
> > +
> > +    if (act == Wrapper::CALL)
> > +        return true;
> 
> This is backwards

Fixed.

> @@ +181,5 @@
> > +    JSObject *obj = Wrapper::wrappedObject(wrapper);
> > +
> > +    const char *name = DOMClassName(obj);
> > +
> > +    bool set = act == Wrapper::SET;
> 
> Nit: compute these both (|name| and |set|) under the if (), without
> linebreaks, in the order that they're passed as parameters to IsPermitted.

Done.

> @@ +190,5 @@
> > +        size_t propLength;
> > +        const jschar *propChars =
> > +            JS_GetInternedStringCharsAndLength(JS_FORGET_STRING_FLATNESS(prop), &propLength);
> > +        if (!propLength)
> > +            return false;
> 
> I'd return true here.

Done.

> ::: js/xpconnect/wrappers/AccessCheck.h
> @@ +99,5 @@
> > +        return AccessCheck::isCrossOriginAccessPermitted(cx, wrapper, id, act) &&
> > +               !AccessCheck::isForbiddenBySandbox(wrapper, id, act);
> > +    }
> > +    static bool deny(JSContext *cx, jsid id, js::Wrapper::Action act) {
> > +        AccessCheck::deny(cx, id);
> 
> This line needs to go away since bug 836301 landed. See how the other
> policies look now.

Removed.

> ::: js/xpconnect/wrappers/WrapperFactory.cpp
> @@ +393,5 @@
> > +    // we need to use a special wrapper that will account for the sandboxing
> > +    // by using a more restrictive cross domain policy.
> > +    // flags set on the document.
> > +    } else if ((targetdata->isSandboxedNavigation &&
> > +                !IsTopLevel(cx, obj) && IsDescendant(cx, obj, target)) ||
> 
> Shouldn't this be !IsDescendant? Convince me.
> 
> @@ +394,5 @@
> > +    // by using a more restrictive cross domain policy.
> > +    // flags set on the document.
> > +    } else if ((targetdata->isSandboxedNavigation &&
> > +                !IsTopLevel(cx, obj) && IsDescendant(cx, obj, target)) ||
> > +               (targetdata->isSandboxedTopNavigation && IsTopLevel(cx, obj))) {
> 
> In general, this logic doesn't make a lot of sense to me. Here's how I see
> it.
> 
> sandboxed = false;
> if (isSandboxedNavigation && !isDescendant(cx, obj, target)) {
>   if (IsTopLevel(cx, obj) && (target descends from obj))
>     sandboxed = !isSandboxedTopNavigation;
>   else
>     sandboxed = true;
> }
> 
> Does that seem right?

Yes, but I've changed it to an isSandboxedFrom function and used the IsSandboxedFrom function I created in nsDocShell for bug 766282 for the main logic.
Clearly the way I've included nsDocShell.h is not good ... should I create an nsDocShellUtils similar to nsContentUtils to hold the IsSandboxedFrom static function?

> @@ +396,5 @@
> > +    } else if ((targetdata->isSandboxedNavigation &&
> > +                !IsTopLevel(cx, obj) && IsDescendant(cx, obj, target)) ||
> > +               (targetdata->isSandboxedTopNavigation && IsTopLevel(cx, obj))) {
> > +        wrapper = &FilteringWrapper<SecurityXrayXPCWN,
> > +                                   SandboxedCrossOriginAccessiblePropertiesOnly>::singleton;
> 
> So this is going to break as soon as you have a non-XPCWN DOM object wrapped
> in a sandboxed situation.
> 
> I think the best strategy is to compute a |sandboxedPolicy| boolean right
> where we compute |wantXrays| and everything, and then pass that boolean to
> SelectWrapper. This will involve making both DOM and XPCWN versions of SBXOW
> btw.
> 
> (And yes, I'm aware that I originally said to hoist this out of
> SelectWrapper. But that was more about the computation of IsDescendant and
> such).

Certainly there were other tests that were broken by these changes, but this was also because we were effectively forcing it to be cross origin when sandboxed, which is not always the case.
I've computed a sandboxed bool and passed this into SelectWrapper.
In there it selects a wrapper with just sandboxing restrictions or both cross origin and sandboxing where necessary.

I've added processing for hash and assign to isForbiddenBySandbox, for when it's used on its own.

> @@ +609,5 @@
> > +bool
> > +WrapperFactory::IsTopLevel(JSContext *cx, JSObject *obj)
> > +{
> > +  JSAutoCompartment ac(cx, obj);
> > +  JSObject * global = JS_GetGlobalForObject(cx, obj);
> 
> Nit: Don't use the wishy-washy star. Bind it right for XPConnect, which uses
> js-style.
> 
> @@ +612,5 @@
> > +  JSAutoCompartment ac(cx, obj);
> > +  JSObject * global = JS_GetGlobalForObject(cx, obj);
> > +
> > +  nsISupports * xpcomObj = nsXPConnect::GetXPConnect()->GetNativeOfWrapper(cx, global);
> > +
> 
> Nit: no space between the function call and the error check. This happens in
> a few places. Also, when using XPCOM-y things, NS_ENSURE_* is preferred.

IsTopLevel removed.

> @@ +616,5 @@
> > +
> > +  if (!xpcomObj)
> > +    return false;
> > +
> > +  nsCOMPtr<nsPIDOMWindow> window = do_QueryInterface(xpcomObj);
> 
> You can fold this all into one call:
> 
> nsCOMPtr<nsPIDOMWindow> window =
> do_QueryInterface(nsXPCOnnect::GetXPConnect()->GetNativeOfWrapper(cx,
> global));
> NS_ENSURE_TRUE(window, false).

Done.

> @@ +644,5 @@
> > +  JSAutoCompartment ac(cx, obj);
> > +  JSObject *globalForOrigin = JS_GetGlobalForObject(cx, obj);
> > +  if (!globalForOrigin)
> > +    return false;
> > +
> 
> JS_GetGlobalForObject is infallible.

Removed check.

> @@ +650,5 @@
> > +  if (!xpcomObjOrigin)
> > +    return false;
> > +
> > +  nsCOMPtr<nsPIDOMWindow> windowTarget = do_QueryInterface(xpcomObjTarget);
> > +  nsCOMPtr<nsPIDOMWindow> windowOrigin = do_QueryInterface(xpcomObjOrigin);
> 
> As above, lets fold these QIs into the same line as the GetNativeOfWrapper
> call. Might be worth doing nsIXPConnect *xpc = nsXPConnect::GetXPConnect()
> at the top of the function to make that stuff a little shorter.

Done.

> @@ +662,5 @@
> > +  NS_ENSURE_TRUE(docShellTarget, false);
> > +  NS_ENSURE_TRUE(docShellOrigin, false);
> > +
> > +  nsCOMPtr<nsIDocShellTreeItem> treeItemTarget = do_QueryInterface(docShellTarget);
> > +  nsCOMPtr<nsIDocShellTreeItem> treeItemOrigin = do_QueryInterface(docShellOrigin);
> 
> Just do treeItemTarget = do_QueryInterface(windowTarget->GetDocShell());
> 
> etc. The idea is to coalesce null checks whenever possible to make the code
> more readable. And you can always pass null to QI.

Done.

> @@ +672,5 @@
> > +
> > +    if (parent == treeItemTarget)
> > +      return true;
> > +
> > +    current = parent;
> 
> I would nix both of these newlines.

Done.

> @@ +745,5 @@
> >      return newSameCompartmentWrapper;
> >  }
> >  
> >  }
> > +
> 
> Nit - unnecessary whitespace change here.

Removed.


Limited try run:
https://tbpl.mozilla.org/?tree=Try&rev=6e8f29941883
Assignee: ian.melven → bobowencode
Attachment #717431 - Attachment is obsolete: true
Attachment #830908 - Flags: feedback?(bobbyholley+bmo)
Attached patch Tests v3 (obsolete) — — Splinter Review 
New version of tests just merged with latest changes.

I think these will need to be expanded quite a bit to try and go through all the paths in the new code.
Attachment #717400 - Attachment is obsolete: true
Comment on attachment 830908 [details] [diff] [review]
Patch v4

Sorry for the delay, this is all a little fuzzy in my brain.

If I recall correctly, the outcome of [1] was that Hixie scolded me for trying to reinvent the security characteristics of sandboxing with wrappers, because there are various situations where doing these checks at property access time results in different results than doing it at load time (such as href target="foo").

So it seems like we should ditch the security wrapper stuff and try to more-closely mimic the algorithm that the spec describes. What do you think, Bob? I'm happy to help brainstorm a strategy if this is something you're interested in digging into.

[1] https://www.w3.org/Bugs/Public/show_bug.cgi?id=19662#c12
Attachment #830908 - Flags: feedback?(bobbyholley+bmo) → feedback-
(In reply to Bobby Holley (:bholley) from comment #97)
> Comment on attachment 830908 [details] [diff] [review]
> Patch v4
> 
> Sorry for the delay, this is all a little fuzzy in my brain.

No problem, less than 48 hours doesn't seem like much of a delay to me :)

> If I recall correctly, the outcome of [1] was that Hixie scolded me for
> trying to reinvent the security characteristics of sandboxing with wrappers,
> because there are various situations where doing these checks at property
> access time results in different results than doing it at load time (such as
> href target="foo").

Ah ... I thought that all of this went away when it was decided to use the incumbent script.
There was obviously more context that I missed from the IRC conversation. 
By 'href target="foo"', I take it you mean on an anchor.
I don't quite see how this affects this bug, I believe anchors are dealt with by existing code.

> So it seems like we should ditch the security wrapper stuff and try to
> more-closely mimic the algorithm that the spec describes. What do you think,
> Bob? I'm happy to help brainstorm a strategy if this is something you're
> interested in digging into.

I do see that, while effectively these changes give the same end result, they don't follow the implementation given in the spec.
I think there are already some other differences from the spec in the way that Firefox implements the
http://www.whatwg.org/specs/web-apps/current-work/multipage/browsers.html#browsing-context-names
and
http://www.whatwg.org/specs/web-apps/current-work/multipage/history.html#navigate
algorithms, but I suppose that's no reason to perpetuate things.

So, we'll need to make sure that location.href, location.replace, etc. pass the relevant source browsing context (docshell) to the function doing the navigation.
I think this is normally in nsDocShell::InternalLoad.

I'll start looking into how we might do that.
I should have a bit more time tomorrow and at the weekend, and I'll do what I can while I'm waiting for my next contract to appear, but I should be spending more time making that happen, even if it's far less interesting :)
Depends on: 937317
Boris and I have been sprinting to get the incumbent script stuff ready - see bug 937317.
Attached patch Patch v5 - wrapper free fix. (obsolete) — — Splinter Review 
bholley - once I'd read your patch to bug 937317, the solution seemed reasonable clear in my head, so given the focus on this bug, I've jumped straight to that without any of the other refactorings I talked about.

I grabbed your changes from the last try run.
I tried to revert some of the changes to try and stop the WrapperFactory issue ... it didn't work :)

My try run: https://tbpl.mozilla.org/?tree=Try&rev=3f7ad19d6c35

Summary:

nsDocShellLoadInfo:
Added SourceDocShell so this can be passed into nsDocShell::LoadURI.

nsDocShell:
Added SourceDocShell argument to InternalLoad.
Argument (often null) passed from LoadURI (from LoadInfo), LoadErrorPage, Reload, InternalLoadEvent, LoadHistoryEntry, OnLinkClickSync and InternalLoad itself.

Removed sandbox checking from FindItemWithName and made IsSandboxedFrom a member function on nsIDocShell, so this can also be called from nsWindowWatcher.
Had a problem with SameCOMIdentity using |this|, so just simple equality operator at the moment.

Added sandboxing checks to InternalLoad when a SourceDocShell (browsing context) is passed.

nsWindowWatcher:
Added sandboxing checks to OpenWindowInternal after search for target item.
Re-used the nsPIDOMWindow parent window further down in the code as I'd already got it.

nsLocation:
In SetURI (which href, hash, replace etc. all call), get the incumbent docshell, using bholley's new function, and add into LoadInfo for nsDocShell::LoadURI.


As we discussed, I definitely think that some other work could be done in this area:
* Move the search for the target out of nsDocShell::InternalLoad
* Move the similar search out of nsWindowWatcher::OpenWindowInternal (at the moment these both run for anchors when a new window is opened, with potentially slightly different results).
* The content policy check near the top of InternalLoad looks a bit wrong for when we have a target, as it is using information from |this| and the target.
So it will only set TYPE_SUBDOCUMENT, if we are not top as well as the target.
This gets run again on the second call to InternalLoad (on the target), but I wonder if it might cause problems.
* I also think it would be nice to move the logic of the docshell tree structure into its own class (possibly two for the same type stuff).
At the moment it is distributed throughout the docshells, which means that whenever we need to find top (confusingly the root of the tree), we have to run up (er ... down) the tree. A class that held the whole structure would just know. 
The FindItemWithName logic could be moved into this or possibly other classes.
Attachment #830908 - Attachment is obsolete: true
Attachment #8333505 - Flags: feedback?(bobbyholley+bmo)
Attached patch Patch v5 - wrapper free fix. (obsolete) — — Splinter Review 
Sorry, loaded slightly earlier export of the patch by mistake.
Attachment #8333505 - Attachment is obsolete: true
Attachment #8333505 - Flags: feedback?(bobbyholley+bmo)
Attachment #8333517 - Flags: feedback?(bobbyholley+bmo)
Comment on attachment 8333517 [details] [diff] [review]
Patch v5 - wrapper free fix.

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

Nice! That's pretty fast turnaround. There's still some stuff to be worked out, but this is totally the right direction.

So yeah - my incumbent script stuff isn't really done yet, so GetIncumbentGlobal probably isn't going to work right as it stands. But your patches are using the API correctly, so that part's just for me to fix. Just don't be surprised if the tests don't have the behavior you'd expect. Actually, an interesting bonus challenge would be to see how many different ways you can make the tests fail on account of the fact that, currently, we only push things onto the Script Settings Stack when invoking WebIDL callbacks (so event listeners will do the right thing, but top-level scripts will not).

I'm a bit concerned that we're setting ourselves up for a foot-gun by making the source browsing context an explicit opt-in by the consumer, and requiring them to use nsDocShellLoadInfo correctly. Why not just check the incumbent global directly in LoadURI?

And indeed, one case is already missing - nsFrameLoader.cpp. As it stands, navigations triggered by iframe.setAttribute(src, url) will circumvent sandboxing.

I'm getting pretty tired, so I didn't really look at the nsWindowWatcher stuff.

::: docshell/base/nsDocShell.cpp
@@ +4719,5 @@
>  
>      return InternalLoad(errorPageURI, nullptr, nullptr,
>                          INTERNAL_LOAD_FLAGS_INHERIT_OWNER, nullptr, nullptr,
>                          NullString(), nullptr, nullptr, LOAD_ERROR_PAGE,
> +                        nullptr, true, NullString(), nullptr, nullptr,nullptr);

add a space between the last two nullptrs while you're here?

@@ +8863,5 @@
> +            aSourceDocShell->IsSandboxedFrom(this, &isSandboxed);
> +            if (isSandboxed) {
> +                return NS_ERROR_DOM_INVALID_ACCESS_ERR;
> +            }
> +        }

I don't understand why we need two separate calls to IsSandboxedFrom. In the case where we have a target, we just forward to the InternalLoad of the target, right? So can't we just do this check once in InternalLoad, outside all the conditional goop, after all the relevant forwarding has concluded?

@@ +9032,5 @@
>                                                aLoadType,
>                                                aSHEntry,
>                                                aFirstParty,
>                                                aSrcdoc,
> +                                              isNewWindow ? nullptr : aSourceDocShell,

Can you explain why we want to pass nullptr in the isNewWindow case here?

@@ +9115,5 @@
>              nsCOMPtr<nsIRunnable> ev =
>                  new InternalLoadEvent(this, aURI, aReferrer, aOwner, aFlags,
>                                        aTypeHint, aPostData, aHeadersData,
> +                                      aLoadType, aSHEntry, aFirstParty, aSrcdoc,
> +                                      nullptr);

Err, why don't we want to pass aSourceDocShell through here?

::: docshell/base/nsDocShellLoadInfo.cpp
@@ +211,5 @@
>     return NS_OK;
>  }
>  
> +NS_IMETHODIMP nsDocShellLoadInfo::GetSourceDocShell(nsIDocShell** aSourceDocShell)
> +{

This code is fine, but the more modern pattern would look like this:

{
MOZ_ASSERT(aSourceDocShell);
nsCOMPtr<nsIDocShell> result = mSourceDocShell;
result.forget(aSourceDocShell);
return NS_OK;
}

@@ +218,5 @@
> +   *aSourceDocShell = mSourceDocShell;
> +   NS_IF_ADDREF(*aSourceDocShell);
> +   return NS_OK;
> +}
> + 

Whitespace error.

::: docshell/base/nsIDocShell.idl
@@ +788,5 @@
>    /**
> +   * Returns true if we are sandboxed from aTargetItem.
> +   * aTargetItem - the browsing context we are attempting to navigate.
> +   */
> +  bool isSandboxedFrom(in nsIDocShellTreeItem aTargetItem);

If you like, you can mark this [noscript,notxpcom,nostdcall], which lets you use a more natural |bool IsSandboxedFrom(nsIDocShellTreeItem* aTargetItem) {}| method signature and avoid all the outparam and NS_IMETHODIMP goop (at the expense of not being able to call it from script). I tend to prefer this for code I write, but it's really up to you.

::: dom/base/nsLocation.cpp
@@ +237,5 @@
>        loadInfo->SetLoadType(nsIDocShellLoadInfo::loadStopContent);
>      }
>  
> +    // Get the incumbent script's browsing context to set as source.
> +    nsCOMPtr<nsPIDOMWindow> sourceWindow = 

whitespace error.
Attachment #8333517 - Flags: feedback?(bobbyholley+bmo) → feedback+
(In reply to Bobby Holley (:bholley) from comment #102)
> Comment on attachment 8333517 [details] [diff] [review]
> Patch v5 - wrapper free fix.
> 
> Review of attachment 8333517 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Nice! That's pretty fast turnaround. There's still some stuff to be worked
> out, but this is totally the right direction.

Thanks.
 
> Actually, an interesting bonus challenge would be to see how many different
> ways you can make the tests fail on account of the fact that, currently, we
> only push things onto the Script Settings Stack when invoking WebIDL
> callbacks (so event listeners will do the right thing, but top-level scripts
> will not).

Showing my inexperience of such things, but I'll need to read more about the difference between WebIDL callbacks and other ones, to understand these issues.
I can see how it not working for top-level scripts will cause problems and I definitely need to add more tests.
I've got some notes for starters in comment 89 from when I gave feedback.
 
> I'm a bit concerned that we're setting ourselves up for a foot-gun by making
> the source browsing context an explicit opt-in by the consumer, and
> requiring them to use nsDocShellLoadInfo correctly. Why not just check the
> incumbent global directly in LoadURI?

I did think about doing that, but I didn't for a couple of reasons:

1) In bug 859454 comment 18 (and others), bz seemed to be questioning whether we would be able to get the correct script at this point. Although I assume that is partly what bug 937317 is trying to address.

2) I'm not sure the incumbent script is always the right (or at least specced) thing to be using.
Best explained after the next point ...

> And indeed, one case is already missing - nsFrameLoader.cpp. As it stands,
> navigations triggered by iframe.setAttribute(src, url) will circumvent
> sandboxing.

Yes, I meant to add that to my list (for other potential bugs/refactorings) at the bottom of comment 100 as well as to check for other things that cause navigation.

However, my reading of the spec says that navigation from changing src or srcdoc should use the iframe's document's browsing context.
http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-element.html#process-the-iframe-attributes (and couple of line above)
At the moment I don't think we do this, we use the iframe's internal window's browsing context (the target), but from a sandboxing point of view that effectively doesn't matter as they'll both always be able to navigate the target.

Now we could argue that this should be changed in the spec and always check for the incumbent script in LoadURI and use it as the source if there is one, but then I thought of this ...

If we have a sandboxed iframe A (with allow-popups/scripts) that opens a new window (via window.open) then I think the navigation for this would come through LoadURI and the incumbent script would be from A, so this would be the source.
This is the correct source and can navigate, as the one permitted sandboxed navigator.
However, if this new document also contained an iframe, then the load of this would also come through LoadURI and I'm guessing we'd still have the same incumbent script and therefore source (A) and this time the navigation would fail.

I'm not sure that this is how it would all work, but it put enough doubt in my mind over doing this implicitly in LoadURI.

Sorry rambled on a bit there, but basically I decided to put the main framework for Source Browsing Context in place.
Change everything that calls InternalLoad, so that it works as it does now.
Change nsLocation to attempt to fix this bug (as a priority) and then follow it up with other bugs where necessary.

> I'm getting pretty tired, so I didn't really look at the nsWindowWatcher
> stuff.

Thanks for taking the time on your Sunday evening to look at this.

I'm changing a broken isolation switch for a shower in a bit, so I'm going to have to power everything down, so it might be tomorrow before I get some more time on this.

> ::: docshell/base/nsDocShell.cpp
> @@ +4719,5 @@
> >  
> >      return InternalLoad(errorPageURI, nullptr, nullptr,
> >                          INTERNAL_LOAD_FLAGS_INHERIT_OWNER, nullptr, nullptr,
> >                          NullString(), nullptr, nullptr, LOAD_ERROR_PAGE,
> > +                        nullptr, true, NullString(), nullptr, nullptr,nullptr);
> 
> add a space between the last two nullptrs while you're here?

I have no idea why I didn't do that :)
I remember seeing it - there's even one character left out of the 80 to use.
I'd started later on Saturday than I'd hoped and it was getting a bit late when I was completing this (he mumbles as an excuse).

> @@ +8863,5 @@
> > +            aSourceDocShell->IsSandboxedFrom(this, &isSandboxed);
> > +            if (isSandboxed) {
> > +                return NS_ERROR_DOM_INVALID_ACCESS_ERR;
> > +            }
> > +        }
> 
> I don't understand why we need two separate calls to IsSandboxedFrom. In the
> case where we have a target, we just forward to the InternalLoad of the
> target, right? So can't we just do this check once in InternalLoad, outside
> all the conditional goop, after all the relevant forwarding has concluded?

As the check used to be in FindItemWithName, I wanted to make sure that I wasn't changing current behaviour, but as I think there are only security checks before it transfers to the InternalLoad of the target I think it can go.
I was being a bit paranoid, because I had to put the check in nsWindowWatcher::OpenWindowInternal after its version of FindItemWithName.
I was hoping to wait until it called LoadURI, but even if it finds an existing docshell it does things like set the opener (which I think is a bug anyway) before it calls LoadURI.

> @@ +9032,5 @@
> >                                                aLoadType,
> >                                                aSHEntry,
> >                                                aFirstParty,
> >                                                aSrcdoc,
> > +                                              isNewWindow ? nullptr : aSourceDocShell,
> 
> Can you explain why we want to pass nullptr in the isNewWindow case here?

If we've opened a new window then we know we can navigate it, but I should probably not second guess things like that and pass through the source and allow the sandbox check to run.

> @@ +9115,5 @@
> >              nsCOMPtr<nsIRunnable> ev =
> >                  new InternalLoadEvent(this, aURI, aReferrer, aOwner, aFlags,
> >                                        aTypeHint, aPostData, aHeadersData,
> > +                                      aLoadType, aSHEntry, aFirstParty, aSrcdoc,
> > +                                      nullptr);
> 
> Err, why don't we want to pass aSourceDocShell through here?

Because I figured that we'd already done the check once, so we wouldn't need to do it again on the next time round. Do you think I should pass it through anyway?

> ::: docshell/base/nsDocShellLoadInfo.cpp
> @@ +211,5 @@
> >     return NS_OK;
> >  }
> >  
> > +NS_IMETHODIMP nsDocShellLoadInfo::GetSourceDocShell(nsIDocShell** aSourceDocShell)
> > +{
> 
> This code is fine, but the more modern pattern would look like this:
> 
> {
> MOZ_ASSERT(aSourceDocShell);
> nsCOMPtr<nsIDocShell> result = mSourceDocShell;
> result.forget(aSourceDocShell);
> return NS_OK;
> }

OK, I'll change it.

> @@ +218,5 @@
> > +   *aSourceDocShell = mSourceDocShell;
> > +   NS_IF_ADDREF(*aSourceDocShell);
> > +   return NS_OK;
> > +}
> > + 
> 
> Whitespace error.

Sorry, I noticed this and the other one as well as indentation issues in nsLocation after I'd uploaded the patch for the second time.
I knew that I'd need to upload another one, so I thought I'd wait and not spam everyone again.

> ::: docshell/base/nsIDocShell.idl
> @@ +788,5 @@
> >    /**
> > +   * Returns true if we are sandboxed from aTargetItem.
> > +   * aTargetItem - the browsing context we are attempting to navigate.
> > +   */
> > +  bool isSandboxedFrom(in nsIDocShellTreeItem aTargetItem);
> 
> If you like, you can mark this [noscript,notxpcom,nostdcall], which lets you
> use a more natural |bool IsSandboxedFrom(nsIDocShellTreeItem* aTargetItem)
> {}| method signature and avoid all the outparam and NS_IMETHODIMP goop (at
> the expense of not being able to call it from script). I tend to prefer this
> for code I write, but it's really up to you.

I definitely do prefer ... that's the first thing I'm going to change.
I'd not done any XPCOM stuff before starting to look at Firefox in January, so I'm fairly new to it.
This outparam stuff is one of the things I really don't like.
Could I do something similar for onePermittedSanboxedNavigator that I added for bug 766282?
(In reply to Bob Owen from comment #103)
> (In reply to Bobby Holley (:bholley) from comment #102)
> Showing my inexperience of such things, but I'll need to read more about the
> difference between WebIDL callbacks and other ones, to understand these
> issues.

WebIDL callbacks are basically all the callbacks that exist from the spec perspective - event listeners, event handlers, Function, Callback objects, etc.

> > I'm a bit concerned that we're setting ourselves up for a foot-gun by making
> > the source browsing context an explicit opt-in by the consumer, and
> > requiring them to use nsDocShellLoadInfo correctly. Why not just check the
> > incumbent global directly in LoadURI?
> 
> I did think about doing that, but I didn't for a couple of reasons:
> 
> 1) In bug 859454 comment 18 (and others), bz seemed to be questioning
> whether we would be able to get the correct script at this point. Although I
> assume that is partly what bug 937317 is trying to address.
> 
> 2) I'm not sure the incumbent script is always the right (or at least
> specced) thing to be using.
> Best explained after the next point ...
> 
> > And indeed, one case is already missing - nsFrameLoader.cpp. As it stands,
> > navigations triggered by iframe.setAttribute(src, url) will circumvent
> > sandboxing.
> 
> Yes, I meant to add that to my list (for other potential bugs/refactorings)
> at the bottom of comment 100 as well as to check for other things that cause
> navigation.
> 
> However, my reading of the spec says that navigation from changing src or
> srcdoc should use the iframe's document's browsing context.
> http://www.whatwg.org/specs/web-apps/current-work/multipage/the-iframe-
> element.html#process-the-iframe-attributes (and couple of line above)
> At the moment I don't think we do this, we use the iframe's internal
> window's browsing context (the target), but from a sandboxing point of view
> that effectively doesn't matter as they'll both always be able to navigate
> the target.
> 
> Now we could argue that this should be changed in the spec and always check
> for the incumbent script in LoadURI and use it as the source if there is
> one, but then I thought of this ...
> 
> If we have a sandboxed iframe A (with allow-popups/scripts) that opens a new
> window (via window.open) then I think the navigation for this would come
> through LoadURI and the incumbent script would be from A, so this would be
> the source.
> This is the correct source and can navigate, as the one permitted sandboxed
> navigator.
> However, if this new document also contained an iframe, then the load of
> this would also come through LoadURI and I'm guessing we'd still have the
> same incumbent script and therefore source (A) and this time the navigation
> would fail.
> 
> I'm not sure that this is how it would all work, but it put enough doubt in
> my mind over doing this implicitly in LoadURI.

You are totally right. Bam.

I just talked with Hixie about this, and he convinced me that the current spec for iframe src navigation is fine. It basically boils down to the fact that the same load algorithm gets used for the programmatic (setAttribute(src, ...)) and declarative (<iframe src="">) cases. We effectively have the same situation in Gecko. So really, |win.location = foo| is the special case, because it's a path whereby script can programatically navigate a browsing context that it cannot otherwise arbitrarily configure.

So yes, doing this implicitly is wrong, and we should do it explicitly like you've done.

> Sorry rambled on a bit there

Not at all - it was very insightful. :-)


> > I'm getting pretty tired, so I didn't really look at the nsWindowWatcher
> > stuff.
> 
> Thanks for taking the time on your Sunday evening to look at this.

Thanks for taking time on your weekend to fix this bug. :-)


> 
> > @@ +8863,5 @@
> > > +            aSourceDocShell->IsSandboxedFrom(this, &isSandboxed);
> > > +            if (isSandboxed) {
> > > +                return NS_ERROR_DOM_INVALID_ACCESS_ERR;
> > > +            }
> > > +        }
> > 
> > I don't understand why we need two separate calls to IsSandboxedFrom. In the
> > case where we have a target, we just forward to the InternalLoad of the
> > target, right? So can't we just do this check once in InternalLoad, outside
> > all the conditional goop, after all the relevant forwarding has concluded?
> 
> As the check used to be in FindItemWithName, I wanted to make sure that I
> wasn't changing current behaviour, but as I think there are only security
> checks before it transfers to the InternalLoad of the target I think it can
> go.


> > >              nsCOMPtr<nsIRunnable> ev =
> > >                  new InternalLoadEvent(this, aURI, aReferrer, aOwner, aFlags,
> > >                                        aTypeHint, aPostData, aHeadersData,
> > > +                                      aLoadType, aSHEntry, aFirstParty, aSrcdoc,
> > > +                                      nullptr);
> > 
> > Err, why don't we want to pass aSourceDocShell through here?
> 
> Because I figured that we'd already done the check once, so we wouldn't need
> to do it again on the next time round. Do you think I should pass it through
> anyway?

Well, that assumes that we only use aSourceDocShell for the one indempotent security check, which might change. I think we should always pass it. Also, if we move the security check further down in the function, then we won't end up doing it twice.


> Could I do something similar for onePermittedSanboxedNavigator that I added
> for bug 766282?

For attributes, the story is even better - you can just mark it as [infallible], and then XPIDL will generate a helper C++ getter/setter for you that wraps the XPCOM version in a more idiomatic calling convention, and asserts that the return value is always NS_OK. :-)

Thanks for all your hard work on this stuff - I'll try to get the script settings stack stuff done ASAP.
Attached patch Patch v6 - wrapper free fix. (obsolete) — — Splinter Review 
(In reply to Bobby Holley (:bholley) from comment #104)
> (In reply to Bob Owen from comment #103)
> > (In reply to Bobby Holley (:bholley) from comment #102)
> WebIDL callbacks are basically all the callbacks that exist from the spec
> perspective - event listeners, event handlers, Function, Callback objects,
> etc.

Thanks.
My time's been a bit patchy over the last few days, but hopefully tomorrow I'll get onto adding more tests.
 
> I just talked with Hixie about this, and he convinced me that the current
> spec for iframe src navigation is fine. It basically boils down to the fact
> that the same load algorithm gets used for the programmatic
> (setAttribute(src, ...)) and declarative (<iframe src="">) cases. We
> effectively have the same situation in Gecko. So really, |win.location =
> foo| is the special case, because it's a path whereby script can
> programatically navigate a browsing context that it cannot otherwise
> arbitrarily configure.
> 
> So yes, doing this implicitly is wrong, and we should do it explicitly like
> you've done.

OK thanks.
Nice to get the iframe stuff confirmed and clarified as well.
I'll pick that up as another bug though I think, as it's only marginally related to this one and at least from the sandboxing point of view our behaviour is effectively the same.
 
> > > >              nsCOMPtr<nsIRunnable> ev =
> > > >                  new InternalLoadEvent(this, aURI, aReferrer, aOwner, aFlags,
> > > >                                        aTypeHint, aPostData, aHeadersData,
> > > > +                                      aLoadType, aSHEntry, aFirstParty, aSrcdoc,
> > > > +                                      nullptr);
> > > 
> > > Err, why don't we want to pass aSourceDocShell through here?
> > 
> > Because I figured that we'd already done the check once, so we wouldn't need
> > to do it again on the next time round. Do you think I should pass it through
> > anyway?
> 
> Well, that assumes that we only use aSourceDocShell for the one indempotent
> security check, which might change. I think we should always pass it. 

I agree, so I've changed this and in all the other places where I was passing null, I've changed it to |this|.
This is the right thing in all cases apart from LoadHistoryEntry, the spec says this should use the source browsing context that was used the first time the entry was created. I'm pretty sure we don't store this currently and I can see it causing some issues, so I think this needs to be another bug.

This covers all the direct callers to InternalLoad.
Once I've got this bug finished, I'll follow up with a bug(s) for the ones that call through LoadURI.
Once they were done, I was thinking it might be a good idea to add an assertion to the top of InternalLoad to make sure |aSourceDocShell| is not null.
The spec says there should always be one (even if it's us).
Also, it would stop people casually passing null (like I did :) ) and make them think about what the right thing should be.

While reading the spec, I've also realised these changes start to set things up for implementing seamless iframes in this area.

> Also,
> if we move the security check further down in the function, then we won't
> end up doing it twice.

I've moved the check to below the transfer on finding a new target, but I'm not sure we can move it to after the possible async.
I think the line |doc->TryCancelFrameLoaderInitialization(this);| before this might change things, so the check needs to be before that.

I'm not sure whether this async call relates to the async stuff from the navigate algorithm in the spec or whether we always needs to re-run the security checks above this part.
If we didn't maybe (in another bug) we could have an InternalLoadSynchronous, that does the first bits and an InternalLoadMaybeAsynchronous, that does the rest.
That way we wouldn't do everything twice.

Anyway, I've tried to make the IsSandboxedFrom checks as fast as I can, so I don't think calling them twice will make much difference in the grand scheme of things.
 
> > Could I do something similar for onePermittedSanboxedNavigator that I added
> > for bug 766282?
> 
> For attributes, the story is even better - you can just mark it as
> [infallible], and then XPIDL will generate a helper C++ getter/setter for
> you that wraps the XPCOM version in a more idiomatic calling convention, and
> asserts that the return value is always NS_OK. :-)

Thanks, I'll pick that up in another bug, although there are assertions in the setter and it uses nsWeakPtr, so that might scupper things.
Attachment #8333517 - Attachment is obsolete: true
Attachment #8335353 - Flags: review?(bobbyholley+bmo)
(In reply to Bob Owen from comment #105)

> Thanks.
> My time's been a bit patchy over the last few days, but hopefully tomorrow
> I'll get onto adding more tests.

No worries at all - I'm really the one being the long pole here. ;-)


> This is the right thing in all cases apart from LoadHistoryEntry, the spec
> says this should use the source browsing context that was used the first
> time the entry was created. I'm pretty sure we don't store this currently
> and I can see it causing some issues, so I think this needs to be another
> bug.

Ah, interesting - please file? We need to think carefully about the lifetime management there. We don't want SHEntries to start holding more stuff alive than they used to in the common case. I'm pretty sure we can manage the lifetime similarly to the way we manage the lifetime for the target docshell of the SHEntry, and reason that, at least in most cases, we won't be holding anything alive that we weren't already. But we should think it through carefully, and consult smaug. Tests for that would be crucial.

> Once they were done, I was thinking it might be a good idea to add an
> assertion to the top of InternalLoad to make sure |aSourceDocShell| is not
> null.

That sounds great.

> While reading the spec, I've also realised these changes start to set things
> up for implementing seamless iframes in this area.

Ah, interesting.


> > Also,
> > if we move the security check further down in the function, then we won't
> > end up doing it twice.
> 
> I've moved the check to below the transfer on finding a new target, but I'm
> not sure we can move it to after the possible async.
> I think the line |doc->TryCancelFrameLoaderInitialization(this);| before
> this might change things, so the check needs to be before that.

Oh, good catch. But maybe we should move the TryCancelFrameLoaderInitialization call to after the async stuff?

> I'm not sure whether this async call relates to the async stuff from the
> navigate algorithm in the spec or whether we always needs to re-run the
> security checks above this part.
> If we didn't maybe (in another bug) we could have an
> InternalLoadSynchronous, that does the first bits and an
> InternalLoadMaybeAsynchronous, that does the rest.
> That way we wouldn't do everything twice.

I think this async stuff is just related to delaying appropriately for onunload events. I'm not the expert here though.

> Anyway, I've tried to make the IsSandboxedFrom checks as fast as I can, so I
> don't think calling them twice will make much difference in the grand scheme
> of things.

Sure. It's just an issue of keeping the code maintainable and comprehensible.

> > For attributes, the story is even better - you can just mark it as
> > [infallible], and then XPIDL will generate a helper C++ getter/setter for
> > you that wraps the XPCOM version in a more idiomatic calling convention, and
> > asserts that the return value is always NS_OK. :-)
> 
> Thanks, I'll pick that up in another bug, although there are assertions in
> the setter and it uses nsWeakPtr, so that might scupper things.

Assertions in the setter are fine. In the end, it's just a question of whether there's a meaningful concept of failure that can be propagated back to the caller. If not, squelch, maybe warn, and mark it [infallible].
Comment on attachment 8335353 [details] [diff] [review]
Patch v6 - wrapper free fix.

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

Looks good, though I'm not the expert here, so smaug should look at this too.

r=bholley with comments addressed, but note that this obviously can't land until bug 937317 lands.

::: docshell/base/nsDocShell.cpp
@@ +8830,5 @@
>      if (aWindowTarget && *aWindowTarget) {
>          // Locate the target DocShell.
>          nsCOMPtr<nsIDocShellTreeItem> targetItem;
> +        FindItemWithName(aWindowTarget, nullptr, this,
> +                         getter_AddRefs(targetItem));

Again, the signature of this function should probably change to void.

@@ +11126,5 @@
>                        aLoadType,          // Load type
>                        aEntry,             // SHEntry
>                        true,
>                        srcdoc,
> +                      this,               // This needs to be same as original

This comment is a bit opaque.

::: docshell/base/nsIDocShell.idl
@@ +788,5 @@
>    /**
> +   * Returns true if we are sandboxed from aTargetItem.
> +   * aTargetItem - the browsing context we are attempting to navigate.
> +   */
> +  [noscript,notxpcom,nostdcall] bool isSandboxedFrom(in nsIDocShellTreeItem aTargetItem);

Is there any good reason for the in-param here to be nsIDocShellTreeItem? It seems like it should be nsIDocShell.

::: dom/base/nsLocation.cpp
@@ +241,5 @@
> +    nsCOMPtr<nsPIDOMWindow> sourceWindow =
> +        do_QueryInterface(mozilla::dom::GetIncumbentGlobal());
> +    if (sourceWindow) {
> +      loadInfo->SetSourceDocShell(sourceWindow->GetDocShell());
> +    }

I just realized that we won't be able to assert that the source browsing context is non-null. In the spec, scripts always have a browsing context. But in Gecko, there are lots of things like sandboxes and whatnot that don't. So we unfortunately need to handle that (generally, things that don't QI to nsIPIDOMWindow can't be sandboxed).

Maybe what we should instead is to force callers to invoke SetSourceDocShell, even if they pass null. We could have a debug-only boolean on nsDocShellLoadInfo that checks whether the source docshell was ever set, and we could assert that booleam when someone calls GetSourceDocShell.

::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
@@ +474,5 @@
>    }
>  
>    // try to find an extant window with the given name
>    nsCOMPtr<nsIDOMWindow> foundWindow;
> +  SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow));

If we don't believe this can fail any more, we should change the call signatures wherever we can, and MOZ_ASSERT(NS_SUCCEEDED(rv)) elsewhere.

@@ +478,5 @@
> +  SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow));
> +  GetWindowTreeItem(foundWindow, getter_AddRefs(newDocShellItem));
> +
> +  // Do sandbox checks here, because too much happens before the checks
> +  // following the call to nsIDocShell::LoadURI.

Can you elaborate on 'too much happens'? Does that mean that we irreversibly alter state? If so, please make that more explicit in the comment.
Attachment #8335353 - Flags: review?(bugs)
Attachment #8335353 - Flags: review?(bobbyholley+bmo)
Attachment #8335353 - Flags: review+
(In reply to Bobby Holley (:bholley) from comment #106)
> (In reply to Bob Owen from comment #105)
> > While reading the spec, I've also realised these changes start to set things
> > up for implementing seamless iframes in this area.
> 
> Ah, interesting.

It's not much, but source browsing context is needed for step 3 of the navigation algorithm.
I'm sure there's a lot more work besides. 
 
> > I think the line |doc->TryCancelFrameLoaderInitialization(this);| before
> > this might change things, so the check needs to be before that.
> 
> Oh, good catch. But maybe we should move the
> TryCancelFrameLoaderInitialization call to after the async stuff?

I'll try this, hopefully smaug will have some ideas on whether this is safe.
 

(In reply to Bobby Holley (:bholley) from comment #107)
> Comment on attachment 8335353 [details] [diff] [review]
> Patch v6 - wrapper free fix.
> 
> Review of attachment 8335353 [details] [diff] [review]:
> -----------------------------------------------------------------
> 
> Looks good, though I'm not the expert here, so smaug should look at this too.
> 
> r=bholley with comments addressed, but note that this obviously can't land
> until bug 937317 lands.

Thanks and noted.
I'll wait for smaug's comments until I upload another patch.
I'm going to start looking at tests this afternoon (hopefully).
 
> ::: docshell/base/nsDocShell.cpp
> @@ +8830,5 @@
> >      if (aWindowTarget && *aWindowTarget) {
> >          // Locate the target DocShell.
> >          nsCOMPtr<nsIDocShellTreeItem> targetItem;
> > +        FindItemWithName(aWindowTarget, nullptr, this,
> > +                         getter_AddRefs(targetItem));
> 
> Again, the signature of this function should probably change to void.

OK, this has just gone back to how it was before bug 766282.
There's a load of recursion and calls to FindItemWithName in other classes.
I'm pretty sure the top level call always returns |NS_OK|, but I think some of the recursive ones can in theory fail, although it may never happen.
Should I just add an assertion then?
Do I use DebugOnly<> for the return value?
 
> @@ +11126,5 @@
> >                        aLoadType,          // Load type
> >                        aEntry,             // SHEntry
> >                        true,
> >                        srcdoc,
> > +                      this,               // This needs to be same as original
> 
> This comment is a bit opaque.

I was trying to fit in with the other comments, but you're right it fails the "would someone with less context know what I'm taking about" test.
I'll file that bug and add a better comment above that references it.

> ::: docshell/base/nsIDocShell.idl
> @@ +788,5 @@
> >    /**
> > +   * Returns true if we are sandboxed from aTargetItem.
> > +   * aTargetItem - the browsing context we are attempting to navigate.
> > +   */
> > +  [noscript,notxpcom,nostdcall] bool isSandboxedFrom(in nsIDocShellTreeItem aTargetItem);
> 
> Is there any good reason for the in-param here to be nsIDocShellTreeItem? It
> seems like it should be nsIDocShell.

Ah, thanks.
It was a hangover from when it was called from FindItemWithName, when both parameters were tree items.
I think I'll have to add a QI above the call to it in nsWindowWatcher.
I'm hoping that call will go if I ever get round to that refactoring.
So, I think it's the right thing to do.

> ::: dom/base/nsLocation.cpp
> @@ +241,5 @@
> > +    nsCOMPtr<nsPIDOMWindow> sourceWindow =
> > +        do_QueryInterface(mozilla::dom::GetIncumbentGlobal());
> > +    if (sourceWindow) {
> > +      loadInfo->SetSourceDocShell(sourceWindow->GetDocShell());
> > +    }
> 
> I just realized that we won't be able to assert that the source browsing
> context is non-null. In the spec, scripts always have a browsing context.
> But in Gecko, there are lots of things like sandboxes and whatnot that
> don't. So we unfortunately need to handle that (generally, things that don't
> QI to nsIPIDOMWindow can't be sandboxed).
> 
> Maybe what we should instead is to force callers to invoke
> SetSourceDocShell, even if they pass null. We could have a debug-only
> boolean on nsDocShellLoadInfo that checks whether the source docshell was
> ever set, and we could assert that booleam when someone calls
> GetSourceDocShell.

Won't they always be able to pass the docshell on which they are calling LoadURI?

> ::: embedding/components/windowwatcher/src/nsWindowWatcher.cpp
> @@ +474,5 @@
> >    }
> >  
> >    // try to find an extant window with the given name
> >    nsCOMPtr<nsIDOMWindow> foundWindow;
> > +  SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow));
> 
> If we don't believe this can fail any more, we should change the call
> signatures wherever we can, and MOZ_ASSERT(NS_SUCCEEDED(rv)) elsewhere.

This can only fail since my changes for bug 766282, before that it only returned |NS_OK|.
I needed to know if FindItemWithName was returning null because it hadn't found anything or because it had, but was sandboxed.
Now that I've moved the sandbox checks out of nsDocShell::FindItemWithName, I should probably roll back these changes.
Should I just change this to void, or return the found window?

> @@ +478,5 @@
> > +  SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow));
> > +  GetWindowTreeItem(foundWindow, getter_AddRefs(newDocShellItem));
> > +
> > +  // Do sandbox checks here, because too much happens before the checks
> > +  // following the call to nsIDocShell::LoadURI.
> 
> Can you elaborate on 'too much happens'? Does that mean that we irreversibly
> alter state? If so, please make that more explicit in the comment.

Fails that test again :)
Looks like I need to go back through my comments.
Comment on attachment 8335353 [details] [diff] [review]
Patch v6 - wrapper free fix.

>@@ -231,16 +232,23 @@ nsLocation::SetURI(nsIURI* aURI, bool aR
>       return NS_ERROR_FAILURE;
> 
>     if (aReplace) {
>       loadInfo->SetLoadType(nsIDocShellLoadInfo::loadStopContentAndReplace);
>     } else {
>       loadInfo->SetLoadType(nsIDocShellLoadInfo::loadStopContent);
>     }
> 
>+    // Get the incumbent script's browsing context to set as source.
>+    nsCOMPtr<nsPIDOMWindow> sourceWindow =
>+        do_QueryInterface(mozilla::dom::GetIncumbentGlobal());
use 2 space indentation in DOM code.

I don't see GetIncumbentGlobal() declared anywhere in this patch, nor in mxr, so can't really say much about it.
I guess it returns "incumbent settings object".
(But why it isn't then called GetIncumbentSettingsObject(), though I hope the naming in the spec will be fixed to something
less jargon-y)
So, bholley or bob, could you tell where GetIncumbentGlobal() is defined?



>   // try to find an extant window with the given name
>   nsCOMPtr<nsIDOMWindow> foundWindow;
>-  if (SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow)) ==
>-      NS_ERROR_DOM_INVALID_ACCESS_ERR) {
>-    return NS_ERROR_DOM_INVALID_ACCESS_ERR;
>+  SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow));
>+  GetWindowTreeItem(foundWindow, getter_AddRefs(newDocShellItem));
>+
>+  // Do sandbox checks here, because too much happens before the checks
>+  // following the call to nsIDocShell::LoadURI.
>+  nsCOMPtr<nsPIDOMWindow> parentWindow = do_QueryInterface(aParent);
>+  if (parentWindow) {
>+    nsCOMPtr<nsIDocShell> parentDocShell = parentWindow->GetDocShell();
>+    if (parentDocShell && parentDocShell->IsSandboxedFrom(newDocShellItem)) {
>+      return NS_ERROR_DOM_INVALID_ACCESS_ERR;
>+    }
This is a bit ugly, but don't have better suggestions right now.
Some helper in docshell code perhaps, but if this is the only code which would use it, probably not
worth it.

>+  if (parentWindow)
>+    parentDocShell = parentWindow->GetDocShell();
While you're here, could you add {}


r+ pending GetIncumbentGlobal() is explained.
Attachment #8335353 - Flags: review?(bugs) → review+
(In reply to Olli Pettay [:smaug] from comment #109)
> I don't see GetIncumbentGlobal() declared anywhere in this patch, nor in
> mxr, so can't really say much about it.

It doesn't exist yet. I'm working on adding it in bug 937317.

> I guess it returns "incumbent settings object".
> (But why it isn't then called GetIncumbentSettingsObject(), though I hope
> the naming in the spec will be fixed to something
> less jargon-y)

Well, it's a bit tricky, because we don't have an explicit representation of the spec's script settings object per se. In the spec, every script has a corresponding browsing context etc. But in Gecko, it doesn't. So we track global objects, and let the caller QI to nsIPIDOMWindow if they want things like a docshell or a document.
(In reply to Bob Owen from comment #108)
> OK, this has just gone back to how it was before bug 766282.
> There's a load of recursion and calls to FindItemWithName in other classes.
> I'm pretty sure the top level call always returns |NS_OK|, but I think some
> of the recursive ones can in theory fail, although it may never happen.
> Should I just add an assertion then?
> Do I use DebugOnly<> for the return value?

If we think it might fail, we should handle it with NS_ENSURE_SUCCESS(rv, rv). If we don't think it can, we should ideally just change the signature so that it doesn't return an error code. If that's not possible for some reason, NS_ENSURE_SUCCESS(rv, rv) is probably better than DebugOnly<> and assert.


> > I just realized that we won't be able to assert that the source browsing
> > context is non-null. In the spec, scripts always have a browsing context.
> > But in Gecko, there are lots of things like sandboxes and whatnot that
> > don't. So we unfortunately need to handle that (generally, things that don't
> > QI to nsIPIDOMWindow can't be sandboxed).
> > 
> > Maybe what we should instead is to force callers to invoke
> > SetSourceDocShell, even if they pass null. We could have a debug-only
> > boolean on nsDocShellLoadInfo that checks whether the source docshell was
> > ever set, and we could assert that booleam when someone calls
> > GetSourceDocShell.
> 
> Won't they always be able to pass the docshell on which they are calling
> LoadURI?

Well, those two things are semantically different, right? One of them is saying "this docshell being navigated is the source browsing context", and the other is "we don't have a useful source browsing context". Currently the end behavior might be the same, but I think that the semantics of this distinction might become important later on.


> This can only fail since my changes for bug 766282, before that it only
> returned |NS_OK|.
> I needed to know if FindItemWithName was returning null because it hadn't
> found anything or because it had, but was sandboxed.
> Now that I've moved the sandbox checks out of nsDocShell::FindItemWithName,
> I should probably roll back these changes.
> Should I just change this to void, or return the found window?

If there's a return value, returning that directly as an already_AddRefed<T> is preferable to returning void and using an outparam.
(In reply to Olli Pettay [:smaug] from comment #109)
> Comment on attachment 8335353 [details] [diff] [review]
> Patch v6 - wrapper free fix.
>
> >+    // Get the incumbent script's browsing context to set as source.
> >+    nsCOMPtr<nsPIDOMWindow> sourceWindow =
> >+        do_QueryInterface(mozilla::dom::GetIncumbentGlobal());
> use 2 space indentation in DOM code.

Sorry, I thought other places were doing 4 for continuation indentation to distinguish.
I'll fix it.

> So, bholley or bob, could you tell where GetIncumbentGlobal() is defined?

<deleted>because bholley just beat me to it</deleted>

> >   // try to find an extant window with the given name
> >   nsCOMPtr<nsIDOMWindow> foundWindow;
> >-  if (SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow)) ==
> >-      NS_ERROR_DOM_INVALID_ACCESS_ERR) {
> >-    return NS_ERROR_DOM_INVALID_ACCESS_ERR;
> >+  SafeGetWindowByName(name, aParent, getter_AddRefs(foundWindow));
> >+  GetWindowTreeItem(foundWindow, getter_AddRefs(newDocShellItem));
> >+
> >+  // Do sandbox checks here, because too much happens before the checks
> >+  // following the call to nsIDocShell::LoadURI.
> >+  nsCOMPtr<nsPIDOMWindow> parentWindow = do_QueryInterface(aParent);
> >+  if (parentWindow) {
> >+    nsCOMPtr<nsIDocShell> parentDocShell = parentWindow->GetDocShell();
> >+    if (parentDocShell && parentDocShell->IsSandboxedFrom(newDocShellItem)) {
> >+      return NS_ERROR_DOM_INVALID_ACCESS_ERR;
> >+    }
> This is a bit ugly, but don't have better suggestions right now.
> Some helper in docshell code perhaps, but if this is the only code which
> would use it, probably not
> worth it.

It's not the prettiest.
I am hoping to follow up with a separate bug and perhaps split the search for the docshell and the navigation out into separate functions.
That way we would only be making changes on new windows and we could leave the sandbox checks until InternalLoad.
It would also mean this function wasn't having to check its flags to see what mode it is in.
 
> >+  if (parentWindow)
> >+    parentDocShell = parentWindow->GetDocShell();
> While you're here, could you add {}

Sorry, yes should have checked the standards, I prefer it with them included anyway.

> 
> r+ pending GetIncumbentGlobal() is explained.

Thanks smaug.
(In reply to Bobby Holley (:bholley) from comment #111)
> (In reply to Bob Owen from comment #108)
> If we think it might fail, we should handle it with NS_ENSURE_SUCCESS(rv,
> rv). If we don't think it can, we should ideally just change the signature
> so that it doesn't return an error code. If that's not possible for some
> reason, NS_ENSURE_SUCCESS(rv, rv) is probably better than DebugOnly<> and
> assert.

I think that because when this function is called as part of its recursion, it might return an error I'll have to use NS_ENSURE_SUCCESS(rv, rv) for now.
I'll add it to my list of things to think about, to see if the recursion and the search in general can be changed.

> > Won't they always be able to pass the docshell on which they are calling
> > LoadURI?
> 
> Well, those two things are semantically different, right? One of them is
> saying "this docshell being navigated is the source browsing context", and
> the other is "we don't have a useful source browsing context". Currently the
> end behavior might be the same, but I think that the semantics of this
> distinction might become important later on.

Fair point, best to keep things clear for future changes.
When I follow up with another bug to get everything else passing appropriately to LoadURI, I'll implement it as you suggest.

> If there's a return value, returning that directly as an already_AddRefed<T>
> is preferable to returning void and using an outparam.

OK, will do.
A lot of my free time over the last 5 or 6 days has been taken over by other priorities, so sorry about the slow progress.

I've had a reasonable amount of time today, so I'm hoping to get the tests finished tomorrow or the day after.
Hopefully I'll get the final review comments addressed on the fix by then as well.

I thought I'd get the current state of the tests uploaded and get some feedback.

I've created a bit of a framework to allow me to run a series of tests that re-use test rig windows.
This is so the tests can run on B2G, where opening many windows at once causes a problem.

The framework script is in SimpleTestSuite.js - I've started adding comments, but I've still got quite a few to go.

The tests are then generated in stSuite_iframe_sandbox_navigation_by_location.js.
I've generated the tests, as if I'd have added each separately it would have meant a lot of copy and pasting.
I hope the way I've done it will be more maintainable and easier to change many tests at once if needed.

try run: https://tbpl.mozilla.org/?tree=Try&rev=becd1aa29cf6

I've still got quite a few test cases to add, for child, parent and one permitted sandboxed navigator.
I'll also add some for other ways of invoking the script, but I'll probably cut down the different properties of location that I use for those.

Once I've got the tests and fix done, I can start raising and looking at the bug suggestions from Comment 100 as well as bug 944354 (once we've decided what we need to do).
Attachment #830910 - Attachment is obsolete: true
Attachment #8341264 - Flags: feedback?(bobbyholley+bmo)
Blocks: 944363
Comment on attachment 8341264 [details] [diff] [review]
Bug 785310 - Tests - including SimpleTestSuite framework.

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

Sorry for the delay - I've been dealing with a firehouse of review requests, and trying to get the incumbent script stuff landed. Looks like that's all mostly sorted out. :-)

Looking at this, I don't think I'm the right person to review the test harness stuff. My gut feeling is that it doesn't make sense to stick something like SimpleTestSuite.js in content/html. Either it should go in testing/ somewhere, or should be merged into some kind of existing mechanism.

jmaher, can you look at this, or suggest someone else to? Bob is making some heroic efforts to make these tests run on b2g, and might need some guidance as to how to best hook everything up to our automation in a sane way.
Attachment #8341264 - Flags: feedback?(bobbyholley+bmo) → feedback?(jmaher)
New version of tests with extra test cases mentioned in comment 114.

These seem to be running fine on B2G, so that's good:
try push: https://tbpl.mozilla.org/?tree=Try&rev=c09b879a17b6

(In reply to Bobby Holley (:bholley) from comment #115)
> Sorry for the delay - I've been dealing with a firehouse of review requests,
> and trying to get the incumbent script stuff landed. Looks like that's all
> mostly sorted out. :-)

No problem, I can see you've been busy.
All that effort, just so I can call GetIncumbentGlobal and let you do all the hard work :-)
 
> Looking at this, I don't think I'm the right person to review the test
> harness stuff. My gut feeling is that it doesn't make sense to stick
> something like SimpleTestSuite.js in content/html. Either it should go in
> testing/ somewhere, or should be merged into some kind of existing mechanism.

I hoped, if people thought it was good enough that it might get moved to somewhere more general, but I thought I'd keep it local initially, so that any issues could be ironed out before anyone else uses it (if ever).
At some point I want to re-write the other sandbox tests that don't currently run on B2G and may need to tweak things as I go. 

> jmaher, can you look at this, or suggest someone else to? Bob is making some
> heroic efforts to make these tests run on b2g, and might need some guidance
> as to how to best hook everything up to our automation in a sane way.

I've added you both for review.
jmaher: pretty much all the test harness stuff is in SimpleTestSuite.js, with some helper scripts to be used in test rigs in stSuite_testrig_iframe_common.js and stSuite_testrig_window_common.js.

bholley: most of the test cases can be seen in stSuite_iframe_sandbox_navigation_by_location_<common or 1 or 2>.js
Everything is working pretty much how I expected.
The only ones that surprised me are where I am inserting a method from a sandboxed context into a non-sandboxed one (or vice versa) and then calling it.
The scripts for this can be seen in ...location_common.js.
The incumbent script seems to be the one that created the function not where it now lives ... is this what you'd expect.

Hopefully, I'll have time to get those review comments on the fix sorted today and a new patch uploaded, but it may have to wait until Sunday.
Attachment #8341264 - Attachment is obsolete: true
Attachment #8341264 - Flags: feedback?(jmaher)
Attachment #8343664 - Flags: review?(jmaher)
Attachment #8343664 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8343664 [details] [diff] [review]
Bug 785310 - Tests v5 - including SimpleTestSuite framework.

I'd prefer to get this out of my queue until we've sorted out what we want the test infrastructure stuff to look like. Reflag me once that's all settled? :-)
Attachment #8343664 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8343664 [details] [diff] [review]
Bug 785310 - Tests v5 - including SimpleTestSuite framework.

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

overall this looks normal, here are a few thoughts:
1) what platforms will this run on, right now it looks like all desktop, android and b2g
2) the file names are long, if this runs on windows, is there a chance we could hit a limit of 255 characters assuming this was unpacked into a large named directory structure?
3) is this a test suite from a 3rd party?  I see no licensing to indicate so.
4) SimpleTestSuite.js is very similar to SimpleTest.js (I got confused 3 different times reading this)
5) I would like to see this in a subdir of its own, we do that with webgl (content/canvas/test/webgl/*)

I assume all of these pass, what if there is a failure, how would we turn a test off?  is there a submanifest somewhere or a way to add the tests to the mochitest.ini?

I need to learn more about this before doing an official review.
Attachment #8343664 - Flags: review?(jmaher)
Blocks: 947716
(In reply to Joel Maher (:jmaher) from comment #118)
> Comment on attachment 8343664 [details] [diff] [review]
> Bug 785310 - Tests v5 - including SimpleTestSuite framework.
> 
> Review of attachment 8343664 [details] [diff] [review]:
> -----------------------------------------------------------------

Thanks for looking at this so quickly.

> overall this looks normal, here are a few thoughts:
> 1) what platforms will this run on, right now it looks like all desktop,
> android and b2g

I'm hoping these tests will run on all platforms, the only one they are failing on is B2G desktop, but all the current iframe sandbox tests (as well as a lot of others) are disabled at the moment, so I think this might be for other reasons.
I'm hoping to have a look at this later today.

> 2) the file names are long, if this runs on windows, is there a chance we
> could hit a limit of 255 characters assuming this was unpacked into a large
> named directory structure?

I suppose some of them are a bit long, I was trying to be descriptive.
I've done a find and grep of what I think are test files and I found that there are at least 7 that are the same length or longer (including the directories).
Of course these may not run on windows.
I'm happy to reduce them, to what limit do you think I should stick?

> 3) is this a test suite from a 3rd party?  I see no licensing to indicate so.

No, I've just written it.
I've done it in an attempt to get more of the iframe sandbox tests running on B2G.
At the moment, because of the nature of them, they do a lot of document navigation and some of them open a lot of windows. Most of this happens in parallel.
This doesn't work on B2G, so I wanted to run the tests in serial, only opening one window at a time.
The test suite script allows me to do that as well as reusing the same "test rig" window or frame for many tests.
It also gives a consistent style for the pass and fail messages, which make it easier to work out which test has failed.

> 4) SimpleTestSuite.js is very similar to SimpleTest.js (I got confused 3
> different times reading this)

Ah, sorry about that.
I called it that, because it manages the running of test suites within a single SimpleTest mochitest and uses the SimpleTest functions to do it's reporting.
I'm happy to change it to something else, any suggestions welcome ... perhaps just TestSuiteManager?

> 5) I would like to see this in a subdir of its own, we do that with webgl
> (content/canvas/test/webgl/*)

Hopefully, as bholley suggested, the framework scripts could move to under testing/ somewhere, but I thought it might be better to do that once I had used it a bit more in re-writing the other iframe sandbox tests, in case I have to make any breaking changes.
It might be worth having all the iframe sandbox tests in their own directory, although it will mean we will have to be even more careful with the length of the file names.

> I assume all of these pass, what if there is a failure, how would we turn a
> test off?  is there a submanifest somewhere or a way to add the tests to the
> mochitest.ini?

Well the overall test is still in mochitest.ini and so could be turned off or ignored for certain platforms in the *.json files. This is no worse than the current situation.
In this set of tests, I have generated the tests added to each test suite through script, because they all have a lot in common. So it should be fairly easy to stop any problem tests being run.
If the test suites script is used in its simplest form, each test would be added separately through a call to stSuite.TestSuite.addTest, so it would be easy to take out any that are broken.

> I need to learn more about this before doing an official review.

Sorry, I should have explained the background more when I uploaded this.
Hopefully, I've made things clearer, let me know if you have any other questions.
I don't see .json files added to the patch, all I see is a single entry in mochitest.ini.  If we wanted to turn off a subtest or two for a given platform, how could that be done (I could be overlooking the obvious)?

TestSuiteManager is a better name than SimpleTestSuite.js, good suggestion.

I am fine with the longer filenames, I only ask that we don't duplicate descriptive text between directory and filenames. 

Please ask for review again when you have some little details sorted out!
r=bholley - Review comments now addressed from comment 107.
r=smaug - Review comments now addressed from comment 109.
Attachment #8335353 - Attachment is obsolete: true
Attachment #8344691 - Flags: review+
Depends on: 948948
(In reply to Joel Maher (:jmaher) from comment #120)
> I don't see .json files added to the patch, all I see is a single entry in
> mochitest.ini.  If we wanted to turn off a subtest or two for a given
> platform, how could that be done (I could be overlooking the obvious)?

Sorry, I meant the ones in testing/mochitest/ to turn the whole test off.
This is no worse than for other tests that contain multiple subtests.
However, I can see it would be really useful to be able to switch off individual subtests.  It would also be helpful when you are creating the subtests, so you can isolate one while you are working on it.

This would be fairly straight forward if I could determine the platform on which it was running.  I could then have a json file with the same name as the test and a suffix of the platform, with excluded or included test and test suite IDs.
Either that or have a central json file for each platform like the existing ones.

Having poked around the mochitest stuff, I can see this may take a little while to add, so I don't want it to delay this bug, if that's OK.
I'll pick it up when I get to do the re-write of the other tests that don't run on B2G.

> TestSuiteManager is a better name than SimpleTestSuite.js, good suggestion.

This was fairly easy with a few careful ex commands in vim on the patch file.
I've also moved the three files that I think would definitely be common under a directory called tsm.
With a view to this moving under (or adjacent to) SimpleTest, once I've used it for the other sandbox tests.

> I am fine with the longer filenames, I only ask that we don't duplicate
> descriptive text between directory and filenames. 

I think it would be a good idea to have the iframe sandbox tests in their own directory, as you say I'll be able to remove the iframe_sandbox_ part from the file names, so it shouldn't cause any issues.
Again, I'll leave this until I look at the rest of the tests, as it makes sense to move them all together.

I've also raised bug 948948, for the failure on B2G Desktop Linux x64, which seems to be something more fundamental.
I've disabled the new tests for this platform, referencing that bug.
Attachment #8343664 - Attachment is obsolete: true
Attachment #8345908 - Flags: review?(jmaher)
Latest try push: https://tbpl.mozilla.org/?tree=Try&rev=1689af29cb29

Test iframe sandbox tests run in mochitest-2 on B2G and mochitest-3 on Android.
Comment on attachment 8345908 [details] [diff] [review]
Bug 785310 - Tests v6 - including TestSuiteManager framework.

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

This looks good, thanks for addressing my previous feedback, please ensure this passes on try server prior to landing.
Attachment #8345908 - Flags: review?(jmaher) → review+
Comment on attachment 8345908 [details] [diff] [review]
Bug 785310 - Tests v6 - including TestSuiteManager framework.

Thanks jmaher.
They're all passing on try except for B2G Desktop, where window.open doesn't seem to be working at all.

bholley: most of the test cases can be seen in stSuite_iframe_sandbox_navigation_by_location_<common or 1 or 2>.js
Everything is working pretty much how I expected.
The only ones that surprised me are where I am inserting a method from a sandboxed context into a non-sandboxed one (or vice versa) and then calling it.
The scripts for this can be seen in ...location_common.js.
The incumbent script seems to be the one that created the function not where it now lives ... is this what you'd expect?

Looks like bug 937317 is all sorted and these patches apply fine on the current tip.
So, once you're happy with my tests, I'll do a full try push and look at landing this.
Thanks.
Attachment #8345908 - Flags: review?(bobbyholley+bmo)
I'm really sorry it took me so long to respond on this, especially because my feedback is something that would have been best delivered earlier.

In a nutshell, I don't think that adding a custom test framework here is the right approach. I'll try to explain why.

One of the most time-consuming parts of writing code for Mozilla is getting the tree green with a patch. Part of this is just the result of the fact that Gecko is a complicated beast, and that it's easy to break things in unexpected ways. That's what tests are there for. But the task is made extra difficult by the proliferation of test harnesses, because the developer loses the ability to leverage the existing patterns that they've learned. When I get a regular mochitest failure, I know exactly how to reproduce it, and my eyes know exactly how to skip over the boilerplate and find the meat of the test. When I get a failure in some other kind of test harness that I'm less familiar with, it takes me much, much more time to get to a state where I can comprehend and debug it.

In some cases, the proliferation of different frameworks is unavoidable - B2G has very different requirements than desktop, which is why we need something like Marionette to test certain parts of it. But I think we should make every effort to strive for consistency, and to keep the set of test harnesses as small as possible.

It's also often worth it to sacrifice exhaustiveness for readability. As tempting as it is to create nifty abstractions in a test that let you test all of the possible combinations of factors, there's a point at which doing so takes a significant toll on the ability of developers to comprehend the test. And that matters.

I recognize and appreciate your desire to make these tests work on b2g. But I don't think that writing a custom harness along with your tests here is the right approach. And unless there are specific concerns as to why this functionality is particularly likely to regress on b2g, I think the b2g testing story is orthogonal enough to your goals that it's not a good use of your time to worry about it.

Martijn (CCed) has been doing a lot of work getting mochitests to run on b2g. It's definitely worth talking to him to figure out what it would take to support your use cases in the existing test framework.

In the mean time though, I think we should make the tests for this stuff as simple as possible, and follow as many of the existing testing patterns as we can. That way, our brains can filter out the framework and get directly to work analyzing the stuff that's actually being tested. :-)

Also CCing jgriffin in case he has any thoughts on the matter.
Attachment #8345908 - Flags: review?(bobbyholley+bmo)
(In reply to Bobby Holley (:bholley) from comment #126)
> I'm really sorry it took me so long to respond on this, especially because
> my feedback is something that would have been best delivered earlier.

I know you've been really busy with reviews, your own fixes and other things, like the discussions about cross-origin access to window and location that I've been following at https://www.w3.org/Bugs/Public/show_bug.cgi?id=20701.
When I say following ... I mean I've read nearly all the words. :)

> In a nutshell, I don't think that adding a custom test framework here is the
> right approach. I'll try to explain why.

Well, it would have been nice to get this earlier, but not all the effort is wasted ... I've learned a lot more about JavaScript while I was writing it. :-)

> It's also often worth it to sacrifice exhaustiveness for readability. As
> tempting as it is to create nifty abstractions in a test that let you test
> all of the possible combinations of factors, there's a point at which doing
> so takes a significant toll on the ability of developers to comprehend the
> test. And that matters.

I'm not sure that's often a trade off, but if you mean through generating the tests that were put into the test suites, then I was a bit worried about that as I implied back in comment 114.
Although I've not seen all that many bugs, there does seem to be a tendency to write tests after the code, something I've been guilty of myself on this bug.
So, while I agree that trying to test all combinations can be counter-productive, you have to be careful about the reasons for dropping certain tests.
For example writing the tests afterwards can mean you don't test things because you take into account how you know the code is implemented.

> I recognize and appreciate your desire to make these tests work on b2g. But
> I don't think that writing a custom harness along with your tests here is
> the right approach. And unless there are specific concerns as to why this
> functionality is particularly likely to regress on b2g, I think the b2g
> testing story is orthogonal enough to your goals that it's not a good use of
> your time to worry about it.
> 
> Martijn (CCed) has been doing a lot of work getting mochitests to run on
> b2g. It's definitely worth talking to him to figure out what it would take
> to support your use cases in the existing test framework.
> 
> In the mean time though, I think we should make the tests for this stuff as
> simple as possible, and follow as many of the existing testing patterns as
> we can. That way, our brains can filter out the framework and get directly
> to work analyzing the stuff that's actually being tested. :-)

It's not just these tests, a lot of the iframe sandbox tests don't run on B2G, there is an outstanding bug for this.
So, I was trying to come up with things that could be applied to re-writes of those tests as well.
The main issue is that you are testing things that happen asynchronously, concurrently and in new windows. Opening several windows at once on B2G seems to be a problem and lots of concurrent navigation seems to be causing time-outs on other platforms as well.
The framework just allowed you to to manage asynchronous subtests within a single mochitest, so they were run serially.
I could have written separate mochitests for many of the test cases, but I think that would make the tests quite disjointed, more difficult to maintain and I wasn't sure how much overhead it would add.
However, having looked at your tests for bug 937317 and having had a very quick play, I think I can write the tests in a more straight forward way.
I'll still have to put in some of the mechanisms that the framework provided, but I think I can do this in a much more readily understandable and transparent way.
The last thing I want to do is make things difficult for people looking at these in the future.

Not sure how much I'll get done in the next few days, but hopefully it won't take too long to get them finished.
Sounds great! I appreciate your understanding.

> I'll still have to put in some of the mechanisms that the framework provided,
> but I think I can do this in a much more readily understandable and transparent way.

Yeah. If any of that framework is general and reusable, we should think about putting it into SimpleTest itself.

Looking forward to working with you more!
(In reply to Bob Owen from comment #125)
> Comment on attachment 8345908 [details] [diff] [review]
> Bug 785310 - Tests v6 - including TestSuiteManager framework.
> 
> Thanks jmaher.
> They're all passing on try except for B2G Desktop, where window.open doesn't
> seem to be working at all.

I'm not sure how this works, but if this is a new framework, it might have to add the mozbrowseropenwindow event listener to open a new window.
But I see that the idea of a new framework was discarded.
Attached patch WIP: Bug 785310 - Tests v7 (obsolete) — — Splinter Review 
I've made reasonable progress on these tests.
Hopefully they are much more straight forward.

I thought I'd get some feedback to make sure they're along the right lines.
I still need to do the tests for location on sibling, child, etc.
Attachment #8345908 - Attachment is obsolete: true
Attachment #8356710 - Flags: feedback?(bobbyholley+bmo)
Comment on attachment 8356710 [details] [diff] [review]
WIP: Bug 785310 - Tests v7

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

This is much more grokable. Thank you!

I think these tests should go in their own directory, which should shorten the filenames a bit. How about docshell/test/iframesandbox/ ?

::: content/html/content/test/test_iframe_sandbox_top_navigation_by_location.html
@@ +13,5 @@
> +  SimpleTest.waitForExplicitFinish();
> +
> +  var testWinHtml = "<script>function onNav() { opener.postMessage('', '*'); } window.onload = onNav; window.onhashchange = onNav;<\/script><body><iframe name='if1' sandbox='allow-scripts allow-same-origin'></iframe><iframe name='if2' sandbox='allow-scripts allow-same-origin allow-top-navigation'></iframe><iframe name='if3' sandbox='allow-scripts allow-top-navigation'></iframe></body>";
> +  var testWinUri = "data:text/html," + testWinHtml;
> +  var testWin;

data:uris are already kind of special, and I'm concerned that they might have subtly-different behavior with respect to loads. I think we should split this out into a support file (for test_blah.html this would be file_blah.html);

@@ +28,5 @@
> +      if (shouldBeBlocked) {
> +        timeoutId = setTimeout(function() {
> +          info(desc + ": no failures within 500ms");
> +          runNextTest();
> +        }, 500);

Hm, timeout-based tests are generally pretty verboten. They take way too long in the common case, and subject to timing flakiness.

Is there really no way to tell from the DOM that a load has been blocked? If not, maybe we could add an XPCOM observer notification that we fire when we block a load?

@@ +43,5 @@
> +      "Test 1: top.location.replace should be blocked when sandboxed without allow-top-navigation",
> +      "top.location.replace(\"" + testWinUri + "\")",
> +      "if1",
> +      true
> +    )},

The classic way to do this is with runNavigationTest.bind(null, arg1, arg2, ...); This avoids the extra anonymous function and braces.
Attachment #8356710 - Flags: feedback?(bobbyholley+bmo) → feedback+
(In reply to Bobby Holley (:bholley) from comment #131)
>
> Is there really no way to tell from the DOM that a load has been blocked? If
> not, maybe we could add an XPCOM observer notification that we fire when we
> block a load?

I don't think so - bsterne and sstamm ran into this while writing tests for CSP. 

They did exactly what you suggest for the CSP tests, see, for example, http://mxr.mozilla.org/mozilla-central/source/content/base/test/csp/test_CSP.html?force=1#62
Attached patch Bug 785310 - Tests v7 (obsolete) — — Splinter Review 
Added tests for: parent, child, sibling, auxiliary opened by us and auxiliary opened by someone else.

I think these should still all run on B2G.

try push: https://tbpl.mozilla.org/?tree=Try&rev=25965adcb0fc

(In reply to Bobby Holley (:bholley) from comment #131)
> Comment on attachment 8356710 [details] [diff] [review]
> WIP: Bug 785310 - Tests v7
> 
> Review of attachment 8356710 [details] [diff] [review]:
> -----------------------------------------------------------------
> I think these tests should go in their own directory, which should shorten
> the filenames a bit. How about docshell/test/iframesandbox/ ?

Tests moved.
I'll look at possibly moving some of the other tests, if it makes sense, when I (eventually) get round to rewriting them to run on B2G.
 
> :::
> content/html/content/test/test_iframe_sandbox_top_navigation_by_location.html
> @@ +13,5 @@
> > +  SimpleTest.waitForExplicitFinish();
> > +
> > +  var testWinHtml = "<script>function onNav() { opener.postMessage('', '*'); } window.onload = onNav; window.onhashchange = onNav;<\/script><body><iframe name='if1' sandbox='allow-scripts allow-same-origin'></iframe><iframe name='if2' sandbox='allow-scripts allow-same-origin allow-top-navigation'></iframe><iframe name='if3' sandbox='allow-scripts allow-top-navigation'></iframe></body>";
> > +  var testWinUri = "data:text/html," + testWinHtml;
> > +  var testWin;
> 
> data:uris are already kind of special, and I'm concerned that they might
> have subtly-different behavior with respect to loads. I think we should
> split this out into a support file (for test_blah.html this would be
> file_blah.html);

I think that all the sandboxing checks happen before this matters, but I've moved most of the data: URIs to normal files anyway.
I've left some as data: URIs to try and give the broadest test coverage. 
 
> @@ +28,5 @@
> > +      if (shouldBeBlocked) {
> > +        timeoutId = setTimeout(function() {
> > +          info(desc + ": no failures within 500ms");
> > +          runNextTest();
> > +        }, 500);
> 
> Hm, timeout-based tests are generally pretty verboten. They take way too
> long in the common case, and subject to timing flakiness.

They should only take 500ms for the ones that don't throw and while there is a chance of timing problems I think it is slim.
In addition to that, once I change these tests for bug 944363 and get that landed, they'll all throw and I'll be able to remove these timeouts.

> Is there really no way to tell from the DOM that a load has been blocked? If
> not, maybe we could add an XPCOM observer notification that we fire when we
> block a load?

Well I could do this (once I've worked out what it means :-) ), but do you think it is worth it given the above?

> @@ +43,5 @@
> > +      "Test 1: top.location.replace should be blocked when sandboxed without allow-top-navigation",
> > +      "top.location.replace(\"" + testWinUri + "\")",
> > +      "if1",
> > +      true
> > +    )},
> 
> The classic way to do this is with runNavigationTest.bind(null, arg1, arg2,
> ...); This avoids the extra anonymous function and braces.

Yeah, I changed it to this and then I realised that as I am using the same function all the time, I could just store objects in the array and pass them to the function.
I think this makes it even more readable ... hopefully you'll agree :-)
Attachment #8356710 - Attachment is obsolete: true
Attachment #8358551 - Flags: review?(bobbyholley+bmo)
(In reply to Bob Owen from comment #133)

> > Hm, timeout-based tests are generally pretty verboten. They take way too
> > long in the common case, and subject to timing flakiness.
> 
> They should only take 500ms for the ones that don't throw

How many of those are there?

> and while there is
> a chance of timing problems I think it is slim.

It's surprisingly common. This has been a pain-point for years, which led to the ban on timeout-based tests.

> In addition to that, once I change these tests for bug 944363 and get that
> landed, they'll all throw and I'll be able to remove these timeouts.

Is that true? I thought there were still some async cases where we wouldn't be able to throw. Or are none of those paths tested here?

If so, how much work is bug 944363? If it just involves returning a failure code in a few places, then I think we should land this all together. We really shouldn't be putting any timeout-dependent tests onto TBPL.
(In reply to Bobby Holley (:bholley) from comment #134)
> (In reply to Bob Owen from comment #133)
> How many of those are there?

5

> > In addition to that, once I change these tests for bug 944363 and get that
> > landed, they'll all throw and I'll be able to remove these timeouts.
> 
> Is that true? I thought there were still some async cases where we wouldn't
> be able to throw. Or are none of those paths tested here?

These are all sync.
There are other cases that aren't, for example anchor clicks, but we're not testing that here.

I am using setTimeout in one of the tests, but I'm specifically testing setTimeout, so I'm not sure whether that would still be banned.
 
> If so, how much work is bug 944363? If it just involves returning a failure
> code in a few places, then I think we should land this all together. We
> really shouldn't be putting any timeout-dependent tests onto TBPL.

The fix is done, I'll just need to change these new tests, but that won't take a second.
Should I wrap it all into these patches or just try to land the bugs together?
Flags: needinfo?(bobbyholley+bmo)
Just mark this bug as dependent on that one, and land them together. The Gecko patches can remain distinct, but the tests can be rolled into one patch (using this bug number for all the tests is fine).
Flags: needinfo?(bobbyholley+bmo)
I should have time to get onto this tomorrow.
Comment on attachment 8358551 [details] [diff] [review]
Bug 785310 - Tests v7

Cancelling review for now per comment 135.
Attachment #8358551 - Flags: review?(bobbyholley+bmo)
No longer blocks: 944363
Depends on: 944363
Merged with mozilla-central.
Also noticed another case where I'd changed an if statement that hadn't got curly braces.

r=bholley - Review comments now addressed from comment 107.
r=smaug - Review comments now addressed from comment 109.
Attachment #8344691 - Attachment is obsolete: true
Attachment #8359079 - Flags: review+
Attached patch Bug 785310 - Tests v8 (obsolete) — — Splinter Review 
Changed to allow for changes in bug 944363, where all navigation due to changes to Location now propagates any errors.
Bug 944363 must be landed with this one now.

Also added lines to b2g-desktop.json for some of the tests as window.open still appears to be broken.

Thanks for pushing me on these tests, not only are they more comprehensible, they still run on B2G and they're also a lot faster :-)

try push: https://tbpl.mozilla.org/?tree=Try&rev=cc760aa0a1e6
Attachment #8358551 - Attachment is obsolete: true
Attachment #8359082 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8359082 [details] [diff] [review]
Bug 785310 - Tests v8

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

::: docshell/test/iframesandbox/test_top_navigation_by_location.html
@@ +23,5 @@
> +    };
> +    try {
> +      SpecialPowers.wrap(testWin[testCase.iframeName]).eval(testCase.script);
> +      if (testCase.shouldBeBlocked) {
> +        timeoutId = setTimeout(function() {

I don't understand - I thought we were getting rid of the timeout-based block detection? Cancelling review based on that for now, let me know if I misunderstood something.

::: docshell/test/mochitest.ini
@@ +31,5 @@
>    file_bug680257.html
>    file_bug703855.html
>    file_bug728939.html
>    historyframes.html
> +  iframesandbox/file_other_auxiliary_navigation_by_location.html

I think we should probably give the iframesandbox/ director its own mochitest.ini, no?
Attachment #8359082 - Flags: review?(bobbyholley+bmo)
Attached patch Bug 785310 - Tests v9 (obsolete) — — Splinter Review 
(In reply to Bobby Holley (:bholley) from comment #141)
> Comment on attachment 8359082 [details] [diff] [review]
> > +        timeoutId = setTimeout(function() {
> 
> I don't understand - I thought we were getting rid of the timeout-based
> block detection? Cancelling review based on that for now, let me know if I
> misunderstood something.

Sorry bholley, don't know how I missed that one - now fixed.
I think it might have been at the end of my vim chain and I didn't save it.

> I think we should probably give the iframesandbox/ director its own
> mochitest.ini, no?

Ah, I misremembered the post I read, it's just the moz.build that you don't create.
Now fixed.

I meant to say before, the only tests that I'm not totally sure about whether the behaviour is correct are Tests 13 and 15 in test_top_navigation_by_location_exotic.html


New try push (combined with bug 944363):
https://tbpl.mozilla.org/?tree=Try&rev=528ee18ab10e
Attachment #8359082 - Attachment is obsolete: true
Attachment #8359841 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8359841 [details] [diff] [review]
Bug 785310 - Tests v9

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

These test are top-notch! They're very readable and very thorough. This is exactly the kind of attention to detail that we need for stuff like this. I'm super impressed. :-)

At the work week in San Francisco, we're going to have a discussion on how to share our tests with the spec community (and what to do about the scattered instances of SpecialPowers we need to glue things together). I think these tests are a great candidate for trying to share. Can you remember to bring it up?

> I meant to say before, the only tests that I'm not totally sure about whether the
> behaviour is correct are Tests 13 and 15 in test_top_navigation_by_location_exotic.html

That is very helpful to know. I've spot-checked a few of them and I'll trust you on the rest, which vastly reduces the amount of time it takes to review this.

::: docshell/test/iframesandbox/test_top_navigation_by_location.html
@@ +14,5 @@
> +
> +  var testWin;
> +
> +  function runScriptNavigationTest(testCase) {
> +    window.onmessage = function() {

Maybe add an |evt| param and check that |SpecialPowers.wrap(evt.source).location| is file_top_navigation_by_location.html? This would catch any weirdness where the tests start postMessaging to the wrong parent or something.

@@ +146,5 @@
> +      SimpleTest.finish();
> +      return;
> +    }
> +
> +    if (testCaseIndex < testCases.length) {

Seems like this conditional should be removed given the early-return above. This occurs in a number of the other test_*.html files as well.

::: docshell/test/iframesandbox/test_top_navigation_by_location_exotic.html
@@ +11,5 @@
> +<link rel="stylesheet" type="text/css" href="/tests/SimpleTest/test.css"/>
> +<script>
> +  SimpleTest.waitForExplicitFinish();
> +
> +  var testRigHtml = "<script>function onBlock() { opener.postMessage(true, '*'); } function onNav() { opener.postMessage(false, '*'); } function setOwnHref() { location.href = location.href } window.onload = onNav;<\/script><body><iframe name='if1' sandbox='allow-scripts allow-same-origin'></iframe><iframe name='if2' sandbox='allow-scripts allow-same-origin allow-top-navigation'></iframe></body>";

Is the any reason not to separate this out into a file_top_navigation_by_location_exotic.html like the rest? Seems like it would be more readable and more consistent with the rest.

@@ +106,5 @@
> +      shouldBeBlocked: false
> +    },
> +    {
> +      desc: "Test 13: top.location.href, via function inserted in top, should be blocked when sandboxed without allow-top-navigation",
> +      script: "top.doTest = function() { top.location.href = top.location.href }; top.doTest();",

This is correct. The function is defined in the child, which means that the child will be the incumbent script when it executes.

@@ +120,5 @@
> +    {
> +      desc: "Test 15: top.location.href, via function inserted in us by top, should NOT be blocked when sandboxed without allow-top-navigation",
> +      script: "top.eval('window[\"if1\"].doTest = function() { top.location.href = top.location.href };'), doTest();",
> +      iframeName: "if1",
> +      shouldBeBlocked: false

This is correct as well. It exposes the precise reasons why disallowing top-navigation with allow-same-origin is non-sensical. Make sense?

::: docshell/test/moz.build
@@ +14,5 @@
>  
> +MOCHITEST_MANIFESTS += [
> +    'iframesandbox/mochitest.ini',
> +    'mochitest.ini'
> +]

So, does this allow you to do |mach mochitest-plain docshell/test/iframesandbox| and run only the iframesandbox tests? I think so, but I just wanted to check, since that's one of the main reasons for putting it in a subdirectory.
Attachment #8359841 - Flags: review?(bobbyholley+bmo) → review+
Attached patch Bug 785310 - Tests v10 (obsolete) — — Splinter Review 
(In reply to Bobby Holley (:bholley) from comment #143)
> Comment on attachment 8359841 [details] [diff] [review]
> Bug 785310 - Tests v9
> These test are top-notch! They're very readable and very thorough. This is
> exactly the kind of attention to detail that we need for stuff like this.
> I'm super impressed. :-)

Thanks.
When is the SFO work week?

> Maybe add an |evt| param and check that
> |SpecialPowers.wrap(evt.source).location| is
> file_top_navigation_by_location.html? This would catch any weirdness where
> the tests start postMessaging to the wrong parent or something.

Had a problem getting the location from the source when cross origin, even with SpecialPowers.
So, I've just passed and checked something in the message data.

> > +    if (testCaseIndex < testCases.length) {
> 
> Seems like this conditional should be removed given the early-return above.
> This occurs in a number of the other test_*.html files as well.

Thanks ... removed, I had a problem with too many messages getting posted at one point.

> Is the any reason not to separate this out into a
> file_top_navigation_by_location_exotic.html like the rest? Seems like it
> would be more readable and more consistent with the rest.

I thought I'd keep some data: URI stuff, to give the broadest test coverage.
All this stuff should work for them as well.
I can break it out into a separate file if you like.

> > +      desc: "Test 15: top.location.href, via function inserted in us by top, should NOT be blocked when sandboxed without allow-top-navigation",
> > +      script: "top.eval('window[\"if1\"].doTest = function() { top.location.href = top.location.href };'), doTest();",
> > +      iframeName: "if1",
> > +      shouldBeBlocked: false
> 
> This is correct as well. It exposes the precise reasons why disallowing
> top-navigation with allow-same-origin is non-sensical. Make sense?

It does make sense.
I thought they were correct, but I wasn't sure.
 
> > +MOCHITEST_MANIFESTS += [
> > +    'iframesandbox/mochitest.ini',
> > +    'mochitest.ini'
> > +]
> 
> So, does this allow you to do |mach mochitest-plain
> docshell/test/iframesandbox| and run only the iframesandbox tests? I think
> so, but I just wanted to check, since that's one of the main reasons for
> putting it in a subdirectory.

Yes, it does.
In fact even with the tests defined in the parent directory's mochitest.ini, like I had before, that works.
It's better with its own mochitest.ini though and I believe that keeping down the number of |moz.build|s is what helps with build times.
Attachment #8359841 - Attachment is obsolete: true
Attachment #8360464 - Flags: review?(bobbyholley+bmo)
Comment on attachment 8360464 [details] [diff] [review]
Bug 785310 - Tests v10

(In reply to Bob Owen (:bobowen) from comment #144)
> Created attachment 8360464 [details] [diff] [review]
> When is the SFO work week?

The week of January 27th. I've been told you'll be there.

> I thought I'd keep some data: URI stuff, to give the broadest test coverage.
> All this stuff should work for them as well.
> I can break it out into a separate file if you like.

I'd probably break it out into a separate file for consistency, and do an explicit test for data uris somewhere. But it's not a big deal.

> Yes, it does.
> In fact even with the tests defined in the parent directory's mochitest.ini,
> like I had before, that works.
> It's better with its own mochitest.ini though and I believe that keeping
> down the number of |moz.build|s is what helps with build times.

Good to know.
Attachment #8360464 - Flags: review?(bobbyholley+bmo) → review+
Blocks: 960506
Blocks: 960545
r=bholley - Final review comments addressed below.

Limited try run for just these tests, to make sure I haven't broken them on other platforms:
https://tbpl.mozilla.org/?tree=Try&rev=1dfc8375f605

(In reply to Bobby Holley (:bholley) from comment #145)
> Comment on attachment 8360464 [details] [diff] [review]
> Bug 785310 - Tests v10

> > When is the SFO work week?
> 
> The week of January 27th. I've been told you'll be there.

Apparently I will :-)
At least for some of the time.
 
> > I thought I'd keep some data: URI stuff, to give the broadest test coverage.
> > All this stuff should work for them as well.
> > I can break it out into a separate file if you like.
> 
> I'd probably break it out into a separate file for consistency, and do an
> explicit test for data uris somewhere. But it's not a big deal.

Now in separate file.

I've been thinking about what actually needs testing around data URIs.
It doesn't really matter for the browsing context we are attempting to navigate, because I believe the type of URI used to load a document within it has no bearing on whether others can navigate it (barring some cross origin restrictions).

We do need tests to check that documents created through data URIs (and srcdocs), inherit and imbue the sandbox flags correctly.
There is a small check of this still in these tests, but generally these should probably live with some of the other iframe sandboxing tests ... I'll have a look at them.
Attachment #8360464 - Attachment is obsolete: true
Attachment #8361137 - Flags: review+
Needs to be landed with fix patch for bug 944363.

https://bugzilla.mozilla.org/attachment.cgi?id=8359842
Keywords: checkin-needed
https://hg.mozilla.org/mozilla-central/rev/337e1ea7a390
https://hg.mozilla.org/mozilla-central/rev/4002565ecee5
Status: ASSIGNED → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
Keywords: sec-low → sec-high
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: