From 76bee39a9682bc9c637ea26040e71d6e1229a8af Mon Sep 17 00:00:00 2001 From: Neil Deakin Date: Mon, 6 May 2024 17:16:31 +0000 Subject: [PATCH] Bug 1891234, additional filename filter checks, r=Gijs,extension-reviewers,robwu Differential Revision: https://phabricator.services.mozilla.com/D208659 --- .../xpcshell/test_ext_downloads_urlencoded.js | 2 +- .../exthandler/nsExternalHelperAppService.cpp | 10 ++++- .../tests/mochitest/save_filenames.html | 6 +-- .../test_invalidCharFileExtension.xhtml | 2 +- .../tests/unit/test_filename_sanitize.js | 41 ++++++++++--------- xpcom/base/nsCRTGlue.h | 4 +- 6 files changed, 37 insertions(+), 28 deletions(-) diff --git a/toolkit/components/extensions/test/xpcshell/test_ext_downloads_urlencoded.js b/toolkit/components/extensions/test/xpcshell/test_ext_downloads_urlencoded.js index ae40faf9093a..8532ce4a9931 100644 --- a/toolkit/components/extensions/test/xpcshell/test_ext_downloads_urlencoded.js +++ b/toolkit/components/extensions/test/xpcshell/test_ext_downloads_urlencoded.js @@ -83,7 +83,7 @@ add_task(async function test_decoded_filename_download() { const FILE_NAME_DECODED_2 = "file\u{0001F6B2}encoded.txt"; const FILE_NAME_ENCODED_URL_2 = BASE + "/" + FILE_NAME_ENCODED_2; const FILE_NAME_ENCODED_3 = "file%X%20encode.txt"; - const FILE_NAME_DECODED_3 = "file%X encode.txt"; + const FILE_NAME_DECODED_3 = "file_X encode.txt"; const FILE_NAME_ENCODED_URL_3 = BASE + "/" + FILE_NAME_ENCODED_3; const FILE_NAME_ENCODED_4 = "file%E3%80%82encode.txt"; const FILE_NAME_DECODED_4 = "file\u3002encode.txt"; diff --git a/uriloader/exthandler/nsExternalHelperAppService.cpp b/uriloader/exthandler/nsExternalHelperAppService.cpp index 4573e28470c5..dea4a8a2139c 100644 --- a/uriloader/exthandler/nsExternalHelperAppService.cpp +++ b/uriloader/exthandler/nsExternalHelperAppService.cpp @@ -3487,7 +3487,7 @@ void nsExternalHelperAppService::SanitizeFileName(nsAString& aFileName, // Replace known invalid characters. fileName.ReplaceChar(u"" KNOWN_PATH_SEPARATORS, u'_'); - fileName.ReplaceChar(u"" FILE_ILLEGAL_CHARACTERS, u' '); + fileName.ReplaceChar(u"" FILE_ILLEGAL_CHARACTERS, u'_'); fileName.StripChar(char16_t(0)); const char16_t *startStr, *endStr; @@ -3669,6 +3669,14 @@ void nsExternalHelperAppService::SanitizeFileName(nsAString& aFileName, outFileName.Truncate(lastNonTrimmable); } + nsAutoString extension; + int32_t dotidx = outFileName.RFind(u"."); + if (dotidx != -1) { + extension = Substring(outFileName, dotidx + 1); + extension.StripWhitespace(); + outFileName = Substring(outFileName, 0, dotidx + 1) + extension; + } + #ifdef XP_WIN if (nsLocalFile::CheckForReservedFileName(outFileName)) { outFileName.Truncate(); diff --git a/uriloader/exthandler/tests/mochitest/save_filenames.html b/uriloader/exthandler/tests/mochitest/save_filenames.html index 8755186206e6..ec307e2d8110 100644 --- a/uriloader/exthandler/tests/mochitest/save_filenames.html +++ b/uriloader/exthandler/tests/mochitest/save_filenames.html @@ -18,7 +18,7 @@ - + @@ -329,11 +329,11 @@ + data-pickedfilename='"peach".png' data-filename='_peach_.png'> + data-pickedfilename='"violet".png' data-filename="_violet_.png"> diff --git a/uriloader/exthandler/tests/mochitest/test_invalidCharFileExtension.xhtml b/uriloader/exthandler/tests/mochitest/test_invalidCharFileExtension.xhtml index 177af3757f1a..d7c796589fad 100644 --- a/uriloader/exthandler/tests/mochitest/test_invalidCharFileExtension.xhtml +++ b/uriloader/exthandler/tests/mochitest/test_invalidCharFileExtension.xhtml @@ -11,7 +11,7 @@ var tests = [ ["test.png:large", "test.png"], ["test.png/large", "test.png"], - [":test.png::large:", "test.png"], + [":test.png::large:", "_test.png"], ]; add_task(async function() { diff --git a/uriloader/exthandler/tests/unit/test_filename_sanitize.js b/uriloader/exthandler/tests/unit/test_filename_sanitize.js index d8ab12c266a0..2cc0f2844aa0 100644 --- a/uriloader/exthandler/tests/unit/test_filename_sanitize.js +++ b/uriloader/exthandler/tests/unit/test_filename_sanitize.js @@ -31,8 +31,9 @@ add_task(async function validate_filename_method() { Assert.equal(checkFilename("\\path.png", 0), "_path.png"); Assert.equal( checkFilename("\\path*and/$?~file.png", 0), - "_path and_$ ~file.png" + "_path_and_$_~file.png" ); + Assert.equal( checkFilename(" \u180e whit\u180ee.png \u180e", 0), "whit\u180ee.png" @@ -103,12 +104,12 @@ add_task(async function validate_filename_method() { // For whatever reason, the Android mime handler accepts the .jpeg // extension for image/png, so skip this test there. if (AppConstants.platform != "android") { - Assert.equal(checkFilename("thi/*rd.jpeg", 0), "thi_ rd.png"); + Assert.equal(checkFilename("thi/*rd.jpeg", 0), "thi__rd.png"); } Assert.equal( checkFilename("f*\\ourth file.jpg", mimeService.VALIDATE_SANITIZE_ONLY), - "f _ourth file.jpg" + "f__ourth file.jpg" ); Assert.equal( checkFilename( @@ -116,7 +117,7 @@ add_task(async function validate_filename_method() { mimeService.VALIDATE_SANITIZE_ONLY | mimeService.VALIDATE_DONT_COLLAPSE_WHITESPACE ), - "f _ift h.jpe _g" + "f__ift h.jpe__g" ); Assert.equal(checkFilename("sixth.j pe/*g", 0), "sixth.png"); @@ -157,25 +158,25 @@ add_task(async function validate_filename_method() { repeatStr.substring(0, 254 - ext.length) + ext ); - ext = "lo%?n/ginvalid? ch\\ars"; + ext = "lo#?n/ginvalid? ch\\ars"; Assert.equal( checkFilename(repeatStr + ext, mimeService.VALIDATE_SANITIZE_ONLY), - repeatStr + "lo% n_" + repeatStr + "lo#_n_" ); - ext = ".long/invalid%? ch\\ars"; + ext = ".long/invalid#? ch\\ars"; Assert.equal( checkFilename(repeatStr + ext, mimeService.VALIDATE_SANITIZE_ONLY), - repeatStr.substring(0, 233) + ".long_invalid% ch_ars" + repeatStr.substring(0, 232) + ".long_invalid#_ch_ars" ); Assert.equal( checkFilename("test_ﾃｽﾄ_T\x83E\\S\x83T.png", 0), - "test_ﾃｽﾄ_T E_S T.png" + "test_ﾃｽﾄ_T_E_S_T.png" ); Assert.equal( checkFilename("test_ﾃｽﾄ_T\x83E\\S\x83T.pﾃ\x83ng", 0), - "test_ﾃｽﾄ_T E_S T.png" + "test_ﾃｽﾄ_T_E_S_T.png" ); // Check we don't invalidate surrogate pairs when trimming. @@ -248,11 +249,11 @@ add_task(async function validate_filename_method() { // cropped to fit into 255 bytes. Assert.equal( mimeService.validateFileNameForSaving( - "라이브9.9만 시청컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장24%102 000원 브랜드데이 앵콜 🎁 1.등 유산균 컬처렐 특가!", + "라이브9.9만 시청컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장24%102 000원 브랜드데이 앵콜 🎁 1.등-유산균-컬처렐-특가!", "text/unknown", mimeService.VALIDATE_SANITIZE_ONLY ), - "라이브9.9만 시청컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 .등 유산균 컬처렐 특가!", + "라이브9.9만 시청컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 .등-유산균-컬처렐-특가!", "very long filename with extension" ); @@ -270,11 +271,11 @@ add_task(async function validate_filename_method() { // This filename is cropped at 254 bytes. Assert.equal( mimeService.validateFileNameForSaving( - ".라이브99만 시청컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장24%102 000원 브랜드데이 앵콜 🎁 1등 유산균 컬처렐 특가!", + ".라이브99만 시청컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장24_102 000원 브랜드데이 앵콜 🎁 1등 유산균 컬처렐 특가!", "text/unknown", mimeService.VALIDATE_SANITIZE_ONLY ), - "라이브99만 시청컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장24%102 000원 브랜드데", + "라이브99만 시청컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장컬처렐 다이제스티브 3박스 - 3박스 더 (뚱랑이 굿즈 증정) - 선물용 쇼핑백 2장24_102 000원 브랜드데", "very filename with extension only" ); @@ -311,7 +312,7 @@ add_task(async function validate_filename_method() { Assert.equal( mimeService.validateFileNameForSaving("filename.lnk\n", "text/unknown", 0), - "filename.lnk.download", + "filename.lnk_", "filename.lnk with newline" ); @@ -321,7 +322,7 @@ add_task(async function validate_filename_method() { "text/unknown", 0 ), - "filename.lnk.download", + "filename.lnk_", "filename.lnk with newline" ); @@ -331,7 +332,7 @@ add_task(async function validate_filename_method() { "text/unknown", 0 ), - "filename. lnk", + "filename.__lnk", "filename.lnk with space and newline" ); @@ -361,7 +362,7 @@ add_task(async function validate_filename_method() { "text/unknown", mimeService.VALIDATE_ALLOW_INVALID_FILENAMES ), - "filename.LNK", + "filename.LNK_", "filename.LNK allow invalid" ); @@ -372,7 +373,7 @@ add_task(async function validate_filename_method() { mimeService.VALIDATE_SANITIZE_ONLY | mimeService.VALIDATE_ALLOW_INVALID_FILENAMES ), - "filename.URL", + "filename.URL_", "filename.URL allow invalid, sanitize only" ); @@ -392,7 +393,7 @@ add_task(async function validate_filename_method() { mimeService.VALIDATE_SANITIZE_ONLY | mimeService.VALIDATE_ALLOW_INVALID_FILENAMES ), - "filename.DESKTOP", + "filename.DESKTOP_", "filename.DESKTOP allow invalid, sanitize only" ); }); diff --git a/xpcom/base/nsCRTGlue.h b/xpcom/base/nsCRTGlue.h index 2bfe47b2a8e1..f97029538253 100644 --- a/xpcom/base/nsCRTGlue.h +++ b/xpcom/base/nsCRTGlue.h @@ -132,11 +132,11 @@ void NS_MakeRandomString(char* aBuf, int32_t aBufLen); #if defined(ANDROID) // On mobile devices, the file system may be very limited in what it // considers valid characters. To avoid errors, sanitize conservatively. -# define OS_FILE_ILLEGAL_CHARACTERS "/:*?\"<>|;,+=[]" +# define OS_FILE_ILLEGAL_CHARACTERS "/:*%?\"<>|;,+=[]" #else // Otherwise, we use the most restrictive filesystem as our default set of // illegal filename characters. This is currently Windows. -# define OS_FILE_ILLEGAL_CHARACTERS "/:*?\"<>|" +# define OS_FILE_ILLEGAL_CHARACTERS "/:*%?\"<>|" #endif // We also provide a list of all known file path separators for all filesystems.