Reject invalid HTTP methods and resources #1019
Conversation
andy-maier
force-pushed
the
andy/strict-http-methods
branch
4 times, most recently
from
0b283ee
to
8a06693
Compare
This change addresses the issue that currently, any HTTP method is handled
by returning success and metrics data, which causes network scanners to
report issues.
Details:
* This change rejects any HTTP methods and resources other than the following:
OPTIONS /* - returns 200 and an 'Allow' header indicating allowed methods
GET / - returns 200 and metrics
GET /metrics - returns 200 and metrics
GET /favicon.ico - returns 200 and no body (this is no change)
Other resources than these on the allowed HTTP methods are rejected with
404 "Resource Not Found".
Other HTTP methods than these are rejected with 405 "Method Not Allowed"
and an 'Allow' header indicating the allowed HTTP methods.
Any returned HTTP errors are also displayed in the response body after a
hash sign and with a brief hint,
e.g. "# HTTP 405 Method Not Allowed: XXX; use OPTIONS or GET".
* Needed to pin asgiref to ==3.6.0 also for py3.8 to circumvent
the same error as for pypy3.8.
Signed-off-by: Andreas Maier <[email protected]>
andy-maier
force-pushed
the
andy/strict-http-methods
branch
from
8a06693
to
0a15d30
Compare
| # Serve empty response for browsers | ||
| status = '200 OK' | ||
| headers = [('', '')] | ||
| output = b'' | ||
| elif environ['PATH_INFO'].strip('/') not in ('', 'metrics'): |
There was a problem hiding this comment.
This is the breaking change I am most worried about, some users might be using non-standard paths to get their metrics as that all works right now and we don't require /metrics. What would you think of omitting this check?
There was a problem hiding this comment.
I agree with this. Even though /metrics is the convention, it's not enforced. e.g. for the client_golang, one can specify anything when they register a handler https://github.com/prometheus/client_golang/blob/e133e490296d2ff915bfc23cdec87c235ad36ef3/examples/simple/main.go#L43
There was a problem hiding this comment.
I can move the test fix into a separate PR.
There was a problem hiding this comment.
I was also unsure about the path checking and I can remove it.
This change addresses issue #1018 (currently, any HTTP method is handled by returning success and metrics data, which causes network scanners to report issues).
Note, this needs careful review w.r.t.:
/metricsresource for which metrics are returned, need to be configurable by the users of this package.Note, the pinning of asgiref==3.6.0 removes a test error in the py3.8 tox environment (same fix that already existed for the pypy3.8 tox environment). I don't know why the error on py3.8 comes up in the first place. I guess someone more experienced than me needs to look at that.
For details, see the commit message.