Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2975623002:
Fix: Loading cid: resources in WebView. (Closed)
|
Created:
3 years, 5 months ago by arthursonzogni Modified:
3 years, 5 months ago CC:
chromium-reviews, tyoshino+watch_chromium.org, loading-reviews_chromium.org, gavinp+loader_chromium.org, blink-reviews, loading-reviews+fetch_chromium.org, kinuko+watch, nasko Target Ref:
refs/heads/master Project:
chromium Visibility:
Public. |
DescriptionFix: Loading cid: resources in WebView.
A bug has been introduced in: https://crrev.com/2834013002.
The assumption that Content-ID URLs can't be loaded without an
MHTMLArchive doesn't hold. It is possible for instance with the Yahoo
Mail app. It embeds chrome as a Webview. The webview load attached image
with the Content-ID scheme.
This CL allows the load instead of blocking it.
BUG=739658
TBR=japhet@chromium.org
Review-Url: https://codereview.chromium.org/2975623002
Cr-Commit-Position: refs/heads/master@{#486796}
Committed: https://chromium.googlesource.com/chromium/src/+/2cccbe128c2425ab5d68c74af8760fe6a7af72f4
Patch Set 1 #
Total comments: 4
Patch Set 2 : Nit (@clamy) #Patch Set 3 : Add test. #Patch Set 4 : Rebase. #
Total comments: 2
Messages
Total messages: 40 (25 generated)
Description was changed from ========== Fix: Loading cid: resources in WebView. A bug as been introduced in: https://crrev.com/2834013002. The assumption that Content-ID URLs can't be loaded without an MHTMLArchive doesn't hold. It is possible for instance with the Yahoo Mail app. It embeds chrome as a Webview. The webview load attached image with the Content-ID scheme. This CL allows it instead of blocking it. BUG=739658 ========== to ========== Fix: Loading cid: resources in WebView. A bug has been introduced in: https://crrev.com/2834013002. The assumption that Content-ID URLs can't be loaded without an MHTMLArchive doesn't hold. It is possible for instance with the Yahoo Mail app. It embeds chrome as a Webview. The webview load attached image with the Content-ID scheme. This CL allows the load instead of blocking it. BUG=739658 ==========
arthursonzogni@chromium.org changed reviewers: + clamy@chromium.org, japhet@chromium.org
Hi Nate and Camille, Could you please review this CL? I did something very simple in order to facilitate the merge in M60. What I will probably do in a follow up will be what I did in the first patchset of https://codereview.chromium.org/2834013002/#ps140001 (i.e. returning true in ShouldMakeNetworkRequestForURL(GURL("cid:xxxx")) and modify RenderFrameImpl::DecidePolicyForNavigation() appropriately).
Thanks! Lgtm. https://codereview.chromium.org/2975623002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2975623002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:649: // Please note that there is some embedder of WebView that are using nit: s/Please note/Note: nit: s/is some embedder/are some embedders https://codereview.chromium.org/2975623002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:652: if (!archive_ && factory.GetType() == Resource::kMainResource && nit: this if now needs braces.
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 #2 (id:20001) has been deleted
Patchset #2 (id:40001) has been deleted
Thanks Camille! https://codereview.chromium.org/2975623002/diff/1/third_party/WebKit/Source/p... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2975623002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:649: // Please note that there is some embedder of WebView that are using On 2017/07/07 16:14:22, clamy wrote: > nit: s/Please note/Note: > nit: s/is some embedder/are some embedders Done. https://codereview.chromium.org/2975623002/diff/1/third_party/WebKit/Source/p... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:652: if (!archive_ && factory.GetType() == Resource::kMainResource && On 2017/07/07 16:14:22, clamy wrote: > nit: this if now needs braces. Done.
japhet@ gentle ping :-)
I don't have access to https://crbug.com/739658, could I get cc:ed (or even better, make it public)? LGTM, but please add a ResourceFetcherTest case for this.
On 2017/07/11 16:12:29, Nate Chapin wrote: > I don't have access to https://crbug.com/739658, could I get cc:ed (or even > better, make it public)? > > LGTM, but please add a ResourceFetcherTest case for this. Apparently I do have access, just tried to use the wrong account. My brain is still on vacation, apparently. :)
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...
Thanks! Test added. Does it looks good to you too?
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: Try jobs failed on following builders: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_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...
Patchset #4 (id:100001) has been deleted
Dry run: Try jobs failed on following builders: mac_chromium_compile_dbg_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_comp...)
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: mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)
Hi Nate, Even if I have the LGTM, I will wait for you to review the tests I added. Then I will ask to merge it in M60.
arthursonzogni@chromium.org changed reviewers: + yhirano@chromium.org
Hi yhirano@ Could you please take a look? Nate is OOO and I still need a review for the test. This patch needs to be merged in M60. +CC torne@
torne@chromium.org changed reviewers: + torne@chromium.org
The test LGTM but I'm not a blink owner, so up to you whether you want to wait for someone else to review. https://codereview.chromium.org/2975623002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2975623002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:650: // URLs for sub-resources, even without any MHTMLArchive. Please see Just a note (don't expect you to change this CL for this, but maybe a followup): while the specific broken cases we've seen in the wild are subresources, in general WebView embedders can do anything they please with URLs and there's no reason to assume it's not also used for the main resource somewhere maybe. How important is it that this check exists at all? The only URL usage we have actually forbidden in WebView is intercepting the chrome:// scheme, because the security assumptions around this are too deeply embedded in chromium to work around.
Description was changed from ========== Fix: Loading cid: resources in WebView. A bug has been introduced in: https://crrev.com/2834013002. The assumption that Content-ID URLs can't be loaded without an MHTMLArchive doesn't hold. It is possible for instance with the Yahoo Mail app. It embeds chrome as a Webview. The webview load attached image with the Content-ID scheme. This CL allows the load instead of blocking it. BUG=739658 ========== to ========== Fix: Loading cid: resources in WebView. A bug has been introduced in: https://crrev.com/2834013002. The assumption that Content-ID URLs can't be loaded without an MHTMLArchive doesn't hold. It is possible for instance with the Yahoo Mail app. It embeds chrome as a Webview. The webview load attached image with the Content-ID scheme. This CL allows the load instead of blocking it. BUG=739658 TBR=japhet@chromium.org ==========
TBR'ing Nate - we need to land this urgently to cherrypick to M60 for stable, and several people involved seem to be OOO. I've reviewed the code and test to the best of my knowledge about blink and it looks fine.
The CQ bit was checked by torne@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from clamy@chromium.org, japhet@chromium.org Link to the patchset: https://codereview.chromium.org/2975623002/#ps120001 (title: "Rebase.")
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": 120001, "attempt_start_ts": 1500046291842220,
"parent_rev": "a09001664d28ea938508e2c9081578f86d33c94f", "commit_rev":
"2cccbe128c2425ab5d68c74af8760fe6a7af72f4"}
Message was sent while issue was closed.
Description was changed from ========== Fix: Loading cid: resources in WebView. A bug has been introduced in: https://crrev.com/2834013002. The assumption that Content-ID URLs can't be loaded without an MHTMLArchive doesn't hold. It is possible for instance with the Yahoo Mail app. It embeds chrome as a Webview. The webview load attached image with the Content-ID scheme. This CL allows the load instead of blocking it. BUG=739658 TBR=japhet@chromium.org ========== to ========== Fix: Loading cid: resources in WebView. A bug has been introduced in: https://crrev.com/2834013002. The assumption that Content-ID URLs can't be loaded without an MHTMLArchive doesn't hold. It is possible for instance with the Yahoo Mail app. It embeds chrome as a Webview. The webview load attached image with the Content-ID scheme. This CL allows the load instead of blocking it. BUG=739658 TBR=japhet@chromium.org Review-Url: https://codereview.chromium.org/2975623002 Cr-Commit-Position: refs/heads/master@{#486796} Committed: https://chromium.googlesource.com/chromium/src/+/2cccbe128c2425ab5d68c74af876... ==========
Message was sent while issue was closed.
Committed patchset #4 (id:120001) as https://chromium.googlesource.com/chromium/src/+/2cccbe128c2425ab5d68c74af876...
Message was sent while issue was closed.
Thanks torne@! https://codereview.chromium.org/2975623002/diff/120001/third_party/WebKit/Sou... File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp (right): https://codereview.chromium.org/2975623002/diff/120001/third_party/WebKit/Sou... third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:650: // URLs for sub-resources, even without any MHTMLArchive. Please see On 2017/07/14 13:55:47, Torne wrote: > Just a note (don't expect you to change this CL for this, but maybe a followup): > while the specific broken cases we've seen in the wild are subresources, in > general WebView embedders can do anything they please with URLs and there's no > reason to assume it's not also used for the main resource somewhere maybe. How > important is it that this check exists at all? > > The only URL usage we have actually forbidden in WebView is intercepting the > chrome:// scheme, because the security assumptions around this are too deeply > embedded in chromium to work around. Thanks for this information. Without this check, some weird pages caused the renderer to be killed. What I will probably do in a follow up will be what I did in the first patchset of https://codereview.chromium.org/2834013002/#ps140001 (i.e. returning true in ShouldMakeNetworkRequestForURL(GURL("cid:xxxx")) and modify RenderFrameImpl::DecidePolicyForNavigation() appropriately). It will result in the removal of the part that block Content-ID URLs in the ResourceFetcher. I didn't do that right now in order to have a very simple CL that is easy to merge in M60.
Message was sent while issue was closed.
On 2017/07/17 10:58:15, arthursonzogni wrote: > Thanks torne@! > > https://codereview.chromium.org/2975623002/diff/120001/third_party/WebKit/Sou... > File third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp > (right): > > https://codereview.chromium.org/2975623002/diff/120001/third_party/WebKit/Sou... > third_party/WebKit/Source/platform/loader/fetch/ResourceFetcher.cpp:650: // URLs > for sub-resources, even without any MHTMLArchive. Please see > On 2017/07/14 13:55:47, Torne wrote: > > Just a note (don't expect you to change this CL for this, but maybe a > followup): > > while the specific broken cases we've seen in the wild are subresources, in > > general WebView embedders can do anything they please with URLs and there's no > > reason to assume it's not also used for the main resource somewhere maybe. How > > important is it that this check exists at all? > > > > The only URL usage we have actually forbidden in WebView is intercepting the > > chrome:// scheme, because the security assumptions around this are too deeply > > embedded in chromium to work around. > Thanks for this information. > > Without this check, some weird pages caused the renderer to be killed. > > What I will probably do in a follow up will be what I did in the first patchset > of https://codereview.chromium.org/2834013002/#ps140001 > (i.e. returning true in ShouldMakeNetworkRequestForURL(GURL("cid:xxxx")) and > modify RenderFrameImpl::DecidePolicyForNavigation() appropriately). > It will result in the removal of the part that block Content-ID URLs in the > ResourceFetcher. > I didn't do that right now in order to have a very simple CL that is easy to > merge in M60. Sure; that's fine, and I don't think it's that likely that any WebView apps actually *do* use cid: URLs for the main frame, so there's no rush there; it just seems better not to have the restriction if it's not needed, and your plan sounds reasonable to me. Thanks for following up! |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
