WordPress.org

Make WordPress Core

Opened 7 months ago

Last modified 3 months ago

#25350 new enhancement

Make URL parameter for activation key parameter filterable

Reported by: boonebgorges Owned by:
Milestone: Future Release Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords: has-patch
Focuses: multisite Cc:

Description

The Office 365 email service (perhaps among others? Who knows what lurks out there?) filters all outgoing links, for security purposes. Among its security restrictions is that emailed links with URL parameters containing the string 'key' are forbidden; clicking them leads to an error. This includes links of the form http://example.com/wp-activate.php?key=12345, the link format used when sending activation emails in multisite. The result is that users cannot click the link to activate their accounts - and, in fact, they can't even copy and paste it in normal ways, because Microsoft masks the link (ugh).

This is an annoying problem that is in no way WordPress's fault. However, it causes severe problems in places where WP is used and Office 365 use is required, such as some universities. And, while it's easy to fix the text of the outgoing email using the notification filters in ms-functions.php, it's very difficult to modify the way that wp-activate.php works without resorting to the hackiest of hacks.

As a lightweight and backward compatibile workaround, I'm suggesting that the 'key' parameter be filterable. Patch attached, which abstracts the parameter into a filter wrapper. Not the most elegant thing in the world, but we are dealing with a problem of mammoth stupidity in the first place. Thanks for considering.

Attachments (4)

25350.patch (3.2 KB) - added by boonebgorges 7 months ago.
25350.02.patch (3.2 KB) - added by boonebgorges 7 months ago.
25350.03.patch (3.5 KB) - added by boonebgorges 7 months ago.
25350.04.patch (3.6 KB) - added by boonebgorges 7 months ago.

Download all attachments as: .zip

Change History (16)

boonebgorges7 months ago

comment:1 jeremyfelt7 months ago

  • Milestone changed from Awaiting Review to Future Release

I like it. It does look like line 61 and 76 in wp-activate.php should be $_GET[ $key_param], no?

There's also a // TODO: Don't hard code activation link. from a few years ago in ms-functions.php. We could expand this to filter both they key and the URL, but key seems most important now.

comment:2 nacin7 months ago

Man, this is stupid.

comment:3 johnbillion7 months ago

In addition to the filter, it would make sense to change the default parameter name to something that Office 365 does let through.

I'd also like to express my disbelief at how stupid this is.

comment:4 boonebgorges7 months ago

https://www.youtube.com/watch?v=qileP4bAzek sums up the situation pretty well.

It does look like line 61 and 76 in wp-activate.php should be $_GET[ $key_param], no?

Oops, my bad. See 25350.02.patch.

We could expand this to filter both they key and the URL, but key seems most important now.

Yeah, that's a good idea, though not central to the bug in this ticket.

In addition to the filter, it would make sense to change the default parameter name to something that Office 365 does let through.

The problem with changing the default parameter is that it'll probably break a fair number of sites. Some plugins (like BuddyPress) and probably many more standalone installations use the 'wpmu_signup_blog_notification_email' and 'wpmu_signup_user_notification_email' to modify the default content of the activation email. It's likely that in many of these cases, the activation link is being built using the hardcoded string 'key'. I suppose we could do something like 25350.03.patch, which would continue to support the 'key' param where it's currently in use.

boonebgorges7 months ago

boonebgorges7 months ago

comment:5 SergeyBiryukov7 months ago

A new function is probably not necessary, we could just apply the filter in these three instances, like we did for register filter in [23558].

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

comment:6 follow-ups: azaozz7 months ago

Yeah, extremely stupid URL filtering...

Not even sure we need a specialized filter for the key. It needs to be renamed and we need to support the old name for back-compat. Are there any other user cases where this filter can be used?

Also we look at both GET and POST for that key (not sure why not in REQUEST), seems the renamed key will have to be supported for POST too.

comment:7 in reply to: ↑ 6 boonebgorges7 months ago

Replying to SergeyBiryukov:

A new function is probably not necessary, we could just apply the filter in these three instances, like we did for register filter in [23558].

Yup, whatever the WP convention is for such things. The one small argument in favor of a standalone function is that some plugins may be filtering the notification messages, which will require retrieving the filtered key. See eg http://buddypress.trac.wordpress.org/browser/tags/1.8.1/bp-core/bp-core-filters.php#L275, line 277. This is marginally cleaner to do if there's a function, rather than running apply_filters() ourselves.

Also we look at both GET and POST for that key (not sure why not in REQUEST), seems the renamed key will have to be supported for POST too.

That makes sense. See 25350.04.patch.

Not even sure we need a specialized filter for the key.

I think it's valuable to have a filter on it, because you never know what kind of dumb rules some specific mail program will impose, and the filter allows for flexibility. Whether it needs to be *specialized*, I guess I don't know. The same issue arises with password reset URLs - I can write a separate patch for that, or roll it into this one, if we're agreed on the approach - and I suppose it'd be fine to use the same filter for each.

boonebgorges7 months ago

comment:8 in reply to: ↑ 6 SergeyBiryukov7 months ago

Replying to azaozz:

Not even sure we need a specialized filter for the key. It needs to be renamed and we need to support the old name for back-compat. Are there any other user cases where this filter can be used?

If we rename it, there's no guarantee that some other service won't do something equally stupid to the new key :)

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

comment:9 azaozz7 months ago

If we rename it, there's no guarantee that some other service won't do something equally stupid to the new key :)

Right, but if that happens, we still will have to change the default and support three different keys, filter or no filter... Then there is a question of back-compat when a plugin uses this filter and changes the key to incompatible value.

...The same issue arises with password reset URLs.

Exactly. Seems better to have filter(s) for the whole URLs that can be used for other fixes/changes rather than separate filter(s) for only one part of the query string.

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

comment:10 nacin7 months ago

I'm strongly inclined to stubbornly wontfix this and suggest it is a bug with Office 365 that Microsoft, not us, needs to fix.

comment:11 nacin7 months ago

I've been talking with a few developer advocates at Microsoft, specifically their Office division, and they're looking into it.

comment:12 jeremyfelt3 months ago

  • Component changed from Multisite to Login and Registration
  • Focuses multisite added
Note: See TracTickets for help on using tickets.