Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@WheelWizard/Helpers/DownloadHelper.cs`:
- Around line 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.
In `@WheelWizard/Services/Installation/ModInstallation.cs`:
- 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.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: 1bd0e7e1-7472-4166-81ce-68e07058b902
📒 Files selected for processing (3)
WheelWizard/Helpers/DownloadHelper.csWheelWizard/Services/Installation/ModInstallation.csWheelWizard/Views/App.axaml.cs
| var finalExtension = Path.GetExtension(finalUrl); | ||
| if (!string.IsNullOrWhiteSpace(finalExtension)) | ||
| safeFileName = Path.ChangeExtension(safeFileName, finalExtension); |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# First, locate and read the DownloadHelper.cs file
find . -name "DownloadHelper.cs" -type fRepository: 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}");
}
}
EOFCSHARPRepository: 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}")
EOFPYTHONRepository: 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.
| 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.
| !Path.GetFullPath(entryDestinationPath) | ||
| .StartsWith(Path.GetFullPath(destinationDirectory), StringComparison.OrdinalIgnoreCase) | ||
| ) | ||
| if (!normalizedEntryDestinationPath.StartsWith(normalizedDestinationDirectory, StringComparison.OrdinalIgnoreCase)) |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
# Find and examine ModInstallation.cs around line 151
find . -name "ModInstallation.cs" -type f | head -5Repository: TeamWheelWizard/WheelWizard
Length of output: 125
🏁 Script executed:
# Find DownloadHelper to compare the containment check logic
find . -name "DownloadHelper.cs" -type f | head -5Repository: 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 -100Repository: TeamWheelWizard/WheelWizard
Length of output: 4874
🏁 Script executed:
# Continue reading DownloadHelper.cs to find the EnsurePathStaysWithinDirectory method
cat -n ./WheelWizard/Helpers/DownloadHelper.cs | tail -100Repository: 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.
| 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.
|
niet meer 🥉 |
2 participants
Wheel Wizard downloads a mod into its temp folder \CT-MKWII\Mods\Temp\
But when it decides what filename to save as, it trusts the server’s filename from the HTTP response in WheelWizard/Helpers/DownloadHelper.cs:79. It does:
and never checks that the final path is still inside Mods\Temp.
So if the attacker makes the filename something like:
......\Microsoft\Windows\Start Menu\Programs\Startup\evil.bat
then Wheel Wizard can end up saving the download here instead:
C:\Users\AppData\Roaming\Microsoft\Windows\Start Menu\Programs\Startup\evil.bat
Then on next windows launch it could launch this file, this is not something we want
Summary by CodeRabbit
Security
Refactor