-
Notifications
You must be signed in to change notification settings - Fork 357
Consolidate home directory path expansion #2720
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.
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,32 @@ | ||
| package path | ||
|
|
||
| import ( | ||
| "errors" | ||
| "os" | ||
| "path/filepath" | ||
| "strings" | ||
| ) | ||
|
|
||
| // ExpandHomeDir expands a leading home-directory reference in a path. | ||
| // It expands "~", "~/...", and "~\\..." on Windows. Other tilde forms, | ||
| // such as "~user/...", are returned unchanged. | ||
| func ExpandHomeDir(path string) (string, error) { | ||
| if !isHomeDirPath(path) { | ||
| return path, nil | ||
| } | ||
|
|
||
| homeDir, err := os.UserHomeDir() | ||
| if err != nil || homeDir == "" { | ||
| return "", errors.New("failed to get user home directory") | ||
| } | ||
|
|
||
| if path == "~" { | ||
| return filepath.Clean(homeDir), nil | ||
| } | ||
|
|
||
| return filepath.Join(homeDir, path[2:]), nil | ||
| } | ||
|
|
||
| func isHomeDirPath(path string) bool { | ||
| return path == "~" || strings.HasPrefix(path, "~/") || (filepath.Separator == '\\' && strings.HasPrefix(path, `~\`)) | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,89 +1,83 @@ | ||
| package root | ||
| package path | ||
|
|
||
| import ( | ||
| "path/filepath" | ||
| "testing" | ||
|
|
||
| "github.com/stretchr/testify/assert" | ||
| "github.com/stretchr/testify/require" | ||
|
|
||
| "github.com/docker/docker-agent/pkg/paths" | ||
| ) | ||
|
|
||
| func TestExpandTilde(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| homeDir := paths.GetHomeDir() | ||
| require.NotEmpty(t, homeDir, "Home directory should be available for tests") | ||
| func TestExpandHomeDir(t *testing.T) { | ||
| homeDir := t.TempDir() | ||
| t.Setenv("HOME", homeDir) | ||
| t.Setenv("USERPROFILE", homeDir) | ||
|
|
||
| tests := []struct { | ||
| name string | ||
| input string | ||
| expected string | ||
| wantErr bool | ||
| }{ | ||
| { | ||
| name: "expands_tilde_prefix", | ||
| name: "tilde only", | ||
| input: "~", | ||
| expected: homeDir, | ||
| }, | ||
| { | ||
| name: "expands tilde prefix", | ||
| input: "~/session.db", | ||
| expected: filepath.Join(homeDir, "session.db"), | ||
| }, | ||
| { | ||
| name: "expands_tilde_with_nested_path", | ||
| name: "expands tilde with nested path", | ||
| input: "~/.cagent/session.db", | ||
| expected: filepath.Join(homeDir, ".cagent", "session.db"), | ||
| }, | ||
| { | ||
| name: "expands_tilde_with_deep_path", | ||
| name: "expands tilde with deep path", | ||
| input: "~/path/to/some/file.db", | ||
| expected: filepath.Join(homeDir, "path", "to", "some", "file.db"), | ||
| }, | ||
| { | ||
| name: "absolute_path_unchanged", | ||
| name: "absolute path unchanged", | ||
| input: "/absolute/path/session.db", | ||
| expected: "/absolute/path/session.db", | ||
| }, | ||
| { | ||
| name: "relative_path_unchanged", | ||
| name: "relative path unchanged", | ||
| input: "relative/path/session.db", | ||
| expected: "relative/path/session.db", | ||
| }, | ||
| { | ||
| name: "tilde_in_middle_unchanged", | ||
| name: "tilde in middle unchanged", | ||
| input: "/some/~/path/session.db", | ||
| expected: "/some/~/path/session.db", | ||
| }, | ||
| { | ||
| name: "tilde_without_slash_unchanged", | ||
| name: "tilde without separator unchanged", | ||
| input: "~something", | ||
| expected: "~something", | ||
| }, | ||
| { | ||
| name: "just_tilde_slash_expands", | ||
| name: "tilde slash expands", | ||
| input: "~/", | ||
| expected: homeDir, | ||
| }, | ||
| { | ||
| name: "empty_string_unchanged", | ||
| name: "empty string unchanged", | ||
| input: "", | ||
| expected: "", | ||
| }, | ||
| { | ||
| name: "dot_path_unchanged", | ||
| name: "dot path unchanged", | ||
| input: "./session.db", | ||
| expected: "./session.db", | ||
| }, | ||
| } | ||
|
|
||
| for _, tt := range tests { | ||
| t.Run(tt.name, func(t *testing.T) { | ||
| t.Parallel() | ||
|
|
||
| result, err := expandTilde(tt.input) | ||
|
|
||
| if tt.wantErr { | ||
| require.Error(t, err) | ||
| return | ||
| } | ||
| result, err := ExpandHomeDir(tt.input) | ||
|
|
||
| require.NoError(t, err) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [LOW] Error path of The test suite covers all the happy-path expansions, but never exercises the case where Consider adding a test that unsets both env vars and asserts that |
||
| assert.Equal(t, tt.expected, result) | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[LOW] Fragile tilde-detection via identity comparison in
AbsolutePathThe check
if expanded == p && p != "~"detects unsupported tilde forms like~user/fooby relying onExpandHomeDirreturning the input unchanged when it doesn't expand it. This is correct today, but it is brittle: if a future change toExpandHomeDirnormalises or cleans an unrecognised tilde path before returning (e.g. callsfilepath.Clean), the check would silently pass and fall through to path resolution with a literal~in the path.Consider adding an explicit check instead:
This makes the intent clear regardless of what
ExpandHomeDirdoes internally.