Fix panic in storageHandler #27446
Merged
Fix panic in storageHandler #27446
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
GiteaBot
added
the
lgtm/need 2
pull-request-size
bot
added
the
size/M
lunny
added
type/bug
type/enhancement
|
The prefix parameter could be removed. |
|
@lunny Do you mean that it could be extracted from request URL? |
|
If it's a |
|
wxiaoguang
approved these changes
Oct 5, 2023
GiteaBot
added
lgtm/need 1
storageHandler() is written as a middleware but is used as an endpoint handler, and thus `next` is actually `nil`, which causes a null pointer dereference when a request URL does not match the pattern (where it calls `next.ServerHTTP()`). Example CURL command to trigger the panic: ``` curl -I "http://yourhost/gitea//avatars/a" ``` Fixes go-gitea#27409
sryze
force-pushed
the
storage-handler-panic
branch
from
4ac8e0f
to
3e3ae4a
Compare
pull-request-size
bot
added
the
size/M
OK, added it back |
pull-request-size
bot
added
size/L
If the prefix parameter is removed, |
lunny
approved these changes
Oct 5, 2023
GiteaBot
added
lgtm/done
lunny
added
the
reviewed/wait-merge
GiteaBot
pushed a commit
to GiteaBot/gitea
that referenced
this pull request
Oct 6, 2023
storageHandler() is written as a middleware but is used as an endpoint handler, and thus `next` is actually `nil`, which causes a null pointer dereference when a request URL does not match the pattern (where it calls `next.ServerHTTP()`). Example CURL command to trigger the panic: ``` curl -I "http://yourhost/gitea//avatars/a" ``` Fixes go-gitea#27409 --- Note: the diff looks big but it's actually a small change - all I did was to remove the outer closure (and one level of indentation) ~and removed the HTTP method and pattern checks as they seem redundant because go-chi already does those checks~. You might want to check "Hide whitespace" when reviewing it. Alternative solution (a bit simpler): append `, misc.DummyOK` to the route declarations that utilize `storageHandler()` - this makes it return an empty response when the URL is invalid. I've tested this one and it works too. Or maybe it would be better to return a 400 error in that case (?)
GiteaBot
pushed a commit
to GiteaBot/gitea
that referenced
this pull request
Oct 6, 2023
storageHandler() is written as a middleware but is used as an endpoint handler, and thus `next` is actually `nil`, which causes a null pointer dereference when a request URL does not match the pattern (where it calls `next.ServerHTTP()`). Example CURL command to trigger the panic: ``` curl -I "http://yourhost/gitea//avatars/a" ``` Fixes go-gitea#27409 --- Note: the diff looks big but it's actually a small change - all I did was to remove the outer closure (and one level of indentation) ~and removed the HTTP method and pattern checks as they seem redundant because go-chi already does those checks~. You might want to check "Hide whitespace" when reviewing it. Alternative solution (a bit simpler): append `, misc.DummyOK` to the route declarations that utilize `storageHandler()` - this makes it return an empty response when the URL is invalid. I've tested this one and it works too. Or maybe it would be better to return a 400 error in that case (?)
GiteaBot
removed
the
reviewed/wait-merge
silverwind
pushed a commit
that referenced
this pull request
Oct 6, 2023
Backport #27446 by @sryze storageHandler() is written as a middleware but is used as an endpoint handler, and thus `next` is actually `nil`, which causes a null pointer dereference when a request URL does not match the pattern (where it calls `next.ServerHTTP()`). Example CURL command to trigger the panic: ``` curl -I "http://yourhost/gitea//avatars/a" ``` Fixes #27409 --- Note: the diff looks big but it's actually a small change - all I did was to remove the outer closure (and one level of indentation) ~and removed the HTTP method and pattern checks as they seem redundant because go-chi already does those checks~. You might want to check "Hide whitespace" when reviewing it. Alternative solution (a bit simpler): append `, misc.DummyOK` to the route declarations that utilize `storageHandler()` - this makes it return an empty response when the URL is invalid. I've tested this one and it works too. Or maybe it would be better to return a 400 error in that case (?) Co-authored-by: Sergey Zolotarev <[email protected]>
silverwind
pushed a commit
that referenced
this pull request
Oct 6, 2023
Backport #27446 by @sryze storageHandler() is written as a middleware but is used as an endpoint handler, and thus `next` is actually `nil`, which causes a null pointer dereference when a request URL does not match the pattern (where it calls `next.ServerHTTP()`). Example CURL command to trigger the panic: ``` curl -I "http://yourhost/gitea//avatars/a" ``` Fixes #27409 --- Note: the diff looks big but it's actually a small change - all I did was to remove the outer closure (and one level of indentation) ~and removed the HTTP method and pattern checks as they seem redundant because go-chi already does those checks~. You might want to check "Hide whitespace" when reviewing it. Alternative solution (a bit simpler): append `, misc.DummyOK` to the route declarations that utilize `storageHandler()` - this makes it return an empty response when the URL is invalid. I've tested this one and it works too. Or maybe it would be better to return a 400 error in that case (?) Co-authored-by: Sergey Zolotarev <[email protected]>
|
I was unable to create a backport for 1.20. @sryze, please send one manually. 🍵 |
GiteaBot
added
the
backport/manual
|
I was unable to create a backport for 1.21. @sryze, please send one manually. 🍵 |
|
I think the bot is trying to backport the same commit twice as it appears to be already in those release branches. |
zjjhot
added a commit
to zjjhot/gitea
that referenced
this pull request
Oct 8, 2023
* giteaoffical/main: (79 commits) Pre-register OAuth application for tea (go-gitea#27509) Fix mermaid flowchart margin issue (go-gitea#27503) add a shortcut to user's profile page to admin user details (go-gitea#27299) Fix actionlint (go-gitea#27513) [skip ci] Updated translations via Crowdin Update JS and PY dependencies (go-gitea#27501) Improve feed icons and feed merge text color (go-gitea#27498) Downgrade `go-co-op/gocron` to v1.31.1 (go-gitea#27511) Enable markdownlint `no-duplicate-header` (go-gitea#27500) bump go-deps (go-gitea#27489) Apply to became a maintainer (go-gitea#27491) change runner for binary [skip ci] Updated translations via Crowdin Remove .exe suffix when cross-compiling on Windows (go-gitea#27448) move re-useable workflow add checkout to disk-clean use hosted runners for nightly actions (go-gitea#27485) Avoid run change title process when the title is same (go-gitea#27467) Fix panic in storageHandler (go-gitea#27446) Rename the default themes to gitea-light, gitea-dark, gitea-auto (go-gitea#27419) ...
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.
storageHandler() is written as a middleware but is used as an endpoint handler, and thus
nextis actuallynil, which causes a null pointer dereference when a request URL does not match the pattern (where it callsnext.ServerHTTP()).Example CURL command to trigger the panic:
Fixes #27409
Note: the diff looks big but it's actually a small change - all I did was to remove the outer closure (and one level of indentation)
and removed the HTTP method and pattern checks as they seem redundant because go-chi already does those checks. You might want to check "Hide whitespace" when reviewing it.Alternative solution (a bit simpler): append
, misc.DummyOKto the route declarations that utilizestorageHandler()- this makes it return an empty response when the URL is invalid. I've tested this one and it works too. Or maybe it would be better to return a 400 error in that case (?)