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.

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

[bugfix] Replace ~ with user homedir if $GOPASS_HOMEDIR is not set #2961

Merged
merged 1 commit into from
Oct 7, 2024

Conversation

ghost
Copy link

@ghost ghost commented Oct 2, 2024

Fixes #2916

@AnomalRoil
Copy link
Member

AnomalRoil commented Oct 3, 2024

Mhhh, I think this conflicts with the idea of

// CleanPath resolves common aliases in a path and cleans it as much as possible.
func CleanPath(path string) string {
// Only replace ~ if GOPASS_HOMEDIR is set. In that case we do expect any reference
// to the users homedir to be replaced by the value of GOPASS_HOMEDIR. This is mainly
// for testing and experiments. In all other cases we do want to leave ~ as-is.
if len(path) > 1 && path[:2] == "~/" {
if hd := os.Getenv("GOPASS_HOMEDIR"); hd != "" {
return filepath.Clean(hd + path[2:])
}
}
if p, err := filepath.Abs(path); err == nil && !strings.HasPrefix(path, "~") {
return p
}
return filepath.Clean(path)
}

The idea being that "~" is replaced with $GOPASS_HOMEDIR when it's set instead of $HOME, for testing and dev purposes.
Your change feels like it wouldn't do that for git operations anymore (and I'm surprised our tests are passing since they do use GOPASS_HOMEDIR IIRC).

Furthermore, the fact that in #2916 it's working for the root store but not substores seems to hint at a deeper problem in how we handle substore vs root store paths.

@AnomalRoil
Copy link
Member

I think the deeper problem here is that ~ is a shell meta-character, and so paths starting with ~ have been set by incorrectly quoting "~" while setting the path, thus preventing it from being expanded to $HOME by the shell itself, demo:

$ gopass config mounts.path
/home/anomalroil/.local/share/gopass/stores/root
$ gopass config mounts.path "~/.local/share/gopass/stores/root"
~/.local/share/gopass/stores/root
$ gopass config mounts.path
~/.local/share/gopass/stores/root
$ gopass config mounts.path ~/.local/share/gopass/stores/root
/home/anomalroil/.local/share/gopass/stores/root
$ gopass config mounts.path
/home/anomalroil/.local/share/gopass/stores/root

Note that bash does allow to have quoted ~ in $PATH, but most shells do not.

But I think the initial idea was to be able to share a gopass config across a team, and so supporting ~ makes sense I think.
Just maybe not at the git level, but at a upper level?

@ghost
Copy link
Author

ghost commented Oct 3, 2024

Does that mean I should change CleanPath to replace ~ with $HOME if $GOPASS_HOMEDIR is not set?

@dominikschulz
Copy link
Member

@cal-andrew Not sure if CleanPath covers all instances, but I think it would be a good start. Changing this on the gitfs doesn't make sense as @AnomalRoil pointed out.

@ghost ghost force-pushed the git-tilde-path branch 2 times, most recently from 531d992 to f9d788c Compare October 6, 2024 05:35
@ghost ghost changed the title [BUGFIX] Replace ~ with $HOME in path for git commands [BUGFIX] Replace ~ with $HOME if $GOPASS_HOMEDIR is not set Oct 6, 2024
Does not replace ~ if neither are set.

Fixes #2916

Signed-off-by: Callum Andrew <contact@candrew.net>
@ghost ghost force-pushed the git-tilde-path branch from f9d788c to 8c7d7d8 Compare October 6, 2024 05:52
@ghost ghost changed the title [BUGFIX] Replace ~ with $HOME if $GOPASS_HOMEDIR is not set [bugfix] Replace ~ with user homedir if $GOPASS_HOMEDIR is not set Oct 6, 2024
@ghost
Copy link
Author

ghost commented Oct 6, 2024

The PR now tries to replace ~ in CleanPath, and enables testing this in Windows.

@AnomalRoil AnomalRoil merged commit 9797b87 into gopasspw:master Oct 7, 2024
8 checks passed
@ghost ghost deleted the git-tilde-path branch October 7, 2024 11:16
@ghost
Copy link
Author

ghost commented Oct 7, 2024

Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

config: git operations do not work using ~ in mounts.path
2 participants