Skip to content

Sanitize download filenames and prevent traversal #251

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Draft
patchzyy wants to merge 1 commit into
base: dev
Choose a base branch
from
Draft

Sanitize download filenames and prevent traversal #251

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 33 additions & 2 deletions WheelWizard/Helpers/DownloadHelper.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -78,8 +78,9 @@ public static class DownloadHelper

// Check for filename in Content-Disposition or fallback to URL
var contentDisposition = response.Content.Headers.ContentDisposition;
var fileName = contentDisposition?.FileName?.Trim('"') ?? Path.GetFileName(new Uri(url).AbsolutePath);
fileName = Path.ChangeExtension(fileName, Path.GetExtension(finalUrl));
var fileName =
contentDisposition?.FileNameStar ?? contentDisposition?.FileName ?? Path.GetFileName(new Uri(url).AbsolutePath);
fileName = GetSafeDownloadFileName(fileName, finalUrl, url);

// Add extension if missing in file path
if (!Path.HasExtension(fileName))
Expand All @@ -93,6 +94,7 @@ public static class DownloadHelper

// Update resolvedFilePath with resolved fileName
resolvedFilePath = Path.Combine(directory, fileName);
EnsurePathStaysWithinDirectory(resolvedFilePath, directory);
}

var totalBytes = response.Content.Headers.ContentLength ?? -1;
Expand Down Expand Up @@ -199,4 +201,33 @@ public static class DownloadHelper
progressPopupWindow.SetCancellationTokenSource(null);
}
}

