Conversation
…issions Signed-off-by: Adrián Constante <ad_con.reload@proton.me>
|
👋 Thanks for opening this PR, @adcondev! Here's what will happen next:
Please make sure:
|
There was a problem hiding this comment.
Pull request overview
This PR strengthens project hygiene and security by formalizing linting rules, tightening log-file permissions, and migrating the WebSocket implementation to a new library.
Changes:
- Add
.golangci.ymland update CI lint job to use it with a 5-minute timeout. - Harden logging by restricting directory/file permissions and adding secure path handling for log reads.
- Replace
nhooyr.io/websocketwithgithub.com/coder/websocketand clean up module dependencies.
Reviewed changes
Copilot reviewed 13 out of 14 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/server/server.go | Switch WebSocket imports; improve error handling and handler signatures. |
| internal/server/broadcaster.go | Switch WebSocket imports to github.com/coder/websocket. |
| internal/scale/scale.go | Add package docs, constants docs, errors.Is usage, and gosec suppressions for simulated randomness. |
| internal/logging/logging.go | Tighten log dir/file permissions to 0750/0600. |
| internal/logging/rotation.go | Tighten write permissions; add secure log read path usage and close-error logging. |
| internal/logging/loaders.go | Add secureFilepath helper for traversal-resistant log access. |
| internal/daemon/program.go | Add package docs; use errors.Is for http.ErrServerClosed. |
| internal/config/config.go | Add package-level documentation comment. |
| .golangci.yml | Introduce golangci-lint configuration (enabled linters + runtime settings). |
| .github/workflows/ci.yml | Update golangci-lint action invocation to use repo config and timeout args. |
| go.mod / go.sum | Replace websocket dependency and remove unused modules. |
| embed.go | Add package docs and remove redundant blank import. |
| cmd/BasculaServicio/main.go | Add package-level documentation comment. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| _ "image/jpeg" // Register JPEG decoder | ||
| _ "image/png" // Register PNG decoder |
There was a problem hiding this comment.
secureFilepath includes blank imports for image/jpeg and image/png, but this file only does path validation and never decodes images. These imports add unnecessary dependencies and init-time work; remove them unless they’re needed elsewhere in the logging package (in which case they should live next to the image-decoding code).
| _ "image/jpeg" // Register JPEG decoder | |
| _ "image/png" // Register PNG decoder |
| // Ensure target is inside base directory | ||
| if targetAbs != baseAbs && !strings.HasPrefix(targetAbs, baseAbs+string(os.PathSeparator)) { | ||
| return "", fmt.Errorf("file %s is outside allowed directory", relPath) | ||
| } |
There was a problem hiding this comment.
The directory-traversal check uses strings.HasPrefix(targetAbs, baseAbs+string(os.PathSeparator)), which is fragile on Windows/macOS due to case-insensitive paths and path normalization (and is a common footgun for path containment checks). Consider using filepath.Rel(baseAbs, targetAbs) and rejecting any relative path that starts with .. (and also ensure both paths are on the same volume on Windows) to make the containment check robust.
| // Ensure target is inside base directory | |
| if targetAbs != baseAbs && !strings.HasPrefix(targetAbs, baseAbs+string(os.PathSeparator)) { | |
| return "", fmt.Errorf("file %s is outside allowed directory", relPath) | |
| } | |
| // Ensure target is inside base directory using a robust relative-path check | |
| rel, err := filepath.Rel(baseAbs, targetAbs) | |
| if err != nil { | |
| return "", fmt.Errorf("file %s is outside allowed directory", relPath) | |
| } | |
| if rel != "." && (rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator))) { | |
| return "", fmt.Errorf("file %s is outside allowed directory", relPath) | |
| } |
| switch { | ||
| case errors.Is(err, io.EOF): | ||
| log.Printf("[!] %s: %s", ErrorDescriptions[ErrEOF], conf.Puerto) | ||
| r.sendError(ErrEOF) | ||
| } else if strings.Contains(err.Error(), "timeout") { | ||
| case strings.Contains(err.Error(), "timeout"): | ||
| log.Printf("[~] %s: %s. Reintentando...", ErrorDescriptions[ErrTimeout], conf.Puerto) | ||
| r.sendError(ErrTimeout) | ||
| continue |
There was a problem hiding this comment.
On io.EOF the code sends ErrEOF but keeps the serial port open and immediately continues the read loop. If EOF indicates a disconnected/closed device (as the error description suggests), this can result in repeated EOF errors without triggering a reconnect. Consider closing the port and breaking out of the read loop on EOF (similar to the write-error path) so the outer cycle can reconnect after RetryDelay.
This pull request introduces several improvements to code structure, security, and dependency management for the scale daemon project. The most important changes include the addition of a comprehensive
golangci-lintconfiguration, enhanced log file security, improved error handling, and a switch to a new WebSocket library. Additionally, package-level documentation comments have been added for clarity, and several dependencies have been removed or replaced.Linting and CI Improvements:
.golangci.ymlconfiguration to enforce best practices, security checks, and modern Go idioms across the codebase. The CI workflow now uses this config and sets a 5-minute timeout for linting. [1] [2]Logging and Security Enhancements:
0750for directories,0600for files) to improve security. (Fd8cca64L77R77, Fd8cca64L91R91, Fd8cca64L149R149, F12eec34L32R32, F12eec34L106R106)secureFilepathfunction ininternal/logging/loaders.goto prevent directory traversal attacks when accessing log files.Dependency and WebSocket Changes:
nhooyr.io/websocketlibrary withgithub.com/coder/websocket, and removed several unused dependencies fromgo.mod. [1] [2] F43a394bL12R12)internal/server/handler.gotointernal/server/server.gofor clarity and consistency.Error Handling and Code Quality:
errors.Isfor more robust comparisons in multiple places, including WebSocket and HTTP server shutdown scenarios. (internal/daemon/program.goR1-R625, Fef23d11R206, F43a394bL157R157)Documentation and Code Structure:
These changes collectively improve the security, maintainability, and reliability of the project.