Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2496293003:
PlzNavigate: add origin header (Closed)
DescriptionPlzNavigate: add origin header
This CL ensures the origin header is properly set when using
browser-side navigation.
BUG=648588
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/ce1a32c1989a041c641d2a630362177dffaf14de
Cr-Commit-Position: refs/heads/master@{#437662}
Patch Set 1 #Patch Set 2 : PlzNavigate: add origin header #
Total comments: 8
Patch Set 3 : Rebase #Patch Set 4 : Addressed comments #
Messages
Total messages: 28 (18 generated)
Description was changed from ========== PlzNavigate: add origin header This CL ensures the origin header is properly set when using browser-side navigation. BUG=648588 ========== to ========== PlzNavigate: add origin header This CL ensures the origin header is properly set when using browser-side navigation. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)
The CQ bit was checked by clamy@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== PlzNavigate: add origin header This CL ensures the origin header is properly set when using browser-side navigation. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: add origin header This CL ensures the origin header is properly set when using browser-side navigation. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
clamy@chromium.org changed reviewers: + mkwst@chromium.org
@mkwst: PTAL @arthursonzogni: FYI (this follows your patch for the upgrade insecure request header).
mkwst@chromium.org changed reviewers: + tyoshino@chromium.org
https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:122: return false; This doesn't seem right: we send the `Origin` header for `GET` requests in various cases. XMLHttpRequest that adds strange headers, `<img crossorigin>`, etc. CCing tyoshino@ to see if he can explain the existing code, as it seems incorrect to me.
https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:122: return false; On 2016/11/17 at 14:27:12, Mike West wrote: > This doesn't seem right: we send the `Origin` header for `GET` requests in various cases. XMLHttpRequest that adds strange headers, `<img crossorigin>`, etc. > > CCing tyoshino@ to see if he can explain the existing code, as it seems incorrect to me. Oh. This is only for navigations. It might be the case that we only send the header for `<form>` POST (though I'm not sure that would be correct).
@tyoshino, mkwst: friendly ping :).
Sorry! lgtm https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:160: if (NeedsHTTPOrigin(headers, method)) { early return? https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:165: blink::WebSandboxFlags::Origin) != blink::WebSandboxFlags::None; how about calculating this in the else block below? https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:169: } else if (!origin_is_sandboxed){ space before {
On 2016/12/06 at 16:39:20, tyoshino wrote: > Sorry! lgtm > > https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... > File content/browser/frame_host/navigation_request.cc (right): > > https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... > content/browser/frame_host/navigation_request.cc:160: if (NeedsHTTPOrigin(headers, method)) { > early return? > > https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... > content/browser/frame_host/navigation_request.cc:165: blink::WebSandboxFlags::Origin) != blink::WebSandboxFlags::None; > how about calculating this in the else block below? > > https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... > content/browser/frame_host/navigation_request.cc:169: } else if (!origin_is_sandboxed){ > space before { I'm happy if tyoshino is happy.
Thanks! https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:160: if (NeedsHTTPOrigin(headers, method)) { On 2016/12/06 16:39:20, tyoshino wrote: > early return? Done. https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:165: blink::WebSandboxFlags::Origin) != blink::WebSandboxFlags::None; On 2016/12/06 16:39:20, tyoshino wrote: > how about calculating this in the else block below? Done. https://codereview.chromium.org/2496293003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:169: } else if (!origin_is_sandboxed){ On 2016/12/06 16:39:20, tyoshino wrote: > space before { Done.
The CQ bit was checked by clamy@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from tyoshino@chromium.org Link to the patchset: https://codereview.chromium.org/2496293003/#ps60001 (title: "Addressed comments")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 60001, "attempt_start_ts": 1481306312551750,
"parent_rev": "3ed689824ca28e34fbe77b06230da834c292ce40", "commit_rev":
"590e7fbf6b2ea712db13e28ca3d8f577aacbef77"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: add origin header This CL ensures the origin header is properly set when using browser-side navigation. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== ==========
Message was sent while issue was closed.
Committed patchset #4 (id:60001)
Message was sent while issue was closed.
Description was changed from ========== ========== to ========== PlzNavigate: add origin header This CL ensures the origin header is properly set when using browser-side navigation. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: add origin header This CL ensures the origin header is properly set when using browser-side navigation. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate: add origin header This CL ensures the origin header is properly set when using browser-side navigation. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/ce1a32c1989a041c641d2a630362177dffaf14de Cr-Commit-Position: refs/heads/master@{#437662} ==========
Message was sent while issue was closed.
Patchset 4 (id:??) landed as https://crrev.com/ce1a32c1989a041c641d2a630362177dffaf14de Cr-Commit-Position: refs/heads/master@{#437662} |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
