-
-
Notifications
You must be signed in to change notification settings - Fork 5.5k
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
Add AutoHead functionality. #5186
Add AutoHead functionality. #5186
Conversation
Codecov Report
@@ Coverage Diff @@
## master #5186 +/- ##
==========================================
- Coverage 37.5% 37.48% -0.02%
==========================================
Files 309 309
Lines 45820 45821 +1
==========================================
- Hits 17183 17176 -7
- Misses 26176 26181 +5
- Partials 2461 2464 +3
Continue to review full report at Codecov.
|
There are some URLs that are GET that perform an action. There are a few security tickets that are open currently to hopefully resolve this, so we may want to wait before merging this PR. |
@techknowlogick I was suspicious that might be the case. Hence the speculative nature of the pull request! I think it's possible to overrule a head method for such URLs if necessary - otherwise it may just be better to go down the more traditional route of creating individual head methods. (Obviously that would require knowing which URLs #5153 requires and a different pull request.) Is it worth using this pull request to list such naughty URLs and implement dumb head requests for these - or given the security concerns do you think we should abandon and reconsider this later? |
@techknowlogick Hmm, just thinking on I'm not sure how enabling HEAD would make these problems worse. They're already there with GET, enabling HEAD on those routes isn't making new routes - it's just that anyone who is trying to use HEAD might call those actions twice - but they're unlikely to be calling HEAD on such routes anyway - e.g. HEAD on /user/logout. I don't think a malicious user will gain much by using HEAD on these as they're going to be exactly the same as the GET processing just without any reply and if they wanted to, they could just send 2 GETs to get the same effect. |
@zeripath yup. that's reasonable. and I'm thinking now that the HEAD requests wouldn't send the appropriate cookies (example from the ticket is mastodon and that certainly wouldn't have the cookies). So I think this PR is fine to let through. |
4534572
to
8892fde
Compare
0540d93
to
b41274f
Compare
This pull-request adds a call to SetAutoHead which will simply provide HEAD request support for every GET request. This may not be the best way to provide this, as some GET requests may take time to process whereas a HEAD could be much more lightweight.
Fixes #5153