Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2435593007:
Temporarily reintroduce blob/filesystem URL security checks on the IO thread. (Closed)
DescriptionTemporarily reintroduce blob/filesystem URL security checks on the IO thread.
These checks were originally introduced in r419019 and r422954. This
was merged back to M54 and M55.
They were later moved to the UI thread (into
ExtensionNavigationThrottle) in r424937 to make them compatible with
PlzNavigate. However, that only exists in M56.
Per linked bug, we need to tighten the security check currently in M54
and M55 for apps with a "webview" permission where the problematic
blob/filesystem URL requests aren't made from the guest process.
This is the plan for fixing this:
1. <This CL> Reintroduce these IO thread checks on M56.
2. https://codereview.chromium.org/2437753003: Tighten the checks for
the scenario above using ChildProcessSecurityPolicy.
3. Merge the CL from step 2 back to M55 and M54.
4. Remove these checks from M56 and tighten the checks in
ExtensionNavigationThrottle.
BUG=656752
Committed: https://crrev.com/78760ccdaf6c22e6e436ce21d9d31a6030a5634e
Cr-Commit-Position: refs/heads/master@{#426855}
Patch Set 1 #
Total comments: 9
Patch Set 2 : Comment nit #
Dependent Patchsets: Messages
Total messages: 26 (14 generated)
The CQ bit was checked by alexmos@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Description was changed from ========== Temporarily reintroduce blob/filesystem URL security checks on the IO thread. These checks were originally introduced in r419019 and r422954. This was merged back to M54 and M55. They were later moved to the UI thread (into ExtensionNavigationThrottle) in r424937 to make them compatible with PlzNavigate. However, that only exists in M56. Per linked bug, we need to tighten the security check currently in M54 and M55 for apps with a "webview" permission where the problematic blob/filesystem URL requests aren't made from the guest process. This is the plan for fixing this: 1. <this CL> Reintroduce these IO thread checks on M56. 2. <https://X> Tighten the checks for the scenario above using ChildProcessSecurityPolicy. 3. Merge the CL from step 2 back to M55 and M54. 4. Remove these checks from M56 and tighten the checks in ExtensionNavigationThrottle. BUG=656752 ========== to ========== Temporarily reintroduce blob/filesystem URL security checks on the IO thread. These checks were originally introduced in r419019 and r422954. This was merged back to M54 and M55. They were later moved to the UI thread (into ExtensionNavigationThrottle) in r424937 to make them compatible with PlzNavigate. However, that only exists in M56. Per linked bug, we need to tighten the security check currently in M54 and M55 for apps with a "webview" permission where the problematic blob/filesystem URL requests aren't made from the guest process. This is the plan for fixing this: 1. <this CL> Reintroduce these IO thread checks on M56. 2. <https://codereview.chromium.org/2437753003> Tighten the checks for the scenario above using ChildProcessSecurityPolicy. 3. Merge the CL from step 2 back to M55 and M54. 4. Remove these checks from M56 and tighten the checks in ExtensionNavigationThrottle. BUG=656752 ==========
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Description was changed from ========== Temporarily reintroduce blob/filesystem URL security checks on the IO thread. These checks were originally introduced in r419019 and r422954. This was merged back to M54 and M55. They were later moved to the UI thread (into ExtensionNavigationThrottle) in r424937 to make them compatible with PlzNavigate. However, that only exists in M56. Per linked bug, we need to tighten the security check currently in M54 and M55 for apps with a "webview" permission where the problematic blob/filesystem URL requests aren't made from the guest process. This is the plan for fixing this: 1. <this CL> Reintroduce these IO thread checks on M56. 2. <https://codereview.chromium.org/2437753003> Tighten the checks for the scenario above using ChildProcessSecurityPolicy. 3. Merge the CL from step 2 back to M55 and M54. 4. Remove these checks from M56 and tighten the checks in ExtensionNavigationThrottle. BUG=656752 ========== to ========== Temporarily reintroduce blob/filesystem URL security checks on the IO thread. These checks were originally introduced in r419019 and r422954. This was merged back to M54 and M55. They were later moved to the UI thread (into ExtensionNavigationThrottle) in r424937 to make them compatible with PlzNavigate. However, that only exists in M56. Per linked bug, we need to tighten the security check currently in M54 and M55 for apps with a "webview" permission where the problematic blob/filesystem URL requests aren't made from the guest process. This is the plan for fixing this: 1. <This CL> Reintroduce these IO thread checks on M56. 2. https://codereview.chromium.org/2437753003: Tighten the checks for the scenario above using ChildProcessSecurityPolicy. 3. Merge the CL from step 2 back to M55 and M54. 4. Remove these checks from M56 and tighten the checks in ExtensionNavigationThrottle. BUG=656752 ==========
alexmos@chromium.org changed reviewers: + nick@chromium.org
Nick, can you please take a look? The IO thread check here is the same as what was removed in https://codereview.chromium.org/2411693003, with an additional check to not block anything for PlzNavigate, to avoid breaking tests that jam@ fixed. This is the part that won't be merged. The next CL is the actual tightening of the blocking (https://codereview.chromium.org/2437753003).
lgtm https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... File chrome/browser/net/chrome_extensions_network_delegate.cc (right): https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... chrome/browser/net/chrome_extensions_network_delegate.cc:203: !content::IsBrowserSideNavigationEnabled()) { To be fully clear, my understanding is that we don't need to merge back the !content::IsBrowserSideNavigationEnabled() check to M55/M54? (i.e. we don't care about breaking plznavigate on beta/stable because it's disabled and there's no field trial). If so then this looks right.
https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... File chrome/browser/net/chrome_extensions_network_delegate.cc (right): https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... chrome/browser/net/chrome_extensions_network_delegate.cc:203: !content::IsBrowserSideNavigationEnabled()) { On 2016/10/20 17:22:32, ncarter wrote: > To be fully clear, my understanding is that we don't need to merge back the > !content::IsBrowserSideNavigationEnabled() check to M55/M54? (i.e. we don't care > about breaking plznavigate on beta/stable because it's disabled and there's no > field trial). If so then this looks right. Yes, correct, the plan is not to merge this check back.
alexmos@chromium.org changed reviewers: + sky@chromium.org
sky@, can you please stamp this for OWNERS? This was already reviewed in https://codereview.chromium.org/2345473003 and https://codereview.chromium.org/2387323002, and later moved to a different place in https://codereview.chromium.org/2411693003. I'm reintroducing this temporarily to tighten up the security check (in the followup CL, https://codereview.chromium.org/2437753003) with the intention of merging that to M55/M54. Then I'll remove this again.
Description was changed from ========== Temporarily reintroduce blob/filesystem URL security checks on the IO thread. These checks were originally introduced in r419019 and r422954. This was merged back to M54 and M55. They were later moved to the UI thread (into ExtensionNavigationThrottle) in r424937 to make them compatible with PlzNavigate. However, that only exists in M56. Per linked bug, we need to tighten the security check currently in M54 and M55 for apps with a "webview" permission where the problematic blob/filesystem URL requests aren't made from the guest process. This is the plan for fixing this: 1. <This CL> Reintroduce these IO thread checks on M56. 2. https://codereview.chromium.org/2437753003: Tighten the checks for the scenario above using ChildProcessSecurityPolicy. 3. Merge the CL from step 2 back to M55 and M54. 4. Remove these checks from M56 and tighten the checks in ExtensionNavigationThrottle. BUG=656752 ========== to ========== Temporarily reintroduce blob/filesystem URL security checks on the IO thread. These checks were originally introduced in r419019 and r422954. This was merged back to M54 and M55. They were later moved to the UI thread (into ExtensionNavigationThrottle) in r424937 to make them compatible with PlzNavigate. However, that only exists in M56. Per linked bug, we need to tighten the security check currently in M54 and M55 for apps with a "webview" permission where the problematic blob/filesystem URL requests aren't made from the guest process. This is the plan for fixing this: 1. <This CL> Reintroduce these IO thread checks on M56. 2. https://codereview.chromium.org/2437753003: Tighten the checks for the scenario above using ChildProcessSecurityPolicy. 3. Merge the CL from step 2 back to M55 and M54. 4. Remove these checks from M56 and tighten the checks in ExtensionNavigationThrottle. BUG=656752 ==========
sky@chromium.org changed reviewers: + mmenke@chromium.org - sky@chromium.org
Please prefer a more local owner when possible: sky->mmenke
https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... File chrome/browser/net/chrome_extensions_network_delegate.cc (right): https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... chrome/browser/net/chrome_extensions_network_delegate.cc:191: // M56. It is reintroduced temporarily to tighten this blocking for apps with "It is" meaning this check, not the one in the throttle? https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... chrome/browser/net/chrome_extensions_network_delegate.cc:192: // a "webview" permission on M55/54 (see https://crbug.com/656752). It will Could you give me access to the bug? I want to know what's going on here. https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... chrome/browser/net/chrome_extensions_network_delegate.cc:195: // and is disabled for that mode. Why is this check not compatible with PlzNavigate?
Thanks! https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... File chrome/browser/net/chrome_extensions_network_delegate.cc (right): https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... chrome/browser/net/chrome_extensions_network_delegate.cc:191: // M56. It is reintroduced temporarily to tighten this blocking for apps with On 2016/10/20 20:52:04, mmenke wrote: > "It is" meaning this check, not the one in the throttle? Yes. Tweaked the comment to say this explicitly. https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... chrome/browser/net/chrome_extensions_network_delegate.cc:192: // a "webview" permission on M55/54 (see https://crbug.com/656752). It will On 2016/10/20 20:52:04, mmenke wrote: > Could you give me access to the bug? I want to know what's going on here. Done. Also CC'ed you on a couple of linked bugs that you'd need for full context. https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... chrome/browser/net/chrome_extensions_network_delegate.cc:195: // and is disabled for that mode. On 2016/10/20 20:52:04, mmenke wrote: > Why is this check not compatible with PlzNavigate? See https://codereview.chromium.org/2411693003. IIRC, I think with PlzNavigate, we don't know which process is responsible for the request here, i.e., GetChildID() is -1, so we can't use it for the process_map() check.
https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... File chrome/browser/net/chrome_extensions_network_delegate.cc (right): https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... chrome/browser/net/chrome_extensions_network_delegate.cc:195: // and is disabled for that mode. On 2016/10/20 21:18:16, alexmos wrote: > On 2016/10/20 20:52:04, mmenke wrote: > > Why is this check not compatible with PlzNavigate? > > See https://codereview.chromium.org/2411693003. IIRC, I think with PlzNavigate, > we don't know which process is responsible for the request here, i.e., > GetChildID() is -1, so we can't use it for the process_map() check. Just a bit more context: when I originally introduced this blocking, it broke PlzNavigate tests (see https://crbug.com/647712). The PlzNavigate exclusion avoids breaking these tests on ToT.
On 2016/10/20 21:23:45, alexmos wrote: > https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... > File chrome/browser/net/chrome_extensions_network_delegate.cc (right): > > https://codereview.chromium.org/2435593007/diff/1/chrome/browser/net/chrome_e... > chrome/browser/net/chrome_extensions_network_delegate.cc:195: // and is disabled > for that mode. > On 2016/10/20 21:18:16, alexmos wrote: > > On 2016/10/20 20:52:04, mmenke wrote: > > > Why is this check not compatible with PlzNavigate? > > > > See https://codereview.chromium.org/2411693003. IIRC, I think with > PlzNavigate, > > we don't know which process is responsible for the request here, i.e., > > GetChildID() is -1, so we can't use it for the process_map() check. > > Just a bit more context: when I originally introduced this blocking, it broke > PlzNavigate tests (see https://crbug.com/647712). The PlzNavigate exclusion > avoids breaking these tests on ToT. Thanks, LGTM
The CQ bit was checked by alexmos@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from nick@chromium.org Link to the patchset: https://codereview.chromium.org/2435593007/#ps20001 (title: "Comment nit")
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
Message was sent while issue was closed.
Description was changed from ========== Temporarily reintroduce blob/filesystem URL security checks on the IO thread. These checks were originally introduced in r419019 and r422954. This was merged back to M54 and M55. They were later moved to the UI thread (into ExtensionNavigationThrottle) in r424937 to make them compatible with PlzNavigate. However, that only exists in M56. Per linked bug, we need to tighten the security check currently in M54 and M55 for apps with a "webview" permission where the problematic blob/filesystem URL requests aren't made from the guest process. This is the plan for fixing this: 1. <This CL> Reintroduce these IO thread checks on M56. 2. https://codereview.chromium.org/2437753003: Tighten the checks for the scenario above using ChildProcessSecurityPolicy. 3. Merge the CL from step 2 back to M55 and M54. 4. Remove these checks from M56 and tighten the checks in ExtensionNavigationThrottle. BUG=656752 ========== to ========== Temporarily reintroduce blob/filesystem URL security checks on the IO thread. These checks were originally introduced in r419019 and r422954. This was merged back to M54 and M55. They were later moved to the UI thread (into ExtensionNavigationThrottle) in r424937 to make them compatible with PlzNavigate. However, that only exists in M56. Per linked bug, we need to tighten the security check currently in M54 and M55 for apps with a "webview" permission where the problematic blob/filesystem URL requests aren't made from the guest process. This is the plan for fixing this: 1. <This CL> Reintroduce these IO thread checks on M56. 2. https://codereview.chromium.org/2437753003: Tighten the checks for the scenario above using ChildProcessSecurityPolicy. 3. Merge the CL from step 2 back to M55 and M54. 4. Remove these checks from M56 and tighten the checks in ExtensionNavigationThrottle. BUG=656752 ==========
Message was sent while issue was closed.
Committed patchset #2 (id:20001)
Message was sent while issue was closed.
Description was changed from ========== Temporarily reintroduce blob/filesystem URL security checks on the IO thread. These checks were originally introduced in r419019 and r422954. This was merged back to M54 and M55. They were later moved to the UI thread (into ExtensionNavigationThrottle) in r424937 to make them compatible with PlzNavigate. However, that only exists in M56. Per linked bug, we need to tighten the security check currently in M54 and M55 for apps with a "webview" permission where the problematic blob/filesystem URL requests aren't made from the guest process. This is the plan for fixing this: 1. <This CL> Reintroduce these IO thread checks on M56. 2. https://codereview.chromium.org/2437753003: Tighten the checks for the scenario above using ChildProcessSecurityPolicy. 3. Merge the CL from step 2 back to M55 and M54. 4. Remove these checks from M56 and tighten the checks in ExtensionNavigationThrottle. BUG=656752 ========== to ========== Temporarily reintroduce blob/filesystem URL security checks on the IO thread. These checks were originally introduced in r419019 and r422954. This was merged back to M54 and M55. They were later moved to the UI thread (into ExtensionNavigationThrottle) in r424937 to make them compatible with PlzNavigate. However, that only exists in M56. Per linked bug, we need to tighten the security check currently in M54 and M55 for apps with a "webview" permission where the problematic blob/filesystem URL requests aren't made from the guest process. This is the plan for fixing this: 1. <This CL> Reintroduce these IO thread checks on M56. 2. https://codereview.chromium.org/2437753003: Tighten the checks for the scenario above using ChildProcessSecurityPolicy. 3. Merge the CL from step 2 back to M55 and M54. 4. Remove these checks from M56 and tighten the checks in ExtensionNavigationThrottle. BUG=656752 Committed: https://crrev.com/78760ccdaf6c22e6e436ce21d9d31a6030a5634e Cr-Commit-Position: refs/heads/master@{#426855} ==========
Message was sent while issue was closed.
Patchset 2 (id:??) landed as https://crrev.com/78760ccdaf6c22e6e436ce21d9d31a6030a5634e Cr-Commit-Position: refs/heads/master@{#426855} |
|||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
