Conversation
Signed-off-by: Johannes Großmann <grossmann.johannes@t-online.de>
There was a problem hiding this comment.
Review Summary
Assessment: 🔴 CRITICAL
This PR changes the Windows socket path from ProgramData (system-wide) to LOCALAPPDATA (user-specific). While this may be intentional, the implementation has critical bugs that will break existing installations and could cause runtime failures.
Critical Issues Found
- Broken fallback path logic - Can produce relative paths instead of absolute paths
- Breaking change without migration - No upgrade path for existing installations
- Undocumented security model change - Shifts from system-wide to user-specific without justification
Details
All 3 issues are in the newly changed code and were verified to be real bugs.
This review focuses only on the changed code. Pre-existing code was not evaluated.
| base := os.Getenv("LOCALAPPDATA") | ||
| if base == "" { | ||
| base = `C:\ProgramData` | ||
| base = filepath.Join(os.Getenv("USERPROFILE"), "AppData", "Local") |
There was a problem hiding this comment.
🔴 HIGH SEVERITY - Missing validation for USERPROFILE environment variable
If both LOCALAPPDATA and USERPROFILE environment variables are unset or empty, os.Getenv("USERPROFILE") returns an empty string. This causes filepath.Join("", "AppData", "Local") to produce a relative path "AppData\Local" instead of an absolute path.
This could cause:
- Socket created in unexpected location (current working directory)
- Connection failures between daemon and client
- Security issues if processes have different working directories
The old code had a hardcoded absolute fallback (C:\ProgramData), which prevented this issue.
Recommendation: Add validation for USERPROFILE or use a hardcoded absolute fallback:
if base == "" {
userProfile := os.Getenv("USERPROFILE")
if userProfile == "" {
base = `C:\Users\Default\AppData\Local` // or another safe default
} else {
base = filepath.Join(userProfile, "AppData", "Local")
}
}|
|
||
| func DaemonSocketPath() string { | ||
| base := os.Getenv("ProgramData") | ||
| base := os.Getenv("LOCALAPPDATA") |
There was a problem hiding this comment.
🔴 HIGH SEVERITY - Breaking change without migration path
This change moves the socket from ProgramData (typically C:\ProgramData) to LOCALAPPDATA (typically C:\Users\<username>\AppData\Local). This breaks backward compatibility:
- Existing installations will have the socket in the old location
- After upgrade, the daemon creates a socket in the new location
- Clients using the old path will fail to connect
- No automatic cleanup of old sockets
- No fallback mechanism to check the old location
Impact: Systems will be left in an inconsistent state during upgrades.
Recommendations:
- Add migration logic to move/cleanup old socket
- Implement fallback to check old location if new one doesn't exist
- Document the upgrade path
- Consider a configuration option for backwards compatibility
|
|
||
| func DaemonSocketPath() string { | ||
| base := os.Getenv("ProgramData") | ||
| base := os.Getenv("LOCALAPPDATA") |
There was a problem hiding this comment.
🟡 MEDIUM SEVERITY - Security model change without justification
This changes the security model from system-wide (ProgramData, requires admin privileges) to user-specific (LOCALAPPDATA, user-writable).
Potential issues:
- Multi-user scenarios: Socket now accessible only to specific user, potentially breaking shared daemon usage
- Privilege escalation risks: If daemon runs as SYSTEM but uses user's LOCALAPPDATA
- Deployment model change: Multiple users might create conflicting daemon instances
Recommendation: Document the intended security model and deployment scenario. If this change is intentional, add comments explaining:
- Why the move from system-wide to user-specific
- Expected privilege level for the daemon
- How multi-user scenarios should be handled
No description provided.