Prevent panic on error whilst getting storage #13164
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
* The `.Use` of storageHandler before setting up the template renderer causes a panic if there is an error to log. * The error passed to `ctx.Error` in that case may contain sensitive information and should not be rendered to the end user. We should instead log the error and render a simple error message. * There is no handling of missing avatars and this needs a 404. Minio errors need to be mapped to standard golang errors such as os.ErrNotExist. * There is no logging when storage is set up. Related go-gitea#13159 Signed-off-by: Andrew Thornton <art27@cantab.net>
zeripath
changed the title
|
I have not marked this a fixing #13159 because I am uncertain as to the root of their errors. It could simply be that the default location for avatars is now different but I think we looked very carefully at that to prevent that. Unfortunately there was no logging so we cannot be sure what the configuration is. This PR now adds some logging. |
GiteaBot
added
the
lgtm/need 2
6543
approved these changes
Oct 15, 2020
GiteaBot
added
lgtm/need 1
Codecov Report
@@ Coverage Diff @@
## master #13164 +/- ##
=======================================
Coverage 42.04% 42.04%
=======================================
Files 681 681
Lines 75145 75173 +28
=======================================
+ Hits 31596 31609 +13
- Misses 38393 38410 +17
+ Partials 5156 5154 -2
Continue to review full report at Codecov.
|
lunny
approved these changes
Oct 16, 2020
GiteaBot
added
lgtm/done
lafriks
approved these changes
Oct 16, 2020
|
Damn damn damn. I got the error conversion incorrect here - this'll teach me for not actually making unit tests! We should be mostly converting to os.PathError and checking that instead of plain os errors. |
|
@zeripath I think, at this point we can invent a new error strukts ourselve :) |
|
I think if we just use errors.Is(err, os.ErrNotExist) then we can keep the convert very simple. |
Closed
zeripath
added a commit
to zeripath/gitea
that referenced
this pull request
Oct 16, 2020
Unfortunately there was a mistake in go-gitea#13164 which fails to handle os.PathError wrapping an os.ErrNotExist Signed-off-by: Andrew Thornton <art27@cantab.net>
ivanvc
added a commit
to ivanvc/gitea
that referenced
this pull request
Oct 16, 2020
…ments-in-pull-request-label-style * origin/master: [skip ci] Updated translations via Crowdin Fix diff skipping lines (go-gitea#13154) Update go-version v1.2.3 -> v1.2.4 (go-gitea#13169) Vendor Update Go Libs (go-gitea#13166) Prevent panics with missing storage (go-gitea#13164) Improve users management through the CLI (go-gitea#6001) (go-gitea#10492) Change order of possible-owner organizations to alphabetical (go-gitea#13160) Slightly simplify the queue settings code to help reduce the risk of problems (go-gitea#12976) [Vendor] Update go-ldap to v3.2.4 (go-gitea#13163) [skip ci] Updated translations via Crowdin Update external-renderers.en-us.md (go-gitea#13165)
Closed
techknowlogick
added a commit
that referenced
this pull request
Oct 18, 2020
Unfortunately there was a mistake in #13164 which fails to handle os.PathError wrapping an os.ErrNotExist Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: techknowlogick <techknowlogick@gitea.io>
techknowlogick
added a commit
to techknowlogick/gitea
that referenced
this pull request
Oct 18, 2020
…ea#13178) Unfortunately there was a mistake in go-gitea#13164 which fails to handle os.PathError wrapping an os.ErrNotExist Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: techknowlogick <techknowlogick@gitea.io>
techknowlogick
added
the
backport/done
|
Backport sent. It is #13193 |
zeripath
added a commit
to zeripath/gitea
that referenced
this pull request
Oct 25, 2020
Signed-off-by: Andrew Thornton <art27@cantab.net>
techknowlogick
pushed a commit
that referenced
this pull request
Oct 26, 2020
* Fix Storage mapping (#13297) Backport #13297 This PR fixes several bugs in setting storage * The default STORAGE_TYPE should be the provided type. * The Storage config should be passed in to NewStorage as a pointer - otherwise the Mappable interface function MapTo will not be found * There was a bug in the MapTo function. Fix #13286 Signed-off-by: Andrew Thornton <art27@cantab.net> * add missing changes from backport #13164 Signed-off-by: Andrew Thornton <art27@cantab.net> Co-authored-by: Lunny Xiao <xiaolunwen@gmail.com>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
.Useof storageHandler before setting up the template renderercauses a panic if there is an error to log.
ctx.Errorin that case may contain sensitiveinformation and should not be rendered to the end user. We should
instead log the error and render a simple error message.
errors need to be mapped to standard golang errors such as
os.ErrNotExist.
Related #13159
Signed-off-by: Andrew Thornton art27@cantab.net