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

Ensure valid git author names passed in signatures #5774

Merged

Conversation

zeripath
Copy link
Contributor

Fix #5772 - Git author names are not allowed to include \n < or > and
must not be empty. Ensure that the name passed in a signature is valid.

Signed-off-by: Andrew Thornton [email protected]

@zeripath
Copy link
Contributor Author

This should probably be backported to at least v1.7.

@bkcsoft bkcsoft added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Jan 19, 2019
@codecov-io
Copy link

codecov-io commented Jan 19, 2019

Codecov Report

Merging #5774 into master will increase coverage by 0.01%.
The diff coverage is 93.75%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #5774      +/-   ##
==========================================
+ Coverage   37.88%   37.89%   +0.01%     
==========================================
  Files         328      328              
  Lines       48260    48273      +13     
==========================================
+ Hits        18282    18292      +10     
- Misses      27347    27351       +4     
+ Partials     2631     2630       -1
Impacted Files Coverage Δ
models/user.go 48.74% <93.75%> (+0.55%) ⬆️
models/unit.go 0% <0%> (-14.29%) ⬇️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update cd83c2c...b221f7b. Read the comment docs.

@adelowo
Copy link
Member

adelowo commented Jan 19, 2019

Might need some Tests ????

@bkcsoft bkcsoft 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 19, 2019
Copy link
Member

@techknowlogick techknowlogick left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM. I second what @adelowo days re:tests

models/user.go Outdated Show resolved Hide resolved
Fix go-gitea#5772 - Git author names are not allowed to include `\n` `<` or `>` and
must not be empty. Ensure that the name passed in a signature is valid.

Signed-off-by: Andrew Thornton <[email protected]>
LDAP and the like usernames are not checked in the same way that users who signup are.
Therefore just ensure that user names are also git safe and if totally pathological -
Set them to "user-$UID"

Signed-off-by: Andrew Thornton <[email protected]>
Make our testcases a little more pathological so that we be sure that integration
tests have a chance to spot these cases.

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath force-pushed the issue-5772-escape-full-name-in-git-author branch from 740c1f5 to 1ae279a Compare January 20, 2019 12:08
@zeripath
Copy link
Contributor Author

OK tests are up. I've made the test fixtures for user a little more pathological. We should try to make our test fixtures a bit more pathological in general.

zeripath added a commit to zeripath/gitea that referenced this pull request Jan 20, 2019
@lafriks lafriks added this to the 1.8.0 milestone Jan 20, 2019
@zeripath
Copy link
Contributor Author

Hi @lafriks @lunny any more changes needed?

@lafriks
Copy link
Member

lafriks commented Jan 24, 2019

I'm ok with changes, needs @lunny approval

@bkcsoft bkcsoft 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, 2019
@lafriks lafriks merged commit 44371b9 into go-gitea:master Jan 24, 2019
@zeripath zeripath deleted the issue-5772-escape-full-name-in-git-author branch January 24, 2019 17:02
bmackinney pushed a commit to bmackinney/gitea that referenced this pull request Jan 27, 2019
* Ensure valid git author names passed in signatures

Fix go-gitea#5772 - Git author names are not allowed to include `\n` `<` or `>` and
must not be empty. Ensure that the name passed in a signature is valid.

* Account for pathologically named external users

LDAP and the like usernames are not checked in the same way that users who signup are.
Therefore just ensure that user names are also git safe and if totally pathological -
Set them to "user-$UID"

* Add Tests and adjust test users

Make our testcases a little more pathological so that we be sure that integration
tests have a chance to spot these cases.

Signed-off-by: Andrew Thornton <[email protected]>
@zeripath zeripath mentioned this pull request Jan 30, 2019
6 tasks
lafriks pushed a commit that referenced this pull request Feb 12, 2019
… LFS support (#5702)

* Use git plumbing for upload: #5621 repo_editor.go: UploadRepoFile

* Use git plumbing for upload: #5621 repo_editor.go: GetDiffPreview

* Use git plumbing for upload: #5621 repo_editor.go: DeleteRepoFile

* Use git plumbing for upload: #5621 repo_editor.go: UploadRepoFiles

* Move branch checkout functions out of repo_editor.go as they are no longer used there

* BUGFIX: The default permissions should be 100644

    This is a change from the previous code but is more in keeping
    with the default behaviour of git.

Signed-off-by: Andrew Thornton <[email protected]>

* Standardise cleanUploadFilename to more closely match git

See verify_path in: https://github.com/git/git/blob/7f4e64169352e03476b0ea64e7e2973669e491a2/read-cache.c#L951

Signed-off-by: Andrew Thornton <[email protected]>

* Redirect on bad paths

Signed-off-by: Andrew Thornton <[email protected]>

* Refactor to move the uploading functions out to a module

Signed-off-by: Andrew Thornton <[email protected]>

* Add LFS support

Signed-off-by: Andrew Thornton <[email protected]>

* Update upload.go attribution header

Upload.go is essentially the remnants of repo_editor.go. The remaining code is essentially unchanged from the Gogs code, hence the Gogs attribution.

* Delete upload files after session committed

* Ensure that GIT_AUTHOR_NAME etc. are valid for git

see #5774

Signed-off-by: Andrew Thornton <[email protected]>

* Add in test cases per @lafriks comment

* Add space between gitea and github imports

Signed-off-by: Andrew Thornton <[email protected]>

* more examples in TestCleanUploadName

Signed-off-by: Andrew Thornton <[email protected]>

* fix formatting

Signed-off-by: Andrew Thornton <[email protected]>

* Set the SSH_ORIGINAL_COMMAND to ensure hooks are run

Signed-off-by: Andrew Thornton <[email protected]>

* Switch off SSH_ORIGINAL_COMMAND

Signed-off-by: Andrew Thornton <[email protected]>
@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
lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Internal Server Error when creating a repository in edge case
7 participants