Feature/fix email From: w/ configurable "displayname"

[sphakka]

I made an attempt at a configurable "displayname" string to be used in the "From:" header of outgoing e-mails. This should fix Issue #15. So, one should be able to configure

displayname = "(fcron)"  # empty by default

and get a "From:" header like

From: (fcron) <your-adress@your-domain>

instead of the original problematic one

From: <your-adress@your-domain> (fcron)

Be warned that I couldn't test it as the build is a bit shaky on my Gentoo box.

[yo8192]

Thanks for your contribution @sphakka !

It would be great if you could test the change. I am not sure what issue you have with the build, ideally that would work so you can test on your 'real' system. Alternatively, you could perhaps try your change in a temporary (Virtualbox or similar) VM to get more confidence it works as expected?

[yo8192]

I'm cleaning up old PRs... @sphakka did you have a chance to test? let me know if you'd like to go back to this PR to finish it.

[sphakka]

I'm cleaning up old PRs... @sphakka did you have a chance to test? let me know if you'd like to go back to this PR to finish it.

Sorry, I forgot about this. Yep, I'd like to finish it, will redo my tests right now.

[sphakka]

I ended up writing a function to properly format the "From:" header. Tests are OK -- I wrote a pseudo unit test, but don't know how to integrate... would be nice to have libcheck support with autoconf. On my Gentoo installation: [/etc/fcron/fcron.conf]

displayname = Fcron "Mod"ified

[/etc/crontab

!erroronlymail(true)
!mailfrom(root@foobar.com)
...
@ 5 echo "Fake job failing. $(date)" >> /var/log/fcron.debug.log && exit 1

the received mail exhibits the correct header in Thunbderbird:

• (via "Copy Name and Email address"): From: Fcron "Mod"ified <root@foobar.com>
• (via Ctrl + u): From: "Fcron \"Mod\"ified" <root@foobar.com>

[yo8192]

I ended up writing a function to properly format the "From:" header. Tests are OK -- I wrote a pseudo unit test, but don't know how to integrate... would be nice to have libcheck support with autoconf. On my Gentoo installation: [/etc/fcron/fcron.conf]

displayname = Fcron "Mod"ified

[/etc/crontab

!erroronlymail(true)
!mailfrom(root@foobar.com)
...
@ 5 echo "Fake job failing. $(date)" >> /var/log/fcron.debug.log && exit 1

the received mail exhibits the correct header in Thunbderbird:

* (via "Copy Name and Email address"): `From: Fcron "Mod"ified <root@foobar.com>`

* (via Ctrl + u): `From: "Fcron \"Mod\"ified" <root@foobar.com>`

Thanks!

Regarding the unit test, I am playing around with cmocka at the moment. I'll see if I can share at least a branch in the coming days. In the meantime, feel free to add your test code in a separate file so we can integrate it as a unit test once ready.

[yo8192]

I don't really mind whether the 'displayname' is configurable or not. Some people might find it convenient to do email filtering, but it's not clear how many people would end up actually using it.

However I'd rather we give an option for users to keep the old behaviour, e.g. in case they created filters in their mail systems and mailboxes that would suddenly stop working.

So either keep a 'diplayname' option but ensure that we default to the old behaviour if left unset/empty, or have a different option 'rfc_compliant_email' (or similar) to switch between the old format or the new (static) RFC compliant format?

On Tue, 4 Jun 2024, 10:29 sphakka, @.***> wrote:

@.**** commented on this pull request.

In files/fcron.conf.in https://github.com/yo8192/fcron/pull/17#discussion_r1625678308:

+# Display name for the "From: " header of mails sent by us. Watch out! It +# might cause rejections from picky SMTPDs.

Well, not really:

• old behavior: 'mailbox' addr = @.*** (fcron) -- breaks the RFC,
• new behavior: 'mailbox' addr = DISPLAYNAME @.***>.

With empty DISPLAYNAME, it'd give 'mailbox' addr = @. The closest we can get to the old behavior is by having a default displayname = (fcron), which gives 'mailbox' addr = "(fcron)" @.> (notice the double quotes).

Otherwise, we can just fix the whole affair with a sane hardcoded mailbox addr "(fcron)" <...>, and drop the configurable displayname (that's maybe a bit overkill) ^^.

What do you think?

— Reply to this email directly, view it on GitHub https://github.com/yo8192/fcron/pull/17#discussion_r1625678308, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPHBIWBUTDDGJI47QMFN4LZFWCIZAVCNFSM5OETF57KU5DIOJSWCZC7NNSXTPCQOVWGYUTFOF2WK43UKJSXM2LFO45TEMBZGU4DIOJWGEYA . You are receiving this because you were mentioned.Message ID: @.***>

[sphakka]

I don't really mind whether the 'displayname' is configurable or not. Some people might find it convenient to do email filtering, but it's not clear how many people would end up actually using it. However I'd rather we give an option for users to keep the old behaviour, e.g. in case they created filters in their mail systems and mailboxes that would suddenly stop working. So either keep a 'diplayname' option but ensure that we default to the old behaviour if left unset/empty, or have a different option 'rfc_compliant_email' (or similar) to switch between the old format or the new (static) RFC compliant format?

Right. Then we'll have:

1. a new bool option rfc_compliant_email:
   • if false (default), follow the old behavior -- bypass all together the new displayname option.
   • else true: enable the new behavior;
2. the proposed displayname option, defaulting to '(fcron)'.

That should ensure maximum POLA.

Just tell me please if a deprecation warning should be given about using the old behavior.

[sphakka]

Right. Then we'll have:

1. a new bool option `rfc_compliant_email`:

Actually, since this would be transitional, option rfc_compliant_email wouldn't last much. How about making it simpler with just the one option displayname? E.g.:

• default is unset (empty) => old behavior;
• set => new behavior.

That also seems less confusing and easier to maintain.

[yo8192]

Yes single option with a way to get the old behaviour (by leaving it empty) works.

I'm open to having it default to the new behaviour or the old one for now. In any case I'll cut a new release to explicitly call out the change when we make the new behaviour the default.

On Tue, 4 Jun 2024, 15:20 sphakka, @.***> wrote:

Right. Then we'll have:

1. a new bool option rfc_compliant_email:

Actually, since this would be transitional, option rfc_compliant_email wouldn't last much. How about making it simpler with just the one option displayname? E.g.:

• default is unset (empty) => old behavior;
• set => new behavior.

That also seems less confusing and easier to maintain.

— Reply to this email directly, view it on GitHub https://github.com/yo8192/fcron/pull/17#issuecomment-2147663040, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPHBIVA53MJNY6EZQWUEX3ZFXEJHAVCNFSM5OETF57KU5DIOJSWCZC7NNSXTN2JONZXKZKDN5WW2ZLOOQ5TEMJUG43DMMZQGQYA . You are receiving this because you were mentioned.Message ID: @.***>

[sphakka]

Here we go.

Code refactored in two functions, so that displayname is configured and checked once at startup. Old behavior applies if displayname is empty/null. Added buffer overflow checks, possibly a bit clumsy about real buffer length, though tests are OK: if length is exceeded along the chain (excess in configuration line, overflow because of quoting), log errors and discard displayname.

Minimal prototype testing support via make test: it actually only compiles and runs mailbox_addr.c (pseudo unit test).

Sorry for the massive space/indentation changes in the conf man page (EN only; FR left to native speakers), but it was really difficult to read!

[sphakka]

Thanks for all your work on this! Note I need to think a bit more about the overflow conditions -- I haven't really reviewed that aspect yet. I thought I'd share what I have now so you get faster feedback.

You're welcome!

Indeed, the check for displayname in the config layer is actually validation code. I'd say that all config file's lines should be checked against buffer overflow, maybe after getting ptr2. Then each call to Set() might accommodate a callback to a specific validation function, like format_displayname().

BTW, I forgot to note in my previous comment that the unit test can be run with make all test. I also valgrinded it, so the code should be safe at test-level.

I hope I am not wearing you down with all my comments... The work you have done is very useful already, and I think exchanging on this will result in better code than if either of us had done it in isolation.

If you are happy to continue like this, sounds good to me!

Don't worry, I'm fine with ping-pong review dynamics. Your feedback is indeed much appreciated, because I'm back to C programming after 20 years ^^

Alternatively, I may be able to change the PR code directly, and you could then double check what I am doing -- if that sounds appealing, let me know and I could try.

I'll be less reactive in the following two weeks because of vacations but will give feedback on the conversations above ASAP. Of course, if you're in a hurry for whatever reason, feel free to amend/optimize the obvious parts.

[yo8192]

Yes I think it is best to standardize on spaces, thanks.

On Mon, Jul 8, 2024, 15:44 sphakka @.***> wrote:

@.**** commented on this pull request.

In Makefile.in https://github.com/yo8192/fcron/pull/17#discussion_r1668780213:

@@ -55,6 +55,7 @@ ANSWERALL := @ANSWERALL@ USEPAM := @USEPAM@ FCRONDYN := @FCRONDYN@ SYSTEMD_DIR := @SYSTEMD_DIR@ +DISPLAYNAME := @DISPLAYNAME@

There's a mix of BLANKS and TABS (see also line 27, 44), indeed. Makefile's standard (?) in Emacs is to indent with TABS -- might have screwed that, indeed. However, alignments like var = value which depend on strings' length will always be problematic. What about aligning only with spaces at the price of some more editing overhead?

— Reply to this email directly, view it on GitHub https://github.com/yo8192/fcron/pull/17#discussion_r1668780213, or unsubscribe https://github.com/notifications/unsubscribe-auth/ABPHBIQO2LZEY44CRWLGFBLZLKQT3AVCNFSM5OETF57KU5DIOJSWCZC7NNSXTPCQOVWGYUTFOF2WK43UKJSXM2LFO45TEMJWGM2TMMJVGQZA . You are receiving this because you were mentioned.Message ID: @.***>

[yo8192]

@sphakka I think you are still working on this, which is perfectly fine. Just wanted to say: please add an explicit comment asking me to review again when you are ready, to make sure it doesn't get missed in all the comments and threads :) Thanks again for your work on this.

[sphakka]

@sphakka I think you are still working on this, which is perfectly fine. Just wanted to say: please add an explicit comment asking me to review again when you are ready, to make sure it doesn't get missed in all the comments and threads :) Thanks again for your work on this.

Oops, actually, I forgot to say that I'm done with this round ^^ Ready for review.

[yo8192]

@sphakka note I have renamed the configuration key to maildisplayname in https://github.com/yo8192/fcron/pull/33, along with some refactoring and small improvements.

Thanks again for all your work on this!