Skip to content

Commit

Permalink
lfshttp: don't strip off initial slash on SSH commands
Browse files Browse the repository at this point in the history 
When we process an SSH URL, we intentionally strip off the slash at the
beginning of the URL.  While that was convenient for
git-lfs-authenticate, it also prevents us from handling an absolute path
in git-lfs-transfer, since the path will have its leading slash stripped
off and will therefore be relative.

Instead, let's adopt Git's behavior, which is to not remove the leading
slash.  This is an incompatible change, but we're about to do a major
release, so it's a good time to make it.  This will affect both
git-lfs-transfer and git-lfs-authenticate commands, but at least GitHub
already supports the proper syntax.

Note that since we process the non-URL form of SSH remotes by converting
them to a URL and then parsing, let's strip off the leading slash when
we process that form, since there we do have the ability to distinguish
between absolute and relative paths.

Update the lfs-ssh-echo binary to handle this new format.
  • Loading branch information
@bk2204
bk2204 committed Jul 20, 2021
1 parent a487e7c commit a0190a6
Show file tree
Hide file tree
Showing 5 changed files with 26 additions and 29 deletions.
18 changes: 9 additions & 9 deletions lfsapi/endpoint_finder_test.go
View file
Expand Up @@ -155,7 +155,7 @@ func TestSSHEndpointAddsLfsSuffix(t *testing.T) {
e := finder.Endpoint("download", "")
assert.Equal(t, "https://example.com/foo/bar.git/info/lfs", e.Url)
assert.Equal(t, "git@example.com", e.SSHMetadata.UserAndHost)
assert.Equal(t, "foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "/foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "", e.SSHMetadata.Port)
}

Expand All @@ -167,7 +167,7 @@ func TestSSHCustomPortEndpointAddsLfsSuffix(t *testing.T) {
e := finder.Endpoint("download", "")
assert.Equal(t, "https://example.com/foo/bar.git/info/lfs", e.Url)
assert.Equal(t, "git@example.com", e.SSHMetadata.UserAndHost)
assert.Equal(t, "foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "/foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "9000", e.SSHMetadata.Port)
}

Expand All @@ -179,7 +179,7 @@ func TestGitSSHEndpointAddsLfsSuffix(t *testing.T) {
e := finder.Endpoint("download", "")
assert.Equal(t, "https://example.com/foo/bar.git/info/lfs", e.Url)
assert.Equal(t, "git@example.com", e.SSHMetadata.UserAndHost)
assert.Equal(t, "foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "/foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "", e.SSHMetadata.Port)
}

Expand All @@ -191,7 +191,7 @@ func TestGitSSHCustomPortEndpointAddsLfsSuffix(t *testing.T) {
e := finder.Endpoint("download", "")
assert.Equal(t, "https://example.com/foo/bar.git/info/lfs", e.Url)
assert.Equal(t, "git@example.com", e.SSHMetadata.UserAndHost)
assert.Equal(t, "foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "/foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "9000", e.SSHMetadata.Port)
}

Expand All @@ -203,7 +203,7 @@ func TestSSHGitEndpointAddsLfsSuffix(t *testing.T) {
e := finder.Endpoint("download", "")
assert.Equal(t, "https://example.com/foo/bar.git/info/lfs", e.Url)
assert.Equal(t, "git@example.com", e.SSHMetadata.UserAndHost)
assert.Equal(t, "foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "/foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "", e.SSHMetadata.Port)
}

