[fileapi] Add a File::createWithName method to avoid obfuscated filename leakage from FileEntry.file() method
Created attachment 94143 [details] Patch
This patch also happens to ensure the MIME type of the File is chosen based on FileEntry.name instead of the obfuscated name (covered by the same tests). I tested this locally in chrome with --use-obfuscated-file-system --allow-file-access-from-files --unlimited-quota-for-files
(In reply to comment #2) > This patch also happens to ensure the MIME type of the File is chosen based on FileEntry.name instead of the obfuscated name (covered by the same tests). > > I tested this locally in chrome with --use-obfuscated-file-system --allow-file-access-from-files --unlimited-quota-for-files Can you add a layout test for this? Also, for the new File constructor: File(const String& path, const String& name); it seems that you allow name to be empty. This seems to be unnatural.
Comment on attachment 94143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94143&action=review > Source/WebCore/fileapi/File.cpp:39 > + const String& nameForMIMEType = !name.isEmpty() ? name : path; I thought about making name a required argument and always using it to figure out the MIME type but was a little worried that this could change behavior on some platform where pathGetFileName() did something surprising (basically, anything other than pulling out the last path component).
(In reply to comment #3) > (In reply to comment #2) > > This patch also happens to ensure the MIME type of the File is chosen based on FileEntry.name instead of the obfuscated name (covered by the same tests). > > > > I tested this locally in chrome with --use-obfuscated-file-system --allow-file-access-from-files --unlimited-quota-for-files > > Can you add a layout test for this? You mean with the Chromium flag set? I'll defer to Eric about whether that's a good idea at this point. Is there some standard way to say "run this test with the following Chromium flag set?" I'm not aware of one. > Also, for the new File constructor: > File(const String& path, const String& name); > it seems that you allow name to be empty. This seems to be unnatural. Not sure what you mean by "allow". The existing code doesn't care of m_path is empty, nor if m_name ends up empty. What did you think the code should do if name is empty?
(In reply to comment #5) > (In reply to comment #3) > > (In reply to comment #2) > > > This patch also happens to ensure the MIME type of the File is chosen based on FileEntry.name instead of the obfuscated name (covered by the same tests). > > > > > > I tested this locally in chrome with --use-obfuscated-file-system --allow-file-access-from-files --unlimited-quota-for-files > > > > Can you add a layout test for this? > > You mean with the Chromium flag set? I'll defer to Eric about whether that's a good idea at this point. Is there some standard way to say "run this test with the following Chromium flag set?" I'm not aware of one. Also, I should say that --use-obfuscated-file-system will be the default very soon (by M13), at which point the existing tests will provide this coverage.
(In reply to comment #3) > (In reply to comment #2) > > This patch also happens to ensure the MIME type of the File is chosen based on FileEntry.name instead of the obfuscated name (covered by the same tests). > > > > I tested this locally in chrome with --use-obfuscated-file-system --allow-file-access-from-files --unlimited-quota-for-files > > Can you add a layout test for this? There's no way to test this via layout test yet, as this constructor isn't exposed to javascript. When Chromium switches to using obfuscated filesystem paths [next week], the existing layout test fast/filesystem/file-from-file-entry.html will test this code [in fact, it's why we're adding this before making the Chromium change].
Comment on attachment 94143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94143&action=review > Source/WebCore/ChangeLog:20 > + Added an optional name argument to ensure MIME type lookup uses the I think you can move your comment "Added an ..." to be immediately following ": " of the previous line. > Source/WebCore/ChangeLog:24 > + as it now needs to mutate the created File before returning it. Not sure I understand your comment about "out-of-lined" and "mutate". Probably just the first half sentence seems to be enough. > Source/WebCore/fileapi/File.h:56 > + static PassRefPtr<File> createWithName(const String& path, const String& name) Please add the comment saying that |name| is the filename that is observed by the user, that might be different from the real filename embedded in |path|. Or maybe even better, consider rename |name| to something like |observedName|.
Created attachment 94150 [details] Patch
Comment on attachment 94143 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94143&action=review >> Source/WebCore/ChangeLog:20 >> + Added an optional name argument to ensure MIME type lookup uses the > > I think you can move your comment "Added an ..." to be immediately following ": " of the previous line. Done, and same elsewhere. >> Source/WebCore/ChangeLog:24 >> + as it now needs to mutate the created File before returning it. > > Not sure I understand your comment about "out-of-lined" and "mutate". Probably just the first half sentence seems to be enough. Done. >> Source/WebCore/fileapi/File.h:56 >> + static PassRefPtr<File> createWithName(const String& path, const String& name) > > Please add the comment saying that |name| is the filename that is observed by the user, that might be different from the real filename embedded in |path|. > > Or maybe even better, consider rename |name| to something like |observedName|. I'd rather the comment in this case, actually, otherwise the comment is specifying how it will be called, which is really up to the callers. See if you like my comment.
Comment on attachment 94150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94150&action=review > Source/WebCore/fileapi/File.cpp:39 > + const String& nameForMIMEType = !name.isEmpty() ? name : path; Simpler to say: static PassOwnPtr<BlobData> createBlobDataForFile(const String& path, const String& nameForMIMEType = String()) { ... if (nameForMIMEType.isEmpty()) nameForMIMEType = path; > Source/WebCore/fileapi/File.h:56 > + // Create a file whose name, for purposes of MIME type and access through |File.name|, does not match its path. How about something like: // Create a file with different name exposed to the user (File.type and File.name in DOM), that is different from the one provided in the path.
Created attachment 94438 [details] Update comment
Comment on attachment 94150 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=94150&action=review >> Source/WebCore/fileapi/File.cpp:39 >> + const String& nameForMIMEType = !name.isEmpty() ? name : path; > > Simpler to say: > static PassOwnPtr<BlobData> createBlobDataForFile(const String& path, const String& nameForMIMEType = String()) > { > ... > if (nameForMIMEType.isEmpty()) > nameForMIMEType = path; No can do: references can't be re-bound. I _could_ do static PassOwnPtr<BlobData> createBlobDataForFile(const String& path, String nameForMIMEType = String()) { ... if (nameForMIMEType.isEmpty()) nameForMIMEType = path; } But that incurs unnecessary copies. >> Source/WebCore/fileapi/File.h:56 >> + // Create a file whose name, for purposes of MIME type and access through |File.name|, does not match its path. > > How about something like: > // Create a file with different name exposed to the user (File.type and File.name in DOM), that is different from the one provided in the path. Done. Now reads: // Create a file with a name exposed to the author (via File.name and associated DOM properties) that differs from the one provided in the path. (switched "user" for "author", avoided direct mention of MIME type)
Comment on attachment 94438 [details] Update comment Looks good except one minor comment. Please fix it before you land the patch. View in context: https://bugs.webkit.org/attachment.cgi?id=94438&action=review > Source/WebCore/ChangeLog:13 > + (WebCore::DOMFileSystem::createFile): Updated to call createWithName() Please end the sentence with period and do the same thing for other comments.
Created attachment 94445 [details] Patch for landing
Comment on attachment 94445 [details] Patch for landing Clearing flags on attachment: 94445 Committed r87095: <http://trac.webkit.org/changeset/87095>
All reviewed patches have been landed. Closing bug.