diff --git a/CMakeLists.txt b/CMakeLists.txt index ca498cfeab..185f636af7 100644 --- a/CMakeLists.txt +++ b/CMakeLists.txt @@ -39,6 +39,7 @@ else() add_subdirectory( "${CORE_ROOT_DIR}/OfficeUtils/tests" officeutils_test ) add_subdirectory( "${CORE_ROOT_DIR}/OdfFile/Reader/Converter/SMCustomShape2OOXML/TestSMCustomShape" starmath_smcustomshape_test ) add_subdirectory( "${CORE_ROOT_DIR}/OdfFile/Test/test_odf" test_odf ) + add_subdirectory( "${CORE_ROOT_DIR}/DesktopEditor/common/test" path_test ) endif() endif() diff --git a/DesktopEditor/common/Path.cpp b/DesktopEditor/common/Path.cpp index de30236a51..c3344e5bb7 100644 --- a/DesktopEditor/common/Path.cpp +++ b/DesktopEditor/common/Path.cpp @@ -213,7 +213,7 @@ namespace NSSystemPath if (L".." == wsToken) { - if (!arStack.empty() && L".." == arStack.top()) + if (!arStack.empty() && L".." != arStack.top()) arStack.pop(); else arStack.push(wsToken); @@ -237,6 +237,12 @@ namespace NSSystemPath arStack.pop(); } + // wsNewPath stays empty when the loop above breaks on the first entry + // (bRemoveExternalPath with a leading ".."), and pop_back() on an empty + // string is undefined behaviour. + if (wsNewPath.empty()) + return std::wstring(); + wsNewPath.pop_back(); #if !defined(_WIN32) && !defined(_WIN64) diff --git a/DesktopEditor/common/test/CMakeLists.txt b/DesktopEditor/common/test/CMakeLists.txt new file mode 100644 index 0000000000..9000983969 --- /dev/null +++ b/DesktopEditor/common/test/CMakeLists.txt @@ -0,0 +1,19 @@ +cmake_minimum_required(VERSION 3.10) + +project(path_test) + +set(CORE_ROOT_DIR "${CMAKE_CURRENT_LIST_DIR}/../../..") + +include(${CORE_ROOT_DIR}/common.cmake) + +# NSSystemPath is part of kernel. +if(NOT TARGET kernel) + add_subdirectory(${CORE_ROOT_DIR}/Common kernel) +endif() + +add_core_gtest( + NAME path_test + SOURCES path.cpp + LIBS kernel + GTEST_MAIN +) diff --git a/DesktopEditor/common/test/path.cpp b/DesktopEditor/common/test/path.cpp new file mode 100644 index 0000000000..e4c9a5f0cc --- /dev/null +++ b/DesktopEditor/common/test/path.cpp @@ -0,0 +1,77 @@ +#include "gtest/gtest.h" + +#include "../Path.h" + +// ShortenPath resolves "." and ".." lexically, without touching the filesystem. +// Its result is used as a containment check by every one of its callers: the +// ZipSlip guard in OfficeUtils/src/ZipUtilsCP.cpp, the "../" rejection in +// EpubFile/src/CEpubFile.cpp, and several starts_with(root) checks in HtmlFile2, +// OFDFile and the SVG image loader. It therefore decides whether a path taken +// from a document is accepted, which is why it is worth pinning precisely. + +TEST(ShortenPath, keeps_a_plain_relative_path) +{ + EXPECT_EQ(L"a/b/c", NSSystemPath::ShortenPath(L"a/b/c")); +} + +TEST(ShortenPath, resolves_a_parent_reference_in_the_middle) +{ + EXPECT_EQ(L"b", NSSystemPath::ShortenPath(L"a/../b")); +} + +// --- defects reproduced below --- + +TEST(ShortenPath, resolves_a_trailing_parent_reference) +{ + EXPECT_EQ(L"", NSSystemPath::ShortenPath(L"a/..")); +} + +TEST(ShortenPath, keeps_consecutive_parent_references) +{ + EXPECT_EQ(L"../..", NSSystemPath::ShortenPath(L"../..")); +} + +TEST(ShortenPath, does_not_crash_on_bare_parent_when_removing_external_path) +{ + EXPECT_EQ(L"", NSSystemPath::ShortenPath(L"..", true)); +} + +// --- guards: behaviour that must not change (these pass before the fix too) --- + +TEST(ShortenPath, keeps_a_leading_parent_chain) +{ + EXPECT_EQ(L"../../etc/passwd", NSSystemPath::ShortenPath(L"../../etc/passwd")); +} + +TEST(ShortenPath, keeps_an_absolute_path) +{ + EXPECT_EQ(L"/tmp/evil.txt", NSSystemPath::ShortenPath(L"/tmp/evil.txt")); +} + +TEST(ShortenPath, returns_empty_for_empty_input) +{ + EXPECT_EQ(L"", NSSystemPath::ShortenPath(L"")); +} + +TEST(ShortenPath, drops_current_directory_segments) +{ + EXPECT_EQ(L"a/b", NSSystemPath::ShortenPath(L"./a/./b")); +} + +// Archive entries and HTML sources routinely use backslashes; the callers rely on +// them being treated as separators rather than as part of a name. +TEST(ShortenPath, treats_a_backslash_as_a_separator) +{ + EXPECT_EQ(L"b", NSSystemPath::ShortenPath(L"a\\..\\b")); +} + +TEST(ShortenPath, collapses_repeated_and_trailing_separators) +{ + EXPECT_EQ(L"a/b", NSSystemPath::ShortenPath(L"a//b")); + EXPECT_EQ(L"a/b", NSSystemPath::ShortenPath(L"a/b/")); +} + +TEST(ShortenPath, drops_the_external_prefix_when_asked) +{ + EXPECT_EQ(L"a", NSSystemPath::ShortenPath(L"../a", true)); +}