🛡️ Sentinel: HIGH Fix path traversal vulnerability in Lua Sandbox IO#187
🛡️ Sentinel: HIGH Fix path traversal vulnerability in Lua Sandbox IO#187mleem97 wants to merge 1 commit into
Conversation
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
Up to standards ✅🟢 Issues
|
| Metric | Results |
|---|---|
| Complexity | 0 |
| Duplication | 0 |
AI Reviewer: first review requested successfully. AI can make mistakes. Always validate suggestions.
TIP This summary will be updated as you push new changes.
There was a problem hiding this comment.
Pull Request Overview
The PR successfully implements trailing slash validation in the Lua Sandbox IO module, which is a critical fix for preventing sibling directory bypasses. Codacy analysis indicates the code is up to standards; however, the implementation currently relies on relative paths that are resolved against the process's Current Working Directory (CWD) at runtime. This introduces a risk where the sandbox could become invalid or point to an incorrect location if the CWD changes during the application's lifecycle.
Most importantly, while the PR title indicates a 'HIGH' severity fix, no unit tests were included in the diff to verify the security logic. There is no automated verification for sibling directory isolation or protection against '..' traversal sequences. These tests are essential for ensuring the sandbox remains robust against future regressions.
About this PR
- No test files were included in the PR diff to verify the security fix, despite the description stating that 'Tests pass'. Automated unit tests are required to validate the sandbox boundaries.
Test suggestions
- Missing recommended test scenario: Accessing a file deep within the valid data directory
- Missing recommended test scenario: Attempting to access a sibling directory with a matching prefix (e.g., /data_secrets)
- Missing recommended test scenario: Accessing the base directory itself exactly
- Missing recommended test scenario: Attempting path traversal using '..' sequences to escape the sandbox
Prompt proposal for missing tests
Consider implementing these tests if applicable:
1. Missing recommended test scenario: Accessing a file deep within the valid data directory
2. Missing recommended test scenario: Attempting to access a sibling directory with a matching prefix (e.g., /data_secrets)
3. Missing recommended test scenario: Accessing the base directory itself exactly
4. Missing recommended test scenario: Attempting path traversal using '..' sequences to escape the sandbox
Low confidence findings
- The use of 'StringComparison.OrdinalIgnoreCase' for path validation is appropriate for Windows but may be overly permissive on case-sensitive filesystems like Linux. Ensure this aligns with the intended cross-platform support for the sandbox.
TIP Improve review quality by adding custom instructions
TIP How was this review? Give us feedback
| /// </summary> | ||
| public static void Register(Table greg, Script script, string modId, string modDir) | ||
| { | ||
| string dataDir = Path.Combine(modDir, "data"); |
There was a problem hiding this comment.
🟡 MEDIUM RISK
Suggestion: Resolve 'dataDir' to an absolute path immediately during registration to ensure consistent behavior regardless of changes to the current working directory.
| string dataDir = Path.Combine(modDir, "data"); | |
| string dataDir = Path.GetFullPath(Path.Combine(modDir, "data")); |
| string dataDirFull = Path.GetFullPath(dataDir); | ||
| string dataDirFullWithSlash = dataDirFull; | ||
| if (!dataDirFullWithSlash.EndsWith(Path.DirectorySeparatorChar.ToString())) | ||
| { | ||
| dataDirFullWithSlash += Path.DirectorySeparatorChar; | ||
| } | ||
|
|
There was a problem hiding this comment.
⚪ LOW RISK
Suggestion: Refactor 'GregIoLuaModule' to pre-calculate the absolute sandbox path and its trailing-slash version once in the 'Register' method. Then, update 'ResolveSafe' to use these stored values to avoid redundant path normalization and string checks during every IO call.
🚨 Severity: HIGH
💡 Vulnerability: Sandbox boundary check used
StartsWithon a base path without a trailing slash, allowing bypasses like/data_secretmatching/data.🎯 Impact: Malicious Lua mods could read, write, or delete arbitrary files in directories prefixed similarly to the allowed data directory (e.g. escaping the
/data/sandbox).🔧 Fix: Added trailing slash to the base directory path before the
StartsWithcheck and allowed exact matches.✅ Verification: Tests pass and code inspection confirms exact string matches and correct path prefixing.
PR created automatically by Jules for task 6908275699745430770 started by @mleem97