Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1442433003:
Add "Show saved copy" button in dino page when there's offline copy (Closed)
DescriptionAdd "Show saved copy" button in dino page when there's offline copy
When there is offline copy for the page, "Show saved copy" will be shown. Otherwise when there is offline copy for other pages, "Show all saved pages" will be shown.
BUG=559354
Committed: https://crrev.com/4bc70b49a4d47fa390b64c93a1dc1c5d99f14c0a
Cr-Commit-Position: refs/heads/master@{#361305}
Patch Set 1 : Patch #
Total comments: 2
Patch Set 2 : Some minor fixes #
Total comments: 8
Patch Set 3 : Address feedback #Patch Set 4 : Add more comments #
Total comments: 23
Patch Set 5 : Rebase #Patch Set 6 : Address more feedback #
Total comments: 6
Patch Set 7 : Add error page check #Patch Set 8 : Address more feedback #Patch Set 9 : Fix trybots #
Total comments: 20
Patch Set 10 : Address mmenke's feedback #
Total comments: 9
Patch Set 11 : Change to pass offline page status only #Patch Set 12 : Fix trybots #
Total comments: 11
Patch Set 13 : Address feedback #Patch Set 14 : Fix trybots #
Total comments: 8
Patch Set 15 : Address thestig's feedback #
Total comments: 12
Patch Set 16 : Address nasko's comment + reformat #
Total comments: 3
Patch Set 17 : Address thestig's more feedback #Patch Set 18 : Make sure IPC coming from main frame #Patch Set 19 : Update button color for "Show all saved pages" per UI review #Messages
Total messages: 87 (21 generated)
Description was changed from ========== Add "Show saved copy" button in dino page when there's offline copy BUG=491352 ========== to ========== Add "Show saved copy" button in dino page when there's offline copy When there is offline copy for the page, "Show saved copy" will be shown. Otherwise when there is offline copy for other pages, "Show all saved pages" will be shown. BUG=491352 ==========
jianli@chromium.org changed reviewers: + mmenke@chromium.org, newt@chromium.org
Patchset #1 (id:1) has been deleted
Patchset #1 (id:20001) has been deleted
https://codereview.chromium.org/1442433003/diff/40001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1442433003/diff/40001/chrome/common/localized... chrome/common/localized_error.cc:777: error_strings->Set("showOfflineCopyButton", show_offline_copy_button); What's the difference between an offline copy and a saved copy, just above?
https://codereview.chromium.org/1442433003/diff/40001/chrome/common/localized... File chrome/common/localized_error.cc (right): https://codereview.chromium.org/1442433003/diff/40001/chrome/common/localized... chrome/common/localized_error.cc:777: error_strings->Set("showOfflineCopyButton", show_offline_copy_button); On 2015/11/12 19:55:18, mmenke (OOO Nov 11 - Nov 15) wrote: > What's the difference between an offline copy and a saved copy, just above? If offline pages feature is on, we're going to show offline related button as: 1) If there is an offline copy of current page, "Show saved copy" button will be provided. Note that the button text is same as the one for showing saved copy in the cached (decided by the UI people). 2) Otherwise, "Show all saved pages" button will be provided when there is offline copies saved for other pages. To differentiate from saved copy in the cache feature, I decided to name offline related stuff as "offline ***". So I renamed "saved pages" to "offline pages" and called the new "saved copy" as "offline copy". Also, when there is something to show in the cache, neither of offline button will be offered.
jianli@chromium.org changed reviewers: + dcheng@chromium.org, rkaplow@chromium.org
rkaplow for UMA dcheng for new IPC message
https://codereview.chromium.org/1442433003/diff/60001/chrome/browser/android/... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/1442433003/diff/60001/chrome/browser/android/... chrome/browser/android/tab_android.cc:290: bool TabAndroid::HasOfflinePages() const { All the methods related to offline pages should be grouped together. And the order of the methods in the .cc and .h file should be the same. Longer term, I'd like to see the offline page functionality moved into a separate class, e.g. an OfflinePageTabHelper or something like that.
dcheng@chromium.org changed reviewers: + nasko@chromium.org
Punting the IPC review to nasko@, since this potentially involves navigation/fetching that I don't have time to dive into right now. I've included some comments from my initial review though. https://codereview.chromium.org/1442433003/diff/60001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/60001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:319: #endif I'm guessing this is to match the #if defined(OS_ANDROID) above, but can we add a comment explicitly here? https://codereview.chromium.org/1442433003/diff/60001/chrome/common/render_me... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1442433003/diff/60001/chrome/common/render_me... chrome/common/render_messages.h:352: GURL /* url */) Since the renderer controls this parameter, doesn't this mean it can load the offline copy for any page? Are there any security implications to this? From the design, it looks like we plan on using MHTML right now (which can't run scripts for security reasons). https://codereview.chromium.org/1442433003/diff/60001/chrome/common/render_me... chrome/common/render_messages.h:353: #endif Can we add an // defined(OS_ANDROID) comment here?
https://codereview.chromium.org/1442433003/diff/60001/chrome/browser/android/... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/1442433003/diff/60001/chrome/browser/android/... chrome/browser/android/tab_android.cc:290: bool TabAndroid::HasOfflinePages() const { On 2015/11/13 19:20:19, newt wrote: > All the methods related to offline pages should be grouped together. And the > order of the methods in the .cc and .h file should be the same. Done. > > Longer term, I'd like to see the offline page functionality moved into a > separate class, e.g. an OfflinePageTabHelper or something like that. Will do. Added the workitem into our tracking list. https://codereview.chromium.org/1442433003/diff/60001/chrome/browser/net/net_... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/60001/chrome/browser/net/net_... chrome/browser/net/net_error_tab_helper.cc:319: #endif On 2015/11/13 19:40:25, dcheng wrote: > I'm guessing this is to match the #if defined(OS_ANDROID) above, but can we add > a comment explicitly here? Done. https://codereview.chromium.org/1442433003/diff/60001/chrome/common/render_me... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1442433003/diff/60001/chrome/common/render_me... chrome/common/render_messages.h:352: GURL /* url */) On 2015/11/13 19:40:25, dcheng wrote: > Since the renderer controls this parameter, doesn't this mean it can load the > offline copy for any page? Are there any security implications to this? From the > design, it looks like we plan on using MHTML right now (which can't run scripts > for security reasons). Yes, the renderer could send the IPC to load the offline page, but only the error page handler has the logic to do that, not any other pages. Yes, we're using MHTML which does not support running scripts. https://codereview.chromium.org/1442433003/diff/60001/chrome/common/render_me... chrome/common/render_messages.h:353: #endif On 2015/11/13 19:40:25, dcheng wrote: > Can we add an // defined(OS_ANDROID) comment here? Done.
tab_android lgtm
https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/android... chrome/browser/android/tab_android.cc:820: void TabAndroid::LoadOfflineCopy(const GURL& url) { This parameter doesn't seem to be used anywhere. Why is it passed in? https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/android... chrome/browser/android/tab_android.cc:833: return; Empty line after the return statement and before the next block of code. https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:313: void NetErrorTabHelper::LoadOfflineCopy(const GURL& url) { Since this URL comes from the renderer, it cannot be directly trusted. It needs to be validated first. The pattern used is: GURL validated_url(url); if (validated_url.is_valid()) // do whatever is correct for invalid URLs, here probably just return Also, Daniel's comment still stands - doesn't this method allow for any page to be requested? Is this intentional? What are the allowed pages to load? https://codereview.chromium.org/1442433003/diff/100001/chrome/common/localize... File chrome/common/localized_error.h (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/common/localize... chrome/common/localized_error.h:34: HAS_OTHER_COPIES, Shouldn't this be HAS_OTHER_PAGES_COPIES or just HAS_OTHER_PAGES? As it is, I read it as "there are other copies of the page", which this isn't. https://codereview.chromium.org/1442433003/diff/100001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:329: int /* offline_page_status */) Since there is OfflinePageStatus enum, why not use it here? https://codereview.chromium.org/1442433003/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:350: // the page that fails to load due to no internet. s/internet/network connectivity/ https://codereview.chromium.org/1442433003/diff/100001/chrome/renderer/net/ne... File chrome/renderer/net/net_error_helper.h (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/renderer/net/ne... chrome/renderer/net/net_error_helper.h:128: // if offline related button will be provided in the error page. Comment should be included in the #if defined section. https://codereview.chromium.org/1442433003/diff/100001/components/error_page/... File components/error_page/renderer/net_error_helper_core.h (right): https://codereview.chromium.org/1442433003/diff/100001/components/error_page/... components/error_page/renderer/net_error_helper_core.h:120: // Loads the offline copy of the page that was saved in the storage. nit: s/in the storage/in storage/. https://codereview.chromium.org/1442433003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1442433003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67895: + <int value="21" label="Show Offline Pages Button Shown"/> Shouldn't this be "Show Offline Copy Button Clicked"? 21 is defined as NETWORK_ERROR_PAGE_SHOW_OFFLINE_COPY_BUTTON_CLICKED.
https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/android... chrome/browser/android/tab_android.cc:820: void TabAndroid::LoadOfflineCopy(const GURL& url) { On 2015/11/13 23:55:13, nasko (slow to review) wrote: > This parameter doesn't seem to be used anywhere. Why is it passed in? Added the validation at line 836. https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/android... chrome/browser/android/tab_android.cc:833: return; On 2015/11/13 23:55:13, nasko (slow to review) wrote: > Empty line after the return statement and before the next block of code. Done. https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:313: void NetErrorTabHelper::LoadOfflineCopy(const GURL& url) { On 2015/11/13 23:55:13, nasko (slow to review) wrote: > Since this URL comes from the renderer, it cannot be directly trusted. It needs > to be validated first. The pattern used is: > > GURL validated_url(url); > if (validated_url.is_valid()) > // do whatever is correct for invalid URLs, here probably just return Done. > > Also, Daniel's comment still stands - doesn't this method allow for any page to > be requested? Is this intentional? What are the allowed pages to load? No. Yes, currently only error page is allowed to trigger loading the offline page. https://codereview.chromium.org/1442433003/diff/100001/chrome/common/localize... File chrome/common/localized_error.h (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/common/localize... chrome/common/localized_error.h:34: HAS_OTHER_COPIES, On 2015/11/13 23:55:13, nasko (slow to review) wrote: > Shouldn't this be HAS_OTHER_PAGES_COPIES or just HAS_OTHER_PAGES? As it is, I > read it as "there are other copies of the page", which this isn't. Changed to HAS_OTHER_PAGES_COPIES. https://codereview.chromium.org/1442433003/diff/100001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:329: int /* offline_page_status */) On 2015/11/13 23:55:13, nasko (slow to review) wrote: > Since there is OfflinePageStatus enum, why not use it here? We will then have to add IPC::ParamTraits instantiation for this enum. Also I need to include localized_error.h in several places which might not be what we want. https://codereview.chromium.org/1442433003/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:350: // the page that fails to load due to no internet. On 2015/11/13 23:55:13, nasko (slow to review) wrote: > s/internet/network connectivity/ Done. https://codereview.chromium.org/1442433003/diff/100001/chrome/renderer/net/ne... File chrome/renderer/net/net_error_helper.h (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/renderer/net/ne... chrome/renderer/net/net_error_helper.h:128: // if offline related button will be provided in the error page. On 2015/11/13 23:55:13, nasko (slow to review) wrote: > Comment should be included in the #if defined section. Done. https://codereview.chromium.org/1442433003/diff/100001/components/error_page/... File components/error_page/renderer/net_error_helper_core.h (right): https://codereview.chromium.org/1442433003/diff/100001/components/error_page/... components/error_page/renderer/net_error_helper_core.h:120: // Loads the offline copy of the page that was saved in the storage. On 2015/11/13 23:55:14, nasko (slow to review) wrote: > nit: s/in the storage/in storage/. Done. https://codereview.chromium.org/1442433003/diff/100001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1442433003/diff/100001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:67895: + <int value="21" label="Show Offline Pages Button Shown"/> On 2015/11/13 23:55:14, nasko (slow to review) wrote: > Shouldn't this be "Show Offline Copy Button Clicked"? 21 is defined as > NETWORK_ERROR_PAGE_SHOW_OFFLINE_COPY_BUTTON_CLICKED. Done.
https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:313: void NetErrorTabHelper::LoadOfflineCopy(const GURL& url) { On 2015/11/14 00:26:05, jianli wrote: > On 2015/11/13 23:55:13, nasko (slow to review) wrote: > > Since this URL comes from the renderer, it cannot be directly trusted. It > needs > > to be validated first. The pattern used is: > > > > GURL validated_url(url); > > if (validated_url.is_valid()) > > // do whatever is correct for invalid URLs, here probably just return > > Done. > > > > > Also, Daniel's comment still stands - doesn't this method allow for any page > to > > be requested? Is this intentional? What are the allowed pages to load? > > No. Yes, currently only error page is allowed to trigger loading the offline > page. > The renderer is untrusted. Any page could claim to be an error page (And you're not even checking that the URL came from an error page, actually). Looks like the TabAndroid code will only let bookmarked pages in the "offline page cache" (For lack of a better term) to be shown, so any compromised renderer could navigate to a bookmarked paged. I guess this could leak the information about whether or not a page is bookmarked, since if it's not, the navigation won't occur. I don't see any other security or privacy issues here? A security engineer should still look at this (And you need an owner for renderer_messages.h, anyways, conveniently enough)
https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:313: void NetErrorTabHelper::LoadOfflineCopy(const GURL& url) { On 2015/11/14 00:26:05, jianli wrote: > On 2015/11/13 23:55:13, nasko (slow to review) wrote: > > Since this URL comes from the renderer, it cannot be directly trusted. It > needs > > to be validated first. The pattern used is: > > > > GURL validated_url(url); > > if (validated_url.is_valid()) > > // do whatever is correct for invalid URLs, here probably just return > > Done. > > > > > Also, Daniel's comment still stands - doesn't this method allow for any page > to > > be requested? Is this intentional? What are the allowed pages to load? > > No. Yes, currently only error page is allowed to trigger loading the offline > page. When the renderer process is exploited by an attacker, it can lie that an error page has been loaded in the process. How are we guarding against that? https://codereview.chromium.org/1442433003/diff/100001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:329: int /* offline_page_status */) On 2015/11/14 00:26:05, jianli wrote: > On 2015/11/13 23:55:13, nasko (slow to review) wrote: > > Since there is OfflinePageStatus enum, why not use it here? > > We will then have to add IPC::ParamTraits instantiation for this enum. Also I > need to include localized_error.h in several places which might not be what we > want. You don't need to add IPC::ParamTraits, you just need to define its range using a macro - IPC_ENUM_TRAITS_MIN_MAX_VALUE(type, minvalue, maxvalue); It can be put in a common header file that makes sense to be included in all the locations this is needed. https://codereview.chromium.org/1442433003/diff/140001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/1442433003/diff/140001/chrome/browser/android... chrome/browser/android/tab_android.cc:838: return; Same empty line comment applies here. https://codereview.chromium.org/1442433003/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1442433003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:68093: + <int value="21" label="Show Offline Copy Button Shown"/> Clicked.
https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:313: void NetErrorTabHelper::LoadOfflineCopy(const GURL& url) { On 2015/11/14 00:43:49, nasko (slow to review) wrote: > On 2015/11/14 00:26:05, jianli wrote: > > On 2015/11/13 23:55:13, nasko (slow to review) wrote: > > > Since this URL comes from the renderer, it cannot be directly trusted. It > > needs > > > to be validated first. The pattern used is: > > > > > > GURL validated_url(url); > > > if (validated_url.is_valid()) > > > // do whatever is correct for invalid URLs, here probably just return > > > > Done. > > > > > > > > Also, Daniel's comment still stands - doesn't this method allow for any page > > to > > > be requested? Is this intentional? What are the allowed pages to load? > > > > No. Yes, currently only error page is allowed to trigger loading the offline > > page. > > When the renderer process is exploited by an attacker, it can lie that an error > page has been loaded in the process. How are we guarding against that? Per my understanding, the button click handling in error page are done by NetErrorPageController that is injected into v8. If we have problem with this, all methods defined in NetErrorPageController face the same problem.
On 2015/11/14 00:46:05, jianli wrote: > https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... > File chrome/browser/net/net_error_tab_helper.cc (right): > > https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... > chrome/browser/net/net_error_tab_helper.cc:313: void > NetErrorTabHelper::LoadOfflineCopy(const GURL& url) { > On 2015/11/14 00:43:49, nasko (slow to review) wrote: > > On 2015/11/14 00:26:05, jianli wrote: > > > On 2015/11/13 23:55:13, nasko (slow to review) wrote: > > > > Since this URL comes from the renderer, it cannot be directly trusted. It > > > needs > > > > to be validated first. The pattern used is: > > > > > > > > GURL validated_url(url); > > > > if (validated_url.is_valid()) > > > > // do whatever is correct for invalid URLs, here probably just return > > > > > > Done. > > > > > > > > > > > Also, Daniel's comment still stands - doesn't this method allow for any > page > > > to > > > > be requested? Is this intentional? What are the allowed pages to load? > > > > > > No. Yes, currently only error page is allowed to trigger loading the offline > > > page. > > > > When the renderer process is exploited by an attacker, it can lie that an > error > > page has been loaded in the process. How are we guarding against that? > > Per my understanding, the button click handling in error page are done by > NetErrorPageController that is injected into v8. If we have problem with this, > all methods defined in NetErrorPageController face the same problem. Then all of those methods need to be audited for the security implications that any attacker can call them, unless they were reviewed in the past with those assumptions in mind.
isherman@chromium.org changed reviewers: + isherman@chromium.org
https://codereview.chromium.org/1442433003/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1442433003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:68092: - <int value="20" label="Show Saved Pages Button Click Load Error"/> What happened to this case?
https://codereview.chromium.org/1442433003/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (left): https://codereview.chromium.org/1442433003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:68092: - <int value="20" label="Show Saved Pages Button Click Load Error"/> On 2015/11/14 00:50:13, Ilya Sherman wrote: > What happened to this case? Per offline conversation, this case was never used, so it's safe to remove. Given that, histograms.xml lgtm.
On 2015/11/14 00:48:31, nasko (slow to review) wrote: > On 2015/11/14 00:46:05, jianli wrote: > > > https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... > > File chrome/browser/net/net_error_tab_helper.cc (right): > > > > > https://codereview.chromium.org/1442433003/diff/100001/chrome/browser/net/net... > > chrome/browser/net/net_error_tab_helper.cc:313: void > > NetErrorTabHelper::LoadOfflineCopy(const GURL& url) { > > On 2015/11/14 00:43:49, nasko (slow to review) wrote: > > > On 2015/11/14 00:26:05, jianli wrote: > > > > On 2015/11/13 23:55:13, nasko (slow to review) wrote: > > > > > Since this URL comes from the renderer, it cannot be directly trusted. > It > > > > needs > > > > > to be validated first. The pattern used is: > > > > > > > > > > GURL validated_url(url); > > > > > if (validated_url.is_valid()) > > > > > // do whatever is correct for invalid URLs, here probably just return > > > > > > > > Done. > > > > > > > > > > > > > > Also, Daniel's comment still stands - doesn't this method allow for any > > page > > > > to > > > > > be requested? Is this intentional? What are the allowed pages to load? > > > > > > > > No. Yes, currently only error page is allowed to trigger loading the > offline > > > > page. > > > > > > When the renderer process is exploited by an attacker, it can lie that an > > error > > > page has been loaded in the process. How are we guarding against that? > > > > Per my understanding, the button click handling in error page are done by > > NetErrorPageController that is injected into v8. If we have problem with this, > > all methods defined in NetErrorPageController face the same problem. > > Then all of those methods need to be audited for the security implications that > any attacker can call them, unless they were reviewed in the past with those > assumptions in mind. They all underwent security reviews when they were added (Note that everything that talks directly to the browser in the net error code required an addition to renderer_messages.h, just like this CL, and that file is only owned by members of the security team).
jianli@chromium.org changed reviewers: + estark@chromium.org
estark, can you review this new IPC message from the security perspective? thanks.
On 2015/11/14 01:07:26, jianli wrote: > estark, can you review this new IPC message from the security perspective? > thanks. Can we start by answering my question from above? When the renderer process is exploited by an attacker, it can lie that an error page has been loaded in the process. How are we guarding against that? P.S. In case it wasn't clear eariler, I'm on the security team and IPC reviewer.
On 2015/11/14 01:51:45, nasko (slow to review) wrote: > On 2015/11/14 01:07:26, jianli wrote: > > estark, can you review this new IPC message from the security perspective? > > thanks. > > Can we start by answering my question from above? > > When the renderer process is exploited by an attacker, it can lie that an error > page has been loaded in the process. How are we guarding against that? > > P.S. In case it wasn't clear eariler, I'm on the security team and IPC reviewer. Sorry, nasko - I associate you with the OOPIF work and navigation, forgot you were on the security team.
On 2015/11/14 01:51:45, nasko (slow to review) wrote: > On 2015/11/14 01:07:26, jianli wrote: > > estark, can you review this new IPC message from the security perspective? > > thanks. > > Can we start by answering my question from above? > > When the renderer process is exploited by an attacker, it can lie that an error > page has been loaded in the process. How are we guarding against that? When this occurs, can the compromised page in the renderer process access NetErrorPageController that was only injected to the generated error page? I am not familiar with the security design for error page. So please advice me on this. Also, when the renderer process was asking for an offline copy via IPC, the browser process will only load the saved offline copy of that page. Will this mitigate the concern? > > P.S. In case it wasn't clear eariler, I'm on the security team and IPC reviewer. Sorry, I was not aware of this. The reason for including estark is that she has been reviewing security related issues in our offline pages feature.
On 2015/11/14 06:43:12, jianli wrote: > On 2015/11/14 01:51:45, nasko (slow to review) wrote: > > On 2015/11/14 01:07:26, jianli wrote: > > > estark, can you review this new IPC message from the security perspective? > > > thanks. > > > > Can we start by answering my question from above? > > > > When the renderer process is exploited by an attacker, it can lie that an > error > > page has been loaded in the process. How are we guarding against that? > > When this occurs, can the compromised page in the renderer process access > NetErrorPageController that was only injected to the generated error page? I am > not familiar with the security design for error page. So please advice me on > this. Yes. When a renderer process is compromised, the attacker has full control over the process and can execute any code they wish, not limited to using the created NetErrorPageController, but even creating one on their own. > Also, when the renderer process was asking for an offline copy via IPC, the > browser process will only load the saved offline copy of that page. Will this > mitigate the concern? The browser process should know whether the renderer process has hit an error page or not. You can check WebContents::GetController::GetLastCommittedEntry::GetPageType(). It should say it is an error page when you are in that state. If it isn't, then the browser should not be allowing the load to proceed. > > > > P.S. In case it wasn't clear eariler, I'm on the security team and IPC > reviewer. > > Sorry, I was not aware of this. The reason for including estark is that she has > been reviewing security related issues in our offline pages feature.
On 2015/11/16 16:39:16, nasko (slow to review) wrote: > On 2015/11/14 06:43:12, jianli wrote: > > On 2015/11/14 01:51:45, nasko (slow to review) wrote: > > > On 2015/11/14 01:07:26, jianli wrote: > > > > estark, can you review this new IPC message from the security perspective? > > > > thanks. > > > > > > Can we start by answering my question from above? > > > > > > When the renderer process is exploited by an attacker, it can lie that an > > error > > > page has been loaded in the process. How are we guarding against that? > > > > When this occurs, can the compromised page in the renderer process access > > NetErrorPageController that was only injected to the generated error page? I > am > > not familiar with the security design for error page. So please advice me on > > this. > > Yes. When a renderer process is compromised, the attacker has full control over > the process and can execute any code they wish, not limited to using the created > NetErrorPageController, but even creating one on their own. > > > Also, when the renderer process was asking for an offline copy via IPC, the > > browser process will only load the saved offline copy of that page. Will this > > mitigate the concern? > > The browser process should know whether the renderer process has hit an error > page or not. You can check > WebContents::GetController::GetLastCommittedEntry::GetPageType(). It should say > it is an error page when you are in that state. If it isn't, then the browser > should not be allowing the load to proceed. Doesn't this information also ultimately come from the renderer? That will change with PlzNavigate, I suppose.
On 2015/11/16 17:15:07, mmenke (OOO Nov 11 - Nov 15) wrote: > On 2015/11/16 16:39:16, nasko (slow to review) wrote: > > On 2015/11/14 06:43:12, jianli wrote: > > > On 2015/11/14 01:51:45, nasko (slow to review) wrote: > > > > On 2015/11/14 01:07:26, jianli wrote: > > > > > estark, can you review this new IPC message from the security > perspective? > > > > > thanks. > > > > > > > > Can we start by answering my question from above? > > > > > > > > When the renderer process is exploited by an attacker, it can lie that an > > > error > > > > page has been loaded in the process. How are we guarding against that? > > > > > > When this occurs, can the compromised page in the renderer process access > > > NetErrorPageController that was only injected to the generated error page? I > > am > > > not familiar with the security design for error page. So please advice me on > > > this. > > > > Yes. When a renderer process is compromised, the attacker has full control > over > > the process and can execute any code they wish, not limited to using the > created > > NetErrorPageController, but even creating one on their own. > > > > > Also, when the renderer process was asking for an offline copy via IPC, the > > > browser process will only load the saved offline copy of that page. Will > this > > > mitigate the concern? > > > > The browser process should know whether the renderer process has hit an error > > page or not. You can check > > WebContents::GetController::GetLastCommittedEntry::GetPageType(). It should > say > > it is an error page when you are in that state. If it isn't, then the browser > > should not be allowing the load to proceed. > > Doesn't this information also ultimately come from the renderer? That will > change with PlzNavigate, I suppose. Yes, it does come from the renderer and yes, it will change with PlzNavigate. We should be writing code such that it is correct based on our security model. If there are deficiency in it (as it is prior to PlzNavigate), I don't consider it a reason to avoid adding the appropriate checks, as we are strengthening Chrome constantly.
Also added check for coming from error page per your suggested. If there is anything more I can do, please let me know. https://codereview.chromium.org/1442433003/diff/100001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1442433003/diff/100001/chrome/common/render_m... chrome/common/render_messages.h:329: int /* offline_page_status */) On 2015/11/14 00:43:49, nasko (slow to review) wrote: > On 2015/11/14 00:26:05, jianli wrote: > > On 2015/11/13 23:55:13, nasko (slow to review) wrote: > > > Since there is OfflinePageStatus enum, why not use it here? > > > > We will then have to add IPC::ParamTraits instantiation for this enum. Also I > > need to include localized_error.h in several places which might not be what we > > want. > > You don't need to add IPC::ParamTraits, you just need to define its range using > a macro - IPC_ENUM_TRAITS_MIN_MAX_VALUE(type, minvalue, maxvalue); > > It can be put in a common header file that makes sense to be included in all the > locations this is needed. Done. https://codereview.chromium.org/1442433003/diff/140001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/1442433003/diff/140001/chrome/browser/android... chrome/browser/android/tab_android.cc:838: return; On 2015/11/14 00:43:49, nasko (slow to review) wrote: > Same empty line comment applies here. Done. https://codereview.chromium.org/1442433003/diff/140001/tools/metrics/histogra... File tools/metrics/histograms/histograms.xml (right): https://codereview.chromium.org/1442433003/diff/140001/tools/metrics/histogra... tools/metrics/histograms/histograms.xml:68093: + <int value="21" label="Show Offline Copy Button Shown"/> On 2015/11/14 00:43:49, nasko (slow to review) wrote: > Clicked. Done.
Thank you for working through adding the needed security checks! LGTM with a couple of nits. https://codereview.chromium.org/1442433003/diff/200001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1442433003/diff/200001/chrome/common/render_m... chrome/common/render_messages.h:81: error_page::OfflinePageStatus::HAS_OTHER_PAGES_COPIES) nit: Defining a "LAST" element allows future changes to add more enum values without changing this file and making it invalid. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... File components/error_page/common/offline_page_types.h (right): https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/common/offline_page_types.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. No "(c)" needed.
On 2015/11/16 18:29:43, nasko (slow to review) wrote: > On 2015/11/16 17:15:07, mmenke (OOO Nov 11 - Nov 15) wrote: > > On 2015/11/16 16:39:16, nasko (slow to review) wrote: > > > On 2015/11/14 06:43:12, jianli wrote: > > > > On 2015/11/14 01:51:45, nasko (slow to review) wrote: > > > > > On 2015/11/14 01:07:26, jianli wrote: > > > > > > estark, can you review this new IPC message from the security > > perspective? > > > > > > thanks. > > > > > > > > > > Can we start by answering my question from above? > > > > > > > > > > When the renderer process is exploited by an attacker, it can lie that > an > > > > error > > > > > page has been loaded in the process. How are we guarding against that? > > > > > > > > When this occurs, can the compromised page in the renderer process access > > > > NetErrorPageController that was only injected to the generated error page? > I > > > am > > > > not familiar with the security design for error page. So please advice me > on > > > > this. > > > > > > Yes. When a renderer process is compromised, the attacker has full control > > over > > > the process and can execute any code they wish, not limited to using the > > created > > > NetErrorPageController, but even creating one on their own. > > > > > > > Also, when the renderer process was asking for an offline copy via IPC, > the > > > > browser process will only load the saved offline copy of that page. Will > > this > > > > mitigate the concern? > > > > > > The browser process should know whether the renderer process has hit an > error > > > page or not. You can check > > > WebContents::GetController::GetLastCommittedEntry::GetPageType(). It should > > say > > > it is an error page when you are in that state. If it isn't, then the > browser > > > should not be allowing the load to proceed. > > > > Doesn't this information also ultimately come from the renderer? That will > > change with PlzNavigate, I suppose. > > Yes, it does come from the renderer and yes, it will change with PlzNavigate. We > should be writing code such that it is correct based on our security model. If > there are deficiency in it (as it is prior to PlzNavigate), I don't consider it > a reason to avoid adding the appropriate checks, as we are strengthening Chrome > constantly. Is this security model documented somewhere? Last I looked, WebContents was rather lacking in documentation.
On 2015/11/17 17:42:04, mmenke wrote: > On 2015/11/16 18:29:43, nasko (slow to review) wrote: > > On 2015/11/16 17:15:07, mmenke (OOO Nov 11 - Nov 15) wrote: > > > On 2015/11/16 16:39:16, nasko (slow to review) wrote: > > > > On 2015/11/14 06:43:12, jianli wrote: > > > > > On 2015/11/14 01:51:45, nasko (slow to review) wrote: > > > > > > On 2015/11/14 01:07:26, jianli wrote: > > > > > > > estark, can you review this new IPC message from the security > > > perspective? > > > > > > > thanks. > > > > > > > > > > > > Can we start by answering my question from above? > > > > > > > > > > > > When the renderer process is exploited by an attacker, it can lie that > > an > > > > > error > > > > > > page has been loaded in the process. How are we guarding against that? > > > > > > > > > > When this occurs, can the compromised page in the renderer process > access > > > > > NetErrorPageController that was only injected to the generated error > page? > > I > > > > am > > > > > not familiar with the security design for error page. So please advice > me > > on > > > > > this. > > > > > > > > Yes. When a renderer process is compromised, the attacker has full control > > > over > > > > the process and can execute any code they wish, not limited to using the > > > created > > > > NetErrorPageController, but even creating one on their own. > > > > > > > > > Also, when the renderer process was asking for an offline copy via IPC, > > the > > > > > browser process will only load the saved offline copy of that page. Will > > > this > > > > > mitigate the concern? > > > > > > > > The browser process should know whether the renderer process has hit an > > error > > > > page or not. You can check > > > > WebContents::GetController::GetLastCommittedEntry::GetPageType(). It > should > > > say > > > > it is an error page when you are in that state. If it isn't, then the > > browser > > > > should not be allowing the load to proceed. > > > > > > Doesn't this information also ultimately come from the renderer? That will > > > change with PlzNavigate, I suppose. > > > > Yes, it does come from the renderer and yes, it will change with PlzNavigate. > We > > should be writing code such that it is correct based on our security model. If > > there are deficiency in it (as it is prior to PlzNavigate), I don't consider > it > > a reason to avoid adding the appropriate checks, as we are strengthening > Chrome > > constantly. > > Is this security model documented somewhere? Last I looked, WebContents was > rather lacking in documentation. Not trying to give you a hard time here, just want to review it to see if there are any other issues with the error pages, or any other feature I've worked on. They have had security reviews, but can never be too careful Oh, and jianli - I'll do another (Presumably final) pass this afternoon.
On 2015/11/17 17:45:20, mmenke wrote: > On 2015/11/17 17:42:04, mmenke wrote: > > On 2015/11/16 18:29:43, nasko (slow to review) wrote: > > > On 2015/11/16 17:15:07, mmenke (OOO Nov 11 - Nov 15) wrote: > > > > On 2015/11/16 16:39:16, nasko (slow to review) wrote: > > > > > On 2015/11/14 06:43:12, jianli wrote: > > > > > > On 2015/11/14 01:51:45, nasko (slow to review) wrote: > > > > > > > On 2015/11/14 01:07:26, jianli wrote: > > > > > > > > estark, can you review this new IPC message from the security > > > > perspective? > > > > > > > > thanks. > > > > > > > > > > > > > > Can we start by answering my question from above? > > > > > > > > > > > > > > When the renderer process is exploited by an attacker, it can lie > that > > > an > > > > > > error > > > > > > > page has been loaded in the process. How are we guarding against > that? > > > > > > > > > > > > When this occurs, can the compromised page in the renderer process > > access > > > > > > NetErrorPageController that was only injected to the generated error > > page? > > > I > > > > > am > > > > > > not familiar with the security design for error page. So please advice > > me > > > on > > > > > > this. > > > > > > > > > > Yes. When a renderer process is compromised, the attacker has full > control > > > > over > > > > > the process and can execute any code they wish, not limited to using the > > > > created > > > > > NetErrorPageController, but even creating one on their own. > > > > > > > > > > > Also, when the renderer process was asking for an offline copy via > IPC, > > > the > > > > > > browser process will only load the saved offline copy of that page. > Will > > > > this > > > > > > mitigate the concern? > > > > > > > > > > The browser process should know whether the renderer process has hit an > > > error > > > > > page or not. You can check > > > > > WebContents::GetController::GetLastCommittedEntry::GetPageType(). It > > should > > > > say > > > > > it is an error page when you are in that state. If it isn't, then the > > > browser > > > > > should not be allowing the load to proceed. > > > > > > > > Doesn't this information also ultimately come from the renderer? That > will > > > > change with PlzNavigate, I suppose. > > > > > > Yes, it does come from the renderer and yes, it will change with > PlzNavigate. > > We > > > should be writing code such that it is correct based on our security model. > If > > > there are deficiency in it (as it is prior to PlzNavigate), I don't consider > > it > > > a reason to avoid adding the appropriate checks, as we are strengthening > > Chrome > > > constantly. > > > > Is this security model documented somewhere? Last I looked, WebContents was > > rather lacking in documentation. > > Not trying to give you a hard time here, just want to review it to see if there > are any other issues with the error pages, or any other feature I've worked on. > They have had security reviews, but can never be too careful To be clear, I understand the untrusted renderer concept, it's the "WebContents navigation info is the Source of Truth" part of the equation I'm unfamiliar with.
On 2015/11/17 17:47:00, mmenke wrote: > On 2015/11/17 17:45:20, mmenke wrote: > > On 2015/11/17 17:42:04, mmenke wrote: > > > On 2015/11/16 18:29:43, nasko (slow to review) wrote: > > > > On 2015/11/16 17:15:07, mmenke (OOO Nov 11 - Nov 15) wrote: > > > > > On 2015/11/16 16:39:16, nasko (slow to review) wrote: > > > > > > On 2015/11/14 06:43:12, jianli wrote: > > > > > > > On 2015/11/14 01:51:45, nasko (slow to review) wrote: > > > > > > > > On 2015/11/14 01:07:26, jianli wrote: > > > > > > > > > estark, can you review this new IPC message from the security > > > > > perspective? > > > > > > > > > thanks. > > > > > > > > > > > > > > > > Can we start by answering my question from above? > > > > > > > > > > > > > > > > When the renderer process is exploited by an attacker, it can lie > > that > > > > an > > > > > > > error > > > > > > > > page has been loaded in the process. How are we guarding against > > that? > > > > > > > > > > > > > > When this occurs, can the compromised page in the renderer process > > > access > > > > > > > NetErrorPageController that was only injected to the generated error > > > page? > > > > I > > > > > > am > > > > > > > not familiar with the security design for error page. So please > advice > > > me > > > > on > > > > > > > this. > > > > > > > > > > > > Yes. When a renderer process is compromised, the attacker has full > > control > > > > > over > > > > > > the process and can execute any code they wish, not limited to using > the > > > > > created > > > > > > NetErrorPageController, but even creating one on their own. > > > > > > > > > > > > > Also, when the renderer process was asking for an offline copy via > > IPC, > > > > the > > > > > > > browser process will only load the saved offline copy of that page. > > Will > > > > > this > > > > > > > mitigate the concern? > > > > > > > > > > > > The browser process should know whether the renderer process has hit > an > > > > error > > > > > > page or not. You can check > > > > > > WebContents::GetController::GetLastCommittedEntry::GetPageType(). It > > > should > > > > > say > > > > > > it is an error page when you are in that state. If it isn't, then the > > > > browser > > > > > > should not be allowing the load to proceed. > > > > > > > > > > Doesn't this information also ultimately come from the renderer? That > > will > > > > > change with PlzNavigate, I suppose. > > > > > > > > Yes, it does come from the renderer and yes, it will change with > > PlzNavigate. > > > We > > > > should be writing code such that it is correct based on our security > model. > > If > > > > there are deficiency in it (as it is prior to PlzNavigate), I don't > consider > > > it > > > > a reason to avoid adding the appropriate checks, as we are strengthening > > > Chrome > > > > constantly. > > > > > > Is this security model documented somewhere? Last I looked, WebContents was > > > rather lacking in documentation. I don't think we have explicit documentation on it, but each project that we have has documented the security implications. I do have it on my list of things to actually write up some design/security guidelines, but want PlzNavigate and Site Isolation to be closer to reality before I do it. > > Not trying to give you a hard time here, just want to review it to see if > there > > are any other issues with the error pages, or any other feature I've worked > on. > > They have had security reviews, but can never be too careful Please, do give me hard time :). I do like to ensure my guidance is solid and reasonable, so questioning it is always welcome. > To be clear, I understand the untrusted renderer concept, it's the "WebContents > navigation info is the Source of Truth" part of the equation I'm unfamiliar > with. Well, if the concept of "untrusted renderer" is accepted as a baseline, then we can never trust them, right? Doesn't that by definition mean that the browser process is the only source we can trust? Yes, currently a compromised renderer can lie to us and some areas of the code will believe those lies (unfortunately). This doesn't mean we shouldn't trust the browser process though or that we should be taking our information from the renderer. P.S. I feel strongly about fixing the cases where we trust the renderer blindly and I'm working on projects to make this happen.
On 2015/11/17 18:00:32, nasko (slow to review) wrote: > On 2015/11/17 17:47:00, mmenke wrote: > > On 2015/11/17 17:45:20, mmenke wrote: > > > On 2015/11/17 17:42:04, mmenke wrote: > > > > On 2015/11/16 18:29:43, nasko (slow to review) wrote: > > > > > On 2015/11/16 17:15:07, mmenke (OOO Nov 11 - Nov 15) wrote: > > > > > > On 2015/11/16 16:39:16, nasko (slow to review) wrote: > > > > > > > On 2015/11/14 06:43:12, jianli wrote: > > > > > > > > On 2015/11/14 01:51:45, nasko (slow to review) wrote: > > > > > > > > > On 2015/11/14 01:07:26, jianli wrote: > > > > > > > > > > estark, can you review this new IPC message from the security > > > > > > perspective? > > > > > > > > > > thanks. > > > > > > > > > > > > > > > > > > Can we start by answering my question from above? > > > > > > > > > > > > > > > > > > When the renderer process is exploited by an attacker, it can > lie > > > that > > > > > an > > > > > > > > error > > > > > > > > > page has been loaded in the process. How are we guarding against > > > that? > > > > > > > > > > > > > > > > When this occurs, can the compromised page in the renderer process > > > > access > > > > > > > > NetErrorPageController that was only injected to the generated > error > > > > page? > > > > > I > > > > > > > am > > > > > > > > not familiar with the security design for error page. So please > > advice > > > > me > > > > > on > > > > > > > > this. > > > > > > > > > > > > > > Yes. When a renderer process is compromised, the attacker has full > > > control > > > > > > over > > > > > > > the process and can execute any code they wish, not limited to using > > the > > > > > > created > > > > > > > NetErrorPageController, but even creating one on their own. > > > > > > > > > > > > > > > Also, when the renderer process was asking for an offline copy via > > > IPC, > > > > > the > > > > > > > > browser process will only load the saved offline copy of that > page. > > > Will > > > > > > this > > > > > > > > mitigate the concern? > > > > > > > > > > > > > > The browser process should know whether the renderer process has hit > > an > > > > > error > > > > > > > page or not. You can check > > > > > > > WebContents::GetController::GetLastCommittedEntry::GetPageType(). It > > > > should > > > > > > say > > > > > > > it is an error page when you are in that state. If it isn't, then > the > > > > > browser > > > > > > > should not be allowing the load to proceed. > > > > > > > > > > > > Doesn't this information also ultimately come from the renderer? That > > > will > > > > > > change with PlzNavigate, I suppose. > > > > > > > > > > Yes, it does come from the renderer and yes, it will change with > > > PlzNavigate. > > > > We > > > > > should be writing code such that it is correct based on our security > > model. > > > If > > > > > there are deficiency in it (as it is prior to PlzNavigate), I don't > > consider > > > > it > > > > > a reason to avoid adding the appropriate checks, as we are strengthening > > > > Chrome > > > > > constantly. > > > > > > > > Is this security model documented somewhere? Last I looked, WebContents > was > > > > rather lacking in documentation. > > I don't think we have explicit documentation on it, but each project that we > have has documented the security implications. I do have it on my list of things > to actually write up some design/security guidelines, but want PlzNavigate and > Site Isolation to be closer to reality before I do it. > > > > Not trying to give you a hard time here, just want to review it to see if > > there > > > are any other issues with the error pages, or any other feature I've worked > > on. > > > They have had security reviews, but can never be too careful > > Please, do give me hard time :). I do like to ensure my guidance is solid and > reasonable, so questioning it is always welcome. > > > To be clear, I understand the untrusted renderer concept, it's the > "WebContents > > navigation info is the Source of Truth" part of the equation I'm unfamiliar > > with. > > Well, if the concept of "untrusted renderer" is accepted as a baseline, then we > can never trust them, right? Doesn't that by definition mean that the browser > process is the only source we can trust? This is true, except for the past 5 years, I've been working with navigation information from WebContents / TabContents and the NavigationController, it's never been that well documented, and it's always been clear the information that comes from its navigation state is untrusted, so my mental model has been that it's an extended part of the IPC system, and the information from it thus untrusted (And unreliable due to the same process vs cross-process navigation). > Yes, currently a compromised renderer can lie to us and some areas of the code > will believe those lies (unfortunately). This doesn't mean we shouldn't trust > the browser process though or that we should be taking our information from the > renderer. > > P.S. I feel strongly about fixing the cases where we trust the renderer blindly > and I'm working on projects to make this happen.
On 2015/11/17 18:06:45, mmenke wrote: > On 2015/11/17 18:00:32, nasko (slow to review) wrote: > > On 2015/11/17 17:47:00, mmenke wrote: > > > On 2015/11/17 17:45:20, mmenke wrote: > > > > On 2015/11/17 17:42:04, mmenke wrote: > > > > > On 2015/11/16 18:29:43, nasko (slow to review) wrote: > > > > > > On 2015/11/16 17:15:07, mmenke (OOO Nov 11 - Nov 15) wrote: > > > > > > > On 2015/11/16 16:39:16, nasko (slow to review) wrote: > > > > > > > > On 2015/11/14 06:43:12, jianli wrote: > > > > > > > > > On 2015/11/14 01:51:45, nasko (slow to review) wrote: > > > > > > > > > > On 2015/11/14 01:07:26, jianli wrote: > > > > > > > > > > > estark, can you review this new IPC message from the > security > > > > > > > perspective? > > > > > > > > > > > thanks. > > > > > > > > > > > > > > > > > > > > Can we start by answering my question from above? > > > > > > > > > > > > > > > > > > > > When the renderer process is exploited by an attacker, it can > > lie > > > > that > > > > > > an > > > > > > > > > error > > > > > > > > > > page has been loaded in the process. How are we guarding > against > > > > that? > > > > > > > > > > > > > > > > > > When this occurs, can the compromised page in the renderer > process > > > > > access > > > > > > > > > NetErrorPageController that was only injected to the generated > > error > > > > > page? > > > > > > I > > > > > > > > am > > > > > > > > > not familiar with the security design for error page. So please > > > advice > > > > > me > > > > > > on > > > > > > > > > this. > > > > > > > > > > > > > > > > Yes. When a renderer process is compromised, the attacker has full > > > > control > > > > > > > over > > > > > > > > the process and can execute any code they wish, not limited to > using > > > the > > > > > > > created > > > > > > > > NetErrorPageController, but even creating one on their own. > > > > > > > > > > > > > > > > > Also, when the renderer process was asking for an offline copy > via > > > > IPC, > > > > > > the > > > > > > > > > browser process will only load the saved offline copy of that > > page. > > > > Will > > > > > > > this > > > > > > > > > mitigate the concern? > > > > > > > > > > > > > > > > The browser process should know whether the renderer process has > hit > > > an > > > > > > error > > > > > > > > page or not. You can check > > > > > > > > WebContents::GetController::GetLastCommittedEntry::GetPageType(). > It > > > > > should > > > > > > > say > > > > > > > > it is an error page when you are in that state. If it isn't, then > > the > > > > > > browser > > > > > > > > should not be allowing the load to proceed. > > > > > > > > > > > > > > Doesn't this information also ultimately come from the renderer? > That > > > > will > > > > > > > change with PlzNavigate, I suppose. > > > > > > > > > > > > Yes, it does come from the renderer and yes, it will change with > > > > PlzNavigate. > > > > > We > > > > > > should be writing code such that it is correct based on our security > > > model. > > > > If > > > > > > there are deficiency in it (as it is prior to PlzNavigate), I don't > > > consider > > > > > it > > > > > > a reason to avoid adding the appropriate checks, as we are > strengthening > > > > > Chrome > > > > > > constantly. > > > > > > > > > > Is this security model documented somewhere? Last I looked, WebContents > > was > > > > > rather lacking in documentation. > > > > I don't think we have explicit documentation on it, but each project that we > > have has documented the security implications. I do have it on my list of > things > > to actually write up some design/security guidelines, but want PlzNavigate and > > Site Isolation to be closer to reality before I do it. > > > > > > Not trying to give you a hard time here, just want to review it to see if > > > there > > > > are any other issues with the error pages, or any other feature I've > worked > > > on. > > > > They have had security reviews, but can never be too careful > > > > Please, do give me hard time :). I do like to ensure my guidance is solid and > > reasonable, so questioning it is always welcome. > > > > > To be clear, I understand the untrusted renderer concept, it's the > > "WebContents > > > navigation info is the Source of Truth" part of the equation I'm unfamiliar > > > with. > > > > Well, if the concept of "untrusted renderer" is accepted as a baseline, then > we > > can never trust them, right? Doesn't that by definition mean that the browser > > process is the only source we can trust? > > This is true, except for the past 5 years, I've been working with navigation > information from WebContents / TabContents and the NavigationController, it's > never been that well documented, and it's always been clear the information that > comes from its navigation state is untrusted, so my mental model has been that > it's an extended part of the IPC system, and the information from it thus > untrusted (And unreliable due to the same process vs cross-process navigation). I think your mental model is correct and you should continue using it. I did not mean to imply that one should fully trust the browser process navigation state at this time. However, I do believe we should write code defensively, since in the case of just plain old bugs (no attacker) we will avoid doing the wrong thing. > > Yes, currently a compromised renderer can lie to us and some areas of the code > > will believe those lies (unfortunately). This doesn't mean we shouldn't trust > > the browser process though or that we should be taking our information from > the > > renderer. > > > > P.S. I feel strongly about fixing the cases where we trust the renderer > blindly > > and I'm working on projects to make this happen.
Should we have an integration test for this, make sure everything's hooked up correctly? Or do browser tests still not get run on Android? Suppose they probably don't. :( https://codereview.chromium.org/1442433003/diff/200001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/200001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:317: return; Should we also compare the URL with the web_contents URL (Using GetVisibleURL())? Not sure how much that really gets us, though. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... File components/error_page/common/offline_page_types.h (right): https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/common/offline_page_types.h:13: // The offline copy of current page exists. nit: The -> An https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/common/offline_page_types.h:14: HAS_PAGE_COPY, I'd say HAS_PAGE or HAS_OFFLINE_PAGE ("Offline" and "Page" seem more important than "Copy" here). Or go for the full HAS_OFFLINE_COPY_OF_PAGE. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/common/offline_page_types.h:15: // The offline copy of current page does not exist, but the offline copies of nit: The -> An https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/common/offline_page_types.h:17: HAS_OTHER_PAGES_COPIES, Suggest HAS_OTHER_PAGES or HAS_OTHER_OFFLINE_PAGES or HAS_OFFLINE_COPY_OF_OTHER_PAGES, depending on your choice above. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... File components/error_page/renderer/net_error_helper_core.cc (right): https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/renderer/net_error_helper_core.cc:708: OfflinePageStatus offline_page_status) { This seems racy - we could get information for the wrong page. Consider: 1) We start navigate to http://blah/has_offline_data. 2) Navigation makes it to the browser UI thread, browser sends value, this gets called with HAS_PAGE_COPY. 3) Navigation commits. 4) We start navigating to http://blah/has_no_offline_data 5) Request data from the IO Thread, get an error (Maybe it's blocked by policy or something - no trip to UI thread needed) 6) Error page commits, incorrectly showing button. 7) Navigation makes it to the browser UI thread, browser sends correct value. So how do we get accurate information? We could send a URL, and compare it. I'm not sure there's a simpler option, though I'd really like one. We can't, for instance, attach it to the navigation information, without touching with content, which I don't think we want to do. Open to other ideas. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... File components/error_page/renderer/net_error_helper_core.h (right): https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/renderer/net_error_helper_core.h:189: // Notifies by the browser about the offline page info. nit: "Notifies |this| that information about the presence of an offline version of the page has been received." Matches the other comments a bit better, and the "by the browser" isn't quite grammatically correct. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/renderer/net_error_helper_core.h:304: // will be provided in certain error page. provided -> shown?
https://codereview.chromium.org/1442433003/diff/200001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/200001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:317: return; On 2015/11/17 21:02:15, mmenke wrote: > Should we also compare the URL with the web_contents URL (Using > GetVisibleURL())? Not sure how much that really gets us, though. Done. https://codereview.chromium.org/1442433003/diff/200001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1442433003/diff/200001/chrome/common/render_m... chrome/common/render_messages.h:81: error_page::OfflinePageStatus::HAS_OTHER_PAGES_COPIES) On 2015/11/17 16:42:24, nasko wrote: > nit: Defining a "LAST" element allows future changes to add more enum values > without changing this file and making it invalid. Done. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... File components/error_page/common/offline_page_types.h (right): https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/common/offline_page_types.h:1: // Copyright (c) 2015 The Chromium Authors. All rights reserved. On 2015/11/17 16:42:24, nasko wrote: > No "(c)" needed. I didn't realize that we had dropped (c) part. Removed. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/common/offline_page_types.h:13: // The offline copy of current page exists. On 2015/11/17 21:02:15, mmenke wrote: > nit: The -> An Done. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/common/offline_page_types.h:14: HAS_PAGE_COPY, On 2015/11/17 21:02:15, mmenke wrote: > I'd say HAS_PAGE or HAS_OFFLINE_PAGE ("Offline" and "Page" seem more important > than "Copy" here). Or go for the full HAS_OFFLINE_COPY_OF_PAGE. Changed to HAS_OFFLINE_PAGE. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/common/offline_page_types.h:15: // The offline copy of current page does not exist, but the offline copies of On 2015/11/17 21:02:15, mmenke wrote: > nit: The -> An Done. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/common/offline_page_types.h:17: HAS_OTHER_PAGES_COPIES, On 2015/11/17 21:02:15, mmenke wrote: > Suggest HAS_OTHER_PAGES or HAS_OTHER_OFFLINE_PAGES or > HAS_OFFLINE_COPY_OF_OTHER_PAGES, depending on your choice above. Changed to HAS_OTHER_OFFLINE_PAGES. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... File components/error_page/renderer/net_error_helper_core.cc (right): https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/renderer/net_error_helper_core.cc:708: OfflinePageStatus offline_page_status) { On 2015/11/17 21:02:15, mmenke wrote: > This seems racy - we could get information for the wrong page. > > Consider: > > 1) We start navigate to http://blah/has_offline_data. > 2) Navigation makes it to the browser UI thread, browser sends value, this gets > called with HAS_PAGE_COPY. > 3) Navigation commits. > 4) We start navigating to http://blah/has_no_offline_data > 5) Request data from the IO Thread, get an error (Maybe it's blocked by policy > or something - no trip to UI thread needed) > 6) Error page commits, incorrectly showing button. > 7) Navigation makes it to the browser UI thread, browser sends correct value. > > So how do we get accurate information? We could send a URL, and compare it. > I'm not sure there's a simpler option, though I'd really like one. We can't, > for instance, attach it to the navigation information, without touching with > content, which I don't think we want to do. Open to other ideas. Yes, this has problem, but with navigating backward and forward. I fixed it by sending the list of offline page URLs. Other solution will be far more complicated than this. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... File components/error_page/renderer/net_error_helper_core.h (right): https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/renderer/net_error_helper_core.h:189: // Notifies by the browser about the offline page info. On 2015/11/17 21:02:15, mmenke wrote: > nit: > > "Notifies |this| that information about the presence of an offline version of > the page has been received." > > Matches the other comments a bit better, and the "by the browser" isn't quite > grammatically correct. Done. https://codereview.chromium.org/1442433003/diff/200001/components/error_page/... components/error_page/renderer/net_error_helper_core.h:304: // will be provided in certain error page. On 2015/11/17 21:02:15, mmenke wrote: > provided -> shown? Done.
Sending all cached URLs to the renderer not LGTM. We should not be leaking such user data to potentially compromised renderer process. https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:325: if (validated_url != web_contents()->GetVisibleURL()) Why use GetVisibleURL? This is inconsistent with the check in IsFromErrorPage and will lead to problems. https://codereview.chromium.org/1442433003/diff/220001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1442433003/diff/220001/chrome/common/render_m... chrome/common/render_messages.h:330: std::vector<GURL> /* urls of pages that have offline version */) This should not be done. This leaks unrelated user data into an untrusted process.
https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:325: if (validated_url != web_contents()->GetVisibleURL()) On 2015/11/19 15:11:41, nasko wrote: > Why use GetVisibleURL? This is inconsistent with the check in IsFromErrorPage > and will lead to problems. I was concerned about allowing any page (Even one that's nominally an error page) navigate to any cached url, both from a synchronization / racy navigation messages standpoint, and from a random page telling us to navigate to random cached pages standpoint, so I made this suggestion.
https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:325: if (validated_url != web_contents()->GetVisibleURL()) On 2015/11/19 17:39:12, mmenke wrote: > On 2015/11/19 15:11:41, nasko wrote: > > Why use GetVisibleURL? This is inconsistent with the check in IsFromErrorPage > > and will lead to problems. > > I was concerned about allowing any page (Even one that's nominally an error > page) navigate to any cached url, both from a synchronization / racy navigation > messages standpoint, and from a random page telling us to navigate to random > cached pages standpoint, so I made this suggestion. A page that has not committed should not be able to execute JS. If we want to be defensive here, I'd suggest adding a BadMessageReceived() call and seeing if this is ever hit. It will kill the renderer process and we can check for crash reports caused by this condition.
On 2015/11/19 17:52:53, nasko wrote: > https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... > File chrome/browser/net/net_error_tab_helper.cc (right): > > https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... > chrome/browser/net/net_error_tab_helper.cc:325: if (validated_url != > web_contents()->GetVisibleURL()) > On 2015/11/19 17:39:12, mmenke wrote: > > On 2015/11/19 15:11:41, nasko wrote: > > > Why use GetVisibleURL? This is inconsistent with the check in > IsFromErrorPage > > > and will lead to problems. > > > > I was concerned about allowing any page (Even one that's nominally an error > > page) navigate to any cached url, both from a synchronization / racy > navigation > > messages standpoint, and from a random page telling us to navigate to random > > cached pages standpoint, so I made this suggestion. > > A page that has not committed should not be able to execute JS. If we want to be > defensive here, I'd suggest adding a BadMessageReceived() call and seeing if > this is ever hit. It will kill the renderer process and we can check for crash > reports caused by this condition. From a race standpoint, I'm more concerned about the previous page having executed JS. If this happens, I don't think we want to kill the renderer with BadMessageReceived(). (Just consider, for instance, double clicking on the show cached page link. By the time we get the second message, we may be using a magic url for the cached version) From a security standpoint, error pages are loaded in the same renderer as the navigation would have been loaded in, so if it's a same site navigation, if the renderer were compromised, it would still be compromised when showing an error page.
On 2015/11/19 17:57:15, mmenke wrote: > On 2015/11/19 17:52:53, nasko wrote: > > > https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... > > File chrome/browser/net/net_error_tab_helper.cc (right): > > > > > https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... > > chrome/browser/net/net_error_tab_helper.cc:325: if (validated_url != > > web_contents()->GetVisibleURL()) > > On 2015/11/19 17:39:12, mmenke wrote: > > > On 2015/11/19 15:11:41, nasko wrote: > > > > Why use GetVisibleURL? This is inconsistent with the check in > > IsFromErrorPage > > > > and will lead to problems. > > > > > > I was concerned about allowing any page (Even one that's nominally an error > > > page) navigate to any cached url, both from a synchronization / racy > > navigation > > > messages standpoint, and from a random page telling us to navigate to random > > > cached pages standpoint, so I made this suggestion. > > > > A page that has not committed should not be able to execute JS. If we want to > be > > defensive here, I'd suggest adding a BadMessageReceived() call and seeing if > > this is ever hit. It will kill the renderer process and we can check for crash > > reports caused by this condition. > > From a race standpoint, I'm more concerned about the previous page having > executed JS. If this happens, I don't think we want to kill the renderer with > BadMessageReceived(). (Just consider, for instance, double clicking on the show > cached page link. By the time we get the second message, we may be using a > magic url for the cached version) I'm not sure I follow quite what sequence of events will lead to this. I'm fine leaving it as is. > From a security standpoint, error pages are loaded in the same renderer as the > navigation would have been loaded in, so if it's a same site navigation, if the > renderer were compromised, it would still be compromised when showing an error > page.
https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:325: if (validated_url != web_contents()->GetVisibleURL()) On 2015/11/19 17:39:12, mmenke wrote: > On 2015/11/19 15:11:41, nasko wrote: > > Why use GetVisibleURL? This is inconsistent with the check in IsFromErrorPage > > and will lead to problems. > > I was concerned about allowing any page (Even one that's nominally an error > page) navigate to any cached url, both from a synchronization / racy navigation > messages standpoint, and from a random page telling us to navigate to random > cached pages standpoint, so I made this suggestion. We do not allow any page to navigate to any offline url. A page could only access the offline copy of this page, not any other page. This is the reason we send URL in the IPC. We've already been doing the validattion in TabAndroid::LoadOfflineCopy. The extra check here is to just add another guard. To address nasko's concern, should we just check against GetURL here? https://codereview.chromium.org/1442433003/diff/220001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1442433003/diff/220001/chrome/common/render_m... chrome/common/render_messages.h:330: std::vector<GURL> /* urls of pages that have offline version */) On 2015/11/19 15:11:41, nasko wrote: > This should not be done. This leaks unrelated user data into an untrusted > process. We can pass a list of hash values of all URLs. How do you think?
https://codereview.chromium.org/1442433003/diff/220001/chrome/common/render_m... File chrome/common/render_messages.h (right): https://codereview.chromium.org/1442433003/diff/220001/chrome/common/render_m... chrome/common/render_messages.h:330: std::vector<GURL> /* urls of pages that have offline version */) On 2015/11/19 23:28:08, jianli wrote: > On 2015/11/19 15:11:41, nasko wrote: > > This should not be done. This leaks unrelated user data into an untrusted > > process. > > We can pass a list of hash values of all URLs. How do you think? I completely defer to Nasko here, but I wonder... Are hashes any better here? Shouldn't be hard to tell if facebook.com, google.com, etc, are cached, for instance.
jianli@chromium.org changed reviewers: - dcheng@chromium.org
jianli@chromium.org changed reviewers: - estark@chromium.org
dimich@chromium.org changed reviewers: + dimich@chromium.org
Small nit, drive-by: https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/android... chrome/browser/android/tab_android.cc:799: void TabAndroid::GetAllOfflinePageURLS( URLS->URLs
Patchset #11 (id:240001) has been deleted
Patchset #11 (id:260001) has been deleted
Patchset #11 (id:280001) has been deleted
I found out there was a bug in previous patch that only passed the offline page status. We should pass the validated_url to calling NetErrorTabHelper::SetOfflinePageInfo since this is called when provisional load was started and thus WebContents/TabAndroid did not have the URL that was navigated to. So I changed back to previous approach that only passed the status and everything worked fine. https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/android... File chrome/browser/android/tab_android.cc (right): https://codereview.chromium.org/1442433003/diff/220001/chrome/browser/android... chrome/browser/android/tab_android.cc:799: void TabAndroid::GetAllOfflinePageURLS( On 2015/11/20 01:53:39, Dmitry Titov wrote: > URLS->URLs Removed since this function is not longer needed.
Description was changed from ========== Add "Show saved copy" button in dino page when there's offline copy When there is offline copy for the page, "Show saved copy" will be shown. Otherwise when there is offline copy for other pages, "Show all saved pages" will be shown. BUG=491352 ========== to ========== Add "Show saved copy" button in dino page when there's offline copy When there is offline copy for the page, "Show saved copy" will be shown. Otherwise when there is offline copy for other pages, "Show all saved pages" will be shown. BUG=559354 ==========
fgorski@chromium.org changed reviewers: + fgorski@chromium.org
offline_pages lgtm, but please clean up the test. https://codereview.chromium.org/1442433003/diff/320001/components/offline_pag... File components/offline_pages/offline_page_model_unittest.cc (right): https://codereview.chromium.org/1442433003/diff/320001/components/offline_pag... components/offline_pages/offline_page_model_unittest.cc:857: GURL offline_url = store->last_saved_page().GetOfflineURL(); Do you need this? https://codereview.chromium.org/1442433003/diff/320001/components/offline_pag... components/offline_pages/offline_page_model_unittest.cc:868: GURL offline_url2 = store->last_saved_page().GetOfflineURL(); And that one.
Sorry for slowness, this LGTM. I assume you have gone through, or will go through before launching this, UI review. https://codereview.chromium.org/1442433003/diff/320001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/320001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:309: if (offline_page_model->HasOfflinePages()) nit: Use braces. https://codereview.chromium.org/1442433003/diff/320001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:334: GURL validated_url(url); I don't think you need to make a copy here? can just use url everywhere.
https://codereview.chromium.org/1442433003/diff/320001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/320001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:302: return; nit: Suggest blank lines after this return and the next, to make the code easier to read.
https://codereview.chromium.org/1442433003/diff/320001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/320001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:302: return; On 2015/11/23 19:55:22, mmenke wrote: > nit: Suggest blank lines after this return and the next, to make the code > easier to read. Done. https://codereview.chromium.org/1442433003/diff/320001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:309: if (offline_page_model->HasOfflinePages()) On 2015/11/23 19:54:26, mmenke wrote: > nit: Use braces. Done. https://codereview.chromium.org/1442433003/diff/320001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:334: GURL validated_url(url); On 2015/11/23 19:54:26, mmenke wrote: > I don't think you need to make a copy here? can just use url everywhere. This is introduced based on nasko's comment: Since this URL comes from the renderer, it cannot be directly trusted. It needs to be validated first. The pattern used is: GURL validated_url(url); if (validated_url.is_valid()) // do whatever is correct for invalid URLs, here probably just return https://codereview.chromium.org/1442433003/diff/320001/components/offline_pag... File components/offline_pages/offline_page_model_unittest.cc (right): https://codereview.chromium.org/1442433003/diff/320001/components/offline_pag... components/offline_pages/offline_page_model_unittest.cc:857: GURL offline_url = store->last_saved_page().GetOfflineURL(); On 2015/11/20 23:09:46, fgorski wrote: > Do you need this? Removed. https://codereview.chromium.org/1442433003/diff/320001/components/offline_pag... components/offline_pages/offline_page_model_unittest.cc:868: GURL offline_url2 = store->last_saved_page().GetOfflineURL(); On 2015/11/20 23:09:46, fgorski wrote: > And that one. Removed.
jianli@chromium.org changed reviewers: + thestig@chromium.org - dimich@chromium.org
Adding thestig for approval for general changes under chrome.
https://codereview.chromium.org/1442433003/diff/320001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/320001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:334: GURL validated_url(url); On 2015/11/23 22:08:34, jianli wrote: > On 2015/11/23 19:54:26, mmenke wrote: > > I don't think you need to make a copy here? can just use url everywhere. > > This is introduced based on nasko's comment: > Since this URL comes from the renderer, it cannot be directly trusted. > It needs to be validated first. The pattern used is: > > GURL validated_url(url); > if (validated_url.is_valid()) > // do whatever is correct for invalid URLs, here probably just return Sorry, I missed Nasko's comment. I completely to defer to Nasko on that.
nasko: Have your concerns been addressed? https://codereview.chromium.org/1442433003/diff/360001/chrome/common/BUILD.gn File chrome/common/BUILD.gn (right): https://codereview.chromium.org/1442433003/diff/360001/chrome/common/BUILD.gn... chrome/common/BUILD.gn:74: "//components/error_page/common", This is: (a) not in alphabetical order (b) already on line 67 ^ (c) not in chrome/chrome_common.gypi for some reason https://codereview.chromium.org/1442433003/diff/360001/chrome/common/DEPS File chrome/common/DEPS (right): https://codereview.chromium.org/1442433003/diff/360001/chrome/common/DEPS#new... chrome/common/DEPS:18: "+components/error_page/common", Is this needed? I see the same entry in chrome/DEPS already.
Just had a talk with nasko to clear things. He is looking into this now. https://codereview.chromium.org/1442433003/diff/360001/chrome/common/BUILD.gn File chrome/common/BUILD.gn (right): https://codereview.chromium.org/1442433003/diff/360001/chrome/common/BUILD.gn... chrome/common/BUILD.gn:74: "//components/error_page/common", On 2015/11/23 22:50:21, Lei Zhang wrote: > This is: > > (a) not in alphabetical order > (b) already on line 67 ^ > (c) not in chrome/chrome_common.gypi for some reason Thanks for catching this. This is due to my rebase from my other patch. Removed the line here. For (c), it should be in chrome_common.gypi even without my change. Not sure why it was not there. I just added it. https://codereview.chromium.org/1442433003/diff/360001/chrome/common/DEPS File chrome/common/DEPS (right): https://codereview.chromium.org/1442433003/diff/360001/chrome/common/DEPS#new... chrome/common/DEPS:18: "+components/error_page/common", On 2015/11/23 22:50:21, Lei Zhang wrote: > Is this needed? I see the same entry in chrome/DEPS already. Removed.
https://codereview.chromium.org/1442433003/diff/360001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/360001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:299: content::RenderFrameHost* render_frame_host, const GURL& url) { Each parameter should be on a new line. Also, running "git cl format" is the easiest way to ensure style guide compliance. https://codereview.chromium.org/1442433003/diff/360001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:342: if (validated_url != web_contents()->GetURL()) Please don't use "GetURL". There is a big note not to use that in the header file. Pick either GetLastCommittedURL or GetVisibleURL, depending on what you are looking for.
https://codereview.chromium.org/1442433003/diff/360001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/360001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:299: content::RenderFrameHost* render_frame_host, const GURL& url) { On 2015/11/23 23:29:08, nasko wrote: > Each parameter should be on a new line. Also, running "git cl format" is the > easiest way to ensure style guide compliance. Done. https://codereview.chromium.org/1442433003/diff/360001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:342: if (validated_url != web_contents()->GetURL()) On 2015/11/23 23:29:08, nasko wrote: > Please don't use "GetURL". There is a big note not to use that in the header > file. Pick either GetLastCommittedURL or GetVisibleURL, depending on what you > are looking for. Changed to use GetLastCommittedURL.
https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:305: offline_pages::OfflinePageModelFactory::GetForBrowserContext( Why are we doing a BrowserContext -> Profile -> BrowserContext dance? Does this not work? offline_pages::OfflinePageModel* offline_page_model = offline_pages::OfflinePageModelFactory::GetForBrowserContext( web_contents()->GetBrowserContext()); https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:322: // Makes sure that this is coming from from an error page. nit: from from, ditto elsewhere in this file. https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:337: GURL validated_url(url); Why do we need a copy of |url| ? Why not just check url.is_valid()? https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:341: DCHECK(web_contents()); Not really needed. You'd know when the DCHECK fails because it'll just crash on the next line. https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:353: if (!entry) Squish this into: return entry && (entry->GetPageType() == content::PAGE_TYPE_ERROR); https://codereview.chromium.org/1442433003/diff/370028/chrome/common/localize... File chrome/common/localized_error.h (right): https://codereview.chromium.org/1442433003/diff/370028/chrome/common/localize... chrome/common/localized_error.h:30: Not sure why this is here - git cl lint will complain about this.
https://codereview.chromium.org/1442433003/diff/390001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/390001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:338: if (!validated_url.is_valid()) A note for future people looking at this. The more correct version of validating URL is to call RenderProcessHost::FilterURL, which does more checks and ensures we only pass through URLs that are loadable in the specific renderer process. However, in this case we are doing a very strict check - comparing to the last committed URL, so we know this is valid. https://codereview.chromium.org/1442433003/diff/390001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:342: if (validated_url != web_contents()->GetLastCommittedURL()) Since we are checking the WebContents URL, we must also ensure that this message comes only from the main frame. The RenderFrameHost that sent this IPC must be the web_contents()->GetMainFrame() one.
https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:305: offline_pages::OfflinePageModelFactory::GetForBrowserContext( On 2015/11/23 23:43:37, Lei Zhang wrote: > Why are we doing a BrowserContext -> Profile -> BrowserContext dance? Does this > not work? > > offline_pages::OfflinePageModel* offline_page_model = > offline_pages::OfflinePageModelFactory::GetForBrowserContext( > web_contents()->GetBrowserContext()); Thanks for catching this. Fixed. https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:322: // Makes sure that this is coming from from an error page. On 2015/11/23 23:43:37, Lei Zhang wrote: > nit: from from, ditto elsewhere in this file. Done. https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:337: GURL validated_url(url); On 2015/11/23 23:43:37, Lei Zhang wrote: > Why do we need a copy of |url| ? Why not just check url.is_valid()? This is introduced based on nasko's comment: Since this URL comes from the renderer, it cannot be directly trusted. It needs to be validated first. The pattern used is: GURL validated_url(url); if (validated_url.is_valid()) // do whatever is correct for invalid URLs, here probably just return https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:341: DCHECK(web_contents()); On 2015/11/23 23:43:37, Lei Zhang wrote: > Not really needed. You'd know when the DCHECK fails because it'll just crash on > the next line. Done. https://codereview.chromium.org/1442433003/diff/370028/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:353: if (!entry) On 2015/11/23 23:43:37, Lei Zhang wrote: > Squish this into: > > return entry && (entry->GetPageType() == content::PAGE_TYPE_ERROR); Done. https://codereview.chromium.org/1442433003/diff/370028/chrome/common/localize... File chrome/common/localized_error.h (right): https://codereview.chromium.org/1442433003/diff/370028/chrome/common/localize... chrome/common/localized_error.h:30: On 2015/11/23 23:43:37, Lei Zhang wrote: > Not sure why this is here - git cl lint will complain about this. Done.
https://codereview.chromium.org/1442433003/diff/390001/chrome/browser/net/net... File chrome/browser/net/net_error_tab_helper.cc (right): https://codereview.chromium.org/1442433003/diff/390001/chrome/browser/net/net... chrome/browser/net/net_error_tab_helper.cc:342: if (validated_url != web_contents()->GetLastCommittedURL()) On 2015/11/23 23:47:05, nasko wrote: > Since we are checking the WebContents URL, we must also ensure that this message > comes only from the main frame. The RenderFrameHost that sent this IPC must be > the web_contents()->GetMainFrame() one. Added check in OnMessageReceived. Since another message in this class ChromeViewHostMsg_RunNetworkDiagnostics should also been triggered from main frame, I simply added the check in OnMessageReceived.
LGTM, assuming mmenke@ is ok with the main frame check in OnMessageReceived.
lgtm if all the other reviewers are happy.
On 2015/11/24 00:26:17, Lei Zhang wrote: > lgtm if all the other reviewers are happy. Still happy
Talked with mmenke@ and he was fine with adding main frame check. Also, per UI review, the only change is to change the button color for "Show all saved pages" from blue to gray. I added this change.
The CQ bit was checked by jianli@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from newt@chromium.org, isherman@chromium.org, fgorski@chromium.org, mmenke@chromium.org, thestig@chromium.org, nasko@chromium.org Link to the patchset: https://codereview.chromium.org/1442433003/#ps450001 (title: "Update button color for "Show all saved pages" per UI review")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442433003/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442433003/450001
The CQ bit was unchecked by commit-bot@chromium.org
Try jobs failed on following builders: linux_chromium_chromeos_rel_ng on tryserver.chromium.linux (JOB_FAILED, http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
The CQ bit was checked by jianli@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1442433003/450001 View timeline at https://chromium-cq-status.appspot.com/patch-timeline/1442433003/450001
Message was sent while issue was closed.
Committed patchset #19 (id:450001)
Message was sent while issue was closed.
Patchset 19 (id:??) landed as https://crrev.com/4bc70b49a4d47fa390b64c93a1dc1c5d99f14c0a Cr-Commit-Position: refs/heads/master@{#361305} |
