File operations (hash, archive) - #6
Merged
Merged
Conversation
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Comment |
Coverage Report for CI Build 30671638208Coverage increased (+4.5%) to 40.662%Details
Uncovered Changes
Coverage RegressionsNo coverage regressions found. Coverage Stats
💛 - Coveralls |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 8
🧹 Nitpick comments (2)
internal/app/archive/create_tar.go (1)
60-88: 🩺 Stability & Availability | 🔵 Trivial | ⚡ Quick winReturn a closer from
decompressReader.extractTarandextractSingleboth use the returned stream without closing it, so gzip/zstd resources stay open for the full archive read. Return anio.ReadCloser(or close func) and defer it in both call sites.🤖 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/app/archive/create_tar.go` around lines 60 - 88, Update decompressReader to return an io.ReadCloser for every compression mode, wrapping non-closer readers and bzip2/xz readers as needed while preserving existing initialization errors. In both extractTar and extractSingle, capture the returned closer and defer closing it immediately after successful decompression.internal/app/grpc/archive_handler_test.go (1)
206-215: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winConsider covering the empty-
request_iddrop path and the timeout-exceeded branch.No test exercises
HandleArchiveRequestwith an emptyrequest_id(dropped at archive_handler.go lines 63-66) or the"timeout exceeded"branch ofsendErrorResponse(archive_handler.go line 252). Both are cheap to add given the existingfakeSenderharness.🤖 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/app/grpc/archive_handler_test.go` around lines 206 - 215, Extend the archive handler tests around TestGRPCArchiveHandler_NoOperation to cover an empty request ID being dropped without a response, and add a sendErrorResponse scenario where the sender timeout is exceeded and the error is reported as "timeout exceeded". Reuse the existing fakeSender harness and assert the expected no-response and timeout behaviors.
🤖 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 `@go.mod`:
- Line 31: Update the google.golang.org/grpc dependency from v1.82.0 to v1.82.1
or later in go.mod, then rerun dependency scanning and the test suite to verify
the upgrade.
In `@internal/app/archive/archiver.go`:
- Around line 51-71: Clamp request-supplied maxBytes to math.MaxInt64 in
newAccumulator before storing it, so the int64 conversion used by bytesLeft and
addEntry cannot overflow. Preserve the existing defaulting behavior for zero
values and keep maxFiles handling unchanged.
In `@internal/app/archive/create_tar.go`:
- Around line 108-148: The compressor stream is not closed when archive creation
returns early. In internal/app/archive/create_tar.go lines 108-148, defer
closing closer after it is created and guard the existing success-path Close
with a closed flag or named-error defer; in
internal/app/archive/create_single.go lines 29-45, defer closer.Close
immediately after compressWriter succeeds while retaining the explicit
success-path close so close errors remain reported.
In `@internal/app/archive/create_test.go`:
- Around line 346-351: Fix the compression-level initialization in
TestCreateCompressionLevels by replacing the invalid new(int32(1)) expression
with a valid pointer to an int32 value of 1, while preserving the existing
values for the "store" and "best" entries.
In `@internal/app/archive/create.go`:
- Around line 199-221: Update walkSource so the depth limit tracks only followed
symlink resolutions: increment the symlink-hop counter when resolving a symlink,
pass that counter through recursive calls unchanged for ordinary directories,
and retain the “symlink nesting too deep” check for hops exceeding
maxFollowDepth. Do not use directory recursion depth to trigger this
symlink-specific error.
In `@internal/app/archive/extract_formats.go`:
- Around line 219-252: Update extractRar to detect symlink entries using
hdr.Mode() and route them through the existing symlink-handling path before the
regular putFile call. Preserve directory handling and ensure symlink entries are
not treated as regular files, matching the ZIP/TAR extraction behavior.
In `@internal/app/grpc/archive_handler.go`:
- Around line 124-176: Add panic recovery to the goroutine entrypoint method
GRPCArchiveHandler.run so panics from daemonarchive.Create or
daemonarchive.Extract are converted into an operation failure instead of
escaping the process. Use a deferred recover handler that records the recovered
value as an error (using fmt as needed), logs it through the existing log.Entry,
and sends the failure with h.sendErrorResponse using the current request context
and archive metadata; preserve normal cleanup and error handling.
In `@internal/app/grpc/file_handler.go`:
- Around line 614-644: Update the file-hashing flow around root.Lstat,
info.IsDir, and io.Copy to reject every non-regular file before root.Open,
preserving the existing error-return pattern. Capture io.Copy’s returned byte
count and assign it to fh.Size after a successful copy, so the reported size
matches the data used for hashing rather than the initial metadata.
---
Nitpick comments:
In `@internal/app/archive/create_tar.go`:
- Around line 60-88: Update decompressReader to return an io.ReadCloser for
every compression mode, wrapping non-closer readers and bzip2/xz readers as
needed while preserving existing initialization errors. In both extractTar and
extractSingle, capture the returned closer and defer closing it immediately
after successful decompression.
In `@internal/app/grpc/archive_handler_test.go`:
- Around line 206-215: Extend the archive handler tests around
TestGRPCArchiveHandler_NoOperation to cover an empty request ID being dropped
without a response, and add a sendErrorResponse scenario where the sender
timeout is exceeded and the error is reported as "timeout exceeded". Reuse the
existing fakeSender harness and assert the expected no-response and timeout
behaviors.
🪄 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: 88250604-6e6f-451f-bc41-be3b88f99d5d
⛔ Files ignored due to path filters (3)
go.sumis excluded by!**/*.sumtest/files/test.7zis excluded by!**/*.7ztest/files/test.raris excluded by!**/*.rar
📒 Files selected for processing (20)
go.modinternal/app/archive/archiver.gointernal/app/archive/create.gointernal/app/archive/create_single.gointernal/app/archive/create_tar.gointernal/app/archive/create_test.gointernal/app/archive/create_zip.gointernal/app/archive/extract.gointernal/app/archive/extract_formats.gointernal/app/archive/extract_test.gointernal/app/archive/format.gointernal/app/archive/helpers_test.gointernal/app/di/internal/definitions/grpc.gointernal/app/grpc/archive_handler.gointernal/app/grpc/archive_handler_test.gointernal/app/grpc/client.gointernal/app/grpc/file_handler.gointernal/app/grpc/file_handler_hash_test.gointernal/app/grpc/hash.gointernal/app/grpc/server_handler.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
gameap/gameap.github.io(manual)
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 (2)
internal/app/grpc/archive_handler.go (2)
154-201: 🗄️ Data Integrity & Integration | 🟠 Major | ⚡ Quick winFix: panic recovery does not stop the progress loop before responding.
The recover defer at lines 157-163 is registered before
progressDoneandprogressStoppedare declared (lines 182-184) and beforeprogressLoopstarts (line 184). A closure cannot reference variables declared later in the same block, so the recover handler structurally cannot join the progress reporter.The happy path (lines 197-201) explicitly closes
progressDoneand waits forprogressStoppedbefore sending the final response, to guarantee that no progress message follows the final response.TestGRPCArchiveHandler_ProgressStopsBeforeResponseenforces this guarantee for the success path. On the panic path, this join does not happen. Ifdaemonarchive.Create/Extractpanics whileprogressLoop's ticker is about to fire, the error response can go out beforeentry.cancel()stops the reporter, and a stray progress message can follow the final response. This violates the same invariant the happy path protects.Move the panic-recovery defer registration to after
progressDone/progressStoppedare created, and reuse the stop logic in both paths.🛡️ Proposed fix
- // Registered last so it runs first (LIFO): the failure response goes out - // before entry.cancel() marks the context canceled, and the remaining - // defers (sem release, cancel, registry delete) still run after recover. - defer func() { - if r := recover(); r != nil { - err := errors.Errorf("archive operation panicked: %v", r) - l.WithError(err).Error("Archive operation panicked") - h.sendErrorResponse(ctx, entry, requestID, format, err) - } - }() - var progress atomic.Pointer[archiveProgressState] progressFn := func(filesProcessed, bytesProcessed int64, currentEntry string) { progress.Store(&archiveProgressState{ filesProcessed: filesProcessed, bytesProcessed: bytesProcessed, currentEntry: currentEntry, }) } progressInterval := req.GetProgressInterval().AsDuration() if progressInterval <= 0 { progressInterval = defaultProgressInterval } if progressInterval < minProgressInterval { progressInterval = minProgressInterval } progressDone := make(chan struct{}) progressStopped := make(chan struct{}) go h.progressLoop(ctx, progressDone, progressStopped, progressInterval, requestID, &progress) + var stopOnce sync.Once + stopProgress := func() { + stopOnce.Do(func() { + close(progressDone) + <-progressStopped + }) + } + + // Registered last so it runs first (LIFO): the failure response goes out + // before entry.cancel() marks the context canceled, and the remaining + // defers (sem release, cancel, registry delete) still run after recover. + // stopProgress() ensures no stray progress message follows this error + // response, matching the ordering guarantee on the success path below. + defer func() { + if r := recover(); r != nil { + stopProgress() + err := errors.Errorf("archive operation panicked: %v", r) + l.WithError(err).Error("Archive operation panicked") + h.sendErrorResponse(ctx, entry, requestID, format, err) + } + }() var result *daemonarchive.Result var err error if create := req.GetCreate(); create != nil { l.WithField("archive_path", create.GetArchivePath()).Info("Creating archive") result, err = daemonarchive.Create(ctx, h.workDir, create, progressFn) } else { extract := req.GetExtract() l.WithField("archive_path", extract.GetArchivePath()).Info("Extracting archive") result, err = daemonarchive.Extract(ctx, h.workDir, extract, progressFn) } - // Wait for the reporter to actually stop, not just to be told to: the proto - // promises a single final response that ends the operation, and a progress - // message queued after it would reopen an operation the API considers done. - close(progressDone) - <-progressStopped + // Wait for the reporter to actually stop, not just to be told to: the proto + // promises a single final response that ends the operation, and a progress + // message queued after it would reopen an operation the API considers done. + stopProgress()Add
"sync"to the import block if not already present.🤖 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/app/grpc/archive_handler.go` around lines 154 - 201, Move the panic-recovery defer registration in the archive handler to after progressDone and progressStopped are created and progressLoop is started, so it can stop and join the reporter before sending the error response. Extract or reuse the existing progress shutdown logic from the normal completion path in both the recovery handler and success path, preserving the guarantee that no progress message follows the final response; add the sync import only if required by the shared shutdown coordination.
239-275: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winAdd panic recovery to
progressLoop.
progressLoopruns in its own goroutine (started at line 184/185) and is not covered byrun's panic recovery. It callsh.responseSender.Send(...)(lines 267-272) on every tick. If that call ever panics, the panic escapes unrecovered in this goroutine and crashes the whole daemon process, the exact failure mode the recovery added torunwas meant to prevent for every managed game server, not just this one operation.Wrap the send in a recover, matching the protection already added to
run.
🧹 Nitpick comments (2)
internal/app/archive/extract_formats.go (1)
225-236: 🎯 Functional Correctness | 🔵 Trivial | ⚡ Quick winRoute the 7z entry read errors through
wrapArchiveReadErr.
extract7zmaps encrypted archives only forsevenzip.NewReader. A 7z archive can store an unencrypted header with encrypted entry data. In that case the failure surfaces atf.Open()or during the copy, and the caller receives a generic "failed to open 7z entry" error instead ofErrArchiveEncrypted. The API then cannot prompt for a password.♻️ Proposed change
rc, err := f.Open() if err != nil { - return errors.Wrapf(err, "failed to open 7z entry %q", f.Name) + return errors.Wrapf(wrapArchiveReadErr(err, "7z"), "entry %q", f.Name) }🤖 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/app/archive/extract_formats.go` around lines 225 - 236, Update extract7z’s 7z entry read path around f.Open and the subsequent putSymlinkFrom/s.putFile calls to pass errors through wrapArchiveReadErr, preserving ErrArchiveEncrypted mapping for encrypted entry data. Replace the generic open-entry wrapping with the shared helper and apply it to copy/read failures before returning to the caller.internal/app/grpc/archive_handler_test.go (1)
256-286: 📐 Maintainability & Code Quality | 🔵 TrivialConsider a test for the panic-recovery path.
These tests cover empty request ID and timeout handling well, but no test exercises the panic-recovery branch added in
archive_handler.go(therecover()inrun). Given the past critical review comment that motivated this recovery, a test that forces a panic (e.g., via a fault-injectingProgressFuncor a crafted request that panics insidedaemonarchive.Create) and asserts a graceful error response — and that no progress message follows it — would directly validate the fix and catch the ordering gap flagged inarchive_handler.golines 154-201.🤖 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/app/grpc/archive_handler_test.go` around lines 256 - 286, The archive handler tests lack coverage for the panic-recovery branch in run. Add a test that deterministically forces a panic, such as through a fault-injecting ProgressFunc or crafted request, then assert the sender receives a graceful failed response and no progress message is emitted after that final response.
🤖 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 @.github/dependabot.yml:
- Around line 9-17: Update the Dependabot configuration by adding a separate
archive decoder group configured with applies-to: security-updates and the same
patterns as archive-decoders. Keep the existing version-update group unchanged
and give the security group a distinct name.
In @.github/workflows/test.yml:
- Around line 17-18: Update all affected workflow sites in
.github/workflows/test.yml: add persist-credentials: false to the checkout steps
at lines 17-18 and 28-31, and set repo-checkout: false for
golang/govulncheck-action@v1 at lines 60-61.
In `@internal/app/archive/extract_formats.go`:
- Around line 99-106: Update putSymlinkFrom to detect symlink targets exceeding
maxLinkTargetBytes by reading up to one byte beyond the allowed limit, then
return an error when that extra byte exists instead of calling s.putSymlink.
Preserve the existing wrapped read-error handling and normal symlink creation
for targets within the limit.
- Around line 248-251: Update the error matching logic associated with the
rardecode reader initialization to declare the sevenzip read-error target as a
*sevenzip.ReadError pointer before calling errors.As. Preserve the existing
encrypted-read error handling while ensuring errors returned as pointers are
matched.
In `@internal/app/archive/extract.go`:
- Around line 302-313: Update the overwrite replacement flow and its surrounding
symlink validation to preserve confinement after directory-to-symlink
replacements: reject replacing a directory that existing symlinks depend on, or
revalidate all affected symlink targets after each replacement. Ensure the final
archive state cannot contain a symlink such as one resolved by resolveLinkTarget
and checked by withinDest that escapes the destination.
---
Outside diff comments:
In `@internal/app/grpc/archive_handler.go`:
- Around line 154-201: Move the panic-recovery defer registration in the archive
handler to after progressDone and progressStopped are created and progressLoop
is started, so it can stop and join the reporter before sending the error
response. Extract or reuse the existing progress shutdown logic from the normal
completion path in both the recovery handler and success path, preserving the
guarantee that no progress message follows the final response; add the sync
import only if required by the shared shutdown coordination.
---
Nitpick comments:
In `@internal/app/archive/extract_formats.go`:
- Around line 225-236: Update extract7z’s 7z entry read path around f.Open and
the subsequent putSymlinkFrom/s.putFile calls to pass errors through
wrapArchiveReadErr, preserving ErrArchiveEncrypted mapping for encrypted entry
data. Replace the generic open-entry wrapping with the shared helper and apply
it to copy/read failures before returning to the caller.
In `@internal/app/grpc/archive_handler_test.go`:
- Around line 256-286: The archive handler tests lack coverage for the
panic-recovery branch in run. Add a test that deterministically forces a panic,
such as through a fault-injecting ProgressFunc or crafted request, then assert
the sender receives a graceful failed response and no progress message is
emitted after that final response.
🪄 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: 6a4181cb-b66b-4f76-9f15-1510d8281fc2
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (18)
.github/dependabot.yml.github/workflows/test.ymlgo.modinternal/app/archive/archiver.gointernal/app/archive/create.gointernal/app/archive/create_single.gointernal/app/archive/create_tar.gointernal/app/archive/create_test.gointernal/app/archive/detect.gointernal/app/archive/extract.gointernal/app/archive/extract_formats.gointernal/app/archive/extract_test.gointernal/app/di/internal/definitions/grpc.gointernal/app/grpc/archive_handler.gointernal/app/grpc/archive_handler_test.gointernal/app/grpc/client.gointernal/app/grpc/file_handler.gointernal/app/grpc/file_handler_hash_test.go
🔗 Linked repositories identified
CodeRabbit considers these linked repositories for cross-repo context during reviews:
gameap/gameap.github.io(manual)
🚧 Files skipped from review as they are similar to previous changes (5)
- internal/app/archive/create_single.go
- internal/app/di/internal/definitions/grpc.go
- go.mod
- internal/app/archive/create_tar.go
- internal/app/archive/create.go
Comment on lines
+9
to
+17
| # The archive decoders parse untrusted user-uploaded bytes; keep them | ||
| # together so a security bump lands as one reviewable change. | ||
| archive-decoders: | ||
| patterns: | ||
| - github.com/bodgit/* | ||
| - github.com/nwaples/rardecode/* | ||
| - github.com/dsnet/compress | ||
| - github.com/ulikunitz/xz | ||
| - github.com/klauspost/compress |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -eu
printf '%s\n' '--- .github/dependabot.yml ---'
cat -n .github/dependabot.yml
printf '%s\n' '--- Dependabot-related files and patterns ---'
rg -n --hidden -S 'archive-decoders|applies-to|dependabot|github.com/(bodgit|nwaples/rardecode|dsnet/compress|ulikunitz/xz|klauspost/compress)' .github go.mod go.sum 2>/dev/null || trueRepository: gameap/daemon
Length of output: 3435
🏁 Script executed (no clone):
curl -Ls https://docs.github.com/en/code-security/reference/supply-chain-security/dependabot-options-reference | rg -n -A8 -B8 'applies-to|security-updates' | head -80
Length of output: 50344
🏁 Script executed (no clone):
#!/bin/bash
set -eu
log=/tmp/coderabbit-shell-logs/shell-output-MxsXMD
printf '%s\n' '--- Relevant documentation excerpts ---'
rg -o '.{0,500}(applies-to|security-updates|groups).{0,1200}' "$log" | head -20
Length of output: 30166
Configure the archive group for security updates.
The group applies only to version updates because applies-to is omitted. Add a separate group with applies-to: security-updates and the same patterns.
🤖 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 @.github/dependabot.yml around lines 9 - 17, Update the Dependabot
configuration by adding a separate archive decoder group configured with
applies-to: security-updates and the same patterns as archive-decoders. Keep the
existing version-update group unchanged and give the security group a distinct
name.
Comment on lines
+17
to
+18
| - name: Checkout code | ||
| uses: actions/checkout@v5 |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
rg -n -C 3 'actions/checkout@|persist-credentials|repo-checkout' .github/workflows/test.yml
curl -fsSL \
https://raw.githubusercontent.com/golang/govulncheck-action/v1/action.yml |
rg -n -C 3 'repo-checkout|actions/checkout|persist-credentials'Repository: gameap/daemon
Length of output: 1103
🏁 Script executed:
#!/usr/bin/env bash
set -euo pipefail
printf '%s\n' '--- workflow ---'
cat -n .github/workflows/test.yml
printf '%s\n' '--- all checkout-related configuration in workflows ---'
rg -n -C 4 'uses:\s*([^ ]*/)?actions/checkout@|persist-credentials|repo-checkout|permissions:' .github/workflowsRepository: gameap/daemon
Length of output: 4816
Disable GitHub credential persistence for all checkouts used by this workflow.
Set persist-credentials: false on both actions/checkout@v5 steps. Set repo-checkout: false on golang/govulncheck-action@v1 to disable its default checkout.
🧰 Tools
🪛 zizmor (1.28.0)
[warning] 17-18: credential persistence through GitHub Actions artifacts (artipacked): does not set persist-credentials: false
(artipacked)
📍 Affects 1 file
.github/workflows/test.yml#L17-L18(this comment).github/workflows/test.yml#L28-L31.github/workflows/test.yml#L60-L61
🤖 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 @.github/workflows/test.yml around lines 17 - 18, Update all affected
workflow sites in .github/workflows/test.yml: add persist-credentials: false to
the checkout steps at lines 17-18 and 28-31, and set repo-checkout: false for
golang/govulncheck-action@v1 at lines 60-61.
Source: Linters/SAST tools
Comment on lines
+99
to
+106
| func putSymlinkFrom(r io.Reader, name string, s *sink) error { | ||
| target, err := io.ReadAll(io.LimitReader(r, maxLinkTargetBytes)) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "failed to read symlink entry %q", name) | ||
| } | ||
|
|
||
| return s.putSymlink(name, string(target)) | ||
| } |
There was a problem hiding this comment.
🎯 Functional Correctness | 🟡 Minor | ⚡ Quick win
Reject an oversized symlink target instead of truncating it.
io.LimitReader(r, maxLinkTargetBytes) stops at the cap without an error. If an entry stores a longer target, the code creates a symlink to a truncated path. The link is still confined by putSymlink, so this is not an escape, but the extracted tree gets a link that points at the wrong place and no error is reported. Read one byte past the cap and fail instead.
🛡️ Proposed fix
func putSymlinkFrom(r io.Reader, name string, s *sink) error {
- target, err := io.ReadAll(io.LimitReader(r, maxLinkTargetBytes))
+ target, err := io.ReadAll(io.LimitReader(r, maxLinkTargetBytes+1))
if err != nil {
return errors.Wrapf(err, "failed to read symlink entry %q", name)
}
+ if len(target) > maxLinkTargetBytes {
+ return errors.Errorf("symlink entry %q has a target longer than %d bytes", name, maxLinkTargetBytes)
+ }
return s.putSymlink(name, string(target))
}📝 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.
Suggested change
| func putSymlinkFrom(r io.Reader, name string, s *sink) error { | |
| target, err := io.ReadAll(io.LimitReader(r, maxLinkTargetBytes)) | |
| if err != nil { | |
| return errors.Wrapf(err, "failed to read symlink entry %q", name) | |
| } | |
| return s.putSymlink(name, string(target)) | |
| } | |
| func putSymlinkFrom(r io.Reader, name string, s *sink) error { | |
| target, err := io.ReadAll(io.LimitReader(r, maxLinkTargetBytes+1)) | |
| if err != nil { | |
| return errors.Wrapf(err, "failed to read symlink entry %q", name) | |
| } | |
| if len(target) > maxLinkTargetBytes { | |
| return errors.Errorf("symlink entry %q has a target longer than %d bytes", name, maxLinkTargetBytes) | |
| } | |
| return s.putSymlink(name, string(target)) | |
| } |
🤖 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/app/archive/extract_formats.go` around lines 99 - 106, Update
putSymlinkFrom to detect symlink targets exceeding maxLinkTargetBytes by reading
up to one byte beyond the allowed limit, then return an error when that extra
byte exists instead of calling s.putSymlink. Preserve the existing wrapped
read-error handling and normal symlink creation for targets within the limit.
Comment on lines
+248
to
+251
| rr, err := rardecode.NewReader(archiveFile, rardecode.MaxDictionarySize(maxRarDictBytes)) | ||
| if err != nil { | ||
| return wrapArchiveReadErr(err, "rar") | ||
| } |
There was a problem hiding this comment.
🗄️ Data Integrity & Integration | 🟡 Minor | ⚡ Quick win
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Confirm the rardecode and sevenzip API surface used by extract_formats.go.
set -euo pipefail
echo "== declared versions =="
rg -n 'rardecode|sevenzip' go.mod
moddir="$(go env GOPATH 2>/dev/null || echo "$HOME/go")/pkg/mod"
echo "== rardecode symbols =="
rg -n 'func MaxDictionarySize|ErrArchiveEncrypted|func NewReader' "$moddir"/github.com/nwaples/rardecode* 2>/dev/null || echo "module cache unavailable"
echo "== sevenzip ReadError =="
rg -n -C4 'type ReadError|Encrypted' "$moddir"/github.com/bodgit/sevenzip* 2>/dev/null || echo "module cache unavailable"Repository: gameap/daemon
Length of output: 12044
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== extract_formats.go relevant sections =="
sed -n '220,325p' internal/app/archive/extract_formats.go
echo "== all sevenzip error handling in the file =="
rg -n -C5 'ReadError|Encrypted|errors\.As|ErrArchiveEncrypted|MaxDictionarySize' internal/app/archive/extract_formats.go
echo "== exact dependency declarations =="
rg -n 'github.com/(nwaples/rardecode|bodgit/sevenzip)' go.mod go.sumRepository: gameap/daemon
Length of output: 3950
Match sevenzip.ReadError as a pointer.
sevenzip returns *sevenzip.ReadError, so errors.As(err, &readErr) with var readErr sevenzip.ReadError does not match encrypted read errors. Declare var readErr *sevenzip.ReadError 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 `@internal/app/archive/extract_formats.go` around lines 248 - 251, Update the
error matching logic associated with the rardecode reader initialization to
declare the sevenzip read-error target as a *sevenzip.ReadError pointer before
calling errors.As. Preserve the existing encrypted-read error handling while
ensuring errors returned as pointers are matched.
Comment on lines
+302
to
+313
| // The link itself must stay inside the destination. | ||
| if path.IsAbs(linkTarget) { | ||
| return errors.Errorf("archive entry %q has an absolute symlink target", name) | ||
| } | ||
|
|
||
| resolved, err := s.resolveLinkTarget(path.Dir(target), linkTarget) | ||
| if err != nil { | ||
| return errors.Wrapf(err, "archive entry %q", name) | ||
| } | ||
| if !s.withinDest(resolved) { | ||
| return errors.Errorf("archive entry %q has a symlink target escaping the destination", name) | ||
| } |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Inspect conflict resolution and symlink/dir replacement rules in the extraction sink.
set -euo pipefail
fd -t f 'extract.go' internal/app/archive --exec cat -n {}
echo "== conflict policy handling =="
rg -n -C6 'resolveConflict|ConflictPolicy|Remove(All)?\(' internal/app/archiveRepository: gameap/daemon
Length of output: 32947
🏁 Script executed:
#!/bin/bash
set -euo pipefail
echo "== extraction dispatch and entry ordering =="
rg -n -C8 'extractEntries|putSymlink|Symlink|symlink|ConflictPolicy' internal/app/archive --glob '*.go'
echo "== symlink and conflict tests =="
rg -n -C12 'symlink|Symlink|directory.*symlink|overwrite.*directory|redeclare|entry order' internal/app/archive/*_test.goRepository: gameap/daemon
Length of output: 50369
🏁 Script executed:
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
from itertools import product
import posixpath
DEST = "dst"
NODES = set()
LINKS = {}
def clean(p):
return posixpath.normpath(p)
def within(p):
return p == DEST or p.startswith(DEST + "/")
def resolver(link_dir, target):
cur = link_dir
pending = target.split("/")
hops = 0
while pending:
part, pending = pending[0], pending[1:]
if part in ("", "."):
continue
if part == "..":
if cur == ".":
return None
cur = posixpath.dirname(cur)
continue
nxt = part if cur == "." else posixpath.join(cur, part)
if nxt not in LINKS:
cur = nxt
continue
hops += 1
if hops > 8:
return None
nested = LINKS[nxt]
if nested.startswith("/"):
return None
pending = nested.split("/") + pending
return cur
def mkdir_parent(path):
parent = posixpath.dirname(path)
# The tested candidates use ordinary parents. Model mkdir-all for them.
parts = parent.split("/")
cur = ""
for part in parts:
if not part:
continue
cur = part if not cur else cur + "/" + part
NODES.add(cur)
def add_link(name, target, overwrite=True):
path = DEST + "/" + name
resolved = resolver(posixpath.dirname(path), target)
if resolved is None or not within(resolved):
return False
# resolveConflict(..., false), then overwrite removes any existing node.
if path in NODES or path in LINKS:
if not overwrite:
return False
NODES.discard(path)
LINKS.pop(path, None)
mkdir_parent(path)
LINKS[path] = target
return True
def actual(path):
# Resolve a relative symlink as the kernel does, including symlinked parents.
todo = path.split("/")
cur = []
hops = 0
while todo:
part, todo = todo[0], todo[1:]
if part in ("", "."):
continue
if part == "..":
if cur:
cur.pop()
continue
cur.append(part)
candidate = "/".join(cur)
if candidate in LINKS:
hops += 1
if hops > 40:
return None
target = LINKS[candidate]
cur.pop()
if target.startswith("/"):
cur = []
todo = target.lstrip("/").split("/") + todo
else:
todo = target.split("/") + todo
return "/".join(cur) or "."
# Search short overwrite sequences. A final link is unsafe if its target
# resolves outside dst, even when the link entry itself was accepted.
names = ["a", "b", "c", "x", "l", "a/b", "a/c", "b/c", "c/x", "a/b/c"]
targets = ["a", "b", "c", "x", "../a", "../b", "../c", "../x",
"../../a", "../../b", "../../c", "../../x",
"a/b", "b/c", "c/x", "../a/b", "../b/c", "../c/x",
"a/../b", "b/../c", "c/../a"]
found = []
for seq in product([(n,t) for n in names for t in targets], repeat=3):
NODES = {DEST}
LINKS = {}
accepted = True
for n,t in seq:
if not add_link(n,t):
accepted = False
break
if not accepted:
continue
for path in LINKS:
resolved = actual(path)
if resolved is not None and not within(resolved):
found.append((seq, path, resolved, dict(LINKS)))
break
if found:
break
print("counterexample:", found[0] if found else "none in bounded search")
PYRepository: gameap/daemon
Length of output: 143
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os
import tempfile
with tempfile.TemporaryDirectory() as work:
dst = os.path.join(work, "dst")
os.mkdir(dst)
os.mkdir(os.path.join(dst, "p"))
# This is accepted while p is a directory: dst/p/../outside == dst/outside.
os.symlink("p/../outside", os.path.join(dst, "l"))
# OVERWRITE removes the directory and installs the later symlink.
os.unlink(os.path.join(dst, "p"))
os.symlink(".", os.path.join(dst, "p"))
link = os.path.join(dst, "l")
resolved = os.path.realpath(link)
print("link_target:", os.readlink(link))
print("replacement_target:", os.readlink(os.path.join(dst, "p")))
print("resolved:", resolved)
print("escapes_destination:", not (resolved == dst or resolved.startswith(dst + os.sep)))
assert resolved == os.path.join(work, "outside")
PY
Length of output: 270
🏁 Script executed (no clone):
#!/bin/bash
set -euo pipefail
python3 - <<'PY'
import os
import shutil
import tempfile
with tempfile.TemporaryDirectory() as work:
dst = os.path.join(work, "dst")
os.mkdir(dst)
os.mkdir(os.path.join(dst, "p"))
os.symlink("p/../outside", os.path.join(dst, "l"))
shutil.rmtree(os.path.join(dst, "p"))
os.symlink(".", os.path.join(dst, "p"))
link = os.path.join(dst, "l")
resolved = os.path.realpath(link)
print("link_target:", os.readlink(link))
print("replacement_target:", os.readlink(os.path.join(dst, "p")))
print("resolved:", resolved)
print("escapes_destination:", not (resolved == dst or resolved.startswith(dst + os.sep)))
assert resolved == os.path.join(work, "outside")
PY
Length of output: 234
Preserve symlink confinement after overwrite. With ARCHIVE_CONFLICT_POLICY_OVERWRITE, dst/p/, dst/l -> p/../outside, then dst/p -> . leaves dst/l resolving outside dst. Reject directory-to-symlink replacements or revalidate existing symlink targets after each replacement.
🤖 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/app/archive/extract.go` around lines 302 - 313, Update the overwrite
replacement flow and its surrounding symlink validation to preserve confinement
after directory-to-symlink replacements: reject replacing a directory that
existing symlinks depend on, or revalidate all affected symlink targets after
each replacement. Ensure the final archive state cannot contain a symlink such
as one resolved by resolveLinkTarget and checked by withinDest that escapes the
destination.
et-nik
added a commit
that referenced
this pull request
Aug 1, 2026
* server tasks * xid updates for server task executions * update gameap * command arguments updates and fixes * fix tests * upload assets to s3 * remote repository replacement * remove legacy * review changes * File operations (hash, archive) (#6) * review changes * bump up dependencies
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
New Features
Bug Fixes
Chores