private static string GetSafeDownloadFileName(string fileName, string finalUrl, string originalUrl)
{
var trimmedName = fileName.Trim().Trim('"');
if (string.IsNullOrWhiteSpace(trimmedName))
{
trimmedName = Path.GetFileName(new Uri(originalUrl).AbsolutePath);
}

// Only allow a basename from remote input, never a relative or absolute path.
var safeFileName = Path.GetFileName(trimmedName.Replace('\\', '/'));
if (string.IsNullOrWhiteSpace(safeFileName))
throw new InvalidOperationException("The server returned an invalid download filename.");

var finalExtension = Path.GetExtension(finalUrl);
if (!string.IsNullOrWhiteSpace(finalExtension))
safeFileName = Path.ChangeExtension(safeFileName, finalExtension);
Comment on lines +218 to +220
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🟠 Major

🧩 Analysis chain

🏁 Script executed:

# First, locate and read the DownloadHelper.cs file
find . -name "DownloadHelper.cs" -type f

Repository: TeamWheelWizard/WheelWizard

Length of output: 110

🏁 Script executed:

# Read the relevant section of DownloadHelper.cs
cat -n WheelWizard/Helpers/DownloadHelper.cs | sed -n '200,230p'

Repository: TeamWheelWizard/WheelWizard

Length of output: 1674

🏁 Script executed:

# Test Path.GetExtension behavior with full URLs
cat > /tmp/test_extension.cs << 'EOF'
using System;
using System.IO;

// Test Path.GetExtension behavior with full URLs
var testCases = new[]
{
    "https://example.com/download.php?id=1",
    "https://example.com/download.php?id=1&file=mod.zip",
    "/download.php?id=1",
    "https://example.com/files/mod.zip",
    "https://example.com/files/mod.zip?token=abc",
    "mod.zip"
};

Console.WriteLine("Path.GetExtension() results:");
foreach (var testCase in testCases)
{
    var ext = Path.GetExtension(testCase);
    Console.WriteLine($"  Input: {testCase}");
    Console.WriteLine($"  Extension: '{ext}'");
    Console.WriteLine();
}

Console.WriteLine("\nUri.AbsolutePath results:");
foreach (var testCase in testCases)
{
    try
    {
        if (testCase.StartsWith("http"))
        {
            var uri = new Uri(testCase);
            var ext = Path.GetExtension(uri.AbsolutePath);
            Console.WriteLine($"  Input: {testCase}");
            Console.WriteLine($"  AbsolutePath: {uri.AbsolutePath}");
            Console.WriteLine($"  Extension: '{ext}'");
            Console.WriteLine();
        }
    }
    catch (Exception ex)
    {
        Console.WriteLine($"  Error with {testCase}: {ex.Message}");
    }
}
EOF

# Try to run with dotnet if available, otherwise use csharp interactive
dotnet script /tmp/test_extension.cs 2>/dev/null || csharp << 'EOFCSHARP'
using System;
using System.IO;

// Test Path.GetExtension behavior with full URLs
var testCases = new[]
{
    "https://example.com/download.php?id=1",
    "https://example.com/download.php?id=1&file=mod.zip",
    "/download.php?id=1",
    "https://example.com/files/mod.zip",
    "https://example.com/files/mod.zip?token=abc",
    "mod.zip"
};

Console.WriteLine("Path.GetExtension() results:");
foreach (var testCase in testCases)
{
    var ext = Path.GetExtension(testCase);
    Console.WriteLine($"  Input: {testCase}");
    Console.WriteLine($"  Extension: '{ext}'");
    Console.WriteLine();
}

Console.WriteLine("\nUri.AbsolutePath results:");
foreach (var testCase in testCases)
{
    try
    {
        if (testCase.StartsWith("http"))
        {
            var uri = new Uri(testCase);
            var ext = Path.GetExtension(uri.AbsolutePath);
            Console.WriteLine($"  Input: {testCase}");
            Console.WriteLine($"  AbsolutePath: {uri.AbsolutePath}");
            Console.WriteLine($"  Extension: '{ext}'");
            Console.WriteLine();
        }
    }
    catch (Exception ex)
    {
        Console.WriteLine($"  Error with {testCase}: {ex.Message}");
    }
}
EOFCSHARP

Repository: TeamWheelWizard/WheelWizard

Length of output: 118

🏁 Script executed:

# Use Python to demonstrate the behavior since we can't easily run C#
python3 << 'EOFPYTHON'
import urllib.parse
from pathlib import Path as PathlibPath

# Simulate Path.GetExtension behavior
def path_get_extension(path_str):
    """Simulate .NET Path.GetExtension() behavior"""
    if not path_str:
        return ""
    # Get the last dot and everything after it, but only in the filename part
    # Path.GetExtension returns everything after the last dot in the filename
    parts = path_str.rsplit('/', 1)
    filename = parts[-1]
    
    if '.' in filename:
        ext_part = filename.rsplit('.', 1)[1]
        return '.' + ext_part
    return ""

# Test cases matching the review scenario
test_cases = [
    "https://example.com/download.php?id=1",
    "https://example.com/download.php?id=1&file=mod.zip",
    "/download.php?id=1",
    "https://example.com/files/mod.zip",
    "https://example.com/files/mod.zip?token=abc",
    "mod.zip",
    "https://example.com/path/file.tar.gz?download=1"
]

print("Simulated Path.GetExtension() results:")
print("-" * 70)
for test in test_cases:
    ext = path_get_extension(test)
    print(f"Input:     {test}")
    print(f"Extension: '{ext}'")
    print()

print("\nUri.AbsolutePath approach:")
print("-" * 70)
for test in test_cases:
    if test.startswith("http"):
        try:
            from urllib.parse import urlparse
            parsed = urlparse(test)
            abs_path = parsed.path
            ext = path_get_extension(abs_path)
            print(f"Input:        {test}")
            print(f"AbsolutePath: {abs_path}")
            print(f"Extension:    '{ext}'")
            print()
        except Exception as e:
            print(f"Error: {e}")
EOFPYTHON

Repository: TeamWheelWizard/WheelWizard

Length of output: 1301

Only use the URL extension as a fallback.

Lines 218-220 unconditionally replace the filename's extension using Path.GetExtension(finalUrl) on a full URL string. This captures query parameters as part of the extension—e.g., a response with filename="mod.zip" from /download.php?id=1 becomes mod.php?id=1, which is invalid. Use new Uri(finalUrl).AbsolutePath to extract only the path portion, and only apply it when safeFileName doesn't already have a valid extension.

Suggested fix 
-        var finalExtension = Path.GetExtension(finalUrl);
-        if (!string.IsNullOrWhiteSpace(finalExtension))
-            safeFileName = Path.ChangeExtension(safeFileName, finalExtension);
+        if (!Path.HasExtension(safeFileName))
+        {
+            var finalExtension = Path.GetExtension(new Uri(finalUrl).AbsolutePath);
+            if (!string.IsNullOrWhiteSpace(finalExtension))
+                safeFileName = Path.ChangeExtension(safeFileName, finalExtension);
+        }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
var finalExtension = Path.GetExtension(finalUrl);
if (!string.IsNullOrWhiteSpace(finalExtension))
safeFileName = Path.ChangeExtension(safeFileName, finalExtension);
if (!Path.HasExtension(safeFileName))
{
var finalExtension = Path.GetExtension(new Uri(finalUrl).AbsolutePath);
if (!string.IsNullOrWhiteSpace(finalExtension))
safeFileName = Path.ChangeExtension(safeFileName, finalExtension);
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@WheelWizard/Helpers/DownloadHelper.cs` around lines 218 - 220, The current
code uses Path.GetExtension(finalUrl) which can include query strings; instead
parse the URL path and only apply the extension as a fallback when safeFileName
lacks one: obtain the path via new Uri(finalUrl).AbsolutePath, call
Path.GetExtension on that path (not on finalUrl), check
Path.GetExtension(safeFileName) first and only call
Path.ChangeExtension(safeFileName, ext) if safeFileName has no extension and the
extracted ext is non-empty; update the logic around finalExtension,
safeFileName, Path.GetExtension and Path.ChangeExtension accordingly.


return safeFileName;
}

private static void EnsurePathStaysWithinDirectory(string path, string directory)
{
var normalizedDirectory = Path.GetFullPath(directory + Path.DirectorySeparatorChar);
var normalizedPath = Path.GetFullPath(path);
var comparison = OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
if (!normalizedPath.StartsWith(normalizedDirectory, comparison))
throw new InvalidOperationException("The download path escaped the target directory.");
}
}
7 changes: 3 additions & 4 deletions WheelWizard/Services/Installation/ModInstallation.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -109,6 +109,7 @@ public static bool ModExists(ObservableCollection<Mod> mods, string modName) =>
public static void ProcessFile(string file, string destinationDirectory, ProgressWindow progressWindow)
{
var extension = Path.GetExtension(file).ToLowerInvariant();
var normalizedDestinationDirectory = Path.GetFullPath(destinationDirectory + Path.DirectorySeparatorChar);

if (!Directory.Exists(destinationDirectory))
Directory.CreateDirectory(destinationDirectory);
Expand Down Expand Up @@ -144,12 +145,10 @@ public static void ProcessFile(string file, string destinationDirectory, Progres
);

var entryDestinationPath = Path.Combine(destinationDirectory, sanitizedKey);
var normalizedEntryDestinationPath = Path.GetFullPath(entryDestinationPath);

// Ensure the entry destination path is within the destination directory
if (
!Path.GetFullPath(entryDestinationPath)
.StartsWith(Path.GetFullPath(destinationDirectory), StringComparison.OrdinalIgnoreCase)
)
if (!normalizedEntryDestinationPath.StartsWith(normalizedDestinationDirectory, StringComparison.OrdinalIgnoreCase))
Copy link
Copy Markdown

@coderabbitai coderabbitai Bot Apr 8, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ Potential issue | 🔴 Critical

🧩 Analysis chain

🏁 Script executed:

# Find and examine ModInstallation.cs around line 151
find . -name "ModInstallation.cs" -type f | head -5

Repository: TeamWheelWizard/WheelWizard

Length of output: 125

🏁 Script executed:

# Find DownloadHelper to compare the containment check logic
find . -name "DownloadHelper.cs" -type f | head -5

Repository: TeamWheelWizard/WheelWizard

Length of output: 110

🏁 Script executed:

# Examine ModInstallation.cs around line 151
cat -n ./WheelWizard/Services/Installation/ModInstallation.cs | sed -n '140,160p'

Repository: TeamWheelWizard/WheelWizard

Length of output: 1435

🏁 Script executed:

# Examine DownloadHelper.cs to find the containment check logic
cat -n ./WheelWizard/Helpers/DownloadHelper.cs | head -100

Repository: TeamWheelWizard/WheelWizard

Length of output: 4874

🏁 Script executed:

# Continue reading DownloadHelper.cs to find the EnsurePathStaysWithinDirectory method
cat -n ./WheelWizard/Helpers/DownloadHelper.cs | tail -100

Repository: TeamWheelWizard/WheelWizard

Length of output: 5133

🏁 Script executed:

# Find where normalizedDestinationDirectory is defined in ModInstallation.cs
cat -n ./WheelWizard/Services/Installation/ModInstallation.cs | sed -n '100,150p'

Repository: TeamWheelWizard/WheelWizard

Length of output: 2658

Use OS-aware comparison in ProcessFile to prevent path traversal on case-sensitive filesystems.

Line 151 hardcodes StringComparison.OrdinalIgnoreCase, creating a security gap. On case-sensitive filesystems (Linux, macOS), an archive entry like ../Mods/evil.bin normalizes to /tmp/Mods/evil.bin but passes the check against /tmp/mods/ under case-insensitive comparison. This allows extracting outside the intended directory. DownloadHelper already implements the correct pattern with platform-aware comparison—apply the same approach here.

Suggested fix 
-                if (!normalizedEntryDestinationPath.StartsWith(normalizedDestinationDirectory, StringComparison.OrdinalIgnoreCase))
+                var comparison = OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
+                if (!normalizedEntryDestinationPath.StartsWith(normalizedDestinationDirectory, comparison))
                 {
                     throw new UnauthorizedAccessException("Entry is attempting to extract outside of the destination directory.");
                 }
📝 Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
if (!normalizedEntryDestinationPath.StartsWith(normalizedDestinationDirectory, StringComparison.OrdinalIgnoreCase))
var comparison = OperatingSystem.IsWindows() ? StringComparison.OrdinalIgnoreCase : StringComparison.Ordinal;
if (!normalizedEntryDestinationPath.StartsWith(normalizedDestinationDirectory, comparison))
{
throw new UnauthorizedAccessException("Entry is attempting to extract outside of the destination directory.");
}
🤖 Prompt for AI Agents 
Verify each finding against the current code and only fix it if needed.

In `@WheelWizard/Services/Installation/ModInstallation.cs` at line 151, In
ProcessFile, the path containment check uses StringComparison.OrdinalIgnoreCase
which is wrong on case-sensitive OSes; update the comparison to use the same
platform-aware comparison logic as DownloadHelper (i.e., determine comparison
type based on the current OS and use that when calling
normalizedEntryDestinationPath.StartsWith(normalizedDestinationDirectory,
comparison)), ensuring normalizedEntryDestinationPath and
normalizedDestinationDirectory are compared with the OS-appropriate
StringComparison to prevent path traversal on case-sensitive filesystems.

{
throw new UnauthorizedAccessException("Entry is attempting to extract outside of the destination directory.");
}
Expand Down
24 changes: 15 additions & 9 deletions WheelWizard/Views/App.axaml.cs
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -5,11 +5,11 @@
using Microsoft.Extensions.Logging;
using WheelWizard.AutoUpdating;
using WheelWizard.MiiRendering.Services;
using WheelWizard.Settings;
using WheelWizard.Services.Launcher;
using WheelWizard.Services;
using WheelWizard.Services.Launcher;
using WheelWizard.Services.LiveData;
using WheelWizard.Services.UrlProtocol;
using WheelWizard.Settings;
using WheelWizard.Views.Behaviors;
using WheelWizard.Views.Popups.Generic;
using WheelWizard.WheelWizardData;
Expand Down Expand Up @@ -79,15 +79,19 @@ private static StartupLaunchTarget GetStartupLaunchTarget()
for (var i = 1; i < args.Length; i++)
{
var argument = args[i];
if (argument.Equals("--launch", StringComparison.OrdinalIgnoreCase) || argument.Equals("-l", StringComparison.OrdinalIgnoreCase))
if (
argument.Equals("--launch", StringComparison.OrdinalIgnoreCase) || argument.Equals("-l", StringComparison.OrdinalIgnoreCase)
)
{
if (i + 1 >= args.Length)
continue;

var launchTarget = args[++i];
if (launchTarget.Equals("rr", StringComparison.OrdinalIgnoreCase) ||
launchTarget.Equals("retrorewind", StringComparison.OrdinalIgnoreCase) ||
launchTarget.Equals("retro-rewind", StringComparison.OrdinalIgnoreCase))
if (
launchTarget.Equals("rr", StringComparison.OrdinalIgnoreCase)
|| launchTarget.Equals("retrorewind", StringComparison.OrdinalIgnoreCase)
|| launchTarget.Equals("retro-rewind", StringComparison.OrdinalIgnoreCase)
)
{
return StartupLaunchTarget.RetroRewind;
}
Expand All @@ -99,9 +103,11 @@ private static StartupLaunchTarget GetStartupLaunchTarget()
continue;

var launchTargetFromEquals = argument["--launch=".Length..].Trim();
if (launchTargetFromEquals.Equals("rr", StringComparison.OrdinalIgnoreCase) ||
launchTargetFromEquals.Equals("retrorewind", StringComparison.OrdinalIgnoreCase) ||
launchTargetFromEquals.Equals("retro-rewind", StringComparison.OrdinalIgnoreCase))
if (
launchTargetFromEquals.Equals("rr", StringComparison.OrdinalIgnoreCase)
|| launchTargetFromEquals.Equals("retrorewind", StringComparison.OrdinalIgnoreCase)
|| launchTargetFromEquals.Equals("retro-rewind", StringComparison.OrdinalIgnoreCase)
)
{
return StartupLaunchTarget.RetroRewind;
}
Expand Down
Loading