Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

posts:// post:// get:// notifications ignore 404 type errors of the endpoint #2869

Open
duozmo opened this issue Dec 29, 2024 · 5 comments
Open
Assignees
Labels
bug Something isn't working Notifications systems Development of notifications of changes user-interface

Comments

@duozmo
Copy link

duozmo commented Dec 29, 2024

Describe the bug
When Change Detection delivers a notification, an HTTP response status indicating failure, such as 404, is treated as successful and disregarded silently.

Version
v0.48.05

How did you install?

Docker Compose

To Reproduce

Steps to reproduce the behavior:

  1. Go to the notification settings for a watch
  2. Supply a failing URL, such as posts://httpbin.org/status/404
  3. Push Send test notification, then open the notification log
  4. See there is no error or other indicator of failure provided. The only log entry shows SENDING.

Example watch: https://changedetection.io/share/0nFoikTeP3Ua

Expected behavior
Some sort of indication of failure in the log or watch should be shown to the user.

Desktop (please complete the following information):

  • OS: Ubuntu 22.04.3 LTS
  • Browser: Vivaldi
  • Version 7.0

Additional context
This can arise when Change Detection mangles your notification URL, as in #2868.

@dgtlmoon
Copy link
Owner

Hmm this is not quite true, the message is more saying that the notification was processed succefully, it does not actually check the status, for this you should look in the notification debug log

@dgtlmoon dgtlmoon changed the title Unsuccessful HTTP statuses are treated as successful when delivering notifications "Send test notification" Incorrect/misleading message / status check Dec 29, 2024
@duozmo
Copy link
Author

duozmo commented Dec 29, 2024

I did check the notification debug log, but didn't see any indication of trouble. For example, I have a watch set up on this page: https://www.random.org/quick-pick/?tickets=2&lottery=5x69.1x26 (used because it will change every check; please be gentle on this service). Notifications are going to posts://httpbin.org/status/404, which will always return a 404. In the debug log, I see:

2024/12/29 19:32:50,000 - SENDING - [{"title": "Changed: Random.org, for testing", "body": "Example body", "url": "posts://httpbin.org/status/404", "body_format": "text"}]
2024/12/29 19:29:48,000 - SENDING - [{"title": "Changed: Random.org, for testing", "body": "Example body", "url": "posts://httpbin.org/status/404", "body_format": "text"}]
2024/12/29 19:26:46,000 - SENDING - [{"title": "Changed: Random.org, for testing", "body": "Example body", "url": "posts://httpbin.org/status/404", "body_format": "text"}]

and so on, with no error messages. There's also nothing on the homepage / watch list indicating a problem.

Should I be looking somewhere else?

@dgtlmoon
Copy link
Owner

the post:// style notifications dont (yet) care if the other end gives a 404.. i wonder if thats the problem here? this code handles the post:// posts:// etc https://github.com/dgtlmoon/changedetection.io/blob/master/changedetectionio/apprise_plugin/__init__.py

@duozmo
Copy link
Author

duozmo commented Dec 29, 2024

Yeah, I think that's the exact issue. I guess I was seeing it as a bug, but you're saying it's known and expected behavior. I would lobby for giving users a heads up when it's possible to detect failed notification delivery (e.g. 404s), but it's up to you how you want to classify it.

@dgtlmoon
Copy link
Owner

dgtlmoon commented Dec 29, 2024

agreed - it seems like a dead-end/loose-end to me, it should atleast catch 40x errors and give some report

you somehow caught 404's not being handled and double-encoding of the URL! amazing finds :)

@dgtlmoon dgtlmoon added the Notifications systems Development of notifications of changes label Dec 29, 2024
@dgtlmoon dgtlmoon changed the title "Send test notification" Incorrect/misleading message / status check posts:// post:// get:// notifications ignore 404 type errors of the endpoint Dec 29, 2024
@dgtlmoon dgtlmoon added the bug Something isn't working label Dec 29, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working Notifications systems Development of notifications of changes user-interface
Projects
None yet
Development

No branches or pull requests

2 participants