From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from firstgate.proxmox.com (firstgate.proxmox.com [212.224.123.68]) by lore.proxmox.com (Postfix) with ESMTPS id 7F82B1FF13B for ; Wed, 25 Feb 2026 11:19:14 +0100 (CET) Received: from firstgate.proxmox.com (localhost [127.0.0.1]) by firstgate.proxmox.com (Proxmox) with ESMTP id 71C8A33610; Wed, 25 Feb 2026 11:20:11 +0100 (CET) Mime-Version: 1.0 Content-Transfer-Encoding: quoted-printable Content-Type: text/plain; charset=UTF-8 Date: Wed, 25 Feb 2026 11:20:04 +0100 Message-Id: Subject: Re: [PATCH proxmox v2] sendmail: conform more to mime Content-Disposition header and wrap lines To: "Lukas Wagner" X-Mailer: aerc 0.20.0 References: <20260219145805.1114672-1-s.sterz@proxmox.com> In-Reply-To: From: "Shannon Sterz" X-Bm-Milter-Handled: 55990f41-d878-4baa-be0a-ee34c49e34d2 X-Bm-Transport-Timestamp: 1772014787742 X-SPAM-LEVEL: Spam detection results: 0 AWL 0.095 Adjusted score from AWL reputation of From: address BAYES_00 -1.9 Bayes spam probability is 0 to 1% DMARC_MISSING 0.1 Missing DMARC policy KAM_DMARC_STATUS 0.01 Test Rule for DKIM or SPF Failure with Strict Alignment 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: CDTR3YH2LH24FLC6NWAE5PEWLLUSRQCK X-Message-ID-Hash: CDTR3YH2LH24FLC6NWAE5PEWLLUSRQCK 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 CC: Lukas Wagner , pbs-devel@lists.proxmox.com X-Mailman-Version: 3.3.10 Precedence: list List-Id: Proxmox Backup Server development discussion List-Help: List-Owner: List-Post: List-Subscribe: List-Unsubscribe: On Wed Feb 25, 2026 at 10:14 AM CET, Lukas Wagner wrote: > On Thu Feb 19, 2026 at 3:58 PM CET, Shannon Sterz wrote: >> the http and mime Content-Disposition headers function slightly >> differently. for the http version the following would be valid >> according to rfc 6266 [1]: >> >> Content-Disposition: attachment; filename=3D"file.pdf"; >> filename*=3DUTF-8''file.pdf >> >> specifying multiple filename attributes like this isn't valid for mime >> messages. according to rfc 2183 [2] the above should be expressed like >> so for mime messages (one variant, multiple are supported): >> >> Content-Disposition: attachment; >> filename*0*=3DUTF-8''file.bin >> >> while rfc 2183 [2] would in theory allow for multiple filename >> parameters similar to rfc 6266, in practice MIME::Tools [2] considers >> this as ambigous content [3]. in more recent versions of Proxmox Mail >> Gateway, such messages are rejected [4]. >> >> this patch implements proper filename handling and also wraps >> filenames in the content disposition and content type headers >> appropriatelly to a mail being rejected due to having mime headers >> that are too long as described in rfcs 2231 [5] and 2045 [6]. >> >> [1]: https://datatracker.ietf.org/doc/html/rfc6266/ >> [2]: https://datatracker.ietf.org/doc/html/rfc2183#section-2 >> [3]: https://metacpan.org/pod/MIME::Head#ambiguous_content >> [4]: >> https://git.proxmox.com/?p=3Dpmg-api.git;a=3Dcommitdiff;h=3D66f51c62f789= b4c20308b7594fbbb357721b0844 >> [5]: https://datatracker.ietf.org/doc/html/rfc2231#section-7 >> [6]: https://datatracker.ietf.org/doc/html/rfc2045/#section-6.8 >> > > Looks mostly good, one comment inline. My suggestion can be fixed in a > follow-up patch, I don't want to block applying this. > > Reviewed-by: Lukas Wagner > > That being said, I relied on your descriptions in the commit messages > and did not read through the RFCs myself. > > [...] >> + >> +fn format_text( >> + text: &str, >> + prefix: F, >> + suffix: &str, >> + shorten_first: usize, >> + skip_last_suffix: bool, >> +) -> String >> +where >> + F: Fn(Option) -> String, >> +{ >> + const DEFAULT_TEXT_WIDTH: usize =3D 72; >> + >> + let bytes =3D text.as_bytes(); >> + let suffix =3D suffix.as_bytes(); >> + >> + let format_text_len =3D suffix.len() + prefix(None).len(); >> + let lines =3D (bytes.len() + shorten_first).div_ceil(DEFAULT_TEXT_W= IDTH - format_text_len); >> + >> + // bytes + formatting and "\n" per line - no "\n" in the last line >> + let mut capacity =3D bytes.len() + (format_text_len + 1) * lines - = 1; >> + >> + if skip_last_suffix { >> + capacity -=3D suffix.len(); >> + } >> + >> + let mut out =3D Vec::with_capacity(capacity); >> + let first_line_length =3D DEFAULT_TEXT_WIDTH - shorten_first - form= at_text_len; >> + >> + let (total_lines, bytes) =3D >> + if let Some((first, rest)) =3D bytes.split_at_checked(first_lin= e_length) { >> + out.extend_from_slice(prefix(Some(0)).as_bytes()); >> + out.extend_from_slice(first); >> + out.extend_from_slice(suffix); >> + if !rest.is_empty() { >> + out.push(b'\n'); >> + } >> + (1, rest) >> + } else { >> + (0, bytes) >> + }; >> + >> + for (line, chunk) in bytes >> + .chunks(DEFAULT_TEXT_WIDTH - format_text_len) >> + .enumerate() >> + { >> + let line =3D line + total_lines; >> + out.extend_from_slice(prefix(Some(line)).as_bytes()); >> out.extend_from_slice(chunk); >> + >> if line + 1 !=3D lines { >> - // do not end last line with newline to give caller control= over doing so. >> + out.extend_from_slice(suffix); >> out.push(b'\n'); >> + } else if !skip_last_suffix { >> + out.extend_from_slice(suffix); >> } >> } >> >> - // SAFETY: base64 encoded, which is 7-bit chars (ASCII) and thus al= ways valid UTF8 >> + // SAFETY: all input slices were valid utf-8 string slices >> unsafe { String::from_utf8_unchecked(out) } >> } > > It took me uncomfortably long to fully grasp what this function was > doing, some ideas that could help further readers to understand it more > quickly: > > - maybe some comments would be warranted? So maybe a doc comment for > the inputs/outputs, and then in the code some comments explaining > the steps (e.g. why the first line is handled differently, etc.) > - make the function name a bit less generic (although I have a hard > time coming up with a good one at the spot, I have to admit) > - add one ore two test cases calling the function directly, so that > one can see the inputs and expected outputs yeah i'll see to it that i'll add a patch that adds documentation and test cases. i'll admit that after going over the related rfcs i was mostly focused on implementing this right and didn't sufficient context. going over this again i also noticed a small bug, so i'll send a v2 shortly. thanks for the review!