Make WordPress Core

Opened 11 years ago

Closed 8 years ago

Last modified 8 years ago

#25350 closed enhancement (worksforme)

Make URL parameter for activation key parameter filterable

Reported by: boonebgorges's profile boonebgorges Owned by:
Milestone: Priority: normal
Severity: normal Version:
Component: Login and Registration Keywords:
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 11 years ago.
25350.02.patch (3.2 KB) - added by boonebgorges 11 years ago.
25350.03.patch (3.5 KB) - added by boonebgorges 11 years ago.
25350.04.patch (3.6 KB) - added by boonebgorges 11 years ago.

Download all attachments as: .zip

Change History (20)

@boonebgorges
11 years ago

#1 @jeremyfelt
11 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.

#2 @nacin
11 years ago

Man, this is stupid.

#3 @johnbillion
11 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.

#4 @boonebgorges
11 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.

#5 @SergeyBiryukov
11 years ago

A new function is probably not necessary, we could just apply the filter in these three instances.

Version 0, edited 11 years ago by SergeyBiryukov (next)

#6 follow-ups: @azaozz
11 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.

#7 in reply to: ↑ 6 @boonebgorges
11 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.

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

#9 @azaozz
11 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 11 years ago by azaozz (previous) (diff)

#10 @nacin
11 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.

#11 @nacin
11 years ago

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

#12 @jeremyfelt
10 years ago

  • Component changed from Multisite to Login and Registration
  • Focuses multisite added

#13 @chriscct7
8 years ago

  • Keywords needs-testing added

Does anyone have an Office365 account they can test this with?

This ticket was mentioned in Slack in #core by chriscct7. View the logs.


8 years ago

#15 @chriscct7
8 years ago

  • Keywords has-patch needs-testing removed
  • Milestone Future Release deleted
  • Resolution set to worksforme
  • Status changed from new to closed

And then I realized my University of Florida email account runs on Office365. This is no longer valid. Happily closing as worksforme.

#16 @jorbin
8 years ago

I just confirmed Chris's findings with my work office365 account because I didn't read the last comment before testing.

Note: See TracTickets for help on using tickets.