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

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

Open
Jezreel-Zamora-Paidy opened this issue Aug 16, 2024 · 5 comments
Open
Labels

Comments

@Jezreel-Zamora-Paidy
Copy link

📣 Notification Service(s) Impacted
PagerDuty
🐞 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.urlencode to details using the default values which encode the space character to + sign instead of %20. We then run urllib.parse.unquote to 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.

💡 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)

💻 Your System Details:

  • OS: [e.g. RedHat v8.0]
  • Python Version: 3.10

🔮 Additional context
Add any other context about the problem here.

@caronc
Copy link
Owner

caronc commented Aug 16, 2024

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
Copy link
Owner

caronc commented Aug 24, 2024

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
Copy link
Owner

caronc commented Aug 30, 2024

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
Copy link
Author

Jezreel-Zamora-Paidy commented Aug 30, 2024

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
Copy link
Owner

caronc commented Aug 30, 2024

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

No branches or pull requests

2 participants