Expand All @@ -215,7 +215,7 @@ func TestSSHGitCustomPortEndpointAddsLfsSuffix(t *testing.T) {
e := finder.Endpoint("download", "")
assert.Equal(t, "https://example.com/foo/bar.git/info/lfs", e.Url)
assert.Equal(t, "git@example.com", e.SSHMetadata.UserAndHost)
assert.Equal(t, "foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "/foo/bar", e.SSHMetadata.Path)
assert.Equal(t, "9000", e.SSHMetadata.Port)
}

Expand Down Expand Up @@ -672,7 +672,7 @@ func TestInsteadOf(t *testing.T) {
Url: "https://example.com/git-lfs/git-lfs.git/info/lfs",
SSHMetadata: ssh.SSHMetadata{
UserAndHost: "example.com",
Path: "git-lfs/git-lfs.git",
Path: "/git-lfs/git-lfs.git",
Port: "",
},
Operation: "upload",
Expand All @@ -685,7 +685,7 @@ func TestInsteadOf(t *testing.T) {
Url: "https://example.com/git-lfs/git-lfs.git/info/lfs",
SSHMetadata: ssh.SSHMetadata{
UserAndHost: "example.com",
Path: "git-lfs/git-lfs.git",
Path: "/git-lfs/git-lfs.git",
Port: "",
},
Operation: "download",
Expand All @@ -698,7 +698,7 @@ func TestInsteadOf(t *testing.T) {
Url: "https://example.com/git-lfs/git-lfs.git/info/lfs",
SSHMetadata: ssh.SSHMetadata{
UserAndHost: "example.com",
Path: "git-lfs/git-lfs.git",
Path: "/git-lfs/git-lfs.git",
Port: "",
},
Operation: "upload",
Expand Down
15 changes: 6 additions & 9 deletions lfshttp/endpoint.go
View file
Expand Up @@ -54,14 +54,7 @@ func EndpointFromSshUrl(u *url.URL) Endpoint {
endpoint.SSHMetadata.Port = match[2]
}

// u.Path includes a preceding '/', strip off manually
// rooted paths in the URL will be '//path/to/blah'
// this is just how Go's URL parsing works
if strings.HasPrefix(u.Path, "/") {
endpoint.SSHMetadata.Path = u.Path[1:]
} else {
endpoint.SSHMetadata.Path = u.Path
}
endpoint.SSHMetadata.Path = u.Path

// Fallback URL for using HTTPS while still using SSH for git
// u.Host includes host & port so can't use SSH port
Expand Down Expand Up @@ -98,7 +91,11 @@ func EndpointFromBareSshUrl(rawurl string) Endpoint {
return Endpoint{Url: UrlUnknown}
}

return EndpointFromSshUrl(newu)
endpoint := EndpointFromSshUrl(newu)
if strings.HasPrefix(endpoint.SSHMetadata.Path, "/") {
endpoint.SSHMetadata.Path = endpoint.SSHMetadata.Path[1:]
}
return endpoint
}

// Construct a new endpoint from a HTTP URL
Expand Down
14 changes: 7 additions & 7 deletions ssh/ssh_test.go
View file
Expand Up @@ -439,11 +439,11 @@ func TestSSHGetLFSExeAndArgsWithCustomSSH(t *testing.T) {
t.Logf("ENDPOINT: %+v", e)
assert.Equal(t, "12345", e.SSHMetadata.Port)
assert.Equal(t, "git@host.com", e.SSHMetadata.UserAndHost)
assert.Equal(t, "repo", e.SSHMetadata.Path)
assert.Equal(t, "/repo", e.SSHMetadata.Path)

exe, args := ssh.GetLFSExeAndArgs(cli.OSEnv(), cli.GitEnv(), &e.SSHMetadata, "git-lfs-authenticate", "download", false)
assert.Equal(t, "not-ssh", exe)
assert.Equal(t, []string{"-p", "12345", "git@host.com", "git-lfs-authenticate repo download"}, args)
assert.Equal(t, []string{"-p", "12345", "git@host.com", "git-lfs-authenticate /repo download"}, args)
}

func TestSSHGetLFSExeAndArgsInvalidOptionsAsHost(t *testing.T) {
Expand All @@ -457,11 +457,11 @@ func TestSSHGetLFSExeAndArgsInvalidOptionsAsHost(t *testing.T) {
e := lfshttp.EndpointFromSshUrl(u)
t.Logf("ENDPOINT: %+v", e)
assert.Equal(t, "-oProxyCommand=gnome-calculator", e.SSHMetadata.UserAndHost)
assert.Equal(t, "repo", e.SSHMetadata.Path)
assert.Equal(t, "/repo", e.SSHMetadata.Path)

exe, args := ssh.GetLFSExeAndArgs(cli.OSEnv(), cli.GitEnv(), &e.SSHMetadata, "git-lfs-authenticate", "download", false)
assert.Equal(t, "ssh", exe)
assert.Equal(t, []string{"--", "-oProxyCommand=gnome-calculator", "git-lfs-authenticate repo download"}, args)
assert.Equal(t, []string{"--", "-oProxyCommand=gnome-calculator", "git-lfs-authenticate /repo download"}, args)
}

func TestSSHGetLFSExeAndArgsInvalidOptionsAsHostWithCustomSSH(t *testing.T) {
Expand All @@ -478,11 +478,11 @@ func TestSSHGetLFSExeAndArgsInvalidOptionsAsHostWithCustomSSH(t *testing.T) {
e := lfshttp.EndpointFromSshUrl(u)
t.Logf("ENDPOINT: %+v", e)
assert.Equal(t, "--oProxyCommand=gnome-calculator", e.SSHMetadata.UserAndHost)
assert.Equal(t, "repo", e.SSHMetadata.Path)
assert.Equal(t, "/repo", e.SSHMetadata.Path)

exe, args := ssh.GetLFSExeAndArgs(cli.OSEnv(), cli.GitEnv(), &e.SSHMetadata, "git-lfs-authenticate", "download", false)
assert.Equal(t, "not-ssh", exe)
assert.Equal(t, []string{"oProxyCommand=gnome-calculator", "git-lfs-authenticate repo download"}, args)
assert.Equal(t, []string{"oProxyCommand=gnome-calculator", "git-lfs-authenticate /repo download"}, args)
}

func TestSSHGetExeAndArgsInvalidOptionsAsHost(t *testing.T) {
Expand Down Expand Up @@ -515,7 +515,7 @@ func TestSSHGetExeAndArgsInvalidOptionsAsPath(t *testing.T) {
e := lfshttp.EndpointFromSshUrl(u)
t.Logf("ENDPOINT: %+v", e)
assert.Equal(t, "git@git-host.com", e.SSHMetadata.UserAndHost)
assert.Equal(t, "-oProxyCommand=gnome-calculator", e.SSHMetadata.Path)
assert.Equal(t, "/-oProxyCommand=gnome-calculator", e.SSHMetadata.Path)

exe, args, needShell := ssh.GetExeAndArgs(cli.OSEnv(), cli.GitEnv(), &e.SSHMetadata, false)
assert.Equal(t, "ssh", exe)
Expand Down
6 changes: 3 additions & 3 deletions t/cmd/lfs-ssh-echo.go
View file
Expand Up @@ -100,11 +100,11 @@ func main() {
Href: fmt.Sprintf("http://127.0.0.1:%s/%s.git/info/lfs", os.Args[2], repo),
}
switch repo {
case "ssh-expired-absolute":
case "/ssh-expired-absolute":
r.ExpiresAt = time.Now().Add(-5 * time.Minute)
case "ssh-expired-relative":
case "/ssh-expired-relative":
r.ExpiresIn = -5
case "ssh-expired-both":
case "/ssh-expired-both":
r.ExpiresAt = time.Now().Add(-5 * time.Minute)
r.ExpiresIn = -5
}
Expand Down
2 changes: 1 addition & 1 deletion t/t-locks.sh
View file
Expand Up @@ -72,7 +72,7 @@ begin_test "list a single lock (SSH)"
[ $(wc -l < locks.log) -eq 1 ]
grep "f.dat" locks.log
grep "Git LFS Tests" locks.log
grep "lfs-ssh-echo.*git-lfs-authenticate $reponame download" trace.log
grep "lfs-ssh-echo.*git-lfs-authenticate /$reponame download" trace.log
)
end_test

Expand Down

0 comments on commit a0190a6

Please sign in to comment.