diff --git a/commands.go b/commands.go index b569962..362906b 100644 --- a/commands.go +++ b/commands.go @@ -20,8 +20,11 @@ var ( // literally named "git". lsEntryRe = regexp.MustCompile(`^\S+\s+\d+\s+\S+\s+\S+\s+(\d+)\s+(\S+)\s+(\S+)\s+(\S+)\s+(.*)$`) gitDirRe = regexp.MustCompile(`^(.*)\.git$`) - sanRe = regexp.MustCompile(`[,;:\\/='"|?><-]+`) - wsRe = regexp.MustCompile(`\s+`) + // '!' is in here for the server's sake: csh expands history even inside + // single quotes, so a comment like "fix!now" would reach it as something + // else entirely (see remoteRejected). + sanRe = regexp.MustCompile(`[!,;:\\/='"|?><-]+`) + wsRe = regexp.MustCompile(`\s+`) ) // listMarker separates the two sections of the combined listing command, so @@ -58,7 +61,13 @@ func parseDuSizes(lines []string) map[string]int64 { if err != nil { continue } - out[strings.TrimPrefix(strings.TrimSpace(m[2]), "./")] = kb * 1024 + // du echoes the path it was given, and find gives it an absolute one: + // key on the last element, which is what the listing calls the entry + name := strings.TrimSpace(m[2]) + if i := strings.LastIndexByte(name, '/'); i >= 0 { + name = name[i+1:] + } + out[name] = kb * 1024 } return out } @@ -251,14 +260,22 @@ func runCommandDepth(line string, depth int) bool { one, many = "archive", "archives" } pat := strings.ToLower(word(words, 1)) - remote := "/bin/ls -ltr " + shq(path) + remote := "/bin/ls -ltr " + shq(serverPath(path)) if !opt["a"] { // archives are files and carry a real size; repositories are // directories, whose listed size is the inode's, so ask du in the // same round trip. Nothing shell-specific here on purpose: the // login shell may be csh, where "2>/dev/null" is not a redirection // but an argument followed by one. - remote += "; echo " + shq(listMarker) + "; du -sk *.git" + // + // The pattern goes to find, quoted, and never to the shell. A glob + // that matches nothing is a fatal error in a non-interactive zsh — + // "no matches found: *.git" — which aborts the command and turned an + // empty server into "could not list". sh would have handed the + // literal "*.git" to du instead, which is not much better. + remote += "; echo " + shq(listMarker) + + "; find " + shq(serverPath(path)) + " -maxdepth 1 -name " + + shq("*"+suffix) + " -exec du -sk {} +" } lines, err := sshOut(remote) lsLines, duLines := splitAtMarker(lines, listMarker) @@ -284,7 +301,13 @@ func runCommandDepth(line string, depth int) bool { // complain when nothing usable came back at all. if len(entries) == 0 { if err != nil { - errorln("could not list " + many + " on the git server") + what := "could not list " + many + " on the git server" + if opt["a"] { + // by far the likeliest reason, and one the wording used to + // hide behind something that sounded like a dead connection + what += " — is there an 'archive' directory in " + cfg.GitPath + "?" + } + errorln(what) break } what := "no " + many + " on the git server" @@ -325,8 +348,8 @@ func runCommandDepth(line string, depth int) bool { errorln("repository not found") break } - logLines, _ := sshOut("cd " + shq(cfg.GitPath+"/"+prj+".git") + - " && git log --reverse --format='%h %ct %s'") + logLines, _ := sshOut("git --git-dir=" + shq(serverPath(prj+".git")) + + " log --reverse --format='%h %ct %s'") repolog(logLines) case "log": @@ -392,7 +415,7 @@ func runCommandDepth(line string, depth int) bool { if !gitOK(DIR, "push") { break } - sshOK("touch " + shq(cfg.GitPath+"/"+PRJ+".git")) + sshOK("touch " + shq(serverPath(PRJ+".git"))) if targets, _ := cfg.mirrorTargets(); truthy(cfg.Mirror) && len(activeRemotes(targets)) > 0 { handlePushRemote("") // auto-mirror to every active server } @@ -442,13 +465,17 @@ func runCommandDepth(line string, depth int) bool { if comment != "" { name = PRJ + "_" + z + "_" + comment } - if !sshOK("cp -r " + shq(PRJ+".git") + " " + shq("archive/"+name+".git")) { + // the archive directory is mgsh's own convention, so make it rather than + // fail on a server where nobody has created it yet + if !sshOK("mkdir -p " + shq(serverPath("archive")) + + " && cp -r " + shq(serverPath(PRJ+".git")) + " " + shq(serverPath("archive/"+name+".git"))) { break } - if !sshOK("cd archive && tar cvzf " + shq(name+".git.tar.gz") + " " + shq(name+".git")) { + if !sshOK("tar cvzf " + shq(serverPath("archive/"+name+".git.tar.gz")) + + " -C " + shq(serverPath("archive")) + " " + shq(name+".git")) { break } - sshOK("rm -rf " + shq("archive/"+name+".git")) + sshOK("rm -rf " + shq(serverPath("archive/"+name+".git"))) case "init": // create a new repository from the current directory if !requireProject() { @@ -468,11 +495,13 @@ func runCommandDepth(line string, depth int) bool { if exists && !yesno("overwrite existing repository "+PRJ+" on the server?", false) { break } - remote := shq(cfg.GitPath + "/" + PRJ + ".git") + remote := shq(serverPath(PRJ + ".git")) if !sshOK("rm -rf " + remote) { break } - if !sshOK("mkdir " + remote + " && cd " + remote + " && git --bare init") { + // `git init --bare ` makes the directory itself: one command, and + // none of it depends on a `cd` having worked + if !sshOK("git init --bare " + remote) { break } gi := DIR + "/.gitignore" @@ -555,13 +584,14 @@ func runCommandDepth(line string, depth int) bool { break } } else { - if !sshOK("cd archive && tar xvzf " + shq(prj+".git.tar.gz")) { + if !sshOK("tar xvzf " + shq(serverPath("archive/"+prj+".git.tar.gz")) + + " -C " + shq(serverPath("archive"))) { break } if !gitOK(BASE, "clone", URL+"/archive/"+prj+".git") { break } - sshOK("rm -rf " + shq("archive/"+prj+".git")) + sshOK("rm -rf " + shq(serverPath("archive/"+prj+".git"))) } if isDir(BASE + "/" + prj) { PRJ = prj @@ -572,7 +602,7 @@ func runCommandDepth(line string, depth int) bool { if opt["a"] { path = "./archive" } - lines, err := sshOut("/bin/ls " + shq(path)) + lines, err := sshOut("/bin/ls " + shq(serverPath(path))) if err != nil { errorln("could not list repositories on the git server") break diff --git a/completion.go b/completion.go index 9c11557..4df98a9 100644 --- a/completion.go +++ b/completion.go @@ -127,7 +127,7 @@ func fetchServerRepos() { } // a missing ./archive is a permanent, unremarkable state: still cache var archives []string - if lines, err := sshOut("/bin/ls archive"); err == nil { + if lines, err := sshOut("/bin/ls " + shq(serverPath("archive"))); err == nil { for _, ln := range lines { t := strings.TrimSpace(ln) if strings.HasSuffix(t, ".git.tar.gz") { diff --git a/git.go b/git.go index 54371fb..229724f 100644 --- a/git.go +++ b/git.go @@ -1,6 +1,7 @@ package main import ( + "errors" "os" "os/exec" "path/filepath" @@ -127,8 +128,30 @@ var sshExec = func(remote string, capture bool) ([]byte, error) { return out, err } +// errRemoteBang is what a command carrying a '!' comes back with, so a caller +// sees a failure rather than a command that quietly did something else. +var errRemoteBang = errors.New("remote command contains '!'") + +// remoteRejected reports whether a command must not be sent at all, and says +// why. '!' is the one character shq cannot protect: csh expands history *before* +// it looks at quotes, and it does so non-interactively too — `echo 'fix!now'` +// answers "Event not found" on the tcsh server. No spelling survives both csh +// and sh, so the only safe move is not to send one. This is the place every +// remote command passes. +func remoteRejected(remote string) bool { + if !strings.ContainsRune(remote, '!') { + return false + } + errorln("not sending a command with '!' in it — the server's login shell " + + "would expand it instead of passing it on: " + remote) + return true +} + // ssh runs a single remote command over ssh with inherited stdio. func ssh(remote string) error { + if remoteRejected(remote) { + return errRemoteBang + } _, err := sshExec(remote, false) return err } @@ -144,6 +167,9 @@ func sshOK(remote string) bool { // sshOut runs a remote command and returns its stdout split into lines. func sshOut(remote string) ([]string, error) { + if remoteRejected(remote) { + return nil, errRemoteBang + } out, err := sshExec(remote, true) lines := strings.Split(string(out), "\n") for len(lines) > 0 && lines[len(lines)-1] == "" { @@ -152,10 +178,27 @@ func sshOut(remote string) ([]string, error) { return lines, err } +// serverPath anchors a path on the git server at the configured gitpath. +// +// Every remote command has to name its target outright, because the login +// directory of the git user is not necessarily the directory holding the bare +// repositories. With `gituser = git` and `gitpath = /home/git` the two are the +// same place and a bare "." worked by luck; with `gituser = root` and +// `gitpath = /root/mgsh` it lists the home directory, where there is nothing to +// find. +func serverPath(rel string) string { + base := strings.TrimRight(cfg.GitPath, "/") + rel = strings.TrimPrefix(strings.TrimSpace(rel), "./") + if rel == "" || rel == "." { + return base + } + return base + "/" + rel +} + // serverRepoNames lists the bare repositories on the git server, without the // ".git" suffix. func serverRepoNames() ([]string, error) { - lines, err := sshOut("/bin/ls .") + lines, err := sshOut("/bin/ls " + shq(serverPath("."))) if err != nil { return nil, err } @@ -169,10 +212,11 @@ func serverRepoNames() ([]string, error) { } // serverEntryExists reports whether entry is present in the remote directory -// path (relative to the git user's home). The error is returned rather than -// folded into the bool so a failed lookup is never mistaken for "not there". +// path (relative to gitpath, which serverPath resolves). The error is returned +// rather than folded into the bool so a failed lookup is never mistaken for +// "not there". func serverEntryExists(path, entry string) (bool, error) { - lines, err := sshOut("/bin/ls " + shq(path)) + lines, err := sshOut("/bin/ls " + shq(serverPath(path))) if err != nil { return false, err } diff --git a/remotecmd_test.go b/remotecmd_test.go index e829b2c..00cbc11 100644 --- a/remotecmd_test.go +++ b/remotecmd_test.go @@ -101,7 +101,7 @@ func TestInitKeepsServerRepoWhenDeclined(t *testing.T) { if c := findCmd(*sent, "rm -rf"); c != "" { t.Fatalf("init destroyed the server repository after the user declined: %q", c) } - if c := findCmd(*sent, "git --bare init"); c != "" { + if c := findCmd(*sent, "git init --bare"); c != "" { t.Fatalf("init re-created the repository after the user declined: %q", c) } } @@ -147,7 +147,7 @@ func TestInitOnFreshProjectCreatesRepo(t *testing.T) { if findCmd(*sent, "rm -rf "+shq("/home/git/notes.git")) == "" { t.Errorf("init did not clear the target path, sent: %q", *sent) } - if c := findCmd(*sent, "git --bare init"); !strings.Contains(c, shq("/home/git/notes.git")) { + if c := findCmd(*sent, "git init --bare"); !strings.Contains(c, shq("/home/git/notes.git")) { t.Errorf("init did not create the bare repository at the configured path: %q", c) } } @@ -224,25 +224,110 @@ func TestArchiveNamesAndQuotesSnapshot(t *testing.T) { } cp, tar, rm := (*sent)[1], (*sent)[2], (*sent)[3] - // name is __ + // name is __, and every path is spelled + // out from gitpath — the login directory is not necessarily the same place stamp := archiveStamp() name := "notes_" + stamp + "_before_rewrite" - if !strings.HasPrefix(cp, "cp -r "+shq("notes.git")+" ") { + // the archive directory is made first: on a server where nobody created it, + // the copy used to fail with a raw cp error + if !strings.HasPrefix(cp, "mkdir -p "+shq("/home/git/archive")+" && cp -r "+shq("/home/git/notes.git")+" ") { t.Errorf("cp command = %q", cp) } - if !strings.Contains(cp, shq("archive/"+name+".git")) { + if !strings.Contains(cp, shq("/home/git/archive/"+name+".git")) { t.Errorf("cp target = %q, want it to contain %q", cp, name) } - // `cd archive && tar`, not `cd archive;tar`: a failed cd must not let tar - // run in the login directory - if !strings.HasPrefix(tar, "cd archive && tar ") || !strings.Contains(tar, shq(name+".git.tar.gz")) { + // `tar -C `, not `cd && tar`: on the tcsh server `cd` is aliased + // to `cd !*;echo $cwd`, which turns the guard into `cd X; echo && tar` — + // tar then runs in the login directory even when the cd failed, and the + // whole command still reports success + if !strings.HasPrefix(tar, "tar cvzf "+shq("/home/git/archive/"+name+".git.tar.gz")+" -C "+shq("/home/git/archive")+" ") || + !strings.Contains(tar, shq(name+".git")) { t.Errorf("tar command = %q", tar) } - if rm != "rm -rf "+shq("archive/"+name+".git") { + if rm != "rm -rf "+shq("/home/git/archive/"+name+".git") { t.Errorf("cleanup command = %q", rm) } } +// TestServerCommandsAreAnchoredAtGitPath: mgsh used to address the server +// through the login directory, which only worked because `gituser = git` and +// `gitpath = /home/git` happen to be the same place. With `gituser = root` and +// `gitpath = /root/mgsh` every command went to /root instead — `list` came back +// empty and `archive` had nothing to copy. +func TestServerCommandsAreAnchoredAtGitPath(t *testing.T) { + useProject(t, "notes") + cfg.GitPath = "/root/mgsh" // the repositories are NOT in the login directory + sent := fakeServer(t, func(cmd string) (string, error) { + if strings.HasPrefix(cmd, "/bin/ls") { + return "notes.git\n", nil + } + return "", nil + }) + + captureStdout(t, func() { + runCommand("list") + runCommand("list -a") + runCommand("show notes") + runCommand("archive") + }) + + if len(*sent) == 0 { + t.Fatal("no remote commands recorded") + } + for _, c := range *sent { + if !strings.Contains(c, "/root/mgsh") { + t.Errorf("remote command not anchored at gitpath: %q", c) + } + // No `cd` either. On the tcsh server it is aliased to `cd !*;echo $cwd`, + // which splits `cd X && Y` into `cd X; echo $cwd && Y`: Y runs even when + // the cd failed, and the command still exits 0. Every tool mgsh uses can + // be told its directory instead — tar -C, git --git-dir, find . + if strings.HasPrefix(c, "cd ") || strings.Contains(c, " cd ") { + t.Errorf("remote command relies on cd: %q", c) + } + } +} + +// TestRemoteCommandsRefuseExclamationMark: '!' is the one character shq cannot +// protect. csh expands history before it looks at quotes, and does so +// non-interactively too, so `cp -r '/home/git/wei!rd.git' …` arrives as +// something else. Such a command must not be sent at all. +func TestRemoteCommandsRefuseExclamationMark(t *testing.T) { + useProject(t, "wei!rd") + sent := fakeServer(t, func(cmd string) (string, error) { + if strings.HasPrefix(cmd, "/bin/ls") { + return "wei!rd.git\n", nil // the repository is there, so both proceed + } + return "", nil + }) + + out := captureStdout(t, func() { + runCommand("show wei!rd") + runCommand("archive") + }) + + for _, c := range *sent { + if strings.Contains(c, "!") { + t.Errorf("sent a command containing '!': %q", c) + } + } + if c := findCmd(*sent, "--git-dir"); c != "" { + t.Errorf("show sent %q despite the '!' in the name", c) + } + if c := findCmd(*sent, "cp -r"); c != "" { + t.Errorf("archive sent %q despite the '!' in the name", c) + } + if !strings.Contains(out, "not sending") { + t.Errorf("output = %q, want the refusal to say what it did not do", out) + } + + // the other way in is an archive comment, which is why sanitizeComment + // drops the character before it ever becomes part of a name + if got := sanitizeComment("fix!now"); strings.ContainsRune(got, '!') { + t.Errorf("sanitizeComment(%q) = %q, want the '!' gone", "fix!now", got) + } +} + // TestCloneRefusesUnknownRepository: `clone` must not start a git clone for a // repository the server does not list. func TestCloneRefusesUnknownRepository(t *testing.T) { @@ -389,5 +474,15 @@ func TestListSendsNoShellSpecificSyntax(t *testing.T) { if strings.Contains(c, "2>") || strings.Contains(c, "&>") { t.Errorf("remote command uses sh-only redirection: %q", c) } + // A glob is expanded by that same login shell, and a non-interactive + // zsh that finds nothing to match does not pass the pattern on like sh + // does — it fails the command outright ("no matches found: *.git"), + // which is how an empty server came to be reported as unreachable. + for _, idx := range indexesOf(c, "*") { + if idx == 0 || c[idx-1] != '\'' { + t.Errorf("unquoted glob in remote command: %q", c) + break + } + } } } diff --git a/version.txt b/version.txt index 2d8f292..24c232f 100644 --- a/version.txt +++ b/version.txt @@ -1 +1 @@ -4.0.66 +4.0.69