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

panic when loading home page and 404 error #27409

Closed
sryze opened this issue Oct 3, 2023 · 2 comments · Fixed by #27446
Closed

panic when loading home page and 404 error #27409

sryze opened this issue Oct 3, 2023 · 2 comments · Fixed by #27446
Labels

Comments

@sryze
Copy link
Contributor

sryze commented Oct 3, 2023

Description

Gitea panics when loading home page in /avatars/ route (and therefore the avatar image doesn't load) and when opening the home page, it shows a 404 error.

Gitea Version

1.20.4 and git main branch

Can you reproduce the bug on the Gitea demo site?

No

Log Gist

https://gist.github.com/sryze/68fb927ff8b95121597942a3fa118f3e

Screenshots

No response

Git Version

No response

Operating System

No response

How are you running Gitea?

Behind Apache web server as described at https://docs.gitea.com/next/administration/reverse-proxies#apache-httpd-with-a-sub-path

Database

SQLite

@sryze sryze added the type/bug label Oct 3, 2023
@sryze
Copy link
Contributor Author

sryze commented Oct 3, 2023

It seems that next is nil here:

next.ServeHTTP(w, req)

@sryze
Copy link
Contributor Author

sryze commented Oct 4, 2023

Found the root cause - I had a trailing / after the port in my ProxyPass directive in Apache config, and because of this all requests began with //. It would be nice if the server didn't panic in that case.

To reproduce the panic:

curl -I "http:https://yourhost/gitea//avatars/a"

sryze added a commit to sryze/gitea that referenced this issue Oct 5, 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:https://yourhost/gitea//avatars/a"
```

Fixes go-gitea#27409
lunny pushed a commit that referenced this issue 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:https://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 (?)
GiteaBot pushed a commit to GiteaBot/gitea that referenced this issue 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:https://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 issue 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:https://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 (?)
silverwind pushed a commit that referenced this issue 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:https://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 issue 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:https://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]>
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Nov 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant