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

Add an extra header/body separator CRLF to ensure the first line of the body does not become part of the header #23

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

massar
Copy link

@massar massar commented Aug 14, 2021

A very simple fix that likely went unnoticed for a while ;) Thanks for building this system @ytti and @job very cool and useful!

Add an extra header/body separator CRLF to ensure the first line of the body does not become part of the header

Funny that in both instances (set/clear) there was a single word before the ':', thus really making it look like a header ;)

Before for clearing the alarm:

List-Id: ring-sqa <sqa.ring.nlnog.net>
X-Mailer: ring-sqa
massar01.ring.nlnog.net: clearing ipv4 alarm
<Other headers added by spam systems etc>

<empty body>

Or for raising the alarm:

List-Id: ring-sqa <sqa.ring.nlnog.net>
X-Mailer: ring-sqa
Regarding: massar01.ring.nlnog.net ipv4
<anti-spam headers added by other systems>

This is an automated alert from the distributed partial outage
...

After this change:

List-Id: ring-sqa <sqa.ring.nlnog.net>
X-Mailer: ring-sqa
<anti-spam headers>

massar01.ring.nlnog.net: clearing ipv4 alarm

or for raising the alarm:

List-Id: ring-sqa <sqa.ring.nlnog.net>
X-Mailer: ring-sqa
<anti-spam headers added by other systems>

Regarding: massar01.ring.nlnog.net ipv4
This is an automated alert from the distributed partial outage
...

…he body does not become part of the header

Funny that in both instances (set/clear) there was a single word before the ':', thus really making it look like a header ;)

Before for clearing the alarm:
```
List-Id: ring-sqa <sqa.ring.nlnog.net>
X-Mailer: ring-sqa
massar01.ring.nlnog.net: clearing ipv4 alarm
<Other headers added by spam systems etc>

<empty body>
```

Or for raising the alarm:
```
List-Id: ring-sqa <sqa.ring.nlnog.net>
X-Mailer: ring-sqa
Regarding: massar01.ring.nlnog.net ipv4
<anti-spam headers added by other systems>

This is an automated alert from the distributed partial outage
...
```

After this change:
```
List-Id: ring-sqa <sqa.ring.nlnog.net>
X-Mailer: ring-sqa
<anti-spam headers>

massar01.ring.nlnog.net: clearing ipv4 alarm
```

or for raising the alarm:
```
List-Id: ring-sqa <sqa.ring.nlnog.net>
X-Mailer: ring-sqa
<anti-spam headers added by other systems>

Regarding: massar01.ring.nlnog.net ipv4
This is an automated alert from the distributed partial outage
...
```
@ytti
Copy link
Member

ytti commented May 21, 2024

The bug isn't exactly "we forgot to add an empty line", the bug is that I added the empty line incorrectly.

I'm doing [header, ""].join("\n") + body, yielding result of header\n""body, while I needed to dy [header, "", body].join("\n"), or specifically [mail, "", @body].flatten.join("\n")

Now we keep the useless empty string at the end of the header array.

@ytti
Copy link
Member

ytti commented May 22, 2024

I've created another pull request, which attempts to solve the same, without leaving the useless empty string in front of the body.
As well as ensuring we don't have field separator in multiple places, in case we actually want CRLF instead of just LF, we'd only change one place.

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

Successfully merging this pull request may close these issues.

3 participants