July upstream changes#195
Conversation
|
Warning Review limit reached
Next review available in: 29 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
📝 WalkthroughWalkthroughThis PR adds Docker registry credential resolution by image reference, overflow-safe disk quota accounting with a quota-aware file wrapper, protected server-state enforcement that cancels and blocks SFTP sessions, canonical UUID validation for backup identifiers, hardened backup restore download validation with a configurable host allowlist, and a symlink-safe timestamp fix. ChangesDocker registry credential resolution
Disk quota overflow safety and quota-aware writes
Protected server state and SFTP enforcement
Backup UUID validation and restore hardening
Symlink-safe timestamp fix
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Router as postServerRestoreBackup
participant Validator as validateBackupDownloadUrl
participant HttpClient as backupRestoreHttpClient
participant Remote as Download URL
Client->>Router: restore request (download_url)
Router->>Validator: validateBackupDownloadUrl(url)
Validator->>Validator: isBlockedBackupRestoreIP / isAllowedBackupRestoreDestination
Validator-->>Router: ok or backupDownloadError
Router->>HttpClient: GET download_url
HttpClient->>Remote: resolve host, dial, fetch
Remote-->>HttpClient: response
HttpClient-->>Router: status, content-type
Router->>Router: isSupportedBackupRestoreContentType check
Router-->>Client: 400 or restore proceeds
sequenceDiagram
participant Client as SFTP Client
participant Handler
participant Writer as quotaWriterAt
participant Server
Client->>Handler: Filewrite request
Handler->>Server: IsInProtectedState()
Server-->>Handler: true/false
Handler-->>Client: ErrSSHFxPermissionDenied (if protected)
Handler->>Writer: WriteAt(data)
Writer->>Server: IsInProtectedState()
Writer-->>Client: denied or forwarded result
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 inconclusive)
✅ Passed checks (4 passed)
✨ 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
🧹 Nitpick comments (4)
internal/ufs/fs_unix_test.go (1)
322-333: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAlso assert the symlink's own mtime changed.
The test confirms the target is untouched but never verifies that
linkitself actually received the newchangedtimestamp, leaving the positive half of theAT_SYMLINK_NOFOLLOWbehavior unverified.✅ Suggested addition
st, err := os.Lstat(target) if err != nil { t.Fatal(err) } if !st.ModTime().Equal(original) { t.Fatalf("expected target mtime to remain %s, got %s", original, st.ModTime()) } + + linkStat, err := os.Lstat(filepath.Join(root, "link")) + if err != nil { + t.Fatal(err) + } + if !linkStat.ModTime().Equal(changed) { + t.Fatalf("expected link mtime to be %s, got %s", changed, linkStat.ModTime()) + } }🤖 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 `@internal/ufs/fs_unix_test.go` around lines 322 - 333, The symlink timestamp test in the `fs.Chtimes` path only verifies that the target file’s mtime stays unchanged, but it never checks that the symlink itself was updated. After calling `fs.Chtimes("link", changed, changed)`, add an assertion using `os.Lstat("link")` to confirm the link’s own `ModTime()` matches `changed`, alongside the existing target check, so the `AT_SYMLINK_NOFOLLOW` behavior is fully covered.router/router_server_backup.go (1)
249-256: 📐 Maintainability & Code Quality | 🔵 TrivialDuplicate UUID canonicalization logic with
backup.go'snormalizedIdentifier.
parseBackupUuidre-implements the identical parse/length/lowercase-match check found inserver/backup/backup.go. Consider consolidating into one exported helper to avoid drift between the two validation paths.🤖 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 `@router/router_server_backup.go` around lines 249 - 256, The UUID canonicalization check in parseBackupUuid duplicates the same parse/length/lowercase validation already used by normalizedIdentifier in backup.go. Refactor parseBackupUuid to call a shared helper (ideally the existing normalizedIdentifier or a common exported utility) so both validation paths stay identical, while keeping the current AbortWithStatusJSON behavior on failure.server/backup/backup.go (2)
101-133: 📐 Maintainability & Code Quality | 🔵 TrivialLGTM on identifier validation, but logic is duplicated with the router layer.
normalizedIdentifier()'s canonical-UUID check (parse + length + lowercase match) duplicates the identical logic inrouter/router_server_backup.go'sparseBackupUuid. Consider extracting a single shared helper (e.g., in a smalluuidutilpackage or exported frombackup) that both call, so future changes to canonicalization rules don't need to be kept in sync in two places.🤖 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 `@server/backup/backup.go` around lines 101 - 133, The canonical backup UUID validation logic is duplicated between Backup.normalizedIdentifier() and router/router_server_backup.go’s parseBackupUuid, so extract the parse-plus-canonicalization check into one shared helper and have both call it. Keep the existing behavior the same, but move the UUID parsing/length/lowercase validation into a single reusable function (for example in backup or a small uuidutil helper) so future canonicalization changes only need to be made in one place.
144-146: 🔒 Security & Privacy | 🔵 Trivial | 💤 Low valueLGTM on the added validation.
Unrelated to this diff: static analysis flags the pre-existing
sha1.New()call a few lines below as a broken hash algorithm. Since this checksum is only used for backup integrity/dedup rather than a cryptographic security guarantee, and changing it would require coordinated panel-side changes, this is optional and out of scope for this PR.🤖 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 `@server/backup/backup.go` around lines 144 - 146, The new validateIdentifier addition is fine, but the backup checksum code still uses sha1.New(), which static analysis flags as a broken hash algorithm. If you choose to address it in this area, update the checksum path in backup/backup.go to use a non-broken hash implementation and keep the change coordinated with any panel-side checksum consumers, since the checksum is used for backup integrity/dedup and must remain compatible.Source: Linters/SAST tools
🤖 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 `@environment/docker/container.go`:
- Line 393: The build is failing because ensureImageExists references an
undefined img variable; update the Docker registry credential lookup to use the
existing image parameter instead. Fix the assignment in ensureImageExists so
config.Get().Docker.RegistryCredentialsForImage is called with image, keeping
the surrounding registry and registryAuth handling unchanged.
In `@router/router_server_backup.go`:
- Around line 222-227: The deleteServerBackup handler is missing the server UUID
required by backup.LocateLocal. Update the deleteServerBackup flow to bind the
server from middleware.ExtractServer(c) and pass s.ID() as the third argument
when calling backup.LocateLocal, alongside the existing
middleware.ExtractApiClient(c) and backupUuid values.
- Around line 107-120: The backup lookup calls are missing the required server
UUID argument, causing a signature mismatch with backup.LocateLocal. Update both
call sites in router_server_backup.go that invoke backup.LocateLocal to pass the
current server identifier as the third parameter, using the server UUID already
available in this request flow alongside backupUuid.
In `@server/install.go`:
- Around line 172-181: `SetRestoring` is missing the state update and
`IsInProtectedState` contains misplaced unreachable code; move the
`s.restoring.Store(state)` call back into `SetRestoring` so restore state is
persisted before/after `CancelAll`, and remove the stray line from
`IsInProtectedState` so that method only returns the combined protected-state
check using `IsInstalling`, `IsTransferring`, and `IsRestoring`.
In `@sftp/handler.go`:
- Around line 38-61: Fix the server package build issue before touching
quotaWriterAt: in IsInProtectedState() remove the unreachable
s.restoring.Store(state) and replace the undefined state usage with the correct
restoring flag handling, and in SetRestoring() make sure the restoring state is
actually stored so protected-state checks work. Use the existing
IsInProtectedState and SetRestoring methods in server/install.go as the target
locations.
---
Nitpick comments:
In `@internal/ufs/fs_unix_test.go`:
- Around line 322-333: The symlink timestamp test in the `fs.Chtimes` path only
verifies that the target file’s mtime stays unchanged, but it never checks that
the symlink itself was updated. After calling `fs.Chtimes("link", changed,
changed)`, add an assertion using `os.Lstat("link")` to confirm the link’s own
`ModTime()` matches `changed`, alongside the existing target check, so the
`AT_SYMLINK_NOFOLLOW` behavior is fully covered.
In `@router/router_server_backup.go`:
- Around line 249-256: The UUID canonicalization check in parseBackupUuid
duplicates the same parse/length/lowercase validation already used by
normalizedIdentifier in backup.go. Refactor parseBackupUuid to call a shared
helper (ideally the existing normalizedIdentifier or a common exported utility)
so both validation paths stay identical, while keeping the current
AbortWithStatusJSON behavior on failure.
In `@server/backup/backup.go`:
- Around line 101-133: The canonical backup UUID validation logic is duplicated
between Backup.normalizedIdentifier() and router/router_server_backup.go’s
parseBackupUuid, so extract the parse-plus-canonicalization check into one
shared helper and have both call it. Keep the existing behavior the same, but
move the UUID parsing/length/lowercase validation into a single reusable
function (for example in backup or a small uuidutil helper) so future
canonicalization changes only need to be made in one place.
- Around line 144-146: The new validateIdentifier addition is fine, but the
backup checksum code still uses sha1.New(), which static analysis flags as a
broken hash algorithm. If you choose to address it in this area, update the
checksum path in backup/backup.go to use a non-broken hash implementation and
keep the change coordinated with any panel-side checksum consumers, since the
checksum is used for backup integrity/dedup and must remain compatible.
🪄 Autofix (Beta)
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
Run ID: 6045b619-5bed-454b-820c-233cc2894c67
📒 Files selected for processing (23)
config/config.goconfig/config_docker.goconfig/onfig_docker_test.goenvironment/docker/container.gointernal/ufs/fs_quota.gointernal/ufs/fs_quota_test.gointernal/ufs/fs_unix.gointernal/ufs/fs_unix_test.gorouter/router_server_backup.gorouter/router_server_backup_test.goserver/backup/backup.goserver/backup/backup_local.goserver/backup/backup_s3.goserver/backup/backup_test.goserver/filesystem/compress.goserver/filesystem/disk_space.goserver/filesystem/filesystem.goserver/filesystem/filesystem_test.goserver/filesystem/quota_file.goserver/install.goserver/state_test.gosftp/handler.gosftp/handler_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (1)
- GitHub Check: Analyze (go)
⚠️ CI failures not shown inline (5)
GitHub Actions: Push / Build and Test (ubuntu-22.04, 1.25.11, linux, arm64): July upstream changes
Conclusion: failure
##[group]Run go build -v -trimpath -ldflags="-s -w -X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings ${SRC_PATH}
�[36;1mgo build -v -trimpath -ldflags="-s -w -X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings ${SRC_PATH}�[0m
�[36;1mgo build -v -trimpath -ldflags="-X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings_debug ${SRC_PATH}�[0m
�[36;1mchmod 755 dist/*�[0m
shell: /usr/bin/bash -e {0}
env:
GOTOOLCHAIN: local
GOOS: linux
GOARCH: arm64
CGO_ENABLED: 0
SRC_PATH: github.com/pelican-dev/wings
##[endgroup]
internal/goarch
internal/unsafeheader
internal/byteorder
internal/coverage/rtcov
internal/cpu
internal/abi
internal/godebugs
internal/goexperiment
internal/goos
internal/bytealg
internal/chacha8rand
internal/profilerecord
internal/runtime/atomic
internal/runtime/math
internal/runtime/syscall
internal/runtime/strconv
internal/runtime/gc
internal/asan
internal/msan
internal/runtime/cgroup
internal/runtime/exithook
internal/race
internal/runtime/sys
internal/stringslite
internal/trace/tracev2
sync/atomic
internal/synctest
internal/runtime/maps
math/bits
unicode
unicode/utf8
internal/sync
internal/itoa
cmp
math
crypto/internal/fips140/alias
crypto/internal/fips140deps/byteorder
crypto/internal/fips140deps/cpu
crypto/internal/fips140/subtle
crypto/internal/boring/sig
container/list
unicode/utf16
runtime
vendor/golang.org/x/crypto/cryptobyte/asn1
vendor/golang.org/x/crypto/internal/alias
internal/nettrace
encoding
log/internal
image/color
github.com/charmbracelet/x/ansi/parser
github.com/rivo/uniseg
github.com/charmbracelet/bubbles/runeutil
github.com/charmbracelet/huh/internal/selector
github.com/docker/docker/api
github.com/docker/docker/api/types/common
github.com/docker/docker/api/types/storage
github.com/docker/docker/api/types/checkpoint
log/slog/internal
go.opentelemetry.io/otel/trace/embedded
go.opentelemetry.io/otel/metric/embedded
...
GitHub Actions: Push / Build and Test (ubuntu-22.04, 1.26.4, linux, arm64): July upstream changes
Conclusion: failure
##[group]Run go build -v -trimpath -ldflags="-s -w -X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings ${SRC_PATH}
�[36;1mgo build -v -trimpath -ldflags="-s -w -X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings ${SRC_PATH}�[0m
�[36;1mgo build -v -trimpath -ldflags="-X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings_debug ${SRC_PATH}�[0m
�[36;1mchmod 755 dist/*�[0m
shell: /usr/bin/bash -e {0}
env:
GOTOOLCHAIN: local
GOOS: linux
GOARCH: arm64
CGO_ENABLED: 0
SRC_PATH: github.com/pelican-dev/wings
##[endgroup]
internal/goarch
internal/unsafeheader
internal/byteorder
internal/coverage/rtcov
internal/cpu
internal/abi
internal/godebugs
internal/goexperiment
internal/bytealg
internal/chacha8rand
internal/goos
internal/profilerecord
internal/runtime/atomic
internal/runtime/syscall/linux
math/bits
internal/runtime/gc
internal/runtime/sys
internal/asan
internal/strconv
internal/runtime/exithook
internal/msan
internal/race
internal/runtime/gc/scan
internal/runtime/math
internal/runtime/pprof/label
internal/runtime/maps
internal/stringslite
internal/trace/tracev2
sync/atomic
internal/synctest
unicode
internal/sync
internal/runtime/cgroup
unicode/utf8
cmp
crypto/internal/fips140/alias
crypto/internal/fips140deps/byteorder
crypto/internal/fips140deps/cpu
crypto/internal/constanttime
math
crypto/internal/fips140/subtle
crypto/subtle
crypto/internal/boring/sig
container/list
unicode/utf16
runtime
vendor/golang.org/x/crypto/cryptobyte/asn1
vendor/golang.org/x/crypto/internal/alias
internal/nettrace
encoding
log/internal
image/color
github.com/rivo/uniseg
github.com/charmbracelet/bubbles/runeutil
github.com/charmbracelet/x/ansi/parser
github.com/charmbracelet/huh/internal/selector
github.com/docker/docker/api
github.com/docker/docker/api/types/common
github.com/docker/docker/api/types/storage
github.com/docker/docker/api/types/checkpoint
log/slog/internal...
GitHub Actions: Push / 2_Build and Test (ubuntu-22.04, 1.26.4, linux, arm64).txt: July upstream changes
Conclusion: failure
##[group]Run go build -v -trimpath -ldflags="-s -w -X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings ${SRC_PATH}
�[36;1mgo build -v -trimpath -ldflags="-s -w -X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings ${SRC_PATH}�[0m
�[36;1mgo build -v -trimpath -ldflags="-X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings_debug ${SRC_PATH}�[0m
�[36;1mchmod 755 dist/*�[0m
shell: /usr/bin/bash -e {0}
env:
GOTOOLCHAIN: local
GOOS: linux
GOARCH: arm64
CGO_ENABLED: 0
SRC_PATH: github.com/pelican-dev/wings
##[endgroup]
internal/goarch
internal/unsafeheader
internal/byteorder
internal/coverage/rtcov
internal/cpu
internal/abi
internal/godebugs
internal/goexperiment
internal/bytealg
internal/chacha8rand
internal/goos
internal/profilerecord
internal/runtime/atomic
internal/runtime/syscall/linux
math/bits
internal/runtime/gc
internal/runtime/sys
internal/asan
internal/strconv
internal/runtime/exithook
internal/msan
internal/race
internal/runtime/gc/scan
internal/runtime/math
internal/runtime/pprof/label
internal/runtime/maps
internal/stringslite
internal/trace/tracev2
sync/atomic
internal/synctest
unicode
internal/sync
internal/runtime/cgroup
unicode/utf8
cmp
crypto/internal/fips140/alias
crypto/internal/fips140deps/byteorder
crypto/internal/fips140deps/cpu
crypto/internal/constanttime
math
crypto/internal/fips140/subtle
crypto/subtle
crypto/internal/boring/sig
container/list
unicode/utf16
runtime
vendor/golang.org/x/crypto/cryptobyte/asn1
vendor/golang.org/x/crypto/internal/alias
internal/nettrace
encoding
log/internal
image/color
github.com/rivo/uniseg
github.com/charmbracelet/bubbles/runeutil
github.com/charmbracelet/x/ansi/parser
github.com/charmbracelet/huh/internal/selector
github.com/docker/docker/api
github.com/docker/docker/api/types/common
github.com/docker/docker/api/types/storage
github.com/docker/docker/api/types/checkpoint
log/slog/internal...
GitHub Actions: Push / 1_Build and Test (ubuntu-22.04, 1.25.11, linux, arm64).txt: July upstream changes
Conclusion: failure
##[group]Run go build -v -trimpath -ldflags="-s -w -X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings ${SRC_PATH}
�[36;1mgo build -v -trimpath -ldflags="-s -w -X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings ${SRC_PATH}�[0m
�[36;1mgo build -v -trimpath -ldflags="-X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings_debug ${SRC_PATH}�[0m
�[36;1mchmod 755 dist/*�[0m
shell: /usr/bin/bash -e {0}
env:
GOTOOLCHAIN: local
GOOS: linux
GOARCH: arm64
CGO_ENABLED: 0
SRC_PATH: github.com/pelican-dev/wings
##[endgroup]
internal/goarch
internal/unsafeheader
internal/byteorder
internal/coverage/rtcov
internal/cpu
internal/abi
internal/godebugs
internal/goexperiment
internal/goos
internal/bytealg
internal/chacha8rand
internal/profilerecord
internal/runtime/atomic
internal/runtime/math
internal/runtime/syscall
internal/runtime/strconv
internal/runtime/gc
internal/asan
internal/msan
internal/runtime/cgroup
internal/runtime/exithook
internal/race
internal/runtime/sys
internal/stringslite
internal/trace/tracev2
sync/atomic
internal/synctest
internal/runtime/maps
math/bits
unicode
unicode/utf8
internal/sync
internal/itoa
cmp
math
crypto/internal/fips140/alias
crypto/internal/fips140deps/byteorder
crypto/internal/fips140deps/cpu
crypto/internal/fips140/subtle
crypto/internal/boring/sig
container/list
unicode/utf16
runtime
vendor/golang.org/x/crypto/cryptobyte/asn1
vendor/golang.org/x/crypto/internal/alias
internal/nettrace
encoding
log/internal
image/color
github.com/charmbracelet/x/ansi/parser
github.com/rivo/uniseg
github.com/charmbracelet/bubbles/runeutil
github.com/charmbracelet/huh/internal/selector
github.com/docker/docker/api
github.com/docker/docker/api/types/common
github.com/docker/docker/api/types/storage
github.com/docker/docker/api/types/checkpoint
log/slog/internal
go.opentelemetry.io/otel/trace/embedded
go.opentelemetry.io/otel/metric/embedded
...
GitHub Actions: Push / Build and Test (ubuntu-22.04, 1.25.11, linux, amd64): July upstream changes
Conclusion: failure
##[group]Run go build -v -trimpath -ldflags="-s -w -X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings ${SRC_PATH}
�[36;1mgo build -v -trimpath -ldflags="-s -w -X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings ${SRC_PATH}�[0m
�[36;1mgo build -v -trimpath -ldflags="-X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings_debug ${SRC_PATH}�[0m
�[36;1mchmod 755 dist/*�[0m
shell: /usr/bin/bash -e {0}
env:
GOTOOLCHAIN: local
GOOS: linux
GOARCH: amd64
CGO_ENABLED: 0
SRC_PATH: github.com/pelican-dev/wings
##[endgroup]
internal/unsafeheader
internal/byteorder
internal/goarch
internal/coverage/rtcov
internal/godebugs
internal/goexperiment
internal/goos
internal/profilerecord
internal/runtime/math
internal/runtime/strconv
internal/cpu
internal/runtime/atomic
internal/runtime/syscall
internal/abi
internal/runtime/gc
internal/asan
internal/msan
internal/bytealg
internal/chacha8rand
internal/runtime/exithook
internal/runtime/sys
internal/trace/tracev2
internal/runtime/cgroup
internal/stringslite
sync/atomic
math/bits
unicode
internal/race
internal/synctest
internal/runtime/maps
unicode/utf8
internal/sync
internal/itoa
cmp
crypto/internal/fips140/alias
math
crypto/internal/fips140deps/byteorder
crypto/internal/fips140deps/cpu
crypto/internal/fips140/subtle
crypto/internal/boring/sig
container/list
unicode/utf16
vendor/golang.org/x/crypto/cryptobyte/asn1
vendor/golang.org/x/crypto/internal/alias
internal/nettrace
encoding
log/internal
image/color
github.com/rivo/uniseg
runtime
github.com/charmbracelet/x/ansi/parser
github.com/charmbracelet/bubbles/runeutil
github.com/charmbracelet/huh/internal/selector
github.com/docker/docker/api
github.com/docker/docker/api/types/common
github.com/docker/docker/api/types/storage
github.com/docker/docker/api/types/checkpoint
log/slog/internal
go.opentelemetry.io/otel/trace/embedded
go.opentelemetry.io/otel/metric/embedded
...
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2026-03-02T13:53:08.995Z
Learnt from: parkervcp
Repo: pelican-dev/wings PR: 171
File: server/power.go:190-203
Timestamp: 2026-03-02T13:53:08.995Z
Learning: In the server package, when quotas are enabled via config.Get().System.Quotas.Enabled, the disk space check using used >= s.DiskSpace() does not require a special guard for unlimited-disk scenarios (DiskSpace() <= 0). The filesystem handles such cases, so the existing check is sufficient. Apply this pattern to similar quota-related disk checks in the server package and ensure tests/docs reflect that unlimited-disk behavior is governed by the filesystem, not by an extra guard in code.
Applied to files:
server/state_test.goserver/backup/backup_s3.goserver/backup/backup_test.goserver/filesystem/quota_file.goserver/filesystem/filesystem.goserver/backup/backup_local.goserver/filesystem/compress.goserver/filesystem/filesystem_test.goserver/filesystem/disk_space.goserver/install.goserver/backup/backup.go
🪛 ast-grep (0.44.0)
server/backup/backup.go
[warning] 146-146: SHA-1 is a cryptographically broken hash function vulnerable to collision attacks and is unsuitable for security purposes such as signatures, integrity checks, or password hashing. Use a SHA-2 family function (e.g. sha256.New() / sha256.Sum256()) or SHA-3 instead.
Context: sha1.New
Note: [CWE-327] Use of a Broken or Risky Cryptographic Algorithm.
(weak-hash-sha1-go)
🪛 GitHub Actions: Push / 0_Build and Test (ubuntu-22.04, 1.26.4, linux, amd64).txt
environment/docker/container.go
[error] 393-393: Go build failed: undefined: img (at environment/docker/container.go:393:76). Command: go build -v -trimpath -ldflags="-s -w -X ${SRC_PATH}/system.Version=dev-${GITHUB_SHA:0:7}" -o dist/wings ${SRC_PATH}
🪛 GitHub Actions: Push / 1_Build and Test (ubuntu-22.04, 1.25.11, linux, arm64).txt
environment/docker/container.go
[error] 393-393: go build failed: environment/docker/container.go:393:76: undefined: img
🪛 GitHub Actions: Push / 2_Build and Test (ubuntu-22.04, 1.26.4, linux, arm64).txt
environment/docker/container.go
[error] 393-393: Go build failed: undefined: img (environment/docker/container.go:393:76)
🪛 GitHub Actions: Push / 3_Build and Test (ubuntu-22.04, 1.25.11, linux, amd64).txt
environment/docker/container.go
[error] 393-393: go build failed: undefined: img (environment/docker/container.go:393:76)
🪛 GitHub Actions: Push / Build and Test (ubuntu-22.04, 1.25.11, linux, amd64)
environment/docker/container.go
[error] 393-393: go build failed: undefined: img
🪛 GitHub Actions: Push / Build and Test (ubuntu-22.04, 1.25.11, linux, arm64)
environment/docker/container.go
[error] 393-393: go build failed: container.go:393:76: undefined: img
🪛 GitHub Actions: Push / Build and Test (ubuntu-22.04, 1.26.4, linux, amd64)
environment/docker/container.go
[error] 393-393: go build failed: undefined: img (environment/docker/container.go:393:76).
🪛 GitHub Actions: Push / Build and Test (ubuntu-22.04, 1.26.4, linux, arm64)
environment/docker/container.go
[error] 393-393: go build failed: undefined: img (environment/docker/container.go:393:76).
🪛 GitHub Check: Build and Test (ubuntu-22.04, 1.25.11, linux, amd64)
environment/docker/container.go
[failure] 393-393:
undefined: img
🪛 GitHub Check: Build and Test (ubuntu-22.04, 1.25.11, linux, arm64)
environment/docker/container.go
[failure] 393-393:
undefined: img
🪛 GitHub Check: Build and Test (ubuntu-22.04, 1.26.4, linux, amd64)
environment/docker/container.go
[failure] 393-393:
undefined: img
🪛 GitHub Check: Build and Test (ubuntu-22.04, 1.26.4, linux, arm64)
environment/docker/container.go
[failure] 393-393:
undefined: img
🪛 golangci-lint (2.12.2)
environment/docker/container.go
[error] 393-393: : # github.com/pelican-dev/wings/environment/docker
environment/docker/container.go:393:76: undefined: img
(typecheck)
🔇 Additional comments (29)
config/config_docker.go (1)
114-199: LGTM!config/onfig_docker_test.go (1)
1-99: LGTM!server/install.go (1)
204-204: LGTM!Also applies to: 259-261
server/state_test.go (1)
5-47: Test correctly targets state cancellation, but depends on a currently broken package.Per the referenced
server/install.gocontract snippets,IsInProtectedState/SetRestoringcurrently contain a bug that breaks compilation of packageserver— see the detailed comment insftp/handler.go(lines 38-61, 327). This test cannot pass/build until that root cause is fixed.sftp/handler.go (3)
106-108: Whitespace-only changes, no functional impact.Also applies to: 135-137, 181-184
128-128: 🚀 Performance & ScalabilityVerify the cost of forcing a fresh disk-usage scan on every SFTP write open.
HasSpaceAvailable(true)→HasSpaceAvailable(false)forcesDiskUsageto skip the cached value on everyFilewritecall. Given quota is now also enforced at the lowerquotaFilelevel (per the stack's "Overflow-safe disk quota" cohort), confirm this eager recompute is intentional and its cost (e.g., directory walk) is acceptable on the SFTP hot path, rather than a leftover from before the low-level enforcement existed.
43-53: LGTM!Also applies to: 55-60, 121-169, 326-329
sftp/handler_test.go (2)
36-41: "restoring" subtests depend on the same root-cause bug flagged insftp/handler.go.Once
IsInProtectedState/SetRestoringare fixed (see comment onsftp/handler.golines 38-61), these subtests should pass as written; until then they will fail/fail to compile.Also applies to: 81-86
1-18: LGTM!Also applies to: 19-35, 42-80, 87-140
internal/ufs/fs_quota.go (2)
71-97: LGTM!
121-129: LGTM!internal/ufs/fs_quota_test.go (1)
8-47: LGTM!server/filesystem/disk_space.go (2)
168-202: LGTM!
211-240: LGTM!server/filesystem/quota_file.go (2)
23-112: LGTM!
11-21: 🩺 Stability & AvailabilityNo quota bypass here — current
Touchcall paths only write/close the returned handle; none call a promotedTruncate, so this wrapper does not expose a reachable quota bypass.> Likely an incorrect or invalid review comment.server/filesystem/filesystem.go (1)
98-111: LGTM!server/filesystem/compress.go (2)
156-159: LGTM!
247-259: LGTM!server/filesystem/filesystem_test.go (2)
115-196: LGTM!
268-279: LGTM!internal/ufs/fs_unix_test.go (1)
16-16: LGTM!internal/ufs/fs_unix.go (1)
173-174: 🎯 Functional CorrectnessNo internal caller depends on the old symlink-following
Chtimesbehavior.server/backup/backup_local.go (1)
38-53: LGTM!Also applies to: 56-75, 84-110, 114-142
server/backup/backup_s3.go (1)
42-47: LGTM!Also applies to: 56-94
server/backup/backup_test.go (1)
1-103: LGTM!config/config.go (1)
293-297: LGTM!router/router_server_backup.go (1)
4-36: Solid SSRF hardening design.The blocked-IP checks, per-connection DNS resolution + validation in the custom
DialContext, redirect re-validation, and content-type allowlisting together form a robust defense against SSRF via DNS rebinding and redirect chains — the "fail closed if any resolved IP is unsafe" behavior in the dial loop is a good touch.Also applies to: 39-81, 92-106, 181-216, 258-370
router/router_server_backup_test.go (1)
1-337: Good test coverage for the new restore hardening logic; contingent on theLocateLocalfix.This test suite (loopback rejection, non-UUID rejection, bad status, content-type validation, UUID parsing, allowlist) is thorough and directly exercises
postServerRestoreBackup. Note that if thebackup.LocateLocalargument-count issue flagged inrouter_server_backup.gois real, these tests wouldn't currently compile/pass — see that file's review comments.
|
@coderabbitai review |
✅ Action performedReview finished.
|
✅ Action performedReview finished.
|
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@router/router_server_backup_test.go`:
- Around line 86-88: The backup test helper’s PushServerStateChange method
references an undefined ServerStateChange type, causing the router test package
to fail typecheck. Update backupTestRemoteClient.PushServerStateChange to use
the correct fully qualified type or add the missing import/alias for the actual
ServerStateChange definition used elsewhere in the router package, and ensure
the method signature matches the remote client interface exactly.
🪄 Autofix (Beta)
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
Run ID: 9eda69fd-76e8-430c-990d-6b80f1e02511
📒 Files selected for processing (1)
router/router_server_backup_test.go
📜 Review details
⏰ Context from checks skipped due to timeout. (5)
- GitHub Check: Analyze (go)
- GitHub Check: Build and Test (ubuntu-22.04, 1.26.4, linux, amd64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.26.4, linux, arm64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.11, linux, arm64)
- GitHub Check: Build and Test (ubuntu-22.04, 1.25.11, linux, amd64)
🧰 Additional context used
🪛 golangci-lint (2.12.2)
router/router_server_backup_test.go
[error] 86-86: : # github.com/pelican-dev/wings/router [github.com/pelican-dev/wings/router.test]
router/router_server_backup_test.go:86:80: undefined: ServerStateChange
(typecheck)
1 participant
Changes
Summary by CodeRabbit
New Features
restore_host_allowlistsetting to control which hostnames, IPs, and CIDR ranges backup restores may download from.Bug Fixes
chtimesno longer follows symlinks.Behavior / Security