fix encoding of email addresses by rjbs · Pull Request #489 · andk/pause

rjbs · 2024-04-30T23:14:47Z

We saw cases where a user with the "real name" of Bob"Smith could cause problems in how we'd send them mail. This branch will fix that.

Previously, the code often naively formed To or From headers with code like this:

my $to_header = qq{"$asciiname" <$email>};

This worked most of the time. Given an $asciiname with " in it, though, it would break. Probably one that ended in \ would break, and probably other case. Also, it's much nicer to use people's full names, rather than ASCII!

We send our emails using Email::MIME. Email::MIME allows certain kinds of objects to be provided for certain kinds of headers. Notably, an Email::MIME::Header::AddressList object can be passed for To, From, Cc, Reply-To, and some others. This branch makes it much easier to get or create AddressList objects, and then uses them in most places where emails are created.

the PAUSE::Email library now exists, with methods for getting common addresses (internal reporting, contact-us, noreply) and for building new AddressLists
the PAUSE::MailAddress library, used to pick user email addresses, can now supply AddressList objects using properly encoded non-ASCII names
the various contact addresses in $Config are made clearer

rjbs · 2024-05-03T22:49:12Z

The edit_cred pause_2017 test is failing. I can't really tell why. I'll look into it more sometime soon, but maybe somebody (charsbar… 😉) can tell.

I did edit lib/pause_2017/PAUSE/Web/Controller/User/Cred.pm so I will go read that first…

rjbs · 2024-05-04T02:28:10Z

This may be ready for review…

andk · 2025-05-01T10:58:26Z

Hi @rjbs,

what do you think about this pull request ATM? Could it be applied as it is? Would it be possible for you to resolve the four conflicts listed above? Actually only one of the four conflicts needs resolution, the three conflicts in the pause_1999 directory have resolved by removing that whole directory.

charsbar · 2025-05-02T10:05:31Z

cf. #537 which you might also want to address in this PR.

ADMINS was an array reference. CONTACT_ADDRESS is a single address. We just use ['modules@perl.org'] as ADMINS, and do things like interpolate it into strings, meaning that if we had two addresses, we would get: Just email user@example.com other@example.mil …which is weird. Also, having to deal with joining the ADMINS arrayref is just a pain. Instead, make it one string!

ADMIN is the address that many emails are sent from. Now that the code will use this setting to make address objects, leaving it undef will not do! We get warnings. CONTACT_ADDRESS is the setting where we usually put "modules@perl.org", where we tell people to send the admins mail. We will want to check this in PAUSE daemon tests.

* admin_email_list - a List containing the emails in the ADMIN config * email_list - given Email::Address::XS objects, make a List * fromaddr_email_list - a List containing the email in the UPLOAD config * is_valid_email - test whether a string is a single valid email address

There is a fair bit of rewriting here, hopefully making the code clearer. The most important part, though, is adding the ->email_header_object method, which will give us an Add object suitable for passing into header constructors.

This is a test run for things to come. In most places, we only use the user email address, not their name. But in some places, most especially in paused, we use their name. There were two problems there: 1. we were using the ASCII name, which we really don't need to do 2. we were not escaping any quotes (or anything else) in the name field, which could cause problems if someone puts a double quote in their fullname It wasn't trivial to test and fix those locations because we don't (yet!) have any tests for paused. This introduces the use of fullname to mldistwatch, using the new form: get the PAUSE::MailAddress for the user, then use its ->email_header_object method to create a Email::MIME::Header::AddressList. We'll switch all the mailing to use this, soon.

I could not yet remove Email::Address, because we use its $addr_spec, which is easy, but not trivial, to replace.

UPLOAD will become NOREPLY_ADDRESS soon.

I don't know this code well, so this may be a bit of a wreck. First, `$mgr->prepare_sendto` is updated. 1. prefer fullname to asciiname, because it is more polite 2. use real email header generation, not just concat 3. if we had "Bob <x@y>" and "Alice <x@y>" we now only send one email, not one per unique phrase; I think this is fine Where the prepare_sendto code was basically duplicated, I have switched things to use prepare_sendto. Internal report emails now go to the internal reporting address, not the public contact address. In some places, the code was quite tough for me to read, so I just left `$email` and the like used. This should work fine. It would be nice to add phases later. **I didn't understand this**: `$pause->{send_to}`. That was being set to a string, but I don't know really why or how it was used, although I saw some hints. The new code would've put ARRAY(0x1234) in that, so I changed the text. I will seek advice.

This is the address a user is told to write to when their upload URI can't be recognized, so it should be the public contact address. Co-Authored-By: Claude <noreply@anthropic.com>

INTERNAL_REPORT_ADDRESS is where the software sends reports for the admins to read. It is not where we should tell users to write, and it should not appear on the public whois page or in the welcome email. CONTACT_ADDRESS exists for exactly that, so use it everywhere a human other than an admin will see the address. Co-Authored-By: Claude <noreply@anthropic.com>

The report address lives in INTERNAL_REPORT_ADDRESS now, but the error message still said ADMIN. Co-Authored-By: Claude <noreply@anthropic.com>

