third_party/WebKit/Source/core/loader/HttpEquiv.cpp - Issue 1833063002: Store the list of trial tokens in OriginTrialContext

Unified Diff: third_party/WebKit/Source/core/loader/HttpEquiv.cpp

Issue 1833063002: Store the list of trial tokens in OriginTrialContext (Closed) Base URL: https://chromium.googlesource.com/chromium/src.git@master

Patch Set: Created 4 years, 9 months ago

Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.

Jump to:

View side-by-side diff with in-line comments

Download patch

« third_party/WebKit/LayoutTests/http/tests/origin_trials/javascript-insert-token.html ('K') | « third_party/WebKit/Source/core/dom/ExecutionContext.cpp ('k') | third_party/WebKit/Source/core/origin_trials/DocumentOriginTrialContext.h » ('j') | third_party/WebKit/Source/core/origin_trials/OriginTrialsBase.h » ('J')
Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

Index: third_party/WebKit/Source/core/loader/HttpEquiv.cpp

diff --git a/third_party/WebKit/Source/core/loader/HttpEquiv.cpp b/third_party/WebKit/Source/core/loader/HttpEquiv.cpp

index 26447d3b2f3e86bc72df7bac709ff79edf455f69..6cf60e70d802fcecc90d31a43070fed7aa6c38bc 100644

--- a/third_party/WebKit/Source/core/loader/HttpEquiv.cpp

+++ b/third_party/WebKit/Source/core/loader/HttpEquiv.cpp

@@ -5,6 +5,7 @@

#include "core/loader/HttpEquiv.h"

#include "core/dom/Document.h"

+#include "core/dom/ScriptableDocumentParser.h"

#include "core/dom/StyleEngine.h"

#include "core/fetch/ClientHintsPreferences.h"

#include "core/frame/UseCounter.h"

@@ -12,6 +13,7 @@

#include "core/html/HTMLDocument.h"

#include "core/inspector/ConsoleMessage.h"

#include "core/loader/DocumentLoader.h"

+#include "core/origin_trials/OriginTrials.h"

#include "platform/network/HTTPParsers.h"

#include "platform/weborigin/KURL.h"

@@ -42,6 +44,10 @@ void HttpEquiv::process(Document& document, const AtomicString& equiv, const Ato

document.contentSecurityPolicy()->reportMetaOutsideHead(content);

} else if (equalIgnoringCase(equiv, "suborigin")) {

document.addConsoleMessage(ConsoleMessage::create(SecurityMessageSource, ErrorMessageLevel, "Error with Suborigin header: Suborigin header with value '" + content + "' was delivered via a <meta> element and not an HTTP header, which is disallowed. The Suborigin has been ignored."));

+ } else if (equalIgnoringCase(equiv, HTTPNames::Origin_Trial)) {

iclelland 2016/03/31 15:00:00 I like using HTTPNames here -- do you have any ide

Marijn Kruisselbrink 2016/03/31 19:01:48 No idea, so maybe core/ owners will object to usin

iclelland 2016/04/01 15:19:09 Okay; we should be explicit and #include "platform

+ bool isScriptGenerated = document.hasFinishedParsing() || (document.scriptableDocumentParser() && document.scriptableDocumentParser()->isExecutingScript());

Marijn Kruisselbrink 2016/03/25 19:08:39 I'm not sure if this is entirely the right check t

iclelland 2016/03/31 15:00:00 You can see what I'd done in https://codereview.ch

Marijn Kruisselbrink 2016/03/31 19:01:48 Yeah, I wasn't sure what situations we actually wa

iclelland 2016/04/01 15:19:09 Let's keep this here, then -- I haven't been able

Marijn Kruisselbrink 2016/04/01 23:54:47 I started trying to specify what behavior we're ai

I started trying to specify what behavior we're aiming for at https://mkruisselbrink.github.io/ExperimentalFramework/ (and as a pull request). As you speculate onload/onerror on a link attribute is indeed another way to execute script in the HEAD without a script tag. That one isn't actually caught by my current implementation (so I added it to the layout test, but commented out the link tag). But I do wonder how important it is to block all this anyway. Where did this requirement come from?

iclelland 2016/04/05 17:26:28 Out of a bunch of discussions, so a spec is probab

On 2016/04/01 23:54:47, Marijn Kruisselbrink wrote: > On 2016/04/01 at 15:19:09, iclelland wrote: > > On 2016/03/31 19:01:48, Marijn Kruisselbrink wrote: > > > On 2016/03/31 at 15:00:00, iclelland wrote: > > > > On 2016/03/25 19:08:39, Marijn Kruisselbrink wrote: > > > > > I'm not sure if this is entirely the right check to detect any > script-made > > > > > modifications to these meta tags (nor am I entirely sure what we exactly > > > want to > > > > > prevent), but this seems to work for the cases I've tested. > > > > > > > > You can see what I'd done in https://codereview.chromium.org/1840193006/ > (WIP, > > > uploaded for comparison here). Rather than specifically checking whether a > > > script is running, I figured we'd probably want to have a concept of > > > 'finalizing' the set of tokens, either when the <head> closes out, or the > first > > > <script> tag is encountered. Then we can't get any XSS-added tags, and we > also > > > never get a situation where a trial is disabled in one script (before the > meta > > > tag) and enabled in the next (after it). > > > > > > Yeah, I wasn't sure what situations we actually want to allow/prevent. If in > the > > > case of <head><script><meta><script></head> we indeed want to ignore the > tokens > > > in the meta tag then doing something like what you did is probably a better > > > approach. Probably for things like this it might actually make sense to do > try > > > to come up with a more formal spec to specify how/when tokens should get > > > applied. Even if we don't expect other browser to follow the same meta tag > > > format, clearly documenting these things still seems like a good idea (and > gives > > > us something to point to for the meta tags and http header registries). > > > > Let's keep this here, then -- I haven't been able to completely rule out JS > execution in the HEAD yet (maybe something like an onload="" attribute in a > <link> tag) > > I'll merge my code in afterwards to restrict the placement of the tags to > pre-script only. > > I started trying to specify what behavior we're aiming for at > https://mkruisselbrink.github.io/ExperimentalFramework/ (and as a pull request). > As you speculate onload/onerror on a link attribute is indeed another way to > execute script in the HEAD without a script tag. That one isn't actually caught > by my current implementation (so I added it to the layout test, but commented > out the link tag). > > But I do wonder how important it is to block all this anyway. Where did this > requirement come from?

Out of a bunch of discussions, so a spec is probably in order. I think part of what we're aiming for is the principle of least surprise -- - Can we avoid a situation where one script can't see the experimental attributes, but the next script can? That could mess up feature detection, if libraries do it early enough. Also, we want to avoid XSS attacks - especially stored XSS, where an attacker could enable a trial on a page that the page's owner doesn't want it used on. If we can say that the tokens must be present before any JS runs, then it doesn't matter when we initialize the objects in V8, and we could eventually get to a place where the JS world is just constructed with the right experimental features enabled, with no possibility of initialization-after-the-fact. If we allow tokens after script tags, then we can't do that.

Marijn Kruisselbrink 2016/04/05 20:56:08 Hmmm, okay. At least it seems the code in this CL

Hmmm, okay. At least it seems the code in this CL is a step in the right direction, even if it doesn't fully capture whatever behavior we want. I'm not sure I entirely get what kind of XSS attacks we want to avoid, but that's probably best to have as a separate discussion on the spec repo. And the answer to that will probably also depend on what we decide to do about trial tokens for about:blank, blob urls, data urls etc. I don't think "before any JS runs" is very well defined though (and rather hard to define precisely: what about one page including another page in an iframe. The iframed page never "runs and JS", but the outside page can run JS and do whatever it wants with the iframe). And what if in the future we launch some experimental css feature using the experimental framework. Do we then all of a sudden also ignore all tokens after the first <link> and/or <style> elements? Or even the first element with an inline style= attribute? I'm not convinced that ignoring tokens because they appear in the "wrong" location is really the least surprising. Ignoring tokens outside of <head>, and tokens only being applied after they were encountered at least is consistent with things like csp policies specified by meta tags.

+ if (!isScriptGenerated)

+ OriginTrials::from(&document)->addToken(content);

}