fix: harden security and fix resource leaks#63
Conversation
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughEnables gosec/bodyclose linters; tightens filesystem permissions (dirs 0750, files 0600); applies filepath.Clean() to file I/O; fixes HTTP response-body leaks; replaces math/rand with crypto/rand; adds overflow/clamping checks; limits zip extraction size and file count; expands and adds many unit tests. Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes
🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![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
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
internal/storage/node/store.go (1)
183-188:⚠️ Potential issue | 🟡 MinorDead code in traversal check:
len(rel) < 2 && rel == ".."is always false.
".."has length 2, solen(rel) < 2 && rel == ".."can never be true. The check relies entirely on the second clause. Consider simplifying tostrings.HasPrefix(rel, "..")or justrel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)).Proposed fix
- if err != nil || len(rel) < 2 && rel == ".." || (len(rel) >= 2 && rel[:2] == "..") { + if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) {Note: This would require adding
"strings"to imports.
🤖 Fix all issues with AI agents
In `@cmd/cloud/logs.go`:
- Around line 108-115: Replace the blank-identifier error discards for
table.Append and table.Render with proper error handling: capture the returned
errors from table.Append(...) and table.Render(), and if non-nil write a clear
error message to stderr (or the module's logger) including context (e.g., the
log entry timestamp/level/resource) so failures aren’t silent; specifically
update the calls to table.Append and table.Render to check their error returns
and log/report any error instead of using `_ =`.
In `@internal/core/services/function.go`:
- Around line 335-342: io.CopyN(dst, src, maxFileSize) will silently truncate
files larger than maxFileSize; change the logic so after calling io.CopyN you
detect the "too large" case: keep the existing io.CopyN call with maxFileSize
and then, if err == nil (meaning exactly maxFileSize were read but more data may
remain), attempt to read one byte from src (e.g. buf := make([]byte,1); n, err2
:= src.Read(buf)); if n>0 || (err2 != nil && err2 != io.EOF) return a specific
"file too large" error; preserve the existing error handling for err != nil &&
err != io.EOF and treat err == io.EOF as success. Ensure you reference
maxFileSize, io.CopyN, dst and src in the change.
In `@internal/repositories/filesystem/adapter.go`:
- Around line 108-109: The Assemble function currently uses
os.Create(filepath.Clean(destPath)) which yields files with default 0666
permissions; change this to use os.OpenFile with flags
os.O_CREATE|os.O_WRONLY|os.O_TRUNC and explicit mode 0600 (keeping
filepath.Clean(destPath)) so the created file honors the same 0600 hardening
used by Write; ensure the returned file handle is used/closed as before.
In `@internal/storage/node/store.go`:
- Around line 164-166: The write to the metadata file currently ignores errors
(_ = os.WriteFile(metaPath, buf, 0600)), causing silent failures; update the
code in store.go to capture the error from os.WriteFile (e.g., err :=
os.WriteFile(metaPath, buf, 0600)) and if non-nil return or propagate that error
from the enclosing function (the function that assembles the object which sets
uNow, buf, and metaPath), ensuring callers get a failure instead of proceeding
without a .meta file.
In `@tests/elastic_ip_e2e_test.go`:
- Around line 153-155: Replace the silent response closes with the existing
helper: instead of calling `_ = termResp.Body.Close()` directly, call the test
helper closeResponse(t, termResp) so close errors are logged; ensure you still
nil-check termResp before calling closeResponse (i.e., if termResp != nil {
closeResponse(t, termResp) }) and apply this change to both cleanup paths that
currently use `_ = termResp.Body.Close()`.
🧹 Nitpick comments (6)
cmd/api/main.go (1)
77-81: ExtractReadHeaderTimeoutvalue into a named constant.The
10 * time.Secondliteral is a magic number. As per coding guidelines, "Do not use magic numbers - use named constants instead".Suggested fix
Add near the existing
defaultDBInitTimeoutconstant:const ( defaultDBInitTimeout = 120 * time.Second + defaultReadHeaderTimeout = 10 * time.Second )Then reference it:
return &http.Server{ Addr: addr, Handler: handler, - ReadHeaderTimeout: 10 * time.Second, + ReadHeaderTimeout: defaultReadHeaderTimeout, }internal/repositories/docker/adapter_unit_test.go (1)
338-339: Permission tightening to0750looks good.Minor note: the
MkdirAllerror is silently discarded. If the directory creation fails, the test will likely fail with a confusing error downstream. Consider usingrequire.NoErrorort.Fatalinstead of_ =. As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like_ = someFunc()".Suggested fix
- _ = os.MkdirAll(configPath, 0750) + require.NoError(t, os.MkdirAll(configPath, 0750))cmd/cloud/storage.go (1)
144-144:os.Createuses permissive default mode (0666 before umask).Other parts of this PR tighten file permissions to
0600. For consistency and to match the PR's security hardening intent, consider usingos.OpenFilewith explicit0600permissions here, since the downloaded content could be sensitive.💡 Suggested change
- out, err := os.Create(filepath.Clean(dest)) + out, err := os.OpenFile(filepath.Clean(dest), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600).golangci.yml (2)
31-34: Global G106 exclusion makes per-file//nolint:gosec // G106directives redundant.G106 (ssh.InsecureIgnoreHostKey) is excluded globally here, yet
internal/repositories/k8s/node_executor.goline 78 also has a//nolint:gosec // G106directive. The per-line suppression is harmless but unnecessary. Consider keeping only one approach for consistency — either the global exclusion or the per-line nolint with its explanatory comment (the latter is more self-documenting).
47-49: Blanket gosec exclusion for all test files.This suppresses all gosec rules in test files. While convenient, it may hide genuine security issues in test helpers or fixtures that get reused. Consider excluding only specific gosec rules (e.g., G101, G304) in tests instead of the entire linter.
internal/core/services/function.go (1)
291-294: Use a named constant for the max file count.The limit
1000is a magic number. Per coding guidelines, use a named constant.Proposed fix
func (s *FunctionService) extractZip(rc io.Reader, tmpDir string) error { // Limit extraction to 50MB to prevent Zip bombs const maxZipSize = 50 * 1024 * 1024 + const maxZipFiles = 1000 lr := io.LimitReader(rc, maxZipSize) ... - if len(zr.File) > 1000 { + if len(zr.File) > maxZipFiles { return fmt.Errorf("too many files in zip: %d", len(zr.File)) }
| _ = table.Append([]string{ | ||
| e.Timestamp.Format(time.RFC3339), | ||
| e.Level, | ||
| resource, | ||
| e.Message, | ||
| }) | ||
| } | ||
| table.Render() | ||
| _ = table.Render() |
There was a problem hiding this comment.
Blank identifier assignments violate the coding guideline against silent failures.
Both _ = table.Append(...) and _ = table.Render() silently discard errors. Consider logging failures to stderr, which satisfies both the errcheck linter and the project guideline.
Proposed fix
- _ = table.Append([]string{
+ if err := table.Append([]string{
e.Timestamp.Format(time.RFC3339),
e.Level,
resource,
e.Message,
- })
+ }); err != nil {
+ fmt.Fprintf(os.Stderr, "Error appending log entry: %v\n", err)
+ }
}
- _ = table.Render()
+ if err := table.Render(); err != nil {
+ fmt.Fprintf(os.Stderr, "Error rendering table: %v\n", err)
+ }As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like _ = someFunc()".
📝 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.
| _ = table.Append([]string{ | |
| e.Timestamp.Format(time.RFC3339), | |
| e.Level, | |
| resource, | |
| e.Message, | |
| }) | |
| } | |
| table.Render() | |
| _ = table.Render() | |
| if err := table.Append([]string{ | |
| e.Timestamp.Format(time.RFC3339), | |
| e.Level, | |
| resource, | |
| e.Message, | |
| }); err != nil { | |
| fmt.Fprintf(os.Stderr, "Error appending log entry: %v\n", err) | |
| } | |
| } | |
| if err := table.Render(); err != nil { | |
| fmt.Fprintf(os.Stderr, "Error rendering table: %v\n", err) | |
| } |
🤖 Prompt for AI Agents
In `@cmd/cloud/logs.go` around lines 108 - 115, Replace the blank-identifier error
discards for table.Append and table.Render with proper error handling: capture
the returned errors from table.Append(...) and table.Render(), and if non-nil
write a clear error message to stderr (or the module's logger) including context
(e.g., the log entry timestamp/level/resource) so failures aren’t silent;
specifically update the calls to table.Append and table.Render to check their
error returns and log/report any error instead of using `_ =`.
| // Prevent decompression bomb: limit copy size to 10MB per file | ||
| // and check for excessive total size if needed. | ||
| const maxFileSize = 10 * 1024 * 1024 | ||
| _, err = io.CopyN(dst, src, maxFileSize) | ||
| if err != nil && err != io.EOF { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
io.CopyN silently truncates files larger than 10 MB.
io.CopyN(dst, src, maxFileSize) copies exactly maxFileSize bytes and returns nil error when there is more data remaining. Files larger than 10 MB will be silently truncated with no error. You need to detect the "file too large" case and return an error.
Proposed fix
const maxFileSize = 10 * 1024 * 1024
- _, err = io.CopyN(dst, src, maxFileSize)
- if err != nil && err != io.EOF {
- return err
+ written, err := io.CopyN(dst, src, maxFileSize+1)
+ if err != nil && err != io.EOF {
+ return err
+ }
+ if written > maxFileSize {
+ return fmt.Errorf("file exceeds maximum size of %d bytes: %s", maxFileSize, file.Name)
}
return nil🤖 Prompt for AI Agents
In `@internal/core/services/function.go` around lines 335 - 342, io.CopyN(dst,
src, maxFileSize) will silently truncate files larger than maxFileSize; change
the logic so after calling io.CopyN you detect the "too large" case: keep the
existing io.CopyN call with maxFileSize and then, if err == nil (meaning exactly
maxFileSize were read but more data may remain), attempt to read one byte from
src (e.g. buf := make([]byte,1); n, err2 := src.Read(buf)); if n>0 || (err2 !=
nil && err2 != io.EOF) return a specific "file too large" error; preserve the
existing error handling for err != nil && err != io.EOF and treat err == io.EOF
as success. Ensure you reference maxFileSize, io.CopyN, dst and src in the
change.
| // G304: Explicitly clean path before creation | ||
| f, err := os.Create(filepath.Clean(destPath)) |
There was a problem hiding this comment.
os.Create defaults to 0666 permissions — inconsistent with the 0600 hardening elsewhere.
Write (line 43) correctly uses os.OpenFile(..., 0600), but Assemble uses os.Create which creates files with mode 0666 (before umask). This undermines the permission tightening goal of this PR.
Proposed fix
- // G304: Explicitly clean path before creation
- f, err := os.Create(filepath.Clean(destPath))
+ // G304/G306: Explicitly clean path and set restrictive permissions
+ f, err := os.OpenFile(filepath.Clean(destPath), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600)📝 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.
| // G304: Explicitly clean path before creation | |
| f, err := os.Create(filepath.Clean(destPath)) | |
| // G304/G306: Explicitly clean path and set restrictive permissions | |
| f, err := os.OpenFile(filepath.Clean(destPath), os.O_WRONLY|os.O_CREATE|os.O_TRUNC, 0600) |
🤖 Prompt for AI Agents
In `@internal/repositories/filesystem/adapter.go` around lines 108 - 109, The
Assemble function currently uses os.Create(filepath.Clean(destPath)) which
yields files with default 0666 permissions; change this to use os.OpenFile with
flags os.O_CREATE|os.O_WRONLY|os.O_TRUNC and explicit mode 0600 (keeping
filepath.Clean(destPath)) so the created file honors the same 0600 hardening
used by Write; ensure the returned file handle is used/closed as before.
|
|
||
| binary.LittleEndian.PutUint64(buf, uNow) | ||
| _ = os.WriteFile(metaPath, buf, 0600) |
There was a problem hiding this comment.
Silent failure writing metadata for assembled object.
If os.WriteFile fails here, the assembled object will have no .meta file, causing reads to fall back to ModTime—which may be inaccurate in distributed scenarios. At minimum, return this error since the function already returns an error value. As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like _ = someFunc()".
Proposed fix
- _ = os.WriteFile(metaPath, buf, 0600)
-
- return totalSize, nil
+ if err := os.WriteFile(metaPath, buf, 0600); err != nil {
+ return 0, err
+ }
+ return totalSize, nil📝 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.
| binary.LittleEndian.PutUint64(buf, uNow) | |
| _ = os.WriteFile(metaPath, buf, 0600) | |
| binary.LittleEndian.PutUint64(buf, uNow) | |
| if err := os.WriteFile(metaPath, buf, 0600); err != nil { | |
| return 0, err | |
| } | |
| return totalSize, nil |
🤖 Prompt for AI Agents
In `@internal/storage/node/store.go` around lines 164 - 166, The write to the
metadata file currently ignores errors (_ = os.WriteFile(metaPath, buf, 0600)),
causing silent failures; update the code in store.go to capture the error from
os.WriteFile (e.g., err := os.WriteFile(metaPath, buf, 0600)) and if non-nil
return or propagate that error from the enclosing function (the function that
assembles the object which sets uNow, buf, and metaPath), ensuring callers get a
failure instead of proceeding without a .meta file.
| if termResp != nil { | ||
| _ = termResp.Body.Close() | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Use the existing closeResponse helper for consistency.
Both cleanup paths use _ = termResp.Body.Close(), but the file already defines closeResponse(t, resp) (Line 27) which properly logs close errors. Every other response close in this file uses that helper. As per coding guidelines, "Do not use silent failures - avoid blank identifier assignment like _ = someFunc()".
Proposed fix
At lines 153–155:
if termResp != nil {
- _ = termResp.Body.Close()
+ closeResponse(t, termResp)
}At lines 186–188:
if termResp != nil {
- _ = termResp.Body.Close()
+ closeResponse(t, termResp)
}Also applies to: 186-188
🤖 Prompt for AI Agents
In `@tests/elastic_ip_e2e_test.go` around lines 153 - 155, Replace the silent
response closes with the existing helper: instead of calling `_ =
termResp.Body.Close()` directly, call the test helper closeResponse(t, termResp)
so close errors are logged; ensure you still nil-check termResp before calling
closeResponse (i.e., if termResp != nil { closeResponse(t, termResp) }) and
apply this change to both cleanup paths that currently use `_ =
termResp.Body.Close()`.
There was a problem hiding this comment.
Pull request overview
This PR implements security hardening and resource leak fixes based on strict linting rules (gosec, bodyclose, errcheck). The changes span multiple areas: tightened file permissions, path sanitization, integer overflow protection, HTTP server security improvements, cryptographically secure random number generation, and proper resource cleanup in tests.
Changes:
- Enabled gosec and bodyclose linters with inline nolint directives for audited exceptions
- Hardened file/directory permissions from 0755/0644 to 0750/0600 across the codebase
- Added path sanitization with filepath.Clean and traversal checks
- Fixed integer overflow risks in type conversions (uint64→int64, uint32→int32)
- Added ReadHeaderTimeout to HTTP servers to prevent Slowloris attacks
- Replaced math/rand with crypto/rand for security-sensitive random selection
- Added zip bomb protections (file count limit, per-file size limit) to function extraction
- Ensured HTTP response bodies are properly closed in tests
Reviewed changes
Copilot reviewed 36 out of 36 changed files in this pull request and generated 12 comments.
Show a summary per file
| File | Description |
|---|---|
| .golangci.yml | Enabled gosec and bodyclose linters; configured exclusions |
| tests/multitenancy_e2e_test.go | Added response body closure in deleteInstance |
| tests/helpers/helpers_test.go | Added response body closure in test helpers |
| tests/elastic_ip_e2e_test.go | Inlined response body closure (inconsistent with file's pattern) |
| pkg/testutil/constants.go | Added nolint directive for test constant |
| pkg/sshutil/client.go | Added nolint directive with documentation for InsecureIgnoreHostKey |
| pkg/httputil/httputil_test.go | Added nolint directive for test constant |
| internal/storage/node/store.go | Tightened permissions, added path sanitization and overflow checks |
| internal/storage/node/gossip.go | Switched to crypto/rand for peer selection |
| internal/storage/coordinator/service.go | Switched to crypto/rand for node selection |
| internal/repositories/libvirt/real_client.go | Added overflow checks for uint32→int32 flag conversions |
| internal/repositories/libvirt/lb_proxy_test.go | Tightened permissions and added path sanitization |
| internal/repositories/libvirt/lb_proxy.go | Tightened file permissions to 0750/0600 |
| internal/repositories/libvirt/adapter.go | Tightened cloud-init file permissions |
| internal/repositories/k8s/node_executor.go | Added nolint directive with justification for InsecureIgnoreHostKey |
| internal/repositories/filesystem/adapter.go | Tightened permissions, added path sanitization (but checks are insufficient) |
| internal/repositories/docker/adapter_unit_test.go | Tightened directory permissions in test |
| internal/handlers/ws/ws_test.go | Added response body closure for websocket dial responses |
| internal/handlers/ws/client_test.go | Added response body closure for websocket dial responses |
| internal/core/services/function.go | Added zip bomb protections and path sanitization (but checks are insufficient) |
| internal/core/services/cache.go | Added overflow checks for memory stats uint64→int64 conversions |
| cmd/doccheck/main_test.go | Tightened file permissions in test |
| cmd/cloud/storage.go | Added filepath.Clean to file operations |
| cmd/cloud/ssh_key.go | Added filepath.Clean to ReadFile |
| cmd/cloud/secrets.go | Added nolint directive for format string constant |
| cmd/cloud/logs.go | Ignored Append/Render return values per .golangci.yml config |
| cmd/cloud/kubernetes_test.go | Added nolint directive for test constant |
| cmd/cloud/instance.go | Added nolint directive for syscall.Exec with user args |
| cmd/cloud/iac_cli_test.go | Tightened file permissions in test |
| cmd/cloud/iac.go | Added filepath.Clean to ReadFile calls |
| cmd/cloud/function_cli_test.go | Tightened file permissions in test |
| cmd/cloud/function.go | Added filepath.Clean to ReadFile calls |
| cmd/cloud/auth_test.go | Tightened permissions in test |
| cmd/cloud/auth.go | Tightened config permissions and added filepath.Clean |
| cmd/api/main_test.go | Added ReadHeaderTimeout to test HTTP server |
| cmd/api/main.go | Added ReadHeaderTimeout to production HTTP server |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if termResp != nil { | ||
| _ = termResp.Body.Close() | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling pattern: This file defines a closeResponse helper function at line 27 that already handles nil checks and is used throughout the file. These two call sites should use the existing closeResponse helper for consistency instead of inlining the nil check. Replace with closeResponse(t, termResp) to match the pattern used elsewhere in this file (lines 48, 73, 91, 113, 133, 165, 181, 194, 200).
| if termResp != nil { | ||
| _ = termResp.Body.Close() | ||
| } |
There was a problem hiding this comment.
Inconsistent error handling pattern: This file defines a closeResponse helper function at line 27 that already handles nil checks and is used throughout the file. This call site should use the existing closeResponse helper for consistency instead of inlining the nil check. Replace with closeResponse(t, termResp) to match the pattern used elsewhere in this file (lines 48, 73, 91, 113, 133, 165, 181, 194, 200).
| @@ -1,4 +1,5 @@ | |||
| run: | |||
| version: 1 | |||
There was a problem hiding this comment.
The "version" field under "run" section is not a valid golangci-lint configuration option. This field does not exist in the golangci-lint configuration schema and should be removed. The golangci-lint version is specified in the GitHub Actions workflow (line 36 of .github/workflows/ci.yml) or as a CLI argument, not in the configuration file.
| version: 1 |
| func (s *LocalFileStore) Assemble(ctx context.Context, bucket, key string, parts []string) (int64, error) { | ||
| bucketPath := filepath.Join(s.basePath, filepath.Clean(bucket)) | ||
| destPath := filepath.Join(bucketPath, filepath.Clean(key)) | ||
|
|
||
| if !strings.HasPrefix(destPath, filepath.Clean(s.basePath)) { | ||
| return 0, errors.New(errors.InvalidInput, errTraversal) | ||
| } | ||
|
|
||
| f, err := os.Create(destPath) | ||
| // G304: Explicitly clean path before creation | ||
| f, err := os.Create(filepath.Clean(destPath)) | ||
| if err != nil { | ||
| return 0, errors.Wrap(errors.Internal, "failed to create dest file", err) | ||
| } | ||
| defer func() { _ = f.Close() }() | ||
|
|
||
| var totalSize int64 | ||
| for _, partKey := range parts { | ||
| partPath := filepath.Join(bucketPath, filepath.Clean(partKey)) | ||
| if !strings.HasPrefix(partPath, filepath.Clean(s.basePath)) { | ||
| return 0, errors.New(errors.InvalidInput, errTraversal) | ||
| } |
There was a problem hiding this comment.
Path traversal check is insufficient in multiple locations: Lines 104-105 and 118-119 use HasPrefix checks which can be bypassed. All these checks should be replaced with filepath.Rel validation. See internal/storage/node/store.go:184-187 for the correct pattern.
| //nolint:gosec // G106: Default to insecure for legacy compatibility, but now configurable | ||
| HostKeyCallback: ssh.InsecureIgnoreHostKey(), |
There was a problem hiding this comment.
Misleading comment: The comment states "now configurable" but the HostKeyCallback field is set to ssh.InsecureIgnoreHostKey() without providing any way to configure it through the NewClientWithKey constructor. While the Client struct has a public HostKeyCallback field that could be modified after creation, the comment suggests this PR made it configurable, which is not the case. Either add a parameter to make it actually configurable, or update the comment to accurately reflect that it remains hardcoded but can be modified on the returned Client struct.
| //nolint:gosec // G305: Path sanitization and prefix check are performed below | ||
| path := filepath.Join(cleanTmpDir, file.Name) | ||
| if !strings.HasPrefix(filepath.Clean(path), cleanTmpDir+string(os.PathSeparator)) { |
There was a problem hiding this comment.
Path traversal check is insufficient: The check !strings.HasPrefix(filepath.Clean(path), cleanTmpDir+string(os.PathSeparator)) will fail to detect traversal for paths like tmpDir/.. because filepath.Clean(path) would result in the parent of tmpDir, which won't have tmpDir as a prefix. However, the more critical issue is that this check happens AFTER the filepath.Join on line 308. If file.Name contains path traversal sequences like ../../../etc/passwd, the join may escape the tmpDir. The correct approach is to check if filepath.Clean(file.Name) contains .. components BEFORE joining, and also verify the final path stays within tmpDir using filepath.Rel or filepath.EvalSymlinks.
| //nolint:gosec // G305: Path sanitization and prefix check are performed below | |
| path := filepath.Join(cleanTmpDir, file.Name) | |
| if !strings.HasPrefix(filepath.Clean(path), cleanTmpDir+string(os.PathSeparator)) { | |
| // Sanitize the file name before joining to prevent path traversal | |
| cleanName := filepath.Clean(file.Name) | |
| // Skip entries that resolve to the current directory | |
| if cleanName == "." { | |
| return nil | |
| } | |
| // Reject absolute paths | |
| if filepath.IsAbs(cleanName) { | |
| return fmt.Errorf("invalid absolute file path in zip: %s", file.Name) | |
| } | |
| // Reject any path containing ".." components | |
| for _, part := range strings.Split(cleanName, string(os.PathSeparator)) { | |
| if part == ".." { | |
| return fmt.Errorf("invalid file path in zip (path traversal): %s", file.Name) | |
| } | |
| } | |
| // Construct the target path within the cleaned tmpDir | |
| path := filepath.Join(cleanTmpDir, cleanName) | |
| // Ensure the resulting path is still within tmpDir | |
| rel, err := filepath.Rel(cleanTmpDir, path) | |
| if err != nil || rel == ".." || strings.HasPrefix(rel, ".."+string(os.PathSeparator)) { |
| func (s *LocalFileStore) Write(ctx context.Context, bucket, key string, r io.Reader) (int64, error) { | ||
| bucketPath := filepath.Join(s.basePath, filepath.Clean(bucket)) | ||
| filePath := filepath.Join(bucketPath, filepath.Clean(key)) | ||
|
|
||
| if !strings.HasPrefix(filePath, filepath.Clean(s.basePath)) { | ||
| return 0, errors.New(errors.InvalidInput, errTraversal) | ||
| } |
There was a problem hiding this comment.
Path traversal check is insufficient: The check on line 35 uses !strings.HasPrefix(filePath, filepath.Clean(s.basePath)) which can be bypassed. After filepath.Join(bucketPath, filepath.Clean(key)) on line 33, if key contains traversal sequences, the result may escape basePath. A more robust approach is to use filepath.Rel (like internal/storage/node/store.go:184-187 does) to verify the path stays within basePath: rel, err := filepath.Rel(filepath.Clean(s.basePath), filePath); if err != nil || strings.HasPrefix(rel, "..") { return error }. The HasPrefix check alone is insufficient because filepath.Clean may resolve symlinks or handle edge cases differently.
| func (s *LocalFileStore) Read(ctx context.Context, bucket, key string) (io.ReadCloser, error) { | ||
| filePath := filepath.Join(s.basePath, filepath.Clean(bucket), filepath.Clean(key)) | ||
| if !strings.HasPrefix(filePath, filepath.Clean(s.basePath)) { | ||
| return nil, errors.New(errors.InvalidInput, errTraversal) | ||
| } |
There was a problem hiding this comment.
Path traversal check is insufficient: Same issue as in Write method - the HasPrefix check on line 59 can be bypassed. Use filepath.Rel to verify the path stays within basePath instead of string prefix checking. See internal/storage/node/store.go:184-187 for the correct pattern.
| now := time.Now().UnixNano() | ||
| var uNow uint64 | ||
| if now < 0 { | ||
| uNow = 0 | ||
| } else { | ||
| uNow = uint64(now) | ||
| } | ||
|
|
||
| binary.LittleEndian.PutUint64(buf, uNow) |
There was a problem hiding this comment.
Unnecessary defensive check: time.Now().UnixNano() cannot be negative in practice unless the system clock is set to before January 1, 1970 (Unix epoch), which would indicate a serious system misconfiguration. While technically possible, this check adds complexity for an extremely unlikely edge case. If you want to keep the check for robustness, consider adding a comment explaining why it's needed, or simply use uint64(now) directly since Go's conversion truncates the value predictably.
| now := time.Now().UnixNano() | |
| var uNow uint64 | |
| if now < 0 { | |
| uNow = 0 | |
| } else { | |
| uNow = uint64(now) | |
| } | |
| binary.LittleEndian.PutUint64(buf, uNow) | |
| now := uint64(time.Now().UnixNano()) | |
| binary.LittleEndian.PutUint64(buf, now) |
| var uTimestamp uint64 | ||
| if timestamp < 0 { | ||
| uTimestamp = 0 | ||
| } else { | ||
| uTimestamp = uint64(timestamp) | ||
| } |
There was a problem hiding this comment.
Inconsistent handling: This negative timestamp check might make sense if timestamps come from external sources (like they do in the Write method parameter), but it's inconsistent with the approach in Assemble where time.Now().UnixNano() is checked. Consider documenting why negative timestamps need to be handled, or remove the check in Assemble (lines 157-163) if external negative timestamps are the only concern.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 36 out of 36 changed files in this pull request and generated 3 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| gosec: | ||
| excludes: | ||
| - G101 | ||
| - G106 |
There was a problem hiding this comment.
gosec is enabled, but G101 and G106 are globally excluded here, which means newly introduced hardcoded credentials or ssh.InsecureIgnoreHostKey() usages won’t be flagged anywhere. Since the PR already adds targeted //nolint:gosec annotations, consider removing these global excludes (or scoping them to specific paths) so future regressions still get caught by CI.
| gosec: | |
| excludes: | |
| - G101 | |
| - G106 |
| if !strings.HasPrefix(filePath, filepath.Clean(s.basePath)) { | ||
| return 0, errors.New(errors.InvalidInput, errTraversal) | ||
| } | ||
|
|
||
| if err := os.MkdirAll(filepath.Dir(filePath), 0755); err != nil { | ||
| if err := os.MkdirAll(filepath.Dir(filePath), 0750); err != nil { |
There was a problem hiding this comment.
The traversal guard strings.HasPrefix(filePath, filepath.Clean(s.basePath)) is vulnerable to prefix-bypass (e.g., base /tmp/base, joined path /tmp/base2/... still matches). This can allow escaping the base directory with inputs like bucket ../base2. Use filepath.Rel from the base to the computed path and reject rel == ".." or any rel starting with ".."+PathSeparator, or ensure the prefix check includes a path separator boundary.
| resp, err := client.Do(req) | ||
| if err == nil { | ||
| defer func() { _ = resp.Body.Close() }() | ||
| } |
There was a problem hiding this comment.
http.Client.Do can return a non-nil resp even when err is non-nil; in that case the response body still needs to be closed. The current if err == nil { ... Close() } can leak. Close the body whenever resp != nil (and consider asserting on err/status if this request is expected to succeed).
There was a problem hiding this comment.
🤖 Fix all issues with AI agents
Verify each finding against the current code and only fix it if needed.
In `@internal/repositories/libvirt/adapter.go`:
- Around line 284-293: The 5-minute magic duration in waitInitialIP should be
extracted to a named constant (e.g., waitIPHardTimeout) and the time.After call
replaced with a time.NewTimer so the timer can be stopped/GC'd if the function
returns early; add const waitIPHardTimeout = 5 * time.Minute at top of the file,
create timer := time.NewTimer(waitIPHardTimeout) before the loop, defer
timer.Stop() (or stop it when returning), and change the select to read from
timer.C instead of time.After; update any error text to reference the constant
usage (the timeout remains for instance id in waitInitialIP).
In `@tests/database_replication_e2e_test.go`:
- Around line 110-117: Replace the blank-identifier close calls for the HTTP
responses with explicit error handling: instead of `_ = resp1.Body.Close()` and
`_ = resp2.Body.Close()`, call `err := resp1.Body.Close()` / `err :=
resp2.Body.Close()` and if `err != nil` use the test logger (e.g.,
`t.Logf("closing response body: %v", err)`) to record the error; apply the same
change for the other response closes in this test file (variables resp1, resp2
and any other resp* returned by deleteRequest) to satisfy the
no-blank-identifier guideline and errcheck.
🧹 Nitpick comments (2)
🤖 Fix all nitpicks with AI agents
Verify each finding against the current code and only fix it if needed. In `@internal/repositories/libvirt/adapter.go`: - Around line 284-293: The 5-minute magic duration in waitInitialIP should be extracted to a named constant (e.g., waitIPHardTimeout) and the time.After call replaced with a time.NewTimer so the timer can be stopped/GC'd if the function returns early; add const waitIPHardTimeout = 5 * time.Minute at top of the file, create timer := time.NewTimer(waitIPHardTimeout) before the loop, defer timer.Stop() (or stop it when returning), and change the select to read from timer.C instead of time.After; update any error text to reference the constant usage (the timeout remains for instance id in waitInitialIP). In `@tests/database_replication_e2e_test.go`: - Around line 110-117: Replace the blank-identifier close calls for the HTTP responses with explicit error handling: instead of `_ = resp1.Body.Close()` and `_ = resp2.Body.Close()`, call `err := resp1.Body.Close()` / `err := resp2.Body.Close()` and if `err != nil` use the test logger (e.g., `t.Logf("closing response body: %v", err)`) to record the error; apply the same change for the other response closes in this test file (variables resp1, resp2 and any other resp* returned by deleteRequest) to satisfy the no-blank-identifier guideline and errcheck.tests/database_replication_e2e_test.go (1)
110-117: Good fix: nil-guarded response body closure prevents resource leaks.The nil check before deferring
Body.Close()is the right pattern here, sincedeleteRequestmay return a nil response on failure.One note: the
_ = resp.Body.Close()pattern on lines 112 and 116 conflicts with the repository's coding guideline against blank identifier assignments. If you want to stay compliant while satisfyingerrcheck, you could log the error in tests instead:♻️ Optional: log close errors instead of discarding
resp1 := deleteRequest(t, client, fmt.Sprintf("%s/databases/%s", testutil.TestBaseURL, primaryID), token) if resp1 != nil { - defer func() { _ = resp1.Body.Close() }() + defer func() { + if err := resp1.Body.Close(); err != nil { + t.Logf("failed to close resp1 body: %v", err) + } + }() } resp2 := deleteRequest(t, client, fmt.Sprintf("%s/databases/%s", testutil.TestBaseURL, replicaID), token) if resp2 != nil { - defer func() { _ = resp2.Body.Close() }() + defer func() { + if err := resp2.Body.Close(); err != nil { + t.Logf("failed to close resp2 body: %v", err) + } + }() }This same pattern is used in unchanged lines (42, 61, 78, 92, 98) throughout the file — worth addressing holistically if you agree.
As per coding guidelines,
**/*.go: "Do not use silent failures - avoid blank identifier assignment like_ = someFunc()".🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/database_replication_e2e_test.go` around lines 110 - 117, Replace the blank-identifier close calls for the HTTP responses with explicit error handling: instead of `_ = resp1.Body.Close()` and `_ = resp2.Body.Close()`, call `err := resp1.Body.Close()` / `err := resp2.Body.Close()` and if `err != nil` use the test logger (e.g., `t.Logf("closing response body: %v", err)`) to record the error; apply the same change for the other response closes in this test file (variables resp1, resp2 and any other resp* returned by deleteRequest) to satisfy the no-blank-identifier guideline and errcheck.internal/repositories/libvirt/adapter.go (1)
284-293: Extract the 5-minute timeout as a named constant.The
5 * time.Minuteduration is a magic number. Per coding guidelines, use a named constant (e.g.,waitIPTimeout). Also, note thattime.Afterallocates a timer that cannot be stopped/GC'd until it fires — ifwaitInitialIPreturns early, the timer lingers for the remaining duration. Considertime.NewTimerwith an explicitStop()in a defer for cleaner resource management.Proposed fix
Add a constant near the top of the file:
const waitIPHardTimeout = 5 * time.MinuteThen in
waitInitialIP:- // Safety limit: max 5 minutes regardless of context - timeout := time.After(5 * time.Minute) + // Safety limit: hard cap regardless of context + timer := time.NewTimer(waitIPHardTimeout) + defer timer.Stop()And update the select case:
- case <-timeout: + case <-timer.C:As per coding guidelines, "Do not use magic numbers - use named constants".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/repositories/libvirt/adapter.go` around lines 284 - 293, The 5-minute magic duration in waitInitialIP should be extracted to a named constant (e.g., waitIPHardTimeout) and the time.After call replaced with a time.NewTimer so the timer can be stopped/GC'd if the function returns early; add const waitIPHardTimeout = 5 * time.Minute at top of the file, create timer := time.NewTimer(waitIPHardTimeout) before the loop, defer timer.Stop() (or stop it when returning), and change the select to read from timer.C instead of time.After; update any error text to reference the constant usage (the timeout remains for instance id in waitInitialIP).
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 39 out of 39 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if !strings.HasPrefix(filePath, filepath.Clean(s.basePath)) { | ||
| return nil, errors.New(errors.InvalidInput, errTraversal) | ||
| } | ||
| f, err := os.Open(filePath) | ||
| // filepath.Clean is used above, but calling it again inside Open to satisfy gosec G304 explicitly | ||
| f, err := os.Open(filepath.Clean(filePath)) |
There was a problem hiding this comment.
The traversal check strings.HasPrefix(filePath, filepath.Clean(s.basePath)) is bypassable due to common-prefix matches (e.g., "/tmp/base2" has prefix "/tmp/base"). Use a filepath.Rel-based containment check (with a path-separator-aware ".." prefix test) to reliably enforce that reads stay under basePath.
| if !strings.HasPrefix(destPath, filepath.Clean(s.basePath)) { | ||
| return 0, errors.New(errors.InvalidInput, errTraversal) | ||
| } | ||
|
|
||
| f, err := os.Create(destPath) | ||
| // G304: Explicitly clean path before creation | ||
| f, err := os.Create(filepath.Clean(destPath)) |
There was a problem hiding this comment.
The traversal check strings.HasPrefix(destPath, filepath.Clean(s.basePath)) is bypassable via prefix tricks (e.g., escaping to a sibling directory whose name shares the base prefix). Use filepath.Rel containment validation (as used elsewhere in the repo) to ensure destPath is truly inside basePath before creating/writing the assembled object.
| if !strings.HasPrefix(partPath, filepath.Clean(s.basePath)) { | ||
| return 0, errors.New(errors.InvalidInput, errTraversal) | ||
| } | ||
| pf, err := os.Open(partPath) | ||
| // G304: Explicitly clean path before opening | ||
| pf, err := os.Open(filepath.Clean(partPath)) |
There was a problem hiding this comment.
The part path traversal check uses strings.HasPrefix(partPath, filepath.Clean(s.basePath)), which can be bypassed via common-prefix paths (e.g., base "/tmp/base" vs "/tmp/base2"). Switch to a filepath.Rel-based containment check for partPath before opening/removing parts.
| zr, err := zip.NewReader(bytes.NewReader(buf.Bytes()), int64(buf.Len())) | ||
| if err != nil { | ||
| return err | ||
| } | ||
|
|
||
| // Limit total number of files to prevent resource exhaustion | ||
| if len(zr.File) > 1000 { | ||
| return fmt.Errorf("too many files in zip: %d", len(zr.File)) | ||
| } |
There was a problem hiding this comment.
extractZip reads the entire zip from an io.LimitReader into buf and then calls zip.NewReader. If the incoming zip exceeds maxZipSize, the data will be silently truncated by the limit and may produce confusing parse/extract errors. Consider explicitly detecting truncation (e.g., via an io.LimitedReader and checking N == 0 after copy, or probing for one extra byte) and returning a clear "zip too large" error.
|
|
||
| // Safety limit: max 5 minutes regardless of context | ||
| timeout := time.After(5 * time.Minute) | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return "", ctx.Err() | ||
| case <-timeout: |
There was a problem hiding this comment.
Using time.After(5 * time.Minute) inside waitInitialIP can leak the underlying timer if the function returns early (common when IP is obtained quickly), because the timer can't be stopped/drained. Prefer time.NewTimer with defer timer.Stop() (and drain if needed) to avoid accumulating timers over time.
| // Safety limit: max 5 minutes regardless of context | |
| timeout := time.After(5 * time.Minute) | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return "", ctx.Err() | |
| case <-timeout: | |
| // Safety limit: max 5 minutes regardless of context | |
| timer := time.NewTimer(5 * time.Minute) | |
| defer timer.Stop() | |
| for { | |
| select { | |
| case <-ctx.Done(): | |
| return "", ctx.Err() | |
| case <-timer.C: |
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 84 out of 84 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // Prevent decompression bomb: limit copy size to 10MB per file | ||
| // and check for excessive total size if needed. | ||
| const maxFileSize = 10 * 1024 * 1024 | ||
| _, err = io.CopyN(dst, src, maxFileSize) | ||
| if err != nil && err != io.EOF { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The use of io.CopyN with maxFileSize will always attempt to read exactly maxFileSize bytes. If the file is smaller than 10MB, io.CopyN returns io.EOF error which is then ignored. However, if the file is exactly or larger than 10MB, data beyond 10MB will be silently truncated without any error being raised. This could lead to corrupted function deployments where files larger than 10MB are silently truncated.
Consider using io.LimitReader instead to properly handle files of any size up to the limit, or return an error if a file exceeds the limit.
| if flags > math.MaxInt32 { | ||
| return libvirt.StorageVol{}, fmt.Errorf("flags overflow int32: %d", flags) | ||
| } | ||
| return r.conn.StorageVolCreateXML(pool, xml, libvirt.StorageVolCreateFlags(int32(flags))) |
There was a problem hiding this comment.
The overflow check only validates that flags doesn't exceed math.MaxInt32, but doesn't handle the case where flags is a valid uint32 value between math.MaxInt32+1 and math.MaxUint32. When cast to int32, these values will wrap around to negative numbers due to two's complement representation, potentially causing unexpected behavior in the libvirt library.
Consider either: 1) Documenting that only flags in the range 0 to MaxInt32 are supported, or 2) Using a bitmask approach to preserve the bit pattern if negative int32 values are valid flag representations.
| // Safety limit: max 5 minutes regardless of context | ||
| timeout := time.After(5 * time.Minute) | ||
|
|
||
| for { | ||
| select { | ||
| case <-ctx.Done(): | ||
| return "", ctx.Err() | ||
| case <-timeout: | ||
| return "", fmt.Errorf("timed out waiting for IP for instance %s", id) |
There was a problem hiding this comment.
The timeout added here creates a hard 5-minute limit that runs in parallel with the context's deadline. If the context has a shorter deadline, it will cancel first (correct behavior). However, if the context has a longer deadline or no deadline, this timeout will trigger and return a generic error message instead of respecting the caller's context.
Consider using context.WithTimeout to create a child context with the 5-minute limit, which would properly integrate with the existing context cancellation mechanism and return context.DeadlineExceeded consistently.
| timeout 60s bash -c 'until pg_isready -h localhost -p 5433 -U cloud; do | ||
| echo "Waiting for postgres..." | ||
| sleep 2 | ||
| done | ||
| done' |
There was a problem hiding this comment.
The timeout 60s bash -c '...' command structure will timeout the bash subprocess, but won't timeout the pg_isready loop inside if it hangs. The timeout command will send SIGTERM to bash, but the loop might not respond to it properly.
A more robust approach would be to use timeout 60s sh -c 'until ...' without wrapping in bash, or add explicit signal handling. However, this is likely fine in practice since pg_isready itself should timeout quickly.
| go test -p 1 -v -run TestInstanceService ./internal/core/services/... | ||
|
|
||
| # Test with Libvirt backend | ||
| export COMPUTE_BACKEND=libvirt | ||
| go test -v -run TestInstanceService ./internal/core/services/... | ||
| go test -p 1 -v -run TestInstanceService ./internal/core/services/... |
There was a problem hiding this comment.
The -p 1 flag limits test package parallelism to 1, which significantly increases test execution time. This change should be documented in the PR description as it's a notable performance impact.
Consider if this is truly necessary or if there are specific tests that have race conditions that should be fixed instead of globally disabling parallelism.
There was a problem hiding this comment.
Actionable comments posted: 2
Note
Due to the large number of review comments, Critical severity comments were prioritized as inline comments.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (2)
internal/core/services/auth_unit_test.go (2)
28-65:⚠️ Potential issue | 🟡 MinorMissing
AssertExpectationscalls leave mock expectations unverified.
Register_Success(lines 28–44):mockAuditSvc.On("Log", ...)is set up with.Once()butmockAuditSvc.AssertExpectations(t)is never called. OnlymockUserRepoandmockTenantSvcare verified.Login_Success(lines 46–65):mockUserRepo,mockIdentitySvc, andmockAuditSvcall have.Once()expectations but noAssertExpectationscall exists at all.
AssertExpectationsForObjects(andAssertExpectations) asserts that everything specified withOnandReturnwas in fact called as expected — calls may have occurred in any order. Without these calls, the tests will pass even if the audit log or identity key creation paths are never exercised.♻️ Proposed fix
t.Run("Register_Success", func(t *testing.T) { ... mockUserRepo.AssertExpectations(t) mockTenantSvc.AssertExpectations(t) + mockAuditSvc.AssertExpectations(t) }) t.Run("Login_Success", func(t *testing.T) { ... assert.Equal(t, "test-key", token) + mockUserRepo.AssertExpectations(t) + mockIdentitySvc.AssertExpectations(t) + mockAuditSvc.AssertExpectations(t) })🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/auth_unit_test.go` around lines 28 - 65, The tests are missing mock expectation verifications; after the Register_Success subtest call svc.Register, add mockAuditSvc.AssertExpectations(t) (in addition to existing mockUserRepo.AssertExpectations(t) and mockTenantSvc.AssertExpectations(t)) to ensure the audit Log was invoked, and after the Login_Success subtest (after asserting token) call mockUserRepo.AssertExpectations(t), mockIdentitySvc.AssertExpectations(t), and mockAuditSvc.AssertExpectations(t) to verify all .On(...).Once() expectations for GetByEmail, CreateKey, and Log were met.
15-75:⚠️ Potential issue | 🟠 MajorSecurity test regressions:
WeakPasswordandDuplicateEmailsubtests were removed.The AI summary confirms these two subtests were deleted. Both represent critical validation paths for an auth service:
WeakPassword— ensures the service rejects passwords that don't meet strength requirements.DuplicateEmail— ensures the service rejects registration for an already-registered email.Removing them leaves these code paths untested. Given the PR's stated security-hardening goals, this is a step backwards.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@internal/core/services/auth_unit_test.go` around lines 15 - 75, Re-add two subtests to TestAuthService_Unit_Extended: "WeakPassword" should call svc.Register with a deliberately weak password, assert it returns a validation/error and that mockUserRepo.On("Create", ...) was not invoked (use mockUserRepo.AssertNotCalled), and "DuplicateEmail" should have mockUserRepo.On("GetByEmail", mock.Anything, email).Return(&domain.User{ID: ...}, nil) and then call svc.Register expecting an error and verifying mockUserRepo.Create was not called (and mockAuditSvc.Log not called) to ensure duplicate-email registration is rejected; use the existing svc, mockUserRepo, and mockAuditSvc symbols to add these expectations and assertions.
|
|
||
| // Create a valid 32-byte hex master key | ||
| masterKey := make([]byte, 32) | ||
| rand.Read(masterKey) |
There was a problem hiding this comment.
rand.Read error return is unchecked — CI lint blocker.
This is the confirmed errcheck failure causing CI to fail. While crypto/rand.Read calls io.ReadFull on Reader and crashes the program irrecoverably if an error is returned, with the default Reader using OS APIs documented to never return an error on modern systems, the function signature still returns (int, error) and errcheck flags the unhandled return.
🐛 Proposed fix
- rand.Read(masterKey)
+ _, err = rand.Read(masterKey)
+ require.NoError(t, err)This requires changing the err declaration on line 39 from := to a pre-declared var err error before the rand.Read call, or restructuring slightly:
masterKey := make([]byte, 32)
- rand.Read(masterKey)
+ _, err := rand.Read(masterKey)
+ require.NoError(t, err)
masterKeyHex := hex.EncodeToString(masterKey)
- svc, err := services.NewEncryptionService(mockRepo, masterKeyHex)
+ svc, err := services.NewEncryptionService(mockRepo, masterKeyHex) // err is already declared above; use = hereOr more simply keep separate declarations:
masterKey := make([]byte, 32)
- rand.Read(masterKey)
+ _, randErr := rand.Read(masterKey)
+ require.NoError(t, randErr)
masterKeyHex := hex.EncodeToString(masterKey)📝 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.
| rand.Read(masterKey) | |
| _, randErr := rand.Read(masterKey) | |
| require.NoError(t, randErr) |
🧰 Tools
🪛 GitHub Check: lint
[failure] 36-36:
Error return value of rand.Read is not checked (errcheck)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/core/services/encryption_svc_unit_test.go` at line 36, The call to
rand.Read(masterKey) currently ignores its (int, error) return and fails
errcheck; update the test to capture and handle the error from rand.Read — e.g.,
assign the return to _, err (or predeclare var err error and then call
rand.Read(masterKey)) and then check if err != nil and fail the test with
t.Fatalf or t.Fatal including the error; change the existing := usage so the err
variable is properly set and checked around the rand.Read(masterKey) call.
| var ( | ||
| execCommand = exec.Command | ||
| execCommandContext = exec.CommandContext | ||
| lookPath = exec.LookPath | ||
| osOpen = os.Open | ||
| ) |
There was a problem hiding this comment.
Remove the dead package-level var block — it breaks CI and violates the no-globals guideline.
The static-analysis report confirms all four variables (execCommand, execCommandContext, lookPath, osOpen) are flagged as unused by the linter and are causing CI lint failures. The identical symbols are already declared as struct fields on LibvirtAdapter (lines 71–74) and wired up in NewLibvirtAdapter (lines 131–134), so the package-level declarations are pure dead code. Beyond the lint failures, keeping package-level mutable function vars is exactly the anti-pattern the guideline prohibits.
Additionally, the execCommandContext struct field itself is initialised in the constructor but never called in any adapter method — it can also be removed to keep the surface minimal.
🗑️ Proposed fix
-var (
- execCommand = exec.Command
- execCommandContext = exec.CommandContext
- lookPath = exec.LookPath
- osOpen = os.Open
-)
-
// LibvirtAdapter implements compute backend operations using libvirt/KVM.And in the struct definition / constructor, drop execCommandContext if it has no callers:
// OS dependencies for testability
execCommand func(name string, arg ...string) *exec.Cmd
- execCommandContext func(ctx context.Context, name string, arg ...string) *exec.Cmd
lookPath func(file string) (string, error)
osOpen func(name string) (*os.File, error) execCommand: exec.Command,
- execCommandContext: exec.CommandContext,
lookPath: exec.LookPath,
osOpen: os.Open,As per coding guidelines, **/*.go: "Do not use global variables (e.g., var DB *sql.DB)".
📝 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.
| var ( | |
| execCommand = exec.Command | |
| execCommandContext = exec.CommandContext | |
| lookPath = exec.LookPath | |
| osOpen = os.Open | |
| ) |
🧰 Tools
🪛 GitHub Check: lint
[failure] 50-50:
var osOpen is unused (unused)
[failure] 49-49:
var lookPath is unused (unused)
[failure] 48-48:
var execCommandContext is unused (unused)
[failure] 47-47:
var execCommand is unused (unused)
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@internal/repositories/libvirt/adapter.go` around lines 46 - 51, Remove the
dead package-level var block (execCommand, execCommandContext, lookPath, osOpen)
and delete the unused execCommandContext field from the LibvirtAdapter struct
and its initialization in NewLibvirtAdapter; scan for any remaining references
to execCommandContext and replace them to use the struct's execCommand,
lookPath, or osOpen fields as appropriate (or remove those calls if unused) so
the package has no global mutable function vars and no unused struct field.
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 84 out of 84 changed files in this pull request and generated 5 comments.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| if flags > math.MaxInt32 { | ||
| return fmt.Errorf("flags overflow int32: %d", flags) | ||
| } | ||
| return r.conn.StorageVolDelete(vol, libvirt.StorageVolDeleteFlags(int32(flags))) |
There was a problem hiding this comment.
Same overflow check issue as in StorageVolCreateXML above - flags > math.MaxInt32 doesn't properly validate uint32 to int32 conversion. Use flags > 0x7FFFFFFF instead.
| // Prevent decompression bomb: limit copy size to 10MB per file | ||
| // and check for excessive total size if needed. | ||
| const maxFileSize = 10 * 1024 * 1024 | ||
| _, err = io.CopyN(dst, src, maxFileSize) | ||
| if err != nil && err != io.EOF { | ||
| return err | ||
| } | ||
| return nil |
There was a problem hiding this comment.
The use of io.CopyN with maxFileSize as the limit will truncate files larger than 10MB without error. If a file in the zip is exactly 10MB or larger, it will be silently truncated. Consider checking if the full file was copied by comparing the number of bytes written, or reading one extra byte to detect if the file exceeds the limit and returning an explicit error in that case.
| if !strings.HasPrefix(filepath.Clean(path), cleanTmpDir+string(os.PathSeparator)) { | ||
| return fmt.Errorf("invalid file path in zip: %s", file.Name) |
There was a problem hiding this comment.
The path traversal check has a potential issue. The check !strings.HasPrefix(filepath.Clean(path), cleanTmpDir+string(os.PathSeparator)) would allow a file named exactly as cleanTmpDir (without separator). For example, if cleanTmpDir is "/tmp/fn-123", a zip entry named "fn-123" would pass the check. Consider using filepath.Rel and checking for ".." prefix instead, consistent with the pattern seen elsewhere in the codebase (see internal/storage/node/store.go:184-186).
| timeout 60s bash -c 'until pg_isready -h localhost -p 5433 -U cloud; do | ||
| echo "Waiting for postgres..." | ||
| sleep 2 | ||
| done | ||
| done' |
There was a problem hiding this comment.
The timeout command usage here wraps the entire bash command, which is good. However, if pg_isready hangs indefinitely, the timeout will kill the bash process but may not necessarily kill the pg_isready subprocess. Consider adding set -e inside the bash command or using timeout directly on pg_isready in a loop for more robust timeout handling.
| if flags > math.MaxInt32 { | ||
| return libvirt.StorageVol{}, fmt.Errorf("flags overflow int32: %d", flags) | ||
| } | ||
| return r.conn.StorageVolCreateXML(pool, xml, libvirt.StorageVolCreateFlags(int32(flags))) |
There was a problem hiding this comment.
The overflow check flags > math.MaxInt32 is incorrect for unsigned to signed conversion. A uint32 value of 2^31 (0x80000000) would pass this check but when cast to int32 would become -2147483648 (negative), potentially causing unexpected behavior. The correct check should be flags > 0x7FFFFFFF or use a different pattern to ensure the value fits properly in the signed int32 range.
This PR addresses multiple security vulnerabilities and resource leaks identified by strict linting rules.
Changes:
Verification:
Summary by CodeRabbit
Bug Fixes
Security & Permissions
Stability Improvements
Tests
Code Quality