[5.2] Bug 1837680: Add Message-ID header when missing at send#205
[5.2] Bug 1837680: Add Message-ID header when missing at send#205mrenvoize wants to merge 4 commits into
Conversation
justdave
left a comment
justdave
left a comment
There was a problem hiding this comment.
The issues mentioned here apply to the Harmony PR as well (bugzilla/harmony#146)
| $sitespec =~ s/:\/\//\./; # Make the protocol look like part of the domain | ||
| $sitespec =~ s/^([^:\/]+):(\d+)/$1/; # Remove a port number, to relocate |
There was a problem hiding this comment.
This does not appear to adequately sanitize the potential values of urlbase, since it might contain a path after the domain name and / is also an illegal character after the @ in the message ID.
for example, if urlbase = https://bugs.example.org/bugzilla/ then after these regexps, sitespec becomes roughly @https.bugs.example.org/bugzilla/
This is apparently an issue in the existing build_thread_marker that you probably copied this logic from, so would need to be dealt with in both places. Since the whole point of this bug is fixing acceptance and places that are strict about Message-IDs, that probably should be included in this patch.
| if (!defined $user_id) { | ||
| $user_id = Bugzilla->user->id; | ||
| } |
There was a problem hiding this comment.
There are several places this MessageToMTA gets called from where there will be no user, and there's also a few places where the mail being sent may be irrelevant to the current logged in user. It might be safer to leave this blank or use a static string instead if it's not passed in, to avoid tagging a user to something they weren't responsible for. The random string added later should take care of it still being unique.
There was a problem hiding this comment.
My syntax checker is warning me that $2 may contain values from a previous regexp match attempt if this one fails to match anything, since Perl doesn't clear them in between. It's suggesting the code should check if the regexp succeeded or not before depending on a match position variable to work.
Detect whether a Message-ID header has been included in the message passed to MessageToMTA and generate one if it's missing. GMail and other modern mail services now expect a Message-ID as part of the message else they mark them as spam or drop them entirely.
- Strip path components from urlbase when building sitespec, as '/' is illegal after '@' in a Message-ID (affects both build_thread_marker and build_message_id) - Fix stale $2 capture variable risk by conditioning port relocation on the substitution succeeding, in both functions - Avoid falling back to Bugzilla->user->id in build_message_id, which is called from contexts with no logged-in user; random bits ensure uniqueness without a user identifier - Omit the user_id segment entirely when absent to avoid a double-dash in the Message-ID local-part
Test the sitespec transformation for four urlbase forms (plain, with path, with port, with port+path) to guard against regressions in the '/' stripping and port relocation logic. Also covers user_id handling in build_message_id (present, absent, no double-dash).
2 participants
Details
Detect whether a Message-ID header has been included in the message passed to MessageToMTA and generate one if it's missing.
GMail and other modern mail services now expect a Message-ID as part of the message else they mark them as spam or drop them entirely.
Additional info