Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2879853002:
PlzNavigate: Fix form submission to OOPIF frame. (Closed)
|
Created:
3 years, 7 months ago by arthursonzogni Modified:
3 years, 7 months ago Reviewers:
nasko CC:
chromium-reviews, jam, nasko+codewatch_chromium.org, darin-cc_chromium.org, creis+watch_chromium.org, Charlie Reis, clamy, Łukasz Anforowicz, Tom Sepez Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionPlzNavigate: Fix form submission to OOPIF frame.
When a renderer-initiated form-submission targets an OOPIF iframe, the
navigation uses the OpenURL path. Renderer-initiated form-submission was
not implemented here with PlzNavigate.
This CL makes it work. As a consequence, it fixes the XSS Auditor
issue (https://crbug.com/710937).
BUG=710937
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel
Review-Url: https://codereview.chromium.org/2879853002
Cr-Commit-Position: refs/heads/master@{#472306}
Committed: https://chromium.googlesource.com/chromium/src/+/bc5732b54102e0c89f3098e03ddb865636d5a8bd
Patch Set 1 #Patch Set 2 : Fix errors && add a test. #Patch Set 3 : Addressed comments Nasko@ #
Total comments: 2
Patch Set 4 : Add TODO. #
Total comments: 7
Patch Set 5 : Nits. #Patch Set 6 : Nits. #
Messages
Total messages: 52 (42 generated)
Description was changed from ========== PlzNavigate: fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequences, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 ========== to ========== PlzNavigate: fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequences, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation ==========
Description was changed from ========== PlzNavigate: fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequences, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 ========== to ========== PlzNavigate: fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequences, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ==========
Description was changed from ========== PlzNavigate: fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequences, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequences, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ==========
Description was changed from ========== PlzNavigate: fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequences, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequence, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ==========
The CQ bit was checked by arthursonzogni@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_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
Description was changed from ========== PlzNavigate: fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequence, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequence, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ==========
The CQ bit was checked by arthursonzogni@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...
Description was changed from ========== PlzNavigate: fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequence, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequence, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ==========
Patchset #3 (id:40001) has been deleted
The CQ bit was checked by arthursonzogni@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...
Patchset #3 (id:60001) has been deleted
The CQ bit was checked by arthursonzogni@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...
Description was changed from ========== PlzNavigate: Fix form submission to remote frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequence, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Fix form submission to OOPIF frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequence, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Patchset #2 (id:20001) has been deleted
Patchset #3 (id:100001) has been deleted
The CQ bit was checked by arthursonzogni@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...
arthursonzogni@chromium.org changed reviewers: + nasko@chromium.org
Hi Nasko, Please take a look.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED, https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)
Mostly deferring to Nasko, but one drive-by question below. https://codereview.chromium.org/2879853002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2879853002/diff/120001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:894: // classified as browser-initiated when this one has already navigated. This is a good question. Do you know any consequences of this? I think we mainly use this cloned entry to get the parameters to pass to the renderer, but it's possible that having the wrong value might affect things. If it looks like there may be problems here, please file a bug and CC me. Thanks for spotting!
The CQ bit was checked by arthursonzogni@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...
https://codereview.chromium.org/2879853002/diff/120001/content/browser/frame_... File content/browser/frame_host/navigator_impl.cc (right): https://codereview.chromium.org/2879853002/diff/120001/content/browser/frame_... content/browser/frame_host/navigator_impl.cc:894: // classified as browser-initiated when this one has already navigated. On 2017/05/12 23:50:20, Charlie Reis wrote: > This is a good question. Do you know any consequences of this? I think we > mainly use this cloned entry to get the parameters to pass to the renderer, but > it's possible that having the wrong value might affect things. > > If it looks like there may be problems here, please file a bug and CC me. > Thanks for spotting! I did a quick look at the possible consequences. I haven't found something very bad. |is_renderer_initiated| is used in several conditions and in NavigationHandle::IsRendererInitiated(). It would be nice being able to rely on this for the NavigationThrottle. It is probably something we have to fix. I filled the bug(722251). It is probably very easy to fix.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM with a few nits. https://codereview.chromium.org/2879853002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2879853002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:206: // renderer-initiated form submission that took the OpenURL path or either a nit: s/or either/or/ https://codereview.chromium.org/2879853002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:210: // Standard form-submission from the renderer. nit: no need for "-" between form and submission https://codereview.chromium.org/2879853002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:218: bool is_form_submission = bool(request_body); nit: !!request_body?
The CQ bit was checked by arthursonzogni@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: ios-simulator on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)
Thanks Nasko! https://codereview.chromium.org/2879853002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2879853002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:206: // renderer-initiated form submission that took the OpenURL path or either a On 2017/05/16 04:08:21, nasko (out) wrote: > nit: s/or either/or/ Done. https://codereview.chromium.org/2879853002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:210: // Standard form-submission from the renderer. On 2017/05/16 04:08:21, nasko (out) wrote: > nit: no need for "-" between form and submission Done. https://codereview.chromium.org/2879853002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:218: bool is_form_submission = bool(request_body); On 2017/05/16 04:08:21, nasko (out) wrote: > nit: !!request_body? I would prefer not to. If you don't like: "bool(...)", maybe you would prefer: "(request_body != nullptr)" If it is okay, feel free to run the CQ.
https://codereview.chromium.org/2879853002/diff/140001/content/browser/frame_... File content/browser/frame_host/navigation_request.cc (right): https://codereview.chromium.org/2879853002/diff/140001/content/browser/frame_... content/browser/frame_host/navigation_request.cc:218: bool is_form_submission = bool(request_body); On 2017/05/16 12:45:58, arthursonzogni wrote: > On 2017/05/16 04:08:21, nasko (out) wrote: > > nit: !!request_body? > > I would prefer not to. If you don't like: "bool(...)", maybe you would prefer: > "(request_body != nullptr)" > > If it is okay, feel free to run the CQ. The main point is to keep code consistent. I haven't seen "bool()" usage around and it surprised me. I'm fine with "(request_body != nullptr)", though we try to avoid that too in favor of the "!!ptr" pattern.
The CQ bit was checked by arthursonzogni@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nasko@chromium.org Link to the patchset: https://codereview.chromium.org/2879853002/#ps180001 (title: "Nits.")
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
Try jobs failed on following builders: win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)
The CQ bit was checked by nasko@chromium.org
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": 180001, "attempt_start_ts": 1494978057786220,
"parent_rev": "2ec70963f4e92f375b9387f2bac2ae1be750a013", "commit_rev":
"bc5732b54102e0c89f3098e03ddb865636d5a8bd"}
Message was sent while issue was closed.
Description was changed from ========== PlzNavigate: Fix form submission to OOPIF frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequence, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel ========== to ========== PlzNavigate: Fix form submission to OOPIF frame. When a renderer-initiated form-submission targets an OOPIF iframe, the navigation uses the OpenURL path. Renderer-initiated form-submission was not implemented here with PlzNavigate. This CL makes it work. As a consequence, it fixes the XSS Auditor issue (https://crbug.com/710937). BUG=710937 CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.linux:linux_site_isolation;master.tryserver.chromium.linux:linux_site_isolation,linux_chromium_browser_side_navigation_rel Review-Url: https://codereview.chromium.org/2879853002 Cr-Commit-Position: refs/heads/master@{#472306} Committed: https://chromium.googlesource.com/chromium/src/+/bc5732b54102e0c89f3098e03ddb... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:180001) as https://chromium.googlesource.com/chromium/src/+/bc5732b54102e0c89f3098e03ddb... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
