Reject HTTP requests using a non-standard method #78
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
At the moment, request with methods such as DEBUG or PROPFIND will be
passed through the CDN and hit the origin, where they'll be rejected by
nginx as 501s.
This means we end up with a bunch of 5XX responses in our metrics and
logs, even though there's nothing wrong with our system.
$ curl -w '\n%{http_code}\n' -X DEBUG https://www.gov.uk
DEBUG method is not supported
501
This commit rejects these at the CDN, and uses the 405 HTTP status
instead of 501 to avoid polluting the CDN metrics with unactionable
5XXs.
I've included all standard HTTP methods, and additionally FASTLYPURGE
which is how the non-standard PURGE method appears in VCL[1].
I've also checked which non-standard HTTP methods have appeared in
requests so far in May:
select method, count(*)
from fastly_logs.govuk_www
where year=2024 and month=5
and method not in ('GET','HEAD','POST','PUT','DELETE','CONNECT','OPTIONS','TRACE','PATCH','FASTLYPURGE')
group by method
order by count(*) desc
# method _col1
1 DEBUG 23086
2 TENB 14458
3 PROPFIND 7700
4 HEADX 4029
5 TRACK 3
6 BVRPGMCC 1
7 CET 1
8 get 1
9 INDEX 1
... which is enough evidence for me that this won't block anything
unintentionally.
Temporarily, I've restricted this to integration so I can test it,
instead of just yeeting it into production.
[1]: https://www.fastly.com/documentation/reference/vcl/variables/client-request/req-method/
|
Closing this - on discussion, we've agreed that it's fine for these responses to be 501s, and that we should just handle 501s as "not our fault" errors when analysing logs / metrics. |
nimalank7
added a commit
that referenced
this pull request
Aug 30, 2024
Description: - Previously non-standard/unimplemented HTTP requests such as `DEBUG` will pass through Fastly and hit the origin where nginx rejects them as 501. See [MDN documentation](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/501) as to why this is the appropriate status code to return - Here Fastly is configured to throw a 501 and return custom HTML. The error code `806` and not `805` is used as the latter is used by `_security_txt_response.vcl` in the [shared VCL](https://github.com/alphagov/govuk-fastly/blob/main/modules/shared/_security_txt_response.vcl) - Tested in integration and staging: ``` curl -w '\n%{http_code}\n' -X DEBUG https://www.staging.publishing.service.gov.uk <!DOCTYPE html> <html> <head> <title>Welcome to GOV.UK</title> <style> body { font-family: Arial, sans-serif; margin: 0; } header { background: black; } h1 { color: white; font-size: 29px; margin: 0 auto; padding: 10px; max-width: 990px; } p { color: black; margin: 30px auto; max-width: 990px; } </style> </head> <body> 501 ``` - See [proof of concept here](#78)
nimalank7
added a commit
that referenced
this pull request
Aug 30, 2024
Description: - Previously non-standard/unimplemented HTTP requests such as `DEBUG` will pass through Fastly and hit the origin where nginx rejects them as 501. See [MDN documentation](https://developer.mozilla.org/en-US/docs/Web/HTTP/Status/501) as to why this is the appropriate status code to return - Here Fastly is configured to throw a 501 and return custom HTML. The error code `806` and not `805` is used as the latter is used by `_security_txt_response.vcl` in the [shared VCL](https://github.com/alphagov/govuk-fastly/blob/main/modules/shared/_security_txt_response.vcl) - `FASTLYPURGE` is how the non-standard `PURGE` method appears in [VCL](https://www.fastly.com/documentation/reference/vcl/variables/client-request/req-method/) - Tested in integration and staging: ``` curl -w '\n%{http_code}\n' -X DEBUG https://www.staging.publishing.service.gov.uk <!DOCTYPE html> <html> <head> <title>Welcome to GOV.UK</title> <style> body { font-family: Arial, sans-serif; margin: 0; } header { background: black; } h1 { color: white; font-size: 29px; margin: 0 auto; padding: 10px; max-width: 990px; } p { color: black; margin: 30px auto; max-width: 990px; } </style> </head> <body> 501 ``` - See [proof of concept here](#78) with data to show that it won't block anything unintentionally - Closes #79
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
At the moment, request with methods such as DEBUG or PROPFIND will be passed through the CDN and hit the origin, where they'll be rejected by nginx as 501s.
This means we end up with a bunch of 5XX responses in our metrics and logs, even though there's nothing wrong with our system.
This commit rejects these at the CDN, and uses the 405 HTTP status instead of 501 to avoid polluting the CDN metrics with unactionable 5XXs.
I've included all standard HTTP methods, and additionally FASTLYPURGE which is how the non-standard PURGE method appears in VCL1.
I've also checked which non-standard HTTP methods have appeared in requests so far in May:
... which is enough evidence for me that this won't block anything unintentionally.
Temporarily, I've restricted this to integration so I can test it, instead of just yeeting it into production.