feat: added HTTP method option for push monitor

[0tt0sson]

Description

Some services use POST as their preferred callback HTTP method. This PR adds HTTP method option for push monitor so it can handle all the HTTP methods.

Type of change

• New feature (non-breaking change which adds functionality)

Checklist

• My code follows the style guidelines of this project
• I ran ESLint and other linters for modified files
• I have performed a self-review of my own code and tested it
• I have commented my code, particularly in hard-to-understand areas
  (including JSDoc for methods)
• My changes generate no new warnings
• My code needed automated testing. I have added them (this is optional task)

Screenshots

[louislam]

Thanks for the pr. In my opinion, it should be standardized, so I am afraid I cannot merge this pr.

[Computroniks]

Thanks for the pr. In my opinion, it should be standardized, so I am afraid I cannot merge this pr.

Do you mean standardized as in only GET requests? I don't really see a problem with giving the option of using different request types.

[louislam]

Thanks for the pr. In my opinion, it should be standardized, so I am afraid I cannot merge this pr.

Do you mean standardized as in only GET requests? I don't really see a problem with giving the option of using different request types.

Yes, only GET request.

The first reason is that, the push url is using query string ?msg=123&ping=123 to pass values, which is very common and easy to use in GET request. While in POST request, it is not common. A lot of HTTP libraries will eventually convert their POST request data into x-www-form-urlencoded or multipart/form-data instead of query string. Developers who don't aware this must be confused.

The second reason is that, it should a part of API docs in the future, I think dynamic HTTP methods should be wired.

So in my opinion, drawbacks > benefits.

On the other hand, what exact services which does provide POST request only but not GET request?

[ErmakovDmitriy]

Just my humble opinion: if Uptime Kuma supports HTTP POST as "Liveness" probe, it will allow me to use it as an endpoint to Prometheus-Alertmanager Watchdog alert.

In the Prometheus-Alertmanager stack, the Watchdog can be used to monitor the monitoring pipeline itself (if the Watchdog alert is present, then we know that Prometheus can contact Alertmanager, Alertmanager can contact "outside world").

[Computroniks]

I see where your coming from with the query string being used. I think that this is just an issue with this implementation as opposed to the actual idea of using a post request. It would be far better if the details were passed in the post body.

For the docs, openapi supports multiple methods for each path and it not uncommon to see such things, especially with a REST API.

If this PR were to be altered so that a separate post handler is used instead of the .all option and params are passed in the request body instead of query strings, would you be open to accepting it?

[0tt0sson]

I use Duplicacy that can send a POST call on backup completion.

The PR was created with as few changes and reusing as much existing code as possible to keep it simple.
I'm open to change it to only accept GET or POST if it preferred.

[svengerber]

I came across this limitation for configuring an uptime kuma push monitor integration with the watchdog alert from alertmanager.
Alertmanager always sends POST request to webhook urls.

It would be great to have a push monitor in uptime kuma that accepts POST requests.

For the time beeing, I have written a small middleware converting the POST to GET requests, that can be run as a lightweight sidecar to alertmanager. We are currently using this internally as a workaround.
Feel free to try it out.
https://github.com/natrontech/alertmanager-uptime-kuma-push

[ErebusBat]

FWIW Healthchecks.io does have a standardized Push API: https://healthchecks.io/docs/http_api/

HEAD, GET, and POST requests methods. The HTTP POST requests can optionally include diagnostic information in the request body. If the request body looks like a UTF-8 string, Healthchecks.io stores the request body (limited to the first 100KB for each received ping).

I recently installed https://github.com/favonia/cloudflare-ddns which supports the healthchecks API and hence will not work with Uptime Kuma.

Perhaps a decent compromise would be to support all three HTTP methods, but if you want to submit data (other than success or failure) then you must use a GET?

[deefdragon]

@louislam I very strongly believe this should be merged in (merge conflicts resolved of course) and ask you to reconsider. The fact that svengerber has gone and made a program (which others are have been using) to handle JUST this issue for alert-manager is indicative of how desired a feature it is.

While in POST request, [query parameters are] not common

Query parameters, the request method, and the request body are all fundamentally distinct parts in a request, and should not impact each other's usage. I have worked in many a project that uses query parameters in get and post requests (along with in patch, delete etc.

Developers who don't aware this must be confused.

I feel you are vastly underestimating the number of people who are using tooling that they can not really control (IE alertmanager or external services sending webhooks) in favor of potential developer confusion over what are standardized features of an http request.

A lot of HTTP libraries will eventually convert their POST request data into x-www-form-urlencoded or multipart/form-data instead of query string

While it may be better to allow for the data to be passed in the body when using a post etc. request, accepting using query parameters now, and potentially allowing both in the future would, I think, be good adherence to the robustness principal anyway.

At a fundamental level, this feature adds a lot of benefit for little added complexity, and (IMO) little confusion.

[favonia]

I recently installed https://github.com/favonia/cloudflare-ddns which supports the healthchecks API and hence will not work with Uptime Kuma.

@ErebusBat While waiting for the resolution of this PR, I (the auther of the quoted DDNS tool) just implemented the specific support for Uptime Kuma, so it should be working now... (but you need to use the new environment variable; see the README).

However, in general I think it's a good idea to accept requests other than GET requests, at least the POST ones. One can even argue that a ping request here is not idempotent and the ping should be POST instead GET. That is, POST is making it absolutely clear to any proxy in the middle that (1) it can have important effects on the server (and it does) and (2) things should not be cached, ever. It also has the benefit that misclicking on the link (which is possible in a modern terminal emulator) will not accidentally ping the monitor. All in all I think the benefit of allowing POST requests is significant given the small code change. I also hope @louislam could reconsider the decision.

[CommanderStorm]

First of all, I am fine with allowing more HTTP-Methods, but not fine with the rest of the changes, as they seem not fully thought out (or tested, see comment about the likely missing migration)

=> I would merge that line

I see problems with the others:

• Why would a user want to only use a specific method for a push monitor? => I don't see a reason to restrict this
• I also don't think that we need to change our examples for this

Note

I am going to disregart the api-docs argument previously in the issue as I don't think this route would need to be documented.
The examples we show after saving are likely enough..

[0tt0sson]

First of all, I am fine with allowing more HTTP-Methods, but not fine with the rest of the changes, as they seem not fully thought out (or tested, see comment about the likely missing migration)

=> I would merge that line

I see problems with the others:

• Why would a user want to only use a specific method for a push monitor? => I don't see a reason to restrict this
• I also don't think that we need to change our examples for this

Note

I am going to disregart the api-docs argument previously in the issue as I don't think this route would need to be documented. The examples we show after saving are likely enough..

I reduced this PR to the minimal changes needed to get this feature working.

[CommanderStorm]

Thanks for the change to the monitor! 🎉

Note

This PR is part of the v2.0 merge window => see #4500 for the bugs that need to be addressed before we can ship this release ^^

_{All changes in this PR are small and uncontroversial ⇒ merging with junior maintainer approval}