Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

VAULT-32330: fix sanitize path function to account for backslashes #28878

Merged
merged 8 commits into from
Nov 13, 2024
Merged

VAULT-32330: fix sanitize path function to account for backslashes #28878

Show file tree
Hide file tree
Changes from 7 commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
ff68b56
expand the leading prefix check to check for double forward and back …
aslamovamir Nov 11, 2024
2a78b54
improve the logic to be more concise
aslamovamir Nov 12, 2024
917f890
add unit tests
aslamovamir Nov 12, 2024
5db03a3
Merge branch 'main' into aslamovamir-vault-32330
aslamovamir Nov 12, 2024
0aa5c4f
add a changelog
aslamovamir Nov 12, 2024
05f38a0
make it a bug type
aslamovamir Nov 12, 2024
5dc2205
feedback: reconstruct the check to explicitly check for backslash as …
aslamovamir Nov 12, 2024
9805350
Merge branch 'main' into aslamovamir-vault-32330
aslamovamir Nov 13, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 4 additions & 0 deletions changelog/28878.txt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
```release-note:bug
core: Improved an internal helper function that sanitizes paths by adding a check for leading backslashes
in addition to the existing check for leading slashes.
```
10 changes: 8 additions & 2 deletions vault/logical_system.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6168,8 +6168,14 @@
path += "/"
}

for strings.HasPrefix(path, "/") {
path = path[1:]
// Check for the specified prefixes and trim them if present
for strings.HasPrefix(path, "/") || strings.HasPrefix(path, "/\\") {
Dismissed Show dismissed Hide dismissed
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This seems to satisfy the requirements, it may be worth double checking with Mickael or somebody to make sure this is accurate, since the code scanner seems to still be complaining. I'll hook you two up :)

switch {
case strings.HasPrefix(path, "/\\"):
path = path[2:]
case strings.HasPrefix(path, "/"):
Dismissed Show dismissed Hide dismissed
path = path[1:]
}
}

return path
Expand Down
28 changes: 28 additions & 0 deletions vault/logical_system_test.go
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7257,6 +7257,22 @@ func Test_sanitizePath(t *testing.T) {
path: "/mount/path/",
want: "mount/path/",
},
{
path: "//mount/path/",
want: "mount/path/",
},
{
path: "/\\mount/path/",
want: "mount/path/",
},
{
path: "\\mount/path/",
want: "\\mount/path/",
},
{
path: "\\//mount/path/",
want: "\\//mount/path/",
},
{
path: "",
want: "",
Expand All @@ -7269,6 +7285,18 @@ func Test_sanitizePath(t *testing.T) {
path: "///",
want: "",
},
{
path: "\\",
want: "\\/",
},
{
path: "\\/",
want: "\\/",
},
{
path: "/\\",
want: "",
},
}
for _, tc := range testCases {
tc := tc
Expand Down
Loading