Make ssh commands used in the git smart transport compatible with libgit2 #852

Merged
darkowlzz merged 2 commits from ssh-go-transport-fix into main 2021-11-07 14:20:57 -06:00
1 changed files with 10 additions and 2 deletions

12
ssh.go
View File

@ -17,6 +17,7 @@ import (
"net"
"net/url"
"runtime"
"strings"
"unsafe"
"golang.org/x/crypto/ssh"
@ -74,6 +75,13 @@ func (t *sshSmartSubtransport) Action(urlString string, action SmartServiceActio
return nil, err
}
// Escape \ and '.
uPath := strings.Replace(u.Path, `\`, `\\`, -1)
uPath = strings.Replace(uPath, `'`, `\'`, -1)
// TODO: Add percentage decode similar to libgit2.
lhchavez commented 2021-11-07 12:37:32 -06:00 (Migrated from github.com)
Review

can \ and ' be escaped? otherwise one of these characters can be added to the path and cause some unwanted shenanigans on the remote host.

also, the link that was shared made me realize that we are currently missing the %-decoding that's done in libgit2! 358a60e1b4/src/str.c (L455-L481) since this does not regress on that regard, it's fine if that is not fixed in this same commit (to keep its scope small), but would it be possible to add a small TODO for us to fix it later?

can `\` and `'` be escaped? otherwise one of these characters can be added to the path and cause some unwanted shenanigans on the remote host. also, the link that was shared made me realize that we are currently missing the %-decoding that's done in libgit2! https://github.com/libgit2/libgit2/blob/358a60e1b46000ea99ef10b4dd709e92f75ff74b/src/str.c#L455-L481 since this does not regress on that regard, it's fine if that is not fixed in this same commit (to keep its scope small), _but_ would it be possible to add a small `TODO` for us to fix it later?
darkowlzz commented 2021-11-07 13:51:35 -06:00 (Migrated from github.com)
Review

Thanks for pointing that out.
I've tried escaping them. Please correct me if it's not right.
And also, if the TODO message is reasonable.

Thanks for pointing that out. I've tried escaping them. Please correct me if it's not right. And also, if the TODO message is reasonable.
// Refer: https://github.com/libgit2/libgit2/blob/358a60e1b46000ea99ef10b4dd709e92f75ff74b/src/str.c#L455-L481
var cmd string
switch action {
case SmartServiceActionUploadpackLs, SmartServiceActionUploadpack:
@ -83,7 +91,7 @@ func (t *sshSmartSubtransport) Action(urlString string, action SmartServiceActio
}
t.Close()
}
cmd = fmt.Sprintf("git-upload-pack %q", u.Path)
cmd = fmt.Sprintf("git-upload-pack '%s'", uPath)
case SmartServiceActionReceivepackLs, SmartServiceActionReceivepack:
if t.currentStream != nil {
@ -92,7 +100,7 @@ func (t *sshSmartSubtransport) Action(urlString string, action SmartServiceActio
}
t.Close()
}
cmd = fmt.Sprintf("git-receive-pack %q", u.Path)
cmd = fmt.Sprintf("git-receive-pack '%s'", uPath)
default:
return nil, fmt.Errorf("unexpected action: %v", action)