Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(19)

Issue 1617043002: Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. (Closed)

Created:
4 years, 11 months ago by Mike West
Modified:
4 years, 7 months ago
CC:
blink-reviews, carlosk, chromium-reviews, clamy, creis+watch_chromium.org, darin-cc_chromium.org, davidben, gavinp+loader_chromium.org, jam, Nate Chapin, loading-reviews_chromium.org, nasko+codewatch_chromium.org, site-isolation-reviews_chromium.org, tyoshino+watch_chromium.org
Base URL:
https://chromium.googlesource.com/chromium/src.git@block-response
Target Ref:
refs/pending/heads/master
Project:
chromium
Visibility:
Public.

Description

Introduce AncestorThrottle, which will process 'X-Frame-Options' headers. This moves the ancestor-based blocking behavior from Blink up into the browser, and depends on https://codereview.chromium.org/1616943003 for some infrastructure changes. BUG=555418 CQ_INCLUDE_TRYBOTS=tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/26a6fc92ae361b4271f8f2197abe7eb063fc43ed Cr-Commit-Position: refs/heads/master@{#392032}

Patch Set 1 #

Total comments: 14

Patch Set 2 : Rebase. #

Patch Set 3 : Fix. #

Total comments: 14

Patch Set 4 : Hrm. #

Total comments: 2

Patch Set 5 : Rebasing on top of https://codereview.chromium.org/1920873002 #

Patch Set 6 : Drop XFO from Blink. #

Total comments: 5

Patch Set 7 : Rebase. #

Patch Set 8 : Test+ErrorPage #

Total comments: 10

Patch Set 9 : mmenke@ + Windows #

Patch Set 10 : rebase #

Total comments: 1

Patch Set 11 : WINDOWS. #

Total comments: 1

Patch Set 12 : CONNECTION_REFUSED #

Total comments: 3

Patch Set 13 : DCHECK. #

Total comments: 1
Unified diffs Side-by-side diffs Delta from patch set Stats (+516 lines, -138 lines) Patch
M components/error_page/common/localized_error.cc View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +9 lines, -0 lines 0 comments Download
A content/browser/frame_host/ancestor_throttle.h View 1 2 3 4 5 6 7 8 9 10 1 chunk +67 lines, -0 lines 0 comments Download
A content/browser/frame_host/ancestor_throttle.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +186 lines, -0 lines 0 comments Download
A content/browser/frame_host/ancestor_throttle_unittest.cc View 1 2 3 4 5 6 7 8 9 10 1 chunk +183 lines, -0 lines 0 comments Download
M content/browser/frame_host/navigation_handle_impl.cc View 1 2 3 4 5 6 7 8 6 chunks +13 lines, -8 lines 0 comments Download
M content/browser/loader/navigation_resource_throttle.cc View 1 2 3 4 5 6 7 8 9 10 11 12 1 chunk +10 lines, -0 lines 0 comments Download
M content/content_browser.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +2 lines, -0 lines 0 comments Download
M content/content_tests.gypi View 1 2 3 4 5 6 7 8 9 10 11 1 chunk +1 line, -0 lines 0 comments Download
M content/public/browser/navigation_throttle.h View 1 2 chunks +7 lines, -1 line 1 comment Download
M net/base/net_error_list.h View 1 2 3 4 5 6 7 8 1 chunk +5 lines, -0 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/inspector/network/x-frame-options-deny.html View 1 2 3 4 5 6 7 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/inspector/network/x-frame-options-deny-expected.txt View 1 2 3 4 5 6 7 1 chunk +1 line, -2 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-deny-expected.txt View 1 2 3 4 5 6 7 8 9 10 1 chunk +3 lines, -1 line 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-multiple-headers-conflict-expected.txt View 1 2 3 4 5 6 7 8 9 1 chunk +4 lines, -3 lines 0 comments Download
M third_party/WebKit/LayoutTests/http/tests/security/XFrameOptions/x-frame-options-multiple-headers-sameorigin-deny-expected.txt View 1 2 3 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/dom/DocumentInit.cpp View 1 2 3 4 5 6 1 chunk +3 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorInstrumentation.cpp View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/inspector/InspectorInstrumentationCustomInl.h View 1 2 3 4 5 6 7 1 chunk +1 line, -1 line 0 comments Download
M third_party/WebKit/Source/core/loader/DocumentLoader.h View 1 2 3 4 5 3 chunks +4 lines, -4 lines 0 comments Download
M third_party/WebKit/Source/core/loader/DocumentLoader.cpp View 1 2 3 4 5 6 7 4 chunks +10 lines, -22 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.h View 1 2 3 4 5 1 chunk +0 lines, -2 lines 0 comments Download
M third_party/WebKit/Source/core/loader/FrameLoader.cpp View 1 2 3 4 5 6 1 chunk +0 lines, -47 lines 0 comments Download
M third_party/WebKit/Source/core/loader/HttpEquiv.h View 1 2 3 4 5 1 chunk +0 lines, -1 line 0 comments Download
M third_party/WebKit/Source/platform/network/HTTPParsers.h View 1 2 3 4 5 2 chunks +0 lines, -9 lines 0 comments Download
M third_party/WebKit/Source/platform/network/HTTPParsers.cpp View 1 2 3 4 5 1 chunk +0 lines, -30 lines 0 comments Download
M tools/metrics/histograms/histograms.xml View 1 2 3 4 5 6 7 8 9 10 11 2 chunks +2 lines, -0 lines 0 comments Download

