Reject invalid HTTP methods and resources#1019
Reject invalid HTTP methods and resources#1019csmarchbanks merged 1 commit intoprometheus:masterfrom
Conversation
andy-maier
force-pushed
the
andy/strict-http-methods
branch
5 times, most recently
from
8a06693 to
0a15d30
Compare
csmarchbanks
left a comment
csmarchbanks
left a comment
There was a problem hiding this comment.
I double checked and Prometheus only issues GET requests and there is no way to override that, so I think blocking non-GET methods is reasonable.
Would you be willing to move the test fix into a separate PR?
prometheus_client/exposition.py
Outdated
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 <maiera@de.ibm.com>
andy-maier
force-pushed
the
andy/strict-http-methods
branch
from
aa7e9e6 to
1ad7525
Compare
csmarchbanks
left a comment
csmarchbanks
left a comment
There was a problem hiding this comment.
LGTM, thanks for the updates!
3 participants
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.