FazBrowse GitHub Viewer | Trending |
URL:
| Home
Tools: [Download Repo ZIP]   [Original HTTPS Page]

Reject invalid HTTP methods and resources by andy-maier · Pull Request #1019 · prometheus/client_python · GitHub

/ client_python Public

Reject invalid HTTP methods and resources - #1019

Merged
csmarchbanks merged 1 commit into
prometheus:masterfrom
andy-maier:andy/strict-http-methods
Aug 2, 2024
ConversationCommits (1)ChecksFiles changed
Merged

Reject invalid HTTP methods and resources#1019
csmarchbanks merged 1 commit into
prometheus:masterfrom
andy-maier:andy/strict-http-methods

Conversation

andy-maier commented Mar 30, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor

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.:

  • Does Prometheus issue any HTTP requests that are now rejected.
  • Does HEAD need to be supported.
  • Does the /metrics resource for which metrics are returned, need to be configurable by the users of this package.
  • Does the strict handling need to be enabled/disabled by the users of this package.
  • Are unit tests needed.

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.

andy-maier mentioned this pull request Mar 30, 2024
Closed
andy-maier force-pushed the andy/strict-http-methods branch 5 times, most recently from 8a06693 to 0a15d30 Compare March 31, 2024 13:32
andy-maier mentioned this pull request Apr 2, 2024
Closed
csmarchbanks reviewed Apr 8, 2024
View reviewed changes

csmarchbanks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

Comment thread prometheus_client/exposition.py Outdated
status = '200 OK'
headers = [('', '')]
output = b''
elif environ['PATH_INFO'].strip('/') not in ('', 'metrics'):

csmarchbanks Apr 8, 2024 •
edited
Loading

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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?

kakkoyun Apr 10, 2024

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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

andy-maier May 31, 2024 •
edited
Loading

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I can move the test fix into a separate PR.

andy-maier May 31, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

I was also unsure about the path checking and I can remove it.

andy-maier Jul 29, 2024

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

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 July 29, 2024 14:45
This was referenced Jul 29, 2024
Merged
Open
Closed
andy-maier force-pushed the andy/strict-http-methods branch from 5ae9852 to aa7e9e6 Compare July 30, 2024 08:36

andy-maier commented Jul 30, 2024

Copy link
Copy Markdown
Contributor Author

@csmarchbanks I moved the test fix into its own PR #1053 .

Reject invalid HTTP methods and resources
1ad7525 
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 July 30, 2024 08:49
csmarchbanks approved these changes Aug 2, 2024
View reviewed changes

csmarchbanks left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Choose a reason Spam Abuse Off Topic Outdated Duplicate Resolved Low Quality

LGTM, thanks for the updates!

csmarchbanks merged commit 7c45f84 into prometheus:master Aug 2, 2024
andy-maier deleted the andy/strict-http-methods branch September 28, 2024 05:13
This file contains hidden or 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
Show hidden characters
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

kakkoyun kakkoyun left review comments
csmarchbanks csmarchbanks approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

Back | FazBrowse Home | New Git URL