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 HEAD method for robots.txt #30603

Merged
merged 1 commit into from
Apr 19, 2024
Merged

Conversation

wxiaoguang
Copy link
Contributor

@wxiaoguang wxiaoguang commented Apr 19, 2024

Fix #30601

~$ curl --head localhost:3000/robots.txt
HTTP/1.1 200 OK
Accept-Ranges: bytes
Cache-Control: max-age=0, private, must-revalidate
Content-Length: 5
Content-Type: text/plain; charset=utf-8
Last-Modified: Wed, 19 Jul 2023 04:56:12 GMT
X-Gitea-Debug: RUN_MODE=dev
Date: Fri, 19 Apr 2024 12:59:44 GMT

@GiteaBot GiteaBot added the lgtm/need 2 This PR needs two approvals by maintainers to be considered for merging. label Apr 19, 2024
@pull-request-size pull-request-size bot added the size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. label Apr 19, 2024
@github-actions github-actions bot added the modifies/go Pull requests that update Go code label Apr 19, 2024
@wxiaoguang wxiaoguang added the backport/v1.21 This PR should be backported to Gitea 1.21 label Apr 19, 2024
@GiteaBot GiteaBot 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 Apr 19, 2024
@silverwind silverwind added the backport/v1.22 This PR should be backported to Gitea 1.22 label Apr 19, 2024
Copy link
Member

@silverwind silverwind left a comment

Choose a reason for hiding this comment

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

HEAD requests by definition should not have a body, e.g. no Content-Length or Content-Length: 0 only. As per MDN:

Warning: A response to a HEAD method should not have a body. If it has one anyway, that body must be ignored: any representation headers that might describe the erroneous body are instead assumed to describe the response which a similar GET request would have received.

@GiteaBot GiteaBot added lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged and removed lgtm/need 1 This PR needs approval from one additional maintainer to be merged. labels Apr 19, 2024
@wxiaoguang
Copy link
Contributor Author

wxiaoguang commented Apr 19, 2024

HEAD requests by definition must not have a body, e.g. no Content-Length or Content-Length: 0 only.

Are you sure? Would you like to learn it first before putting a change request?

https://developer.mozilla.org/en-US/docs/Web/HTTP/Methods/HEAD

image

image

@silverwind
Copy link
Member

Ah, Content-Length is fine, but it does not send any actual content, right?

@wxiaoguang wxiaoguang enabled auto-merge (squash) April 19, 2024 13:43
@GiteaBot GiteaBot added lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. and removed lgtm/blocked A maintainer has reservations with the PR and thus it cannot be merged labels Apr 19, 2024
@wxiaoguang wxiaoguang merged commit f60e1a1 into go-gitea:main Apr 19, 2024
26 checks passed
@GiteaBot GiteaBot added this to the 1.23.0 milestone Apr 19, 2024
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 19, 2024
@wxiaoguang wxiaoguang deleted the fix-robots-head branch April 19, 2024 13:44
GiteaBot pushed a commit to GiteaBot/gitea that referenced this pull request Apr 19, 2024
@silverwind
Copy link
Member

@wxiaoguang why are you dismissing without answering my question? That's rude.

@wxiaoguang
Copy link
Contributor Author

Ah, Content-Length is fine, but it does not send any actual content, right?

It calls Golang's standard HTTP library, Golang already handles it correctly.

image

@wxiaoguang
Copy link
Contributor Author

@wxiaoguang why are you dismissing without answering my question? That's rude.

I think the question has been answered and the concern has been addressed. All are standard.

@silverwind
Copy link
Member

Ok, I think we should enable HEAD methods on all these GET methods defined in Routes.

@wxiaoguang
Copy link
Contributor Author

Ok, I think we should enable HEAD methods on all these GET methods defined in Routes.

Actually no. Because most dynamic route handlers do not support HEAD.

Golang standard library only handles "file serving".

@silverwind
Copy link
Member

@wxiaoguang why are you dismissing without answering my question? That's rude.

I think the question has been answered and the concern has been addressed. All are standard.

