caronc / apprise

Apprise - Push Notifications that work with just about every platform!
https://hub.docker.com/r/caronc/apprise
BSD 2-Clause "Simplified" License
12.1k stars 421 forks source link

White space in 'details' in PagerDuty is not parsed properly #1183

Open Jezreel-Zamora-Paidy opened 3 months ago

Jezreel-Zamora-Paidy commented 3 months ago

:mega: Notification Service(s) Impacted PagerDuty :lady_beetle: Describe the bug The details PagerDuty parameter is not being parsed properly and the space in the resulting message is changed into + sign. The root cause of this is that we run urllib.parse.urlencodeto details using the default values which encode the space character to + sign instead of %20. We then run urllib.parse.unquoteto the details which expects %20 for spaces instead of + sign.

Either we change how we encode the details or use urllib.parse.unquote_plus when decoding the url parameters. Happy to make the PR if the problem make sense.

:bulb: Screenshots and Logs

We are sending the alert via Prefect integration which uses appraise plugin:

@flow(name="Pager Alert Test")
def pager_test():
    pagerduty_webhook_block = PagerDutyWebHook.load("pagerduty-webhook")

    pagerduty_webhook_block.custom_details = {"error_message": "'This is the error message'"}
    pagerduty_webhook_block.block_initialization()

    with pagerduty_webhook_block.raise_on_failure():
        result = pagerduty_webhook_block.notify(subject="Hello from Prefect!", body="This is the body")

We get this result in PagerDuty

image (3)

:computer: Your System Details:

:crystal_ball: Additional context Add any other context about the problem here.

caronc commented 3 months ago

The plus is used by a lot of other plugins to mark a special keyword assignment to be a header change, or to allow Apprise to not lose it from the URL when it passes it along.

I.e.: schema://credentials/?+X-My-Header=value

The above would allow Apprise to use the + as its own internal action to move what follows into the header.

I don't think this is used for the plugin in question, but a global unquote plus may break this.

I'm also curious how many people we could impact who got used to the + working for them (now having to escape it if we went through with your suggestion)?

It's a tricky, but very interesting edge case. I do see your issue though.

caronc commented 3 months ago

Having a closer look at this, the error appears to be related to this flow application and the handling of the content you provide "before" it gets to Apprise. I don't see one reference to the Apprise library and it's functions itself in your shared example.

caronc commented 3 months ago

Let me know your thoughts, but this issue you brought forth is not Apprise. There is some data manipulation taking place between the flow interface you're using and the underlining Apprise library.

Will close this issue if i don't hear back in a month

Jezreel-Zamora-Paidy commented 3 months ago

Hey @caronc ,

Sorry for not getting back to you sooner. I debug this on my side and the data is being change once it was passed to apprise. I'll try to give more details next week as I was not able to record the logs last time.

Also, this is where prefect wrap the apprise plugin with their own class https://github.com/PrefectHQ/prefect/blob/aea34fd415bdb2a2edb94a06c572d0e0e0b830fc/src/prefect/blocks/notifications.py#L164 . As far as I can tell, they just passed the parameters to the Apprise plugin, but let me check again.

caronc commented 3 months ago

You may not be wrong, but the link you shared appears to just be the object initialization. I don't see where the code is being called. It's a call to send() i think we're looking for

caronc commented 1 month ago

I will close this ticket soon if i don't hear anything further; i can confirm this is an upstream issue not an issue with Apprise.