Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2075273002:
Resource requests from Save-Page-As should go through CanRequestURL checks. (Closed)
|
Created:
4 years, 6 months ago by Łukasz Anforowicz Modified:
4 years, 4 months ago CC:
asanka, chromium-reviews, darin-cc_chromium.org, jam, loading-reviews_chromium.org, rginda+watch_chromium.org, site-isolation-reviews_chromium.org Base URL:
https://chromium.googlesource.com/chromium/src.git@master Target Ref:
refs/pending/heads/master Project:
chromium Visibility:
Public. |
DescriptionResource requests from Save-Page-As should go through CanRequestURL checks.
This CL:
- Added checks to ResourceDispatcherHostImpl::BeginSaveFile to verify if
the renderer process is authorized to access a given resource.
- Removed separate code path for file: URIs that used to be implemented
in SaveFileManager::SaveLocalFile. Avoiding a separate code path
helps consolidate all authorization checks in one place.
BUG=616429
Committed: https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6
Cr-Commit-Position: refs/heads/master@{#408235}
Patch Set 1 #
Total comments: 17
Patch Set 2 : Attribute save-item-related network requests to the right frame. #Patch Set 3 : Revert changes that unnecessarily break legitimate cases. #Patch Set 4 : Add test that saving from a local file continues to work (despite removal of SaveFileManager::SaveL… #Patch Set 5 : Rebasing... #Patch Set 6 : Updated docs. #
Total comments: 10
Patch Set 7 : Tried addressing CR feedback from rdsmith@ and mmenke@. #Patch Set 8 : Fixed accuracy of the frame-counting-comment in the browser test. #Patch Set 9 : Using ResourceLoader to cancel unauthorized resource request. #Patch Set 10 : Cancelling the request via SaveFileResourceHandler::MarkAsUnauthorized. #Patch Set 11 : Rebasing... #
Total comments: 2
Patch Set 12 : Added comments + added DCHECK(!is_pending) to MarkAsUnauthorized. #
Total comments: 2
Patch Set 13 : Replace MarkAsUnauthorized with constructor argument. #Messages
Total messages: 38 (12 generated)
lukasza@chromium.org changed reviewers: + asanka@chromium.org
Asanka, can you please take a look? I am not quite sure if this CL is ready to land, but I think it is ready for discussion about the overall direction + about the remaining open issues: - open issue #1: using correct child_id (frame with the given resource, rather than always the main frame - these can differ with OOPIFs present) - open issue #2: is it ok to rewrite a link to inaccessible resources as something like <img src="./foo_files/resource-failed-to-save-<guid>.txt"> https://codereview.chromium.org/2075273002/diff/1/chrome/test/data/save_page/... File chrome/test/data/save_page/unauthorized-access.htm (right): https://codereview.chromium.org/2075273002/diff/1/chrome/test/data/save_page/... chrome/test/data/save_page/unauthorized-access.htm:16: </html> In theory I could reuse b.htm here, but a separate test file seems cleaner, more readable (i.e. allows for comments like I have above). https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... File content/browser/download/save_file_manager.cc (left): https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... content/browser/download/save_file_manager.cc:360: } The deleted code was added in the "initial commit". https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... File content/browser/download/save_package.cc (left): https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... content/browser/download/save_package.cc:1001: } I've added these checks in https://codereview.chromium.org/1812693002. I have a feeling that they were necessary (i.e. that some tests were failing otherwise), but I cannot recall the exact reasons why I've added these checks. Tests are green, so I guess writing a link to "resource-failed-to-save-<guid>.txt" is okay. https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... content/browser/download/save_package.cc:837: // (OTOH, this only matters if OOPIFs are present). I need to double-check how process_id/child_id and render_view_routing_id and render_frame_routing_id are used by ResourceDispatcherHostImpl. Maybe I should just pass the right thing here (with appropriate tracking via an extra map in SavePackage)? OTOH, this would make it harder to merge the fix to beta/stable branches... FWIW, I don't think this is a ship blocker for --isolate-extensions, but please double-check my thought process. AFAICT, we will get the following behavior, if we always pass main frame's ID (instead of the ID of the frame this resource originated from): case #1: main frame authorized, subframe unauthorized: security bug, BUT this would only impact the case when saving a page where the main frame comes from an extension (I am not sure if this is possible; so far I couldn't trigger Save-Page dialog for background pages). case #2: main frame unauthorized, subframe authorized: not a security bug; impact is that some legitimate resources are not saved. This should not be an issue because AFAIK we don't consider save-page-as feature as a blocker for --isolate-extensions. https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... content/browser/download/save_package.cc:1001: "resource-failed-to-save-" + base::GenerateGUID() + ".txt")); This is tricky. We could analyze this with an example of a website that embeds a resource (i.e. an <img> tag) with file: URI, but it will be easier to understand with an example of an *internet* website that links to a resources on the *intranet*. Currently we forbid websites from linking to resources on the local filesystem, but in the future we plan to forbid internet websites from linking to intranet websites (https://crbug.com/590714). Let's say that Eve creates an internet webpage with a tag saying: <img src="http://bob-s-intranet/send-all-money-to-eve.jpg">. Now, I claim that: 1. The browser should not access the intranet link when *showing* the internet website 2. The browser should not access the intranet link when *saving* the internet website 3. The browser should not access the intranet link when *showing* the *saved* contents of the internet website To prevent item 3 above, we should not save the intranet link when serializing the "complete html" of the internet webpage being saved. I've decided to instead serialize a link to a non-existent file (non-existency guarantess by uniqueness of the guid). I am not sure if I am happy with this solution, but I can't think of a better alternative. I recognize that the code above can negatively impact legitimate/non-malicious scenarios. For example previously, the saved page could be opened and would renderer all resources even if some resources were inaccessible during the save process (e.g. because of network flakiness, not because of authorization issues). Hmmm... maybe we should differentiate between unauthorized and other error codes? OTOH, this would seem fragile (i.e. would require exhaustively enumerating all future error codes related to unauthorized access OR all future error codes related to authorized access). WDYT? https://codereview.chromium.org/2075273002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1915: url)) { This is based on a check done by ResourceDispatcherHostImpl::BeginDownload. AFAIK CanRequestURL(child_id) is the best we can do (i.e. 1) there are no more granular [say, origin-based] checks and 2) no other checks apply [say, ShouldServiceRequest checks]).
Description was changed from ========== Resource requests from Save-Page-As should go through CanRequestURL checks. BUG=616429 ========== to ========== Resource requests from Save-Page-As should go through CanRequestURL checks. This CL: - Added checks to ResourceDispatcherHostImpl::BeginSaveFile to verify if the renderer process is authorized to access a given resource. - Removed separate code path for file: URIs that used to be implemented in SaveFileManager::SaveLocalFile. Avoiding a separate code path helps consolidate all authorization checks in one place. - When serializing HTML, we no longer emit the original URI for resources that failed to save to disk. Instead we write a path to a non-existant local file. This ensures that a malicious website cannot trigger access to an unauthorized resource at a later time (when the saved page is opened). It is recognized that this behavior change also impacts non-malicious scenarios (i.e. when a resource failed to save because of a network issue). BUG=616429 ==========
https://codereview.chromium.org/2075273002/diff/1/chrome/test/data/save_page/... File chrome/test/data/save_page/unauthorized-access.htm (right): https://codereview.chromium.org/2075273002/diff/1/chrome/test/data/save_page/... chrome/test/data/save_page/unauthorized-access.htm:16: </html> On 2016/06/18 at 00:37:20, Łukasz Anforowicz wrote: > In theory I could reuse b.htm here, but a separate test file seems cleaner, more readable (i.e. allows for comments like I have above). Acknowledged. https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... File content/browser/download/save_package.cc (left): https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... content/browser/download/save_package.cc:1001: } On 2016/06/18 at 00:37:20, Łukasz Anforowicz wrote: > I've added these checks in https://codereview.chromium.org/1812693002. I have a feeling that they were necessary (i.e. that some tests were failing otherwise), but I cannot recall the exact reasons why I've added these checks. Tests are green, so I guess writing a link to "resource-failed-to-save-<guid>.txt" is okay. Acknowledged. https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... content/browser/download/save_package.cc:837: // (OTOH, this only matters if OOPIFs are present). On 2016/06/18 at 00:37:20, Łukasz Anforowicz wrote: > I need to double-check how process_id/child_id and render_view_routing_id and render_frame_routing_id are used by ResourceDispatcherHostImpl. Maybe I should just pass the right thing here (with appropriate tracking via an extra map in SavePackage)? OTOH, this would make it harder to merge the fix to beta/stable branches... > > > FWIW, I don't think this is a ship blocker for --isolate-extensions, but please double-check my thought process. > > AFAICT, we will get the following behavior, if we always pass main frame's ID (instead of the ID of the frame this resource originated from): > > case #1: main frame authorized, subframe unauthorized: security bug, BUT this would only impact the case when saving a page where the main frame comes from an extension (I am not sure if this is possible; so far I couldn't trigger Save-Page dialog for background pages). > > case #2: main frame unauthorized, subframe authorized: not a security bug; impact is that some legitimate resources are not saved. This should not be an issue because AFAIK we don't consider save-page-as feature as a blocker for --isolate-extensions. Thanks for calling this out. In addition to the two cases listed above, we can also be in a situation where the parent and the child frames are using different request contexts via storage partitions. In this case invoking 'Save as' would leak PII by mixing requests from different cookie stores. We do consider this to be a problem. In practice, this only applies to apps that use <webview> elements. I don't know what would happen if one were to attempt to invoke 'save as..' on an app document. Invoking "Save as" inside the webview (if it's even possible), should correctly use the inner WebContents. This is certainly something worth testing. https://codereview.chromium.org/1924473003 is the CL where I added StorageParition safety to regular downloads. You might be able to add something like the test changes made there for chrome/browser/apps/guest_view/web_view_browsertest.cc . How difficult do you think it would be to get the correct set of IDs here? https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... content/browser/download/save_package.cc:1001: "resource-failed-to-save-" + base::GenerateGUID() + ".txt")); On 2016/06/18 at 00:37:20, Łukasz Anforowicz wrote: > This is tricky. We could analyze this with an example of a website that embeds a resource (i.e. an <img> tag) with file: URI, but it will be easier to understand with an example of an *internet* website that links to a resources on the *intranet*. Currently we forbid websites from linking to resources on the local filesystem, but in the future we plan to forbid internet websites from linking to intranet websites (https://crbug.com/590714). > > Let's say that Eve creates an internet webpage with a tag saying: <img src="http://bob-s-intranet/send-all-money-to-eve.jpg">. Now, I claim that: > 1. The browser should not access the intranet link when *showing* the internet website > 2. The browser should not access the intranet link when *saving* the internet website > 3. The browser should not access the intranet link when *showing* the *saved* contents of the internet website > > To prevent item 3 above, we should not save the intranet link when serializing the "complete html" of the internet webpage being saved. I've decided to instead serialize a link to a non-existent file (non-existency guarantess by uniqueness of the guid). I am not sure if I am happy with this solution, but I can't think of a better alternative. > > I recognize that the code above can negatively impact legitimate/non-malicious scenarios. For example previously, the saved page could be opened and would renderer all resources even if some resources were inaccessible during the save process (e.g. because of network flakiness, not because of authorization issues). > > Hmmm... maybe we should differentiate between unauthorized and other error codes? OTOH, this would seem fragile (i.e. would require exhaustively enumerating all future error codes related to unauthorized access OR all future error codes related to authorized access). WDYT? As long as we are talking about subresources, I'm okay with treating all failed subresources the same way as ones that were not found. #2 should guarantee that the process of saving doesn't persist a mix of resources from intranet/internet (unless the source document is in the intranet?). So there shouldn't be a resource on disk to violate #3. However, the saved document shouldn't suddenly gain the ability to fiddle with the intranet if it didn't have that ability before. #3, I think, would only be possible by preventing file:// URLs from accessing intranet hosts. That doesn't seem in scope for issue 590714, at least from my reading. Note that mixing internet/intranet appears to be a common mechanism when dealing with IOT stuff like IP connected cameras. https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... File content/browser/download/save_types.h (left): https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... content/browser/download/save_types.h:44: SAVE_FILE_FROM_FILE Thanks for getting rid of this case. https://codereview.chromium.org/2075273002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1915: url)) { On 2016/06/18 at 00:37:20, Łukasz Anforowicz wrote: > This is based on a check done by ResourceDispatcherHostImpl::BeginDownload. AFAIK CanRequestURL(child_id) is the best we can do (i.e. 1) there are no more granular [say, origin-based] checks and 2) no other checks apply [say, ShouldServiceRequest checks]). Other things to check (you've probably got this covered, but listing here to doublecheck): * Whether a subresource load can trigger an authentication dialog. [should be no] * Whether URL embedded identities are stripped from URLs for cross origin subresource loads. [should be yes] * Whether blob URLs work.
Thanks for the feedback. I haven't been able to address all of your feedback (the big missing part is tests that you've suggested adding), but I wanted to reply before disappearing for vacation (today is my last day, I'll be back on July 18th). https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... File content/browser/download/save_package.cc (right): https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... content/browser/download/save_package.cc:837: // (OTOH, this only matters if OOPIFs are present). On 2016/06/20 20:24:18, asanka wrote: > On 2016/06/18 at 00:37:20, Łukasz Anforowicz wrote: > > I need to double-check how process_id/child_id and render_view_routing_id and > render_frame_routing_id are used by ResourceDispatcherHostImpl. Maybe I should > just pass the right thing here (with appropriate tracking via an extra map in > SavePackage)? OTOH, this would make it harder to merge the fix to beta/stable > branches... > > > > > > FWIW, I don't think this is a ship blocker for --isolate-extensions, but > please double-check my thought process. > > > > AFAICT, we will get the following behavior, if we always pass main frame's ID > (instead of the ID of the frame this resource originated from): > > > > case #1: main frame authorized, subframe unauthorized: security bug, BUT this > would only impact the case when saving a page where the main frame comes from an > extension (I am not sure if this is possible; so far I couldn't trigger > Save-Page dialog for background pages). > > > > case #2: main frame unauthorized, subframe authorized: not a security bug; > impact is that some legitimate resources are not saved. This should not be an > issue because AFAIK we don't consider save-page-as feature as a blocker for > --isolate-extensions. > > Thanks for calling this out. In addition to the two cases listed above, we can > also be in a situation where the parent and the child frames are using different > request contexts via storage partitions. In this case invoking 'Save as' would > leak PII by mixing requests from different cookie stores. We do consider this to > be a problem. Ack. > > In practice, this only applies to apps that use <webview> elements. I don't know > what would happen if one were to attempt to invoke 'save as..' on an app > document. Invoking "Save as" inside the webview (if it's even possible), should > correctly use the inner WebContents. This is certainly something worth testing. > Hmmm... ok. I'll try to figure this out when I come back from vacation... > https://codereview.chromium.org/1924473003 is the CL where I added > StorageParition safety to regular downloads. You might be able to add something > like the test changes made there for > chrome/browser/apps/guest_view/web_view_browsertest.cc . > > How difficult do you think it would be to get the correct set of IDs here? I think I have the right attribution in patchset 2. Initially, I was a bit concerned that process_id and/or routing_ids would be used in unexpected ways and that correct attribution might break it. It turns out that they *are* used in creative ways (e.g. in SaveFileManager::GetSavePackageFromRenderIds), but AFAICT everything should continue to work despite my changes. https://codereview.chromium.org/2075273002/diff/1/content/browser/download/sa... content/browser/download/save_package.cc:1001: "resource-failed-to-save-" + base::GenerateGUID() + ".txt")); On 2016/06/20 20:24:18, asanka wrote: > On 2016/06/18 at 00:37:20, Łukasz Anforowicz wrote: > > This is tricky. We could analyze this with an example of a website that > embeds a resource (i.e. an <img> tag) with file: URI, but it will be easier to > understand with an example of an *internet* website that links to a resources on > the *intranet*. Currently we forbid websites from linking to resources on the > local filesystem, but in the future we plan to forbid internet websites from > linking to intranet websites (https://crbug.com/590714). > > > > Let's say that Eve creates an internet webpage with a tag saying: <img > src="http://bob-s-intranet/send-all-money-to-eve.jpg">. Now, I claim that: > > 1. The browser should not access the intranet link when *showing* the internet > website > > 2. The browser should not access the intranet link when *saving* the internet > website > > 3. The browser should not access the intranet link when *showing* the *saved* > contents of the internet website > > > > To prevent item 3 above, we should not save the intranet link when serializing > the "complete html" of the internet webpage being saved. I've decided to > instead serialize a link to a non-existent file (non-existency guarantess by > uniqueness of the guid). I am not sure if I am happy with this solution, but I > can't think of a better alternative. > > > > I recognize that the code above can negatively impact legitimate/non-malicious > scenarios. For example previously, the saved page could be opened and would > renderer all resources even if some resources were inaccessible during the save > process (e.g. because of network flakiness, not because of authorization > issues). > > > > Hmmm... maybe we should differentiate between unauthorized and other error > codes? OTOH, this would seem fragile (i.e. would require exhaustively > enumerating all future error codes related to unauthorized access OR all future > error codes related to authorized access). WDYT? > > As long as we are talking about subresources, I'm okay with treating all failed > subresources the same way as ones that were not found. > > #2 should guarantee that the process of saving doesn't persist a mix of > resources from intranet/internet (unless the source document is in the > intranet?). So there shouldn't be a resource on disk to violate #3. However, the > saved document shouldn't suddenly gain the ability to fiddle with the intranet > if it didn't have that ability before. #3, I think, would only be possible by > preventing file:// URLs from accessing intranet hosts. That doesn't seem in > scope for issue 590714, at least from my reading. Yes - propagating origin-specific restrictions into the saved file is really tricky (given that [an untrusted] renderer controls the html contents being serialized + given that today we don't have code that recognizes that a local html file was saved from a web origin). It would probably be best to treat it as a separate issue (with MotW / mark-of-the-web being the proposed propagation mechanism). Note that I've also tried to share some related observations in https://crbug.com/616429#c13 and https://crbug.com/616429#c14. FWIW, I've reverted changes related to protecting against "unauthorized" access from the already saved page (so for subresources that failed to save we will serialize html links pointing at the original [i.e. web] uri [rather than pointing to resource-failed-to-save-<guid>.txt as I proposed in the initial patchset]). > Note that mixing internet/intranet appears to be a common mechanism when dealing > with IOT stuff like IP connected cameras. > Ack. https://codereview.chromium.org/2075273002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1915: url)) { On 2016/06/20 20:24:18, asanka wrote: > On 2016/06/18 at 00:37:20, Łukasz Anforowicz wrote: > > This is based on a check done by ResourceDispatcherHostImpl::BeginDownload. > AFAIK CanRequestURL(child_id) is the best we can do (i.e. 1) there are no more > granular [say, origin-based] checks and 2) no other checks apply [say, > ShouldServiceRequest checks]). > > Other things to check (you've probably got this covered, but listing here to > doublecheck): > > * Whether a subresource load can trigger an authentication dialog. [should be > no] This sounds like something that can be tested via /auth-basic?password=PASS&realm=REALM from embedded test server's default handlers. I would still need to figure out how to dismiss the dialog when preparing the test - when opening the page to be saved. > * Whether URL embedded identities are stripped from URLs for cross origin > subresource loads. [should be yes] I don't understand what this means :-( > * Whether blob URLs work. Ok - sounds like a good test. I hope that this can be done by simply extending chrome/test/data/save_page/frames-runtime-changes.htm
https://codereview.chromium.org/2075273002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1915: url)) { On 2016/06/21 at 16:39:34, Łukasz (vacation till July 18) wrote: > On 2016/06/20 20:24:18, asanka wrote: > > On 2016/06/18 at 00:37:20, Łukasz Anforowicz wrote: > > > This is based on a check done by ResourceDispatcherHostImpl::BeginDownload. > > AFAIK CanRequestURL(child_id) is the best we can do (i.e. 1) there are no more > > granular [say, origin-based] checks and 2) no other checks apply [say, > > ShouldServiceRequest checks]). > > > > Other things to check (you've probably got this covered, but listing here to > > doublecheck): > > > > * Whether a subresource load can trigger an authentication dialog. [should be > > no] > > This sounds like something that can be tested via /auth-basic?password=PASS&realm=REALM from embedded test server's default handlers. I would still need to figure out how to dismiss the dialog when preparing the test - when opening the page to be saved. Yeah. From the same origin, source an image or some other sub resource from /auth-basic?... > > > * Whether URL embedded identities are stripped from URLs for cross origin > > subresource loads. [should be yes] > > I don't understand what this means :-( E.g. if you have a document at foo.example.com/a.html containing <img src="http://user:password@bar.example.com/a.png">, then the network request that results from attempting to save a.html should strip user:password from the URL. This is done for regular resource loads. Which is why I wanted to see if we replicate the behavior for SavePackage downloads. THis is also related to the internet/intranet threat mitigation discussed elsewhere in this CL. > > * Whether blob URLs work. > > Ok - sounds like a good test. I hope that this can be done by simply extending chrome/test/data/save_page/frames-runtime-changes.htm
I've investigated tests suggested in the earlier CR feedback - please see my responses below. I think the CL might be ready to land in the current form. https://codereview.chromium.org/2075273002/diff/1/content/browser/loader/reso... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/1/content/browser/loader/reso... content/browser/loader/resource_dispatcher_host_impl.cc:1915: url)) { On 2016/06/21 16:53:28, asanka (On leave until Aug 29) wrote: > On 2016/06/21 at 16:39:34, Łukasz (vacation till July 18) wrote: > > On 2016/06/20 20:24:18, asanka wrote: > > > On 2016/06/18 at 00:37:20, Łukasz Anforowicz wrote: > > > > This is based on a check done by > ResourceDispatcherHostImpl::BeginDownload. > > > AFAIK CanRequestURL(child_id) is the best we can do (i.e. 1) there are no > more > > > granular [say, origin-based] checks and 2) no other checks apply [say, > > > ShouldServiceRequest checks]). > > > > > > Other things to check (you've probably got this covered, but listing here to > > > doublecheck): > > > > > > * Whether a subresource load can trigger an authentication dialog. [should > be > > > no] > > > > This sounds like something that can be tested via > /auth-basic?password=PASS&realm=REALM from embedded test server's default > handlers. I would still need to figure out how to dismiss the dialog when > preparing the test - when opening the page to be saved. > > Yeah. From the same origin, source an image or some other sub resource from > /auth-basic?... > This was broken, but this seems to be a separate issue from the current one. I've opened bug https://crbug.com/629285 to track this (and I have a tentative fix at https://codereview.chromium.org/2155333002). > > > > > * Whether URL embedded identities are stripped from URLs for cross origin > > > subresource loads. [should be yes] > > > > I don't understand what this means :-( > > E.g. if you have a document at foo.example.com/a.html containing <img > src="http://user:password@bar.example.com/a.png">, then the network request that > results from attempting to save a.html should strip user:password from the URL. > This is done for regular resource loads. Which is why I wanted to see if we > replicate the behavior for SavePackage downloads. > > THis is also related to the internet/intranet threat mitigation discussed > elsewhere in this CL. > I think I can say by reading the code that this stripping is only applied to regular resource requests, but not to 1) downloads and 2) save-page-as requests. I am saying this, because LOAD_DO_NOT_USE_EMBEDDED_IDENTITY is only used for regular resource requests. This might be addressed by avoiding auth dialog altogether during save-page-as, which is being fixed as described in my other response above. > > > * Whether blob URLs work. > > > > Ok - sounds like a good test. I hope that this can be done by simply > extending chrome/test/data/save_page/frames-runtime-changes.htm I found out that savable_resources.cc will only report resources referred to via http(s) or file URI - see https://chromium.googlesource.com/chromium/src/+/222a04026cfacf032174f4aaa372... Please open a bug if you think this is an issue. It seems that not saving resources from ftp URIs is an explicit engineering decision from the past. Not saving data: URIs should not be an issue. Not saving blob: URIs might be an issue, but only if the saved javascript is not working properly and not able to recreate the blob (and the element where it is used).
lukasza@chromium.org changed reviewers: + rdsmith@chromium.org
Randy, could you please take a look? (Asanka left quite a few good comments, but he is on a leave until the end of August :-( ).
Pretty much only nits below. Just in case our timing doesn't work out and I don't get back to this before I leave, LGTM with nits, but I'd like to do one more round if we can. https://codereview.chromium.org/2075273002/diff/100001/chrome/browser/downloa... File chrome/browser/download/save_page_browsertest.cc (right): https://codereview.chromium.org/2075273002/diff/100001/chrome/browser/downloa... chrome/browser/download/save_page_browsertest.cc:1066: // text (svg and png objects do not get a separate frame) I do not understand the relationship between the comment and the following line, probably because 4 != 7? https://codereview.chromium.org/2075273002/diff/100001/content/browser/downlo... File content/browser/download/docs/save-page-as.md (right): https://codereview.chromium.org/2075273002/diff/100001/content/browser/downlo... content/browser/download/docs/save-page-as.md:70: * Step 2: `SavePackage` first processes `SAVE_FILE_FROM_NET` Worth changing the enum name now that it's no-longer making a distinction with local files? https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1893: VLOG(1) << "Denied unauthorized save of " << url.possibly_invalid_spec(); Reasonable to make this a DVLOG? I don't think there are a lot of cases where we want to do this type of printing in the field.
mmenke@chromium.org changed reviewers: + mmenke@chromium.org
https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1897: nullptr); // |defer|; ignored by SaveFileResourceHandler. This seems kind of hideous. ResourceDispatcherHostImpl shouldn't depend on the implementation details of SaveFileResourceHandler (Not accessing |defer|, let alone setting it to |true|, or that this just magically works without setting up a ResourceLoader).
https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1897: nullptr); // |defer|; ignored by SaveFileResourceHandler. On 2016/07/21 19:38:43, mmenke wrote: > This seems kind of hideous. ResourceDispatcherHostImpl shouldn't depend on the > implementation details of SaveFileResourceHandler (Not accessing |defer|, let > alone setting it to |true|, or that this just magically works without setting up > a ResourceLoader). You're right. Mea culpa. (Mind you, this is a string the other end of which is getting BeginSaveFile out of ResourceDispatcherHostImpl, but that's clearly not in scope for this CL, and I would agree with you that calling OnResponseCompleted() in a reasonably handler agnostic way is.)
mmenke@ + rdsmith@, can you take another look please? https://codereview.chromium.org/2075273002/diff/100001/chrome/browser/downloa... File chrome/browser/download/save_page_browsertest.cc (right): https://codereview.chromium.org/2075273002/diff/100001/chrome/browser/downloa... chrome/browser/download/save_page_browsertest.cc:1066: // text (svg and png objects do not get a separate frame) On 2016/07/21 19:29:53, Randy Smith - Not in Fridays wrote: > I do not understand the relationship between the comment and the following line, > probably because 4 != 7? Ooops - done. I screwed that up in https://chromium.googlesource.com/chromium/src/+/c5aebc965aa088c133f20b51f627... https://codereview.chromium.org/2075273002/diff/100001/content/browser/downlo... File content/browser/download/docs/save-page-as.md (right): https://codereview.chromium.org/2075273002/diff/100001/content/browser/downlo... content/browser/download/docs/save-page-as.md:70: * Step 2: `SavePackage` first processes `SAVE_FILE_FROM_NET` On 2016/07/21 19:29:54, Randy Smith - Not in Fridays wrote: > Worth changing the enum name now that it's no-longer making a distinction with > local files? Maybe. I cannot think of a better name. The gist here is that SAVE_FILE_FROM_NET save items are saved via a new resource request (which can come from the net, but we hope it will come from in-memory cache). Some name ideas that I don't like: - SAVE_FILE_VIA_RESOURCE_REQUEST - it misses "FROM", so seems like a bad name for a SaveFile*Source* enum. - SAVE_FILE_FROM_RESOURCE_REQUEST - this seems confusing / I am not sure what it means to save something "from" a resource request I think that SAVE_FILE_FROM_NET is okay, even with the changes (because resource requests are a concept from the //net layer). WDYT? https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1893: VLOG(1) << "Denied unauthorized save of " << url.possibly_invalid_spec(); On 2016/07/21 19:29:54, Randy Smith - Not in Fridays wrote: > Reasonable to make this a DVLOG? I don't think there are a lot of cases where > we want to do this type of printing in the field. Done. Note that we have a VLOG for other similar messages: - in ShouldServiceRequest (line 304) - in BeginRequest (line 1460) OTOH, we have a DVLOG here: - in BeginDownload (line 718) So my change makes it 50-50 :-P. Still, I agree that this is not that useful in the field + making it a DVLOG should make the release version of the browser a tiny bit leaner. https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1897: nullptr); // |defer|; ignored by SaveFileResourceHandler. On 2016/07/21 19:52:53, Randy Smith - Not in Fridays wrote: > On 2016/07/21 19:38:43, mmenke wrote: > > This seems kind of hideous. ResourceDispatcherHostImpl shouldn't depend on > the > > implementation details of SaveFileResourceHandler (Not accessing |defer|, let > > alone setting it to |true|, or that this just magically works without setting > up > > a ResourceLoader). > > You're right. Mea culpa. > > (Mind you, this is a string the other end of which is getting BeginSaveFile out > of ResourceDispatcherHostImpl, but that's clearly not in scope for this CL, and > I would agree with you that calling OnResponseCompleted() in a reasonably > handler agnostic way is.) Thanks for raising this up. I thought that it might be okay to depend on the type, because SaveFileResourceHandler is created right above on line 1886. I am not sure what "calling OnResponseCompleted in a reasonable handler agnostic way" means :-( In particular, there is no documentation of |defer| parameter and I am not sure if it is appropriate to duplicate what other callers of OnResponseCompleted do (|if (!defer) ResumeIfDeferred();|) since there is nothing save-page-related to resume and resuming non-save-page-related things seems out of place here. FWIW, in the latest patchset I've tried to duplicate what another piece of code is doing at https://cs-staging.chromium.org/chromium/src/content/browser/loader/resource_... (including darin's TODO... :-o).
https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1897: nullptr); // |defer|; ignored by SaveFileResourceHandler. On 2016/07/21 23:44:30, Łukasz Anforowicz wrote: > On 2016/07/21 19:52:53, Randy Smith - Not in Fridays wrote: > > On 2016/07/21 19:38:43, mmenke wrote: > > > This seems kind of hideous. ResourceDispatcherHostImpl shouldn't depend on > > the > > > implementation details of SaveFileResourceHandler (Not accessing |defer|, > let > > > alone setting it to |true|, or that this just magically works without > setting > > up > > > a ResourceLoader). > > > > You're right. Mea culpa. > > > > (Mind you, this is a string the other end of which is getting BeginSaveFile > out > > of ResourceDispatcherHostImpl, but that's clearly not in scope for this CL, > and > > I would agree with you that calling OnResponseCompleted() in a reasonably > > handler agnostic way is.) > > Thanks for raising this up. I thought that it might be okay to depend on the > type, because SaveFileResourceHandler is created right above on line 1886. > > I am not sure what "calling OnResponseCompleted in a reasonable handler agnostic > way" means :-( In particular, there is no documentation of |defer| parameter > and I am not sure if it is appropriate to duplicate what other callers of > OnResponseCompleted do (|if (!defer) ResumeIfDeferred();|) since there is > nothing save-page-related to resume and resuming non-save-page-related things > seems out of place here. The method is inherited from content::ResourceHandler, so that's where they're documented. Beyond the NULL defer parameter, it also doesn't have its ResourceController field set, either. > FWIW, in the latest patchset I've tried to duplicate what another piece of code > is doing at > https://cs-staging.chromium.org/chromium/src/content/browser/loader/resource_... > (including darin's TODO... :-o). Yea....Unfortunately for you, I wasn't an owner when that was signed off on, and I don't think we need any more instances of that that's going to be hanging around for 5 more years. Code should be able to assume APIs are consumed correctly. There are a few options, as I see it: 1) Create a ResourceLoader as normal and cancel it (Need to make sure the URLRequest never starts). 2) Never create a URLRequest, just cancel the request directly. AbortRequestBeforeItStarts does this, and if that works for you, let's use it. However, I assume SaveFileResourceHandler has some sort of UI hooks to tell the user something went wrong? Maybe a magic InformWhateverAPIWe'reTalkingToOfFailure method, that combines what starting + failing the request does?
mmenke@, could you please take another look? I have little confidence in the latest changes I am making in resource_dispatcher_host_impl.cc (to be honest I had higher confidence in correctness of the previous, simple code - otoh, I see that tight coupling with SaveFileResourceHandler's implementation might be undesirable for long-term maintenance, despite presence of regression tests that exercise that path). FWIW, I've verified that in the latest patchset (using ResourceLoader::CancelRequest + ResourceDispatcherHostImpl::StartLoading) I do get a call to SaveFileResourceHandler::OnResponseCompleted with false==status.is_success() when running SavePageBrowserTest.SaveUnauthorizedResource that is being added in the current CL. This correctly propagates into !is_success call to SavePackage::SaveFinished. So it seems that the new code is working (at least at a first glance). On 2016/07/22 00:12:16, mmenke wrote: > https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... > content/browser/loader/resource_dispatcher_host_impl.cc:1897: nullptr); // > |defer|; ignored by SaveFileResourceHandler. > On 2016/07/21 23:44:30, Łukasz Anforowicz wrote: > > On 2016/07/21 19:52:53, Randy Smith - Not in Fridays wrote: > > > On 2016/07/21 19:38:43, mmenke wrote: > > > > This seems kind of hideous. ResourceDispatcherHostImpl shouldn't depend > on > > > the > > > > implementation details of SaveFileResourceHandler (Not accessing |defer|, > > let > > > > alone setting it to |true|, or that this just magically works without > > setting > > > up > > > > a ResourceLoader). > > > > > > You're right. Mea culpa. > > > > > > (Mind you, this is a string the other end of which is getting BeginSaveFile > > out > > > of ResourceDispatcherHostImpl, but that's clearly not in scope for this CL, > > and > > > I would agree with you that calling OnResponseCompleted() in a reasonably > > > handler agnostic way is.) > > > > Thanks for raising this up. I thought that it might be okay to depend on the > > type, because SaveFileResourceHandler is created right above on line 1886. > > > > I am not sure what "calling OnResponseCompleted in a reasonable handler > agnostic > > way" means :-( In particular, there is no documentation of |defer| parameter > > and I am not sure if it is appropriate to duplicate what other callers of > > OnResponseCompleted do (|if (!defer) ResumeIfDeferred();|) since there is > > nothing save-page-related to resume and resuming non-save-page-related things > > seems out of place here. > > The method is inherited from content::ResourceHandler, so that's where they're > documented. Ooops. You're right. When looking at the comments I thought that I am looking at the base class, but apparently I was looking at the child... :-( > Beyond the NULL defer parameter, it also doesn't have its > ResourceController field set, either. Err... okay. Not sure which ResourceController I can / should use here. I can instantiate an ad-hoc ResourceLoader I guess (i.e. copy&paste the instantiation from ResourceDispatcherHostImpl::BeginRequestInternal). > > FWIW, in the latest patchset I've tried to duplicate what another piece of > code > > is doing at > > > https://cs-staging.chromium.org/chromium/src/content/browser/loader/resource_... > > (including darin's TODO... :-o). > > Yea....Unfortunately for you, I wasn't an owner when that was signed off on, and > I don't think we need any more instances of that that's going to be hanging > around for 5 more years. Code should be able to assume APIs are consumed > correctly. > > There are a few options, as I see it: > > 1) Create a ResourceLoader as normal and cancel it This is what I am trying to do in the latest patchset. This is slightly tricky wrt management of the lifetime of the ResourceLoader. If I destroy ResourceLoader upon return from BeginSaveFile, then the ResponseCompleted task posted by ResourceLoader::CancelRequestInternal will never run (because of weak ptr usage). OTOH, I am not sure where I should stash the std::unique_ptr<ResourceLoader> - pending_loaders_? blocked_loaders_? just call StartLoading (not sure how that impacts your comment to "make sure the URLRequest never starts")? > (Need to make sure the URLRequest never starts). I am not sure what that means :-(. I am not calling BeginRequestInternal in this code path, but for managing lifetime of ResourceLoader I ended up calling StartLoading. OTOH, I checked that in this case ResourceLoader::StartRequestInternal exits early, because |!request_->status().is_success()| - AFAICT the success is set by URLRequest::DoCancel. > 2) Never create a URLRequest, just cancel the request directly. > AbortRequestBeforeItStarts does this, and if that works for you, let's use it. I couldn't figure out how to do this. - Inside BeginSaveFile ResourceDispatcherHostImpl::filter_ is null (and I don't know what other instance of ResourceMessageFilter I could pass to AbortRequestBeforeItStarts). - I am not sure how request ids work. I see that BeginSaveFile decrements request_id_ which makes me wary of using this field (maybe save-file-related requests don't need a request_id? there is no renderer involved fwiw). > However, I assume SaveFileResourceHandler has some sort of UI hooks to tell the > user something went wrong? SaveFileResourceHandler::OnResponseCompleted will propagate the status of the resource request through SaveFileManager::SaveFinished (FILE thread) and SavePackage::SaveFinished(...is_success...) (UI thread). OTOH note that failure to save a subresource of a page doesn't cause failure of the whole save-page - we just won't rewrite links to this subresource (e.g. we won't rewrite img.src to point at ./foo_files/subresource.png). > Maybe a magic > InformWhateverAPIWe'reTalkingToOfFailure method, that combines what starting + > failing the request does? I think that won't be needed based on the error-propagation-explanation I gave above.
On 2016/07/22 18:10:45, Łukasz Anforowicz wrote: > mmenke@, could you please take another look? I have little confidence in the > latest changes I am making in resource_dispatcher_host_impl.cc (to be honest I > had higher confidence in correctness of the previous, simple code - otoh, I see > that tight coupling with SaveFileResourceHandler's implementation might be > undesirable for long-term maintenance, despite presence of regression tests that > exercise that path). > > FWIW, I've verified that in the latest patchset (using > ResourceLoader::CancelRequest + ResourceDispatcherHostImpl::StartLoading) I do > get a call to SaveFileResourceHandler::OnResponseCompleted with > false==status.is_success() when running > SavePageBrowserTest.SaveUnauthorizedResource that is being added in the current > CL. This correctly propagates into !is_success call to > SavePackage::SaveFinished. So it seems that the new code is working (at least > at a first glance). > > On 2016/07/22 00:12:16, mmenke wrote: > > > https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... > > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > > > > https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... > > content/browser/loader/resource_dispatcher_host_impl.cc:1897: nullptr); // > > |defer|; ignored by SaveFileResourceHandler. > > On 2016/07/21 23:44:30, Łukasz Anforowicz wrote: > > > On 2016/07/21 19:52:53, Randy Smith - Not in Fridays wrote: > > > > On 2016/07/21 19:38:43, mmenke wrote: > > > > > This seems kind of hideous. ResourceDispatcherHostImpl shouldn't depend > > on > > > > the > > > > > implementation details of SaveFileResourceHandler (Not accessing > |defer|, > > > let > > > > > alone setting it to |true|, or that this just magically works without > > > setting > > > > up > > > > > a ResourceLoader). > > > > > > > > You're right. Mea culpa. > > > > > > > > (Mind you, this is a string the other end of which is getting > BeginSaveFile > > > out > > > > of ResourceDispatcherHostImpl, but that's clearly not in scope for this > CL, > > > and > > > > I would agree with you that calling OnResponseCompleted() in a reasonably > > > > handler agnostic way is.) > > > > > > Thanks for raising this up. I thought that it might be okay to depend on > the > > > type, because SaveFileResourceHandler is created right above on line 1886. > > > > > > I am not sure what "calling OnResponseCompleted in a reasonable handler > > agnostic > > > way" means :-( In particular, there is no documentation of |defer| > parameter > > > and I am not sure if it is appropriate to duplicate what other callers of > > > OnResponseCompleted do (|if (!defer) ResumeIfDeferred();|) since there is > > > nothing save-page-related to resume and resuming non-save-page-related > things > > > seems out of place here. > > > > The method is inherited from content::ResourceHandler, so that's where they're > > documented. > > Ooops. You're right. When looking at the comments I thought that I am looking > at the base class, but apparently I was looking at the child... :-( > > > Beyond the NULL defer parameter, it also doesn't have its > > ResourceController field set, either. > > Err... okay. Not sure which ResourceController I can / should use here. I can > instantiate an ad-hoc ResourceLoader I guess (i.e. copy&paste the instantiation > from ResourceDispatcherHostImpl::BeginRequestInternal). > > > > FWIW, in the latest patchset I've tried to duplicate what another piece of > > code > > > is doing at > > > > > > https://cs-staging.chromium.org/chromium/src/content/browser/loader/resource_... > > > (including darin's TODO... :-o). > > > > Yea....Unfortunately for you, I wasn't an owner when that was signed off on, > and > > I don't think we need any more instances of that that's going to be hanging > > around for 5 more years. Code should be able to assume APIs are consumed > > correctly. > > > > There are a few options, as I see it: > > > > 1) Create a ResourceLoader as normal and cancel it > > This is what I am trying to do in the latest patchset. > > This is slightly tricky wrt management of the lifetime of the ResourceLoader. > If I destroy ResourceLoader upon return from BeginSaveFile, then the > ResponseCompleted task posted by ResourceLoader::CancelRequestInternal will > never run (because of weak ptr usage). OTOH, I am not sure where I should stash > the std::unique_ptr<ResourceLoader> - pending_loaders_? blocked_loaders_? just > call StartLoading (not sure how that impacts your comment to "make sure the > URLRequest never starts")? > > > (Need to make sure the URLRequest never starts). > > I am not sure what that means :-(. I am not calling BeginRequestInternal in > this code path, but for managing lifetime of ResourceLoader I ended up calling > StartLoading. OTOH, I checked that in this case > ResourceLoader::StartRequestInternal exits early, because > |!request_->status().is_success()| - AFAICT the success is set by > URLRequest::DoCancel. > > > 2) Never create a URLRequest, just cancel the request directly. > > AbortRequestBeforeItStarts does this, and if that works for you, let's use it. > > I couldn't figure out how to do this. > > - Inside BeginSaveFile ResourceDispatcherHostImpl::filter_ is null (and I don't > know what other instance of ResourceMessageFilter I could pass to > AbortRequestBeforeItStarts). > > - I am not sure how request ids work. I see that BeginSaveFile decrements > request_id_ which makes me wary of using this field (maybe save-file-related > requests don't need a request_id? there is no renderer involved fwiw). > > > However, I assume SaveFileResourceHandler has some sort of UI hooks to tell > the > > user something went wrong? > > SaveFileResourceHandler::OnResponseCompleted will propagate the status of the > resource request through SaveFileManager::SaveFinished (FILE thread) and > SavePackage::SaveFinished(...is_success...) (UI thread). OTOH note that failure > to save a subresource of a page doesn't cause failure of the whole save-page - > we just won't rewrite links to this subresource (e.g. we won't rewrite img.src > to point at ./foo_files/subresource.png). > > > Maybe a magic > > InformWhateverAPIWe'reTalkingToOfFailure method, that combines what starting + > > failing the request does? > > I think that won't be needed based on the error-propagation-explanation I gave > above.
Description was changed from ========== Resource requests from Save-Page-As should go through CanRequestURL checks. This CL: - Added checks to ResourceDispatcherHostImpl::BeginSaveFile to verify if the renderer process is authorized to access a given resource. - Removed separate code path for file: URIs that used to be implemented in SaveFileManager::SaveLocalFile. Avoiding a separate code path helps consolidate all authorization checks in one place. - When serializing HTML, we no longer emit the original URI for resources that failed to save to disk. Instead we write a path to a non-existant local file. This ensures that a malicious website cannot trigger access to an unauthorized resource at a later time (when the saved page is opened). It is recognized that this behavior change also impacts non-malicious scenarios (i.e. when a resource failed to save because of a network issue). BUG=616429 ========== to ========== Resource requests from Save-Page-As should go through CanRequestURL checks. This CL: - Added checks to ResourceDispatcherHostImpl::BeginSaveFile to verify if the renderer process is authorized to access a given resource. - Removed separate code path for file: URIs that used to be implemented in SaveFileManager::SaveLocalFile. Avoiding a separate code path helps consolidate all authorization checks in one place. BUG=616429 ==========
On 2016/07/22 18:10:45, Łukasz Anforowicz wrote: > mmenke@, could you please take another look? I have little confidence in the > latest changes I am making in resource_dispatcher_host_impl.cc (to be honest I > had higher confidence in correctness of the previous, simple code - otoh, I see > that tight coupling with SaveFileResourceHandler's implementation might be > undesirable for long-term maintenance, despite presence of regression tests that > exercise that path). > > FWIW, I've verified that in the latest patchset (using > ResourceLoader::CancelRequest + ResourceDispatcherHostImpl::StartLoading) I do > get a call to SaveFileResourceHandler::OnResponseCompleted with > false==status.is_success() when running > SavePageBrowserTest.SaveUnauthorizedResource that is being added in the current > CL. This correctly propagates into !is_success call to > SavePackage::SaveFinished. So it seems that the new code is working (at least > at a first glance). > > On 2016/07/22 00:12:16, mmenke wrote: > > > https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... > > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > > > > https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... > > content/browser/loader/resource_dispatcher_host_impl.cc:1897: nullptr); // > > |defer|; ignored by SaveFileResourceHandler. > > On 2016/07/21 23:44:30, Łukasz Anforowicz wrote: > > > On 2016/07/21 19:52:53, Randy Smith - Not in Fridays wrote: > > > > On 2016/07/21 19:38:43, mmenke wrote: > > > > > This seems kind of hideous. ResourceDispatcherHostImpl shouldn't depend > > on > > > > the > > > > > implementation details of SaveFileResourceHandler (Not accessing > |defer|, > > > let > > > > > alone setting it to |true|, or that this just magically works without > > > setting > > > > up > > > > > a ResourceLoader). > > > > > > > > You're right. Mea culpa. > > > > > > > > (Mind you, this is a string the other end of which is getting > BeginSaveFile > > > out > > > > of ResourceDispatcherHostImpl, but that's clearly not in scope for this > CL, > > > and > > > > I would agree with you that calling OnResponseCompleted() in a reasonably > > > > handler agnostic way is.) > > > > > > Thanks for raising this up. I thought that it might be okay to depend on > the > > > type, because SaveFileResourceHandler is created right above on line 1886. > > > > > > I am not sure what "calling OnResponseCompleted in a reasonable handler > > agnostic > > > way" means :-( In particular, there is no documentation of |defer| > parameter > > > and I am not sure if it is appropriate to duplicate what other callers of > > > OnResponseCompleted do (|if (!defer) ResumeIfDeferred();|) since there is > > > nothing save-page-related to resume and resuming non-save-page-related > things > > > seems out of place here. > > > > The method is inherited from content::ResourceHandler, so that's where they're > > documented. > > Ooops. You're right. When looking at the comments I thought that I am looking > at the base class, but apparently I was looking at the child... :-( > > > Beyond the NULL defer parameter, it also doesn't have its > > ResourceController field set, either. > > Err... okay. Not sure which ResourceController I can / should use here. I can > instantiate an ad-hoc ResourceLoader I guess (i.e. copy&paste the instantiation > from ResourceDispatcherHostImpl::BeginRequestInternal). No knowing what ResourceController is just the point - we aren't in any way using the ResourceHandler API, so we should either use it in a normal way, through a ResourceLoader, or never instantiate a ResourceHandler, but rather do the same work through another path. > > > FWIW, in the latest patchset I've tried to duplicate what another piece of > > code > > > is doing at > > > > > > https://cs-staging.chromium.org/chromium/src/content/browser/loader/resource_... > > > (including darin's TODO... :-o). > > > > Yea....Unfortunately for you, I wasn't an owner when that was signed off on, > and > > I don't think we need any more instances of that that's going to be hanging > > around for 5 more years. Code should be able to assume APIs are consumed > > correctly. > > > > There are a few options, as I see it: > > > > 1) Create a ResourceLoader as normal and cancel it > > This is what I am trying to do in the latest patchset. > > This is slightly tricky wrt management of the lifetime of the ResourceLoader. > If I destroy ResourceLoader upon return from BeginSaveFile, then the > ResponseCompleted task posted by ResourceLoader::CancelRequestInternal will > never run (because of weak ptr usage). OTOH, I am not sure where I should stash > the std::unique_ptr<ResourceLoader> - pending_loaders_? blocked_loaders_? just > call StartLoading (not sure how that impacts your comment to "make sure the > URLRequest never starts")? > > > (Need to make sure the URLRequest never starts). > > I am not sure what that means :-(. I am not calling BeginRequestInternal in > this code path, but for managing lifetime of ResourceLoader I ended up calling > StartLoading. OTOH, I checked that in this case > ResourceLoader::StartRequestInternal exits early, because > |!request_->status().is_success()| - AFAICT the success is set by > URLRequest::DoCancel. We're blocking a request for security reasons. It must never go over the network. If we start the underlying URLRequest, even if we later cancel the request, it may still go over the network. > > 2) Never create a URLRequest, just cancel the request directly. > > AbortRequestBeforeItStarts does this, and if that works for you, let's use it. > > I couldn't figure out how to do this. > > - Inside BeginSaveFile ResourceDispatcherHostImpl::filter_ is null (and I don't > know what other instance of ResourceMessageFilter I could pass to > AbortRequestBeforeItStarts). > > - I am not sure how request ids work. I see that BeginSaveFile decrements > request_id_ which makes me wary of using this field (maybe save-file-related > requests don't need a request_id? there is no renderer involved fwiw). Ahh...It looks like these requests don't come straight from the renderer, and don't get a renderer-assigned request ID, so cancellation has to go through another path. > > However, I assume SaveFileResourceHandler has some sort of UI hooks to tell > the > > user something went wrong? > > SaveFileResourceHandler::OnResponseCompleted will propagate the status of the > resource request through SaveFileManager::SaveFinished (FILE thread) and > SavePackage::SaveFinished(...is_success...) (UI thread). OTOH note that failure > to save a subresource of a page doesn't cause failure of the whole save-page - > we just won't rewrite links to this subresource (e.g. we won't rewrite img.src > to point at ./foo_files/subresource.png). > > > Maybe a magic > > InformWhateverAPIWe'reTalkingToOfFailure method, that combines what starting + > > failing the request does? > > I think that won't be needed based on the error-propagation-explanation I gave > above. Hrm... Creating a loader, cancelling the loader, and then calling "StartLoading" seems problematic to me as well. It looks like ResourceLoader::StartRequestInternal() checks if the request was cancelled (Before being started), so things may magically work, but... It seems like all we need to do is: BrowserThread::PostTask( BrowserThread::FILE, FROM_HERE, base::Bind(&SaveFileManager::SaveFinished, save_manager, save_item_id, save_package_id, false)); Instead of creating the SaveFileResourceHandler, though we may not want to introduce that level of dependency on the SaveFileManager to the RDH. Could we just tell the SaveFileResourceHandler to fail the request, and have that make SaveFileResourceHandler::OnWillStart return false? Open to other ideas. Just don't want to introduce a dependency on a weird path through the ResourceLoader or the SaveFileResourceHandler. I've learned that depending on non-standard paths through state machines is just asking for trouble.
On 2016/07/22 18:35:23, mmenke wrote: > On 2016/07/22 18:10:45, Łukasz Anforowicz wrote: > > mmenke@, could you please take another look? I have little confidence in the > > latest changes I am making in resource_dispatcher_host_impl.cc (to be honest I > > had higher confidence in correctness of the previous, simple code - otoh, I > see > > that tight coupling with SaveFileResourceHandler's implementation might be > > undesirable for long-term maintenance, despite presence of regression tests > that > > exercise that path). > > > > FWIW, I've verified that in the latest patchset (using > > ResourceLoader::CancelRequest + ResourceDispatcherHostImpl::StartLoading) I do > > get a call to SaveFileResourceHandler::OnResponseCompleted with > > false==status.is_success() when running > > SavePageBrowserTest.SaveUnauthorizedResource that is being added in the > current > > CL. This correctly propagates into !is_success call to > > SavePackage::SaveFinished. So it seems that the new code is working (at least > > at a first glance). > > > > On 2016/07/22 00:12:16, mmenke wrote: > > > > > > https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... > > > File content/browser/loader/resource_dispatcher_host_impl.cc (right): > > > > > > > > > https://codereview.chromium.org/2075273002/diff/100001/content/browser/loader... > > > content/browser/loader/resource_dispatcher_host_impl.cc:1897: nullptr); // > > > |defer|; ignored by SaveFileResourceHandler. > > > On 2016/07/21 23:44:30, Łukasz Anforowicz wrote: > > > > On 2016/07/21 19:52:53, Randy Smith - Not in Fridays wrote: > > > > > On 2016/07/21 19:38:43, mmenke wrote: > > > > > > This seems kind of hideous. ResourceDispatcherHostImpl shouldn't > depend > > > on > > > > > the > > > > > > implementation details of SaveFileResourceHandler (Not accessing > > |defer|, > > > > let > > > > > > alone setting it to |true|, or that this just magically works without > > > > setting > > > > > up > > > > > > a ResourceLoader). > > > > > > > > > > You're right. Mea culpa. > > > > > > > > > > (Mind you, this is a string the other end of which is getting > > BeginSaveFile > > > > out > > > > > of ResourceDispatcherHostImpl, but that's clearly not in scope for this > > CL, > > > > and > > > > > I would agree with you that calling OnResponseCompleted() in a > reasonably > > > > > handler agnostic way is.) > > > > > > > > Thanks for raising this up. I thought that it might be okay to depend on > > the > > > > type, because SaveFileResourceHandler is created right above on line 1886. > > > > > > > > I am not sure what "calling OnResponseCompleted in a reasonable handler > > > agnostic > > > > way" means :-( In particular, there is no documentation of |defer| > > parameter > > > > and I am not sure if it is appropriate to duplicate what other callers of > > > > OnResponseCompleted do (|if (!defer) ResumeIfDeferred();|) since there is > > > > nothing save-page-related to resume and resuming non-save-page-related > > things > > > > seems out of place here. > > > > > > The method is inherited from content::ResourceHandler, so that's where > they're > > > documented. > > > > Ooops. You're right. When looking at the comments I thought that I am > looking > > at the base class, but apparently I was looking at the child... :-( > > > > > Beyond the NULL defer parameter, it also doesn't have its > > > ResourceController field set, either. > > > > Err... okay. Not sure which ResourceController I can / should use here. I > can > > instantiate an ad-hoc ResourceLoader I guess (i.e. copy&paste the > instantiation > > from ResourceDispatcherHostImpl::BeginRequestInternal). > > No knowing what ResourceController is just the point - we aren't in any way > using the ResourceHandler API, so we should either use it in a normal way, > through a ResourceLoader, or never instantiate a ResourceHandler, but rather do > the same work through another path. > > > > > FWIW, in the latest patchset I've tried to duplicate what another piece of > > > code > > > > is doing at > > > > > > > > > > https://cs-staging.chromium.org/chromium/src/content/browser/loader/resource_... > > > > (including darin's TODO... :-o). > > > > > > Yea....Unfortunately for you, I wasn't an owner when that was signed off on, > > and > > > I don't think we need any more instances of that that's going to be hanging > > > around for 5 more years. Code should be able to assume APIs are consumed > > > correctly. > > > > > > There are a few options, as I see it: > > > > > > 1) Create a ResourceLoader as normal and cancel it > > > > This is what I am trying to do in the latest patchset. > > > > This is slightly tricky wrt management of the lifetime of the ResourceLoader. > > If I destroy ResourceLoader upon return from BeginSaveFile, then the > > ResponseCompleted task posted by ResourceLoader::CancelRequestInternal will > > never run (because of weak ptr usage). OTOH, I am not sure where I should > stash > > the std::unique_ptr<ResourceLoader> - pending_loaders_? blocked_loaders_? just > > call StartLoading (not sure how that impacts your comment to "make sure the > > URLRequest never starts")? > > > > > (Need to make sure the URLRequest never starts). > > > > I am not sure what that means :-(. I am not calling BeginRequestInternal in > > this code path, but for managing lifetime of ResourceLoader I ended up calling > > StartLoading. OTOH, I checked that in this case > > ResourceLoader::StartRequestInternal exits early, because > > |!request_->status().is_success()| - AFAICT the success is set by > > URLRequest::DoCancel. > > We're blocking a request for security reasons. It must never go over the > network. If we start the underlying URLRequest, even if we later cancel the > request, it may still go over the network. Thanks for reemphasizing this point - this is after all the whole point behind this CL. OTOH, I don't see how to add such verification to the new SavePageBrowserTest.SaveUnauthorizedResource test - any suggestions? In particular, I am not sure how to monitor "file:" URI requests (e.g. embedded_test_server()->RegisterRequestMonitor would only intercept http requests AFAIK. > > > 2) Never create a URLRequest, just cancel the request directly. > > > AbortRequestBeforeItStarts does this, and if that works for you, let's use > it. > > > > I couldn't figure out how to do this. > > > > - Inside BeginSaveFile ResourceDispatcherHostImpl::filter_ is null (and I > don't > > know what other instance of ResourceMessageFilter I could pass to > > AbortRequestBeforeItStarts). > > > > - I am not sure how request ids work. I see that BeginSaveFile decrements > > request_id_ which makes me wary of using this field (maybe save-file-related > > requests don't need a request_id? there is no renderer involved fwiw). > > Ahh...It looks like these requests don't come straight from the renderer, and > don't get a renderer-assigned request ID, so cancellation has to go through > another path. > > > > However, I assume SaveFileResourceHandler has some sort of UI hooks to tell > > the > > > user something went wrong? > > > > SaveFileResourceHandler::OnResponseCompleted will propagate the status of the > > resource request through SaveFileManager::SaveFinished (FILE thread) and > > SavePackage::SaveFinished(...is_success...) (UI thread). OTOH note that > failure > > to save a subresource of a page doesn't cause failure of the whole save-page - > > we just won't rewrite links to this subresource (e.g. we won't rewrite img.src > > to point at ./foo_files/subresource.png). > > > > > Maybe a magic > > > InformWhateverAPIWe'reTalkingToOfFailure method, that combines what starting > + > > > failing the request does? > > > > I think that won't be needed based on the error-propagation-explanation I gave > > above. > > Hrm... Creating a loader, cancelling the loader, and then calling > "StartLoading" seems problematic to me as well. It looks like > ResourceLoader::StartRequestInternal() checks if the request was cancelled > (Before being started), so things may magically work, but... Yes - it does seem a little bit fragile... > It seems like all we need to do is: > > BrowserThread::PostTask( > BrowserThread::FILE, FROM_HERE, > base::Bind(&SaveFileManager::SaveFinished, save_manager, save_item_id, > save_package_id, false)); > > Instead of creating the SaveFileResourceHandler, though we may not want to > introduce that level of dependency on the SaveFileManager to the RDH. Yes - this would work. > > Could we just tell the SaveFileResourceHandler to fail the request, and have > that make SaveFileResourceHandler::OnWillStart return false? Open to other > ideas. Just don't want to introduce a dependency on a weird path through the > ResourceLoader or the SaveFileResourceHandler. I've learned that depending on > non-standard paths through state machines is just asking for trouble. I have this in the latest patchset. It works and is relatively simple. There is a little bit of indirection (in that we don't return early in BeginSaveFile, but go through with regular request processing and rely on SaveFileResourceHandler cancelling in OnWillStart) but I guess this is okay. WDYT?
On 2016/07/22 19:31:47, Łukasz Anforowicz wrote: > On 2016/07/22 18:35:23, mmenke wrote: > > On 2016/07/22 18:10:45, Łukasz Anforowicz wrote: > > > I am not sure what that means :-(. I am not calling BeginRequestInternal in > > > this code path, but for managing lifetime of ResourceLoader I ended up > calling > > > StartLoading. OTOH, I checked that in this case > > > ResourceLoader::StartRequestInternal exits early, because > > > |!request_->status().is_success()| - AFAICT the success is set by > > > URLRequest::DoCancel. > > > > We're blocking a request for security reasons. It must never go over the > > network. If we start the underlying URLRequest, even if we later cancel the > > request, it may still go over the network. > > Thanks for reemphasizing this point - this is after all the whole point behind > this CL. OTOH, I don't see how to add such verification to the new > SavePageBrowserTest.SaveUnauthorizedResource test - any suggestions? In > particular, I am not sure how to monitor "file:" URI requests (e.g. > embedded_test_server()->RegisterRequestMonitor would only intercept http > requests AFAIK. You can add a URLRequestInterceptor to catch requests for particular URLs through URLRequestFilter (Must be called on the IO thread). Then make sure it never saw anything when the test is done. Make sure to remove the handler at the end of the test. > > Could we just tell the SaveFileResourceHandler to fail the request, and have > > that make SaveFileResourceHandler::OnWillStart return false? Open to other > > ideas. Just don't want to introduce a dependency on a weird path through the > > ResourceLoader or the SaveFileResourceHandler. I've learned that depending on > > non-standard paths through state machines is just asking for trouble. > > I have this in the latest patchset. It works and is relatively simple. There > is a little bit of indirection (in that we don't return early in BeginSaveFile, > but go through with regular request processing and rely on > SaveFileResourceHandler cancelling in OnWillStart) but I guess this is okay. > > WDYT? You're right about the indirection, but I can't think of an approach I like better. If SaveFileManager had a NotifyComplete/Failed/Whatever method that worked from the IO thread, I'd say just call that, but having to do the post task to the file thread from RDH seems uglier to me than this approach (Also, the SaveFileManager is currently exclusively used on the FILE and UI threads, so adding a random method that can be called on any thread doesn't seem a great addition to its API).
On 2016/07/22 19:49:10, mmenke wrote: > On 2016/07/22 19:31:47, Łukasz Anforowicz wrote: > > On 2016/07/22 18:35:23, mmenke wrote: > > > On 2016/07/22 18:10:45, Łukasz Anforowicz wrote: > > > > I am not sure what that means :-(. I am not calling BeginRequestInternal > in > > > > this code path, but for managing lifetime of ResourceLoader I ended up > > calling > > > > StartLoading. OTOH, I checked that in this case > > > > ResourceLoader::StartRequestInternal exits early, because > > > > |!request_->status().is_success()| - AFAICT the success is set by > > > > URLRequest::DoCancel. > > > > > > We're blocking a request for security reasons. It must never go over the > > > network. If we start the underlying URLRequest, even if we later cancel the > > > request, it may still go over the network. > > > > Thanks for reemphasizing this point - this is after all the whole point behind > > this CL. OTOH, I don't see how to add such verification to the new > > SavePageBrowserTest.SaveUnauthorizedResource test - any suggestions? In > > particular, I am not sure how to monitor "file:" URI requests (e.g. > > embedded_test_server()->RegisterRequestMonitor would only intercept http > > requests AFAIK. > > You can add a URLRequestInterceptor to catch requests for particular URLs > through URLRequestFilter (Must be called on the IO thread). Then make sure it > never saw anything when the test is done. Make sure to remove the handler at > the end of the test. > > > > Could we just tell the SaveFileResourceHandler to fail the request, and have > > > that make SaveFileResourceHandler::OnWillStart return false? Open to other > > > ideas. Just don't want to introduce a dependency on a weird path through > the > > > ResourceLoader or the SaveFileResourceHandler. I've learned that depending > on > > > non-standard paths through state machines is just asking for trouble. > > > > I have this in the latest patchset. It works and is relatively simple. There > > is a little bit of indirection (in that we don't return early in > BeginSaveFile, > > but go through with regular request processing and rely on > > SaveFileResourceHandler cancelling in OnWillStart) but I guess this is okay. > > > > WDYT? > > You're right about the indirection, but I can't think of an approach I like > better. > > If SaveFileManager had a NotifyComplete/Failed/Whatever method that worked from > the IO thread, I'd say just call that, but having to do the post task to the > file thread from RDH seems uglier to me than this approach (Also, the > SaveFileManager is currently exclusively used on the FILE and UI threads, so > adding a random method that can be called on any thread doesn't seem a great > addition to its API). Just to be explicit: I'm disappearing for a week, so leaving this to Matt. I'll leave myself on as reviewer in case it takes more than a week (this is the kind of thing I should solidly understand as a content/browser/loader OWNER) but I'm not blocking so that shouldn't get in the way. This is obvious from the conversation but: Don't land without Matt's stamp (I don't see a way to retract my stamp without blocking the CL, but Matt's right to want us to find a different way to do this).
Sorry for the delay. SaveFileResourceHandler and content/browser/loader LGTM. I made two suggestions, but don't think it needs re-review regardless of whether or not you take them. I think I can defer to Randy's earlier signoff for the rest? https://codereview.chromium.org/2075273002/diff/200001/content/browser/downlo... File content/browser/download/save_file_resource_handler.h (right): https://codereview.chromium.org/2075273002/diff/200001/content/browser/downlo... content/browser/download/save_file_resource_handler.h:76: void MarkAsUnauthorized() { is_authorized_ = false; } optional suggestion: To be really paranoid about this, could take this in the constructor as an enum class object. This forces the caller to explicitly declare if something is authorized whenver using this class. Yes, only used in one place, but I'm paranoid. https://codereview.chromium.org/2075273002/diff/200001/content/browser/downlo... content/browser/download/save_file_resource_handler.h:76: void MarkAsUnauthorized() { is_authorized_ = false; } optional: Could add a comment either here or maybe better, to the RDH about why we set up everything as normal except this bit when the request isn't authorized.
On 2016/07/27 14:45:19, mmenke wrote: > Sorry for the delay. No worries. Thanks for the review. > > SaveFileResourceHandler and content/browser/loader LGTM. I made two > suggestions, but don't think it needs re-review regardless of whether or not you > take them. Okay. > > I think I can defer to Randy's earlier signoff for the rest? Yes, I believe this should be ready to land given Randy's and Asanka's earlier reviews. > https://codereview.chromium.org/2075273002/diff/200001/content/browser/downlo... > File content/browser/download/save_file_resource_handler.h (right): > > https://codereview.chromium.org/2075273002/diff/200001/content/browser/downlo... > content/browser/download/save_file_resource_handler.h:76: void > MarkAsUnauthorized() { is_authorized_ = false; } > optional suggestion: To be really paranoid about this, could take this in the > constructor as an enum class object. > > This forces the caller to explicitly declare if something is authorized whenver > using this class. Yes, only used in one place, but I'm paranoid. I don't know, that doesn't feel right. SaveFileResourceHandler always begins its life as authorized - I am not sure if explicitly forcing the caller to state this helps here (because the thing we care about is not specifying the initial authorized-vs-unauthorized state, but checking ChildProcessSecurityPolicy later and marking as unauthorized if needed). > https://codereview.chromium.org/2075273002/diff/200001/content/browser/downlo... > content/browser/download/save_file_resource_handler.h:76: void > MarkAsUnauthorized() { is_authorized_ = false; } > optional: Could add a comment either here or maybe better, to the RDH about why > we set up everything as normal except this bit when the request isn't > authorized. Good point. I've added comments in both places. When writing the comments about having to call MarkAsUnauthorized before starting the request, I thought it would be good to add a DCHECK for this. I think DCHECK(!is_pending()) does a good job here (I've checked that it will get hit if [for test purposes] calling MarkAsUnauthorized after RDHI::BeginRequestInternal).
On 2016/07/27 18:48:02, Łukasz Anforowicz wrote: > On 2016/07/27 14:45:19, mmenke wrote: > > Sorry for the delay. > > No worries. Thanks for the review. > > > > SaveFileResourceHandler and content/browser/loader LGTM. I made two > > suggestions, but don't think it needs re-review regardless of whether or not > you > > take them. > > Okay. > > > > I think I can defer to Randy's earlier signoff for the rest? > > Yes, I believe this should be ready to land given Randy's and Asanka's earlier > reviews. > > > > https://codereview.chromium.org/2075273002/diff/200001/content/browser/downlo... > > File content/browser/download/save_file_resource_handler.h (right): > > > > > https://codereview.chromium.org/2075273002/diff/200001/content/browser/downlo... > > content/browser/download/save_file_resource_handler.h:76: void > > MarkAsUnauthorized() { is_authorized_ = false; } > > optional suggestion: To be really paranoid about this, could take this in the > > constructor as an enum class object. > > > > This forces the caller to explicitly declare if something is authorized > whenver > > using this class. Yes, only used in one place, but I'm paranoid. > > I don't know, that doesn't feel right. SaveFileResourceHandler always begins > its life as authorized - I am not sure if explicitly forcing the caller to state > this helps here (because the thing we care about is not specifying the initial > authorized-vs-unauthorized state, but checking ChildProcessSecurityPolicy later > and marking as unauthorized if needed). Not going to fight over this, but I'm not sure what "begins its life as authorized" means - we could trivially check if it's authorized before we create it. It's not like we can only learn it's not authorized after we create the handler.
Still LGTM https://codereview.chromium.org/2075273002/diff/220001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1900: // because we know MarkAsUnauthorized will cause the request to be nit (x2): Avoid "we" in comments, due to ambiguity ("We the code", "We the programmers", "We the pod people"...)
On 2016/07/27 18:54:18, mmenke wrote: https://codereview.chromium.org/2075273002/diff/200001/content/browser/downlo... > > > content/browser/download/save_file_resource_handler.h:76: void > > > MarkAsUnauthorized() { is_authorized_ = false; } > > > optional suggestion: To be really paranoid about this, could take this in > the > > > constructor as an enum class object. > > > > > > This forces the caller to explicitly declare if something is authorized > > whenver > > > using this class. Yes, only used in one place, but I'm paranoid. > > > > I don't know, that doesn't feel right. SaveFileResourceHandler always begins > > its life as authorized - I am not sure if explicitly forcing the caller to > state > > this helps here (because the thing we care about is not specifying the initial > > authorized-vs-unauthorized state, but checking ChildProcessSecurityPolicy > later > > and marking as unauthorized if needed). > > Not going to fight over this, but I'm not sure what "begins its life as > authorized" means - we could trivially check if it's authorized before we create > it. It's not like we can only learn it's not authorized after we create the > handler. Oh, right. Thanks for pointing out the obvious fact that MarkAsUnauthorized can be folded into the constructor. This is much nicer because setting the authorization state via the constructor totally avoids the problem of calling MarkAsUnauthorized after the request has already started. Done (I think) in latest patchset. https://codereview.chromium.org/2075273002/diff/220001/content/browser/loader... File content/browser/loader/resource_dispatcher_host_impl.cc (right): https://codereview.chromium.org/2075273002/diff/220001/content/browser/loader... content/browser/loader/resource_dispatcher_host_impl.cc:1900: // because we know MarkAsUnauthorized will cause the request to be On 2016/07/27 18:55:59, mmenke wrote: > nit (x2): Avoid "we" in comments, due to ambiguity ("We the code", "We the > programmers", "We the pod people"...) Done :-)
The CQ bit was checked by lukasza@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...
On 2016/07/27 19:16:51, Łukasz Anforowicz wrote: > On 2016/07/27 18:54:18, mmenke wrote: > https://codereview.chromium.org/2075273002/diff/200001/content/browser/downlo... > > > > content/browser/download/save_file_resource_handler.h:76: void > > > > MarkAsUnauthorized() { is_authorized_ = false; } > > > > optional suggestion: To be really paranoid about this, could take this in > > the > > > > constructor as an enum class object. > > > > > > > > This forces the caller to explicitly declare if something is authorized > > > whenver > > > > using this class. Yes, only used in one place, but I'm paranoid. > > > > > > I don't know, that doesn't feel right. SaveFileResourceHandler always > begins > > > its life as authorized - I am not sure if explicitly forcing the caller to > > state > > > this helps here (because the thing we care about is not specifying the > initial > > > authorized-vs-unauthorized state, but checking ChildProcessSecurityPolicy > > later > > > and marking as unauthorized if needed). > > > > Not going to fight over this, but I'm not sure what "begins its life as > > authorized" means - we could trivially check if it's authorized before we > create > > it. It's not like we can only learn it's not authorized after we create the > > handler. > > Oh, right. Thanks for pointing out the obvious fact that MarkAsUnauthorized can > be folded into the constructor. This is much nicer because setting the > authorization state via the constructor totally avoids the problem of calling > MarkAsUnauthorized after the request has already started. > > Done (I think) in latest patchset. Looks done to me, thanks!
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 lukasza@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from rdsmith@chromium.org, mmenke@chromium.org Link to the patchset: https://codereview.chromium.org/2075273002/#ps240001 (title: "Replace MarkAsUnauthorized with constructor argument.")
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.
Committed patchset #13 (id:240001)
Message was sent while issue was closed.
Description was changed from ========== Resource requests from Save-Page-As should go through CanRequestURL checks. This CL: - Added checks to ResourceDispatcherHostImpl::BeginSaveFile to verify if the renderer process is authorized to access a given resource. - Removed separate code path for file: URIs that used to be implemented in SaveFileManager::SaveLocalFile. Avoiding a separate code path helps consolidate all authorization checks in one place. BUG=616429 ========== to ========== Resource requests from Save-Page-As should go through CanRequestURL checks. This CL: - Added checks to ResourceDispatcherHostImpl::BeginSaveFile to verify if the renderer process is authorized to access a given resource. - Removed separate code path for file: URIs that used to be implemented in SaveFileManager::SaveLocalFile. Avoiding a separate code path helps consolidate all authorization checks in one place. BUG=616429 Committed: https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6 Cr-Commit-Position: refs/heads/master@{#408235} ==========
Message was sent while issue was closed.
Patchset 13 (id:??) landed as https://crrev.com/eff8e457298d01b437e4fd78194ad6de3c8d7ad6 Cr-Commit-Position: refs/heads/master@{#408235} |
