From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from gate001.proxmox.com (gate001.proxmox.com [IPv6:2a0f:8001:1:32::40]) by lore.proxmox.com (Postfix) with ESMTPS id 000621FF0E2 for ; Thu, 30 Jul 2026 16:01:00 +0200 (CEST) Received: from gate001.proxmox.com (localhost.localdomain [127.0.0.1]) by gate001.proxmox.com (Proxmox) with ESMTP id 84610213E5; Thu, 30 Jul 2026 16:01:00 +0200 (CEST) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Thu, 30 Jul 2026 16:00:56 +0200 Message-Id: To: "Lukas Wagner" , Subject: Re: [PATCH proxmox] fix #7814: send-mail: normalize LF line endings to CRLF endings X-Mailer: aerc 0.20.0 References: <20260714084719.68076-1-s.sterz@proxmox.com> In-Reply-To: From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1785420047714 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.174 Adjusted score from AWL reputation of From: address DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment (newer systems) RCVD_IN_DNSWL_LOW -0.7 Sender listed at https://www.dnswl.org/, low trust SPF_HELO_NONE 0.001 SPF: HELO does not publish an SPF Record SPF_PASS -0.001 SPF: sender matches SPF record Message-ID-Hash: UNMH2CSZUQBQ4NCEYUZXLFZ5WHHSELID X-Message-ID-Hash: UNMH2CSZUQBQ4NCEYUZXLFZ5WHHSELID X-MailFrom: s.sterz@proxmox.com X-Mailman-Rule-Misses: dmarc-mitigation; no-senders; approved; loop; banned-address; emergency; member-moderation; nonmember-moderation; administrivia; implicit-dest; max-recipients; max-size; news-moderation; no-subject; digests; suspicious-header X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox VE development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed Jul 29, 2026 at 10:18 AM CEST, Lukas Wagner wrote: > Hi Shannon, > > sorry for the delay in getting to review this. Some notes inline. > > For what it's worth, this seems to work as advertised. Code also looks > okay to me in its current form. If you decide to act upon my feedback, > I'll quickly go over it again and send another R-b for that as well. > > Tested-by: Lukas Wagner > Reviewed-by: Lukas Wagner thank you and i'll send a new version in a minute. > On Tue Jul 14, 2026 at 10:47 AM CEST, Shannon Sterz wrote: >> RFC5321 Section 2.3.8 requires that mails do not contain bare CR or LF >> bytes. this patch normalizes unix style LF line endings to CRLF line >> endings. it also removes bare CR bytes. >> >> when text would be base64 encoded anyway, CR bytes will be dropped, to >> keep the behavior for encoded and non-encoded text aligned. however, >> since this code was already extensively tested against multiple MUAs, >> encoded LF line endings are kept as-is. >> >> also adds tests to encode this behavior. >> >> Signed-off-by: Shannon Sterz >> --- >> proxmox-sendmail/src/lib.rs | 246 ++++++++++++++++++++++++++++++++---- >> 1 file changed, 220 insertions(+), 26 deletions(-) >> >> diff --git a/proxmox-sendmail/src/lib.rs b/proxmox-sendmail/src/lib.rs >> index b3ff56e5..a73e8eb7 100644 >> --- a/proxmox-sendmail/src/lib.rs >> +++ b/proxmox-sendmail/src/lib.rs -->8 snip 8<-- >> @@ -63,6 +66,18 @@ fn encode_filename_formatted(filename: &str) -> Strin= g { >> format_encoded_text(&encoded, format_prefix, ";", 0, true) >> } >> >> +/// Strip bare CR bytes, than base64 encode. This is useful to make sur= e we treat encoded (header) > > nit: should be 'then' instead of 'than' done. -->8 snip 8<-- >> @@ -485,11 +499,27 @@ impl<'a> Mail<'a> { >> .spawn() >> .with_context(|| "could not spawn sendmail process")?; >> >> + let capacity =3D message.iter().fold(0, |acc, e| match *e { >> + b'\r' =3D> acc, >> + b'\n' =3D> acc + 2, >> + _ =3D> acc + 1, >> + }); > > No hard feelings, this is not a hot code path after all, but I guess you > could just multiply message.len() with some constant factor (e.g. 2) to > get the initial capacity and avoid the double iteration. The message is > likely not going to be larger than a couple hundred bytes or a few > kilobytes, so the extra memory does not matter IMO. ack, i did that now and added a comment on why we chose to do that. -->8 snip 8<-- >> @@ -606,17 +638,15 @@ impl<'a> Mail<'a> { >> header.push_str("Auto-Submitted: auto-generated;\n"); >> >> for (key, value) in &self.headers { >> - // Strip CR/LF so an interpolated key or value cannot injec= t extra header lines. >> - let key =3D key.replace(['\r', '\n'], ""); >> - let value =3D value.replace(['\r', '\n'], ""); >> + // Strip LF so an interpolated key or value cannot inject e= xtra header lines. Bare CR >> + // will be stripped for non-ASCII text before encoding and = for ASCII text globally >> + // before finishing formatting. >> + let key =3D key.replace('\n', ""); >> + let value =3D value.replace('\n', ""); > > I wonder whether it would be better to just return errors if there are > control characters in Subject, headers, recipient... what do you think? i'd prefer that too. however, this was added previously [1] and there are now multiple internal users of this crate. so imo such a change is a) orthogonal to this patch and b) warrants consultation with the aforementioned internal users who might rely on this behavior. [1]: https://git.proxmox.com/?p=3Dproxmox.git;a=3Dblobdiff;f=3Dproxmox-send= mail/src/lib.rs;h=3Db3ff56e5ab