Reject invalid HTTP methods and resources #1019
Conversation
andy-maier
force-pushed
the
andy/strict-http-methods
branch
5 times, most recently
from
8a06693
to
0a15d30
Compare
prometheus_client/exposition.py
Outdated
| # 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.
There was a problem hiding this comment.
I have now updated the PR to remove the path check for GET. For consistency, I have also removed the path check for OPTIONS.
@kakkoyun @csmarchbanks Could you please review again, and merge if you agree?
andy-maier
force-pushed
the
andy/strict-http-methods
branch
2 times, most recently
from
cc17408
to
5ae9852
Compare
andy-maier
force-pushed
the
andy/strict-http-methods
branch
from
5ae9852
to
aa7e9e6
Compare
|
@csmarchbanks I moved the test fix into its own PR #1053 . |
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 (any) - returns 200 and an 'Allow' header indicating allowed methods
GET (any) - returns 200 and metrics
GET /favicon.ico - returns 200 and no body (this is no change)
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".
Signed-off-by: Andreas Maier <[email protected]>
andy-maier
force-pushed
the
andy/strict-http-methods
branch
from
aa7e9e6
to
1ad7525
Compare
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.