chromeos: Check both original and absolute paths for file: scheme

chromeos: Check both original and absolute paths for file: scheme

Previously, paths not in the whitelist were accessible through
symbolic links via file: scheme. This patch solves this
problem by checking both original and absolute paths (i.e.
symbolic link is resolved with the latter).

It is a known issue that this approach has a race between
time to check the resolved path and time to read the path.

BUG=677933
TEST=confirm that paths not in the whitelist cannot be accessed via symbolic links on Chrome OS
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2786583002
Cr-Commit-Position: refs/heads/master@{#475299}
Committed: https://chromium.googlesource.com/chromium/src/+/ddac044c41e8b7a223c403e4574f5d44dff5e50e

[satorux1, 2017-03-29 05:33:05]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-29 05:33:36]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-03-30 01:59:33]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 02:00:27]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 02:30:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 02:30:05]

Dry run: Try jobs failed on following builders:
  linux_chromium_chromeos_ozone_rel_ng on master.tryserver.chromium.linux
(JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)
  win_chromium_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_rel_...)

[satorux1, 2017-03-30 03:44:37]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 03:45:11]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-30 04:19:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 04:19:29]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[satorux1, 2017-03-30 06:45:33]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-30 06:45:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-03-30 08:04:11]

Patchset #1 (id:1) has been deleted

[satorux1, 2017-03-30 08:04:29]

Patchset #3 (id:60001) has been deleted

[satorux1, 2017-03-30 08:04:43]

Patchset #4 (id:100001) has been deleted

[commit-bot: I haz the power, 2017-03-30 08:50:35]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-30 08:50:37]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[satorux1, 2017-03-31 01:53:39]

Patchset #5 (id:140001) has been deleted

[satorux1, 2017-03-31 03:58:00]

Description was changed from

==========
Use absolute path to check if path is accessible via file: schema

Previously, paths not in the whitelist were accessibly through
symbolic links via file: schema. This patch solves this
problem by resolving symblic links before checking.

It is a known issue that this approach has a race between
time to check the resolved path and time to read the path.

BUG=677933
TEST=confirm that paths not in the whitelist cannot be accessed via symbolic
links
==========

to

==========
chromeos: Use absolute path to check if path is accessible via file://

Previously, paths not in the whitelist were accessibly through
symbolic links via file: schema. This patch solves this
problem by resolving symblic links before checking.

It is a known issue that this approach has a race between
time to check the resolved path and time to read the path.

BUG=677933
TEST=confirm that paths not in the whitelist cannot be accessed via symbolic
links on Chrome OS
==========

[satorux1, 2017-03-31 04:01:29]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 04:01:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 04:13:03]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 04:13:04]

Dry run: Try jobs failed on following builders:
  android_cronet on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_cron...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[satorux1, 2017-03-31 04:22:01]

Description was changed from

==========
chromeos: Use absolute path to check if path is accessible via file://

Previously, paths not in the whitelist were accessibly through
symbolic links via file: schema. This patch solves this
problem by resolving symblic links before checking.

It is a known issue that this approach has a race between
time to check the resolved path and time to read the path.

BUG=677933
TEST=confirm that paths not in the whitelist cannot be accessed via symbolic
links on Chrome OS
==========

to

==========
chromeos: Use absolute path to check if path is accessible via file://

Previously, paths not in the whitelist were accessibly through
symbolic links via file: schema. This patch solves this
problem by resolving symblic links before checking.

It is a known issue that this approach has a race between
time to check the resolved path and time to read the path.

BUG=677933
TEST=confirm that paths not in the whitelist cannot be accessed via symbolic
links on Chrome OS
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[satorux1, 2017-03-31 04:29:06]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 04:29:34]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-03-31 04:37:31]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[satorux1, 2017-03-31 04:37:34]

Patchset #5 (id:160001) has been deleted

[satorux1, 2017-03-31 04:37:51]

Patchset #5 (id:180001) has been deleted

[commit-bot: I haz the power, 2017-03-31 04:37:56]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-03-31 04:48:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 04:48:31]

Dry run: Try jobs failed on following builders:
  ios-device-xcode-clang on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device-xcode-...)

[satorux1, 2017-03-31 04:54:28]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 04:54:45]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-03-31 05:42:34]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 05:42:52]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-03-31 05:45:53]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 05:46:22]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-03-31 05:49:27]

Patchset #6 (id:220001) has been deleted

[satorux1, 2017-03-31 05:49:57]

Patchset #5 (id:200001) has been deleted

[satorux1, 2017-03-31 05:50:09]

Patchset #5 (id:240001) has been deleted

[commit-bot: I haz the power, 2017-03-31 05:56:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 05:56:55]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)
  ios-device on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-device/builds...)

[satorux1, 2017-03-31 07:53:10]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-03-31 07:53:24]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-03-31 07:59:10]

Patchset #5 (id:260001) has been deleted

[commit-bot: I haz the power, 2017-03-31 09:17:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-03-31 09:17:55]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[satorux1, 2017-04-14 07:57:22]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-14 07:57:32]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-14 09:04:43]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-14 09:04:44]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[satorux1, 2017-04-17 05:18:21]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-17 05:18:28]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-17 06:42:19]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-17 06:42:20]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mmenke, 2017-04-18 17:24:44]

mmenke@chromium.org changed reviewers:
+ mmenke@chromium.org

[mmenke, 2017-04-18 17:24:46]

Sorry!  I wrote some comments last week, but forgot to publish them (Maybe I
wasn't finished?  Can't remember).

Feel free to ping people who are unresponsive.

https://codereview.chromium.org/2786583002/diff/320001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:471: const base::FilePath& path =
original_path;
These should be reviewed by someone more familiar with security on both
platforms than I am.

That having been said...Can we, both by default and on ChromeOS, check both
paths, and just make Android the exception?  We should default to more limited
permissions, unless we really need more permissive ones.

https://codereview.chromium.org/2786583002/diff/320001/extensions/shell/brows...
File extensions/shell/browser/shell_network_delegate.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/extensions/shell/brows...
extensions/shell/browser/shell_network_delegate.cc:116: return true;
Why change behavior here?

https://codereview.chromium.org/2786583002/diff/320001/net/base/network_deleg...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/2786583002/diff/320001/net/base/network_deleg...
net/base/network_delegate.h:274: // is allowed.
subclass of what?  (Also, subclass is one word)

https://codereview.chromium.org/2786583002/diff/320001/net/base/network_deleg...
File net/base/network_delegate_impl.h (right):

https://codereview.chromium.org/2786583002/diff/320001/net/base/network_deleg...
net/base/network_delegate_impl.h:188: // is allowed.
same comment as in other file...  Or better, just remove all comments from this
file, as they're redundant with the other one.  If you prefer not to do that in
this CL, that's fine.

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/file_p...
File net/url_request/file_protocol_handler.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/file_p...
net/url_request/file_protocol_handler.cc:31: }
This should probably also be moved into the job classes

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
File net/url_request/url_request_file_dir_job.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.cc:48: weak_factory_.GetWeakPtr()));
I assume there's no security concern with this extra call that accesses the file
system in cases where we'd have failed before any disk access before?

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.cc:121: }
What's the motivation for this change?  I'm not opposed, just want to know if
there's an actual security concern here.

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.cc:157:
URLRequestStatus(URLRequestStatus::FAILED, ERR_ACCESS_DENIED));
URLRequestStatus::FromError(ERR_ACCESS_DENIED) is now preferred.

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
File net/url_request/url_request_file_job.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
net/url_request/url_request_file_job.cc:77: base::Owned(meta_info)));
Again, I assume there's no security/privacy issue with making this call when
we'd have just failed before accessing the disk before.

[satorux1, 2017-04-19 06:27:58]

Matt, I was planning to polish before sending out for a code review, but thank
you for proactively reviewing the patch!


As shown in Patch Set 1, this patch was much smaller originally, but got much
bigger to keep the current behavior for Clank (not resolving symbolic links).

I was thinking about revising the patch to something like:

1) As part of the boot sequence of Clank, get absolute paths for /sdcard and
/mnt/sdcard.
2) Pass the absolute paths to ChromeNetworkDelegate's constructor

This way, Clank will also be able to use absolute paths for checking, and this
patch will be back to a small one.

Does this approach make sense to you?

[satorux1, 2017-04-19 07:21:59]

Description was changed from

==========
chromeos: Use absolute path to check if path is accessible via file://

Previously, paths not in the whitelist were accessibly through
symbolic links via file: schema. This patch solves this
problem by resolving symblic links before checking.

It is a known issue that this approach has a race between
time to check the resolved path and time to read the path.

BUG=677933
TEST=confirm that paths not in the whitelist cannot be accessed via symbolic
links on Chrome OS
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
chromeos: Use absolute path to check if path is accessible via file://

Previously, paths not in the whitelist were accessibly through
symbolic links via file: schema. This patch solves this
problem by resolving symblic links before checking.

It is a known issue that this approach has a race between
time to check the resolved path and time to read the path.

BUG=677933
TEST=confirm that paths not in the whitelist cannot be accessed via symbolic
links on Chrome OS
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[satorux1, 2017-04-19 07:22:23]

https://codereview.chromium.org/2786583002/diff/320001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:471: const base::FilePath& path =
original_path;
Thanks. I'll ask jorgelo@ for a review once the patch is ready for a real code
review. :)

https://codereview.chromium.org/2786583002/diff/320001/extensions/shell/brows...
File extensions/shell/browser/shell_network_delegate.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/extensions/shell/brows...
extensions/shell/browser/shell_network_delegate.cc:116: return true;
On 2017/04/18 17:24:45, mmenke wrote:
> Why change behavior here?

Without this, extensions tests failed, because access to some test files got
rejected by the check in URLRequestFileJob. I need to double-check but it seemed
that the code here is user for testing purpose.

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
File net/url_request/url_request_file_dir_job.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.cc:48: weak_factory_.GetWeakPtr()));
On 2017/04/18 17:24:46, mmenke wrote:
> I assume there's no security concern with this extra call that accesses the
file
> system in cases where we'd have failed before any disk access before?

Sorry I couldn't get your point. Could you elaborate a bit?

As for concerns, it is a known issue that this approach has a race between time
to check the path and time to read the path. eroman@ shared some comments in the
bug.

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.cc:121: }
On 2017/04/18 17:24:46, mmenke wrote:
> What's the motivation for this change?  I'm not opposed, just want to know if
> there's an actual security concern here.

I wanted to exclude inaccessible files. Otherwise, the user is able to click on
them, but the contents won't be shown.

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.cc:157:
URLRequestStatus(URLRequestStatus::FAILED, ERR_ACCESS_DENIED));
On 2017/04/18 17:24:46, mmenke wrote:
> URLRequestStatus::FromError(ERR_ACCESS_DENIED) is now preferred.

Thanks. Fixed locally.

[mmenke, 2017-04-19 15:18:35]

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
File net/url_request/url_request_file_dir_job.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.cc:48: weak_factory_.GetWeakPtr()));
On 2017/04/19 07:22:22, satorux1 wrote:
> On 2017/04/18 17:24:46, mmenke wrote:
> > I assume there's no security concern with this extra call that accesses the
> file
> > system in cases where we'd have failed before any disk access before?
> 
> Sorry I couldn't get your point. Could you elaborate a bit?
> 
> As for concerns, it is a known issue that this approach has a race between
time
> to check the path and time to read the path. eroman@ shared some comments in
the
> bug.

Before, we didn't even access blacklisted files.  Now we always do.

1) This potentially allows sniffing for their existence, if it takes longer to
access a file that does exist over one that doesn't.
2) We're potentially modifying last accessed time on windows.
3) If there's any daemon monitoring file access (Like AV programs), we're
triggering it as well.

I think 1)'s the only possible concern here.  I don't know the threat model
that's motivating denying access to files, but this potentially makes file names
sniffable, at least.

[satorux1, 2017-04-21 00:59:47]

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
File net/url_request/url_request_file_dir_job.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.cc:48: weak_factory_.GetWeakPtr()));
On 2017/04/19 15:18:35, mmenke wrote:
> On 2017/04/19 07:22:22, satorux1 wrote:
> > On 2017/04/18 17:24:46, mmenke wrote:
> > > I assume there's no security concern with this extra call that accesses
the
> > file
> > > system in cases where we'd have failed before any disk access before?
> > 
> > Sorry I couldn't get your point. Could you elaborate a bit?
> > 
> > As for concerns, it is a known issue that this approach has a race between
> time
> > to check the path and time to read the path. eroman@ shared some comments in
> the
> > bug.
> 
> Before, we didn't even access blacklisted files.  Now we always do.
> 
> 1) This potentially allows sniffing for their existence, if it takes longer to
> access a file that does exist over one that doesn't.
> 2) We're potentially modifying last accessed time on windows.
> 3) If there's any daemon monitoring file access (Like AV programs), we're
> triggering it as well.
> 
> I think 1)'s the only possible concern here.  I don't know the threat model
> that's motivating denying access to files, but this potentially makes file
names
> sniffable, at least.

Thank you for explaining this to me very clearly! Motivation on Chrome OS is not
to expose files not intended to be accessible, via file: schema + symlinks. I'll
bring this point to the security team, when I ask them to do a code review.

[satorux1, 2017-04-21 01:04:01]

> As shown in Patch Set 1, this patch was much smaller originally, but got much
> bigger to keep the current behavior for Clank (not resolving symbolic links).
> 
> I was thinking about revising the patch to something like:
> 
> 1) As part of the boot sequence of Clank, get absolute paths for /sdcard and
> /mnt/sdcard.
> 2) Pass the absolute paths to ChromeNetworkDelegate's constructor
> 
> This way, Clank will also be able to use absolute paths for checking, and this
> patch will be back to a small one.
> 
> Does this approach make sense to you?

mmenke: could you also give me a comment on this?

[satorux1, 2017-04-21 06:27:45]

On 2017/04/21 01:04:01, satorux1 wrote:
> > As shown in Patch Set 1, this patch was much smaller originally, but got
much
> > bigger to keep the current behavior for Clank (not resolving symbolic
links).
> > 
> > I was thinking about revising the patch to something like:
> > 
> > 1) As part of the boot sequence of Clank, get absolute paths for /sdcard and
> > /mnt/sdcard.
> > 2) Pass the absolute paths to ChromeNetworkDelegate's constructor
> > 
> > This way, Clank will also be able to use absolute paths for checking, and
this
> > patch will be back to a small one.
> > 
> > Does this approach make sense to you?
> 
> mmenke: could you also give me a comment on this?

BTW, I took a look at the code and found that it'd be not straightforward to do
this as ChromeNetworkDelegate is created in two places. :P

[satorux1, 2017-04-21 06:35:10]

https://codereview.chromium.org/2786583002/diff/320001/extensions/shell/brows...
File extensions/shell/browser/shell_network_delegate.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/extensions/shell/brows...
extensions/shell/browser/shell_network_delegate.cc:116: return true;
On 2017/04/19 07:22:22, satorux1 wrote:
> On 2017/04/18 17:24:45, mmenke wrote:
> > Why change behavior here?
> 
> Without this, extensions tests failed, because access to some test files got
> rejected by the check in URLRequestFileJob. I need to double-check but it
seemed
> that the code here is user for testing purpose.

this turned out to be no longer necessary, due to the change
inextension_protocols.cc

[satorux1, 2017-04-21 07:13:25]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 07:13:44]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 07:59:01]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 07:59:02]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[satorux1, 2017-04-21 08:05:37]

https://codereview.chromium.org/2786583002/diff/320001/net/base/network_deleg...
File net/base/network_delegate.h (right):

https://codereview.chromium.org/2786583002/diff/320001/net/base/network_deleg...
net/base/network_delegate.h:274: // is allowed.
On 2017/04/18 17:24:45, mmenke wrote:
> subclass of what?  (Also, subclass is one word)

Done.

https://codereview.chromium.org/2786583002/diff/320001/net/base/network_deleg...
File net/base/network_delegate_impl.h (right):

https://codereview.chromium.org/2786583002/diff/320001/net/base/network_deleg...
net/base/network_delegate_impl.h:188: // is allowed.
On 2017/04/18 17:24:45, mmenke wrote:
> same comment as in other file...  Or better, just remove all comments from
this
> file, as they're redundant with the other one.  If you prefer not to do that
in
> this CL, that's fine.

Let me do it in a separate patch.

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/file_p...
File net/url_request/file_protocol_handler.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/net/url_request/file_p...
net/url_request/file_protocol_handler.cc:31: }
On 2017/04/18 17:24:46, mmenke wrote:
> This should probably also be moved into the job classes

Moving to Job classes broke net_unittests as the tests pass null
networkdelegate.

[satorux1, 2017-04-21 08:05:42]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 08:05:57]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-04-21 08:07:33]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-04-21 08:07:47]

Dry run: CQ is trying da patch. Follow status at
 
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-04-21 08:36:29]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-04-21 08:36:30]

Dry run: Try jobs failed on following builders:
  linux_chromium_tsan_rel_ng on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/linux_chromium_...)

[mmenke, 2017-04-21 15:14:56]

On 2017/04/21 06:27:45, satorux1 wrote:
> On 2017/04/21 01:04:01, satorux1 wrote:
> > > As shown in Patch Set 1, this patch was much smaller originally, but got
> much
> > > bigger to keep the current behavior for Clank (not resolving symbolic
> links).
> > > 
> > > I was thinking about revising the patch to something like:
> > > 
> > > 1) As part of the boot sequence of Clank, get absolute paths for /sdcard
and
> > > /mnt/sdcard.
> > > 2) Pass the absolute paths to ChromeNetworkDelegate's constructor
> > > 
> > > This way, Clank will also be able to use absolute paths for checking, and
> this
> > > patch will be back to a small one.
> > > 
> > > Does this approach make sense to you?
> > 
> > mmenke: could you also give me a comment on this?
> 
> BTW, I took a look at the code and found that it'd be not straightforward to
do
> this as ChromeNetworkDelegate is created in two places. :P

I think checking both paths is better, in that it draws attention to symbolic
link support to people writing subclasses.  I think an API that encourages
people to think about security issues trumps patch size.  I'd also argue
minimizing patch size should be a non-goal, though certainly too much code churn
can create bugs.

[satorux1, 2017-05-09 07:29:09]

> I think checking both paths is better, in that it draws attention to symbolic
> link support to people writing subclasses.  I think an API that encourages
> people to think about security issues trumps patch size.  I'd also argue
> minimizing patch size should be a non-goal, though certainly too much code
churn
> can create bugs.

Thank you for the feedback! I'm going to upload a revised patch. Sorry for
taking so long!

[satorux1, 2017-05-09 07:43:19]

Description was changed from

==========
chromeos: Use absolute path to check if path is accessible via file://

Previously, paths not in the whitelist were accessibly through
symbolic links via file: schema. This patch solves this
problem by resolving symblic links before checking.

It is a known issue that this approach has a race between
time to check the resolved path and time to read the path.

BUG=677933
TEST=confirm that paths not in the whitelist cannot be accessed via symbolic
links on Chrome OS
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
chromeos: Check both original and absolute paths for file: scheme

Previously, paths not in the whitelist were accessible through
symbolic links via file: scheme. This patch solves this
problem by checking both original and absolute paths (i.e.
symbolic link is resolved with the latter).

It is a known issue that this approach has a race between
time to check the resolved path and time to read the path.

BUG=677933
TEST=confirm that paths not in the whitelist cannot be accessed via symbolic
links on Chrome OS
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

[satorux1, 2017-05-09 07:44:08]

satorux@chromium.org changed reviewers:
+ jorgelo@chromium.org, mmoroz@chromium.org

[satorux1, 2017-05-09 07:44:09]

I think the patch is ready for review. PTAL

https://codereview.chromium.org/2786583002/diff/320001/chrome/browser/net/chr...
File chrome/browser/net/chrome_network_delegate.cc (right):

https://codereview.chromium.org/2786583002/diff/320001/chrome/browser/net/chr...
chrome/browser/net/chrome_network_delegate.cc:471: const base::FilePath& path =
original_path;
On 2017/04/19 07:22:22, satorux1 wrote:
> Thanks. I'll ask jorgelo@ for a review once the patch is ready for a real code
> review. :)

Changed to check both by default.

[satorux1, 2017-05-09 07:44:31]

Patchset #10 (id:380001) has been deleted

[satorux1, 2017-05-09 07:44:49]

Patchset #11 (id:420001) has been deleted

[satorux1, 2017-05-09 07:45:09]

Patchset #10 (id:400001) has been deleted

[satorux1, 2017-05-09 07:45:26]

Patchset #9 (id:360001) has been deleted

[satorux1, 2017-05-09 07:45:50]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[satorux1, 2017-05-09 07:46:13]

satorux@chromium.org changed reviewers:
+ mnissler@chromium.org

[commit-bot: I haz the power, 2017-05-09 07:46:30]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-09 08:28:31]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-09 08:28:32]

Dry run: Try jobs failed on following builders:
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mmoroz, 2017-05-09 13:30:07]

LGTM

https://codereview.chromium.org/2786583002/diff/460001/net/base/directory_lis...
File net/base/directory_lister.cc (right):

https://codereview.chromium.org/2786583002/diff/460001/net/base/directory_lis...
net/base/directory_lister.cc:147: data.absolute_path =
base::MakeAbsoluteFilePath(path);
Excuse me if this is a stupid question :) |MakeAbsoluteFilePath| may return an
empty path in case of an error, but empty path can not bypass |CanAccessFile|
checks, right?

[satorux1, 2017-05-10 06:02:29]

Just noticed that some tests were failing. I'll have a look.

https://codereview.chromium.org/2786583002/diff/460001/net/base/directory_lis...
File net/base/directory_lister.cc (right):

https://codereview.chromium.org/2786583002/diff/460001/net/base/directory_lis...
net/base/directory_lister.cc:147: data.absolute_path =
base::MakeAbsoluteFilePath(path);
On 2017/05/09 13:30:07, mmoroz wrote:
> Excuse me if this is a stupid question :) |MakeAbsoluteFilePath| may return an
> empty path in case of an error, but empty path can not bypass |CanAccessFile|
> checks, right?

That's a great question. An empty path is not allowed by
ChromeNetworkDelegate::IsAccessAllowed(). Added a test for it.

[satorux1, 2017-05-10 06:02:29]

Just noticed that some tests were failing. I'll have a look.

https://codereview.chromium.org/2786583002/diff/460001/net/base/directory_lis...
File net/base/directory_lister.cc (right):

https://codereview.chromium.org/2786583002/diff/460001/net/base/directory_lis...
net/base/directory_lister.cc:147: data.absolute_path =
base::MakeAbsoluteFilePath(path);
On 2017/05/09 13:30:07, mmoroz wrote:
> Excuse me if this is a stupid question :) |MakeAbsoluteFilePath| may return an
> empty path in case of an error, but empty path can not bypass |CanAccessFile|
> checks, right?

That's a great question. An empty path is not allowed by
ChromeNetworkDelegate::IsAccessAllowed(). Added a test for it.

[satorux1, 2017-05-10 07:47:00]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-10 07:47:50]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-05-10 07:48:07]

Patchset #13 (id:520001) has been deleted

[satorux1, 2017-05-10 07:49:17]

On 2017/05/10 06:02:29, satorux1 wrote:
> Just noticed that some tests were failing. I'll have a look.

some null checks were missing. PS#13 should fix the issues.

[commit-bot: I haz the power, 2017-05-10 08:40:04]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-10 08:40:05]

Dry run: Try jobs failed on following builders:
  linux_android_rel_ng on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/linux_androi...)

[mmenke, 2017-05-10 18:03:32]

https://codereview.chromium.org/2786583002/diff/540001/net/url_request/url_re...
File net/url_request/url_request_file_dir_job.cc (right):

https://codereview.chromium.org/2786583002/diff/540001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.cc:121: }
I'm fine with this change, but I don't think this belongs in this CL, and I
think it needs its own test (Which shouldn't be too hard to write).

https://codereview.chromium.org/2786583002/diff/540001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2786583002/diff/540001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1107: TEST_F(URLRequestTest,
AllowFileURLs) {
Think we still need to test the original_path argument somewhere.

https://codereview.chromium.org/2786583002/diff/540001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1189: EXPECT_EQ("", d.data_received());
For the failure cases, can you check that d.request_status() is
ERR_ACCESS_DENIED?

[mmenke, 2017-05-10 18:18:28]

One other question:  Since this is considered a security issue, particularly on
ChromeOS, should we have an integration test for this?

With servicification of the network stack, it's entirely possible that we'll be
using completely different code for file URLs in a year or two, and a
browser_test would provide some protection against this regressing.

[satorux1, 2017-05-12 08:21:30]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-12 08:21:54]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-12 08:33:36]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-12 08:33:38]

Dry run: Try jobs failed on following builders:
  cast_shell_linux on master.tryserver.chromium.linux (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.linux/builders/cast_shell_linu...)

[satorux1, 2017-05-12 13:24:11]

oops. the complilation is failing in the bots since I forgot to rebase. i don't
have a vpn connection now to fix it. anyway, here's a new patch with comments
addressed.

https://codereview.chromium.org/2786583002/diff/540001/net/url_request/url_re...
File net/url_request/url_request_file_dir_job.cc (right):

https://codereview.chromium.org/2786583002/diff/540001/net/url_request/url_re...
net/url_request/url_request_file_dir_job.cc:121: }
On 2017/05/10 18:03:31, mmenke wrote:
> I'm fine with this change, but I don't think this belongs in this CL, and I
> think it needs its own test (Which shouldn't be too hard to write).

good point. removed!

https://codereview.chromium.org/2786583002/diff/540001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2786583002/diff/540001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1107: TEST_F(URLRequestTest,
AllowFileURLs) {
On 2017/05/10 18:03:31, mmenke wrote:
> Think we still need to test the original_path argument somewhere.

Good point! My original plan was to always use absolute paths, and the tests
here were written with that assumption.

I've just updated tests to use both original and absolute paths passed to
PickyNetworkDelegate::OnCanAccessFile().

https://codereview.chromium.org/2786583002/diff/540001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1189: EXPECT_EQ("", d.data_received());
On 2017/05/10 18:03:31, mmenke wrote:
> For the failure cases, can you check that d.request_status() is
> ERR_ACCESS_DENIED?

Good idea! Done.

[satorux1, 2017-05-12 13:28:32]

On 2017/05/10 18:18:28, mmenke wrote:
> One other question:  Since this is considered a security issue, particularly
on
> ChromeOS, should we have an integration test for this?
> 
> With servicification of the network stack, it's entirely possible that we'll
be
> using completely different code for file URLs in a year or two, and a
> browser_test would provide some protection against this regressing.

That's a great point. I think it's not straightforward to add a browser test now
since access to any files is currently allowed on Chrome for Chrome OS running
on Linux desktop [1], that's where browser tests run on bots. Let me ask around
to see if it's OK to get rid of this special behavior on Linux desktop.

[1]
https://cs.chromium.org/chromium/src/chrome/browser/net/chrome_network_delega...

[satorux1, 2017-05-15 00:46:50]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-15 00:47:09]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-15 01:28:21]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-15 01:28:23]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)
  ios-simulator on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/ios-simulator/bui...)

[mmenke, 2017-05-15 19:21:14]

https://codereview.chromium.org/2786583002/diff/600001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2786583002/diff/600001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:839: class PickyNetworkDelegate : public
TestNetworkDelegate {
nit:  Maybe CookieBlockingNetworkDelegate?

https://codereview.chromium.org/2786583002/diff/600001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:865: std::set<base::FilePath>
whitelist_;
nit:  DISALLOW_COPY_AND_ASSIGN

https://codereview.chromium.org/2786583002/diff/600001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1176:
base::FilePath(FILE_PATH_LITERAL("/dev/null")), bad_symlink));
Maybe use something that's windows friendly?  Not sure if we'll ever support
symlinks on Windows, but I don't think that's a reason to use Windows-hostile
code, unless there are benefits in doing so.  Can just whitelist both the good
file and the good symlink, rather than the path (Or do something else - lots of
options).

