🛡️ Sentinel: [CRITICAL] Fix Path Traversal in Plugin Manager#94
🛡️ Sentinel: [CRITICAL] Fix Path Traversal in Plugin Manager#94
Conversation
Untrusted plugin names from parsed URLs or downloaded script metadata
were concatenated directly into file paths using `filepath.Join()`,
allowing attackers to write arbitrary files outside the plugin directory
via directory escapes (e.g. `../`). This explicitly sanitizes input with
`filepath.Base(filepath.Clean("/" + name))` before constructing file paths.
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. |
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request addresses a path traversal vulnerability in the plugin manager by sanitizing plugin names derived from URLs and metadata using filepath.Base and filepath.Clean. Review feedback suggests extracting this duplicated sanitization logic into a centralized helper function to improve maintainability. Additionally, it is recommended to add validation for the resulting names to ensure they are valid identifiers and do not resolve to '.' or '/', which could cause subsequent file operations to fail.
| if len(parts) >= 2 { | ||
| owner, repo := parts[0], parts[1] | ||
| pluginName := strings.TrimPrefix(repo, "prism-plugin-") | ||
| pluginName := filepath.Base(filepath.Clean("/" + strings.TrimPrefix(repo, "prism-plugin-"))) |
There was a problem hiding this comment.
The sanitization logic filepath.Base(filepath.Clean("/" + name)) is duplicated here and at lines 438 and 490. To improve maintainability and ensure consistent behavior across the manager, consider extracting this into a helper function. Additionally, this logic can return . or / for certain inputs (e.g., if the input resolves to the root after cleaning). While this prevents path traversal, it can lead to invalid filenames like prism-plugin-/.sh which will cause subsequent file operations to fail. It would be more robust to validate that the resulting name is a valid, non-empty identifier.
| if meta.Name == "" { | ||
| meta.Name = pluginName | ||
| } else { | ||
| meta.Name = filepath.Base(filepath.Clean("/" + meta.Name)) |
There was a problem hiding this comment.
This is another instance of the duplicated sanitization logic. As mentioned previously, extracting this to a helper function and adding validation for edge cases (like the name resolving to . or /) would improve the robustness of the plugin manager.
| for _, suffix := range []string{"-darwin-arm64", "-darwin-amd64", "-linux-amd64", "-linux-arm64"} { | ||
| pluginName = strings.TrimSuffix(pluginName, suffix) | ||
| } | ||
| pluginName = filepath.Base(filepath.Clean("/" + pluginName)) |
There was a problem hiding this comment.
Third instance of the duplicated sanitization logic. Centralizing this logic would make it easier to apply more thorough validation (e.g., checking for invalid characters or reserved names) consistently across all plugin installation methods.
1 participant
🚨 Severity: CRITICAL
💡 Vulnerability: Untrusted plugin names extracted from URLs or downloaded script metadata (
@name) were concatenated directly into file system paths usingfilepath.Join(). This allowed attackers to use directory escapes (e.g.,../../../etc/passwd) to write files outside the expected plugin directory.🎯 Impact: Arbitrary file overwrite / Path traversal. A malicious URL or script could write or overwrite sensitive files on the user's system, potentially leading to remote code execution or privilege escalation.
🔧 Fix: Sanitized the extracted
pluginNameandmeta.Nameby strictly applyingfilepath.Base(filepath.Clean("/" + name))before any path construction. This guarantees only the final valid filename component is used, preventing directory escapes.✅ Verification: Verified by unit tests and ensuring
filepath.Joinreceives sanitized input. Test files with explicit path traversals demonstrated successful containment.PR created automatically by Jules for task 6654690335110853095 started by @himattm