Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
2913733002:
Add a browser test for access control for file: scheme (Closed)
DescriptionAdd a browser test for access control for file: scheme
Access to files via file: scheme is controled in
ChromeNetworkDelegate::OnCanAccessFile(). The browser
test is to ensure that this function is actually used
to control the access.
BUG=677933
TEST=the test passes on bots
NOPRESUBMIT=true
Review-Url: https://codereview.chromium.org/2913733002
Cr-Commit-Position: refs/heads/master@{#477915}
Committed: https://chromium.googlesource.com/chromium/src/+/d18e61ab5e01b85ff9160c4ab143cba6cd64d7be
Patch Set 1 #Patch Set 2 : Rework the patch per feedback #
Total comments: 22
Patch Set 3 : just rebase #Patch Set 4 : Address comments #
Total comments: 6
Patch Set 5 : Address comments #Patch Set 6 : just rebase #
Messages
Total messages: 45 (26 generated)
The CQ bit was checked by satorux@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...
satorux@chromium.org changed reviewers: + mmenke@chromium.org
PTAL NOPRESUBMIT=true can be removed if https://codereview.chromium.org/2911153002/ lands before this patch. :)
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
On 2017/05/30 06:43:02, satorux1 wrote: > PTAL > > NOPRESUBMIT=true can be removed if https://codereview.chromium.org/2911153002/ > lands before this patch. :) I'm not sure about this approach - seems like depending on a command line flag basically makes it meaningless (Particularly as the reason I asked if we could have a test is because ChromeNetworkDelegate is unlikely to survive the switch to a network service).
On 2017/05/30 20:10:18, mmenke wrote: > On 2017/05/30 06:43:02, satorux1 wrote: > > PTAL > > > > NOPRESUBMIT=true can be removed if https://codereview.chromium.org/2911153002/ > > lands before this patch. :) > > I'm not sure about this approach - seems like depending on a command line flag > basically makes it meaningless (Particularly as the reason I asked if we could > have a test is because ChromeNetworkDelegate is unlikely to survive the switch > to a network service). What I was really thinking was a ChromeOS-only test that tested the behavior of the production code in case that really should be denied. Can file URLs access the temp/ directory on ChromeOS, for instance?
> > I'm not sure about this approach - seems like depending on a command line
flag
> > basically makes it meaningless (Particularly as the reason I asked if we
could
> > have a test is because ChromeNetworkDelegate is unlikely to survive the
switch
> > to a network service).
>
> What I was really thinking was a ChromeOS-only test that tested the behavior
of
> the production code in case that really should be denied. Can file URLs
access
> the temp/ directory on ChromeOS, for instance?
Unfortunately, enabling the production behavior of Chrome OS requires some flag
tweaking in ChromeNetworkDelegate. Below is the code that implements the
for-testing behavior, that we need to cancel to enable the production behavior.
bool ChromeNetworkDelegate::OnCanAccessFile(...) {
#if defined(OS_CHROMEOS)
// browser_tests and interactive_ui_tests rely on the ability to open any
// files via file: scheme.
if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType))
return true;
#endif
If I just get rid of this code and run tests, a large number of tests in
browser_tests and interactive_ui_tests fail because these rely on the ability to
open any files via file: scheme.
I thought about deleting --test-type flag at runtime from a browser test, but
there seems to be no way to remove a flag per command_line.h. We could pass
something like --production-behavior and check the presence in the above
mentioned conditional, but it's not so different from what my patch implemented.
I think my patch is valuable to some extent since it can ensure that
ChromeNetworkDelegate::OnCanAccessFile() is in fact used to control access to
files via file: scheme. If someone is going to replace ChromeNetworkDelegate
with somethign else, I hope they'll port this test to the new thing.
On 2017/05/31 00:32:33, satorux1 wrote:
> > > I'm not sure about this approach - seems like depending on a command line
> flag
> > > basically makes it meaningless (Particularly as the reason I asked if we
> could
> > > have a test is because ChromeNetworkDelegate is unlikely to survive the
> switch
> > > to a network service).
> >
> > What I was really thinking was a ChromeOS-only test that tested the behavior
> of
> > the production code in case that really should be denied. Can file URLs
> access
> > the temp/ directory on ChromeOS, for instance?
>
> Unfortunately, enabling the production behavior of Chrome OS requires some
flag
> tweaking in ChromeNetworkDelegate. Below is the code that implements the
> for-testing behavior, that we need to cancel to enable the production
behavior.
>
> bool ChromeNetworkDelegate::OnCanAccessFile(...) {
> #if defined(OS_CHROMEOS)
> // browser_tests and interactive_ui_tests rely on the ability to open any
> // files via file: scheme.
> if (base::CommandLine::ForCurrentProcess()->HasSwitch(switches::kTestType))
> return true;
> #endif
>
> If I just get rid of this code and run tests, a large number of tests in
> browser_tests and interactive_ui_tests fail because these rely on the ability
to
> open any files via file: scheme.
>
> I thought about deleting --test-type flag at runtime from a browser test, but
> there seems to be no way to remove a flag per command_line.h. We could pass
> something like --production-behavior and check the presence in the above
> mentioned conditional, but it's not so different from what my patch
implemented.
>
>
> I think my patch is valuable to some extent since it can ensure that
> ChromeNetworkDelegate::OnCanAccessFile() is in fact used to control access to
> files via file: scheme. If someone is going to replace ChromeNetworkDelegate
> with somethign else, I hope they'll port this test to the new thing.
I just don't think this one test gets us enough to be worth adding a production
command line flag for, we already have way too many of them, and this test seems
to add too little value. I'm also unaware of any command line flag that isn't a
global constant of some sort.
And the whole reason I suggested an integration test was so that we'd have
something that wouldn't go stale with the new code. The team working on network
servicification is largely relying on existing browser tests to catch stuff, and
I'm skeptical about their willingness to write new tests, having already had to
have one fight on the subject.
> > I think my patch is valuable to some extent since it can ensure that > > ChromeNetworkDelegate::OnCanAccessFile() is in fact used to control access to > > files via file: scheme. If someone is going to replace ChromeNetworkDelegate > > with somethign else, I hope they'll port this test to the new thing. > > I just don't think this one test gets us enough to be worth adding a production > command line flag for, we already have way too many of them, and this test seems > to add too little value. I'm also unaware of any command line flag that isn't a > global constant of some sort. > > And the whole reason I suggested an integration test was so that we'd have > something that wouldn't go stale with the new code. The team working on network > servicification is largely relying on existing browser tests to catch stuff, and > I'm skeptical about their willingness to write new tests, having already had to > have one fight on the subject. I agree that this patch isn't ideal but, I don't have a better idea yet. As mentioned, there is a conditional in ChromeNetworkDelegate::OnCanAccessFile() that checks --test-type that triggers the for-testing behavior. To enable the production behavior from a browser test, we have to somehow cancel this conditional. Do you have any ideas?
On 2017/05/31 22:03:19, satorux1 wrote: > > > I think my patch is valuable to some extent since it can ensure that > > > ChromeNetworkDelegate::OnCanAccessFile() is in fact used to control access > to > > > files via file: scheme. If someone is going to replace ChromeNetworkDelegate > > > with somethign else, I hope they'll port this test to the new thing. > > > > I just don't think this one test gets us enough to be worth adding a > production > > command line flag for, we already have way too many of them, and this test > seems > > to add too little value. I'm also unaware of any command line flag that isn't > a > > global constant of some sort. > > > > And the whole reason I suggested an integration test was so that we'd have > > something that wouldn't go stale with the new code. The team working on > network > > servicification is largely relying on existing browser tests to catch stuff, > and > > I'm skeptical about their willingness to write new tests, having already had > to > > have one fight on the subject. > > I agree that this patch isn't ideal but, I don't have a better idea yet. As > mentioned, there is a conditional in ChromeNetworkDelegate::OnCanAccessFile() > that checks --test-type that triggers the for-testing behavior. To enable the > production behavior from a browser test, we have to somehow cancel this > conditional. Do you have any ideas? Sorry for the delay - took yesterday off, and been a bit low on sleep this week (And I find reviews require the most brainpower to do well, hence avoiding them when too tired). One option, that I'd prefer, would be to add a global bool rather than using the command line flag, and either have the test fixture or individual tests set it to true on ChromeOS, and have this test either not set it or set it to false. Another option would be to modify what ChromeOS tests rely on - I'm not familiar with what they actually need, but they could, for instance, use the test server instead of file URLs.
The CQ bit was checked by satorux@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...
> Sorry for the delay - took yesterday off, and been a bit low on sleep this week > (And I find reviews require the most brainpower to do well, hence avoiding them > when too tired). No worries about it at all! > One option, that I'd prefer, would be to add a global bool > rather than using the command line flag, and either have the test fixture or > individual tests set it to true on ChromeOS, and have this test either not set > it or set it to false. I think this is a great idea. I've updated the patch per this suggestion. Please take a look. > Another option would be to modify what ChromeOS tests > rely on - I'm not familiar with what they actually need, but they could, for > instance, use the test server instead of file URLs. The tests, that rely on the ability to open any file via file: scheme, aren't Chrome OS specific. There are many browser tests that open files via file: scheme [1], that don't pass with Chrome OS's production behaviors. [1] https://cs.chromium.org/search/?q=net::FilePathToFileURL+file:browsertest%5C.... in_reply_to: 5766466041282560 send_mail: 1 subject: Add a browser test for access control for file: scheme
Patchset #2 (id:20001) has been deleted
The CQ bit was checked by satorux@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks really good, mostly just nits, though two more significant suggestions (Those are why I'm not signing off now) https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate_browsertest.cc (right): https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: new files shouldn't use the "(c)" https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:6: #include "base/command_line.h" Neither of these are used. https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:8: #include "base/strings/utf_string_conversions.h" Not used. https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:29: DISALLOW_COPY_AND_ASSIGN(ChromeNetworkDelegateBrowserTest); include base/macros.h https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:33: // ERR_ACCESS_DENIED. Also add a test with a symbolic like that is accessible to a location that isn't? That was the entire motivation for asking for this test, and what I'm most concerned about regressing, when we switch to using a network service (When we'll lose existing unit tests, and file requests may not even go through ChromeNetworkDelegate, or URLRequest, if we decide on using a sandbox that restricts the network process's file I/O capabilities). https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:37: ChromeNetworkDelegate::EnableAccessToAllFilesForTesting(false); It's possible some other module has already started a URLRequest at this point. So to avoid any races between setting the bool and other URLRequests made by chrome (Which would be benign in terms of the test passing, but may still be caught by TSAN), should move this into the test fixture: void SetUp() override { InProcessBrowserTest::SetUp(); // Must be done after the above call, which will set it to true, but also before the browser is set up, to avoid any races) ChromeNetworkDelegate::EnableAccessToAllFilesForTesting(false); } https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:42: const GURL url = net::FilePathToFileURL(test_file); nit: convention is not to use const, except for build time constants and class members (No idea why, it's just prevailing style). https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:46: browser(), net::ErrorToShortString(net::ERR_ACCESS_DENIED))); Rather than sharing errorpage_test_util::IsDisplayingText, could we just use a NavigationObserver instead? Could even been up TestNavigationObserver to grab the net error code from the NavigationHandle (GetNetErrorCode()) in OnDidFinishNavigation, so you could grab it there. https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/erro... File chrome/browser/net/errorpage_test_util.cc (right): https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/erro... chrome/browser/net/errorpage_test_util.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. --(c) https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/erro... File chrome/browser/net/errorpage_test_util.h (right): https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/erro... chrome/browser/net/errorpage_test_util.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. nit: -(c) https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/erro... chrome/browser/net/errorpage_test_util.h:17: // space handling may be weird. It actually doesn't use "innerText" (Know you just copied this). Maybe something like "Searches for first node containing |text|, and if it finds one, searches through all ancestors seeing if any of them is of class "hidden". Since it relies on the hidden class used by network error pages, not suitable for general use."
PTAL https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... File chrome/browser/net/chrome_network_delegate_browsertest.cc (right): https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/06/05 18:31:33, mmenke wrote: > nit: new files shouldn't use the "(c)" Good catch! Done. https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:6: #include "base/command_line.h" On 2017/06/05 18:31:33, mmenke wrote: > Neither of these are used. Good catch again. These are leftover from a previous attempt. Removed! https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:8: #include "base/strings/utf_string_conversions.h" On 2017/06/05 18:31:33, mmenke wrote: > Not used. Done. https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:29: DISALLOW_COPY_AND_ASSIGN(ChromeNetworkDelegateBrowserTest); On 2017/06/05 18:31:33, mmenke wrote: > include base/macros.h Done. https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:33: // ERR_ACCESS_DENIED. On 2017/06/05 18:31:33, mmenke wrote: > Also add a test with a symbolic like that is accessible to a location that > isn't? That was the entire motivation for asking for this test, and what I'm > most concerned about regressing, when we switch to using a network service (When > we'll lose existing unit tests, and file requests may not even go through > ChromeNetworkDelegate, or URLRequest, if we decide on using a sandbox that > restricts the network process's file I/O capabilities). Good point. Added a test for that. Also created a separate CL that is to replace "/tmp" with a call to PathService::Get() for writing the test with a symlink: https://codereview.chromium.org/2926543002/ https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:37: ChromeNetworkDelegate::EnableAccessToAllFilesForTesting(false); On 2017/06/05 18:31:33, mmenke wrote: > It's possible some other module has already started a URLRequest at this point. > So to avoid any races between setting the bool and other URLRequests made by > chrome (Which would be benign in terms of the test passing, but may still be > caught by TSAN), should move this into the test fixture: > > void SetUp() override { > InProcessBrowserTest::SetUp(); > // Must be done after the above call, which will set it to true, but also > before the browser is set up, to avoid any races) > ChromeNetworkDelegate::EnableAccessToAllFilesForTesting(false); > } Good idea! However, I was surprised that your code didn't work because the test function is called from InProcessBrowserTest::SetUp(). :) Hopefully this is documented in browser_test_base.h and the suggested solution is to use SetUpInProcessBrowserTestFixture(). :) https://cs.chromium.org/chromium/src/content/public/test/browser_test_base.h?... https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:42: const GURL url = net::FilePathToFileURL(test_file); On 2017/06/05 18:31:33, mmenke wrote: > nit: convention is not to use const, except for build time constants and class > members (No idea why, it's just prevailing style). Done. https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/chro... chrome/browser/net/chrome_network_delegate_browsertest.cc:46: browser(), net::ErrorToShortString(net::ERR_ACCESS_DENIED))); On 2017/06/05 18:31:33, mmenke wrote: > Rather than sharing errorpage_test_util::IsDisplayingText, could we just use a > NavigationObserver instead? Could even been up TestNavigationObserver to grab > the net error code from the NavigationHandle (GetNetErrorCode()) in > OnDidFinishNavigation, so you could grab it there. This was a brilliant idea. Done! https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/erro... File chrome/browser/net/errorpage_test_util.cc (right): https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/erro... chrome/browser/net/errorpage_test_util.cc:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/06/05 18:31:34, mmenke wrote: > --(c) done - but this file is gone. https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/erro... File chrome/browser/net/errorpage_test_util.h (right): https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/erro... chrome/browser/net/errorpage_test_util.h:1: // Copyright (c) 2017 The Chromium Authors. All rights reserved. On 2017/06/05 18:31:34, mmenke wrote: > nit: -(c) done - but this file is gone. https://codereview.chromium.org/2913733002/diff/40001/chrome/browser/net/erro... chrome/browser/net/errorpage_test_util.h:17: // space handling may be weird. On 2017/06/05 18:31:34, mmenke wrote: > It actually doesn't use "innerText" (Know you just copied this). > > Maybe something like "Searches for first node containing |text|, and if it finds > one, searches through all ancestors seeing if any of them is of class "hidden". > Since it relies on the hidden class used by network error pages, not suitable > for general use." Updated the comment in errorpage_browsertest.cc :)
The CQ bit was checked by satorux@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
Looks great! LGTM! I'll add owners for the test/ files, try and save you a day of roundtrips.
mmenke@chromium.org changed reviewers: + sky@chromium.org
[+sky]: Please review chrome/test/base/in_process_browser_test.cc and content/public/test/ changes.
https://codereview.chromium.org/2913733002/diff/80001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/2913733002/diff/80001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:259: // browser_tests and interactive_ui_tests rely on the ability to open any How come you are only enabling this on chromeos? https://codereview.chromium.org/2913733002/diff/80001/content/public/test/tes... File content/public/test/test_navigation_observer.h (right): https://codereview.chromium.org/2913733002/diff/80001/content/public/test/tes... content/public/test/test_navigation_observer.h:90: net::Error last_net_error_code_; Doesn't this need to be initialized?
mmenke@google.com changed reviewers: + mmenke@google.com
https://codereview.chromium.org/2913733002/diff/80001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/2913733002/diff/80001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:259: // browser_tests and interactive_ui_tests rely on the ability to open any On 2017/06/06 17:18:26, sky wrote: > How come you are only enabling this on chromeos? Only on ChromeOS (And Android) do we restrict what directories can be accessed by file URLs. On other platforms, users can access any file themselves, anyways, and we rely on the OS's privilege model. See ChromeNetworkDelegate::OnCanAccessFile.
https://codereview.chromium.org/2913733002/diff/80001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/2913733002/diff/80001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:259: // browser_tests and interactive_ui_tests rely on the ability to open any On 2017/06/06 17:23:41, mmenke_google.com wrote: > On 2017/06/06 17:18:26, sky wrote: > > How come you are only enabling this on chromeos? > > Only on ChromeOS (And Android) do we restrict what directories can be accessed > by file URLs. On other platforms, users can access any file themselves, > anyways, and we rely on the OS's privilege model. > > See ChromeNetworkDelegate::OnCanAccessFile. Please update the comment to indicate this then.
https://codereview.chromium.org/2913733002/diff/80001/chrome/test/base/in_pro... File chrome/test/base/in_process_browser_test.cc (right): https://codereview.chromium.org/2913733002/diff/80001/chrome/test/base/in_pro... chrome/test/base/in_process_browser_test.cc:259: // browser_tests and interactive_ui_tests rely on the ability to open any On 2017/06/06 20:45:36, sky wrote: > On 2017/06/06 17:23:41, http://mmenke_google.com wrote: > > On 2017/06/06 17:18:26, sky wrote: > > > How come you are only enabling this on chromeos? > > > > Only on ChromeOS (And Android) do we restrict what directories can be accessed > > by file URLs. On other platforms, users can access any file themselves, > > anyways, and we rely on the OS's privilege model. > > > > See ChromeNetworkDelegate::OnCanAccessFile. > > Please update the comment to indicate this then. Done. https://codereview.chromium.org/2913733002/diff/80001/content/public/test/tes... File content/public/test/test_navigation_observer.h (right): https://codereview.chromium.org/2913733002/diff/80001/content/public/test/tes... content/public/test/test_navigation_observer.h:90: net::Error last_net_error_code_; On 2017/06/06 17:18:26, sky wrote: > Doesn't this need to be initialized? Good catch. Done!
The CQ bit was checked by satorux@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
LGTM
The CQ bit was checked by satorux@chromium.org
The patchset sent to the CQ was uploaded after l-g-t-m from mmenke@chromium.org, sky@chromium.org Link to the patchset: https://codereview.chromium.org/2913733002/#ps120001 (title: "just rebase")
CQ is trying da patch. Follow status at: https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...
CQ is committing da patch.
Bot data: {"patchset_id": 120001, "attempt_start_ts": 1496897834618150,
"parent_rev": "9a4b5301058b391e2879792e3e5cf37a2a4665bb", "commit_rev":
"d18e61ab5e01b85ff9160c4ab143cba6cd64d7be"}
Message was sent while issue was closed.
Description was changed from ========== Add a browser test for access control for file: scheme Access to files via file: scheme is controled in ChromeNetworkDelegate::OnCanAccessFile(). The browser test is to ensure that this function is actually used to control the access. BUG=677933 TEST=the test passes on bots NOPRESUBMIT=true ========== to ========== Add a browser test for access control for file: scheme Access to files via file: scheme is controled in ChromeNetworkDelegate::OnCanAccessFile(). The browser test is to ensure that this function is actually used to control the access. BUG=677933 TEST=the test passes on bots NOPRESUBMIT=true Review-Url: https://codereview.chromium.org/2913733002 Cr-Commit-Position: refs/heads/master@{#477915} Committed: https://chromium.googlesource.com/chromium/src/+/d18e61ab5e01b85ff9160c4ab143... ==========
Message was sent while issue was closed.
Committed patchset #6 (id:120001) as https://chromium.googlesource.com/chromium/src/+/d18e61ab5e01b85ff9160c4ab143... |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
