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

git-upload-pack strips leading / only when single-quoted #12471

Closed
1 of 4 tasks
ninewise opened this issue Aug 11, 2020 · 4 comments · Fixed by #12624
Closed
1 of 4 tasks

git-upload-pack strips leading / only when single-quoted #12471

ninewise opened this issue Aug 11, 2020 · 4 comments · Fixed by #12624

Comments

@ninewise
Copy link

ninewise commented Aug 11, 2020

  • Gitea version (or commit ref): v1.12.2
  • Git version: 2.26.2
  • Operating system: Alpine Linux
  • Database (use [x]):
    • PostgreSQL
    • MySQL
    • MSSQL
    • SQLite

Description

Creating this issue for a friend without github.

This line replaces first ocurrence of '/ with ' from parameter. This works for cloning the repository with git over SSH, as the remote command gets executed as git-upload-pack '/user/repo' (strace shows this clearly). In that case, said function rewrites it to git-upload-pack 'user/repo' and later on it gets correctly split in user and repo. Now, there are other implementations of Git which doesn't send the commands in that precise way, but as command git-upload-pack with parameter /user/repo (note no single-quotes). Got is one of such implementations, which can't clone the repo as it fails here.

I think the intended behaviour is to remove any leading / from the parameter, quoted or not. Most correct solution would be using a shell quoting library to normalize the input and then remove the leading /. Alternatively, for a quick fix, removing the replace in parseCmd and changing line 144 to also trim / should do the trick.

@zeripath
Copy link
Contributor

The approach in #12554 would probably help fix this.

@ninewise
Copy link
Author

I don't think that will work. That code only deals with ", which I guess is well-defined in unified diff format. However, this case has to deal with shell quoting. I still believe that it'll be best to do proper shell parsing. If that's too invasive, I think the second-best approach will be teaching Gitea to with both single, double and unquoted parameters.

zeripath added a commit to zeripath/gitea that referenced this issue Aug 27, 2020
Fix go-gitea#12471

Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath added a commit that referenced this issue Aug 28, 2020
Fix #12471

Signed-off-by: Andrew Thornton <art27@cantab.net>
@CL-Jeremy
Copy link
Contributor

CL-Jeremy commented Aug 31, 2020

@zeripath I think this PR broke clone via SSH completely... I actually found myself unable to clone anything via SSH on my own server the same day the PR was merged (which coincidentally had master merged in right on this commit), but thought it was my own mistake in configuration. I've just tried on try.gitea.io and got this:

02:11:41.343266 run-command.c:663       trace: run_command: unset GIT_DIR; ssh git@try.gitea.io 'git-upload-pack '\''CL-Jeremy/test-mirror.git'\'''
fatal: 'CL-Jeremy/test-mirror.git' does not appear to be a git repository
Gitea: Internal error
Failed to execute git command: exit status 128

@CL-Jeremy
Copy link
Contributor

🆗 fixed in #12668

@go-gitea go-gitea locked and limited conversation to collaborators Nov 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants