Conversation
There was a problem hiding this comment.
Pull request overview
Replaces ad-hoc path joining and the third-party otiai10/copy library with a new in-tree fsutil package and confines all caller-supplied file operations to an os.Root opened at the daemon's work directory. This removes a class of symlink/.. escape vulnerabilities (with explicit regression tests) and eliminates the otiai10/copy dependency.
Changes:
- New
internal/app/fsutilpackage providingRootRelpath normalization and a recursiveCopy/CopyTree/CopyInRootbuilt onos.Root, with unit tests. - Refactor TCP
Fileshandler, gRPC file/transfer handlers, andosownerto thread an*os.Rootthrough every operation; oldResolvePath/pathWithinremoved. - Functional test suites reworked to send work-dir–relative paths, mirror fixtures into the per-suite work dir, and replace
otiai10/copycalls withfsutil.Copy.
Reviewed changes
Copilot reviewed 33 out of 34 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| internal/app/fsutil/path.go, copy.go, *_test.go | New path normalization + recursive copy primitives confined to os.Root |
| internal/app/server/files/files.go, response.go | TCP file handler converted to per-request os.Root, staging via in-tree temp file |
| internal/app/server/server.go, services/runner.go | Plumb workPath into NewServer/files.NewFiles |
| internal/app/grpc/file_handler.go, transfer_handler.go | gRPC handlers use os.Root + fsutil.RootRel; ResolvePath/pathWithin removed |
| internal/app/grpc/file_handler_test.go, file_owner_test.go | Replace path-resolution unit tests with end-to-end symlink-escape regression tests |
| internal/app/osowner/owner.go, owner_unix.go, owner_windows.go, owner_test.go | Add MissingSegmentsInRoot / ApplyToPathInRoot with platform-specific lchownInRoot and tests |
| internal/app/game_server_commands/install_server.go, delete_server_test.go | Swap otiai10/copy for fsutil.Copy |
| test/functional/servertest/suite.go and files/*_test.go | New WorkPath plumbing, fixtures mirrored into work dir, helpers handing out (rel, abs) pairs |
| test/functional/{serverscommand,server_tasks,gdtasks}/* | Swap otiai10/copy for fsutil.Copy in test setup |
| go.mod, go.sum | Drop otiai10/copy and otiai10/mint dependencies |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+393
to
+404
| tmpRel := rel + ".upload_tmp" | ||
| tmpFile, err := root.OpenFile(tmpRel, os.O_CREATE|os.O_TRUNC|os.O_WRONLY, permissions) | ||
| if err != nil { | ||
| logger.Error(ctx, errors.WithMessage(err, "failed to create temp file")) | ||
| return writeError(readWriter, "Failed to create temp file") | ||
| } | ||
| defer func(file *os.File) { | ||
| err = file.Close() | ||
| if err != nil { | ||
| logger.Error(ctx, errors.WithMessage(err, "failed to close temp file")) | ||
| } | ||
| err = os.Remove(file.Name()) | ||
| if err != nil { | ||
| logger.Error(ctx, errors.WithMessage(err, "failed to remove temp file")) | ||
| } | ||
| }(tmpFile) | ||
| defer func() { | ||
| // On the success path the temp file has already been closed and | ||
| // renamed away; both calls then fail harmlessly. | ||
| _ = tmpFile.Close() | ||
| _ = root.Remove(tmpRel) | ||
| }() |
Comment on lines
+227
to
233
| if _, err := root.Stat(srcRel); errors.Is(err, os.ErrNotExist) { | ||
| return writeError(readWriter, fmt.Sprintf("Source \"%s\" not found", message.Source)) | ||
| } | ||
|
|
||
| if _, err := os.Stat(message.Destination); !errors.Is(err, os.ErrNotExist) { | ||
| if _, err := root.Stat(dstRel); !errors.Is(err, os.ErrNotExist) { | ||
| return writeError(readWriter, fmt.Sprintf("Destination \"%s\" already exists", message.Destination)) | ||
| } |
Coverage Report for CI Build 25994728228Coverage increased (+1.1%) to 37.337%Details
Uncovered Changes
Coverage Regressions5 previously-covered lines in 2 files lost coverage.
Coverage Stats
💛 - Coveralls |
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
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.
No description provided.