Co-Authored-By: Claude <noreply@anthropic.com>

All the header objects are built by PAUSE::Email and PAUSE::MailAddress, so dist.pm does not need these classes itself. Co-Authored-By: Claude <noreply@anthropic.com>

PAUSE::MailAddress called Encode and PAUSE::Email without loading either, and several pause_2017 files and paused called into PAUSE::Email and Email::Address::XS the same way. These all worked because something else had loaded the module first, which is not a property worth depending on. Co-Authored-By: Claude <noreply@anthropic.com>

parse_bare_address always returns an object, so the "did we get one" check was never false. Worse, the object overloads stringification, and evaluating it in boolean context stringifies it, which warns "Argument contains empty address" for exactly the inputs we are here to reject. Co-Authored-By: Claude <noreply@anthropic.com>

Co-Authored-By: Claude <noreply@anthropic.com>

Now that recipients can be Email::MIME::Header::AddressList objects, interpolating them into the "sending to" and hdebug log lines printed the object's address. PAUSE::Email->header_value_as_string hands back either the plain string or the list's as_string form. Co-Authored-By: Claude <noreply@anthropic.com>

Like the other modules fixed up recently, this only worked because something else loaded Encode first. Co-Authored-By: Claude <noreply@anthropic.com>

rjbs · 2026-10-24T17:04:09Z

@charsbar This touches some of the web code, I think totally safely, but maybe give a look?

charsbar · 2026-10-25T03:45:02Z

docker-compose/PrivatePAUSE.pm needs some fix. I think it's better to add some migration code in PAUSE.pm, or at least add a note on the migration for those who have privatelib/PrivatePAUSE.pm.

charsbar · 2026-10-25T03:43:09Z

