🛡️ Sentinel: [HIGH] Fix predictable temp files in atomic writes#75
🛡️ Sentinel: [HIGH] Fix predictable temp files in atomic writes#75
Conversation
This commit secures the atomic file saving operations in `internal/burnrate/burnrate.go` and `internal/sparkline/sparkline.go`. **Vulnerability:** Previously, `os.WriteFile` was used with predictable temporary file paths (e.g., `path + ".tmp"`). This exposed the application to symlink attacks, allowing unauthorized modifications by replacing the predicted `.tmp` path with a symbolic link before the write occurs. **Fix:** Refactored the file save operations to use `os.CreateTemp` from the `os` package, securely generating unpredictable filenames in the same directory as the target path. The temporary file is explicitly chmoded (0644) and closed before being atomically renamed to its final location. **Verification:** Validated correct behavior by running `go test ./...` and `go fmt ./...`. The updated implementation ensures temporary files are safely created and handles cross-device rename scenarios gracefully by generating the temp file in `filepath.Dir(path)`. 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 security vulnerability by replacing predictable temporary file paths with unpredictable ones using os.CreateTemp for atomic file writes. The changes span documentation and implementation in the burnrate and sparkline packages. Feedback includes a suggestion to use a defer block for more idiomatic and robust cleanup of temporary files, and a recommendation to handle the ignored error from os.Rename to avoid leaving orphaned files on disk.
| if _, err := tmpFile.Write(data); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return nil, false, err | ||
| } | ||
|
|
||
| if err := tmpFile.Chmod(0644); err != nil { | ||
| tmpFile.Close() | ||
| os.Remove(tmpPath) | ||
| return nil, false, err | ||
| } | ||
|
|
||
| if err := tmpFile.Close(); err != nil { | ||
| os.Remove(tmpPath) | ||
| return nil, false, err | ||
| } | ||
|
|
||
| if err := os.Rename(tmpPath, path); err != nil { | ||
| os.Remove(tmpPath) | ||
| return nil, false, err |
There was a problem hiding this comment.
The error handling for the temporary file operations is quite repetitive and prone to errors if new steps are added. Using a defer block with a cleanup flag is a more idiomatic Go pattern that ensures resources are released and temporary files are cleaned up on any error path.
cleanup := true
defer func() {
tmpFile.Close()
if cleanup {
os.Remove(tmpPath)
}
}()
if _, err := tmpFile.Write(data); err != nil {
return nil, false, err
}
if err := tmpFile.Chmod(0644); err != nil {
return nil, false, err
}
if err := tmpFile.Close(); err != nil {
return nil, false, err
}
if err := os.Rename(tmpPath, path); err != nil {
return nil, false, err
}
cleanup = false| } | ||
| os.Rename(tmp, path) | ||
|
|
||
| os.Rename(tmpPath, path) |
There was a problem hiding this comment.
The os.Rename call ignores the return error. If the rename fails (e.g., due to permission issues or filesystem limits), the temporary file will be left orphaned on disk. While the filenames are now unpredictable, it is still best practice to clean up the temporary file if the atomic swap fails.
if err := os.Rename(tmpPath, path); err != nil {
os.Remove(tmpPath)
}
Understood. Acknowledging that this work is now superseded and stopping work on this task. |
1 participant
🚨 Severity: HIGH
💡 Vulnerability: Predictable
.tmpfile paths were used withos.WriteFileduring atomic writes, making the application susceptible to symlink/TOCTOU attacks.🎯 Impact: If exploited, attackers could use symlinks for predictable filenames to overwrite sensitive or arbitrary files, depending on permissions and application context.
🔧 Fix: Replaced the predictable
.tmppaths withos.CreateTemp(filepath.Dir(path), filepath.Base(path)+"-*")to ensure unpredictability and proper directory isolation. Added explicitChmodandClosecalls to maintain correct permissions and lock safety before the atomicos.Rename.✅ Verification: Ran
go test ./...and verified that no existing tests failed and formats complied.PR created automatically by Jules for task 17412171799410978877 started by @himattm