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

Fix error message sanitiziation #3082

Merged
merged 2 commits into from
Dec 4, 2017
Merged

Conversation

ethantkoenig
Copy link
Member

Fixes two sanitization issues:

  1. Previous usage of the HandleCloneUserCredentials function did not cover the case where a message contained a sensitive URLs multiple times; only the first usage would be sanitized.
  2. In the MigratePost handler, an unsanitized error could reach the handleCreateError(..) function, which itself does not do any sanitization. As a result, unsanitized URL could be printed in logs.

This PR additionally:

@lafriks lafriks added type/refactoring Existing code has been cleaned up. There should be no new functionality. topic/security Something leaks user information or is otherwise vulnerable. Should be fixed! labels Dec 3, 2017
@lafriks lafriks added this to the 1.4.0 milestone Dec 3, 2017
@codecov-io
Copy link

codecov-io commented Dec 3, 2017

Codecov Report

Merging #3082 into master will decrease coverage by 0.01%.
The diff coverage is 0%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #3082      +/-   ##
==========================================
- Coverage   33.26%   33.24%   -0.02%     
==========================================
  Files         272      273       +1     
  Lines       39939    39947       +8     
==========================================
- Hits        13284    13281       -3     
- Misses      24757    24767      +10     
- Partials     1898     1899       +1
Impacted Files Coverage Δ
routers/repo/repo.go 21.81% <0%> (-0.16%) ⬇️
models/repo_mirror.go 3.46% <0%> (+0.25%) ⬆️
modules/util/sanitize.go 0% <0%> (ø)
routers/api/v1/repo/repo.go 32.99% <0%> (-0.09%) ⬇️
modules/process/manager.go 76.81% <0%> (-4.35%) ⬇️

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 5dc37b1...0c10249. Read the comment docs.

@tboerger tboerger added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Dec 3, 2017
@lafriks
Copy link
Member

lafriks commented Dec 3, 2017

LGTM

@tboerger tboerger 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 Dec 3, 2017
@sapk
Copy link
Member

sapk commented Dec 4, 2017

LGTM

@tboerger tboerger 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 Dec 4, 2017
@lafriks
Copy link
Member

lafriks commented Dec 4, 2017

make LG-TM work

@lafriks lafriks merged commit 3c1b1ca into go-gitea:master Dec 4, 2017
@lafriks
Copy link
Member

lafriks commented Dec 4, 2017

@ethantkoenig please backport this also to #3078

ethantkoenig added a commit to ethantkoenig/gitea that referenced this pull request Dec 4, 2017
@lunny lunny added the backport/done All backports for this PR have been created label Dec 7, 2017
lafriks pushed a commit that referenced this pull request Dec 8, 2017
* Sanitize logs for mirror sync

* Fix error message sanitiziation (#3082)
@ethantkoenig ethantkoenig deleted the improve/mirror branch December 16, 2017 04:47
@go-gitea go-gitea locked and limited conversation to collaborators Nov 23, 2020
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/refactoring Existing code has been cleaned up. There should be no new functionality.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants