WebKit Bugzilla
You need to log in before you can comment on or make changes to this bug.
Add file path utilities for HTML5 FileSystem API. We need some utility class that takes care of path sanitization (cleaning up relative references), concatenation and etc for virtual filesystem paths.
Created an attachment (id=64643) [details] Patch
Created an attachment (id=64767) [details] Patch Simplified the patch.
(From update of attachment 64767 [details]) (seems like it'll need more logic)
Created an attachment (id=65145) [details] Patch
(From update of attachment 65145 [details]) 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 an attachment (id=65211) [details] Patch
Thanks for your careful review! (In reply to comment #5) > (From update of attachment 65145 [details] [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.
(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.
Created an attachment (id=65298) [details] Patch
(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.
(In reply to comment #11) > (In reply to comment #9) > > (From update of attachment 65211 [details] [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 ;'>.
(From update of attachment 65298 [details]) r=me. LGTM + i trust eric's review.
Committed r65975: <http://trac.webkit.org/changeset/65975>