feat: only allow paths to be in working directory#572
Merged
matthiasbruns merged 6 commits intoAug 15, 2025
Merged
Conversation
Contributor
There was a problem hiding this comment.
Pull Request Overview
This PR implements security improvements for file path handling by restricting file access to a working directory and preventing path traversal attacks. The changes add a new function to safely resolve file paths within a specified working directory with default-deny behavior for path escapes.
- Added
GetBlobInWorkingDirectoryfunction with working directory validation - Implemented
ensureWdOrPathhelper function usingos.OpenInRootfor secure path resolution - Added comprehensive test coverage for various path scenarios including valid and invalid cases
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| bindings/go/blob/filesystem/file.go | Adds new functions for working directory-constrained file access |
| bindings/go/blob/filesystem/file_test.go | Adds test coverage for working directory validation scenarios |
Tip: Customize your code reviews with copilot-instructions.md. Create the file or learn how to get started.
6cdf89c to
845e14a
Compare
717c4a6 to
1c817f1
Compare
Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
1c817f1 to
1dd6726
Compare
Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
fabianburth
requested changes
Aug 15, 2025
fabianburth
left a comment
Contributor
There was a problem hiding this comment.
Looks good now!
I'd like to propose two more tiny changes (which are arguably kind of nits), then we are ready to merge.
fabianburth
reviewed
Aug 15, 2025
Co-authored-by: Fabian Burth <fabian.burth@sap.com> Update bindings/go/blob/filesystem/file_test.go Co-authored-by: Fabian Burth <fabian.burth@sap.com> Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
06c8a30 to
c92da96
Compare
Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
fabianburth
reviewed
Aug 15, 2025
fabianburth
left a comment
Contributor
There was a problem hiding this comment.
Please wrap the errors before returning. This makes it way easier to search for the concrete places in code later.
56bf739 to
bd2b842
Compare
Signed-off-by: Matthias Bruns <git@matthiasbruns.com>
bd2b842 to
7cdd838
Compare
fabianburth
approved these changes
Aug 15, 2025
| // If the path is absolute, it checks if the path is valid within the working directory. | ||
| // If the path is relative, it resolves it against the working directory and prevents escaping the working directory. | ||
| // If the path cannot be resolved or is invalid, it returns an error. | ||
| func EnsurePathInWorkingDirectory(path, workingDirectory string) (_ string, err error) { |
Member
There was a problem hiding this comment.
why was this function exported, is it needed elsewhere?? IMO our blob package should deal with blobs and not expose functions on filepaths.
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.
Based on the comment #557 (comment)
What this PR does / why we need it
Resolve file/dir inputs relative to the constructor file by wiring a filesystem working directory through input methods; default-deny path escapes with an explicit opt-out.
Changes (high level):
Which issue(s) this PR fixes
Fixes #565