WordPress.org

Make WordPress Core

Opened 7 months ago

Closed 5 months ago

Last modified 5 months ago

#42693 closed defect (bug) (fixed)

WordPress 4.9 sends an "admin email address was changed" message to you@example.com on new install

Reported by: tigertech Owned by: SergeyBiryukov
Milestone: 4.9.3 Priority: normal
Severity: normal Version: 4.9
Component: Upgrade/Install Keywords: has-patch needs-testing dev-feedback
Focuses: Cc:

Description

This is a follow-up to #39117.

When I install a new copy of WordPress 4.9, it sends the following message to literal address you@example.com at install time:

To: you@example.com
Subject: [redacted] Notice of Admin Email Change

Hi,

This notice confirms that the admin email address was changed on [redacted].

The new admin email address is [redacted].

This email has been sent to you@example.com

Regards,
All at [redacted]

This happens because "wp-admin/includes/schema.php" initially populates the database with the placeholder address "you@…". Then wp_install() calls

        update_option('admin_email', $user_email);

And that triggers the new code from the change in #39117 to notice the update and send the email message.

Attachments (7)

42693.diff (867 bytes) - added by MattyRob 7 months ago.
42692v2.diff (1.4 KB) - added by MattyRob 7 months ago.
42693v2.diff (1.4 KB) - added by MattyRob 7 months ago.
Renamed patch to avoid ticket number confusion
42693v3.diff (1.5 KB) - added by tigertech 7 months ago.
Patch to check for
42693v4.diff (1.5 KB) - added by MattyRob 7 months ago.
Fix patch syntax error - unbalanced brackets in if statement
42693v5.diff (1.5 KB) - added by tigertech 7 months ago.
I agree that ".example" is reasonable and reads better as a general practice. This patch uses ".example" and applies cleanly against trunk. I tested the patch on a fresh install and verified that it no longer sends mail to "you@…", but that it still correctly sends messages on subsequent address changes.
42693v6.diff (1.6 KB) - added by seanchayes 5 months ago.

Download all attachments as: .zip

Change History (23)

#1 @Clorith
7 months ago

  • Keywords needs-patch added
  • Milestone changed from Awaiting Review to 4.9.1
  • Version changed from trunk to 4.9

#2 @MattyRob
7 months ago

  • Keywords has-patch needs-testing dev-feedback added; needs-patch removed

That attached patch will suppress emails to at default email address, not sure if that's the desired way of fixing but it will work.

@MattyRob
7 months ago

#3 @johnbillion
7 months ago

  • Milestone changed from 4.9.1 to 4.9.2

#4 follow-up: @Clorith
7 months ago

Just a quick note, as some stumbling in here may be wondering, example.com doesn't have a valid MX record, so no emails are actually transmitted.

#5 in reply to: ↑ 4 ; follow-up: @tigertech
7 months ago

Replying to Clorith:

Just a quick note, as some stumbling in here may be wondering, example.com doesn't have a valid MX record, so no emails are actually transmitted.

I know what you mean, but I should note that a message is actually sent to you@example.com via a mail server; it just bounces back as undeliverable. You'll see it if you have WordPress set to send from a real address:

Diagnostic-Code: smtp; 550 5.1.1 <you@example.com>: Recipient address rejected:
    User unknown in virtual mailbox table

But you're right that nobody at example.com is getting mailbombed with these, due to the missing MX record.

It would probably be better if the database was initially populated with an address "@example.invalid", instead of using a fake address at the real domain name "example.com". RFC 2606 specifies that the ".invalid" TLD is guaranteed never to exist and is intended for purposes like this.

Then the patch could check for an address ending with ".invalid" and avoid sending to it.

Last edited 7 months ago by tigertech (previous) (diff)

#6 @MattyRob
7 months ago

Alternative path attached based on last comment.

@MattyRob
7 months ago

@MattyRob
7 months ago

Renamed patch to avoid ticket number confusion

#7 follow-up: @ocean90
7 months ago

What about unhooking the action and/or checking for wp_installing()?

#8 in reply to: ↑ 7 ; follow-up: @MattyRob
7 months ago

Replying to ocean90:

What about unhooking the action and/or checking for wp_installing()?

That's possible but I wondered about what if a user changes their email TO an @example.com or .invalid address after installation for some reason and then changes is back - that's still result in the emails as the email functions are added as default filters.

One other thought I'd had was improving the core is_email() function and only sending the email if that validates however it seems DNS checks were moved from that filter previously and PHP filter_var() is not enabled on all installs.

I guess this is a careful balance to ensure we do send emails as much as possible without sending ones doomed to fail from the start, or maybe accepting failure is better than non-sending so just leave it.

#9 in reply to: ↑ 8 @tigertech
7 months ago

Replying to MattyRob:

One other thought I'd had was improving the core is_email() function and only sending the email if that validates however it seems DNS checks were moved from that filter previously and PHP filter_var() is not enabled on all installs.

Yeah -- DNS checks for working email are tricky and prone to transient false indications of failure. I think the second patch (changing the default TLD to ".invalid" and then avoiding sending to "invalid" addresses) is sufficient and wise.

My only pedantic comment is that the check should probably be

        if ( '.invalid' === substr( $old_email, -8 ) { 

... rather than:

        if ( 'invalid' === substr( $old_email, -7 ) { 

... on the basis that the RFC doesn't technically prohibit things like ".xinvalid" as a valid TLD. It also makes it a little clearer what the code is doing.

@tigertech
7 months ago

Patch to check for

#10 @tigertech
7 months ago

Sorry; I messed up the description of that "42693v3.diff" patch by accidentally pressing return half way through, like an idiot.

What I meant to say was "Patch to check for ".invalid" TLD (including the dot) and prevent sending email". It's the same as v2, except it checks the dot too.

#11 @dd32
7 months ago

While I don't mind using .invalid here, invalid reads a bit harsh anywhere that people *may* see it.

Instead of .invalid we could also use .example which may read better - you@your.domain.example for example.

@MattyRob
7 months ago

Fix patch syntax error - unbalanced brackets in if statement

@tigertech
7 months ago

I agree that ".example" is reasonable and reads better as a general practice. This patch uses ".example" and applies cleanly against trunk. I tested the patch on a fresh install and verified that it no longer sends mail to "you@…", but that it still correctly sends messages on subsequent address changes.

#12 in reply to: ↑ 5 @SergeyBiryukov
5 months ago

Replying to tigertech:

RFC 2606 specifies that the ".invalid" TLD is guaranteed never to exist and is intended for purposes like this.

It also states that example.com is a reserved name that can be used for a purpose like this.

What about keeping the current address and just checking for @example.com before sending the notifications? I think it's a bit cleaner than @your.domain.example.

#13 @seanchayes
5 months ago

The patch needed minor refresh and I also chose the @example.com from #comment:12 (but it can easily be changed).

I spent time testing the regular install of WordPress with this patch. During a fresh install of WordPress and upon completing the install with this patch enabled the actual send was skipped as intended.

For Multi-site, however, my attempts were not succesfull :-( so I could not test completely.

I did, however, notice a similar function update_option_new_admin_email in wp-admin/includes/misc.php

that also triggers an email send and I updated that function with the same check for @example.com used in the other portions of the patch.

The patch is 42693v6.diff

@seanchayes
5 months ago

#14 @dd32
5 months ago

  • Milestone changed from 4.9.2 to 4.9.3

Bumping to 4.9.3 due to 4.9.2s release

#15 @SergeyBiryukov
5 months ago

  • Owner set to SergeyBiryukov
  • Resolution set to fixed
  • Status changed from new to closed

In 42570:

Email: Don't send notifications for site or network admin email address change to the default 'admin_email' value.

Props tigertech, MattyRob, seanchayes.
Fixes #42693.

#16 @SergeyBiryukov
5 months ago

In 42571:

Email: Don't send notifications for site or network admin email address change to the default 'admin_email' value.

Props tigertech, MattyRob, seanchayes.
Merges [42570] to the 4.9 branch.
Fixes #42693.

Note: See TracTickets for help on using tickets.