fix(lint): resolve single-occurrence gosec findings (G107, G110, G122, G302, G305) - #879
Conversation
…, G302, G305) Each of these five rules had exactly one finding repo-wide: - pkg/workspace/image.go (G107): getProjectImage fetched link before checking whether its host is one this code knows how to parse -- link is user-controlled (workspace source argument), so this was an open SSRF. Moved the host check before http.Get. - pkg/daemon/platform/socket.go (G302): the daemon's unix socket was chmod 0666, letting any local user connect. Tightened to 0600 -- no code anywhere relies on cross-user access to this socket. - pkg/extract/zip.go (G305): the existing Zip Slip guard already implements exactly what this rule checks for; gosec can't verify it statically. Documented false positive. - pkg/extract/zip.go (G110): unzipFile's io.Copy had no bound, so a malicious archive (e.g. a compromised provider binary download) could decompress to exhaust disk space. Bounded the copy at 2 GiB per entry via io.CopyN, independent of what the entry's header claims. - e2e/framework/util.go (G122): copyDir's filepath.Walk source is always a test-fixture path supplied by test authors, not attacker-controlled input. Documented false positive. Added characterization/regression tests for the two behavior changes (image.go's host check, socket.go's permission, zip.go's size bound); each was verified to fail against the pre-fix code.
✅ Deploy Preview for devsydev canceled.
|
✅ Deploy Preview for images-devsy-sh canceled.
|
📝 WalkthroughWalkthroughThe changes harden Unix socket permissions, bound ZIP entry extraction, reject unknown image hosts before HTTP requests, and annotate a trusted test-fixture filesystem walk. ChangesUnix socket permissions
Bounded ZIP extraction
Image host validation
Test-fixture walk annotation
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
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 |
Signed-off-by: Samuel K <skevetter@pm.me>
14e298b to
a006779
Compare
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@pkg/daemon/platform/socket.go`:
- Line 40: Update the listener setup around os.Chmod to handle a
permission-change failure: close the live Unix listener and return the os.Chmod
error instead of continuing with the listener. Preserve the existing successful
path when the chmod operation succeeds.
In `@pkg/extract/zip_test.go`:
- Around line 82-101: Strengthen TestUnzipFolder_RejectsOversizedEntry by
asserting that the rejected big.bin path does not exist in dest after
UnzipFolder fails, and add a direct copyBoundedZipEntry test using bytes.Buffer
that verifies writing N+1 bytes is rejected. Keep the existing error assertions
and tiny maxUnzipEntrySize setup.
🪄 Autofix
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro Plus
Run ID: a1771393-9afe-4b0e-b261-5651a551d698
📒 Files selected for processing (7)
e2e/framework/util.gopkg/daemon/platform/socket.gopkg/daemon/platform/socket_test.gopkg/extract/zip.gopkg/extract/zip_test.gopkg/workspace/image.gopkg/workspace/image_test.go
| return nil, err | ||
| } | ||
| _ = os.Chmod(addr, 0o666) | ||
| _ = os.Chmod(addr, 0o600) |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== files =="
git ls-files | rg '(^|/)socket\.go$|pkg/daemon|config|test' || true
echo "== socket.go outline =="
ast-grep outline pkg/daemon/platform/socket.go || true
echo "== socket.go relevant lines =="
cat -n pkg/daemon/platform/socket.go | sed -n '1,120p'
echo "== listen usages =="
rg -n "Listen\\(|socket\\.Listen|platform" pkg -S || true
echo "== tests mentioning socket/listen =="
git ls-files | rg -i '(_test|test)\.go$' | xargs rg -n "socket|Listen|mode|Chmod|0o600|0600" -S || trueRepository: devsy-org/devsy
Length of output: 50372
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== pkg/daemon/platform/socket_test.go =="
cat -n pkg/daemon/platform/socket_test.go | sed -n '1,220p'
echo "== platform socket.go files =="
for f in pkg/daemon/platform/socket.go pkg/daemon/platform/socket_test.go pkg/daemon/platform/socket_unix.go pkg/daemon/platform/socket_windows.go; do
if [ -f "$f" ]; then
echo "--- $f ---"
wc -l "$f"
fi
done
echo "== listen function references in daemon package =="
rg -n 'func listen|Listen\("unix"|net\.Listen|os\.Chmod|Dial|GetSocketAddr' pkg/daemon platform -S || trueRepository: devsy-org/devsy
Length of output: 4097
Permission Check (CWE-732): Incorrect Permission Assignment for Critical Resource
Reachability: Internal · Exploitability: Moderate
Fail closed when os.Chmod fails.
listen still returns the live Unix listener if os.Chmod fails, leaving the socket accessible with broader permissions. Close the listener and return the permission-change error instead.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/daemon/platform/socket.go` at line 40, Update the listener setup around
os.Chmod to handle a permission-change failure: close the live Unix listener and
return the os.Chmod error instead of continuing with the listener. Preserve the
existing successful path when the chmod operation succeeds.
| func TestUnzipFolder_RejectsOversizedEntry(t *testing.T) { | ||
| archivePath := buildZip(t, map[string]string{ | ||
| "big.bin": strings.Repeat("x", 10), | ||
| }) | ||
| dest := t.TempDir() | ||
|
|
||
| // Exercise the bound with a tiny limit so the test doesn't need to | ||
| // build a real multi-GB archive. | ||
| origLimit := maxUnzipEntrySize | ||
| maxUnzipEntrySize = 5 | ||
| defer func() { maxUnzipEntrySize = origLimit }() | ||
|
|
||
| err := UnzipFolder(archivePath, dest) | ||
| if err == nil { | ||
| t.Fatal("expected an error for an oversized entry, got nil") | ||
| } | ||
| if !strings.Contains(err.Error(), "exceeds the") { | ||
| t.Errorf("error = %v, want a size-limit rejection", err) | ||
| } | ||
| } |
There was a problem hiding this comment.
📐 Maintainability & Code Quality | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Files:"
git ls-files | rg '(^|/)zip_test\.go$|(^|/)zip\.go$|pkg/extract' || true
echo
echo "Search symbols:"
rg -n "func (copyBoundedZipEntry|UnzipFolder)|maxUnzipEntrySize|exceeds the" -S . || true
echo
echo "zip_test.go around referenced lines:"
if [ -f pkg/extract/zip_test.go ]; then
sed -n '1,140p' pkg/extract/zip_test.go | cat -n
fi
echo
echo "zip.go outline:"
if [ -f pkg/extract/zip.go ]; then
ast-grep outline pkg/extract/zip.go --view expanded || true
sed -n '1,240p' pkg/extract/zip.go | cat -n
fiRepository: devsy-org/devsy
Length of output: 7312
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Git status/stat:"
git diff --stat || true
git diff HEAD -- pkg/extract/zip_test.go pkg/extract/zip.go | sed -n '1,220p' || trueRepository: devsy-org/devsy
Length of output: 170
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "Behavioral probe for bounded-copy and partial-write before error:"
cat >/tmp/probe.go <<'EOF'
package main
import (
"errors"
"fmt"
"io"
"testing"
)
func CopyN(dst io.Writer, r io.Reader, n int64) (written int64, err error) {
if n <= 0 {
return 0, nil
}
limit := &limitedReader{R: r, N: n}
defer limit.Close()
written, err = io.Copy(dst, limit)
if errors.Is(limit.Err, io.EOF) && written < n {
return written, io.EOF
}
return
}
type limitedReader struct {
R io.Reader
N int64
}
func (l *limitedReader) Read(p []byte) (n int, err error) {
n, err = l.R.Read(p)
if int64(n) > l.N {
l.N = 0
err = io.EOF
} else {
l.N -= int64(n)
}
return
}
func (l *limitedReader) Close() error {
return nil
}
func (l *limitedReader) Copy(p []byte) []byte {
buf := make([]byte, l.N)
copy(buf, p[:n])
return buf
}
func (l *limitedReader) Writer(p []byte) (int64, error) {
n := int64(copy(p, "x"))
l.N -= n
l.Written = append(l.Written, "x"[:n]...)
return n, nil
}
func CopyN_WithWriterInsteadOfR() (n int64) { return 0 }
type limitedWriter struct {
limit int64
data []byte
}
func (l *limitedWriter) Write(p []byte) (int64, error) {
remaining := l.limit - int64(len(l.data))
if remaining <= 0 {
return 0, io.EOF
}
n := int(remaining)
if n > len(p) {
n = len(p)
}
l.data = append(l.data, p[:n]...)
return int64(n), nil
}
func CopyN(dst io.Writer, r io.Reader, n int64) (written int64, err error) {}
func (w *limitedWriter) Read(p []byte) (int64, error) { return 0, nil }
func CopyN(dst io.Writer, r io.Reader, n int64) (written int64, err error) { return 0, nil }
func TestProbe(t *testing.T) {
w := &limitedWriter{limit: 5}
n, err := io.CopyN(w, strings.NewReader(strings.Repeat("x", 10)), 5+1)
t.Logf("CopyN returned n=%d err=%v data=%q len(data)=%d", n, err, w.data, len(w.data))
}
EOF
GOOS= GOARCH= go tool compile -S /tmp/probe.go >/tmp/probe.s 2>&1 || true
python3 - <<'PY'
from pathlib import Path
p=Path('/tmp/probe.s')
print(p.exists())
print(p.read_text() if p.exists() else "")
PYRepository: devsy-org/devsy
Length of output: 948
🌐 Web query:
Go io.CopyN implementation truncated io.EOF behavior returns written before io.EOF
💡 Result:
In Go, the io.CopyN function is designed to copy exactly n bytes from a source to a destination, or stop early if an error or EOF occurs [1][2]. Its behavior regarding EOF is explicitly defined to ensure callers can distinguish between a successful completion and a truncated read [1][2]. The implementation of io.CopyN is as follows [1][2]: func CopyN(dst Writer, src Reader, n int64) (written int64, err error) { written, err = Copy(dst, LimitReader(src, n)) if written == n { return n, nil } if written < n && err == nil { // src stopped early; must have been EOF. err = EOF } return } Key aspects of this implementation include: 1. Guarantee of Return Values: On return, written == n if and only if err == nil [1][2]. 2. Handling of EOF: Because io.Copy treats EOF as a signal that the source has no more data (rather than an error) [3][4], io.CopyN must explicitly detect when the source has finished before n bytes are reached. If the total number of bytes written is less than n and no other error occurred, io.CopyN will return the number of bytes written and the io.EOF error [1][2]. 3. Consistency: This implementation ensures consistent behavior regardless of whether the underlying destination implements the io.ReaderFrom interface (the "fast path") or uses the default buffered copy (the "slow path") [5][6]. While historical versions of Go had inconsistencies where different paths might return nil instead of io.EOF for truncated reads, current implementations correctly normalize this by checking the written count against n and substituting io.EOF when necessary [7][2]. Callers should always check the number of bytes written (written) first to handle partial copies correctly, even if an error is returned [3][4].
Citations:
- 1: https://github.com/golang/go/blob/go1.23.4/src/io/io.go
- 2: https://go.dev/src/io/io.go?m=text
- 3: https://pkg.go.dev/io@go1.26.5
- 4: https://pkg.go.dev/io
- 5: https://go.dev/src/io/io_test.go?m=text
- 6: io: Copyn slow- and fast-path asymmetry golang/go#1383
- 7: https://groups.google.com/g/golang-dev/c/hvd4lUJU7-4
🌐 Web query:
Go io.CopyN destination file partial writes truncation behavior errors.Is io.EOF
💡 Result:
In Go, the io.CopyN function is designed to copy exactly n bytes from a source to a destination. Its behavior regarding errors and EOF is defined as follows: Core Contract io.CopyN returns the number of bytes copied (written) and an error (err) [1][2]. The fundamental invariant is that written == n if and only if err == nil [1][2]. Behavior on EOF If the source reader provides fewer than n bytes before returning io.EOF, io.CopyN will return the number of bytes successfully copied and an io.EOF error [2][3]. This indicates that the source stream ended prematurely relative to the requested amount [2]. Behavior on Partial Writes or Other Errors 1. Partial Writes/Errors: If a write error occurs or the writer fails to accept data, io.CopyN returns the number of bytes written before the error occurred and the error itself [1][2][3]. 2. Truncation: io.CopyN does not perform truncation on the destination file. If you are writing to an io.Writer that is a file (e.g., *os.File), it will continue writing from the current file offset unless the file was explicitly opened with flags like os.O_TRUNC. 3. Error Types: If an error other than EOF occurs (e.g., a disk I/O error), it is returned immediately [1][2]. Implementation Note io.CopyN uses io.Copy internally [2]. If the destination implements the ReaderFrom interface, it may delegate the copy operation to that interface [1][2]. While io.CopyN attempts to provide consistent behavior, the exact error returned can depend on whether the underlying io.Writer/io.Reader implementation behaves correctly (e.g., by not returning io.EOF prematurely) [4][3]. In summary, if you receive err == io.EOF from io.CopyN, it signifies that the source reader reached the end of the stream before n bytes were provided [2]. It does not inherently indicate a file truncation event in the destination, but rather an undersized input stream [2].
Citations:
- 1: https://pkg.go.dev/io@go1.26.5
- 2: https://go.dev/src/io/io.go?m=text
- 3: https://github.com/golang/go/blob/master/src/io/io.go
- 4: io: Copyn slow- and fast-path asymmetry golang/go#1383
🌐 Web query:
pkg.go.dev io CopyN returns number of bytes written err EOF if written less than n
💡 Result:
Yes, if io.CopyN writes fewer than the requested n bytes, it returns io.EOF [1][2]. The official Go documentation for io.CopyN states: "On return, written == n if and only if err == nil" [1][3]. Specifically, the implementation ensures that if fewer than n bytes are copied and no other error occurred (meaning the source reached EOF prematurely), the function explicitly returns io.EOF as the error [2]. The logic inside the function is as follows [2]: 1. It attempts to copy n bytes using io.Copy with a LimitedReader [2]. 2. If the number of bytes written equals n, it returns nil as the error [2]. 3. If the number of bytes written is less than n and no other error was encountered during the copy process, it assigns io.EOF to the error variable before returning [2]. This guarantees that a caller can distinguish between a successful copy of the full requested amount and a short read that reached the end of the source prematurely [1][4].
Citations:
- 1: https://pkg.go.dev/io
- 2: https://go.dev/src/io/io.go?m=text
- 3: https://pkg.go.dev/io@go1.26.5
- 4: https://groups.google.com/g/golang-dev/c/F2l9QXXLNZU
🌐 Web query:
Go io.Copy destination *os.File truncation existing file O_TRUNC CopyN behavior
💡 Result:
The truncation of an existing file occurs at the moment the file is opened with the O_TRUNC flag, not when io.Copy or io.CopyN is called [1][2][3]. 1. O_TRUNC Behavior: When you open a file using os.OpenFile with the O_TRUNC flag, the operating system truncates the file (sets its size to zero) immediately upon opening, provided the file is a regular writable file [1][2][3]. This is a file-system level operation associated with the file descriptor, not the subsequent I/O operations [1]. 2. io.Copy and io.CopyN: These functions are agnostic to the O_TRUNC flag [4][5][6]. They simply use the provided io.Writer (the *os.File) to perform write operations [4][5]. If the *os.File was opened with O_TRUNC, it is already empty by the time io.Copy or io.CopyN begins writing data [1]. 3. Implementation Details: While io.Copy and io.CopyN may use optimization paths (such as calling the destination's ReadFrom method if it implements it, which *os.File does to utilize system calls like copy_file_range) [4][5][6], these optimizations do not change the fact that the file's truncation state is determined solely by how it was opened [1][3]. If you wish to truncate a file after it has already been opened without O_TRUNC, you must explicitly call the (*os.File).Truncate(0) method [2]. In summary, if your *os.File was opened with O_TRUNC, the file will be empty before io.Copy or io.CopyN even starts. The copy functions themselves do not trigger or manage this truncation [1][4].
Citations:
- 1: https://github.com/golang/go/blob/99d4927f/src/os/file.go
- 2: https://pkg.go.dev/os@go1.26.4
- 3: https://go.googlesource.com/go/+/9baddd3f21230c55f0ad2a10f5f20579dcf0a0bb/src/os/file.go
- 4: https://pkg.go.dev/io@go1.26.5
- 5: https://pkg.go.dev/io?GOOS=darwin
- 6: https://pkg.go.dev/io
Write and assert partial cleanup for oversized zip entries.
TestUnzipFolder_RejectsOversizedEntry only checks the returned error, so it passes even while UnzipFolder writes 10 bytes with a 5-byte limit. Add a direct copyBoundedZipEntry test with a bytes.Buffer that rejects N+1 bytes, and assert that UnzipFolder never creates the oversized big.bin file after rejection.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@pkg/extract/zip_test.go` around lines 82 - 101, Strengthen
TestUnzipFolder_RejectsOversizedEntry by asserting that the rejected big.bin
path does not exist in dest after UnzipFolder fails, and add a direct
copyBoundedZipEntry test using bytes.Buffer that verifies writing N+1 bytes is
rejected. Keep the existing error assertions and tiny maxUnzipEntrySize setup.
1 participant
Summary
Fixes the five
gosecrules that had exactly one finding repo-wide, grouped together since each is small:pkg/workspace/image.go(G107) --getProjectImagecalledhttp.Get(link)before checking whetherlink's host was one it knows how to parse.linkis user-controlled (the workspace source argument), so this was an open SSRF: an attacker-supplied URL got fetched regardless of whether the response was later discarded. Moved the host allowlist check before the request.pkg/daemon/platform/socket.go(G302) -- the daemon's unix socket waschmod 0666, letting any local user connect to it. Tightened to0600; nothing in the codebase relies on cross-user access.pkg/extract/zip.go(G305) -- the existing Zip Slip guard (strings.HasPrefixagainst the cleaned destination) already implements exactly what this rule checks for; gosec can't verify it statically. Documented as a reviewed false positive, matching the repo's existing#nosecconvention.pkg/extract/zip.go(G110) --unzipFile'sio.Copyhad no size bound, so a malicious archive (e.g. a compromised provider binary download -- seepkg/provider/download.go'sextractZipArchive) could decompress to exhaust disk space. Bounded the copy to 2 GiB per entry viaio.CopyN, independent of what the entry's header claims (a spoofed header wouldn't defeat this, since the bound is enforced on bytes actually written, not declared size).e2e/framework/util.go(G122) --copyDir'sfilepath.Walksource is always a test-fixture path supplied by test authors within this repo, never attacker-controlled input. Documented as a reviewed false positive.Added regression tests for the three behavior changes (image.go's host check, socket.go's permission, zip.go's size bound) -- each was verified to fail against the pre-fix code before the fix landed.
Summary by CodeRabbit
Security Improvements
Bug Fixes