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

Strip '\r' in notification messages to avoid 'Content-Type: application/octet-stream' #7184

Merged
merged 1 commit into from
Jun 5, 2019

Conversation

robert-scheck
Copy link
Contributor

Without this patch, an accidential \r in e.g. $NOTIFICATIONCOMMENT leads to a Content-Type: application/octet-stream header in e-mails. The accidential \r might slip in usually using Icinga/Nagios apps...

#!/bin/bash

NOTIFICATION_MESSAGE=`cat << EOF
***** Service Monitoring on icinga2.example.net *****

CPU Load on tux.example.net is CRITICAL!

Info:    CRITICAL - load average: 812.35, 553.01, 269.48

When:    2019-05-16 07:39:05 +0200
Service: CPU Load
Host:    tux.example.net
IPv4:    192.0.2.42

Comment by Beastie:
  ACK
EOF
`

# Append accidential '\r' manually
NOTIFICATION_MESSAGE=${NOTIFICATION_MESSAGE}$'\r'

# Bashism to remove '\r'
#NOTIFICATION_MESSAGE=${NOTIFICATION_MESSAGE//[$'\r']}

# Issue reproducer (RHEL/SUSE/etc.)
/usr/bin/printf "%b" "$NOTIFICATION_MESSAGE" \
| mailx -r "monitoring@example.net" -s "[ACKNOWLEDGEMENT] CPU Load on tux.example.net is CRITICAL!" root@localhost

# Bashishm-free '\r' removal (RHEL/SUSE/etc.)
/usr/bin/printf "%b" "$NOTIFICATION_MESSAGE" | tr -d '\015' \
| mailx -r "monitoring@example.net" -s "[ACKNOWLEDGEMENT] CPU Load on tux.example.net is CRITICAL!" root@localhost

…on/octet-stream'

Without this patch, an accidential `\r` in e.g. `$NOTIFICATIONCOMMENT`
leads to a `Content-Type: application/octet-stream` header in e-mails.
The accidential `\r` might slip in usually using Icinga/Nagios apps...
@robert-scheck
Copy link
Contributor Author

@dnsmichi or @mxhash, who could review this?

@dnsmichi
Copy link
Contributor

dnsmichi commented Jun 3, 2019

@bsdlme Does this work on FreeBSD?

@bsdlme
Copy link
Contributor

bsdlme commented Jun 5, 2019

Should work on FreeBSD with

#!/usr/bin/env bash

as shebang.

@robert-scheck
Copy link
Contributor Author

@bsdlme, I'm a bit confused now: My pull request is bashism free, only my bug reproducer uses bash.

@bsdlme
Copy link
Contributor

bsdlme commented Jun 5, 2019

@robert-scheck Oops, yes. Sorry.

The tr -d '\015' line should also work on FreeBSD.

@dnsmichi dnsmichi added area/notifications Notification events area/setup Installation, systemd, sample files labels Jun 5, 2019
@dnsmichi dnsmichi added this to the 2.11.0 milestone Jun 5, 2019
@dnsmichi
Copy link
Contributor

dnsmichi commented Jun 5, 2019

Ok, thanks both 👍

@dnsmichi dnsmichi merged commit f312962 into Icinga:master Jun 5, 2019
@dnsmichi
Copy link
Contributor

dnsmichi commented Jun 6, 2019

Ah, Fedora. Hello :-)

@sircubbi
Copy link

Is there a reason why this bugfix was reverted (c783448)?

With most recent icinga-version mails once again get corrupted (on Rocky 8) is acknowledgements/notifications contain newlines.

@julianbrost
Copy link
Contributor

Seems like due to compatibility issues on Debian: #7648 #7706

@sircubbi
Copy link

Thanks for the pointer. However that issues seems more like related to shell-syntax. Just the removal of carriage-returns via the additional "tr"-command in the pipe shouldn't break Debian/Dash-shell.

The main culprit is, that using newlines on comments will be passed as "\r\n" in "$NOTIFICATION_MESSAGE", which aren't proper newlines on Unix. So either that should already filtered/converted by icingaweb or the icingadaemon, or just stripped out while piping to mail/mailx just like before.

Maybe a light fix might be to just put the "tr -d '\015'" in (and leave the base64-encoding for the subject out, as that part of the fix seems to break on dash and isn't really needed at least for mailx I guess).

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
area/notifications Notification events area/setup Installation, systemd, sample files
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants