Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Use of source browsing context in navigation seems totally broken to me #1130

Open
bzbarsky opened this issue Apr 26, 2016 · 42 comments
Open

Use of source browsing context in navigation seems totally broken to me #1130

bzbarsky opened this issue Apr 26, 2016 · 42 comments
Assignees

Comments

@bzbarsky
Copy link
Contributor

@bzbarsky bzbarsky commented Apr 26, 2016

The way HTML does navigation is that the input is a "source browsing context". This is then used in phrases like "the origin of the source browsing context" (not defined anywhere; browsing contexts don't have origins) and "Set request's client to the source browsing context's active document's Window object's environment settings object".

The problem is that the active document of the source browsing context may not be the document responsible for the navigation, no? Unless we're very careful to ensure that scripts in no-longer-active documents can't perform window.open (even on other windows!), can't submit forms, can't trigger targeted anchors, etc. I'm pretty darned sure nothing prevents the window.open thing, at least.

We should really be passing in the source window or source document or something here.

@annevk
Copy link
Member

@annevk annevk commented Apr 26, 2016

That makes sense. (FWIW, see #1117 and #1128 for some initial cleanup of the navigate algorithm. If those land, I plan on producing a lot more. Really want to get it in a much better shape.)

Loading

@annevk
Copy link
Member

@annevk annevk commented Apr 28, 2016

As far as I can tell this is the concept that Fetch calls "client". That would mean that the relevant settings object of the active document would be the way to go.

Loading

@annevk
Copy link
Member

@annevk annevk commented Apr 28, 2016

Perhaps we should also start calling it the "client" parameter to the navigate algorithm to indicate the break from the broken current pattern.

Loading

@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented Apr 28, 2016

That would mean that the relevant settings object of the active document would be the way to go

Even when the navigation was started by one of the inactive documents?

I agree that fundamentally what we want here is the Fetch "client".

Loading

@annevk
Copy link
Member

@annevk annevk commented Apr 28, 2016

Even when the navigation was started by one of the inactive documents?

Ah, window.open() and similar cases. I'm not sure what would be best. "Current settings object"?

Loading

@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented Apr 28, 2016

We should start by checking what UAs actually do...

Loading

@annevk
Copy link
Member

@annevk annevk commented May 2, 2016

Here's a simple test using window.open() from an <iframe>. It seems to have no effect: http://software.hixie.ch/utilities/js/live-dom-viewer/saved/4130. @domenic thoughts? You're way better versed in this than I am.

Loading

@domenic
Copy link
Member

@domenic domenic commented May 2, 2016

I am? Dang now I have to try to understand this issue... Will give it a try later today or tomorrow.

Loading

@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented May 2, 2016

It seems to have no effect:

No effect in what sense? This open() call is being blocked by the popup blocker, right? Try http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4132 and click the button.

Loading

@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented May 2, 2016

Oh, and what http://software.hixie.ch/utilities/js/live-dom-viewer/?saved=4132 shows is that the referrer being used for window.open is coming from either the incumbent global or the entry global, not from the current or relevant global. I think.

Loading

@annevk
Copy link
Member

@annevk annevk commented May 2, 2016

I meant that the referrer is not coming from the <iframe>, sorry.

Loading

@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented May 2, 2016

OK, true, and useful data. But that's not quite exercising the case I was really worried about above.

The case I was worried about above is what happens when a window.open call happens and the incumbent global (and/or the entry global if we can arrange that somehow) corresponds to a no-longer-active document. In other words, load some frame, grab a function f from it, where f performs a window.open() call on some window (probably not the iframe's window itself, though it's interesting to test how UAs handle that too; I know Firefox has some security checks around it). Then navigate the iframe so the global of f corresponds to a no-longer-current document. Then call f either from your own script (so you're entry but f is incumbent) or by setting f as an event listener (so f is entry).

Then see what happens.

In practice, based on past testing I've done, I expect that the "f as event listener" case won't call the function at all in Firefox. I think it won't in IE either. Not sure about Safari/Chrome. The "f as incumbent" case will throw an exception trying to call f in at least some IE versions, but iirc call it successfully in other browsers. And the behavior in terms of whether f is called or not is quite different, with some web compat constraints, if we remove the iframe from the DOM instead of navigating it. :( None of that says what the referrer will be, of course. Or the place that determines our secure context state, for that matter (ideally this is the same thing, but I have low confidence in this being the case in specs or impls right now, because secure context state is defined in terms of openers iirc and the opener is the relevant global, I think).

Basically, we need to write an actual set of principled tests, using wpt or something; live dom viewer poking is going to get pretty confusing, esp. as people want to add more tests and modify existing ones to test more edge cases.

Loading

@domenic
Copy link
Member

@domenic domenic commented May 2, 2016

OK, trying to get my head around this.

The difference between the OP's suggestion of "source document" and using a settings object is the cases where that relationship is not 1:1. That is: the about:blank initial navigation, where a window = settings object has two documents; and document.open(), where multiple window = settings objects end up mapping to the same document.

It's not clear to me whether these cases matter for what we're talking about, maybe because I am having trouble piecing together a comprehensive list of the ways in which source browsing context is currently used. It sounds like we have mainly that it's used as the fetch client, which impacts referrer, and ... what else?

I guess the discussion has probably moved on beyond that and is now assuming that it should be "source settings object". Seems fine. The question is then whether the navigation algorithm, when invoked, should be invoked with the current, entry, incumbent, or relevant settings object as the "source settings object".

I agree that a comprehensive set of tests would be helpful. It seems like we'd need a matrix of all the places that invoke the navigate algorithm, and in each case, ways to maximally vary the incumbent/entry/current/relevant settings objects, so we can see which one ends up being used. (How do we test which one is being used? I guess by making sure each is at a different URL, and then checking the resulting referrer?)

Loading

@bzbarsky
Copy link
Contributor Author

@bzbarsky bzbarsky commented May 3, 2016

It's not clear to me whether these cases matter for what we're talking about

That's a good question.

The answer is that navigations can be triggered either via scripted APIs, which are naturally associated with a settings object, not a document per se, or via elements, which are more naturally associated with a document.

We should test what UAs do with the obvious edge case here: inserting an anchor into an initial about:blank, waiting for the new document to get loaded, and then doing click() on that anchor. I'm hoping the answer is "there is no navigation" or something and then I think the distinction becomes irrelevant....

and is now assuming that it should be "source settings object"

I certainly didn't mean to give that impression.

The question is then whether the navigation algorithm, when invoked, should be invoked with the current, entry, incumbent, or relevant settings object as the "source settings object".

The answer likely depends on the way its invoked. When a user clicks a link, there is no current, entry, or incumbent settings object at all. There's a relevant one for the link node, of course. On the other hand, when setting location.href or doing window.open I'm pretty sure that relevant settings object is not the right thing (in the sense of matching browsers or being web-compatible). Needs to be checked, of course.

How do we test which one is being used? I guess by making sure each is at a different URL

Yes.

And yes, we should figure out what other than referrer is affected. I think https state might be (and may not match referrer behavior in browsers!); not sure what else.

Loading

annevk added a commit that referenced this issue May 3, 2016
1490eba, from PR #484, removed the
source browsing context definition along with "explicit
self-navigation override". This restores the definition of source
browsing context for these elements as it was before that commit.

This fixes #1131, but note that per #1130 further changes are
required here, as browsing contexts are not a good concept to use as
source.
domenic added a commit that referenced this issue May 3, 2016
1490eba, from PR #484, removed the
source browsing context definition while removing the "explicit
self-navigation override". This restores the definition of source
browsing context for these elements as it was before that commit.

This fixes #1131, but note that per #1130 further changes are
required here, as browsing contexts are not a good concept to use as
source.
@annevk
Copy link
Member

@annevk annevk commented May 20, 2016

We should test what UAs do with the obvious edge case here: inserting an anchor into an initial about:blank, waiting for the new document to get loaded, and then doing click() on that anchor. I'm hoping the answer is "there is no navigation" or something and then I think the distinction becomes irrelevant....

No browser navigates there. Only Edge throws for click(). Only Firefox dispatches a click event. However, does that not mean that the distinction between global and document is relevant? Or would we put an active document check with the <a> element somewhere?

Loading

@annevk
Copy link
Member

@annevk annevk commented May 20, 2016

I created web-platform-tests/wpt#3060 to share the demos/tests I'm making.

Loading

@annevk
Copy link
Member

@annevk annevk commented May 20, 2016

The case I was worried about above is what happens when a window.open call happens and the incumbent global (and/or the entry global if we can arrange that somehow) corresponds to a no-longer-active document. In other words, load some frame, grab a function f from it, where f performs a window.open() call on some window (probably not the iframe's window itself, though it's interesting to test how UAs handle that too; I know Firefox has some security checks around it). Then navigate the iframe so the global of f corresponds to a no-longer-current document. Then call f either from your own script (so you're entry but f is incumbent) or by setting f as an event listener (so f is entry).

I tried the very simple case here and Edge gives "SCRIPT5011: Can't execute code from a freed script", Chrome gives "TypeError: Cannot read property 'open' of null", and Safari and Firefox log nothing and do nothing. This is the commit "Example 2" in my PR linked above.

Loading

@annevk
Copy link
Member

@annevk annevk commented May 20, 2016

Example 3 targets a named frame (instead of _self). Edge and Chrome give the same error. Firefox and Safari navigate the named frame to "HI" (Firefox used to sometimes fail in a non-deterministic way, but that might have been a cache issue).

Loading

@annevk
Copy link
Member

@annevk annevk commented May 20, 2016

Example 4 uses window.location from a navigated away document. Edge and Chrome give the same error as with 3. Safari fails silently. Works in Firefox.

Loading

@annevk
Copy link
Member

@annevk annevk commented May 20, 2016

(This seems to suggest that navigation does not have to work if the "relevant document" is not active. It does not really answer what "source browsing context" should become. Guess I should work on that next through finding some ways to navigate that do work across all browsers.)

Loading

@kengraWin

This comment was marked as spam.

@domenic
Copy link
Member

@domenic domenic commented Jul 17, 2020

There are not too many uses of "source browsing context" left in the navigation algorithm itself left:

  • The initial synchronous "allowed to navigate" check in step 3
    • Seems legit enough, although the "allowed to navigate" algorithm itself is quite old and crusty
  • The initial synchronous "preexisting attempt to navigate" check in step 4
    • Seems legit
  • Setting the request's client in step 1 of "process a navigate fetch".
    • On initial entry this is synchronous, but on recursive calls in the redirect case this is broken.
    • This could be more straightforward if it didn't indirect through browsing context's active document's relevant settings object, and instead the appropriate request client was passed in more directly. I.e., supplying a browsing context is a very indirect way to get a request client.
    • This should be relatively easy to fix by synchronously snapshotting the request client and passing that in to "process a navigate fetch" the first time, which it can then pass to itself recursively.
  • CSP check in step 5 of "process a navigate fetch".
    • Seems to have been refactored at some point; the CSP algorithm does not use the passed-in source value, but instead uses the request's client (i.e., the thing synchronously saved from the previous bullet). We should clean this up.
  • CSP check in step 1 of "process a navigate response":
    • Same as above; can be cleaned up
  • User activation check in passing its URL or data to an external software package
    • This "algorithm" is already sketchy at-a-distance, and it talks about "when the navigate algorithm was invoked", so in theory it's checking the property synchronously.
    • User activation is defined per Window now, so similar to the request client case, using a source browsing context is kind of indirect.
  • same-origin check in "execute a javascript: URL request"
    • Quite broken.
  • CSP check in step 2 of "execute a javascript: URL request"
    • Same as above; can be cleaned up
  • A new one being introduced in #5518
    • That one is actually about BCG switches though, so it might be legit. Let's discuss over there.

To summarize:

  • 2 clearly broken cases, the javascript: URL origin check and the redirects-involved request client
  • 3 CSP calls which don't actually use the source browsing context, and we can clean up
  • The rest seem reasonable; they're either synchronous or they're legimately browsing context related.

Loading

domenic added a commit to domenic/webappsec-csp that referenced this issue Jul 17, 2020
These algorithms do not need as many parameters as they are currently declared to take. This removes the redundant parameters so that HTML can stop supplying them, which helps with whatwg/html#1130.
@annevk
Copy link
Member

@annevk annevk commented Jul 20, 2020

"allowed to navigate" is #313 and very much wants a document/global I think, especially because of the various security checks that need to happen.

Note that passing the environment settings object into Fetch (and CSP relying on it) has known issues, as some policies (including CSP) can change over time.

In general source is used for security checks and a browsing context is not useful for that. Sure, we can get the state from the browsing context and hope navigation is only invoked synchronously (which things like lazy load are changing), but it seems much safer to adjust the signature.

Loading

domenic added a commit that referenced this issue Nov 4, 2020
annevk pushed a commit to w3c/webappsec-csp that referenced this issue Nov 4, 2020
These algorithms do not need as many parameters as they are currently declared to take. This removes the redundant parameters so that HTML can stop supplying them, which helps with whatwg/html#1130.
domenic added a commit that referenced this issue Nov 4, 2020
@domenic
Copy link
Member

@domenic domenic commented Mar 15, 2021

Update on what's left:

  • Navigate
    • Initial synchronous "allowed to navigate" check in step 3
    • Initial synchronous "preexisting attempt to navigate" check in step 4
  • Process a navigate fetch
    • Determining user activation in step 3 for setting request's user-activation flag
    • Setting the request's client in step 2
    • Determing COOP enforcement result in step 6
  • Process a navigate response
    • Allowed to download check in step 7 (the Content-Disposition case)
  • Passing its URL or data to an external software package
    • Transient activation "should" check
  • execute a javascript: URL request
    • Same-origin check in step 2

Loading

domenic added a commit that referenced this issue Mar 15, 2021
Helps with #1130 by removing a deep-in-the-algorithm-tree usage of source browsing context. Does not close #5597, but fixes the actual problems posed by the current architecture.
domenic added a commit that referenced this issue Mar 16, 2021
Helps with #1130 by removing a deep-in-the-algorithm-tree usage of source browsing context. Does not take care of #5597, but fixes the actual problems posed by the current architecture.
domenic added a commit that referenced this issue Mar 16, 2021
Helps with #1130 by removing a deep-in-the-algorithm-tree usage of source browsing context. Does not take care of #5597, but fixes the actual problems posed by the current architecture.
domenic added a commit that referenced this issue Mar 19, 2021
Helps with #1130 by removing more deep-in-the-algorithm-tree uses of source browsing context.
@domenic
Copy link
Member

@domenic domenic commented Mar 19, 2021

Update on what's left after #6497, #6512, and #6801:

  • Navigate
    • Initial synchronous "allowed to navigate" check in step 3
    • Initial synchronous "preexisting attempt to navigate" check in step 4
    • Synchronously determining hasTransientActivation and allowedToDownload
  • Process a navigate fetch
    • Setting the request's client in step 2
    • Determining COOP enforcement result by checking its active document's origin in step 6
  • Passing its URL or data to an external software package
    • References source browsing context's active document's origin as something the UA should tell users about

One thing I realized is that for the origin checks (which are the remaining major ones), I'm unsure whether the difference between incumbentNavigationOrigin and the source browsing context's active document's origin is intentional. I will try to do some digging in Chromium to see if that's the case.

Loading

domenic added a commit that referenced this issue Mar 24, 2021
Helps with #1130 by removing more deep-in-the-algorithm-tree uses of source browsing context.
domenic added a commit that referenced this issue Jun 23, 2021
This makes it non-racy by taking the origin snapshotted at the top of the navigation algorithm. Fixes #2591. Helps with #1130. See #6514 for related investigation.
domenic added a commit that referenced this issue Jun 25, 2021
This makes it non-racy by taking the origin snapshotted at the top of the navigation algorithm. It also switches to same origin-domain, which is more reasonable since if you have synchronous access to the document then you can just run JavaScript directly in it anyway. Fixes #2591. Helps with #1130. See #6514 for related investigation.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Linked pull requests

Successfully merging a pull request may close this issue.

None yet
4 participants