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

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

Closed
zoic21 opened this issue May 30, 2023 · 14 comments
Closed

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

zoic21 opened this issue May 30, 2023 · 14 comments
Labels

Comments

@zoic21
Copy link

zoic21 commented May 30, 2023

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

jpmens commented May 30, 2023

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

@zoic21
Copy link
Author

zoic21 commented May 30, 2023

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

amotl commented May 30, 2023

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 amotl changed the title [ntfy] Error if message contain \n [ntfy] Error if message contains newline character \n May 30, 2023
@amotl
Copy link
Member

amotl commented May 30, 2023

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

codebude commented May 30, 2023

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

zoic21 commented May 31, 2023

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

codebude commented Jun 1, 2023

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

codebude commented Jun 1, 2023

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

@codebude
Copy link

codebude commented Jun 3, 2023

@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:

  • Take the current implementation but send the message as body instead of header. (That should enable line breaks)
  • If it's a message with attachment and no explicit message, then send it as today (attachment in body, everything else as headers)
  • if it's a message with attachment and message, then replace all \n in the message and send attachment as body and message as header

@amotl
Copy link
Member

amotl commented Jun 3, 2023 via email

@amotl
Copy link
Member

amotl commented Oct 14, 2023

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 amotl added the bug label Oct 14, 2023
@amotl
Copy link
Member

amotl commented Jan 4, 2024

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

codebude commented Jan 5, 2024

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

@amotl
Copy link
Member

amotl commented Jan 6, 2024

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

@amotl amotl closed this as completed Jan 6, 2024
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

4 participants