Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Sanitize and Escape refs in git backend #21464

Merged
merged 12 commits into from
Oct 15, 2022
Merged

Sanitize and Escape refs in git backend #21464

merged 12 commits into from
Oct 15, 2022

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Oct 15, 2022

This PR doesn't require new git version, and can be backported easily.

followup of #21462

@codecov-commenter

This comment was marked as off-topic.

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Oct 15, 2022
@GiteaBot 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 Oct 15, 2022
modules/git/command.go Show resolved Hide resolved
modules/git/command_test.go Show resolved Hide resolved
@GiteaBot 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 Oct 15, 2022
@6543 6543 merged commit d98c5db into go-gitea:main Oct 15, 2022
@wxiaoguang wxiaoguang deleted the fix branch October 15, 2022 11:45
@6543 6543 added the backport/done All backports for this PR have been created label Oct 15, 2022
@6543 6543 added this to the 1.18.0 milestone Oct 15, 2022
6543 added a commit that referenced this pull request Oct 15, 2022
Backport #21464 and #21465

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
@6543 6543 changed the title alternative to PR "improve code quality" Sanitize and Escape refs in git backend Oct 15, 2022
@6543 6543 added the topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! label Oct 15, 2022
@zeripath
Copy link
Contributor

As I said on the previous PR - I think we could have just prefixed any initial - with the branch prefix.

Is the purpose of the panic is to detect any code that isn't checking this stuff properly?

@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Oct 15, 2022

I was thinking so, to make the error more obvious.

Then, the behavior has been changed to return an error, in next PR #21465

@wxiaoguang
Copy link
Contributor Author

As I said on the previous PR - I think we could have just prefixed any initial - with the branch prefix.

I would say this approach is wrong, do not guess or assume anything.

@zeripath
Copy link
Contributor

As I said on the previous PR - I think we could have just prefixed any initial - with the branch prefix.

I would say this approach is wrong, do not guess or assume anything.

if it's well defined what will happen it's not a guess or an assumption.

@wxiaoguang
Copy link
Contributor Author

This PR is just a quick patch for the reported problem. I haven't read all git commands to see that all user-provided input is a branch/commit-id, etc, I highly doubt it would be either.

So for the first step I think it's fine to resolve the problems first (no normal user would submit a name with leading slash), and the AddDynamicArguments could be a general (not perfect) method to be used for some quick fixes.

For next step, IMO every git command caller should take care of the argument escaping, but not by the AddArgument family -- it's too late. The work by AddArgument family do not have the context and could only "guess" what the argument is.

@wxiaoguang
Copy link
Contributor Author

And one more thing, as long as every git command caller can escape the argument correctly, AddDynamicArguments will never report any error.

@6543
Copy link
Member

6543 commented Oct 15, 2022

This fix the issues, improve can follow

@go-gitea go-gitea locked as resolved and limited conversation to collaborators Oct 15, 2022
@wxiaoguang
Copy link
Contributor Author

--> Refactor git command arguments and make all arguments is safe to be used #21535

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
backport/done All backports for this PR have been created lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants