Fix file permissions on windows #1758
Conversation
📝 Walkthrough📝 WalkthroughWalkthroughThe changes in this pull request primarily focus on enhancing file permission handling across different operating systems, particularly for Windows. The Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant System
participant ACL
User->>System: Request file operation
System->>System: Check OS
alt Windows
System->>ACL: Set permissions using acl.Chmod
ACL-->>System: Return success/failure
System->>User: Notify result
else Non-Windows
System->>System: Set permissions using os.Chmod
System-->>User: Notify result
end
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (9)
base/utils/fs.go (1)
Line range hint
16-50: Consider architectural improvements
- Consider wrapping errors with more context about the permission operations:
if isWindows { - return acl.Chmod(path, perm) + if err := acl.Chmod(path, perm); err != nil { + return fmt.Errorf("failed to set Windows ACL permissions on %s: %w", path, err) + } + return nil }
- Consider splitting the implementation into platform-specific files (e.g.,
fs_windows.goandfs_unix.go) for better maintainability and clearer separation of concerns.service/updates/helper/electron.go (1)
55-59: Improve error handling for cross-platform supportThe error message in
suidBitWarningis Linux-specific, mentioning kernel configurations and SUID bits. If Windows support is truly needed (which it shouldn't be, given earlier comments), the error messages should be OS-specific.Consider using OS-specific error messages:
var ( linuxSuidBitWarning = `Failed to set SUID permissions for chrome-sandbox. This is required for Linux kernel versions that do not have unprivileged user namespaces (CONFIG_USER_NS_UNPRIVILEGED) enabled. If you're running and up-to-date distribution kernel you can likely ignore this warning. If you encounter issue starting the user interface please either update your kernel or set the SUID bit (mode 0%0o) on %s` windowsPermissionWarning = `Failed to set permissions for chrome-sandbox. This might affect the application's functionality. Path: %s` )base/utils/atomic.go (2)
9-12: Fix import ordering using gci.The imports can be automatically formatted using the gci tool.
import ( "errors" "fmt" "io" "io/fs" "os" "path" "runtime" - "github.com/hectane/go-acl" "github.com/safing/portmaster/base/utils/renameio" + "github.com/hectane/go-acl" )🧰 Tools
🪛 golangci-lint (1.62.2)
12-12: File is not
gci-ed with --skip-generated -s standard -s default(gci)
🪛 GitHub Check: Linter
[failure] 12-12:
File is notgci-ed with --skip-generated -s standard -s default (gci)
46-51: Consider adding error context for Windows-specific failures.When the ACL operation fails on Windows, it would be helpful to include more context about what went wrong.
if opts.Mode != 0 { if runtime.GOOS == "windows" { - err = acl.Chmod(path.Join(opts.TempDir, dest), opts.Mode) + if err := acl.Chmod(tmpFile.Name(), opts.Mode); err != nil { + return fmt.Errorf("failed to set Windows ACL permissions on %q: %w", tmpFile.Name(), err) + } } else { - err = tmpFile.Chmod(opts.Mode) + if err := tmpFile.Chmod(opts.Mode); err != nil { + return fmt.Errorf("failed to set file permissions on %q: %w", tmpFile.Name(), err) + } } - if err != nil { - return fmt.Errorf("failed to update mode bits of temp file: %w", err) - } }go.mod (1)
Remove outdated TODO comment and replace directive
The TODO comment and replace directive can be removed since:
- PR Special handling for API access #4 in tc-hib/winres is closed
- The repository is using winres v0.3.1 directly (see dependencies), not the replaced version
- Remove lines 4-5:
// TODO: Remove when https://github.com/tc-hib/winres/pull/4 is released. replace github.com/tc-hib/winres => github.com/dhaavi/winres v0.2.2🔗 Analysis chain
Line range hint
3-4: Consider updating the TODO commentThe TODO comment about removing the replace directive when PR #4 is released seems outdated. Consider either:
- Updating the comment with current status
- Removing it if the PR has been released
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash # Check if the referenced PR is still open gh api repos/tc-hib/winres/pulls/4 --jq '.state'Length of output: 55
Script:
#!/bin/bash # Check the content of go.mod to see if there's a TODO comment and any replace directive cat go.modLength of output: 5296
base/database/storage/fstree/fstree.go (2)
18-18: LGTM! Minor import ordering suggestion.The addition of the
go-aclpackage is appropriate for handling Windows-specific file permissions.Consider running
gcito properly order the imports according to the project's standards:gci write --skip-generated -s standard -s default base/database/storage/fstree/fstree.go🧰 Tools
🪛 GitHub Check: Linter
[failure] 18-18:
File is notgci-ed with --skip-generated -s standard -s default (gci)
289-290: Document the Windows-specific behaviorSince the TODOs indicate this is a permanent solution (wont-fix), consider adding a comment explaining the Windows-specific behavior and limitations for future maintainers.
// writeFile mirrors os.WriteFile, replacing an existing file with the same -// name atomically. This is not atomic on Windows, but still an improvement. +// name atomically. This is not atomic on Windows, but provides improved reliability +// through separate permission setting and atomic replacement operations. // TODO: Replace with github.com/google/renamio.WriteFile as soon as it is fixed on Windows. // TODO: This has become a wont-fix. Explore other options.service/updates/upgrader.go (1)
353-358: LGTM: Proper Windows-specific permission handlingThe implementation correctly uses platform-specific APIs (
acl.Chmodfor Windows,os.Chmodfor others) while maintaining consistent permissions (0o0755) and error handling.Consider setting the permissions atomically during the file copy operation to eliminate the window where the file might have incorrect permissions. This could be achieved by modifying the
renameiopackage to support setting permissions during the atomic replace operation.Example approach:
// In renameio package type AtomicFile struct { *os.File permissions os.FileMode } func (t *AtomicFile) CloseAtomicallyReplaceWithPerms(perms os.FileMode) error { // Set permissions before the atomic rename if err := t.Chmod(perms); err != nil { return err } return t.CloseAtomicallyReplace() }base/updater/fetch.go (1)
140-145: Consider permission refinements and constantsTwo suggestions for improvement:
- Consider implementing the TODO comment by differentiating between executable and non-executable files
- Consider extracting the permission value (0o0755) into a named constant for better maintainability
Example implementation:
+ const ( + executableFileMode = 0o0755 + regularFileMode = 0o0644 + ) if onWindows { - err = acl.Chmod(rv.storagePath(), 0o0755) + mode := regularFileMode + if isExecutable(rv.storagePath()) { + mode = executableFileMode + } + err = acl.Chmod(rv.storagePath(), mode) if err != nil { log.Warningf("%s: failed to set permissions on downloaded file %s: %s", reg.Name, rv.storagePath(), err) } } else { - // TODO: only set executable files to 0755, set other to 0644 - err = os.Chmod(rv.storagePath(), 0o0755) //nolint:gosec // See TODO above. + mode := regularFileMode + if isExecutable(rv.storagePath()) { + mode = executableFileMode + } + err = os.Chmod(rv.storagePath(), mode) if err != nil { log.Warningf("%s: failed to set permissions on downloaded file %s: %s", reg.Name, rv.storagePath(), err) } }Would you like me to provide an implementation of the
isExecutablehelper function?
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (9)
base/database/storage/fstree/fstree.go(2 hunks)base/updater/fetch.go(2 hunks)base/utils/atomic.go(2 hunks)base/utils/fs.go(3 hunks)base/utils/renameio/writefile.go(2 hunks)cmds/portmaster-start/run.go(2 hunks)go.mod(1 hunks)service/updates/helper/electron.go(2 hunks)service/updates/upgrader.go(2 hunks)
🧰 Additional context used
🪛 GitHub Check: Linter
base/database/storage/fstree/fstree.go
[failure] 18-18:
File is not gci-ed with --skip-generated -s standard -s default (gci)
base/updater/fetch.go
[failure] 17-17:
File is not gci-ed with --skip-generated -s standard -s default (gci)
base/utils/atomic.go
[failure] 12-12:
File is not gci-ed with --skip-generated -s standard -s default (gci)
🪛 golangci-lint (1.62.2)
base/updater/fetch.go
17-17: File is not gci-ed with --skip-generated -s standard -s default
(gci)
base/utils/atomic.go
12-12: File is not gci-ed with --skip-generated -s standard -s default
(gci)
🔇 Additional comments (14)
base/utils/renameio/writefile.go (2)
3-8: LGTM: Import changes are appropriate
The addition of runtime and go-acl packages is necessary for cross-platform permission handling.
22-27: Verify Windows ACL behavior and add documentation
While the implementation looks correct, consider:
- Adding a comment explaining why different permission handling is needed for Windows
- Verifying that
acl.Chmodprovides equivalent permission handling tot.Chmod
Let's verify the ACL package's usage in the codebase:
Consider adding this documentation:
// Set permissions before writing data, in case the data is sensitive.
+// Windows requires special handling through the ACL package to properly
+// set file permissions, while other operating systems can use standard chmod.
if runtime.GOOS == "windows" {✅ Verification successful
Windows ACL handling is consistently implemented across the codebase
The verification shows that:
- The ACL package is used consistently throughout the codebase for Windows-specific permission handling
- The pattern of using
acl.Chmodfor Windows and standardChmodfor other platforms is uniformly applied - Error handling is consistent across all implementations
- The permissions are properly set with equivalent behavior across platforms (same permission modes used)
The implementation in writefile.go follows the established pattern in the codebase and is correct.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for consistent ACL handling across the codebase
# and verify error handling patterns
# Check for other ACL usage patterns
rg -A 3 "acl\.Chmod"
# Verify error handling patterns
ast-grep --pattern 'if runtime.GOOS == "windows" {
$$$
err = acl.Chmod($_, $_)
$$$
}'
Length of output: 3230
base/utils/fs.go (2)
9-10: LGTM: Appropriate package for Windows ACL management
The addition of the go-acl package is the correct approach for handling Windows-specific file permissions.
Line range hint 16-50: Verify Windows-specific permission handling
Please ensure that these changes have been tested on Windows with:
- Different permission combinations
- Special Windows-specific scenarios (e.g., inherited permissions)
- Edge cases like system directories or read-only locations
service/updates/helper/electron.go (2)
11-11: LGTM: Appropriate package for Windows ACL handling
The addition of the go-acl package is appropriate for handling file permissions on Windows systems.
Line range hint 1-63: Verify the scope of Windows permission changes
Let's verify if this is part of a larger pattern of Windows permission fixes and if the changes are consistent across the codebase.
✅ Verification successful
Let me gather more context about the Windows-specific permission handling by checking the implementation in other files.
Let me check one more aspect to understand the complete Windows permission handling pattern across the codebase.
Windows permission handling is consistently implemented
The codebase shows a consistent pattern of Windows-specific permission handling:
- The
go-aclpackage is used systematically across the codebase for Windows-specific permission management - The
EnsureChromeSandboxPermissionsfunction follows the established pattern of using platform-specific code paths - The implementation is isolated to the electron sandbox requirements and doesn't indicate any missing permission fixes elsewhere
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for related Windows permission handling changes
# Check for other files using go-acl
echo "Files using go-acl:"
rg "github.com/hectane/go-acl"
# Check for other Windows-specific permission handling
echo -e "\nOther Windows-specific permission handling:"
rg "runtime.GOOS.*windows.*Chmod"
# Check for related function calls
echo -e "\nUsage of EnsureChromeSandboxPermissions:"
rg "EnsureChromeSandboxPermissions"
Length of output: 1544
Script:
#!/bin/bash
# Check the implementation of Windows-specific permission handling in other files
# Check base/utils/atomic.go implementation
echo "=== atomic.go implementation ==="
ast-grep --pattern 'func $$$Chmod($$$) {
$$$
}' base/utils/atomic.go
# Check base/utils/fs.go implementation
echo -e "\n=== fs.go implementation ==="
ast-grep --pattern 'func $$$Chmod($$$) {
$$$
}' base/utils/fs.go
# Check base/updater/fetch.go implementation
echo -e "\n=== fetch.go implementation ==="
rg -A 10 "go-acl" base/updater/fetch.go
# Check service/updates/upgrader.go for context
echo -e "\n=== upgrader.go context ==="
rg -A 5 "EnsureChromeSandboxPermissions" service/updates/upgrader.go
Length of output: 1125
Script:
#!/bin/bash
# Check for Windows-specific permission handling patterns
# Check for Windows-specific Chmod calls
echo "=== Windows-specific Chmod patterns ==="
rg -A 3 "if runtime.GOOS == \"windows\".*Chmod"
# Check renameio implementation as it's used in multiple places
echo -e "\n=== renameio implementation ==="
rg -A 5 "Chmod.*windows" base/utils/renameio/
# Check database storage implementation
echo -e "\n=== Database storage implementation ==="
rg -A 5 "acl.*Chmod" base/database/storage/
Length of output: 737
go.mod (1)
35-35: Verify the version of go-acl package
The added dependency github.com/hectane/go-acl is using a dated version from January 2023 (v0.0.0-20230122075934-ca0b05cb1adb). Since this package is critical for Windows permission handling:
- Check if there's a newer stable version available
- Consider the API stability implications of using a v0.x.x version
✅ Verification successful
Consider upgrading to a newer commit of go-acl package
The current version (v0.0.0-20230122075934-ca0b05cb1adb) is not the latest. Recent commits show important fixes:
- Error code propagation fix (merged via PR Fix setting data root in startup procedure #17)
- Cross-platform Chmod support (PR Update profile defaults #15)
While there are open issues, they are either feature requests or dependency updates, not critical bugs. The package remains actively maintained with recent commits in 2024.
Recommendation:
- Update to the latest commit to benefit from the error handling improvements
- The v0.x.x version is acceptable as the package is stable despite the version number, evidenced by its long history (since 2017) and focused scope
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check the latest version and commit history of the go-acl package
gh api repos/hectane/go-acl/commits?per_page=5 --jq '.[].commit.message'
# Check for any reported issues or security concerns
gh api repos/hectane/go-acl/issues?state=all --jq '.[] | {title: .title, state: .state, created_at: .created_at}'
Length of output: 3016
base/database/storage/fstree/fstree.go (1)
292-298:
Critical: Fix permission setting logic for Windows
The current implementation has a potential issue when setting permissions on Windows:
- It attempts to set permissions on the target file (
filename) which might not exist yet - This differs from the non-Windows path which correctly sets permissions on the temp file
Consider this fix:
if onWindows {
- err = acl.Chmod(filename, perm)
+ err = acl.Chmod(t.Name(), perm)
} else {
err = t.Chmod(perm)
}Let's verify the usage of acl.Chmod in similar scenarios across the codebase:
✅ Verification successful
Let me analyze the usage patterns of acl.Chmod to make a definitive decision. I need to check the context of the renameio package to understand the correct pattern.
Let me check the complete implementation of the temp file handling to understand the full context.
Review comment is correct - Windows permission setting needs to be fixed
The review comment correctly identified a critical issue. Looking at the reference implementation in base/utils/renameio/writefile.go, we can see that permissions should be set on the temporary file (t.path) before writing data, not on the target filename. This is consistent across the codebase where atomic file operations are performed.
The current implementation in fstree.go deviates from this pattern and could indeed cause issues when the target file doesn't exist yet. The suggested fix in the review comment to use t.Name() instead of filename is the correct approach, making it consistent with the reference implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other instances of acl.Chmod usage to ensure consistent patterns
rg "acl\.Chmod" -A 3 -B 3
Length of output: 3330
Script:
#!/bin/bash
# Check the implementation of renameio WriteFile and related functions
rg -A 10 -B 10 "func WriteFile|func write" base/utils/renameio/
Length of output: 1196
Script:
#!/bin/bash
# Check the TempFile implementation and temp file path handling
cat base/utils/renameio/writefile.go
Length of output: 690
service/updates/upgrader.go (2)
14-14: LGTM: Appropriate package for Windows ACL handling
The addition of the go-acl package is aligned with the PR's objective to fix file permissions on Windows.
353-358: Verify consistent usage of acl.Chmod across the codebase
Let's ensure that all file permission operations on Windows use acl.Chmod consistently.
✅ Verification successful
Let me analyze the context further by checking the implementation details of these files.
Based on the analysis of the search results, I can now generate the final response:
Windows file permission handling is consistently implemented
The codebase shows a consistent pattern where acl.Chmod is used for Windows-specific file permission handling, while os.Chmod is used for non-Windows systems. The implementation in upgrader.go follows this pattern correctly:
- All Windows-specific permission changes use
acl.Chmod - The code is properly wrapped in
if onWindowsconditions - The same permission mode (0o0755) is consistently used across similar operations
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for direct os.Chmod calls that might need to be replaced with acl.Chmod for Windows
rg "os\.Chmod\(" --type go
# Search for existing acl.Chmod usage to verify consistency
rg "acl\.Chmod\(" --type go
Length of output: 1193
Script:
#!/bin/bash
# Check the implementation of os_integration files for Windows and Linux
rg -A 5 "package updates" service/updates/os_integration_
# Check if there are any Windows-specific files or conditions
rg -B 2 -A 2 "if.*[Ww]indows" --type go
Length of output: 11926
base/updater/fetch.go (2)
17-17: LGTM: Appropriate package for Windows ACL management
The addition of github.com/hectane/go-acl is a good choice for handling Windows file permissions.
🧰 Tools
🪛 golangci-lint (1.62.2)
17-17: File is not gci-ed with --skip-generated -s standard -s default
(gci)
🪛 GitHub Check: Linter
[failure] 17-17:
File is not gci-ed with --skip-generated -s standard -s default (gci)
140-145: LGTM: Proper Windows ACL handling implemented
The implementation correctly uses platform-specific APIs (acl.Chmod for Windows, os.Chmod for others) while maintaining consistent behavior across platforms. Error handling is appropriate with non-fatal warnings.
cmds/portmaster-start/run.go (2)
16-16: LGTM: Appropriate package for Windows ACL operations
The addition of github.com/hectane/go-acl is well-suited for handling Windows-specific file permissions.
276-281:
Remove redundant early return and improve error handling
There are several issues in the permission handling logic:
- The early
if onWindows { return nil }check at the beginning of the function conflicts with the new Windows-specific code path. - Windows and non-Windows error cases should be handled separately for better debugging.
Apply this diff to fix the issues:
func fixExecPerm(path string) error {
- if onWindows {
- return nil
- }
info, err := os.Stat(path)
if err != nil {
return fmt.Errorf("failed to stat %s: %w", path, err)
}
if info.Mode() == 0o0755 {
return nil
}
if runtime.GOOS == "windows" {
err = acl.Chmod(path, 0o0755)
+ if err != nil {
+ return fmt.Errorf("failed to set Windows ACL permissions on %s: %w", path, err)
+ }
} else {
err = os.Chmod(path, 0o0755)
+ if err != nil {
+ return fmt.Errorf("failed to chmod %s: %w", path, err)
+ }
}
- if err != nil {
- return fmt.Errorf("failed to chmod %s: %w", path, err)
- }
return nil
}Let's verify the Windows ACL permission value compatibility:
✅ Verification successful
Let me analyze the search results and generate additional verification scripts to check the Windows ACL permission handling.
The first search will help identify the ACL permission patterns, while the second will show how permissions are handled across Windows and non-Windows platforms. This will help verify if the 0o0755 permission value is consistently and correctly handled.
Based on the search results, I can now provide a final response since I have enough information about how permissions are handled across the codebase:
The permission handling changes are correct and consistent with codebase patterns
The changes to the permission handling logic are valid and follow established patterns in the codebase:
- The 0o0755 permission value is consistently used across Windows and non-Windows platforms as shown in multiple files (base/updater/fetch.go, service/updates/upgrader.go, etc.)
- The Windows-specific acl.Chmod() vs os.Chmod() branching pattern is consistently implemented across the codebase
- The early return for Windows should indeed be removed as it conflicts with the proper Windows ACL permission handling that's now implemented
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check if the codebase handles Windows ACL permissions consistently
# and if there are any Windows-specific permission patterns or constants defined.
# Search for Windows-specific permission handling
rg -A 3 'windows|acl\.Chmod|0o0755|0755' --type go
Length of output: 103178
Script:
#!/bin/bash
# Search for ACL permission handling patterns and constants
ast-grep --pattern 'acl.Chmod($path, $_)'
# Search for Windows-specific permission handling in the codebase
rg -A 3 'acl\.Chmod|os\.Chmod' --type go
Length of output: 3374
base/utils/fs.go
Outdated
| } | ||
| return os.Chmod(path, perm) | ||
| if isWindows { | ||
| return acl.Chmod(path, perm) | ||
| } else if f.Mode().Perm() != perm { | ||
| return os.Chmod(path, perm) | ||
| } |
There was a problem hiding this comment.
Fix unreachable and potentially dangerous code
There are several issues in this section:
- The
fvariable is nil in this code path as we just created the directory - The permission check is redundant as we just created the directory with the specified permissions
- The else condition would cause a nil pointer panic
Apply this fix:
if isWindows {
return acl.Chmod(path, perm)
-} else if f.Mode().Perm() != perm {
- return os.Chmod(path, perm)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } | |
| return os.Chmod(path, perm) | |
| if isWindows { | |
| return acl.Chmod(path, perm) | |
| } else if f.Mode().Perm() != perm { | |
| return os.Chmod(path, perm) | |
| } | |
| } | |
| if isWindows { | |
| return acl.Chmod(path, perm) | |
| } |
base/utils/fs.go
Outdated
| if isWindows { | ||
| // TODO: set correct permission on windows | ||
| // acl.Chmod(path, perm) | ||
| return acl.Chmod(path, perm) | ||
| } else if f.Mode().Perm() != perm { | ||
| return os.Chmod(path, perm) | ||
| } |
There was a problem hiding this comment.
Fix potential nil pointer dereference in permission check
The permission check in the else condition could cause a panic when the file doesn't exist, as f would be nil.
Apply this fix:
if isWindows {
return acl.Chmod(path, perm)
-} else if f.Mode().Perm() != perm {
+} else if f != nil && f.Mode().Perm() != perm {
return os.Chmod(path, perm)
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isWindows { | |
| // TODO: set correct permission on windows | |
| // acl.Chmod(path, perm) | |
| return acl.Chmod(path, perm) | |
| } else if f.Mode().Perm() != perm { | |
| return os.Chmod(path, perm) | |
| } | |
| if isWindows { | |
| return acl.Chmod(path, perm) | |
| } else if f != nil && f.Mode().Perm() != perm { | |
| return os.Chmod(path, perm) | |
| } |
service/updates/helper/electron.go
Outdated
| if runtime.GOOS == "windows" { | ||
| err = acl.Chmod(sandboxFile, 0o0755|os.ModeSetuid) | ||
| } else { | ||
| err = os.Chmod(sandboxFile, 0o0755|os.ModeSetuid) | ||
| } |
There was a problem hiding this comment.
Critical: Inconsistent OS handling and unnecessary Windows logic
There are several issues with the Windows-specific changes:
- The function documentation states it's Linux-specific, but now includes Windows logic
- There's an early return for non-Linux systems (line 26) that makes this Windows-specific code unreachable
- Setting SUID bit on Windows is unnecessary as Windows doesn't use the SUID concept
- The Unix permission mask might not translate correctly to Windows ACLs
Consider removing the Windows-specific code since:
- The function is designed for Linux-specific Chrome sandbox SUID requirements
- The early return makes this code unreachable
- SUID permissions aren't applicable to Windows
- if runtime.GOOS == "windows" {
- err = acl.Chmod(sandboxFile, 0o0755|os.ModeSetuid)
- } else {
- err = os.Chmod(sandboxFile, 0o0755|os.ModeSetuid)
- }
+ err = os.Chmod(sandboxFile, 0o0755|os.ModeSetuid)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if runtime.GOOS == "windows" { | |
| err = acl.Chmod(sandboxFile, 0o0755|os.ModeSetuid) | |
| } else { | |
| err = os.Chmod(sandboxFile, 0o0755|os.ModeSetuid) | |
| } | |
| err = os.Chmod(sandboxFile, 0o0755|os.ModeSetuid) |
base/utils/atomic.go
Outdated
| if runtime.GOOS == "windows" { | ||
| err = acl.Chmod(path.Join(opts.TempDir, dest), opts.Mode) | ||
| } else { | ||
| err = tmpFile.Chmod(opts.Mode) | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
Use filepath.Join instead of path.Join for cross-platform compatibility.
The path package is designed for slash-separated paths (like URLs), while filepath is designed for OS-specific path manipulation. On Windows, using path.Join could lead to incorrect path separators.
-import "path"
+import "path/filepath"
- err = acl.Chmod(path.Join(opts.TempDir, dest), opts.Mode)
+ err = acl.Chmod(filepath.Join(opts.TempDir, dest), opts.Mode)Committable suggestion skipped: line range outside the PR's diff.
💡 Codebase verification
🛠️ Refactor suggestion
The review comment is correct. The temporary file path is incorrectly constructed.
Based on the code analysis:
renameio.TempFilecreates a temporary file with pattern.{basename}in the directory determined bytempDirfunction- The
tempDirfunction either uses the caller-specified directory or falls back to the destination file's directory - The current code incorrectly assumes the temp file is at
TempDir/dest, but it's actually at a path returned bytmpFile.Name()
The suggested fix to use tmpFile.Name() is the correct approach as it will use the actual temporary file path rather than an incorrect assumption about its location.
🔗 Analysis chain
Verify the temporary file path construction.
The code attempts to chmod the file at TempDir/dest, but renameio.TempFile might use a different path pattern for the temporary file. This could result in permission changes being applied to the wrong file.
Additionally, consider refactoring to use the actual temporary file path:
if opts.Mode != 0 {
if runtime.GOOS == "windows" {
- err = acl.Chmod(path.Join(opts.TempDir, dest), opts.Mode)
+ err = acl.Chmod(tmpFile.Name(), opts.Mode)
} else {
err = tmpFile.Chmod(opts.Mode)
}Let me check how the tempDir function constructs the temporary directory path.
Let me check the complete implementation of tempDir to understand how it determines the final temporary directory path.
Let me check how the TempFile is being used in the atomic.go file to understand the complete context.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check how renameio.TempFile constructs temporary paths
rg -A 10 'func TempFile' --type go
Length of output: 762
Script:
#!/bin/bash
# Check how tempDir function constructs temporary directory paths
rg -A 5 'func tempDir' --type go
Length of output: 411
Script:
#!/bin/bash
# Get the complete tempDir function implementation
rg -A 15 'func tempDir' --type go
Length of output: 1109
Script:
#!/bin/bash
# Get the context of TempFile usage in atomic.go
rg -B 10 -A 10 'TempFile' base/utils/atomic.go
Length of output: 807
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
service/updates/helper/electron.go (1)
54-54: Consider OS-specific debug loggingThe debug message about fixing SUID permissions is misleading on non-Linux systems where this function returns early.
- log.Debugf("updates: fixed SUID permission for chrome-sandbox") + log.Debugf("updates: fixed SUID permission for chrome-sandbox on Linux")
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
base/utils/atomic.go(2 hunks)cmds/portmaster-start/run.go(3 hunks)service/updates/helper/electron.go(2 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- base/utils/atomic.go
- cmds/portmaster-start/run.go
🧰 Additional context used
🪛 GitHub Check: Linter
service/updates/helper/electron.go
[failure] 11-11:
"github.com/hectane/go-acl" imported as acl and not used (typecheck)
[failure] 11-11:
"github.com/hectane/go-acl" imported as acl and not used) (typecheck)
🔇 Additional comments (1)
service/updates/helper/electron.go (1)
Line range hint 22-57: Implementation doesn't align with PR objectives
This function is explicitly designed for Linux systems (as stated in the documentation) and includes an early return for non-Linux systems. However, the PR's objective is to "Fix file permissions on windows". There seems to be a disconnect between the PR's purpose and this implementation.
Consider:
- Either this function should remain Linux-specific (current state)
- Or it needs to be refactored to handle Windows-specific requirements, which would involve:
- Updating the function documentation
- Removing the early return for non-Linux systems
- Implementing proper Windows permission handling using
acl.Chmod
Let's verify if this function is called on Windows systems:
service/updates/helper/electron.go
Outdated
| @@ -8,6 +8,7 @@ | |||
| "runtime" | |||
| "strings" | |||
|
|
|||
| "github.com/hectane/go-acl" | |||
There was a problem hiding this comment.
Remove unused import
The go-acl package is imported but never used in this file. This appears to be an oversight, possibly from an incomplete implementation of Windows-specific permission handling.
- "github.com/hectane/go-acl"📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| "github.com/hectane/go-acl" |
🧰 Tools
🪛 GitHub Check: Linter
[failure] 11-11:
"github.com/hectane/go-acl" imported as acl and not used (typecheck)
[failure] 11-11:
"github.com/hectane/go-acl" imported as acl and not used) (typecheck)
vlabo
force-pushed
the
fix/windows-permissions
branch
from
5497595
to
447df43
Compare
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
base/utils/atomic.go(2 hunks)cmds/portmaster-start/run.go(3 hunks)service/updates/helper/electron.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- cmds/portmaster-start/run.go
- service/updates/helper/electron.go
🧰 Additional context used
🪛 golangci-lint (1.62.2)
base/utils/atomic.go
12-12: File is not gci-ed with --skip-generated -s standard -s default
(gci)
🪛 GitHub Check: Linter
base/utils/atomic.go
[failure] 12-12:
File is not gci-ed with --skip-generated -s standard -s default (gci)
🔇 Additional comments (1)
base/utils/atomic.go (1)
9-12: LGTM: Import changes are appropriate
The added imports support the Windows-specific permission handling requirements correctly.
🧰 Tools
🪛 golangci-lint (1.62.2)
12-12: File is not gci-ed with --skip-generated -s standard -s default
(gci)
🪛 GitHub Check: Linter
[failure] 12-12:
File is not gci-ed with --skip-generated -s standard -s default (gci)
base/utils/atomic.go
Outdated
| if runtime.GOOS == "windows" { | ||
| err = acl.Chmod(filepath.Join(opts.TempDir, dest), opts.Mode) | ||
| } else { | ||
| err = tmpFile.Chmod(opts.Mode) | ||
| } | ||
| if err != nil { |
There was a problem hiding this comment.
Critical: Incorrect temporary file path used for Windows permissions
The code attempts to set permissions on filepath.Join(opts.TempDir, dest), but this is not the actual path of the temporary file. The temporary file's actual path is available via tmpFile.Name().
Apply this fix to ensure permissions are set on the correct file:
if opts.Mode != 0 {
if runtime.GOOS == "windows" {
- err = acl.Chmod(filepath.Join(opts.TempDir, dest), opts.Mode)
+ err = acl.Chmod(tmpFile.Name(), opts.Mode)
} else {
err = tmpFile.Chmod(opts.Mode)
}This ensures that:
- Permissions are set on the actual temporary file
- The behavior is consistent with the non-Windows path where
tmpFile.Chmodis used
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if runtime.GOOS == "windows" { | |
| err = acl.Chmod(filepath.Join(opts.TempDir, dest), opts.Mode) | |
| } else { | |
| err = tmpFile.Chmod(opts.Mode) | |
| } | |
| if err != nil { | |
| if runtime.GOOS == "windows" { | |
| err = acl.Chmod(tmpFile.Name(), opts.Mode) | |
| } else { | |
| err = tmpFile.Chmod(opts.Mode) | |
| } | |
| if err != nil { |
💡 Codebase verification
Based on the gathered information, I can now provide a final response about the temporary file handling implementation:
Incorrect path handling in Windows-specific code
The current implementation has a path handling issue in the Windows-specific branch. When calling acl.Chmod(filepath.Join(opts.TempDir, dest), opts.Mode), it incorrectly reconstructs the temporary file path. The actual temporary file is created by renameio.TempFile() which:
- Uses
tempDir()function to determine the correct temporary directory - Creates the temp file with a unique name using
os.CreateTemp()with pattern "."+filepath.Base(path)
The Windows branch should use the actual temporary file path from the tmpFile object instead of trying to reconstruct it. This could lead to permission setting failures on Windows as it's targeting the wrong path.
Affected locations:
base/utils/atomic.go:48: Should usetmpFile.Name()instead offilepath.Join(opts.TempDir, dest)
🔗 Analysis chain
Verify temporary file handling implementation
Let's verify how the temporary files are created and managed to understand the full impact of the path issue.
Let me gather more information about the temporary directory handling to fully understand the path construction.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check renameio.TempFile implementation and usage patterns
# Check TempFile implementation
echo "Checking TempFile implementation:"
rg -A 10 'func TempFile' --type go
# Check for other instances of similar permission handling
echo "Checking for similar permission patterns:"
ast-grep --pattern 'if runtime.GOOS == "windows" {
$$$
acl.Chmod($_, $_)
$$$
}'
Length of output: 1020
Script:
#!/bin/bash
# Check tempDir implementation and related path handling
echo "Checking tempDir implementation:"
rg -A 10 'func tempDir' --type go
# Check atomic write implementation context
echo "Checking atomic write implementation:"
rg -B 5 -A 10 'TempFile\(' --type go
# Check for any other Windows-specific path handling
echo "Checking Windows-specific path handling:"
rg 'filepath.Join.*windows' --type go
Length of output: 10327
base/utils/atomic.go
Outdated
| @@ -40,7 +43,12 @@ func CreateAtomic(dest string, r io.Reader, opts *AtomicFileOptions) error { | |||
| defer tmpFile.Cleanup() //nolint:errcheck | |||
|
|
|||
| if opts.Mode != 0 { | |||
| if err := tmpFile.Chmod(opts.Mode); err != nil { | |||
| if runtime.GOOS == "windows" { | |||
| err = acl.Chmod(filepath.Join(opts.TempDir, dest), opts.Mode) | |||
There was a problem hiding this comment.
Did you really mean to change the permission of the directory and not the permissions of the file?
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
base/utils/fs.go (1)
25-27: Improve error handling and documentationWhile ignoring permission errors for non-admin users is intentional, consider these improvements:
- Log the error at debug level for troubleshooting
- Fix the typo in the comment ("none" should be "non")
- // Ignore windows permission error. For none admin users it will always fail. - acl.Chmod(path, perm) + // Ignore windows permission error. For non-admin users it will always fail. + if err := acl.Chmod(path, perm); err != nil { + // Log error at debug level since it's expected for non-admin users + log.Debugf("Failed to set permissions on %s: %v", path, err) + }🧰 Tools
🪛 GitHub Check: Linter
[failure] 26-26:
Error return value is not checked (errcheck)🪛 golangci-lint (1.62.2)
26-26: Error return value is not checked
(errcheck)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
base/utils/fs.go(3 hunks)cmds/portmaster-start/run.go(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- cmds/portmaster-start/run.go
🧰 Additional context used
🪛 GitHub Check: Linter
base/utils/fs.go
[failure] 26-26:
Error return value is not checked (errcheck)
[failure] 46-46:
Error return value is not checked (errcheck)
🪛 golangci-lint (1.62.2)
base/utils/fs.go
26-26: Error return value is not checked
(errcheck)
46-46: Error return value is not checked
(errcheck)
🔇 Additional comments (2)
base/utils/fs.go (2)
9-10: LGTM: Clean import addition
The go-acl package is properly imported for Windows-specific permission handling.
Line range hint 16-54: Verify cross-platform permission behavior
The changes look good overall, but we should verify that the permission handling works consistently across platforms.
✅ Verification successful
Permission handling is consistent across the codebase
The verification shows that permission handling follows a consistent pattern across the codebase:
- Windows: Uses
acl.Chmod()with ignored errors for non-admin users - Non-Windows: Uses
os.Chmod()with proper error handling
The implementation in EnsureDirectory() aligns with this established pattern, as evidenced by similar implementations in:
base/database/storage/fstree/fstree.gobase/utils/renameio/writefile.gobase/updater/fetch.go
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Search for other uses of os.Chmod to ensure consistent handling across the codebase
rg --type go 'os\.Chmod|acl\.Chmod' -B 2 -A 2
# Look for any related permission-setting patterns
ast-grep --pattern 'if isWindows {
$$$
}'
Length of output: 4630
🧰 Tools
🪛 GitHub Check: Linter
[failure] 46-46:
Error return value is not checked (errcheck)
🪛 golangci-lint (1.62.2)
46-46: Error return value is not checked
(errcheck)
| if isWindows { | ||
| // Ignore windows permission error. For none admin users it will always fail. | ||
| acl.Chmod(path, perm) | ||
| return nil | ||
| } else { | ||
| return os.Chmod(path, perm) | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Refactor to reduce duplication and simplify control flow
The Windows permission handling code is duplicated and the control flow could be simplified.
- if isWindows {
- // Ignore windows permission error. For none admin users it will always fail.
- acl.Chmod(path, perm)
- return nil
- } else {
- return os.Chmod(path, perm)
- }
+ if isWindows {
+ // Ignore windows permission error. For non-admin users it will always fail.
+ if err := acl.Chmod(path, perm); err != nil {
+ log.Debugf("Failed to set permissions on %s: %v", path, err)
+ }
+ return nil
+ }
+ return os.Chmod(path, perm)📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if isWindows { | |
| // Ignore windows permission error. For none admin users it will always fail. | |
| acl.Chmod(path, perm) | |
| return nil | |
| } else { | |
| return os.Chmod(path, perm) | |
| } | |
| if isWindows { | |
| // Ignore windows permission error. For non-admin users it will always fail. | |
| if err := acl.Chmod(path, perm); err != nil { | |
| log.Debugf("Failed to set permissions on %s: %v", path, err) | |
| } | |
| return nil | |
| } | |
| return os.Chmod(path, perm) |
🧰 Tools
🪛 GitHub Check: Linter
[failure] 46-46:
Error return value is not checked (errcheck)
🪛 golangci-lint (1.62.2)
46-46: Error return value is not checked
(errcheck)
* [service] Set file permissions on windows * [service] Fix minor windows permission bugs * [service] Fix permission bugs * [service] Fix windows non admin user start
Summary by CodeRabbit
New Features
Bug Fixes
Chores
github.com/hectane/go-aclfor better cross-platform support.