Skip to content

Webhook Channel Configuration could be stricter and use trim #257

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

Closed
martialblog opened this issue Jul 25, 2024 · 2 comments
Closed

Webhook Channel Configuration could be stricter and use trim #257

martialblog opened this issue Jul 25, 2024 · 2 comments

Comments

@martialblog
Copy link
Member

Hi,

the Webhook Channel Configuration HTTP Method and Response Status Codes could be stricter in what input they accept.

Response Status Codes are likely always [0-9]+ and HTTP Method an enum (get, post, etc...)

2024-07-25T10:08:34.214Z ERROR channel Failed to set channel plugin config, terminating the plugin {"id": 2, "name": "Captain Hook", "error": "failed to set plugin config: cannot convert status code \"Lorem ipsum dolor sit amet\" to int: strconv.Atoi: parsing \"Lorem ipsum dolor sit amet\": invalid syntax"}

Also the values could use a trim before being used, not sure if the backend or frontend should do that:

2024-07-25T10:09:56.207Z ERROR channel Failed to set channel plugin config, terminating the plugin {"id": 2, "name": "Captain Hook", "error": "failed to set plugin config: cannot convert status code \" 200 \" to int: strconv.Atoi: parsing \" 200 \": invalid syntax"}

Those sneaky whitespaces

@martialblog martialblog changed the title Webhook Channel Configuration could be stricter use trim Webhook Channel Configuration could be stricter and use trim Jul 25, 2024
@oxzi
Copy link
Member

oxzi commented Jul 26, 2024

This might be related to your other issues Icinga/icinga-notifications#255 and Icinga/icinga-notifications#258.

I would advise against trimming strings in web, as there might be the obscure case for obscure channels where either leading or trailing spaces are necessary. Thus, I would prefer to address Icinga/icinga-notifications#255 on the daemon side.

@nilmerg
Copy link
Member

nilmerg commented Jan 21, 2025

Since those options come from the plugin, Web won't do anything. Unless we introduce a "filter" functionality for options or something similar, where plugins can define such a behavior themselves. Or, like @oxzi said, do it when they're executed.

@nilmerg nilmerg closed this as not planned Won't fix, can't repro, duplicate, stale Jan 21, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants