-
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?
Changes from 1 commit
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -48,6 +48,18 @@ struct DownloadButton: View { | |
|
|
||
| do { | ||
| if let temporaryURL = temporaryURL { | ||
| let tempDir = FileManager.default.temporaryDirectory | ||
| guard temporaryURL.path.hasPrefix(tempDir.path) else { | ||
| print("Security Error: Temporary file path is outside expected directory") | ||
| return | ||
| } | ||
|
Comment on lines
+51
to
+55
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Security Issue: Insecure Path Validation Using
Example Attack: // If tempDir.path = "/tmp"
// An attacker could use: "/tmp/../var/tmp/malicious"
// This passes hasPrefix("/tmp") but resolves outside /tmpRecommended 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. |
||
|
|
||
| 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 | ||
| } | ||
|
Comment on lines
51
to
61
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Critical Security Issue: Insufficient Path Validation The current path validation using
Vulnerable Example: let tempDir = "/tmp" // path is "/tmp"
let maliciousPath = "/tmp/../../../etc/passwd" // hasPrefix("/tmp") = true ✓ (bypassed!)Recommended Fix: 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. |
||
|
|
||
| try FileManager.default.copyItem(at: temporaryURL, to: fileURL) | ||
| print("Writing to \(filename) completed") | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -52,6 +52,18 @@ struct InputButton: View { | |
|
|
||
| do { | ||
| if let temporaryURL = temporaryURL { | ||
| 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 | ||
| } | ||
|
Comment on lines
55
to
65
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 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 |
||
|
|
||
| try FileManager.default.copyItem(at: temporaryURL, to: fileURL) | ||
| print("Writing to \(filename) completed") | ||
|
|
||
|
|
||
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:
Suggested improvement:
Consider adding an error state variable to show security errors to users in the UI.