https://codereview.chromium.org/2786583002/diff/600001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1226:
base::FilePath(FILE_PATH_LITERAL("/dev")), bad_symlink));
Do we really want to be modifying /dev in a test?  Seems like a bad idea.  Could
we just make a non-whitelisted file in the temp directory?

[satorux1, 2017-05-19 07:01:18]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-19 07:02:01]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-05-19 07:02:53]

https://codereview.chromium.org/2786583002/diff/600001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2786583002/diff/600001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:839: class PickyNetworkDelegate : public
TestNetworkDelegate {
On 2017/05/15 19:21:14, mmenke wrote:
> nit:  Maybe CookieBlockingNetworkDelegate?

Done.

https://codereview.chromium.org/2786583002/diff/600001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:865: std::set<base::FilePath>
whitelist_;
On 2017/05/15 19:21:14, mmenke wrote:
> nit:  DISALLOW_COPY_AND_ASSIGN

Done.

https://codereview.chromium.org/2786583002/diff/600001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1176:
base::FilePath(FILE_PATH_LITERAL("/dev/null")), bad_symlink));
On 2017/05/15 19:21:14, mmenke wrote:
> Maybe use something that's windows friendly?  Not sure if we'll ever support
> symlinks on Windows, but I don't think that's a reason to use Windows-hostile
> code, unless there are benefits in doing so.  Can just whitelist both the good
> file and the good symlink, rather than the path (Or do something else - lots
of
> options).

Good point. Done

https://codereview.chromium.org/2786583002/diff/600001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1226:
base::FilePath(FILE_PATH_LITERAL("/dev")), bad_symlink));
On 2017/05/15 19:21:14, mmenke wrote:
> Do we really want to be modifying /dev in a test?  Seems like a bad idea. 
Could
> we just make a non-whitelisted file in the temp directory?

it's not modifying /dev. it's just creating a symlink to /dev, but I agree that
making a non-whitelisted file in the temp dir is better.  Fixed.

[satorux1, 2017-05-19 07:41:56]

https://codereview.chromium.org/2786583002/diff/640001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2786583002/diff/640001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:848: LOG(ERROR) << "@@ AddToWhitelist: "
<< directory.value();
will remove these failures on win bots are addressed.

[commit-bot: I haz the power, 2017-05-19 08:15:08]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-19 08:15:10]

Dry run: Try jobs failed on following builders:
  win_chromium_x64_rel_ng on master.tryserver.chromium.win (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.win/builders/win_chromium_x64_...)

[satorux1, 2017-05-19 13:11:42]

https://codereview.chromium.org/2786583002/diff/640001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2786583002/diff/640001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:848: LOG(ERROR) << "@@ AddToWhitelist: "
<< directory.value();
On 2017/05/19 07:41:56, satorux1 wrote:
> will remove these failures on win bots are addressed.

here's we got: 

[ RUN      ] URLRequestTest.AllowFileURLs
[6084:5248:0519/004647.095:13434494:ERROR:url_request_unittest.cc(848)] @@
AddToWhitelist: C:\Users\CHROME~2\AppData\Local\Temp\scoped_dir6084_22204
[6084:5248:0519/004647.095:13434494:ERROR:url_request_unittest.cc(868)] @@
original_path:
C:\Users\chrome-bot\AppData\Local\Temp\scoped_dir6084_22204\3b93f3d8-e7a5-4c62-a7da-2e9b0895dd17.tmp
[6084:5248:0519/004647.095:13434494:ERROR:url_request_unittest.cc(869)] @@
absolute_path:
C:\Users\chrome-bot\AppData\Local\Temp\scoped_dir6084_22204\3b93f3d8-e7a5-4c62-a7da-2e9b0895dd17.tmp
e:\b\c\b\win\src\net\url_request\url_request_unittest.cc(1146): error: Value of:
d.request_failed()
  Actual: true
Expected: false
e:\b\c\b\win\src\net\url_request\url_request_unittest.cc(1147): error:      
Expected: test_data
      Which is: "monkey"
To be equal to: d.data_received()
      Which is: ""
[6084:5248:0519/004647.096:13434494:ERROR:url_request_unittest.cc(868)] @@
original_path:
C:\Users\chrome-bot\AppData\Local\Temp\scoped_dir6084_22204\3b93f3d8-e7a5-4c62-a7da-2e9b0895dd17.tmp
[6084:5248:0519/004647.096:13434494:ERROR:url_request_unittest.cc(869)] @@
absolute_path:
C:\Users\chrome-bot\AppData\Local\Temp\scoped_dir6084_22204\3b93f3d8-e7a5-4c62-a7da-2e9b0895dd17.tmp
[  FAILED  ] URLRequestTest.AllowFileURLs (2 ms)
[5420/18265] URLRequestTest.AllowFileURLs (2 ms)


"CHROME~2" and "chrome-bot" do not match hence the test is failing...  I'll have
a closer look next week.

[satorux1, 2017-05-22 05:21:25]

Some more logging:

[4912:5716:0521/220205.515:77161682:ERROR:url_request_unittest.cc(1128)] @@
temp_dir: C:\Users\CHROME~2\AppData\Local\Temp\scoped_dir4912_28556
[4912:5716:0521/220205.515:77161682:ERROR:url_request_unittest.cc(1132)] @@
absolute_temp_dir: C:\Users\CHROME~2\AppData\Local\Temp\scoped_dir4912_28556
[4912:5716:0521/220205.515:77161682:ERROR:url_request_unittest.cc(1135)] @@
test_file:
C:\Users\chrome-bot\AppData\Local\Temp\scoped_dir4912_28556\4a398371-a91f-490e-abd5-6f051fcf5186.tmp
[4912:5716:0521/220205.516:77161698:ERROR:url_request_unittest.cc(1136)] @@
absolute_test_file:
C:\Users\chrome-bot\AppData\Local\Temp\scoped_dir4912_28556\4a398371-a91f-490e-abd5-6f051fcf5186.tmp
[4912:5716:0521/220205.516:77161698:ERROR:url_request_unittest.cc(848)] @@
AddToWhitelist: C:\Users\CHROME~2\AppData\Local\Temp\scoped_dir4912_28556
[4912:5716:0521/220205.516:77161698:ERROR:url_request_unittest.cc(868)] @@
original_path:
C:\Users\chrome-bot\AppData\Local\Temp\scoped_dir4912_28556\4a398371-a91f-490e-abd5-6f051fcf5186.tmp
[4912:5716:0521/220205.516:77161698:ERROR:url_request_unittest.cc(869)] @@
absolute_path:
C:\Users\chrome-bot\AppData\Local\Temp\scoped_dir4912_28556\4a398371-a91f-490e-abd5-6f051fcf5186.tmp
e:\b\c\b\win\src\net\url_request\url_request_unittest.cc(1151): error: Value of:
d.request_failed()
  Actual: true

For whatever reason, 'Chrome~2' is changed to 'chrome-bot' in the path returned
from CreateTemporaryFileInDir(). I guess I'll just have to use the dirname of
the path returned from this function as a whitelisted directory. I'll revise the
patch accordingly.

[satorux1, 2017-05-22 05:42:49]

"For whatever reason" -> CreateTemporaryFileInDir() uses GetLongPathName(),
that, I guess, expands CHROME~2 to chrome-bot. :P

[satorux1, 2017-05-22 06:01:57]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 06:02:18]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 06:50:30]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 06:50:31]

Dry run: Try jobs failed on following builders:
  android_clang_dbg_recipe on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_clan...)

[satorux1, 2017-05-22 07:57:00]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-22 07:57:14]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-22 08:44:45]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-22 08:44:47]

Dry run: Try jobs failed on following builders:
  android_n5x_swarming_rel on master.tryserver.chromium.android (JOB_FAILED,
https://build.chromium.org/p/tryserver.chromium.android/builders/android_n5x_...)

[mmenke, 2017-05-22 15:10:38]

Should file a bug against Windows.  "GetLongPathName() does what it is specced
to do"

[satorux1, 2017-05-24 04:58:36]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-24 04:59:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-24 07:59:24]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-24 07:59:25]

Dry run: This issue passed the CQ dry run.

[mmenke, 2017-05-24 19:25:51]

Sorry for the delay!  LGTM.  I'm quite happy with this.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:848: LOG(ERROR) << "@@ AddToWhitelist: "
<< directory.value();
nit:  Remove logging.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:853: bool OnCanAccessFileInternal(const
base::FilePath& path) const {
nit:  Private?  Doesn't really matter, except that a public *Internal method
seems a bit weird.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:867: LOG(ERROR) << "@@ absolute_path: "
<< absolute_path.value();
nit:  Remove logging.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1126: LOG(ERROR) << "@@ temp_dir: " <<
temp_dir.GetPath().value();
nit:  Remove logging (Not going to call out the others)

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1136: // can be slightly different from
temp_dir on Windows.
nit:  |absolute_temp_dir|?  (|| around local names is the convention in net/,
and match variable name)

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1154: // This should be allowed as the
file path is white listed.
nit:  You're using whitelisting above, white listed here.  I believe it's
actually one word.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1167: // This should be rejected as the
file path is not white listed.
whitelisted

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1183: // Create a good directory (will
be white listed) and a good file.
whitelisted

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1190: // See the comment in
AllowFileURLs() for why doing this.
why doing this -> why this is done (Or why this is needed)

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1193: // Create a bad directory (will
not be white listed) and a bad file.
whitelisted

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1218:
default_context_.set_network_delegate(&network_delegate);
Optional:  Can move this network delegate outside of these two blocks, to make
it clearer it's shared.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1251: // Create a good directory (will
be white listed).
whitelisted

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1255: // Create a bad directory (will
not be white listed).
whitelisted

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1276:
default_context_.set_network_delegate(&network_delegate);
Again, can share these

[satorux1, 2017-05-25 01:38:07]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 01:39:15]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-05-25 01:51:07]

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
File net/url_request/url_request_unittest.cc (right):

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:848: LOG(ERROR) << "@@ AddToWhitelist: "
<< directory.value();
On 2017/05/24 19:25:51, mmenke wrote:
> nit:  Remove logging.

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:853: bool OnCanAccessFileInternal(const
base::FilePath& path) const {
On 2017/05/24 19:25:50, mmenke wrote:
> nit:  Private?  Doesn't really matter, except that a public *Internal method
> seems a bit weird.

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:867: LOG(ERROR) << "@@ absolute_path: "
<< absolute_path.value();
On 2017/05/24 19:25:50, mmenke wrote:
> nit:  Remove logging.

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1126: LOG(ERROR) << "@@ temp_dir: " <<
temp_dir.GetPath().value();
On 2017/05/24 19:25:50, mmenke wrote:
> nit:  Remove logging (Not going to call out the others)

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1136: // can be slightly different from
temp_dir on Windows.
On 2017/05/24 19:25:50, mmenke wrote:
> nit:  |absolute_temp_dir|?  (|| around local names is the convention in net/,
> and match variable name)

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1154: // This should be allowed as the
file path is white listed.
On 2017/05/24 19:25:51, mmenke wrote:
> nit:  You're using whitelisting above, white listed here.  I believe it's
> actually one word.

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1167: // This should be rejected as the
file path is not white listed.
On 2017/05/24 19:25:50, mmenke wrote:
> whitelisted

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1183: // Create a good directory (will
be white listed) and a good file.
On 2017/05/24 19:25:51, mmenke wrote:
> whitelisted

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1190: // See the comment in
AllowFileURLs() for why doing this.
On 2017/05/24 19:25:50, mmenke wrote:
> why doing this -> why this is done (Or why this is needed)

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1193: // Create a bad directory (will
not be white listed) and a bad file.
On 2017/05/24 19:25:50, mmenke wrote:
> whitelisted

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1218:
default_context_.set_network_delegate(&network_delegate);
On 2017/05/24 19:25:50, mmenke wrote:
> Optional:  Can move this network delegate outside of these two blocks, to make
> it clearer it's shared.

Good idea. Done!

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1251: // Create a good directory (will
be white listed).
On 2017/05/24 19:25:50, mmenke wrote:
> whitelisted

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1255: // Create a bad directory (will
not be white listed).
On 2017/05/24 19:25:50, mmenke wrote:
> whitelisted

Done.

https://codereview.chromium.org/2786583002/diff/760001/net/url_request/url_re...
net/url_request/url_request_unittest.cc:1276:
default_context_.set_network_delegate(&network_delegate);
On 2017/05/24 19:25:50, mmenke wrote:
> Again, can share these

Done.

[satorux1, 2017-05-25 01:54:10]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-25 01:55:03]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-05-25 01:57:12]

On 2017/05/24 19:25:51, mmenke wrote:
> Sorry for the delay!  LGTM.  I'm quite happy with this.

Thank you for patiently reviewing the change!

[mmenke, 2017-05-25 03:07:10]

On 2017/05/25 01:57:12, satorux1 wrote:
> On 2017/05/24 19:25:51, mmenke wrote:
> > Sorry for the delay!  LGTM.  I'm quite happy with this.
> 
> Thank you for patiently reviewing the change!

Oh, yea, there's still the issue of the browser test - thinking about adding it
in this CL, or a followup?

[satorux1, 2017-05-25 03:45:59]

satorux@chromium.org changed reviewers:
+ alokp@chromium.org, benwells@chromium.org, eugenebut@chromium.org,
mkwst@chromium.org, skyostil@chromium.org, thestig@chromium.org,
torne@chromium.org

[satorux1, 2017-05-25 03:46:01]

PTAL! The changes to the following directories are mostly just to add
|absolute_path| parameter to NetworkDelegate::OnCanAccessFile().

torne: android_webview/...
thestig: chrome/...
alokp: chromecast/...
mkwst: content/shell/...
benwells: extensions/...
skyostil: headless/...
eugenebut: ios/...

[alokp, 2017-05-25 04:04:53]

lgtm

[mmenke, 2017-05-25 04:07:17]

satorux1:  I own chrome/browser/net, so rather than using thestig, a top level
chrome/ owner, you should use an owner of
	chrome/browser/android/offline_pages/.

[satorux1, 2017-05-25 04:09:29]

satorux@chromium.org changed reviewers:
+ dewittj@chromium.org

[satorux1, 2017-05-25 04:09:31]

mmenke: that's good point!

PTAL:
dewittj: chrome/browser/android/offline_pages
thesig: none :)

[benwells, 2017-05-25 05:00:21]

benwells@chromium.org changed reviewers:
+ rdevlin.cronin@chromium.org

[benwells, 2017-05-25 05:00:22]

Sorry I don't know much about this particular extension class, punting to
Devlin.

[commit-bot: I haz the power, 2017-05-25 05:02:54]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-25 05:02:56]

Dry run: This issue passed the CQ dry run.

[Mike West, 2017-05-25 09:33:39]

//content/shell LGTM

[Eugene But (OOO till 7-30), 2017-05-25 14:40:33]

ios lgtm

[Devlin, 2017-05-25 15:06:31]

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
extensions/browser/extension_protocols.cc:291: return true;
What do |original_path| and |absolute_path| here look like?  It would be nice to
have some kind of sanity checks, rather than always returning true.

[Torne, 2017-05-25 16:30:38]

android_webview LGTM

[Sami, 2017-05-25 17:30:20]

headless/ lgtm.

[Lei Zhang, 2017-05-25 21:07:14]

thestig@chromium.org changed reviewers:
+ jianli@chromium.org

[Lei Zhang, 2017-05-25 21:07:16]

I defer to jianli who wrote most of offline_page_request_job.cc.

[satorux1, 2017-05-25 21:14:50]

Devlin, PTAL

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
extensions/browser/extension_protocols.cc:291: return true;
On 2017/05/25 15:06:30, Devlin (catching up) wrote:
> What do |original_path| and |absolute_path| here look like?  It would be nice
to
> have some kind of sanity checks, rather than always returning true.

If the original_path is a symlink in the user's Downloads folder, these look
like

- original_path: /home/chronos/user/Downloads/symlink
- absolute_path: /etc/hotsts

URLRequestJob::CanAccessFile() is added to control access to files via file://
URLs where users can specify arbitrary file paths.

Here, URLRequestExtensionJob is used to read resource files, and this patch
didn't change the existing behavior (there was no check against the file path).
I think we can add some a sanity check like

  return file_path_ == original_path;

Does this sound reasonable?

[Devlin, 2017-05-26 00:55:36]

lgtm if there are no issues

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
extensions/browser/extension_protocols.cc:291: return true;
On 2017/05/25 21:14:50, satorux1 wrote:
> On 2017/05/25 15:06:30, Devlin (catching up) wrote:
> > What do |original_path| and |absolute_path| here look like?  It would be
nice
> to
> > have some kind of sanity checks, rather than always returning true.
> 
> If the original_path is a symlink in the user's Downloads folder, these look
> like
> 
> - original_path: /home/chronos/user/Downloads/symlink
> - absolute_path: /etc/hotsts
> 
> URLRequestJob::CanAccessFile() is added to control access to files via file://
> URLs where users can specify arbitrary file paths.
> 
> Here, URLRequestExtensionJob is used to read resource files, and this patch
> didn't change the existing behavior (there was no check against the file
path).
> I think we can add some a sanity check like
> 
>   return file_path_ == original_path;
> 
> Does this sound reasonable?

Unfortunately, I think we do allow sym links in extensions.  I *think* that we
should be able to at least check that paths are always within the directory. 
I'd suggest something like:

// The access checks for the file are performed before the job is
// created, so we should know that this is safe.
DCHECK(directory_path_.IsParent(absolute_path));
return true;

Can you run try that and see if anything breaks?  (I ran the unit tests and it
seems to be fine, but there might be an edge case I'm missing)

[satorux1, 2017-05-26 03:28:15]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[satorux1, 2017-05-26 03:28:39]

To: Devlin

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
extensions/browser/extension_protocols.cc:291: return true;
On 2017/05/26 00:55:36, Devlin (catching up) wrote:
> On 2017/05/25 21:14:50, satorux1 wrote:
> > On 2017/05/25 15:06:30, Devlin (catching up) wrote:
> > > What do |original_path| and |absolute_path| here look like?  It would be
> nice
> > to
> > > have some kind of sanity checks, rather than always returning true.
> > 
> > If the original_path is a symlink in the user's Downloads folder, these look
> > like
> > 
> > - original_path: /home/chronos/user/Downloads/symlink
> > - absolute_path: /etc/hotsts
> > 
> > URLRequestJob::CanAccessFile() is added to control access to files via
file://
> > URLs where users can specify arbitrary file paths.
> > 
> > Here, URLRequestExtensionJob is used to read resource files, and this patch
> > didn't change the existing behavior (there was no check against the file
> path).
> > I think we can add some a sanity check like
> > 
> >   return file_path_ == original_path;
> > 
> > Does this sound reasonable?
> 
> Unfortunately, I think we do allow sym links in extensions.  I *think* that we
> should be able to at least check that paths are always within the directory. 
> I'd suggest something like:
> 
> // The access checks for the file are performed before the job is
> // created, so we should know that this is safe.
> DCHECK(directory_path_.IsParent(absolute_path));
> return true;
> 
> Can you run try that and see if anything breaks?  (I ran the unit tests and it
> seems to be fine, but there might be an edge case I'm missing)

Done. I'm going to run try bots.

[commit-bot: I haz the power, 2017-05-26 03:29:20]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-05-26 03:45:49]

jianli@, dewitt@:

Could you take a loo at chrome/browser/android/offline_pages ? I think this is
the last piece that needs a review. Thanks!

[commit-bot: I haz the power, 2017-05-26 04:42:07]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 04:42:10]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[satorux1, 2017-05-26 05:00:41]

To: Devlin

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
extensions/browser/extension_protocols.cc:291: return true;
On 2017/05/26 03:28:38, satorux1 wrote:
> On 2017/05/26 00:55:36, Devlin (catching up) wrote:
> > On 2017/05/25 21:14:50, satorux1 wrote:
> > > On 2017/05/25 15:06:30, Devlin (catching up) wrote:
> > > > What do |original_path| and |absolute_path| here look like?  It would be
> > nice
> > > to
> > > > have some kind of sanity checks, rather than always returning true.
> > > 
> > > If the original_path is a symlink in the user's Downloads folder, these
look
> > > like
> > > 
> > > - original_path: /home/chronos/user/Downloads/symlink
> > > - absolute_path: /etc/hotsts
> > > 
> > > URLRequestJob::CanAccessFile() is added to control access to files via
> file://
> > > URLs where users can specify arbitrary file paths.
> > > 
> > > Here, URLRequestExtensionJob is used to read resource files, and this
patch
> > > didn't change the existing behavior (there was no check against the file
> > path).
> > > I think we can add some a sanity check like
> > > 
> > >   return file_path_ == original_path;
> > > 
> > > Does this sound reasonable?
> > 
> > Unfortunately, I think we do allow sym links in extensions.  I *think* that
we
> > should be able to at least check that paths are always within the directory.

> > I'd suggest something like:
> > 
> > // The access checks for the file are performed before the job is
> > // created, so we should know that this is safe.
> > DCHECK(directory_path_.IsParent(absolute_path));
> > return true;
> > 
> > Can you run try that and see if anything breaks?  (I ran the unit tests and
it
> > seems to be fine, but there might be an edge case I'm missing)
> 
> Done. I'm going to run try bots.

I got this error:

[ RUN      ] ExtensionProtocolsTest.VerificationSeenForFileAccessErrors
[6264:771:0525/204813.535575:49518030221189:FATAL:extension_protocols.cc(291)]
Check failed: directory_path_.IsParent(absolute_path).

I'm guessing that |directory_path_| contains a symlink. Let me see if changing
|absolute_path| to |original_path| makes a difference.

[satorux1, 2017-05-26 05:04:54]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-26 05:05:32]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-26 06:10:52]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-26 06:10:54]

Dry run: Try jobs failed on following builders:
  mac_chromium_rel_ng on master.tryserver.chromium.mac (JOB_FAILED,
http://build.chromium.org/p/tryserver.chromium.mac/builders/mac_chromium_rel_...)

[jianli, 2017-05-26 18:52:52]

offline_pages lgtm

https://codereview.chromium.org/2786583002/diff/840001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_request_job.cc (right):

https://codereview.chromium.org/2786583002/diff/840001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_request_job.cc:714: return
true;
Though it is not relevant, will returning false be a bit safer?

[mmenke, 2017-05-26 19:00:04]

https://codereview.chromium.org/2786583002/diff/840001/chrome/browser/android...
File chrome/browser/android/offline_pages/offline_page_request_job.cc (right):

https://codereview.chromium.org/2786583002/diff/840001/chrome/browser/android...
chrome/browser/android/offline_pages/offline_page_request_job.cc:714: return
true;
On 2017/05/26 18:52:52, jianli wrote:
> Though it is not relevant, will returning false be a bit safer?

Wouldn't that break this class entirely?  This subclasses URLRequestFileJob, and
requires access to whatever file it's mapping a request to.  If it can't access
that file, it won't work.

[jianli, 2017-05-26 19:46:51]

On 2017/05/26 19:00:04, mmenke wrote:
>
https://codereview.chromium.org/2786583002/diff/840001/chrome/browser/android...
> File chrome/browser/android/offline_pages/offline_page_request_job.cc (right):
> 
>
https://codereview.chromium.org/2786583002/diff/840001/chrome/browser/android...
> chrome/browser/android/offline_pages/offline_page_request_job.cc:714: return
> true;
> On 2017/05/26 18:52:52, jianli wrote:
> > Though it is not relevant, will returning false be a bit safer?
> 
> Wouldn't that break this class entirely?  This subclasses URLRequestFileJob,
and
> requires access to whatever file it's mapping a request to.  If it can't
access
> that file, it won't work.

You're right. I forgot about that we're doing this check now in
URLRequestFileJob. Thanks for pointing this out.

[satorux1, 2017-05-29 03:35:33]

The CQ bit was checked by satorux@chromium.org to run a CQ dry run

[commit-bot: I haz the power, 2017-05-29 03:35:53]

Dry run: CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[satorux1, 2017-05-29 03:36:16]

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
File extensions/browser/extension_protocols.cc (right):

https://codereview.chromium.org/2786583002/diff/800001/extensions/browser/ext...
extensions/browser/extension_protocols.cc:291: return true;
> I'm guessing that |directory_path_| contains a symlink. Let me see if changing
> |absolute_path| to |original_path| makes a difference.

Changed to |original_path| but still the test failed on a Mac bot. I think it's
complicated since Mac bots use symbolic links for temporary directories [1]. Let
me remove DCHECK for now.


[1] Search for "Mac and Android bots" at
https://codereview.chromium.org/2786583002/diff/840001/net/url_request/url_re...

[commit-bot: I haz the power, 2017-05-29 04:53:28]

The CQ bit was unchecked by commit-bot@chromium.org

[commit-bot: I haz the power, 2017-05-29 04:53:29]

Dry run: This issue passed the CQ dry run.

[satorux1, 2017-05-29 06:00:59]

The CQ bit was checked by satorux@chromium.org

[satorux1, 2017-05-29 06:01:00]

The patchset sent to the CQ was uploaded after l-g-t-m from mmoroz@chromium.org,
mmenke@chromium.org, rdevlin.cronin@chromium.org, skyostil@chromium.org,
eugenebut@chromium.org, torne@chromium.org, alokp@chromium.org,
mkwst@chromium.org, jianli@chromium.org
Link to the patchset: https://codereview.chromium.org/2786583002/#ps880001
(title: "remove dcheck")

[commit-bot: I haz the power, 2017-05-29 06:01:10]

CQ is trying da patch.

Follow status at:
https://chromium-cq-status.appspot.com/v2/patch-status/codereview.chromium.or...

[commit-bot: I haz the power, 2017-05-29 06:06:22]

CQ is committing da patch.

Bot data: {"patchset_id": 880001, "attempt_start_ts": 1496037659851230,
"parent_rev": "b576b1bee0590f3e011cda1652c19057d48f8a93", "commit_rev":
"ddac044c41e8b7a223c403e4574f5d44dff5e50e"}

[commit-bot: I haz the power, 2017-05-29 06:06:35]

Description was changed from

==========
chromeos: Check both original and absolute paths for file: scheme

Previously, paths not in the whitelist were accessible through
symbolic links via file: scheme. This patch solves this
problem by checking both original and absolute paths (i.e.
symbolic link is resolved with the latter).

It is a known issue that this approach has a race between
time to check the resolved path and time to read the path.

BUG=677933
TEST=confirm that paths not in the whitelist cannot be accessed via symbolic
links on Chrome OS
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester
==========

to

==========
chromeos: Check both original and absolute paths for file: scheme

Previously, paths not in the whitelist were accessible through
symbolic links via file: scheme. This patch solves this
problem by checking both original and absolute paths (i.e.
symbolic link is resolved with the latter).

It is a known issue that this approach has a race between
time to check the resolved path and time to read the path.

BUG=677933
TEST=confirm that paths not in the whitelist cannot be accessed via symbolic
links on Chrome OS
CQ_INCLUDE_TRYBOTS=master.tryserver.chromium.android:android_cronet_tester

Review-Url: https://codereview.chromium.org/2786583002
Cr-Commit-Position: refs/heads/master@{#475299}
Committed:
https://chromium.googlesource.com/chromium/src/+/ddac044c41e8b7a223c403e4574f...
==========

[commit-bot: I haz the power, 2017-05-29 06:06:38]

Committed patchset #30 (id:880001) as
https://chromium.googlesource.com/chromium/src/+/ddac044c41e8b7a223c403e4574f...