Look closer at the components of vfsfilenames that are used filenames.

Look closer at the components of vfsfilenames that are used filenames.

BUG=352447
TBR=sky

Committed: https://src.chromium.org/viewvc/chrome?view=rev&revision=276348

[jschuh, 2014-03-14 03:38:00]

You don't have to keep working tonight, but I think we're still letting the
host.name:port case through. Can we talk about this tomorrow?

https://codereview.chromium.org/199153003/diff/1/webkit/browser/database/data...
File webkit/browser/database/database_util.cc (right):

https://codereview.chromium.org/199153003/diff/1/webkit/browser/database/data...
webkit/browser/database/database_util.cc:58: base::string16 suffix =
vfs_file_name.substr(
I can't tell how we verify host:port. Could you check that? I'm worried that's
actually broken in the code today.

[michaeln, 2014-03-14 03:55:56]

https://codereview.chromium.org/199153003/diff/1/webkit/browser/database/data...
File webkit/browser/database/database_util.cc (right):

https://codereview.chromium.org/199153003/diff/1/webkit/browser/database/data...
webkit/browser/database/database_util.cc:58: base::string16 suffix =
vfs_file_name.substr(
On 2014/03/14 03:38:00, Justin Schuh wrote:
> I can't tell how we verify host:port. Could you check that? I'm worried that's
> actually broken in the code today.

Maybe DatabaseIdentifier::Parse should test for ':' in its inputs and fail if
found.

I've added that check to DatabaseUtil::IsValidOriginIdentifier, but lets resume
tomorrow.

[michaeln, 2014-03-18 22:32:46]

hi justin, i'd like to get this looked at again

https://codereview.chromium.org/199153003/diff/60001/webkit/common/database/d...
File webkit/common/database/database_identifier.cc (right):

https://codereview.chromium.org/199153003/diff/60001/webkit/common/database/d...
webkit/common/database/database_identifier.cc:63: char forbidden[] = {'\\', '/',
':' ,'\0'};
i've relocated these checks into DatabaseIdentifier::Parse to cast a wider net

[jschuh, 2014-03-24 22:46:23]

https://codereview.chromium.org/199153003/diff/110001/webkit/common/database/...
File webkit/common/database/database_identifier.cc (right):

https://codereview.chromium.org/199153003/diff/110001/webkit/common/database/...
webkit/common/database/database_identifier.cc:63: char forbidden[] = {'\\', '/',
':' ,'\0'};
Can't we have a whitelist here instead of a blacklist?

[michaeln, 2014-04-21 19:48:07]

https://codereview.chromium.org/199153003/diff/110001/webkit/common/database/...
File webkit/common/database/database_identifier.cc (right):

https://codereview.chromium.org/199153003/diff/110001/webkit/common/database/...
webkit/common/database/database_identifier.cc:61: if (identifier.find("..") !=
std::string::npos)
does this exclude some valid hostnames like www..domain.com... and do we care?

https://codereview.chromium.org/199153003/diff/110001/webkit/common/database/...
webkit/common/database/database_identifier.cc:63: char forbidden[] = {'\\', '/',
':' ,'\0'};
On 2014/03/24 22:46:23, Justin Schuh wrote:
> Can't we have a whitelist here instead of a blacklist?

In talking to you i the hallway, you mentioned vetting the suffix specifically.

The scheme and host components have to survive roundtrips thru GURL. Is GURL
rigid enough about the scheme and hostnames components, particularly the scheme?
I'm queasy about second guessing what GURL accepts as valid inputs for host
components since I don't know the details of how idn names are ascii encoded.

The origin identifiers don't involve less restrictive path components, but
vfsfilenames do. In database_util.cc, the path component hold the suffix and
that is subject to a whitelist (alpha + digit + - + . + _ minus '..').

[michaeln, 2014-06-09 18:30:59]

https://codereview.chromium.org/199153003/diff/110001/webkit/browser/database...
File webkit/browser/database/database_util.cc (right):

https://codereview.chromium.org/199153003/diff/110001/webkit/browser/database...
webkit/browser/database/database_util.cc:22: if (!(IsAsciiAlpha(c) ||
IsAsciiDigit(c) ||
here it is...

[jschuh, 2014-06-10 23:45:46]

On 2014/06/09 18:30:59, michaeln wrote:
>
https://codereview.chromium.org/199153003/diff/110001/webkit/browser/database...
> File webkit/browser/database/database_util.cc (right):
> 
>
https://codereview.chromium.org/199153003/diff/110001/webkit/browser/database...
> webkit/browser/database/database_util.cc:22: if (!(IsAsciiAlpha(c) ||
> IsAsciiDigit(c) ||
> here it is...

My bad. Okay, lgtm.

[michaeln, 2014-06-11 00:48:31]

The CQ bit was checked by michaeln@chromium.org

[commit-bot: I haz the power, 2014-06-11 00:52:18]

CQ is trying da patch. Follow status at
 https://chromium-status.appspot.com/cq/michaeln@chromium.org/199153003/130001

[commit-bot: I haz the power, 2014-06-11 11:32:46]

Change committed as 276348