Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2462513003:
PlzNavigate: Add missing Upgrade-Insecure-Requests header. (Closed)
|
Created:
4 years, 1 month ago by arthursonzogni Modified:
4 years, 1 month ago CC:
Mike West, chromium-reviews, creis+watch_chromium.org, darin-cc_chromium.org, jam, nasko+codewatch_chromium.org Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate:Add missing Upgrade-Insecure-Requests header.
The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests.
A test showing this lack was added with this bugfix.
It **partially** fixes the following tests too:
* http/tests/navigation/post-frames-goback1.html
* http/tests/navigation/post-goback1.html
* virtual/stable/http/tests/navigation/post-frames-goback1.html
* virtual/stable/http/tests/navigation/post-goback1.html
with --enable-browser-side-navigation flag.
BUG=648588
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation
Committed: https://crrev.com/3d060a7060e9085b64eda7c3e617e2f708fb4411
Cr-Commit-Position: refs/heads/master@{#431877}
Patch Set 1 : PlzNavigate:Add missing Upgrade-Insecure-Requests header. #
Total comments: 2
Messages
Total messages: 23 (14 generated)
Description was changed from ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. It **partially** fixes the following tests: * http/tests/navigation/post-frames-goback1.html \ * http/tests/navigation/post-goback1.html \ * virtual/stable/http/tests/navigation/post-frames-goback1.html \ * virtual/stable/http/tests/navigation/post-goback1.html \ with --enable-browser-side-navigation flag. BUG=648588 ========== to ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. It **partially** fixes the following tests: * http/tests/navigation/post-frames-goback1.html \ * http/tests/navigation/post-goback1.html \ * virtual/stable/http/tests/navigation/post-frames-goback1.html \ * virtual/stable/http/tests/navigation/post-goback1.html \ with --enable-browser-side-navigation flag. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. It **partially** fixes the following tests: * http/tests/navigation/post-frames-goback1.html \ * http/tests/navigation/post-goback1.html \ * virtual/stable/http/tests/navigation/post-frames-goback1.html \ * virtual/stable/http/tests/navigation/post-goback1.html \ with --enable-browser-side-navigation flag. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. A test showing this lack was added with this bugfix. It **partially** fixes the following tests too: * http/tests/navigation/post-frames-goback1.html \ * http/tests/navigation/post-goback1.html \ * virtual/stable/http/tests/navigation/post-frames-goback1.html \ * virtual/stable/http/tests/navigation/post-goback1.html \ with --enable-browser-side-navigation flag. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Patchset #1 (id:1) has been deleted
The CQ bit was checked by arthursonzogni@chromium.org to run a CQ dry run
Description was changed from ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. A test showing this lack was added with this bugfix. It **partially** fixes the following tests too: * http/tests/navigation/post-frames-goback1.html \ * http/tests/navigation/post-goback1.html \ * virtual/stable/http/tests/navigation/post-frames-goback1.html \ * virtual/stable/http/tests/navigation/post-goback1.html \ with --enable-browser-side-navigation flag. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. A test showing this lack was added with this bugfix. It **partially** fixes the following tests too: * http/tests/navigation/post-frames-goback1.html \ * http/tests/navigation/post-goback1.html \ * virtual/stable/http/tests/navigation/post-frames-goback1.html \ * virtual/stable/http/tests/navigation/post-goback1.html \ with --enable-browser-side-navigation flag. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
arthursonzogni@chromium.org changed reviewers: + clamy@chromium.org
Hi Camille, This CL adds the Upgrade-Insecure-Requests header with PlzNavigate. The previous CL that fixes the remaining failing layout tests: https://codereview.chromium.org/2284683002/ was not enough, because the header was added in the renderer, just before being intercepted by PlzNavigate. The problem is that this part of code is never called if the navigation the browser-initiated (the navigation stays on the browser side). So this header must be added on the browser-side. Could you take a look? [CC + @mkwst ] I added you for your information. I also have noticed that the current renderer-side implementation doesn't exactly match the specification: https://www.w3.org/TR/upgrade-insecure-requests/#preference For navigational requests, the header is always added. I did the same on the browser-side. I assumed it is okay. Thanks!
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Thanks! One question below. https://codereview.chromium.org/2462513003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2462513003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:130: // Tack an 'Upgrade-Insecure-Requests' header to outgoing navigational This is called also for subframe navigations, is this the expected behavior?
https://codereview.chromium.org/2462513003/diff/20001/content/browser/frame_h... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2462513003/diff/20001/content/browser/frame_h... content/browser/frame_host/navigation_request.cc:130: // Tack an 'Upgrade-Insecure-Requests' header to outgoing navigational On 2016/11/02 14:01:30, clamy (slow) wrote: > This is called also for subframe navigations, is this the expected behavior? Yes it is. The browser-side implementation is putting the header for every navigational requests: https://cs.chromium.org/chromium/src/third_party/WebKit/Source/core/loader/Fr... In contrast, the specification is a little bit different: https://w3c.github.io/webappsec-upgrade-insecure-requests/#preference It defines when the header must be there and when it should be defined, but not when it must not be defined. For subframe navigation, the header might already have been defined by the renderer before being intercepted by PlzNavigate. In that case, this is a no-op. In the future, I plan to upgrade navigational request on the browser and not rely on the renderer (e.g: reverting my first patch: https://codereview.chromium.org/2284683002/). In that case, this will no more be a no-op.
mkwst@chromium.org changed reviewers: + mkwst@chromium.org
lgtm LGTM, FWIW.
Description was changed from ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. A test showing this lack was added with this bugfix. It **partially** fixes the following tests too: * http/tests/navigation/post-frames-goback1.html \ * http/tests/navigation/post-goback1.html \ * virtual/stable/http/tests/navigation/post-frames-goback1.html \ * virtual/stable/http/tests/navigation/post-goback1.html \ with --enable-browser-side-navigation flag. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. A test showing this lack was added with this bugfix. It **partially** fixes the following tests too: * http/tests/navigation/post-frames-goback1.html * http/tests/navigation/post-goback1.html * virtual/stable/http/tests/navigation/post-frames-goback1.html * virtual/stable/http/tests/navigation/post-goback1.html with --enable-browser-side-navigation flag. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Thanks! Lgtm.
Thanks for the reviews!
The CQ bit was checked by arthursonzogni@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. A test showing this lack was added with this bugfix. It **partially** fixes the following tests too: * http/tests/navigation/post-frames-goback1.html * http/tests/navigation/post-goback1.html * virtual/stable/http/tests/navigation/post-frames-goback1.html * virtual/stable/http/tests/navigation/post-goback1.html with --enable-browser-side-navigation flag. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. A test showing this lack was added with this bugfix. It **partially** fixes the following tests too: * http/tests/navigation/post-frames-goback1.html * http/tests/navigation/post-goback1.html * virtual/stable/http/tests/navigation/post-frames-goback1.html * virtual/stable/http/tests/navigation/post-goback1.html with --enable-browser-side-navigation flag. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Message was sent while issue was closed.
Committed patchset #1 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. A test showing this lack was added with this bugfix. It **partially** fixes the following tests too: * http/tests/navigation/post-frames-goback1.html * http/tests/navigation/post-goback1.html * virtual/stable/http/tests/navigation/post-frames-goback1.html * virtual/stable/http/tests/navigation/post-goback1.html with --enable-browser-side-navigation flag. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ========== to ========== PlzNavigate:Add missing Upgrade-Insecure-Requests header. The Upgrade-Insecure-Requests header was missing for browser-initiated navigation requests. A test showing this lack was added with this bugfix. It **partially** fixes the following tests too: * http/tests/navigation/post-frames-goback1.html * http/tests/navigation/post-goback1.html * virtual/stable/http/tests/navigation/post-frames-goback1.html * virtual/stable/http/tests/navigation/post-goback1.html with --enable-browser-side-navigation flag. BUG=648588 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation Committed: https://crrev.com/3d060a7060e9085b64eda7c3e617e2f708fb4411 Cr-Commit-Position: refs/heads/master@{#431877} ==========
Message was sent while issue was closed.
Patchset 1 (id:??) landed as https://crrev.com/3d060a7060e9085b64eda7c3e617e2f708fb4411 Cr-Commit-Position: refs/heads/master@{#431877} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
