mqtt-tools / mqttwarn

A highly configurable MQTT message router, where the routing targets are notification plugins, primarily written in Python.
https://mqttwarn.readthedocs.io/
Eclipse Public License 2.0
950 stars 183 forks source link

[ntfy] Error if message contains newline character `\n` #677

Closed zoic21 closed 6 months ago

zoic21 commented 1 year ago

Hello,

I got a weird issue, if message contain a return line a got this error :

requests.exceptions.InvalidHeader: Invalid leading whitespace, reserved character(s), or returncharacter(s) in header value: '=?utf-8?q?Fin_de_l=27arrosage_de_la_zone_2=2C_humidit=C3=A9_du_sol_=3A_13_?=\n =?utf-8?q?=25?='

I think it's due to \n in my message, maybe you can just remove it when you parse ?

Thank in advance

jpmens commented 1 year ago

Is there a specific reason why you can't remove it before submission? ;-)

zoic21 commented 1 year ago

Yes and no, it's send by esphome and he add this, I need to find why, it's a bug (on my code I think) but maybe you can handle this type of case.

amotl commented 1 year ago

Dear Loïc,

thank you for writing in.

While I've not investigated further yet, I discovered the TestServer_PublishMessageInHeaderWithNewlines function in ntfy's test suite. It may indicate your use case should work well, even when using newlines in the message body, and the header transport style for communications, which mqttwarn is using.

Maybe the problem is indeed on mqttwarn's behalf?

With kind regards, Andreas.

/cc @binwiederhier

amotl commented 1 year ago
requests.exceptions.InvalidHeader: Invalid leading whitespace, reserved character(s), or returncharacter(s) in header value

Ah I see. It is on Python's behalf again, similar to the other error @codebude was reporting.

So, I figure ntfy will gracefully accept an eventual non-standard HTTP header here. I would be willing to evaluate options how to properly marshal newlines (and maybe other non-alphanumeric characters?), even when deviating from standard implementations, if that would be feasible.

@codebude: You did such an excellent job last time you have been concerned with sanding edge cases of the mqttwarn <-> ntfy integration. Maybe this problem can spark your interest? I would be so grateful if you could have a look.

codebude commented 1 year ago

From my understanding linebreaks are not allowed in HTTP headers. They are defined as header separators in the RFC and can't be send in header content: https://stackoverflow.com/a/47422593/251719

I see two options: 1) Either strip all linebreaks (e.g. via re.sub("\r?\n", "", data) ) from the message, before passing it to the Header/encode function: https://github.com/mqtt-tools/mqttwarn/blob/main/mqttwarn/services/ntfy.py#L242 2) Change the way, the ntfy service is implemented and post the message either as HTTP body (https://docs.ntfy.sh/publish/) or just send all properties incl. body as JSON (https://docs.ntfy.sh/publish/#publish-as-json).

Personally I would suggest to go with the second option, because I think it is legit to have linebreaks in notification messages. Thus option 1 seems more like a suboptimal workaround.

zoic21 commented 1 year ago

Hello,

Thank @amotl and @codebude, if i can give my opinion I think also option 2 is better. You will have less issue with json and special char or lenth.

codebude commented 1 year ago

Hm maybe there's one downside with the JSON method. As far as I read the documentation correctly, attachments can only be sent out as links. Not directly sent (as mqttwarn does with the actual implementation). So maybe we should open an issue at ntfy.sh first and ask them, to enable file/attachments via the JSON endpoint. (e.g. as base64 encoded JSON property).

codebude commented 1 year ago

I just opened an enhancement request at ntfy that asks for attachment/file support in the publish via JSON endpoint: https://github.com/binwiederhier/ntfy/issues/762

codebude commented 1 year ago

@amotl I got feedback on the request at ntfy and it's not planned to accept attachments in JSON. So maybe the best workaround then would look like:

amotl commented 1 year ago

Hi Raffael, thanks a stack for reaching out to ntfy, and for coming back with your proposal. I think it is a reasonable approach to resolve this issue, and you outlined it excellently. Cheers, Andreas.

On 3 June 2023 18:42:44 CEST, Raffael Herrmann @.***> wrote:

[...]

-- Sent from my mind. This might have been typed on a mobile device, so please excuse my brevity.

amotl commented 8 months ago

Hi again,

GH-684 intends to implement your suggestion, @codebude. It has been released with mqttwarn 0.35.0. Thank you very much.

With kind regards, Andreas.

amotl commented 6 months ago

Hi again,

first things first, I would like to wish you an excellent 2024.

Regarding the issue, I've not tracked this in detail, but also didn't hear about any complaints. Do you think this issue can be considered resolved?

With kind regards, Andreas.

codebude commented 6 months ago

So far, everything seems to be working as expected. Thanks again for the implementation :-)

amotl commented 6 months ago

Excellent, thank you. Let us know if you observe any other issues on this or related topics.