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

Handling bad URLs, navigation errors, downloads, and 204s in a unified way #248

Closed
domenic opened this issue Aug 28, 2020 · 4 comments
Closed

Comments

@domenic
Copy link
Collaborator

domenic commented Aug 28, 2020

Related to #145, #247, #228, #185, #158, #23.

A "bad URL" is a non-HTTP(S) URL, including data: URLs or about:blank or similar. Portals attempt to prohibit these, and I think that's awesome; they add a lot of complication to iframes. I'll use data:text/html,foo as the example, but the same goes for any of them.

Now, consider the following scenarios:

  1. Bad URL cases:
    1. <portal src="data:text/html,foo"></portal>
    2. <portal src="https://example.com/"></portal> where https://example.com/ 301 redirects to data:text/html,foo.
    3. <portal src="https://example.com/"></portal> where https://example.com/ performs location.href = "data:text/html,foo". (This category also includes other post-load navigations, e.g. <meta http-equiv="refresh"> or linkToBadURL.click().)
  2. Navigation error cases:
    1. <portal src="https://example.com/404"></portal>
    2. <portal src="https://example.unresolvable"></portal>
    3. <portal src="https://example.com/"></portal> where the page is prevented from loading by CSP
    4. <portal src="https://example.com"></portal> but the user is offline
  3. Downloads:
    1. <portal src="https://example.com/content-disposition-attachment"></portal>
    2. <portal src="https://example.com/></portal> where https://example.com/ 301 redirects to a Content-Disposition: attachment page
    3. <portal src="https://example.com/></portal> where https://example.com/ performs location.href = "content-disposition-attachment" or variants.
  4. 204 No Content:
    1. <portal src="https://example.com/204"></portal>
    2. <portal src="https://example.com/></portal> where https://example.com/ 301 redirects to a 204 page
    3. <portal src="https://example.com/></portal> where https://example.com/ performs location.href = "204" or variants.

The spec's currently handling:

  • (1.i) closes the portal
  • (1.ii)-(1.iii) navigate the portal browsing context to the bad URL
  • (2) navigates the portal to an error page
  • (3.i) performs the download while leaving the portal browsing context in its initial empty state, I guess?
  • (3.ii)-(3.iii) performs the download while leaving the portal browsing context on https://example.com/
  • (4.i) leaves the portal browsing context in its initial empty state, I think?
  • (4.ii)-(4.iii) leaves the portal browsing on https://example.com/.

We definitely need to fix (1.ii)-(1.iii), (3.i), and (4.i) in the spec.

We may also want to consider some unification. For example maybe all of (1) and (2) should close the portal, or maybe all of (1) and (2) should display an error page. For (3) and (4), things are a bit trickier, because (3.ii)-(3.iii) and (4.ii)-(4.iii)'s existing behavior is OK, but (3.i) and (4.i) is not. So should we special-case (3.i) and (4.i), or should we maybe treat all of (3) and (4) the same as (1) and (2), or...? (3) has other options, e.g. we could just ignore the Content-Disposition header in (3.i), or we could treat downloads as a no-op instead of causing an error/closing the portal.

I'd love more folks' opinions on this. If I had to pick, I'd go with something simple of (1), (2), (3), and (4) all displaying an error page. (And then we need to spec what happens when you try to activate an error page...)

@kjmcnee
Copy link
Collaborator

kjmcnee commented Aug 28, 2020

I would distinguish portal host initiated navigations from portal content initiated navigations, the latter being (1.iii), (3.iii), and (4.iii).