Messages

Total messages: 59 (19 generated)
Mike West
CCing folks to whom I'd asked questions on the previous CL. :)
4 years, 11 months ago (2016-01-21 14:52:01 UTC) #2
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617043002/1 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617043002/1
4 years, 11 months ago (2016-01-22 12:55:21 UTC) #5
commit-bot: I haz the power
Dry run: Try jobs failed on following builders: win_chromium_x64_rel_ng on tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_rel_ng/builds/157816)
4 years, 11 months ago (2016-01-22 13:23:28 UTC) #7
nasko
Few comments, mostly nits. https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/ancestor_throttle.cc File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/ancestor_throttle.cc#newcode76 content/browser/frame_host/ancestor_throttle.cc:76: return NavigationThrottle::PROCEED; Shouldn't we be ...
4 years, 10 months ago (2016-02-12 23:21:41 UTC) #8
nasko
On 2016/02/12 23:21:41, nasko wrote: > Few comments, mostly nits. > > https://codereview.chromium.org/1617043002/diff/1/content/browser/frame_host/ancestor_throttle.cc > File ...
4 years, 9 months ago (2016-02-26 18:19:14 UTC) #9
clamy
@mkwst: ping. Are you still planning on continuing with that CL? Note that I'm introducing ...
4 years, 9 months ago (2016-03-17 16:48:25 UTC) #10
Mike West
On 2016/03/17 at 16:48:25, clamy wrote: > @mkwst: ping. Are you still planning on continuing ...
4 years, 9 months ago (2016-03-18 07:32:43 UTC) #11
Mike West
Picking this back up again. We need to teach the throttle to deal in some ...
4 years, 8 months ago (2016-04-12 20:13:30 UTC) #15
nasko
Some more nits/questions. https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_host/ancestor_throttle.h File content/browser/frame_host/ancestor_throttle.h (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_host/ancestor_throttle.h#newcode5 content/browser/frame_host/ancestor_throttle.h:5: #ifndef CHROME_BROWSER_SECURITY_ANCESTOR_THROTTLE_H_ Mismatched include guard and ...
4 years, 8 months ago (2016-04-12 22:21:19 UTC) #16
Mike West
More issues with this patch: 1. We're not triggering `onload` _or_ `onerror` events on the ...
4 years, 8 months ago (2016-04-13 13:28:08 UTC) #17
carlosk
On 2016/04/13 13:28:08, Mike West wrote: > 2. Devtools doesn't display any detail about the ...
4 years, 8 months ago (2016-04-13 14:58:16 UTC) #19
Mike West
On 2016/04/13 at 14:58:16, carlosk wrote: > On 2016/04/13 13:28:08, Mike West wrote: > > ...
4 years, 8 months ago (2016-04-13 17:30:12 UTC) #20
carlosk
On 2016/04/13 17:30:12, Mike West wrote: > On 2016/04/13 at 14:58:16, carlosk wrote: > > ...
4 years, 8 months ago (2016-04-18 12:15:34 UTC) #21
Mike West
Worked out one set of issues, and added rudimentary `frame-ancestors` handling. Let's see how it ...
4 years, 8 months ago (2016-04-25 13:06:21 UTC) #22
Mike West
On 2016/04/25 at 13:06:21, Mike West (slow until 25th) wrote: > https://codereview.chromium.org/1617043002/diff/80001/content/renderer/render_frame_impl.cc#newcode2174 > content/renderer/render_frame_impl.cc:2174: !RenderThreadImpl::current()->layout_test_mode()) ...
4 years, 8 months ago (2016-04-25 17:16:04 UTC) #23
Mike West
On 2016/04/25 at 17:16:04, Mike West (slow until 25th) wrote: > Looks like there are ...
4 years, 7 months ago (2016-04-29 12:35:25 UTC) #24
nasko
I think the content/ side is in a good shape. I need to nitpick a ...
4 years, 7 months ago (2016-04-29 18:56:02 UTC) #25
Mike West
Thanks, Nasko! https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_host/ancestor_throttle.h File content/browser/frame_host/ancestor_throttle.h (right): https://codereview.chromium.org/1617043002/diff/60001/content/browser/frame_host/ancestor_throttle.h#newcode5 content/browser/frame_host/ancestor_throttle.h:5: #ifndef CHROME_BROWSER_SECURITY_ANCESTOR_THROTTLE_H_ On 2016/04/29 at 18:56:02, nasko ...
4 years, 7 months ago (2016-05-02 09:37:52 UTC) #26
nasko
content/ LGTM, assuming the include guard actually says CONTENT_ ;). https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_host/navigation_handle_impl.cc File content/browser/frame_host/navigation_handle_impl.cc (right): https://codereview.chromium.org/1617043002/diff/120001/content/browser/frame_host/navigation_handle_impl.cc#newcode288 ...
4 years, 7 months ago (2016-05-02 17:22:09 UTC) #29
Mike West
Thanks, Nasko! I'll fix the `CONTENT` bit. jochen@: Would you take a look at the ...
4 years, 7 months ago (2016-05-02 18:02:23 UTC) #31
mmenke
[+edwardjung]: Mind reviewing the error strings this adds? https://codereview.chromium.org/1617043002/diff/200001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1617043002/diff/200001/net/base/net_error_list.h#newcode112 net/base/net_error_list.h:112: // ...
4 years, 7 months ago (2016-05-02 18:08:15 UTC) #33
mmenke
https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_host/ancestor_throttle.cc File content/browser/frame_host/ancestor_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_host/ancestor_throttle.cc#newcode129 content/browser/frame_host/ancestor_throttle.cc:129: while (headers->EnumerateHeader(&iter, "x-frame-options", &value)) { Is there a w3c ...
4 years, 7 months ago (2016-05-02 18:17:29 UTC) #34
Ilya Sherman
histograms.xml rs lgtm
4 years, 7 months ago (2016-05-02 18:36:38 UTC) #35
jochen (gone - plz use gerrit)
blink lgtm assuming devtools is happy with temporarily breaking this
4 years, 7 months ago (2016-05-03 10:59:22 UTC) #36
edwardjung
https://codereview.chromium.org/1617043002/diff/240001/components/error_page_strings.grdp File components/error_page_strings.grdp (right): https://codereview.chromium.org/1617043002/diff/240001/components/error_page_strings.grdp#newcode459 components/error_page_strings.grdp:459: The page at this URL has requested not to ...
4 years, 7 months ago (2016-05-03 14:39:47 UTC) #37
Mike West
On 2016/05/03 at 14:39:47, edwardjung wrote: > https://codereview.chromium.org/1617043002/diff/240001/components/error_page_strings.grdp > File components/error_page_strings.grdp (right): > > https://codereview.chromium.org/1617043002/diff/240001/components/error_page_strings.grdp#newcode459 ...
4 years, 7 months ago (2016-05-03 15:59:15 UTC) #38
edwardjung
On 2016/05/03 15:59:15, Mike West wrote: > On 2016/05/03 at 14:39:47, edwardjung wrote: > > ...
4 years, 7 months ago (2016-05-03 16:30:04 UTC) #39
Mike West
On 2016/05/03 at 16:30:04, edwardjung wrote: > On 2016/05/03 15:59:15, Mike West wrote: > > ...
4 years, 7 months ago (2016-05-03 18:13:34 UTC) #40
edwardjung
> Hrm. I think only the detail message is displayed on hover. I might be ...
4 years, 7 months ago (2016-05-03 18:55:56 UTC) #41
Mike West
On 2016/05/03 at 18:55:56, edwardjung wrote: > > Hrm. I think only the detail message ...
4 years, 7 months ago (2016-05-04 15:57:47 UTC) #42
edwardjung
> https://codereview.chromium.org/1617043002/diff/260001/components/error_page_strings.grdp#newcode475 > > components/error_page_strings.grdp:475: </message> > > This seems like a too long a ...
4 years, 7 months ago (2016-05-04 21:23:41 UTC) #43
Mike West
Thanks! I think //net/base is the only thing left; mmenke@, WDYT? https://codereview.chromium.org/1617043002/diff/200001/content/browser/frame_host/ancestor_throttle.cc File content/browser/frame_host/ancestor_throttle.cc (right): ...
4 years, 7 months ago (2016-05-05 08:14:28 UTC) #44
commit-bot: I haz the power
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617043002/280001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617043002/280001
4 years, 7 months ago (2016-05-05 09:47:50 UTC) #46
commit-bot: I haz the power
Dry run: This issue passed the CQ dry run.
4 years, 7 months ago (2016-05-05 11:23:36 UTC) #48
mmenke
LGTM https://codereview.chromium.org/1617043002/diff/280001/content/browser/loader/navigation_resource_throttle.cc File content/browser/loader/navigation_resource_throttle.cc (right): https://codereview.chromium.org/1617043002/diff/280001/content/browser/loader/navigation_resource_throttle.cc#newcode245 content/browser/loader/navigation_resource_throttle.cc:245: // consider the right thing to do here. ...
4 years, 7 months ago (2016-05-05 12:36:27 UTC) #49
commit-bot: I haz the power
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1617043002/300001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1617043002/300001
4 years, 7 months ago (2016-05-06 09:36:19 UTC) #52
Mike West
https://codereview.chromium.org/1617043002/diff/280001/net/base/net_error_list.h File net/base/net_error_list.h (right): https://codereview.chromium.org/1617043002/diff/280001/net/base/net_error_list.h#newcode112 net/base/net_error_list.h:112: // checks, for instance). On 2016/05/05 at 12:36:27, mmenke ...
4 years, 7 months ago (2016-05-06 11:23:24 UTC) #53
commit-bot: I haz the power
Committed patchset #13 (id:300001)
4 years, 7 months ago (2016-05-06 11:24:45 UTC) #55
commit-bot: I haz the power
Patchset 13 (id:??) landed as https://crrev.com/26a6fc92ae361b4271f8f2197abe7eb063fc43ed Cr-Commit-Position: refs/heads/master@{#392032}
4 years, 7 months ago (2016-05-06 11:26:18 UTC) #57
carlosk
4 years, 7 months ago (2016-05-10 14:05:36 UTC) #59
Message was sent while issue was closed.
Sorry about the post-mortem comment but I only re-looked at this change now.

https://codereview.chromium.org/1617043002/diff/300001/content/public/browser...
File content/public/browser/navigation_throttle.h (right):

https://codereview.chromium.org/1617043002/diff/300001/content/public/browser...
content/public/browser/navigation_throttle.h:38: // be returned from
WillProcessResponse.
Should also mention that BLOCK_RESPONSE should only be used for subframes (for
now at least, as stated in navigation_resource_throttle.cc).

Powered by Google App Engine
This is Rietveld 408576698