I think there are two major design choices here:

• do we want a generic iframe facility in core (that solves T169027: Provide iframe sandboxing for rich-media extensions (defense in depth)), or something Graph-specific?
• do we embed all iframes in the main page using srcdoc (support; works on all Grade A browsers, and all Grade C browsers except IE 11; results in large page size) or traditional iframes (results in a bunch of extra requests + need to pass state, which imposes GET length constraints)?

If we go with the srcdoc version, I guess we'd create a new OutputPage object and have it return the HTML for the embedded page (ApiParse does that so it's not completely untrodden ground). If we go with the traditional version, we'd have a Special:Iframe or Special:GraphIframe page, pass it the graph specification (or arbitrary HTML body + other OutputPage metadata if we go with the non-Graph-specific version), and probably sign it cryptographically to ensure it's an actual graph persisted in wikitext (in case there is a new vulnerability, dealing with persistent XSS is much easier than dealing with reflected XSS).

The iframe doesn't have to be there at page load time, it can be injected by the graph loader JS on scroll, but the data (value of src or srcdoc) needs to be passed by the server, because the JS code can't sign and can't generate a sufficiently complex HTML (with ResourceLoader modules and whatnot).

@sbassett does that sound reasonable?
@Krinkle if you have some time for this, I'd appreciate your thoughts on the performance aspects of either.

Or maybe the signature part is overkill, since the iframe will be sandboxed anyway.

traditional iframes (results in a bunch of extra requests + need to pass state, which imposes GET length constraints)?

Traditional iframes also need to be on a different domain to be effective sandboxes (Otherwise you just load it directly). Also matching your iframe request to the correct ParserOutput that generated it is very difficult in mediawiki

Anyways, my suggestion would be to generate the iframe in javascript entirely. Graph is already 95% of the way there, in the way it generates vega stuff so it would be a relatively simple change. Just have it render inside a newly created iframe. Most difficult part would be to have some sort of fake RL loader to load the javascript. If IE11 support is desired, we could always detect it on client side and have a less secure fallback like using javascript: urls

I'd like to consider this as part of the generate fragment rendering infrastructure proposed for WikiLambda. Once you have a way to name, store, and retrieve "fragments" of an individual page, the iframe encapsulation issue becomes almost trivial.

In T222807#8811093, @Bawolff wrote:

Traditional iframes also need to be on a different domain to be effective sandboxes (Otherwise you just load it directly).

I mean, traditional in the sense of loading it via an URL. It would still be sandboxed. That would make it behave as if it were from a different domain on almost all browsers where we actually load Javascript

Also matching your iframe request to the correct ParserOutput that generated it is very difficult in mediawiki.

Yeah, as I said I would just serialize the ParserOutput (or the graph itself) and pass it as a signed parameter. That puts a limit on the size but that might not be a big deal for graphs.

(Although now that I think about it, I'm not sure it's actually that hard, you just pass the ParserOptions key as a parameter, and then it can be retrieved from cache. And apparently the Graph API already does something like that, even if at the moment it's unused.)

Anyways, my suggestion would be to generate the iframe in javascript entirely. [...] Most difficult part would be to have some sort of fake RL loader to load the javascript.

Yeah, not sure that's worth the maintenance overhead when generating the same page on the server side is trivial. Javascript can still be in charge of when to generate it, but doing it via API / embedded content seems simpler.

In T222807#8811187, @cscott wrote:

Once you have a way to name, store, and retrieve "fragments" of an individual page

That sorta exists, via extension data in the ParserOutput. Graph uses it already. That's how graphoid fetched the graph definition IIRC although that part was killed due to lack of maintenance and other problems.

I mean, traditional in the sense of loading it via an URL. It would still be sandboxed. That would make it behave as if it were from a different domain on almost all browsers where we actually load Javascript

I just mean, if its accessible on some specific url, then the attacker can navigate the browser directly to the url to bypass the iframe (I suppose there are work arounds to that with CSP headers).

(Although now that I think about it, I'm not sure it's actually that hard, you just pass the ParserOptions key as a parameter, and then it can be retrieved from cache. And apparently the Graph API already does something like that, even if at the moment it's unused.)

There is not a 1:1 relationship between parser options key and actual parser output. e.g. {{#tag:graph| ... {{NUMBEROFARTICLES}}...}} [That's a contrived example, but there are many non-contrived examples such as old revisions or parser output on a special page or ?action=info]

In T222807#8811274, @Bawolff wrote:

I just mean, if its accessible on some specific url, then the attacker can navigate the browser directly to the url to bypass the iframe (I suppose there are work arounds to that with CSP headers).

Or just detect whether we are sandboxed and refuse to load if not. You are right that it's more complex than just having the parent document generate the iframe contents.

There is not a 1:1 relationship between parser options key and actual parser output. e.g. {{#tag:graph| ... {{NUMBEROFARTICLES}}...}} [That's a contrived example, but there are many non-contrived examples such as old revisions or parser output on a special page or ?action=info]

For old revisions it would be the revision output cache, not the parser cache proper, but it doesn't matter much, ParserOutputAccess takes care of the difference. I guess for a non-public revision trying to pass information via the cache would be problematic. (action=info does not need an iframe since the graph is not user-defined; the NUMBEROFARTICLES example I do not get - why would that not work?)

I mean, for any dynamic parser function, you'd be kind of hoping that you retrieve the same parser output. {{NUMBEROFARTICLES}} is commonly used as a very bad pseudo-random number generator on enwiki. It seems like you would often retrieve different parser outputs and the graphs wouldn't match if they used dynamic parser functions, unless i misunderstand the plan.

It would be the same parser output since it would come from the parser cache. In any case, I don't think it's a good implementation choice, just that it's possible.

I'd probably go with rendering a small iframe HTML document for each graph on the server side when parsing the wiki page, and embedding them via srcdoc. I don't think that's a significant performance hit.

Using srcdoc means you can't use headers (such as CSP) though, that seems somewhat painful.

It would be the same parser output since it would come from the parser cache

I don't think that is likely to be true.

Using srcdoc means you can't use headers (such as CSP) though, that seems somewhat painful.

There is support in chrome but not firefox - https://developer.mozilla.org/en-US/docs/Web/API/HTMLIFrameElement/csp

However does it really matter (if using sandbox attribute to set a null origin)? There is still the privacy concern of leaking ip if loading third party urls, but it seems like that can be enforced in vega wrappers (like what we previously did) and is less critical so probably doesn't need browser level sandboxing. (Edit - i mean, assuming vega protections are just sketch and not trivially broken)

Edit: also i assume, that even on browsers not supporting the csp attribute on the iframe, you can still use a csp meta tag in the srcdoc to do the same thing.

In T222807#8848776, @Bawolff wrote:

It would be the same parser output since it would come from the parser cache

I don't think that is likely to be true.

How so? If you request a parser output from ParserOutputAccess with the same parameters as the one in the cache, won't it always return the one in the cache? And there will almost always be one in the cache, since the page did just get displayed to the user. There are some edge cases e.g. with uncacheable pages, but they don't seem important.

Edit: also i assume, that even on browsers not supporting the csp attribute on the iframe, you can still use a csp meta tag in the srcdoc to do the same thing.

Yeah, I didn't mean we can't do CSP in an iframe at all, just that we would have to rework our entire CSP framework, which seems like a lot of effort compared to the otherwise fairly simple job of writing an iframe generator.

A rough sketch:

• Graphs are embedded as <iframe sandbox="allow-script" srcdoc="<some HTML document source>" /> (on demand as they scroll into view). Since the iframe is sandboxed and does not have the allow-same-origin permission, this will more or less cause the browser to treat it as a separate domain with the domain name null, i.e. no access to cookies. (caniuse: sandbox, srcdoc - in browsers which don't support sandboxing, graphs wouldn't load at all. We could probably add an src pointing to an error page.) With a CORS Allow-Origin of * for unauthenticated read API requests, the null origin still works (this is not super well documented but that's my reading of the spec and the explainer kinda-sorta suggests it too) so assuming we ever fix the custom protocols, the iframing wouldn't interfere with those.
• As layered defense, make the iframe's JS logic abort (not load the graph) if the origin is not null, or if the user has auth cookies.
• Set an agressive CSP policy to prevent the iframe from talking to non-Wikimedia domains, so an XSS can't be used to extract user IPs and such. (Iframes don't have headers, so we'd have to modify the ContentSecurityPolicy class to optionally return the policy as meta tags instead. Looking at the code, that seems straightforward.)
• To generate the iframes, we can probably use OutputPage::output( true ) with a fake context (to avoid emitting any headers), when the page is parsed. (At that point we have access to the Vega spec so the data flow is straightforward.) The document could just have an empty body; it would load the ext.graph.render RL module and contain the graph specification as a script that passes it to the renderer. The entire iframe source would then be converted into a JS string and passed to the iframe loader as part of the article page.

Security-wise, this seems solid: iframe sandbox security has the security resources of major browser vendors behind it, the way we rely on its security properties is straightforward, and checking the origin and auth cookies is a still pretty reliable second line of defense. In terms of performance, 1) it would add some bloat to graph specifications (they would be wrapped in an iframe), which doesn't seem like a big deal; 2) it would result in some ResourceLoader requests being repeated once per graph on the page, but they would be read from browser cache (or localStorage cache) so the effect is small, 3) the browser would have to execute the same Javascript several times, in each iframe separately, which still seems minor compared to the amount of JS work graph rendering itself requires.

@Tgr that sounds reasonable. My only concern is that the mechanism seems pretty ad-hoc; I wonder if the basic logic can be factored out in a way which makes it straightforward to reuse for arbitrary content fragments. I'm thinking of something like OutputPage::createIframe($modules, $scriptSrc, $innerHtml) which encapsulates:

• generating the <iframe> tag with the appropriate sandbox attribute and srcdoc
• generating the srcdoc with <meta> tags for the "aggressive CSP policy" and appropriate resource loader hookups for $modules
• "make the iframe's JS logic abort if the origin is not null" otherwise executing $scriptSrc
• setting the src attribute of the iframe and the "JS logic abort" to an appropriate error page

and then returns the result as a string which can either (a) be directly embedded in the page, or (b) converted to a JS string to pass to an on-demand loader.

(Closely reading the above, it seems like for the Graph implementation the $innerHtml would likely be the empty string, with all the work done by $scriptSrc. But still worthwhile to expose the ability to add body contents in a general purpose API.)

Change 948186 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/core@master] WIP: Add OutputPage::createIFrame() for creating iframe sandboxes

https://gerrit.wikimedia.org/r/948186

Change 948187 had a related patch set uploaded (by C. Scott Ananian; author: C. Scott Ananian):

[mediawiki/core@master] DNP: add a simple iframe to Special:Blankpage for testing

https://gerrit.wikimedia.org/r/948187

^ above are some initial implementations of an API in OutputPage for discussion.

Change 973283 had a related patch set uploaded (by Gergő Tisza; author: C. Scott Ananian):

[mediawiki/core@master] WIP: Output: Add IframeSandbox class

https://gerrit.wikimedia.org/r/973283

Change 975120 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/extensions/Graph@master] [WIP] Use IframeSandbox

https://gerrit.wikimedia.org/r/975120

Change 973283 merged by jenkins-bot:

[mediawiki/core@master] Output: Add IframeSandbox class

https://gerrit.wikimedia.org/r/973283

What's the status of this? It's been two months and a half without any updates or public progress done.

In T222807#9561360, @aliu wrote:

What's the status of this? It's been two months and a half without any updates or public progress done.

See T334940#9537862

Change 975120 abandoned by Gergő Tisza:

[mediawiki/extensions/Graph@master] [POC] Use IframeSandbox

Reason:

Per T334940#9537862, we are not going to use iframes.

https://gerrit.wikimedia.org/r/975120

Change 1008466 had a related patch set uploaded (by Gergő Tisza; author: Gergő Tisza):

[mediawiki/core@master] Revert "Output: Add IframeSandbox class"

https://gerrit.wikimedia.org/r/1008466

Change 1008466 merged by jenkins-bot:

[mediawiki/core@master] Revert "Output: Add IframeSandbox class"

https://gerrit.wikimedia.org/r/1008466