Created attachment 64643 [details] Patch

Created attachment 64767 [details] Patch Simplified the patch.

Comment on attachment 64767 [details] Patch (seems like it'll need more logic)

Created attachment 65145 [details] Patch

Comment on attachment 65145 [details] Patch WebCore/storage/DOMFilePath.cpp:43 + const char* DOMFilePath::root = "/"; const char DOMFilePath::root[] = "/"; ? WebCore/storage/DOMFilePath.cpp:73 + return path; This seems odd, when compared to getName. Is returning path really the right thing to do in each case? WebCore/storage/DOMFilePath.cpp:78 + return mayBeChild.startsWith(path); /foo/bar is not the parent of /foo/barrister. WebCore/storage/DOMFilePath.cpp:115 + const char* p = utf8String.data(); What happens here if the UTF-8 representation of a character in the path doesn't fit in a char? WebCore/storage/DOMFilePath.cpp:125 + if (unallowedNamesRegExp1.match(path) >= 0) Is this match case-sensitive? It needs to be case-insensitive on all of these. WebCore/storage/DOMFilePath.cpp:122 + DEFINE_STATIC_LOCAL(RegularExpression, unallowedNamesRegExp1, ("/(CON|PRN|AUX|NUL)([\\./]|$)", TextCaseInsensitive)); I've never used RegularExpressions from WebKit internally before. Is that last '/' in the right place? WebCore/storage/DOMFilePath.cpp:131 + DEFINE_STATIC_LOCAL(RegularExpression, endingRegExp, ("[\\. ](/|$)", TextCaseInsensitive)); What about other kinds of whitespace [tab, vtab, newline, cr, ...] WebCore/storage/DOMFilePath.cpp:138 + DEFINE_STATIC_LOCAL(RegularExpression, unallowedCharsRegExp, ("[\\\\<><>:\\?\\*\"|]", TextCaseInsensitive)); You've got "<>" in there twice. WebCore/storage/DOMFilePath.cpp:108 + bool DOMFilePath::isValid(const String& path) Perhaps "isValidPath" instead of just "isValid"? One might call this by accident for a name and let something containing a directory separator slip through. WebCore/storage/DOMFilePath.h:44 + static const char* root; How about static const char root[]? WebCore/storage/DOMFilePath.cpp:74 + } I think it might be better to return an empty string, or perhaps make sure not to be called in that case. What's the directory of "foo"? I suppose it could be ".". WebCore/storage/DOMFilePath.h:52 + // Checks if a given path is a parent of mayBeChild. This method assumes given paths are absolute and do not have extra parent references. Explain what a parent reference is. Do you mean "../"? WebCore/storage/DOMFilePath.h:55 + // Appends the separator at the end of the path. ...if it's not there already. WebCore/storage/DOMFilePath.h:71 + // Removes extra parent ("..") references. This doesn't take all the references if it goes beyond the root directory. It does more than that. How about something like this? // Evaluates all "../" and "./" segments. Note that "/../" expands to "/", so you can't ever refer to anything above the root directory.

Created attachment 65211 [details] Patch

Thanks for your careful review! (In reply to comment #5) > (From update of attachment 65145 [details]) > WebCore/storage/DOMFilePath.cpp:43 > + const char* DOMFilePath::root = "/"; > const char DOMFilePath::root[] = "/"; ? Fixed. > WebCore/storage/DOMFilePath.cpp:73 > + return path; > This seems odd, when compared to getName. Is returning path really the right thing to do in each case? I wasn't sure what to return, but I changed this to return an empty string. As you say probably "." or an empty string would be the expected value. > WebCore/storage/DOMFilePath.cpp:78 > + return mayBeChild.startsWith(path); > /foo/bar is not the parent of /foo/barrister. Oh of course not... fixed. > WebCore/storage/DOMFilePath.cpp:115 > + const char* p = utf8String.data(); > What happens here if the UTF-8 representation of a character in the path doesn't fit in a char? Fixed. > WebCore/storage/DOMFilePath.cpp:125 > + if (unallowedNamesRegExp1.match(path) >= 0) > Is this match case-sensitive? It needs to be case-insensitive on all of these. All of RegExps are case-insensitive (CaseInsensitive). > WebCore/storage/DOMFilePath.cpp:122 > + DEFINE_STATIC_LOCAL(RegularExpression, unallowedNamesRegExp1, ("/(CON|PRN|AUX|NUL)([\\./]|$)", TextCaseInsensitive)); > I've never used RegularExpressions from WebKit internally before. Is that last '/' in the right place? It's in the place I intended and my intension was: it should match if CON, PRN, AUX or NUL is followed by a period (\\.), slash (/) or the end of string ($). Does that sound right? > WebCore/storage/DOMFilePath.cpp:131 > + DEFINE_STATIC_LOCAL(RegularExpression, endingRegExp, ("[\\. ](/|$)", TextCaseInsensitive)); > What about other kinds of whitespace [tab, vtab, newline, cr, ...] Replaced ' ' with '\s'. > WebCore/storage/DOMFilePath.cpp:138 > + DEFINE_STATIC_LOCAL(RegularExpression, unallowedCharsRegExp, ("[\\\\<><>:\\?\\*\"|]", TextCaseInsensitive)); > You've got "<>" in there twice. Fixed. > WebCore/storage/DOMFilePath.cpp:108 > + bool DOMFilePath::isValid(const String& path) > Perhaps "isValidPath" instead of just "isValid"? One might call this by accident for a name and let something containing a directory separator slip through. Fixed. > WebCore/storage/DOMFilePath.h:44 > + static const char* root; > How about static const char root[]? Fixed. > WebCore/storage/DOMFilePath.h:52 > + // Checks if a given path is a parent of mayBeChild. This method assumes given paths are absolute and do not have extra parent references. > Explain what a parent reference is. Do you mean "../"? Yes I meant a reference to a parent, i.e. "..". Changed the expression a bit. > WebCore/storage/DOMFilePath.h:55 > + // Appends the separator at the end of the path. > ...if it's not there already. Fixed. > WebCore/storage/DOMFilePath.h:71 > + // Removes extra parent ("..") references. This doesn't take all the references if it goes beyond the root directory. > It does more than that. How about something like this? > > // Evaluates all "../" and "./" segments. Note that "/../" expands to "/", so you can't ever refer to anything above the root directory. Changed the comment (and its behavior) as suggested.

> > WebCore/storage/DOMFilePath.cpp:125 > > + if (unallowedNamesRegExp1.match(path) >= 0) > > Is this match case-sensitive? It needs to be case-insensitive on all of these. > > All of RegExps are case-insensitive (CaseInsensitive). I meant... all of the regexps used here are declared with CaseInsensitive for case-insensitive match.

Comment on attachment 65211 [details] Patch WebCore/storage/DOMFilePath.cpp:124 + DEFINE_STATIC_LOCAL(RegularExpression, unallowedNamesRegExp1, ("/(CON|PRN|AUX|NUL)([\\./]|$)", TextCaseInsensitive)); Ah, I think I see how this is supposed to work. However, you're requiring a leading '/' before matching for e.g. "CON". If this isn't an absolute path, that's not safe. "CON" and "CON/safe/rest/of/path" will pass the test. Or you could assert and document that the input path must be absolute.

Created attachment 65298 [details] Patch

(In reply to comment #9) > (From update of attachment 65211 [details]) > WebCore/storage/DOMFilePath.cpp:124 > + DEFINE_STATIC_LOCAL(RegularExpression, unallowedNamesRegExp1, ("/(CON|PRN|AUX|NUL)([\\./]|$)", TextCaseInsensitive)); > Ah, I think I see how this is supposed to work. However, you're requiring a leading '/' before matching for e.g. "CON". If this isn't an absolute path, that's not safe. "CON" and "CON/safe/rest/of/path" will pass the test. Or you could assert and document that the input path must be absolute. Right, fixed.

(In reply to comment #11) > (In reply to comment #9) > > (From update of attachment 65211 [details] [details]) > > WebCore/storage/DOMFilePath.cpp:124 > > + DEFINE_STATIC_LOCAL(RegularExpression, unallowedNamesRegExp1, ("/(CON|PRN|AUX|NUL)([\\./]|$)", TextCaseInsensitive)); > > Ah, I think I see how this is supposed to work. However, you're requiring a leading '/' before matching for e.g. "CON". If this isn't an absolute path, that's not safe. "CON" and "CON/safe/rest/of/path" will pass the test. Or you could assert and document that the input path must be absolute. > > Right, fixed. OK, LGTM. Now you need a reviewer ;'>.

Comment on attachment 65298 [details] Patch r=me. LGTM + i trust eric's review.

Committed r65975: <http://trac.webkit.org/changeset/65975>