From 95e77b7051fcd2591970d713cb5e1e17de5aa9e2 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 2 Aug 2019 13:07:01 +0100 Subject: [PATCH 1/4] Be more strict with git arguments --- modules/git/commit.go | 2 ++ modules/git/repo.go | 5 ++--- modules/git/repo_branch.go | 2 +- modules/git/repo_commit.go | 18 ++++++++++++------ modules/git/repo_compare.go | 2 +- modules/git/repo_index.go | 2 +- modules/git/repo_tag.go | 6 +++--- modules/git/repo_tree.go | 2 +- modules/repofiles/delete.go | 6 ++++++ modules/repofiles/update.go | 7 +++++++ 10 files changed, 36 insertions(+), 16 deletions(-) diff --git a/modules/git/commit.go b/modules/git/commit.go index c86ece984883b..2f7f53e7ce94e 100644 --- a/modules/git/commit.go +++ b/modules/git/commit.go @@ -169,6 +169,7 @@ func AddChanges(repoPath string, all bool, files ...string) error { if all { cmd.AddArguments("--all") } + cmd.AddArguments("--") _, err := cmd.AddArguments(files...).RunInDir(repoPath) return err } @@ -304,6 +305,7 @@ func (c *Commit) GetFilesChangedSinceCommit(pastCommit string) ([]string, error) } // FileChangedSinceCommit Returns true if the file given has changed since the the past commit +// YOU MUST ENSURE THAT pastCommit is a valid commit ID. func (c *Commit) FileChangedSinceCommit(filename, pastCommit string) (bool, error) { return c.repo.FileChangedBetweenCommits(filename, pastCommit, c.ID.String()) } diff --git a/modules/git/repo.go b/modules/git/repo.go index 8a40fb1b91ba2..28e54a1bbcf59 100644 --- a/modules/git/repo.go +++ b/modules/git/repo.go @@ -187,8 +187,7 @@ func Pull(repoPath string, opts PullRemoteOptions) error { if opts.All { cmd.AddArguments("--all") } else { - cmd.AddArguments(opts.Remote) - cmd.AddArguments(opts.Branch) + cmd.AddArguments("--", opts.Remote, opts.Branch) } if opts.Timeout <= 0 { @@ -213,7 +212,7 @@ func Push(repoPath string, opts PushOptions) error { if opts.Force { cmd.AddArguments("-f") } - cmd.AddArguments(opts.Remote, opts.Branch) + cmd.AddArguments("--", opts.Remote, opts.Branch) _, err := cmd.RunInDirWithEnv(repoPath, opts.Env) return err } diff --git a/modules/git/repo_branch.go b/modules/git/repo_branch.go index 05eba1e30ed66..9209f4a764f4a 100644 --- a/modules/git/repo_branch.go +++ b/modules/git/repo_branch.go @@ -135,7 +135,7 @@ func (repo *Repository) DeleteBranch(name string, opts DeleteBranchOptions) erro cmd.AddArguments("-d") } - cmd.AddArguments(name) + cmd.AddArguments("--", name) _, err := cmd.RunInDir(repo.Path) return err diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index 733991612a1e9..db3b469d78215 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -117,20 +117,25 @@ func (repo *Repository) getCommit(id SHA1) (*Commit, error) { return commit, nil } -// GetCommit returns commit object of by ID string. -func (repo *Repository) GetCommit(commitID string) (*Commit, error) { +// ConvertToSHA1 returns a Hash object from a potential ID string +func (repo *Repository) ConvertToSHA1(commitID string) (SHA1, error) { if len(commitID) != 40 { var err error - actualCommitID, err := NewCommand("rev-parse", commitID).RunInDir(repo.Path) + actualCommitID, err := NewCommand("rev-parse", "--verify", commitID).RunInDir(repo.Path) if err != nil { if strings.Contains(err.Error(), "unknown revision or path") { - return nil, ErrNotExist{commitID, ""} + return SHA1{}, ErrNotExist{commitID, ""} } - return nil, err + return SHA1{}, err } commitID = actualCommitID } - id, err := NewIDFromString(commitID) + return NewIDFromString(commitID) +} + +// GetCommit returns commit object of by ID string. +func (repo *Repository) GetCommit(commitID string) (*Commit, error) { + id, err := repo.ConvertToSHA1(commitID) if err != nil { return nil, err } @@ -243,6 +248,7 @@ func (repo *Repository) getFilesChanged(id1, id2 string) ([]string, error) { } // FileChangedBetweenCommits Returns true if the file changed between commit IDs id1 and id2 +// You must ensure that id1 and id2 are valid commit ids. func (repo *Repository) FileChangedBetweenCommits(filename, id1, id2 string) (bool, error) { stdout, err := NewCommand("diff", "--name-only", "-z", id1, id2, "--", filename).RunInDirBytes(repo.Path) if err != nil { diff --git a/modules/git/repo_compare.go b/modules/git/repo_compare.go index ddc81097208b6..7d66a2dc07f67 100644 --- a/modules/git/repo_compare.go +++ b/modules/git/repo_compare.go @@ -39,7 +39,7 @@ func (repo *Repository) GetMergeBase(tmpRemote string, base, head string) (strin } } - stdout, err := NewCommand("merge-base", base, head).RunInDir(repo.Path) + stdout, err := NewCommand("merge-base", "--", base, head).RunInDir(repo.Path) return strings.TrimSpace(stdout), base, err } diff --git a/modules/git/repo_index.go b/modules/git/repo_index.go index 4d26563a91011..2c351e209fa7c 100644 --- a/modules/git/repo_index.go +++ b/modules/git/repo_index.go @@ -12,7 +12,7 @@ import ( // ReadTreeToIndex reads a treeish to the index func (repo *Repository) ReadTreeToIndex(treeish string) error { if len(treeish) != 40 { - res, err := NewCommand("rev-parse", treeish).RunInDir(repo.Path) + res, err := NewCommand("rev-parse", "--verify", treeish).RunInDir(repo.Path) if err != nil { return err } diff --git a/modules/git/repo_tag.go b/modules/git/repo_tag.go index 6d490af9b5354..20c36bd708381 100644 --- a/modules/git/repo_tag.go +++ b/modules/git/repo_tag.go @@ -29,13 +29,13 @@ func (repo *Repository) IsTagExist(name string) bool { // CreateTag create one tag in the repository func (repo *Repository) CreateTag(name, revision string) error { - _, err := NewCommand("tag", name, revision).RunInDir(repo.Path) + _, err := NewCommand("tag", "--", name, revision).RunInDir(repo.Path) return err } // CreateAnnotatedTag create one annotated tag in the repository func (repo *Repository) CreateAnnotatedTag(name, message, revision string) error { - _, err := NewCommand("tag", "-a", "-m", message, name, revision).RunInDir(repo.Path) + _, err := NewCommand("tag", "-a", "-m", message, "--", name, revision).RunInDir(repo.Path) return err } @@ -153,7 +153,7 @@ func (repo *Repository) GetTagNameBySHA(sha string) (string, error) { // GetTagID returns the object ID for a tag (annotated tags have both an object SHA AND a commit SHA) func (repo *Repository) GetTagID(name string) (string, error) { - stdout, err := NewCommand("show-ref", name).RunInDir(repo.Path) + stdout, err := NewCommand("show-ref", "--", name).RunInDir(repo.Path) if err != nil { return "", err } diff --git a/modules/git/repo_tree.go b/modules/git/repo_tree.go index 4b9e61f264d05..b31e4330cd3cf 100644 --- a/modules/git/repo_tree.go +++ b/modules/git/repo_tree.go @@ -26,7 +26,7 @@ func (repo *Repository) getTree(id SHA1) (*Tree, error) { // GetTree find the tree object in the repository. func (repo *Repository) GetTree(idStr string) (*Tree, error) { if len(idStr) != 40 { - res, err := NewCommand("rev-parse", idStr).RunInDir(repo.Path) + res, err := NewCommand("rev-parse", "--verify", idStr).RunInDir(repo.Path) if err != nil { return nil, err } diff --git a/modules/repofiles/delete.go b/modules/repofiles/delete.go index 3d9b06b1c1612..a8ab277b28c02 100644 --- a/modules/repofiles/delete.go +++ b/modules/repofiles/delete.go @@ -92,6 +92,12 @@ func DeleteRepoFile(repo *models.Repository, doer *models.User, opts *DeleteRepo // Assigned LastCommitID in opts if it hasn't been set if opts.LastCommitID == "" { opts.LastCommitID = commit.ID.String() + } else { + lastCommitID, err := t.gitRepo.ConvertToSHA1(opts.LastCommitID) + if err != nil { + return nil, fmt.Errorf("DeleteRepoFile: Invalid last commit ID: %v", err) + } + opts.LastCommitID = lastCommitID.String() } // Get the files in the index diff --git a/modules/repofiles/update.go b/modules/repofiles/update.go index 21df776060bed..26b5113f15ead 100644 --- a/modules/repofiles/update.go +++ b/modules/repofiles/update.go @@ -190,6 +190,13 @@ func CreateOrUpdateRepoFile(repo *models.Repository, doer *models.User, opts *Up // Assigned LastCommitID in opts if it hasn't been set if opts.LastCommitID == "" { opts.LastCommitID = commit.ID.String() + } else { + lastCommitID, err := t.gitRepo.ConvertToSHA1(opts.LastCommitID) + if err != nil { + return nil, fmt.Errorf("DeleteRepoFile: Invalid last commit ID: %v", err) + } + opts.LastCommitID = lastCommitID.String() + } encoding := "UTF-8" From b905a9fa1770f7fc18335a83fa50dfb237929e90 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 2 Aug 2019 15:31:41 +0100 Subject: [PATCH 2/4] fix-up commit test --- modules/git/repo_commit.go | 3 ++- modules/git/repo_commit_test.go | 2 +- 2 files changed, 3 insertions(+), 2 deletions(-) diff --git a/modules/git/repo_commit.go b/modules/git/repo_commit.go index db3b469d78215..28374298c1483 100644 --- a/modules/git/repo_commit.go +++ b/modules/git/repo_commit.go @@ -123,7 +123,8 @@ func (repo *Repository) ConvertToSHA1(commitID string) (SHA1, error) { var err error actualCommitID, err := NewCommand("rev-parse", "--verify", commitID).RunInDir(repo.Path) if err != nil { - if strings.Contains(err.Error(), "unknown revision or path") { + if strings.Contains(err.Error(), "unknown revision or path") || + strings.Contains(err.Error(), "fatal: Needed a single revision") { return SHA1{}, ErrNotExist{commitID, ""} } return SHA1{}, err diff --git a/modules/git/repo_commit_test.go b/modules/git/repo_commit_test.go index c0fdd1697f41e..6d8ee6453f5a1 100644 --- a/modules/git/repo_commit_test.go +++ b/modules/git/repo_commit_test.go @@ -58,5 +58,5 @@ func TestGetCommitWithBadCommitID(t *testing.T) { commit, err := bareRepo1.GetCommit("bad_branch") assert.Nil(t, commit) assert.Error(t, err) - assert.EqualError(t, err, "object does not exist [id: bad_branch, rel_path: ]") + assert.True(t, IsErrNotExist(err)) } From a500d83769c44826e89a5a6fe87434779c430262 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 2 Aug 2019 16:27:44 +0100 Subject: [PATCH 3/4] Branch name is a path therefore it should be cleaned --- routers/repo/editor.go | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/routers/repo/editor.go b/routers/repo/editor.go index 1c7eee98ac6f8..0c06c99ecb940 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -161,7 +161,8 @@ func NewFile(ctx *context.Context) { func editFilePost(ctx *context.Context, form auth.EditRepoFileForm, isNewFile bool) { canCommit := renderCommitRights(ctx) treeNames, treePaths := getParentTreeFields(form.TreePath) - branchName := ctx.Repo.BranchName + branchName := path.Clean("/" + ctx.Repo.BranchName)[1:] + form.NewBranchName = path.Clean("/" + form.NewBranchName)[1:] if form.CommitChoice == frmCommitChoiceNewBranch { branchName = form.NewBranchName } From f3e574bfe3d29c1db93d9a46268829851c964df6 Mon Sep 17 00:00:00 2001 From: Andrew Thornton Date: Fri, 2 Aug 2019 20:42:00 +0100 Subject: [PATCH 4/4] use bindings --- modules/structs/repo_file.go | 4 ++-- routers/repo/editor.go | 3 +-- 2 files changed, 3 insertions(+), 4 deletions(-) diff --git a/modules/structs/repo_file.go b/modules/structs/repo_file.go index b2eeb7f13ad03..cb836e2e23f61 100644 --- a/modules/structs/repo_file.go +++ b/modules/structs/repo_file.go @@ -10,9 +10,9 @@ type FileOptions struct { // message (optional) for the commit of this file. if not supplied, a default message will be used Message string `json:"message"` // branch (optional) to base this file from. if not given, the default branch is used - BranchName string `json:"branch"` + BranchName string `json:"branch" binding:"GitRefName;MaxSize(100)"` // new_branch (optional) will make a new branch from `branch` before creating the file - NewBranchName string `json:"new_branch"` + NewBranchName string `json:"new_branch" binding:"GitRefName;MaxSize(100)"` // `author` and `committer` are optional (if only one is given, it will be used for the other, otherwise the authenticated user will be used) Author Identity `json:"author"` Committer Identity `json:"committer"` diff --git a/routers/repo/editor.go b/routers/repo/editor.go index 0c06c99ecb940..1c7eee98ac6f8 100644 --- a/routers/repo/editor.go +++ b/routers/repo/editor.go @@ -161,8 +161,7 @@ func NewFile(ctx *context.Context) { func editFilePost(ctx *context.Context, form auth.EditRepoFileForm, isNewFile bool) { canCommit := renderCommitRights(ctx) treeNames, treePaths := getParentTreeFields(form.TreePath) - branchName := path.Clean("/" + ctx.Repo.BranchName)[1:] - form.NewBranchName = path.Clean("/" + form.NewBranchName)[1:] + branchName := ctx.Repo.BranchName if form.CommitChoice == frmCommitChoiceNewBranch { branchName = form.NewBranchName }