Prevent navigations to non-HTTP(S) URLs #247
Conversation
There was a problem hiding this comment.
@kjmcnee I think Chromium presently just prevents the navigation rather than closing the portal; does that match your recollection? Any opinions one way or the other?
index.bs
Outdated
|
|
||
| 1. If |browsingContext| is a [=portal browsing context=], and <var ignore>response</var>'s | ||
| [=response/url=] is either null or is a [=URL=] whose [=url/scheme=] is not a [=HTTP(S) | ||
| scheme=], then: |
There was a problem hiding this comment.
IIUC this happens after all redirects etc. Do you know if there are any other fetch schemes that could do redirects, and thus would be missed here? Would it matter if they were?
There was a problem hiding this comment.
Is this the registerProtocolHandler case? If so, I'm not sure what we do in Chromium and we should probably file a bug to figure out out.
|
I'm going to open a new issue for preventing navigation vs. error pages vs. closing the portal. |
Right, we currently ignore/cancel the navigation. We have https://crbug.com/1058489 which is consistent with case 1, I think. I'm not as sure about closing for case 2 instead of canceling, since that would cause the portal to close out from under its host, rather than due to the host's action. That said, these seem like exceptional cases, and if the portal apis encourage pages to consider portals being discarded, then consistency here sounds good. Also, I agree on case 5. |
domenic
force-pushed
the
no-bad-redirects
branch
from
9010024 to
75d3155
Compare
|
Hmm, OK, one interesting question: should we remove the early-close on In particular, consider if you did |
|
Agreed it seems kind of nice, though I don't have enough familiarity with |
|
registerProtocolHandler is pretty reliable; I don't know how high usage is, but I think it points toward an architectural principle here that we shouldn't be acting on |
|
Updated the OP with a list of test cases. Review appreciated! |
index.bs
Outdated
| first step of the queued task: | ||
|
|
||
| 1. If |browsingContext|'s [=portal state=] is not "`none`", then [=close a portal | ||
| element|close=] |browsingContext|'s [=host element=] and abort these steps. |
There was a problem hiding this comment.
Hmm, is this the right fix here? This might be silly but I'm not sure what's actually wrong with it:
<!-- in a portal -->
<a id="loadmore" href="javascript:loadMoreStuff()">load more</a>
<script>
onload = () => document.getElementById("loadmore").click();
</script>
I think I would have expected to ban javascript: from portal src, so you can't start there, and then otherwise permit it (since the document you would load still has a URL that's legal to load in a portal, even if the javascript: URL does replace the document -- and mostly it wouldn't).
This is a very silly edge case, though, so I don't feel strongly.
There was a problem hiding this comment.
Great catch. I forgot javascript: in my case analysis, at least mentally, so I didn't properly harmonize it with our desired outcome for the others.
There was a problem hiding this comment.
So for string-returning JavaScript snippets, the result gets the default 200 status code. I think we want that to close the portal. It's the non-string returning ones that we want to allow.
There was a problem hiding this comment.
Hmm. Well, what falls out naturally from the existing spec text is a bit different.
If we delete this section entirely, we get the following:
- If the JS returns a string, then it will create a 200 response with null url.
- If we're in the initial-load case, this will close the portal
- If we're in the non-initial-load case, then this will bail out and do nothing
- If the JS returns a non-string, then it will create a 204 response with null url.
- Exact same as in the string case.
I guess that's OK? Maybe this just reflects the bad existing JS spec; e.g. maybe some of those responses should be non-null URL. I can look into that, but perhaps we should just move ahead with this PR by deleting this section.
There was a problem hiding this comment.
Final (?) reply-to-myself: it looks like the existing spec is correct in this; both string and non-string cases have null response URL.
There was a problem hiding this comment.
I'm not sure whether the response URL is null or not for the generated response (I don't see anywhere that sets it), but document.URL is not -- it remains the same as the page it replaced (or possibly the initiator?):
Your examples both print document.URL before the synthetic response is generated (because the expression is still being evaluated), and I think they're about:blank because it's the initial about:blank that's executing the javascript: URL in the case where you directly set it as iframe src.
I definitely care way more about non-string ones than string ones (does anyone actually use the string case?), but if it does get to keep the URL of the page it replaces then I would have expected to preserve that, odd as it is.
There was a problem hiding this comment.
Thanks for correcting my bad examples :). Do you think there's anything we should modify in this PR though? Or is this good to merge?
domenic
force-pushed
the
no-bad-redirects
branch
from
75d3155 to
9f60901
Compare
|
Alright, pushed to address comments. Yay for review! |
Closes #145.
Test cases this should generate:
For each
badURLofdata:text/html,foo,javascript:'a string',javascript:undefined,about:blank, ablob:URL, and a real URL that is served with theContent-Disposition: attachmentheader, a real URL that has a 204 response, and a real URL that has a 205 response:<portal src="badURL"></portal>must end up closed<portal src="redirect"></portal>whereredirectredirects (say, 302) tobadURLmust end up closed<portal src="web+test:foo"></portal>whereweb+testis mapped usingregisterProtocolHandlertobadURLmust end up closed. (Only possible for same-origin HTTPS bad URLs; you can't register the others.)<portal src="test"></portal>wheretestdoeslocation.href = badURLmust stay ontestand not end up closed<portal src="test"></portal>wheretestcontains<meta http-equiv="refresh" content="0; URL=badURL">must stay ontestand not end up closed<portal src="test"></portal>wheretestcontains<a href="badURL">a</a><script>document.querySelector("a").click()must stay ontestand not end up closed<portal src="404"></portal>must show the 404 page from the server, and not end up closed<portal src="500"></portal>must show the 500 page from the server, and not end up closedregisterProtocolHandler("web+soup", "soup?url=%s")+<portal src="web+soup:chicken-kïwi"></portal>should load, and not end up closed, despite the non-HTTP(S)src="".<portal src="unresolvable"></portal>must end up closedPreview | Diff