🛡️ Sentinel: [CRITICAL] Fix path traversal in plugin manager#95
🛡️ Sentinel: [CRITICAL] Fix path traversal in plugin manager#95
Conversation
Plugin names parsed from untrusted metadata (`# @name`) or extracted from direct URLs were passed unsanitized into `filepath.Join`, allowing path traversal (e.g., writing or deleting arbitrary files outside the plugin directory).
This patch sanitizes untrusted input using `filepath.Base(filepath.Clean("/" + name))` *before* joining it with base directories to explicitly restrict it to a single filename.
Co-authored-by: himattm <6266621+himattm@users.noreply.github.com>
|
👋 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. |
There was a problem hiding this comment.
Code Review
This pull request addresses a path traversal vulnerability by introducing a sanitizePluginName function and applying it to plugin names during installation and removal. The review feedback highlights that the current sanitization logic could still result in invalid filenames (like /) for certain inputs and suggests sanitizing the name variables directly at the start of functions to ensure consistent usage in metadata and UI output.
| // sanitizePluginName ensures the given name cannot escape the target directory | ||
| // by stripping path components. | ||
| func sanitizePluginName(name string) string { | ||
| return filepath.Base(filepath.Clean("/" + name)) |
There was a problem hiding this comment.
The current implementation returns a path separator (e.g., / or \) if the input name is empty or resolves to the root (e.g., ..). This results in an invalid filename like prism-plugin-/, which will cause subsequent file operations to fail with an "invalid argument" or "is a directory" error. Consider returning a safe default name or an empty string in these cases to ensure a valid filename is always generated.
| } | ||
|
|
||
| destPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s", pluginName)) | ||
| destPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s", sanitizePluginName(pluginName))) |
There was a problem hiding this comment.
Sanitize the pluginName variable itself. This ensures that the sanitized name is used consistently throughout the function, including in the metadata saved at line 388 and for the display name in the UI, rather than just for the destination path.
| destPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s", sanitizePluginName(pluginName))) | |
| pluginName = sanitizePluginName(pluginName) | |
| destPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s", pluginName)) |
| } | ||
|
|
||
| destPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", meta.Name)) | ||
| destPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", sanitizePluginName(meta.Name))) |
There was a problem hiding this comment.
Sanitize the meta.Name field directly. This ensures the sanitized name is used for the destination path, the existence check, and the final confirmation message, providing a consistent user experience and preventing UI spoofing with path-like names.
| destPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", sanitizePluginName(meta.Name))) | |
| meta.Name = sanitizePluginName(meta.Name) | |
| destPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", meta.Name)) |
| var destPath string | ||
| if isScript { | ||
| destPath = filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", pluginName)) | ||
| destPath = filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", sanitizePluginName(pluginName))) |
There was a problem hiding this comment.
Sanitize pluginName here so that the sanitized version is used for both the path construction and the metadata/display logic later in the function (lines 518 and 522).
| destPath = filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", sanitizePluginName(pluginName))) | |
| pluginName = sanitizePluginName(pluginName) | |
| destPath = filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", pluginName)) |
| destPath = filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", sanitizePluginName(pluginName))) | ||
| } else { | ||
| destPath = filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s", pluginName)) | ||
| destPath = filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s", sanitizePluginName(pluginName))) |
| binaryPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s", sanitizePluginName(name))) | ||
| scriptPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", sanitizePluginName(name))) |
There was a problem hiding this comment.
Sanitize the name variable once at the start of the function. This simplifies the path construction and ensures that the sanitized name is used in the final "Removed" message at line 811, avoiding potentially confusing output if a path-like name was provided.
| binaryPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s", sanitizePluginName(name))) | |
| scriptPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", sanitizePluginName(name))) | |
| name = sanitizePluginName(name) | |
| binaryPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s", name)) | |
| scriptPath := filepath.Join(m.pluginDir, fmt.Sprintf("prism-plugin-%s.sh", name)) |
🚨 Severity: CRITICAL
💡 Vulnerability: Plugin names parsed from untrusted metadata (
# @name) or extracted from direct URLs were passed unsanitized intofilepath.Join, allowing path traversal (e.g., writing or deleting arbitrary files outside the plugin directory). Becausefilepath.Joincleans paths but still evaluates../sequences internally, combining a base directory with untrusted input containing../still allows escaping the base directory.🎯 Impact: Remote code execution via arbitrary file write (e.g. overwriting
~/.bashrc), or arbitrary file deletion when updating or removing a plugin.🔧 Fix: Sanitize untrusted input using a robust
filepath.Base(filepath.Clean("/" + name))approach before joining it with base directories to explicitly restrict it to a single filename within the target directory.✅ Verification: Verified code format (
go fmt ./...) and tests (go test ./...) all pass. Inspectedgit diffto ensuresanitizePluginNameis correctly applied to all path creation instances inmanager.go. Added a.jules/sentinel.mdlearning entry documenting the vulnerability and solution pattern.PR created automatically by Jules for task 12422923428382884991 started by @himattm