Especially with src changes causing reloads (#197), the portal host initiated navigations all produce a portal context that is empty (or effectively empty). At that point, it seems like we may as well discard such a context. An error page might work for some of these, but having an extra unactivatable portal state seems worth avoiding, plus having the page handle the discard rather than showing an error page seems like better UX.

For portal content initiated navigation, it seems sufficient to ignore/cancel the navigation, since we still have a usable portal context. Indeed, treating navigations that could be no-ops as ways for the portal context to self-destruct may be excessive, even though these are corner cases. However, a navigation that would be unacceptable even in a top level context (e.g. about:settings, about:flags) could be grounds for discarding.

@jeremyroman
Copy link
Collaborator

My instinct is that things that shouldn't load should be not committed (much like a 204 response would). If we combine that with the idea that src always creates a new browsing context, then we should close that browsing context the moment its initial navigation cannot commit (because that was a bad URL, or redirected to something invalid, etc). i.e.,

  1. Bad URL cases:

    1. <portal src="data:text/html,foo"></portal>

Immediately closed (but we can short-circuit that).

  1. <portal src="https://example.com/"></portal> where https://example.com/ 301 redirects to data:text/html,foo.

Closed when the redirect is found.

  1. <portal src="https://example.com/"></portal> where https://example.com/ performs location.href = "data:text/html,foo". (This category also includes other post-load navigations, e.g. <meta http-equiv="refresh"> or linkToBadURL.click().)

Remains on https://example.com/?

  1. Navigation error cases:

    1. <portal src="https://example.com/404"></portal>

Hmm. My instinct would be "render the 404 page" (it's content of a sort), but I could be convinced that in a cross-origin prerendering case we would rather close, on the basis that if we were prerendering we would abandon prerendering at that point.

We're kinda saved from this being a way to probing vector by the uncredentialed restriction, I think.

AFAIK, HTTP 404 doesn't make iframes fire error or other things like that.

  1. <portal src="https://example.unresolvable"></portal>

Probably close on resolution failure.

  1. <portal src="https://example.com/"></portal> where the page is prevented from loading by CSP

Probably close.

  1. <portal src="https://example.com"></portal> but the user is offline

Probably close.

  1. Downloads:

    1. <portal src="https://example.com/content-disposition-attachment"></portal>

Close? I don't think we want downloads and there's no inline-disposition content to render.

  1. <portal src="https://example.com/></portal> where https://example.com/ 301 redirects to a Content-Disposition: attachment page

Close.

  1. <portal src="https://example.com/></portal> where https://example.com/ performs location.href = "content-disposition-attachment" or variants.

Remain on the same page, since at least we have a committed page? I'm kinda reluctant to remove the previously shown/valid page and throw the whole thing out, but could be convinced.

  1. 204 No Content:

    1. <portal src="https://example.com/204"></portal>

Close.

  1. <portal src="https://example.com/></portal> where https://example.com/ 301 redirects to a 204 page

Close.

  1. <portal src="https://example.com/></portal> where https://example.com/ performs location.href = "204" or variants.

As above, remain?

The spec's currently handling:

  • (1.i) closes the portal
  • (1.ii)-(1.iii) navigate the portal browsing context to the bad URL
  • (2) navigates the portal to an error page
  • (3.i) performs the download while leaving the portal browsing context in its initial empty state, I guess?
  • (3.ii)-(3.iii) performs the download while leaving the portal browsing context on https://example.com/
  • (4.i) leaves the portal browsing context in its initial empty state, I think?
  • (4.ii)-(4.iii) leaves the portal browsing on https://example.com/.

We definitely need to fix (1.ii)-(1.iii), (3.i), and (4.i) in the spec.

We may also want to consider some unification. For example maybe all of (1) and (2) should close the portal, or maybe all of (1) and (2) should display an error page. For (3) and (4), things are a bit trickier, because (3.ii)-(3.iii) and (4.ii)-(4.iii)'s existing behavior is OK, but (3.i) and (4.i) is not. So should we special-case (3.i) and (4.i), or should we maybe treat all of (3) and (4) the same as (1) and (2), or...? (3) has other options, e.g. we could just ignore the Content-Disposition header in (3.i), or we could treat downloads as a no-op instead of causing an error/closing the portal.

I'd love more folks' opinions on this. If I had to pick, I'd go with something simple of (1), (2), (3), and (4) all displaying an error page. (And then we need to spec what happens when you try to activate an error page...)

But @kjmcnee has done more thinking about the concerns related to error pages and the UX and security reasons for/against, and I don't fully remember the past discussions about these cases.

@kjmcnee
Copy link
Collaborator

kjmcnee commented Sep 2, 2020

It looks like jeremyroman and my takes are the same.

It's not meaningful to activate "nothing" (a context that has no committed navigation) and activating an error page makes for a poor experience. I certainly think it would be worth it to avoid additional states in which a portal is unactivatable by simply discarding in the cases specified above. That seems like it would be easier for devs to reason about. I think there would also be UX benefits due to not exposing these corner cases to users, since they are not actionable.

With src changes creating new contexts, the logic is fairly straightforward to capture all of these cases. If a navigation fails to mature in a portal context which has yet to have another navigation mature, discard the context.

@domenic
Copy link
Collaborator Author

domenic commented Sep 2, 2020

Awesome! I'll try to spec all this and see how far I get.

domenic added a commit that referenced this issue Sep 3, 2020
domenic added a commit that referenced this issue Sep 17, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants