diff --git a/connections/implementation/offline_frames_validator.cc b/connections/implementation/offline_frames_validator.cc index 25c7c5f8..a71b4c51 100644 --- a/connections/implementation/offline_frames_validator.cc +++ b/connections/implementation/offline_frames_validator.cc @@ -406,13 +406,6 @@ Exception EnsureValidOfflineFrame( .payload_transfer() .payload_header() .has_file_name()) { - if (offline_frame.v1() - .payload_transfer() - .payload_header() - .file_name() - .empty()) { - return {Exception::kIllegalCharacters}; - } if (CheckForIllegalCharacters(offline_frame.v1() .payload_transfer() .payload_header() @@ -421,8 +414,6 @@ Exception EnsureValidOfflineFrame( kIllegalFileNamePatternsSize)) { return {Exception::kIllegalCharacters}; } - } else { // Filename is empty. - return {Exception::kIllegalCharacters}; } if (offline_frame.v1() .payload_transfer() diff --git a/connections/implementation/offline_frames_validator.h b/connections/implementation/offline_frames_validator.h index 6c7da41e..c3ea3b1d 100644 --- a/connections/implementation/offline_frames_validator.h +++ b/connections/implementation/offline_frames_validator.h @@ -28,8 +28,7 @@ namespace parser { constexpr absl::string_view kIllegalFileNamePatterns[] = {":", "/", "\\"}; -constexpr absl::string_view kIllegalParentFolderPatterns[] = {":", "../", - "..\\"}; +constexpr absl::string_view kIllegalParentFolderPatterns[] = {":", ".."}; const size_t kIllegalFileNamePatternsSize = sizeof(kIllegalFileNamePatterns) / sizeof(*kIllegalFileNamePatterns); diff --git a/connections/implementation/offline_frames_validator_test.cc b/connections/implementation/offline_frames_validator_test.cc index a0574b83..1940c9d0 100644 --- a/connections/implementation/offline_frames_validator_test.cc +++ b/connections/implementation/offline_frames_validator_test.cc @@ -22,7 +22,6 @@ #include "connections/implementation/offline_frames.h" #include "connections/implementation/proto/offline_wire_formats.pb.h" #include "internal/platform/byte_array.h" -#include "internal/platform/logging.h" namespace nearby { namespace connections { @@ -247,7 +246,7 @@ TEST(OfflineFramesValidatorTest, // Sending files larger than 2gb was previously broken (see cl/372382338). // This tests a file larger than int max. header.set_total_size(3e10); - header.set_file_name("file_name.jpg"); + header.set_file_name(std::string()); header.set_parent_folder(std::string()); chunk.set_body("payload data"); chunk.set_offset(150); @@ -288,6 +287,31 @@ TEST(OfflineFramesValidatorTest, ValidatesAsOkTypeFileWithLegalFilePath) { ASSERT_TRUE(ret_value.Ok()); } +TEST(OfflineFramesValidatorTest, ValidatesAsFailedTypeFileWithIllegalFilePath) { + PayloadTransferFrame::PayloadHeader header; + PayloadTransferFrame::PayloadChunk chunk; + header.set_id(12345); + header.set_type(PayloadTransferFrame::PayloadHeader::FILE); + // Sending files larger than 2gb was previously broken (see cl/372382338). + // This tests a file larger than int max. + header.set_total_size(3e10); + header.set_file_name( + std::string("earth_85MB_test (1): (3) (4) (8) (1) (2) (2) (1).jpg")); + header.set_parent_folder(std::string()); + chunk.set_body("payload data"); + chunk.set_offset(150); + chunk.set_flags(1); + + OfflineFrame offline_frame; + + ByteArray bytes = ForDataPayloadTransfer(header, chunk); + offline_frame.ParseFromString(std::string(bytes)); + + auto ret_value = EnsureValidOfflineFrame(offline_frame); + + ASSERT_TRUE(ret_value.value == Exception::kIllegalCharacters); +} + TEST(OfflineFramesValidatorTest, ValidatesAsOkTypeFileWithLegalParentFolder) { PayloadTransferFrame::PayloadHeader header; PayloadTransferFrame::PayloadChunk chunk; @@ -296,7 +320,7 @@ TEST(OfflineFramesValidatorTest, ValidatesAsOkTypeFileWithLegalParentFolder) { // Sending files larger than 2gb was previously broken (see cl/372382338). // This tests a file larger than int max. header.set_total_size(3e10); - header.set_file_name("test_file.txt"); + header.set_file_name(""); header.set_parent_folder(std::string( std::string("earth_85MB_test (1) (3) (4) (8) (1) (2) (2) (1).jpg"))); chunk.set_body("payload data"); @@ -313,6 +337,31 @@ TEST(OfflineFramesValidatorTest, ValidatesAsOkTypeFileWithLegalParentFolder) { ASSERT_TRUE(ret_value.Ok()); } +TEST(OfflineFramesValidatorTest, + ValidatesAsFailedTypeFileWithIllegalParentFolder) { + PayloadTransferFrame::PayloadHeader header; + PayloadTransferFrame::PayloadChunk chunk; + header.set_id(12345); + header.set_type(PayloadTransferFrame::PayloadHeader::FILE); + // Sending files larger than 2gb was previously broken (see cl/372382338). + // This tests a file larger than int max. + header.set_total_size(3e10); + header.set_file_name(""); + header.set_parent_folder(std::string( + std::string("earth_85MB_test (1): (3) (4) (8) (1) (2) (2) (1).jpg"))); + chunk.set_body("payload data"); + chunk.set_offset(150); + chunk.set_flags(1); + + OfflineFrame offline_frame; + + ByteArray bytes = ForDataPayloadTransfer(header, chunk); + offline_frame.ParseFromString(std::string(bytes)); + + auto ret_value = EnsureValidOfflineFrame(offline_frame); + + ASSERT_TRUE(ret_value.value == Exception::kIllegalCharacters); +} TEST(OfflineFramesValidatorTest, ValidatesAsFailWithNullPayloadTransferFrame) { PayloadTransferFrame::PayloadHeader header; PayloadTransferFrame::PayloadChunk chunk; @@ -667,66 +716,6 @@ TEST(OfflineFramesValidatorTest, ASSERT_FALSE(ret_value.Ok()); } -struct FileNameParentFolderTestData { - std::string test_title; - std::string file_name; - std::string parent_folder; - std::string expected; -}; - -using FileNameParentFolderValidationTest = - testing::TestWithParam; - -TEST_P(FileNameParentFolderValidationTest, CheckIllegalFileNameWithParam) { - NEARBY_LOGS(INFO) << "Running FileNameParentFolderValidationTest_" - << GetParam().test_title; - PayloadTransferFrame::PayloadHeader header; - PayloadTransferFrame::PayloadChunk chunk; - header.set_id(12345); - header.set_type(PayloadTransferFrame::PayloadHeader::FILE); - // Sending files larger than 2gb was previously broken (see cl/372382338). - // This tests a file larger than int max. - header.set_total_size(3e10); - header.set_file_name(GetParam().file_name); - header.set_parent_folder(GetParam().parent_folder); - chunk.set_body("payload data"); - chunk.set_offset(150); - chunk.set_flags(1); - - OfflineFrame offline_frame; - - ByteArray bytes = ForDataPayloadTransfer(header, chunk); - offline_frame.ParseFromString(std::string(bytes)); - - auto ret_value = EnsureValidOfflineFrame(offline_frame); - - if (GetParam().expected == "exception") { - ASSERT_TRUE(ret_value.value == Exception::kIllegalCharacters); - } else { - ASSERT_TRUE(ret_value.Ok()); - } -} - -INSTANTIATE_TEST_SUITE_P( - FileNameParentFolderTestInitiation, // This name is only used for - // instantiation - FileNameParentFolderValidationTest, // This is the name of your - // parameterized test - testing::ValuesIn({ - {"CheckLegalFileName", "file_name.jpg", "", "no exception"}, - {"CheckEmptyFileName", "", "", "exception"}, - {"CheckIllegalFileNameColon", "file_name:.jpg", "", "exception"}, - {"CheckIllegalFileNameSlash", "file_name/.jpg", "", "exception"}, - {"CheckIllegalFileNameBackSlash", "file_name\\.jpg", "", "exception"}, - {"CheckLegalParentFolder", "file_name.txt", "parent_/folder", - "no exception"}, - {"CheckIllegalParentFolderColon", "file_name.jpg", "parent:/folder", - "exception"}, - {"CheckIllegalParentFolderDoubleDotSlash", "file_name.txt", - "parent../folder", "exception"}, - {"CheckIllegalParentFolderDoubleDotBackSlash", "file_name.txt", - "parent..\\folder", "exception"}, - })); } // namespace } // namespace parser } // namespace connections diff --git a/internal/platform/implementation/windows/file_path.cc b/internal/platform/implementation/windows/file_path.cc index 0e0c4f09..8a5f9d0a 100644 --- a/internal/platform/implementation/windows/file_path.cc +++ b/internal/platform/implementation/windows/file_path.cc @@ -37,19 +37,16 @@ namespace nearby { namespace windows { -const wchar_t* kUpOneLevel = L"../"; -constexpr wchar_t kEndDot = L'.'; +const wchar_t* kUpOneLevel = L"/.."; constexpr wchar_t kPathDelimiter = L'/'; constexpr wchar_t kReplacementChar = L'_'; constexpr wchar_t kForwardSlash = L'/'; constexpr wchar_t kBackSlash = L'\\'; -const wchar_t kIllegalFileCharacters[] = {L'*', L'?', L'\"', L'<', L'>', L'|'}; -const wchar_t* const kForbiddenPathNames[] = { - L"CON", L"PRN", L"AUX", L"NUL", L"COM1", L"COM2", L"COM3", - L"COM4", L"COM5", L"COM6", L"COM7", L"COM8", L"COM9", L"COM¹", - L"COM²", L"COM³", L"LPT1", L"LPT2", L"LPT3", L"LPT4", L"LPT5", - L"LPT6", L"LPT7", L"LPT8", L"LPT9", L"LPT¹", L"LPT²", L"LPT³"}; +wchar_t const* kForbiddenPathNames[] = { + L"CON", L"PRN", L"AUX", L"NUL", L"COM1", L"COM2", L"COM3", L"COM4", + L"COM5", L"COM6", L"COM7", L"COM8", L"COM9", L"LPT1", L"LPT2", L"LPT3", + L"LPT4", L"LPT5", L"LPT6", L"LPT7", L"LPT8", L"LPT9"}; std::wstring FilePath::GetCustomSavePath(std::wstring parent_folder, std::wstring file_name) { @@ -131,6 +128,7 @@ std::wstring FilePath::CreateOutputFileWithRename(std::wstring path) { std::replace(sanitized_path.begin(), sanitized_path.end(), kBackSlash, kForwardSlash); + // Remove any /..'s SanitizePath(sanitized_path); auto last_delimiter = sanitized_path.find_last_of(kPathDelimiter); @@ -236,21 +234,26 @@ void FilePath::SanitizePath(std::wstring& path) { // If found then erase it from string path.erase(pos, wcslen(kUpOneLevel)); } - while (path[path.size() - 1] == kEndDot) { - // If found then erase it from string - path.erase(path.size() - 1, 1); - } path = MutateForbiddenPathElements(path); ReplaceInvalidCharacters(path); } +char kIllegalFileCharacters[] = {'?', '*', '\'', '<', '>', '|', ':'}; + void FilePath::ReplaceInvalidCharacters(std::wstring& path) { auto it = path.begin(); it += 2; // Skip the 'C:' or any other drive specifier for (; it != path.end(); it++) { + // If 0 < character < 32, it's illegal, replace it + if (*it > 0 && *it < 32) { + NEARBY_LOGS(INFO) << "In path " << wstring_to_string(path) + << " replaced \'" << std::string(1, *it) << "\' with \'" + << std::string(1, kReplacementChar); + *it = kReplacementChar; + } for (auto illegal_character : kIllegalFileCharacters) { if (*it == illegal_character) { NEARBY_LOGS(INFO) << "In path " << wstring_to_string(path) @@ -259,19 +262,6 @@ void FilePath::ReplaceInvalidCharacters(std::wstring& path) { *it = kReplacementChar; } } - if (*it > 0 && - *it < 32) { // If 0 < character < 32, it's illegal, replace it - NEARBY_LOGS(INFO) << "In path " << wstring_to_string(path) - << " replaced \'" << std::string(1, *it) << "\' with \'" - << std::string(1, kReplacementChar); - *it = kReplacementChar; - } - if (*it == 0) { // character is null - NEARBY_LOGS(INFO) << "In path " << wstring_to_string(path) - << " replaced \'NULL\' with \'" - << std::string(1, kReplacementChar); - *it = kReplacementChar; - } } } diff --git a/internal/platform/implementation/windows/file_path_test.cc b/internal/platform/implementation/windows/file_path_test.cc index a7b25abe..2056ceab 100644 --- a/internal/platform/implementation/windows/file_path_test.cc +++ b/internal/platform/implementation/windows/file_path_test.cc @@ -31,16 +31,14 @@ namespace windows { namespace { const wchar_t* kIllegalPathNames[] = { - L"CON", L"PRN", L"AUX", L"NUL", L"COM1", L"COM2", L"COM3", - L"COM4", L"COM5", L"COM6", L"COM7", L"COM8", L"COM9", L"COM¹", - L"COM²", L"COM³", L"LPT1", L"LPT2", L"LPT3", L"LPT4", L"LPT5", - L"LPT6", L"LPT7", L"LPT8", L"LPT9", L"LPT¹", L"LPT²", L"LPT³"}; + L"CON", L"PRN", L"AUX", L"NUL", L"COM1", L"COM2", L"COM3", L"COM4", + L"COM5", L"COM6", L"COM7", L"COM8", L"COM9", L"LPT1", L"LPT2", L"LPT3", + L"LPT4", L"LPT5", L"LPT6", L"LPT7", L"LPT8", L"LPT9"}; const wchar_t* kFileName(L"increment_file_test.txt"); const wchar_t* kFirstIterationFileName(L"/increment_file_test (1).txt"); const wchar_t* kSecondIterationFileName(L"/increment_file_test (2).txt"); const wchar_t* kThirdIterationFileName(L"/increment_file_test (3).txt"); -const wchar_t* kFileNameWithNullReplaced(L"/increment_file_test.txt_.txt"); const wchar_t* kNoDotsFileName(L"incrementfiletesttxt"); const wchar_t* kOneIterationNoDotsFileName(L"/incrementfiletesttxt (1)"); const wchar_t* kMultipleDotsFileName(L"increment.file.test.txt"); @@ -593,7 +591,7 @@ TEST_F(FilePathTests, GetDownloadPath_IllegalFileNameCharacterColon\ ReturnsFileNameWithUnderbarSubstituted) { // char illegal_character_sequence[]{ 0x48, 0x65, 0x6c, 0x6c, 0x6f, 0x2f, // 0x77, 0x6f, 0x72, 0x6c, 0x64, 0x21, 0 }; - auto illegal_character_sequence(L"Test\"Test"); + auto illegal_character_sequence(L"Test:Test"); std::wstring parent_folder(L""); @@ -656,45 +654,6 @@ FileWithIncrementedName) { ASSERT_FALSE(input_file.rdstate() == std::ifstream::goodbit); } -TEST_F(FilePathTests, GetDownloadPath_FileExistsReturns\ -FileWithIncrementedNameWithNull) { - std::wstring file_name(kFileName); - int size = file_name.size(); - file_name.append(L"1.txt"); - file_name[size] = L'\x00'; - std::wstring renamed_file_name(kFileNameWithNullReplaced); - std::wstring parent_folder(L""); - - std::wstring output_file_path(default_download_path_); - output_file_path.append(L"/"); - output_file_path.append(file_name); - - std::wstring expected(default_download_path_); - expected += renamed_file_name; - - std::wifstream input_file; - std::wofstream output_file; - - output_file.open(output_file_path, - std::ofstream::binary | std::ofstream::out); - - ASSERT_TRUE(output_file.rdstate() == std::ofstream::goodbit); - - output_file.close(); - - auto actual(FilePath::GetDownloadPath(parent_folder, file_name)); - - EXPECT_EQ(actual, expected); - - // Remove the file and check that it is removed - // File 1 - _wremove(output_file_path.c_str()); - - input_file.open(output_file_path, std::ifstream::binary | std::ifstream::in); - - ASSERT_FALSE(input_file.rdstate() == std::ifstream::goodbit); -} - TEST_F(FilePathTests, GetDownloadPath_MultipleFilesExist\ ReturnsNextIncrementedFileName) { std::ofstream output_file;