Blocks should be dismissed by their author only, unless the author is unresponsive, then it's fine when someone else dismisses it.

@silverwind
Copy link
Member

silverwind commented Apr 19, 2024

Ok, I think we should enable HEAD methods on all these GET methods defined in Routes.

Actually no. Because most dynamic route handlers do not support HEAD.

Golang standard library only handles "file serving".

Yeah, it would be needed to be coded there. But I see we can maybe enable it for /metrics.

@wxiaoguang
Copy link
Contributor Author

@wxiaoguang why are you dismissing without answering my question? That's rude.

I think the question has been answered and the concern has been addressed. All are standard.

Blocks should be dismissed by their author only, unless the author is unresponsive, then it's fine when someone else dismisses it.

TBH I was a little unhappy about that change request, I also felt a little rude of it too. Because, it seems that you didn't really look into it (your referred document also said the "content-length" could exist). Hopefully you could also understand my feeling.

Yeah, it would be needed to be coded there. But I see we can maybe enable it for /metrics.

If and only if the handlers really support HEAD method.

wxiaoguang pushed a commit that referenced this pull request Apr 19, 2024
@silverwind
Copy link
Member

silverwind commented Apr 19, 2024

My conclusion is that it's a waste of server resources to send Content-Length in HEAD because you are basically building a response body and then immediately throwing it away.

So a proper HEAD implementation would be a custom implementation and not that wasteful thing that golang does.

silverwind pushed a commit that referenced this pull request Apr 19, 2024
Backport #30603 by @wxiaoguang

Fix #30601


```
~$ curl --head localhost:3000/robots.txt
HTTP/1.1 200 OK
Accept-Ranges: bytes
Cache-Control: max-age=0, private, must-revalidate
Content-Length: 5
Content-Type: text/plain; charset=utf-8
Last-Modified: Wed, 19 Jul 2023 04:56:12 GMT
X-Gitea-Debug: RUN_MODE=dev
Date: Fri, 19 Apr 2024 12:59:44 GMT
```

Co-authored-by: wxiaoguang <wxiaoguang@gmail.com>
zjjhot added a commit to zjjhot/gitea that referenced this pull request Apr 22, 2024
* giteaofficial/main:
  [skip ci] Updated licenses and gitignores
  Use correct hash for "git update-index" (go-gitea#30626)
  Fix repo home UI when there is no repo description (go-gitea#30552)
  Fix dropdown text ellipsis (go-gitea#30628)
  fix(api): refactor branch and tag existence checks (go-gitea#30618)
  Add --skip-db option to dump command (go-gitea#30613)
  Fix flash on dashboard (go-gitea#30572)
  chore: use errors.New to replace fmt.Errorf with no parameters will much better (go-gitea#30621)
  Fix issue comment form and quick-submit (go-gitea#30623)
  Use maintained gziphandler (go-gitea#30592)
  [skip ci] Updated translations via Crowdin
  Fix package list performance (go-gitea#30520)
  Clarify permission "HasAccess" behavior (go-gitea#30585)
  Fix links in PyPI Simple Repository API page (go-gitea#30594)
  Use action user as the trigger user of schedules (go-gitea#30581)
  Fix commit file status parser (go-gitea#30602)
  Fix HEAD method for robots.txt (go-gitea#30603)
@wxiaoguang wxiaoguang modified the milestones: 1.23.0, 1.22.0 Apr 27, 2024
@yardenshoham yardenshoham added the backport/done All backports for this PR have been created label May 5, 2024
@go-gitea go-gitea locked as resolved and limited conversation to collaborators Jul 19, 2024
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 backport/v1.21 This PR should be backported to Gitea 1.21 backport/v1.22 This PR should be backported to Gitea 1.22 lgtm/done This PR has enough approvals to get merged. There are no important open reservations anymore. modifies/go Pull requests that update Go code size/XS Denotes a PR that changes 0-9 lines, ignoring generated files. type/bug
Projects
None yet
Development

Successfully merging this pull request may close these issues.

HEAD to /robots.txt returns 404 error
6 participants