WordPress.org

Make WordPress Core

Opened 2 years ago

Last modified 20 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 2 years ago.
25350.02.patch (3.2 KB) - added by boonebgorges 2 years ago.
25350.03.patch (3.5 KB) - added by boonebgorges 2 years ago.
25350.04.patch (3.6 KB) - added by boonebgorges 2 years ago.

Download all attachments as: .zip

Change History (16)

@boonebgorges2 years ago

comment:1 @jeremyfelt2 years 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 @nacin2 years ago

Man, this is stupid.

comment:3 @johnbillion2 years 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 @boonebgorges2 years 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.

@boonebgorges2 years ago

@boonebgorges2 years ago

comment:5 @SergeyBiryukov2 years 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 2 years ago by SergeyBiryukov (previous) (diff)

comment:6 follow-ups: @azaozz2 years 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 @boonebgorges2 years 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.

@boonebgorges2 years ago

comment:8 in reply to: ↑ 6 @SergeyBiryukov2 years 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 2 years ago by SergeyBiryukov (previous) (diff)

comment:9 @azaozz2 years 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 2 years ago by azaozz (previous) (diff)

comment:10 @nacin2 years 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 @nacin2 years ago

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

comment:12 @jeremyfelt20 months ago

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