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

What happens if activate is called while the portal is navigating #227

Closed
jakearchibald opened this issue Jul 9, 2020 · 16 comments · Fixed by #253
Closed

What happens if activate is called while the portal is navigating #227

jakearchibald opened this issue Jul 9, 2020 · 16 comments · Fixed by #253
Assignees
Labels
design work needed An acknowledged gap in the proposal that needs design work to resolve spec todo A nitty-gritty detail that needs spec work

Comments

@jakearchibald
Copy link
Collaborator

We decided in #191 that the timing of activating a portal shouldn't be down to guesswork.

portal.src = otherURL;
portal.activate();

What should happen here? Should it wait for otherURL to fetch, or should it try and activate the current context (which may be absent)?

Similarly, what should we do if the portal is undergoing a navigation that was triggered internally?


If the src is set, it feels like activation should wait for that action to settle.

If the portal is navigating by some other means, waiting seems like an information leak. The alternative would be to abort that navigation, and activate the current document. This is giving the host page some control over an action stated within the portal, but it seems window.stop() can already do that? Thoughts @jyasskin?

@jakearchibald jakearchibald added design work needed An acknowledged gap in the proposal that needs design work to resolve spec todo A nitty-gritty detail that needs spec work labels Jul 9, 2020
@jakearchibald
Copy link
Collaborator Author

Related: If a portal is navigated while it's activating (either via the host page, or itself), what happens? My gut here is that those navigations are no-ops.

@jakearchibald
Copy link
Collaborator Author

One parallel here:

const img = new Image();
img.src = url;
await img.decode();

img.decode() will operate on the current value of src, and it doesn't fail if the image isn't 'ready'.

@jakearchibald
Copy link
Collaborator Author

Potential issue:

  1. Page A has CSP navigate-to 'self' 'unsafe-allow-redirects'.
  2. Page A loads a portal to /foo (which passes CSP).
  3. Page A sets portal src to /slowly-redirects-x-origin, which waits 5 seconds then redirects to another origin.
  4. Page A activates the portal.

What happens? If the redirect is allowed through, then it's a CSP bypass. I guess we'd have to block the redirect. Is it weird to do that after the portal has activated? Maybe it's fine.

@domenic
Copy link
Collaborator

domenic commented Jul 9, 2020

My preferred solution here, in the spirit of #191, is that the code in the OP should try to activate the current context. That context may be absent, causing a failure, or it might be something that exists, in which case it will activate while the navigation is ongoing.

If the developer wants to wait for otherURL to be ready before activating, they should instead write

portal.src = otherURL;
portal.onready = () => portal.activate();

Similarly, what should we do if the portal is undergoing a navigation that was triggered internally?

Similarly, this should activate while the navigation is ongoing. I.e., the previous page should become a TLBC, but the navigation is happening and so soon that TLBC might transition to a new page.

Related: If a portal is navigated while it's activating (either via the host page, or itself), what happens? My gut here is that those navigations are no-ops.

In my mind activation is atomic; it synchronously commits to activating once you call the method. This is supported by the spec in that step 4 ("update the user interface") happens synchronously from activate().

However, I realize that the spec doesn't actually switch the portal state until the posted task comes around, and there are of course implementation issues here; activating involves things happening in another process. From that point of view I'm not sure if step 4 happening synchronously is accurate...

But I think we can still maintain the mental model of activate() "commiting" you to the activation, by having any changes to src be no-ops, as you say.

If the redirect is allowed through, then it's a CSP bypass.

Why is it a CSP bypass? After step 4, /foo or /slowly-redirects-x-origin are in charge, and can redirect wherever they want. Page A's CSP no longer applies after step 4.

@jakearchibald
Copy link
Collaborator Author

the code in the OP should try to activate the current context. That context may be absent, causing a failure, or it might be something that exists, in which case it will activate while the navigation is ongoing.

The comparison to img.decode() wasn't compelling then? It just seems weird to me that a promise-based API would fail because things weren't ready, even though the developer had issued all the relevant instructions.

If the developer wants to wait for otherURL to be ready before activating, they should instead write

portal.src = otherURL;
portal.onready = () => portal.activate();

I'm a little worried that onready is going to be a tougher sell than delaying activate. onready gives you a signal that you can use silently, whereas activate commits you to a navigation. It seems kinda contentious #191 (comment).

From that point of view I'm not sure if step 4 happening synchronously is accurate...

Yeah, I'm proposing to make it async either way.

If the portal activates by moving a browsing context, but the portal navigation also changes browsing context, that feels like a fun race condition 😄 .

Why is it a CSP bypass? After step 4, /foo or /slowly-redirects-x-origin are in charge, and can redirect wherever they want. Page A's CSP no longer applies after step 4.

I think the problem is, although /foo or /slowly-redirects-x-origin are in charge, they didn't perform the navigation. Page A triggered the navigation, and if the portal didn't activate, Page A's rules would cause the navigation to fail. With the portal activation, the navigation succeeds.

Think of this this way:

Malicious code on Page A wants to navigate the page to example.com. /slowly-redirects-x-origin?example.com will do that. The malicious script calls location.href = '/slowly-redirects-x-origin?example.com', but that fails, because of Page A's CSP rules.

But then, the malicious code realises they can bypass this rule:

const portal = document.createElement('portal');
portal.src = '/slowly-redirects-x-origin?example.com';
document.body.append(portal);
portal.activate();

Maybe being able to bypass navigate-to using script isn't a concern.

@jakearchibald
Copy link
Collaborator Author

An alternative: portal.activate() could wait until the portal has a browsing context, but otherwise activates immediately. So if the portal already has a browsing context, then:

portal.src = otherURL;
portal.activate();

…would activate the current browsing context, which would navigate to otherURL.

Although I'm still unsure how that would work if portal navigations may cause a change of browsing context.

@jakearchibald
Copy link
Collaborator Author

For others reading along, TLBC = top level browsing context.

@domenic
Copy link
Collaborator

domenic commented Jul 10, 2020

The comparison to img.decode() wasn't compelling then? It just seems weird to me that a promise-based API would fail because things weren't ready, even though the developer had issued all the relevant instructions.

No. I don't think image decoding and portal activation have anything really in common.

I'm a little worried that onready is going to be a tougher sell than delaying activate. onready gives you a signal that you can use silently, whereas activate commits you to a navigation. It seems kinda contentious #191 (comment).

I think we have to decide whether or not we want activation to be a matter of guesswork. If we don't, then we need onready. But if we think it's an information leak, then we're just going to have to accept that activate() will be a matter of guesswork, IMO. Trying to patch things up by introducing unpredictable delays into the lifecycle doesn't seem like the right direction.

I think the problem is, although /foo or /slowly-redirects-x-origin are in charge, they didn't perform the navigation. Page A triggered the navigation, and if the portal didn't activate, Page A's rules would cause the navigation to fail. With the portal activation, the navigation succeeds.

I still don't think page A's rules matter anymore. Page A is out of the picture by that point.

The malicious script calls location.href = '/slowly-redirects-x-origin?example.com', but that fails

How does /slowly-redirects-x-origin work? Are you anticipating server-side slowness with a Location header, or client-side slowness, e.g. setTimeout(() => location.href = 'example.com', 10000)? The latter would not fail.

@jakearchibald
Copy link
Collaborator Author

jakearchibald commented Jul 10, 2020

No. I don't think image decoding and portal activation have anything really in common.

Hm, fair enough. They both have this pattern.

el.src = url;
await el.performAction();

With image decoding, it waits for the src to load enough to decode or fail, but with portals you have to get the timing right yourself.

I think we have to decide whether or not we want activation to be a matter of guesswork. If we don't, then we need onready. But if we think it's an information leak, then we're just going to have to accept that activate() will be a matter of guesswork, IMO. Trying to patch things up by introducing unpredictable delays into the lifecycle doesn't seem like the right direction.

I don't think we can release something where the core API involves guesswork like that. But I disagree with Mike that it's an information leak too, since you can get the same timing by polling the document of an iframe until it throws.

If we can have a ready promise that means "navigated" (and maybe also event?) then I guess it's fine. If we can have it for portals, we can have it for WindowProxy too. Should I pitch that on the html repo? That means we can have the privacy discussion cross-browser.

How does /slowly-redirects-x-origin work? Are you anticipating server-side slowness with a Location header

Yeah. It wouldn't even need to be that slow either, just as long as the redirect is processed after activation.

@domenic
Copy link
Collaborator

domenic commented Jul 10, 2020

I don't think we can release something where the core API involves guesswork like that.

As you point out, I think it'd be the same as using iframes (or <link rel="preload">) today. So I don't think it's that bad. It's OK to activate things even if they're not fully navigated; it might be subpar, but it'll just mean things are a little less instant.

@domenic
Copy link
Collaborator

domenic commented Jul 10, 2020

If we can have a ready promise that means "navigated" (and maybe also event?) then I guess it's fine. If we can have it for portals, we can have it for WindowProxy too. Should I pitch that on the html repo? That means we can have the privacy discussion cross-browser.

Yes, that seems reasonable. Basically proposing the same event for iframes, I guess.

@jakearchibald
Copy link
Collaborator Author

As you point out, I think it'd be the same as using iframes (or <link rel="preload">) today. So I don't think it's that bad. It's OK to activate things even if they're not fully navigated

I don't think so:

const portal = document.createElement('portal');
portal.src = url;

setTimeout(() => {
  portal.activate();
}, 1000);

The above resolves if url fetches to a non-error response, and enough body is received to warrant creating a browsing context.

So whether it rejects or not depends how fast your connection is, and how quick url's server is. If it rejects, you won't be able to tell the difference between "total failure" and "not ready yet".

@jakearchibald
Copy link
Collaborator Author

Yes, that seems reasonable. Basically proposing the same event for iframes, I guess.

whatwg/html#5725

@jeremyroman
Copy link
Collaborator

I think using the context is associated with the <portal> at that time makes sense, and if we do indeed make setting src always create a new context, then calling activate after setting src won't take you somewhere unexpected. We could whether we'd like it to fail if you're too early (and no navigate fetch response has arrived yet) as we do today, or we could wait for that to occur. It's fairly straightforward given either behavior to implement the other, I think?

@jakearchibald
Copy link
Collaborator Author

I think using the context is associated with the <portal> at that time makes sense, and if we do indeed make setting src always create a new context

Is that synchronous, or would it be racey?

@jeremyroman
Copy link
Collaborator

Teardown of an old context and creation of a new one is not synchronous, but reassociation with a new context can be. The element just requests a new one and as long as everything that actually changes the context itself happens asynchronously, it's fine.

You'd run into trouble if you wanted to synchronously access that context, or if you wanted to allow the existing page in the portal to prevent this via beforeunload, or something like that.

@domenic domenic self-assigned this Sep 28, 2020
domenic added a commit that referenced this issue Sep 28, 2020
Closes #197 by causing all changes to src="" to close the portal browsing context, and re-create it.

Closes #227 by causing activate() to delay promise resolution/rejection until the initial load in a new browsing context has finished.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
design work needed An acknowledged gap in the proposal that needs design work to resolve spec todo A nitty-gritty detail that needs spec work
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants