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

Side by Side 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, 8 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 unified diff | 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')
Toggle Intra-line Diffs ('i') | Expand Comments ('e') | Collapse Comments ('c') | Hide Comments ('s')

OLD	NEW
1 // Copyright 2015 The Chromium Authors. All rights reserved.	1 // Copyright 2015 The Chromium Authors. All rights reserved.

2 // Use of this source code is governed by a BSD-style license that can be	2 // Use of this source code is governed by a BSD-style license that can be

3 // found in the LICENSE file.	3 // found in the LICENSE file.

4	4

5 #include "core/loader/HttpEquiv.h"	5 #include "core/loader/HttpEquiv.h"

6	6

7 #include "core/dom/Document.h"	7 #include "core/dom/Document.h"

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

8 #include "core/dom/StyleEngine.h"	9 #include "core/dom/StyleEngine.h"

9 #include "core/fetch/ClientHintsPreferences.h"	10 #include "core/fetch/ClientHintsPreferences.h"

10 #include "core/frame/UseCounter.h"	11 #include "core/frame/UseCounter.h"

11 #include "core/frame/csp/ContentSecurityPolicy.h"	12 #include "core/frame/csp/ContentSecurityPolicy.h"

12 #include "core/html/HTMLDocument.h"	13 #include "core/html/HTMLDocument.h"

13 #include "core/inspector/ConsoleMessage.h"	14 #include "core/inspector/ConsoleMessage.h"

14 #include "core/loader/DocumentLoader.h"	15 #include "core/loader/DocumentLoader.h"

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

15 #include "platform/network/HTTPParsers.h"	17 #include "platform/network/HTTPParsers.h"

16 #include "platform/weborigin/KURL.h"	18 #include "platform/weborigin/KURL.h"

17	19

18 namespace blink {	20 namespace blink {

19	21

20 void HttpEquiv::process(Document& document, const AtomicString& equiv, const Ato micString& content, bool inDocumentHeadElement)	22 void HttpEquiv::process(Document& document, const AtomicString& equiv, const Ato micString& content, bool inDocumentHeadElement)

21 {	23 {

22 ASSERT(!equiv.isNull() && !content.isNull());	24 ASSERT(!equiv.isNull() && !content.isNull());

23	25

24 if (equalIgnoringCase(equiv, "default-style")) {	26 if (equalIgnoringCase(equiv, "default-style")) {

(...skipping 10 matching lines...) Expand all Loading...
35 processHttpEquivXFrameOptions(document, content);	37 processHttpEquivXFrameOptions(document, content);

36 } else if (equalIgnoringCase(equiv, "accept-ch")) {	38 } else if (equalIgnoringCase(equiv, "accept-ch")) {

37 processHttpEquivAcceptCH(document, content);	39 processHttpEquivAcceptCH(document, content);

38 } else if (equalIgnoringCase(equiv, "content-security-policy") \|\| equalIgnor ingCase(equiv, "content-security-policy-report-only")) {	40 } else if (equalIgnoringCase(equiv, "content-security-policy") \|\| equalIgnor ingCase(equiv, "content-security-policy-report-only")) {

39 if (inDocumentHeadElement)	41 if (inDocumentHeadElement)

40 processHttpEquivContentSecurityPolicy(document, equiv, content);	42 processHttpEquivContentSecurityPolicy(document, equiv, content);

41 else	43 else

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

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

44 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."));	46 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."));

	47 } else if (equalIgnoringCase(equiv, HTTPNames::Origin_Trial)) {
	iclelland 2016/03/31 15:00:00 I like using HTTPNames here -- do you have any ide I like using HTTPNames here -- do you have any idea why the other cases don't do that? Marijn Kruisselbrink 2016/03/31 19:01:48 No idea, so maybe core/ owners will object to usin Show quoted text On 2016/03/31 at 15:00:00, iclelland wrote: > I like using HTTPNames here -- do you have any idea why the other cases don't do that? No idea, so maybe core/ owners will object to using HTTPNames here... iclelland 2016/04/01 15:19:09 Okay; we should be explicit and #include "platform Show quoted text On 2016/03/31 19:01:48, Marijn Kruisselbrink wrote: > On 2016/03/31 at 15:00:00, iclelland wrote: > > I like using HTTPNames here -- do you have any idea why the other cases don't > do that? > > No idea, so maybe core/ owners will object to using HTTPNames here... Okay; we should be explicit and #include "platform/HTTPNames.h" then (and see if it sticks -- I don't see a problem, core/ is allowed to include platform/, but I'll defer to owners.)
	48 bool isScriptGenerated = document.hasFinishedParsing() \|\| (document.scri ptableDocumentParser() && document.scriptableDocumentParser()->isExecutingScript ());
	Marijn Kruisselbrink 2016/03/25 19:08:39 I'm not sure if this is entirely the right check t 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. iclelland 2016/03/31 15:00:00 You can see what I'd done in https://codereview.ch Show quoted text 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). Marijn Kruisselbrink 2016/03/31 19:01:48 Yeah, I wasn't sure what situations we actually wa Show quoted text 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). iclelland 2016/04/01 15:19:09 Let's keep this here, then -- I haven't been able Show quoted text 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. Marijn Kruisselbrink 2016/04/01 23:54:47 I started trying to specify what behavior we're ai Show quoted text 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? iclelland 2016/04/05 17:26:28 Out of a bunch of discussions, so a spec is probab Show quoted text 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 Show quoted text On 2016/04/05 at 17:26:28, iclelland wrote: > 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. 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.
	49 if (!isScriptGenerated)

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

45 }	51 }

46 }	52 }

47	53

48 void HttpEquiv::processHttpEquivContentSecurityPolicy(Document& document, const AtomicString& equiv, const AtomicString& content)	54 void HttpEquiv::processHttpEquivContentSecurityPolicy(Document& document, const AtomicString& equiv, const AtomicString& content)

49 {	55 {

50 if (document.importLoader())	56 if (document.importLoader())

51 return;	57 return;

52 if (equalIgnoringCase(equiv, "content-security-policy"))	58 if (equalIgnoringCase(equiv, "content-security-policy"))

53 document.contentSecurityPolicy()->didReceiveHeader(content, ContentSecur ityPolicyHeaderTypeEnforce, ContentSecurityPolicyHeaderSourceMeta);	59 document.contentSecurityPolicy()->didReceiveHeader(content, ContentSecur ityPolicyHeaderTypeEnforce, ContentSecurityPolicyHeaderSourceMeta);

54 else if (equalIgnoringCase(equiv, "content-security-policy-report-only"))	60 else if (equalIgnoringCase(equiv, "content-security-policy-report-only"))

(...skipping 49 matching lines...) Expand 10 before \| Expand all \| Expand 10 after Loading...
104 frame->loader().stopAllLoaders();	110 frame->loader().stopAllLoaders();

105 // Stopping the loader isn't enough, as we're already parsing the document; to honor the header's	111 // Stopping the loader isn't enough, as we're already parsing the document; to honor the header's

106 // intent, we must navigate away from the possibly partially-rendered docume nt to a location that	112 // intent, we must navigate away from the possibly partially-rendered docume nt to a location that

107 // doesn't inherit the parent's SecurityOrigin.	113 // doesn't inherit the parent's SecurityOrigin.

108 // TODO(dglazkov): This should probably check document lifecycle instead.	114 // TODO(dglazkov): This should probably check document lifecycle instead.

109 if (document.frame())	115 if (document.frame())

110 frame->navigate(document, SecurityOrigin::urlWithUniqueSecurityOrigin(), true, UserGestureStatus::None);	116 frame->navigate(document, SecurityOrigin::urlWithUniqueSecurityOrigin(), true, UserGestureStatus::None);

111 }	117 }

112	118

113 } // namespace blink	119 } // namespace blink

OLD	NEW