Refactor/bug fixing 1

CHANGELOG.md

@@ -1,5 +1,15 @@
 # Changelog
 
+## 0.9.9 - 2026-04-05
+
+### Fixed
+
+- Potential path traversal attack in extract subcommand
+- Empty MPQ archive name when directory supplied with trailing slash
+- Bug where MPQ internal files were added with create command
+- Signature being added automatically when not requested
+- Potential DWORD overflow in `AddFiles`
+
 ## 0.9.8 - 2026-03-22
 
 ### Added

CMakeLists.txt

@@ -1,6 +1,6 @@
 cmake_minimum_required(VERSION 3.10)
 
-project(MPQCLI VERSION 0.9.8)
+project(MPQCLI VERSION 0.9.9)
 
 # Options
 option(BUILD_MPQCLI "Build the mpqcli CLI app" ON)

README.md

@@ -4,7 +4,7 @@
 
 ![Release Version](https://img.shields.io/github/v/release/TheGrayDot/mpqcli?style=flat)
 
-![Release downloads](https://img.shields.io/github/downloads/thegraydot/mpqcli/total?label=release_downloads) ![Package downloads](https://img.shields.io/badge/package_downloads-262-green)
+![Release downloads](https://img.shields.io/github/downloads/thegraydot/mpqcli/total?label=release_downloads) ![Package downloads](https://img.shields.io/badge/package_downloads-353-green)
 
 A command-line tool to create, add, remove, list, extract, read, and verify MPQ archives using the [StormLib library](https://github.com/ladislav-zezula/StormLib).
 

src/gamerules.h

@@ -86,7 +86,7 @@ struct MpqCreateSettings {
           streamFlags(STREAM_PROVIDER_FLAT | BASE_PROVIDER_FILE),
           fileFlags1(MPQ_FILE_DEFAULT_INTERNAL),
           fileFlags2(0),
-          fileFlags3(MPQ_FILE_DEFAULT_INTERNAL),
+          fileFlags3(0),
           attrFlags(0),
           sectorSize(0x1000),
           rawChunkSize(0) {}

src/main.cpp

@@ -245,6 +245,11 @@ int main(int argc, char **argv) {
             outputFilePath = fs::absolute(baseOutput);
         } else {
             outputFilePath = fs::path(baseTarget);
+            // If the path ends with a separator (e.g. "dir/"), strip the
+            // trailing separator first so we get "dir.mpq"
+            if (outputFilePath.filename().empty()) {
+                outputFilePath = outputFilePath.parent_path();
+            }
             outputFilePath.replace_extension(".mpq");
         }
         std::string outputFile = outputFilePath.u8string();

src/mpq.cpp

@@ -15,6 +15,12 @@
 
 namespace fs = std::filesystem;
 
+static const std::vector<std::string> kSpecialMpqFiles = {
+    "(listfile)",
+    "(signature)",
+    "(attributes)"
+};
+
 int OpenMpqArchive(const std::string &filename, HANDLE *hArchive, int32_t flags) {
     if (!SFileOpenArchive(filename.c_str(), 0, flags, hArchive)) {
         std::cerr << "[!] Failed to open: " << filename << std::endl;
@@ -113,6 +119,18 @@ int ExtractFile(HANDLE hArchive, const std::string& output, const std::string& f
 
     // Ensure sub-directories for folder-nested files exist
     fs::path outputFilePathName = outputPathBase / fileNameString;
+
+    // Guard against path traversal attacks: resolve any ".." components and verify
+    // the output path is a descendant of the intended base directory
+    fs::path resolvedOutput = fs::weakly_canonical(outputFilePathName);
+    if (std::mismatch(outputPathBase.begin(), outputPathBase.end(),
+                      resolvedOutput.begin(), resolvedOutput.end()).first
+        != outputPathBase.end()) {
+        std::cerr << "[!] Blocked: path traversal attempt detected: "
+                  << fileNameString << std::endl;
+        return 1;
+    }
+
     std::string outputFileName{outputFilePathName.u8string()};
     std::filesystem::create_directories(outputFilePathName.parent_path());
 
@@ -185,6 +203,12 @@ int AddFiles(HANDLE hArchive, const std::string& inputPath, LCID locale, const G
             // Normalise path for MPQ
             std::string archiveFilePath = WindowsifyFilePath(inputFilePath.u8string());
 
+            // Skip special MPQ files that StormLib manages automatically
+            if (std::find(kSpecialMpqFiles.begin(), kSpecialMpqFiles.end(), archiveFilePath) != kSpecialMpqFiles.end()) {
+                std::cout << "[*] Skipping special MPQ file: " << archiveFilePath << std::endl;
+                continue;
+            }
+
             AddFile(hArchive, entry.path().u8string(), archiveFilePath, locale, gameRules, overrides, false);
         }
     }
@@ -239,7 +263,13 @@ int AddFile(
     }
 
     // Get file size for rule matching
-    const auto fileSize = static_cast<DWORD>(fs::file_size(localFile));
+    const std::uintmax_t rawFileSize = fs::file_size(localFile);
+    if (rawFileSize > std::numeric_limits<DWORD>::max()) {
+        std::cerr << "[!] Warning: file exceeds 4GB, size-based compression rules may not apply correctly: "
+                  << localFile << std::endl;
+    }
+    const DWORD fileSize = static_cast<DWORD>(
+        std::min(rawFileSize, static_cast<std::uintmax_t>(std::numeric_limits<DWORD>::max())));
 
     // Get game-specific rules
     auto [flags, compressionFirst, compressionNext] =
@@ -334,19 +364,11 @@ int ListFiles(HANDLE hArchive, const std::string& listfileName, bool listAll, bo
         listDetailed = true; // If the user specified properties, we need to print the detailed output
     }
 
-    // "Special" files are base files used by MPQ file format
-    // These are skipped, unless "-a" or "--all" are specified
-    std::vector<std::string> specialFiles = {
-        "(listfile)",
-        "(signature)",
-        "(attributes)"
-    };
-
     std::set<std::string> seenFileNames; // Used to prevent printing the same file name multiple times
     // Loop through all files in the MPQ archive
     do {
         // Skip special files unless user wants to list all (like ls -a)
-        if (!listAll && std::find(specialFiles.begin(), specialFiles.end(), findData.cFileName) != specialFiles.end()) {
+        if (!listAll && std::find(kSpecialMpqFiles.begin(), kSpecialMpqFiles.end(), findData.cFileName) != kSpecialMpqFiles.end()) {
             continue;
         }
 

test/conftest.py

@@ -167,6 +167,44 @@ def generate_mpq_without_internal_listfile(binary_path):
             assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}"
 
 
+@pytest.fixture(scope="function")
+def generate_path_traversal_mpq(binary_path):
+    script_dir = Path(__file__).parent
+
+    data_dir = script_dir / "data"
+    data_dir.mkdir(parents=True, exist_ok=True)
+
+    traversal_files_dir = data_dir / "traversal_files"
+    shutil.rmtree(traversal_files_dir, ignore_errors=True)
+    traversal_files_dir.mkdir(parents=True, exist_ok=True)
+
+    mpq_file = data_dir / "mpq_with_path_traversal.mpq"
+    mpq_file.unlink(missing_ok=True)
+
+    safe_file = traversal_files_dir / "safe.txt"
+    safe_file.write_text("This is a safe file.\n", newline="\n")
+
+    # Create a base MPQ containing the safe file
+    result = subprocess.run(
+        [str(binary_path), "create", "-o", str(mpq_file), str(traversal_files_dir)],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        text=True
+    )
+    assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}"
+
+    # Embed a second entry whose archive path traverses above the output directory
+    result = subprocess.run(
+        [str(binary_path), "add", str(safe_file), str(mpq_file), "-p", "../../sneaky.txt"],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        text=True
+    )
+    assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}"
+
+    yield mpq_file
+
+
 @pytest.fixture(scope="session")
 def download_test_files():
     script_dir = Path(__file__).parent

test/test_create.py

@@ -536,6 +536,137 @@ def test_deletion_marker_only_for_zero_size_files(binary_path):
         output_file.unlink(missing_ok=True)
 
 
+def test_create_mpq_no_sign_flag_has_no_signature(binary_path, generate_test_files):
+    """
+    Test that a newly created archive without --sign does not have a weak signature slot.
+
+    This test checks:
+    - If the MPQ archive is created successfully.
+    - If the signature type is None (no signature slot pre-allocated).
+    """
+    _ = generate_test_files
+    script_dir = Path(__file__).parent
+    target_dir = script_dir / "data" / "files"
+
+    output_file = script_dir / "data" / "mpq_no_sign.mpq"
+    output_file.unlink(missing_ok=True)
+
+    result = subprocess.run(
+        [str(binary_path), "create", str(target_dir), "-o", str(output_file)],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        text=True
+    )
+
+    assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}"
+    assert output_file.exists(), "MPQ file was not created"
+
+    info = subprocess.run(
+        [str(binary_path), "info", "-p", "signature-type", str(output_file)],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        text=True
+    )
+
+    assert info.returncode == 0, f"mpqcli info failed with error: {info.stderr}"
+    assert info.stdout.strip() == "None", f"Expected signature type 'None', got: {info.stdout.strip()!r}"
+
+    output_file.unlink(missing_ok=True)
+
+
+def test_create_mpq_directory_with_trailing_slash(binary_path, generate_test_files):
+    """
+    Test MPQ archive creation when the target directory has a trailing slash.
+
+    Regression test for: mpqcli create dir/ producing "dir/.mpq" instead of "dir.mpq".
+
+    This test checks:
+    - The archive is named after the directory, not "<dir>/.mpq".
+    - The archive is created successfully and is non-empty.
+    """
+    _ = generate_test_files
+    script_dir = Path(__file__).parent
+    # Construct the path string with an explicit trailing slash
+    target_dir_str = str(script_dir / "data" / "files") + "/"
+
+    expected_output = script_dir / "data" / "files.mpq"
+    malformed_output = script_dir / "data" / "files" / ".mpq"
+
+    # Clean up from any previous run
+    expected_output.unlink(missing_ok=True)
+    malformed_output.unlink(missing_ok=True)
+
+    result = subprocess.run(
+        [str(binary_path), "create", target_dir_str],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        text=True
+    )
+
+    assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}"
+    assert expected_output.exists(), (
+        f"Expected archive '{expected_output}' was not created. "
+        f"Malformed path exists: {malformed_output.exists()}"
+    )
+    assert expected_output.stat().st_size > 0, "MPQ file is empty"
+    assert not malformed_output.exists(), (
+        f"Malformed archive path '{malformed_output}' was created — trailing slash bug is present"
+    )
+
+    expected_output.unlink(missing_ok=True)
+
+
+def test_create_mpq_skips_special_files(binary_path, tmp_path):
+    """
+    Test that special MPQ files in the source directory are skipped during archive creation.
+
+    This test checks:
+    - That (listfile), (signature), and (attributes) are not added as regular files.
+    - That the signature type is None after creation (no false signature from (signature) file).
+    - That the special files do not appear in the archive file listing.
+    """
+    source_dir = tmp_path / "src"
+    source_dir.mkdir()
+    (source_dir / "readme.txt").write_text("hello")
+
+    # Plant all three special files in the source directory
+    for name in ["(listfile)", "(signature)", "(attributes)"]:
+        (source_dir / name).write_bytes(b"fake content")
+
+    output_file = tmp_path / "output.mpq"
+
+    result = subprocess.run(
+        [str(binary_path), "create", str(source_dir), "-o", str(output_file)],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        text=True
+    )
+
+    assert result.returncode == 0, f"mpqcli failed with error: {result.stderr}"
+    assert output_file.exists(), "MPQ file was not created"
+
+    # Signature type must be None — (signature) must not have been ingested
+    info = subprocess.run(
+        [str(binary_path), "info", "-p", "signature-type", str(output_file)],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        text=True
+    )
+    assert info.returncode == 0, f"mpqcli info failed with error: {info.stderr}"
+    assert info.stdout.strip() == "None", f"Expected signature type 'None', got: {info.stdout.strip()!r}"
+
+    # Special files must not appear in the regular file listing
+    listing = subprocess.run(
+        [str(binary_path), "list", str(output_file)],
+        stdout=subprocess.PIPE,
+        stderr=subprocess.PIPE,
+        text=True
+    )
+    assert listing.returncode == 0, f"mpqcli list failed with error: {listing.stderr}"
+    for name in ["(listfile)", "(signature)", "(attributes)"]:
+        assert name not in listing.stdout, f"Special file {name!r} unexpectedly found in archive listing"
+
+
 def verify_archive_file_content(binary_path, test_file, expected_output):
     result = subprocess.run(
         [str(binary_path), "list", str(test_file), "-d", "-p", "locale"],