-
Notifications
You must be signed in to change notification settings - Fork 0
Fix high-severity security issues found by Snyk Code scan #319
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.
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
base: master
Are you sure you want to change the base?
Conversation
- Fix path traversal vulnerabilities in Swift UI components by validating file paths - Add path validation in DownloadButton.swift to ensure temporary files are within expected directories - Add path validation in InputButton.swift to ensure temporary files are within expected directories - Update API key documentation to emphasize secure configuration practices - Prevent potential path traversal attacks by validating both source and destination paths Co-Authored-By: Jake Cosme <[email protected]>
🤖 Devin AI EngineerI'll be helping with this pull request! Here's what you should know: ✅ I will automatically:
Note: I can only respond to comments from users who have write access to this repository. ⚙️ Control Options:
|
Co-Authored-By: Jake Cosme <[email protected]>
let tempDir = FileManager.default.temporaryDirectory | ||
guard temporaryURL.path.hasPrefix(tempDir.path) else { | ||
print("Security Error: Temporary file path is outside expected directory") | ||
return | ||
} | ||
|
||
let docsDir = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0] | ||
guard fileURL.path.hasPrefix(docsDir.path) else { | ||
print("Security Error: Destination path is outside documents directory") | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Critical Security Issue: Insufficient Path Validation
The current path validation using hasPrefix
is vulnerable to path traversal attacks. This approach doesn't protect against:
- Symbolic links that could point outside the allowed directories
- Path components like
..
that could escape the directory - Non-canonical paths (e.g.,
/tmp/./../../etc/passwd
)
Vulnerable Example:
let tempDir = "/tmp" // path is "/tmp"
let maliciousPath = "/tmp/../../../etc/passwd" // hasPrefix("/tmp") = true ✓ (bypassed!)
Recommended Fix:
Use standardizedFileURL
or resolvingSymlinksInPath()
to normalize paths before validation:
let tempDir = FileManager.default.temporaryDirectory
let normalizedTempURL = temporaryURL.standardizedFileURL
let normalizedTempDir = tempDir.standardizedFileURL
guard normalizedTempURL.path.hasPrefix(normalizedTempDir.path) else {
print("Security Error: Temporary file path is outside expected directory")
return
}
let docsDir = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0]
let normalizedFileURL = fileURL.standardizedFileURL
let normalizedDocsDir = docsDir.standardizedFileURL
guard normalizedFileURL.path.hasPrefix(normalizedDocsDir.path) else {
print("Security Error: Destination path is outside documents directory")
return
}
This ensures all symbolic links are resolved and relative path components are normalized before comparison.
let tempDir = FileManager.default.temporaryDirectory | ||
guard temporaryURL.path.hasPrefix(tempDir.path) else { | ||
print("Security Error: Temporary file path is outside expected directory") | ||
return | ||
} | ||
|
||
let docsDir = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0] | ||
guard fileURL.path.hasPrefix(docsDir.path) else { | ||
print("Security Error: Destination path is outside documents directory") | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same Security Issue: Insufficient Path Validation
This file has the identical path validation vulnerability as DownloadButton.swift
. The hasPrefix
check is insufficient and vulnerable to path traversal attacks.
Apply the same fix here:
let tempDir = FileManager.default.temporaryDirectory
let normalizedTempURL = temporaryURL.standardizedFileURL
let normalizedTempDir = tempDir.standardizedFileURL
guard normalizedTempURL.path.hasPrefix(normalizedTempDir.path) else {
print("Security Error: Temporary file path is outside expected directory")
return
}
let docsDir = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0]
let normalizedFileURL = fileURL.standardizedFileURL
let normalizedDocsDir = docsDir.standardizedFileURL
guard normalizedFileURL.path.hasPrefix(normalizedDocsDir.path) else {
print("Security Error: Destination path is outside documents directory")
return
}
See the comment on DownloadButton.swift
for a detailed explanation of the vulnerability.
let tempDir = FileManager.default.temporaryDirectory | ||
guard temporaryURL.path.hasPrefix(tempDir.path) else { | ||
print("Security Error: Temporary file path is outside expected directory") | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security Issue: Insecure Path Validation
Using hasPrefix
for path validation is vulnerable to path traversal attacks. This approach doesn't handle:
- Path normalization: Paths with
..
components can bypass the check - Symlinks: Symbolic links can point outside the allowed directory
- Path canonicalization: Different representations of the same path
Example Attack:
// If tempDir.path = "/tmp"
// An attacker could use: "/tmp/../var/tmp/malicious"
// This passes hasPrefix("/tmp") but resolves outside /tmp
Recommended Fix:
let tempDir = FileManager.default.temporaryDirectory
let resolvedTempURL = temporaryURL.standardizedFileURL.resolvingSymlinksInPath()
let resolvedTempDir = tempDir.standardizedFileURL.resolvingSymlinksInPath()
guard resolvedTempURL.path.hasPrefix(resolvedTempDir.path + "/") || resolvedTempURL.path == resolvedTempDir.path else {
print("Security Error: Temporary file path is outside expected directory")
return
}
The same fix should be applied to the destination path validation on lines 57-61.
guard temporaryURL.path.hasPrefix(tempDir.path) else { | ||
print("Security Error: Temporary file path is outside expected directory") | ||
return |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
UX Issue: Silent Failure
When path validation fails, the code only prints to console with no user feedback. Users will see their download appear to complete but the file won't be saved, leading to confusion.
Current behavior:
guard temporaryURL.path.hasPrefix(tempDir.path) else {
print("Security Error: Temporary file path is outside expected directory")
return // Silent failure - user sees no error
}
Suggested improvement:
guard temporaryURL.path.hasPrefix(tempDir.path) else {
DispatchQueue.main.async {
self.status = "download" // Reset to allow retry
// Consider adding an @State error message to display in UI
}
print("Security Error: Temporary file path is outside expected directory")
return
}
Consider adding an error state variable to show security errors to users in the UI.
let tempDir = FileManager.default.temporaryDirectory | ||
guard temporaryURL.path.hasPrefix(tempDir.path) else { | ||
print("Security Error: Temporary file path is outside expected directory") | ||
return | ||
} | ||
|
||
let docsDir = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0] | ||
guard fileURL.path.hasPrefix(docsDir.path) else { | ||
print("Security Error: Destination path is outside documents directory") | ||
return | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Security Issue: Same Path Validation Vulnerability
This code has the same insecure path validation issue as DownloadButton.swift. Using hasPrefix
doesn't protect against path traversal attacks with symlinks or ..
components.
Please apply the same fix recommended in DownloadButton.swift:
let tempDir = FileManager.default.temporaryDirectory
let resolvedTempURL = temporaryURL.standardizedFileURL.resolvingSymlinksInPath()
let resolvedTempDir = tempDir.standardizedFileURL.resolvingSymlinksInPath()
guard resolvedTempURL.path.hasPrefix(resolvedTempDir.path + "/") || resolvedTempURL.path == resolvedTempDir.path else {
print("Security Error: Temporary file path is outside expected directory")
return
}
let docsDir = FileManager.default.urls(for: .documentDirectory, in: .userDomainMask)[0]
let resolvedFileURL = fileURL.standardizedFileURL.resolvingSymlinksInPath()
let resolvedDocsDir = docsDir.standardizedFileURL.resolvingSymlinksInPath()
guard resolvedFileURL.path.hasPrefix(resolvedDocsDir.path + "/") || resolvedFileURL.path == resolvedDocsDir.path else {
print("Security Error: Destination path is outside documents directory")
return
}
Also consider adding user-visible error feedback as mentioned in the DownloadButton.swift review.
Make sure to read the contributing guidelines before submitting a PR
Summary
This PR addresses three high-severity security issues identified by Snyk Code scan:
DownloadButton.swift
andInputButton.swift
)settings-config.ts
)Changes
Path Traversal Fixes (DownloadButton.swift & InputButton.swift)
Added path validation guards before file copy operations to prevent path traversal attacks:
API Key Documentation (settings-config.ts)
Updated the API key configuration description to explicitly discourage hardcoding secrets and recommend using environment variables or secure configuration management.
Security Impact
These changes mitigate the risk of:
Important Review Points
🔍 Path Validation Logic: Please carefully review the path validation approach:
hasPrefix
for directory boundary checks🔍 UX Consideration: Downloads that fail security validation will silently fail (no error shown to user). Consider if this is acceptable behavior.
Testing
-DLLAMA_FATAL_WARNINGS=ON
Link to Devin run: https://app.devin.ai/sessions/281e4928d9e04ac3a80752a826ba3865
Requested by: Jake Cosme (@jakexcosme)