Improve naming of methods to add arguments to git calls #22592

delvh · 2023-01-24T13:24:20Z

Old name	Intention	Problem	New name
`AddDynamicArguments`	add untrusted arguments	what's the meaning of "dynamic"?	`AddUntrustedArguments`
`AddArguments`	add trusted arguments	from the name, it is unclear if you can/should pass untrusted arguments	`AddTrustedArguments`

With this, we can ensure that all commands use the correct method to prevent security issues:
String literals should be added using AddTrustedArguments, the rest using AddUntrustedArguments.

The only remaining question is what is better: The old name for AddTrustedArguments as it is significantly shorter and hence stands out more in comparison to AddUntrustedArguments, or the new name which clarifies its purpose better?

modules/gitgraph/graph_test.go

yardenshoham

LGTM

wxiaoguang · 2023-01-24T14:39:03Z

modules/git/command.go

+// AddTrustedArguments adds new git argument(s) to the command.
+// Each argument must be safe to be trusted, so only string literals should be passed.
+// User-provided arguments should be passed to AddUntrustedArguments instead.
+func (c *Command) AddTrustedArguments(args ...CmdArg) *Command {


That's not true: so only string literals should be passed.

Why I didn't choose AddTrustedArguments: it could be misleading. For example, there are codes like:

cmd.AddTrustedArguments("-c", CmdArg("user.name="+opts.Committer.Name): is the CommiterName trusted or not?

cmd.AddTrustedArguments(CmdArg("--prefix=" + filepath.Base(strings.TrimSuffix(repo.Path, ".git")) + "/")): is the generated path trusted?

but I think in these calls you're declaring that you believe that "user.name="+opts.Committer.Name is a safe and trustworthy argument that does not need further checking by git.Command.

2 is just my worry about the readability.

Trusted is a very strong word, it could lead to misunderstand more easliy.

And, using AddTrustedArguments could also have other problems (the comment below).

wxiaoguang · 2023-01-24T14:47:51Z

To answer the question in the table:

what's the meaning of "dynamic"?

dynamic means the argument is constructed dynamically.

from the name, it is unclear if you can/should pass untrusted arguments

It's clear that it only accepts literal string or CmdArg.

Even after this PR's change, AddTrustedArguments could be still misleading, and even worse, it's dangerous to accept AddTrustedArguments(CmdArg(userDynamicInput)), then Remote-Code-Execution occurs. The Trusted word in the function name will make reviewers ignore the danger of userDynamicInput

ps: In mind mind at that time, I was trying to name the argument type by CmdArgTrusted, then AddArguments(CmdArgTrusted(userDynamicInput)) could be more clear to reviewers.

Although I didn't take it at that time (due to the long-name), however now, maybe it seems good to have CmdArgTrusted, it's definitely clearer than AddTrustedArguments.

The framework should do its best to avoid low-level mistakes like #22098 (comment) , make the mistakes more noticeable for reviewers.

ps: there are still some things to do to improve the git module, for example: hide the CmdArg, remove the CmdArgCheck which I have marked it as deprecated before.

wxiaoguang · 2023-01-31T08:11:02Z

My plan was

Refactor git command package to improve security and maintainability #22678

Now here it is.

…22678) This PR follows #21535 (and replace #22592) ## Review without space diff https://github.com/go-gitea/gitea/pull/22678/files?diff=split&w=1 ## Purpose of this PR 1. Make git module command completely safe (risky user inputs won't be passed as argument option anymore) 2. Avoid low-level mistakes like #22098 (comment) 3. Remove deprecated and dirty `CmdArgCheck` function, hide the `CmdArg` type 4. Simplify code when using git command ## The main idea of this PR * Move the `git.CmdArg` to the `internal` package, then no other package except `git` could use it. Then developers could never do `AddArguments(git.CmdArg(userInput))` any more. * Introduce `git.ToTrustedCmdArgs`, it's for user-provided and already trusted arguments. It's only used in a few cases, for example: use git arguments from config file, help unit test with some arguments. * Introduce `AddOptionValues` and `AddOptionFormat`, they make code more clear and simple: * Before: `AddArguments("-m").AddDynamicArguments(message)` * After: `AddOptionValues("-m", message)` * - * Before: `AddArguments(git.CmdArg(fmt.Sprintf("--author='%s <%s>'", sig.Name, sig.Email)))` * After: `AddOptionFormat("--author='%s <%s>'", sig.Name, sig.Email)` ## FAQ ### Why these changes were not done in #21535 ? #21535 is mainly a search&replace, it did its best to not change too much logic. Making the framework better needs a lot of changes, so this separate PR is needed as the second step. ### The naming of `AddOptionXxx` According to git's manual, the `--xxx` part is called `option`. ### How can it guarantee that `internal.CmdArg` won't be not misused? Go's specification guarantees that. Trying to access other package's internal package causes compilation error. And, `golangci-lint` also denies the git/internal package. Only the `git/command.go` can use it carefully. ### There is still a `ToTrustedCmdArgs`, will it still allow developers to make mistakes and pass untrusted arguments? Generally speaking, no. Because when using `ToTrustedCmdArgs`, the code will be very complex (see the changes for examples). Then developers and reviewers can know that something might be unreasonable. ### Why there was a `CmdArgCheck` and why it's removed? At the moment of #21535, to reduce unnecessary changes, `CmdArgCheck` was introduced as a hacky patch. Now, almost all code could be written as `cmd := NewCommand(); cmd.AddXxx(...)`, then there is no need for `CmdArgCheck` anymore. ### Why many codes for `signArg == ""` is deleted? Because in the old code, `signArg` could never be empty string, it's either `-S[key-id]` or `--no-gpg-sign`. So the `signArg == ""` is just dead code. --------- Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>

6543 · 2023-02-04T05:27:09Z

replaced by #22678

delvh added 2 commits January 23, 2023 15:04

Rename AddDynamicArguments -> AddUntrustedArguments

bd772ac

Rename AddArguments -> AddTrustedArguments

79af46a

delvh added type/refactoring Existing code has been cleaned up. There should be no new functionality. skip-changelog This PR is irrelevant for the (next) changelog, for example bug fixes for unreleased features. labels Jan 24, 2023

delvh added this to the 1.19.0 milestone Jan 24, 2023

delvh self-assigned this Jan 24, 2023

yardenshoham reviewed Jan 24, 2023

View reviewed changes

modules/gitgraph/graph_test.go Outdated Show resolved Hide resolved

GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 24, 2023

Undo unintended changes to modules/gitgraph/graph_test.go

36bcf5b

zeripath approved these changes Jan 24, 2023

View reviewed changes

GiteaBot added lgtm/need 1 This PR needs approval from one additional maintainer to be merged. and removed lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. labels Jan 24, 2023

This comment was marked as outdated.

Sign in to view

yardenshoham approved these changes Jan 24, 2023

View reviewed changes

GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Jan 24, 2023

wxiaoguang reviewed Jan 24, 2023

View reviewed changes

wxiaoguang mentioned this pull request Jan 31, 2023

Refactor git command package to improve security and maintainability #22678

Merged

6543 closed this Feb 4, 2023

lunny removed this from the 1.19.0 milestone Feb 4, 2023

delvh deleted the maintenance/rename-adddynamicargument branch February 4, 2023 10:49

go-gitea locked and limited conversation to collaborators May 3, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve naming of methods to add arguments to git calls #22592

Improve naming of methods to add arguments to git calls #22592

delvh commented Jan 24, 2023

This comment was marked as outdated.

This comment was marked as outdated.

yardenshoham left a comment

wxiaoguang Jan 24, 2023

zeripath Jan 24, 2023

wxiaoguang Jan 24, 2023

wxiaoguang commented Jan 24, 2023 •

edited

Loading

wxiaoguang commented Jan 31, 2023

6543 commented Feb 4, 2023

Improve naming of methods to add arguments to git calls #22592

Improve naming of methods to add arguments to git calls #22592

Conversation

delvh commented Jan 24, 2023

This comment was marked as outdated.

This comment was marked as outdated.

yardenshoham left a comment

Choose a reason for hiding this comment

wxiaoguang Jan 24, 2023

Choose a reason for hiding this comment

zeripath Jan 24, 2023

Choose a reason for hiding this comment

wxiaoguang Jan 24, 2023

Choose a reason for hiding this comment

wxiaoguang commented Jan 24, 2023 • edited Loading

wxiaoguang commented Jan 31, 2023

6543 commented Feb 4, 2023

wxiaoguang commented Jan 24, 2023 •

edited

Loading