+    # I don't know why this is like this.  I'm just reworking earlier code.
+    # -- rjbs, 2024-05-03
+    my $is_not_emaily = sub {
+      my ($inside) = $_[0] =~ /^\s*(.+)\s*$/;


Needs to be something like /^\s*(.+?)\s*$/; ?
Splitting the regex to s/^\s*//; and s/\s*$//; seems safer.

charsbar · 2026-10-25T03:44:25Z

     MOD_DATA_SOURCE_NAME => "dbi:mysql:mod",
     NO_SUCCESS_BREAK => 900,
+     NOREPLY_ADDRESS => 'upload@pause.perl.org', # used as a sender address, ideally only used by the receiver for filtering incoming email
     P5P => 'release-announce@perl.org',


P5P is not used anymore?

charsbar · 2026-10-25T03:57:09Z

-      To      => $PAUSE::Config->{ADMIN},
-      From    => "cron daemon cron-daily.pl <upload\@pause.perl.org>",
+      To      => PAUSE::Email->report_email_header_object,
+      From    => PAUSE::Email->noreply_email_header_object,


From's name is changed from "cron daemon cron-daily.pl" to "Perl Authors Upload Server". Not sure if admins use this to filter emails, but it might be better to keep the original name.

charsbar · 2026-10-25T03:58:58Z

-      To      => $PAUSE::Config->{ADMIN},
-      From    => "cron daemon cron-p6daily.pl <upload\@pause.perl.org>",
+      To      => PAUSE::Email->report_email_header_object,
+      From    => PAUSE::Email->noreply_email_header_object,


Same as above.

charsbar · 2026-10-25T04:07:42Z

 use Dumpvalue ();
 use DynaLoader ();
 use Email::MIME;
+use Encode ();


Is this really needed? Or rather, should this really belong to this PR?

charsbar · 2026-10-25T05:31:52Z

          ->new(ERROR =>
                "Unidentified error happened, please write to the PAUSE admin
- at $PAUSE::Config->{ADMIN} and help them identify what's going on. Thanks!")
+ at $PAUSE::Config->{CONTACT_ADDRESS} and help them identify what's going on. Thanks!")


Same as above.

charsbar · 2026-10-25T05:47:00Z


 (This mail has been generated automatically by the Perl Authors Upload
-Server on behalf of the admin <%== $PAUSE::Config->{ADMIN} %>)
+Server on behalf of the admin <%== $PAUSE::Config->{CONTACT_ADDRESS} %>)


Is this intentional? This is an email to give a one time password to the requested user, at https://github.com/andk/pause/blob/master/lib/pause_2017/PAUSE/Web/Plugin/UserRegistration.pm#L90-L96 .

charsbar · 2026-10-25T05:47:17Z

 Thanks & Regards,
 --
-<%== $PAUSE::Config->{ADMIN} %>
+<%== $PAUSE::Config->{CONTACT_ADDRESS} %>


Same as above.

charsbar · 2026-10-25T05:49:58Z

 </ul>

-<p>With this form you can tell the indexer to index selected files again. As it is done by a cron job, it may take up to an hour until the indexer actually executes the command. If this doesn't repair the index, please <a href="mailto:<%= $PAUSE::Config->{UPLOAD} %>">email me</a>.</p>
+<p>With this form you can tell the indexer to index selected files again. As it is done by a cron job, it may take up to an hour until the indexer actually executes the command. If this doesn't repair the index, please <a href="mailto:<%= $PAUSE::Config->{CONTACT_ADDRESS} %>">email me</a>.</p>


Might be better to use INTERNAL_REPORT_ADDRESS so that one of the admins can reindex for the user (or investigate the issue)?

charsbar · 2026-10-25T05:51:52Z

                         Mojo::ByteStream->new(qq{\)Please
 try again or report errors to <a href="mailto:}),
-                          $PAUSE::Config->{ADMIN},
+                          $PAUSE::Config->{CONTACT_ADDRESS},


INTERNAL_REPORT_ADDRESS because this is an internal error report?

rjbs added bug email emails and reports we sent labels Apr 30, 2024

rjbs self-assigned this May 1, 2024

rjbs force-pushed the weird-email-bug branch 5 times, most recently from 4210d5a to 1ba904d Compare May 3, 2024 22:38

rjbs force-pushed the weird-email-bug branch 4 times, most recently from 6fb897a to abc40d4 Compare May 4, 2024 02:13

rjbs force-pushed the weird-email-bug branch 2 times, most recently from 197460c to 6f7b302 Compare May 4, 2024 14:47

rjbs marked this pull request as ready for review May 4, 2024 14:47

rjbs requested review from charsbar and wolfsage May 4, 2024 14:47

rjbs changed the title ~~fix quoting of email addresses~~ fix encoding of email addresses May 4, 2024

rjbs force-pushed the weird-email-bug branch from 6f7b302 to 9c92e7f Compare May 12, 2024 00:34

charsbar mentioned this pull request May 2, 2025

Replace modules@cpan.org with modules@perl.org because the former seems not working #556

Merged

rjbs force-pushed the weird-email-bug branch from 9c92e7f to 665a251 Compare October 22, 2026 01:21

rjbs added 5 commits October 24, 2026 12:21

paused: remove unused variable

a29be1b

PAUSE::MailAddress: add an "email_header_object" method

ef4bcfe

There is a fair bit of rewriting here, hopefully making the code clearer. The most important part, though, is adding the ->email_header_object method, which will give us an Add object suitable for passing into header constructors.

rjbs and others added 22 commits October 24, 2026 12:22

prereqs: add Email::Address::XS

5abdfc2

I could not yet remove Email::Address, because we use its $addr_spec, which is easy, but not trivial, to replace.

pause_2017 RequestId: use is_valid_email, not addr_spec

2c53138

pause_2017 Cred: use is_valid_email, not addr_spec

ba6b3d9

prereqs: drop Email::Address (non-XS version)

e9f8544

update more email sending code to use email lists

c84b940

PAUSE::Email: add (and use) noreply_email_header_object

d32c30a

UPLOAD will become NOREPLY_ADDRESS soon.

PAUSE::Email: add (and use) contact_email_header_object

5266047

PAUSE::Config: replace ADMIN with INTERNAL_REPORT_ADDRESS

bf6210e

PAUSE::Config: replace UPLOAD with NOREPLY_ADDRESS

0dcc6d7

paused: use email header objects for email headers

b62e538

pause_2026 API Upload: use CONTACT_ADDRESS, not ADMIN

9776012

This is the address a user is told to write to when their upload URI can't be recognized, so it should be the public contact address. Co-Authored-By: Claude <noreply@anthropic.com>

PAUSE::Email: name the right config key when dying

408137c

The report address lives in INTERNAL_REPORT_ADDRESS now, but the error message still said ADMIN. Co-Authored-By: Claude <noreply@anthropic.com>

TestPAUSE: drop the ADMIN_LIST override, which nothing reads

6d80a52

Co-Authored-By: Claude <noreply@anthropic.com>

PAUSE::dist: drop two imports it never uses directly

002edab

All the header objects are built by PAUSE::Email and PAUSE::MailAddress, so dist.pm does not need these classes itself. Co-Authored-By: Claude <noreply@anthropic.com>

t/mldistwatch-misc: label the Cc assertions as Cc, not To

685928b

Co-Authored-By: Claude <noreply@anthropic.com>

PAUSE::mldistwatch: load Encode, which it calls

b72c6c5

Like the other modules fixed up recently, this only worked because something else loaded Encode first. Co-Authored-By: Claude <noreply@anthropic.com>

rjbs force-pushed the weird-email-bug branch from 665a251 to b72c6c5 Compare October 24, 2026 17:01

charsbar requested changes Oct 29, 2026

View reviewed changes

Conversation

rjbs commented Apr 30, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rjbs commented May 3, 2024

Uh oh!

rjbs commented May 4, 2024

Uh oh!

andk commented May 1, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

charsbar commented May 2, 2025

Uh oh!

rjbs commented Oct 24, 2026

Uh oh!

charsbar commented Oct 25, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Uh oh!

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

rjbs commented Apr 30, 2024 •

edited

Loading

andk commented May 1, 2025 •

edited

Loading