Skip to content

Commit

Permalink
Refactor git command arguments and make all arguments to be safe to b…
Browse files Browse the repository at this point in the history 
…e used (#21535)

Follow #21464

Make all git command arguments strictly safe. Most changes are one-to-one replacing, keep all existing logic.
  • Loading branch information
@wxiaoguang
wxiaoguang authored Oct 23, 2022
1 parent 4eeea7b commit dcd9fc7
Show file tree
Hide file tree
Showing 71 changed files with 424 additions and 390 deletions.
13 changes: 7 additions & 6 deletions models/migrations/v128.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -83,17 +83,17 @@ func fixMergeBase(x *xorm.Engine) error {

if !pr.HasMerged {
var err error
pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, "merge-base", "--", pr.BaseBranch, gitRefName).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, "merge-base").AddDashesAndList(pr.BaseBranch, gitRefName).RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
var err2 error
pr.MergeBase, _, err2 = git.NewCommand(git.DefaultContext, "rev-parse", git.BranchPrefix+pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err2 = git.NewCommand(git.DefaultContext, "rev-parse").AddDynamicArguments(git.BranchPrefix + pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
if err2 != nil {
log.Error("Unable to get merge base for PR ID %d, Index %d in %s/%s. Error: %v & %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err, err2)
continue
}
}
} else {
parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1", pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
log.Error("Unable to get parents for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
continue
Expand All @@ -103,10 +103,11 @@ func fixMergeBase(x *xorm.Engine) error {
continue
}

args := append([]string{"merge-base", "--"}, parents[1:]...)
args = append(args, gitRefName)
refs := append([]string{}, parents[1:]...)
refs = append(refs, gitRefName)
cmd := git.NewCommand(git.DefaultContext, "merge-base").AddDashesAndList(refs...)

pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, args...).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
log.Error("Unable to get merge base for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
continue
Expand Down
9 changes: 5 additions & 4 deletions models/migrations/v134.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ func refixMergeBase(x *xorm.Engine) error {

gitRefName := fmt.Sprintf("refs/pull/%d/head", pr.Index)

parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1", pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
parentsString, _, err := git.NewCommand(git.DefaultContext, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
log.Error("Unable to get parents for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
continue
Expand All @@ -91,10 +91,11 @@ func refixMergeBase(x *xorm.Engine) error {
}

// we should recalculate
args := append([]string{"merge-base", "--"}, parents[1:]...)
args = append(args, gitRefName)
refs := append([]string{}, parents[1:]...)
refs = append(refs, gitRefName)
cmd := git.NewCommand(git.DefaultContext, "merge-base").AddDashesAndList(refs...)

pr.MergeBase, _, err = git.NewCommand(git.DefaultContext, args...).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
log.Error("Unable to get merge base for merged PR ID %d, Index %d in %s/%s. Error: %v", pr.ID, pr.Index, baseRepo.OwnerName, baseRepo.Name, err)
continue
Expand Down
4 changes: 2 additions & 2 deletions modules/doctor/heads.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool)
numRepos++
runOpts := &git.RunOpts{Dir: repo.RepoPath()}

_, _, defaultBranchErr := git.NewCommand(ctx, "rev-parse", "--", repo.DefaultBranch).RunStdString(runOpts)
_, _, defaultBranchErr := git.NewCommand(ctx, "rev-parse").AddDashesAndList(repo.DefaultBranch).RunStdString(runOpts)

head, _, headErr := git.NewCommand(ctx, "symbolic-ref", "--short", "HEAD").RunStdString(runOpts)

Expand Down Expand Up @@ -49,7 +49,7 @@ func synchronizeRepoHeads(ctx context.Context, logger log.Logger, autofix bool)
}

// otherwise, let's try fixing HEAD
err := git.NewCommand(ctx, "symbolic-ref", "--", "HEAD", repo.DefaultBranch).Run(runOpts)
err := git.NewCommand(ctx, "symbolic-ref").AddDashesAndList("HEAD", repo.DefaultBranch).Run(runOpts)
if err != nil {
logger.Warn("Failed to fix HEAD for %s/%s: %v", repo.OwnerName, repo.Name, err)
return nil
Expand Down
14 changes: 7 additions & 7 deletions modules/doctor/mergebase.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -44,17 +44,17 @@ func checkPRMergeBase(ctx context.Context, logger log.Logger, autofix bool) erro

if !pr.HasMerged {
var err error
pr.MergeBase, _, err = git.NewCommand(ctx, "merge-base", "--", pr.BaseBranch, pr.GetGitRefName()).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err = git.NewCommand(ctx, "merge-base").AddDashesAndList(pr.BaseBranch, pr.GetGitRefName()).RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
var err2 error
pr.MergeBase, _, err2 = git.NewCommand(ctx, "rev-parse", git.BranchPrefix+pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
pr.MergeBase, _, err2 = git.NewCommand(ctx, "rev-parse").AddDynamicArguments(git.BranchPrefix + pr.BaseBranch).RunStdString(&git.RunOpts{Dir: repoPath})
if err2 != nil {
logger.Warn("Unable to get merge base for PR ID %d, #%d onto %s in %s/%s. Error: %v & %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err, err2)
return nil
}
}
} else {
parentsString, _, err := git.NewCommand(ctx, "rev-list", "--parents", "-n", "1", pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
parentsString, _, err := git.NewCommand(ctx, "rev-list", "--parents", "-n", "1").AddDynamicArguments(pr.MergedCommitID).RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
logger.Warn("Unable to get parents for merged PR ID %d, #%d onto %s in %s/%s. Error: %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err)
return nil
Expand All @@ -64,10 +64,10 @@ func checkPRMergeBase(ctx context.Context, logger log.Logger, autofix bool) erro
return nil
}

args := append([]string{"merge-base", "--"}, parents[1:]...)
args = append(args, pr.GetGitRefName())

pr.MergeBase, _, err = git.NewCommand(ctx, args...).RunStdString(&git.RunOpts{Dir: repoPath})
refs := append([]string{}, parents[1:]...)
refs = append(refs, pr.GetGitRefName())
cmd := git.NewCommand(ctx, "merge-base").AddDashesAndList(refs...)
pr.MergeBase, _, err = cmd.RunStdString(&git.RunOpts{Dir: repoPath})
if err != nil {
logger.Warn("Unable to get merge base for merged PR ID %d, #%d onto %s in %s/%s. Error: %v", pr.ID, pr.Index, pr.BaseBranch, pr.BaseRepo.OwnerName, pr.BaseRepo.Name, err)
return nil
Expand Down
63 changes: 48 additions & 15 deletions modules/git/command.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ import (

var (
// globalCommandArgs global command args for external package setting
globalCommandArgs []string
globalCommandArgs []CmdArg

// defaultCommandExecutionTimeout default command execution timeout duration
defaultCommandExecutionTimeout = 360 * time.Second
Expand All @@ -43,6 +43,8 @@ type Command struct {
brokenArgs []string
}

type CmdArg string

func (c *Command) String() string {
if len(c.args) == 0 {
return c.name
Expand All @@ -52,30 +54,39 @@ func (c *Command) String() string {

// NewCommand creates and returns a new Git Command based on given command and arguments.
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommand(ctx context.Context, args ...string) *Command {
func NewCommand(ctx context.Context, args ...CmdArg) *Command {
// Make an explicit copy of globalCommandArgs, otherwise append might overwrite it
cargs := make([]string, len(globalCommandArgs))
copy(cargs, globalCommandArgs)
cargs := make([]string, 0, len(globalCommandArgs)+len(args))
for _, arg := range globalCommandArgs {
cargs = append(cargs, string(arg))
}
for _, arg := range args {
cargs = append(cargs, string(arg))
}
return &Command{
name: GitExecutable,
args: append(cargs, args...),
args: cargs,
parentContext: ctx,
globalArgsLength: len(globalCommandArgs),
}
}

// NewCommandNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommandNoGlobals(args ...string) *Command {
func NewCommandNoGlobals(args ...CmdArg) *Command {
return NewCommandContextNoGlobals(DefaultContext, args...)
}

// NewCommandContextNoGlobals creates and returns a new Git Command based on given command and arguments only with the specify args and don't care global command args
// Each argument should be safe to be trusted. User-provided arguments should be passed to AddDynamicArguments instead.
func NewCommandContextNoGlobals(ctx context.Context, args ...string) *Command {
func NewCommandContextNoGlobals(ctx context.Context, args ...CmdArg) *Command {
cargs := make([]string, 0, len(args))
for _, arg := range args {
cargs = append(cargs, string(arg))
}
return &Command{
name: GitExecutable,
args: args,
args: cargs,
parentContext: ctx,
}
}
Expand All @@ -93,10 +104,12 @@ func (c *Command) SetDescription(desc string) *Command {
return c
}

// AddArguments adds new argument(s) to the command. Each argument must be safe to be trusted.
// AddArguments adds new git argument(s) to the command. Each argument must be safe to be trusted.
// User-provided arguments should be passed to AddDynamicArguments instead.
func (c *Command) AddArguments(args ...string) *Command {
c.args = append(c.args, args...)
func (c *Command) AddArguments(args ...CmdArg) *Command {
for _, arg := range args {
c.args = append(c.args, string(arg))
}
return c
}

Expand All @@ -115,6 +128,26 @@ func (c *Command) AddDynamicArguments(args ...string) *Command {
return c
}

// AddDashesAndList adds the "--" and then add the list as arguments, it's usually for adding file list
// At the moment, this function can be only called once, maybe in future it can be refactored to support multiple calls (if necessary)
func (c *Command) AddDashesAndList(list ...string) *Command {
c.args = append(c.args, "--")
// Some old code also checks `arg != ""`, IMO it's not necessary.
// If the check is needed, the list should be prepared before the call to this function
c.args = append(c.args, list...)
return c
}

// CmdArgCheck checks whether the string is safe to be used as a dynamic argument.
// It panics if the check fails. Usually it should not be used, it's just for refactoring purpose
// deprecated
func CmdArgCheck(s string) CmdArg {
if s != "" && s[0] == '-' {
panic("invalid git cmd argument: " + s)
}
return CmdArg(s)
}

// RunOpts represents parameters to run the command. If UseContextTimeout is specified, then Timeout is ignored.
type RunOpts struct {
Env []string
Expand Down Expand Up @@ -153,7 +186,7 @@ func CommonGitCmdEnvs() []string {
}...)
}

// CommonCmdServEnvs is like CommonGitCmdEnvs but it only returns minimal required environment variables for the "gitea serv" command
// CommonCmdServEnvs is like CommonGitCmdEnvs, but it only returns minimal required environment variables for the "gitea serv" command
func CommonCmdServEnvs() []string {
return commonBaseEnvs()
}
Expand Down Expand Up @@ -318,12 +351,12 @@ func (c *Command) RunStdBytes(opts *RunOpts) (stdout, stderr []byte, runErr RunS
}

// AllowLFSFiltersArgs return globalCommandArgs with lfs filter, it should only be used for tests
func AllowLFSFiltersArgs() []string {
func AllowLFSFiltersArgs() []CmdArg {
// Now here we should explicitly allow lfs filters to run
filteredLFSGlobalArgs := make([]string, len(globalCommandArgs))
filteredLFSGlobalArgs := make([]CmdArg, len(globalCommandArgs))
j := 0
for _, arg := range globalCommandArgs {
if strings.Contains(arg, "lfs") {
if strings.Contains(string(arg), "lfs") {
j--
} else {
filteredLFSGlobalArgs[j] = arg
Expand Down
50 changes: 21 additions & 29 deletions modules/git/commit.go
View file
Original file line number Diff line number Diff line change
Expand Up @@ -92,13 +92,13 @@ func AddChanges(repoPath string, all bool, files ...string) error {
}

// AddChangesWithArgs marks local changes to be ready for commit.
func AddChangesWithArgs(repoPath string, globalArgs []string, all bool, files ...string) error {
func AddChangesWithArgs(repoPath string, globalArgs []CmdArg, all bool, files ...string) error {
cmd := NewCommandNoGlobals(append(globalArgs, "add")...)
if all {
cmd.AddArguments("--all")
}
cmd.AddArguments("--")
_, _, err := cmd.AddArguments(files...).RunStdString(&RunOpts{Dir: repoPath})
cmd.AddDashesAndList(files...)
_, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath})
return err
}

Expand All @@ -112,27 +112,27 @@ type CommitChangesOptions struct {
// CommitChanges commits local changes with given committer, author and message.
// If author is nil, it will be the same as committer.
func CommitChanges(repoPath string, opts CommitChangesOptions) error {
cargs := make([]string, len(globalCommandArgs))
cargs := make([]CmdArg, len(globalCommandArgs))
copy(cargs, globalCommandArgs)
return CommitChangesWithArgs(repoPath, cargs, opts)
}

// CommitChangesWithArgs commits local changes with given committer, author and message.
// If author is nil, it will be the same as committer.
func CommitChangesWithArgs(repoPath string, args []string, opts CommitChangesOptions) error {
func CommitChangesWithArgs(repoPath string, args []CmdArg, opts CommitChangesOptions) error {
cmd := NewCommandNoGlobals(args...)
if opts.Committer != nil {
cmd.AddArguments("-c", "user.name="+opts.Committer.Name, "-c", "user.email="+opts.Committer.Email)
cmd.AddArguments("-c", CmdArg("user.name="+opts.Committer.Name), "-c", CmdArg("user.email="+opts.Committer.Email))
}
cmd.AddArguments("commit")

if opts.Author == nil {
opts.Author = opts.Committer
}
if opts.Author != nil {
cmd.AddArguments(fmt.Sprintf("--author='%s <%s>'", opts.Author.Name, opts.Author.Email))
cmd.AddArguments(CmdArg(fmt.Sprintf("--author='%s <%s>'", opts.Author.Name, opts.Author.Email)))
}
cmd.AddArguments("-m", opts.Message)
cmd.AddArguments("-m").AddDynamicArguments(opts.Message)

_, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath})
// No stderr but exit status 1 means nothing to commit.
Expand All @@ -144,15 +144,13 @@ func CommitChangesWithArgs(repoPath string, args []string, opts CommitChangesOpt

// AllCommitsCount returns count of all commits in repository
func AllCommitsCount(ctx context.Context, repoPath string, hidePRRefs bool, files ...string) (int64, error) {
args := []string{"--all", "--count"}
cmd := NewCommand(ctx, "rev-list")
if hidePRRefs {
args = append([]string{"--exclude=" + PullPrefix + "*"}, args...)
cmd.AddArguments("--exclude=" + PullPrefix + "*")
}
cmd := NewCommand(ctx, "rev-list")
cmd.AddArguments(args...)
cmd.AddArguments("--all", "--count")
if len(files) > 0 {
cmd.AddArguments("--")
cmd.AddArguments(files...)
cmd.AddDashesAndList(files...)
}

stdout, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath})
Expand All @@ -168,8 +166,7 @@ func CommitsCountFiles(ctx context.Context, repoPath string, revision, relpath [
cmd := NewCommand(ctx, "rev-list", "--count")
cmd.AddDynamicArguments(revision...)
if len(relpath) > 0 {
cmd.AddArguments("--")
cmd.AddArguments(relpath...)
cmd.AddDashesAndList(relpath...)
}

stdout, _, err := cmd.RunStdString(&RunOpts{Dir: repoPath})
Expand Down Expand Up @@ -209,7 +206,7 @@ func (c *Commit) HasPreviousCommit(commitHash SHA1) (bool, error) {
return false, nil
}

_, _, err := NewCommand(c.repo.Ctx, "merge-base", "--is-ancestor", that, this).RunStdString(&RunOpts{Dir: c.repo.Path})
_, _, err := NewCommand(c.repo.Ctx, "merge-base", "--is-ancestor").AddDynamicArguments(that, this).RunStdString(&RunOpts{Dir: c.repo.Path})
if err == nil {
return true, nil
}
Expand Down Expand Up @@ -392,15 +389,12 @@ func (c *Commit) GetSubModule(entryname string) (*SubModule, error) {

// GetBranchName gets the closest branch name (as returned by 'git name-rev --name-only')
func (c *Commit) GetBranchName() (string, error) {
args := []string{
"name-rev",
}
cmd := NewCommand(c.repo.Ctx, "name-rev")
if CheckGitVersionAtLeast("2.13.0") == nil {
args = append(args, "--exclude", "refs/tags/*")
cmd.AddArguments("--exclude", "refs/tags/*")
}
args = append(args, "--name-only", "--no-undefined", c.ID.String())

data, _, err := NewCommand(c.repo.Ctx, args...).RunStdString(&RunOpts{Dir: c.repo.Path})
cmd.AddArguments("--name-only", "--no-undefined").AddDynamicArguments(c.ID.String())
data, _, err := cmd.RunStdString(&RunOpts{Dir: c.repo.Path})
if err != nil {
// handle special case where git can not describe commit
if strings.Contains(err.Error(), "cannot describe") {
Expand All @@ -426,7 +420,7 @@ func (c *Commit) LoadBranchName() (err error) {

// GetTagName gets the current tag name for given commit
func (c *Commit) GetTagName() (string, error) {
data, _, err := NewCommand(c.repo.Ctx, "describe", "--exact-match", "--tags", "--always", c.ID.String()).RunStdString(&RunOpts{Dir: c.repo.Path})
data, _, err := NewCommand(c.repo.Ctx, "describe", "--exact-match", "--tags", "--always").AddDynamicArguments(c.ID.String()).RunStdString(&RunOpts{Dir: c.repo.Path})
if err != nil {
// handle special case where there is no tag for this commit
if strings.Contains(err.Error(), "no tag exactly matches") {
Expand Down Expand Up @@ -503,9 +497,7 @@ func GetCommitFileStatus(ctx context.Context, repoPath, commitID string) (*Commi
}()

stderr := new(bytes.Buffer)
args := []string{"log", "--name-status", "-c", "--pretty=format:", "--parents", "--no-renames", "-z", "-1", commitID}

err := NewCommand(ctx, args...).Run(&RunOpts{
err := NewCommand(ctx, "log", "--name-status", "-c", "--pretty=format:", "--parents", "--no-renames", "-z", "-1").AddDynamicArguments(commitID).Run(&RunOpts{
Dir: repoPath,
Stdout: w,
Stderr: stderr,
Expand All @@ -521,7 +513,7 @@ func GetCommitFileStatus(ctx context.Context, repoPath, commitID string) (*Commi

// GetFullCommitID returns full length (40) of commit ID by given short SHA in a repository.
func GetFullCommitID(ctx context.Context, repoPath, shortID string) (string, error) {
commitID, _, err := NewCommand(ctx, "rev-parse", shortID).RunStdString(&RunOpts{Dir: repoPath})
commitID, _, err := NewCommand(ctx, "rev-parse").AddDynamicArguments(shortID).RunStdString(&RunOpts{Dir: repoPath})
if err != nil {
if strings.Contains(err.Error(), "exit status 128") {
return "", ErrNotExist{shortID, ""}
Expand Down
Loading

0 comments on commit dcd9fc7

Please sign in to comment.