Skip to content

/ servo

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

Stop iframes from including their ancestors #25668

Merged
merged 1 commit into from Feb 24, 2020
+24 −1
Merged

Stop iframes from including their ancestors #25668

Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter...
Filter file types
Jump to…
Jump to file
Failed to load files.

Always

Just for now

25 components/script/dom/htmliframeelement.rs
View file
Open in desktop
@@ -287,7 +287,30 @@ impl HTMLIFrameElement {

let url = self.get_url();

// TODO: check ancestor browsing contexts for same URL
// TODO(#25748):
// By spec, we return early if there's an ancestor browsing context
// "whose active document's url, ignoring fragments, is equal".
// However, asking about ancestor browsing contexts is more nuanced than
// it sounds and not implemented here.
// Within a single origin, we can do it by walking window proxies,
// and this check covers only that single-origin case, protecting
// against simple typo self-includes but nothing more elaborate.
let mut ancestor = window.GetParent();
while let Some(a) = ancestor {
This conversation was marked as resolved by jdm

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Jan 31, 2020

Member

This implementation is currently limited to same-origin iframes; unfortunately we need to query the constellation for the full iframe ancestor chain to be certain.

This comment has been minimized.

Sign in to view
Copy link
@pshaughn

pshaughn Jan 31, 2020

Author Member

Thanks for pointing that out. Could you give me a rough sketch of how that goes?

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Feb 1, 2020

Member
  1. add a new message to

    servo/components/script_traits/script_msg.rs

    Line 117 in 688d076

    /// Messages from the script to the constellation.
    that contains an IpcSender that will be used to send the response, along with the desired url and the frame's BrowsingContextId
  2. add a message handler to

    servo/components/constellation/constellation.rs

    Line 1688 in 688d076

    fn handle_request_from_script(&mut self, message: (PipelineId, FromScriptMsg)) {
    that checks the browsing_context map, then gets the browsing context's parent pipeline id (

    servo/components/constellation/browsingcontext.rs

    Line 63 in 688d076

    pub parent_pipeline_id: Option<PipelineId>,
    ) and uses that to get the pipeline from pipelines, which contains a browsing context id which corresponds with the parent. Repeat until there are no parents.
  3. send the result of checking the ancestors on the provided ipc sender.
  4. in the script thread code, use the global scope's script_to_constellation_chan to send the new message, first creating a new ipc channel to include in it
  5. wait on receiving a message on the new ipc channel receiver

This comment has been minimized.

Sign in to view
Copy link
@pshaughn

pshaughn Feb 1, 2020

Author Member

Does checking the url of each browsing context have to happen on that browsing context's own script thread?

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Feb 1, 2020

Member

Are you asking if the check could happen as part of navigation in the constellation instead? I don't know the answer without reading the spec, sadly.

This comment has been minimized.

Sign in to view
Copy link
@pshaughn

pshaughn Feb 1, 2020

Author Member

What I'm not sure about is how to ask a browsing context "what's your URL" without crossing a memory boundary I'm not supposed to cross. This problem might be better tackled by someone who's gone deeper into Servo's threading systems.

This comment has been minimized.

Sign in to view
Copy link
@pshaughn

pshaughn Feb 3, 2020

Author Member

Simplifying realization: if it's not the same script thread that asked the question, it must also not be the same URL, since within an origin we're sticking to one thread. Constellation could hand back a list of contexts, then script thread could iterate them to ask the question of the same-origin ones.

This comment has been minimized.

Sign in to view
Copy link
@pshaughn

pshaughn Feb 3, 2020

Author Member

I can imagine the list of parents changing in mid-check due to something happening in another script thread, like if we're in a complicated tree of iframes and a cross-origin parent hands us off for adoption by another iframe that's also in that origin. The implied synchronousness of this ancestry-check operation in the spec doesn't really match the way Servo does things.

This comment has been minimized.

Sign in to view
Copy link
@jdm

jdm Feb 10, 2020

Member

Ick, yeah. I think I see what you're getting at. One solution might be for the constellation to reply with a list of channels to ancestor script threads, and the original script thread can then query those threads directly.

This comment has been minimized.

Sign in to view
Copy link
@gterzian

gterzian Feb 17, 2020

Member

Does the cross-origin situation apply here? As I understand it, here we are operating on the iframe element, the BC container, not the actual nested BC which potentially is cross-site in which case it would be running in another process.

(and if the nested BC that will be loaded as a result of this navigation is cross-site, I think this implies by definition that the parent BC does not have the same url?)

This comment has been minimized.

Sign in to view
Copy link
@gterzian

gterzian Feb 17, 2020

Member

Ah you mean if the nested BC is cross-site and then loads another iframe for that is cross-site to it, but matches the url of the BC where the first iframe is contained within? Yeah that's a tough one.

This comment has been minimized.

Sign in to view
Copy link
@gterzian

gterzian Feb 17, 2020 •
edited

Member

I think you might want to not completely follow the spec to the letter here. So instead of returning, you could not go ahead with the navigation when the ScriptMsg::ScriptLoadedURLInIFrame is handled by the constellation, using the old_pipeline_id field to check the url of all Pipeline that are ancestor to it?

So you'd have to essentially to the same check as here(as described above by @jdm), but in the constellation over at

servo/components/constellation/constellation.rs

Line 2940 in 89384ff

fn handle_script_loaded_url_in_iframe_msg(&mut self, load_info: IFrameLoadInfoWithData) {

So I think the "local" check you perform here is the "early abort" case that matches the spec. And then in the cross-site case, you'd have to do the check a bit later when the navigation is handled by the constellation. Since the spec only returns, I think the effect of where the check happens will not be noticeable to script (unloading only happens later if the navigation goes through, I think).

You could simply keep both checks, since the second one is "stricter" than this one.

I think it could be done in a separate PR

This comment has been minimized.

Sign in to view
Copy link
@gterzian

gterzian Feb 17, 2020 •
edited

Member

One last thing, if the navigation were to be disallowed in the constellation, you'd probably have to send a message to the script-thread to undo some of the changes that occur as part of

servo/components/script/dom/htmliframeelement.rs

Line 109 in 89384ff

pub fn navigate_or_reload_child_browsing_context(
when the navigation is initiated.

An alternative off-course is the blocking workflow outlined by @jdm above, where the check would happen in the constellation, but the script-thread would block on the result.
if let Some(ancestor_url) = a.document().map(|d| d.url()) {
if ancestor_url.scheme() == url.scheme() &&
ancestor_url.username() == url.username() &&
ancestor_url.password() == url.password() &&
ancestor_url.host() == url.host() &&
ancestor_url.port() == url.port() &&
ancestor_url.path() == url.path() &&
ancestor_url.query() == url.query()
{
return;
}
}
ancestor = a.parent().map(|p| DomRoot::from_ref(p));
}

let creator_pipeline_id = if url.as_str() == "about:blank" {
Some(window.upcast::<GlobalScope>().pipeline_id())
ProTip! Use n and p to navigate between commits in a pull request.
You can’t perform that action at this time.