WordPress.org

Make WordPress Core

Opened 5 years ago

Closed 8 months ago

#17630 closed enhancement (fixed)

Redirect wp-signup.php to wp_signup_location

Reported by: dwieeb Owned by: pbearne
Milestone: 4.4 Priority: normal
Severity: normal Version: 3.1.3
Component: Login and Registration Keywords: good-first-bug has-patch
Focuses: multisite Cc:

Description

I noticed that if wp-signup.php is accessed directly and wp_signup_location is not the default "site_url( 'wp-signup.php' )", it does not redirect to the signup location.

I've attached a potential patch. It works for me.

Attachments (9)

patch.diff (545 bytes) - added by dwieeb 5 years ago.
17630.diff (467 bytes) - added by jeremyfelt 3 years ago.
17630.1.diff (536 bytes) - added by jeremyfelt 3 years ago.
17630.2.diff (807 bytes) - added by jeremyfelt 3 years ago.
17630.3.diff (806 bytes) - added by jeremyfelt 3 years ago.
17630.patch (811 bytes) - added by chriscct7 9 months ago.
17630.2.patch (1.6 KB) - added by pbearne 9 months ago.
Patch for extra action
wp-signup.php_code_tidy.patch (52.3 KB) - added by pbearne 9 months ago.
code tidy with patch
17630.3.patch (745 bytes) - added by pbearne 9 months ago.
removed extra space from action name and removed not needed filter

Download all attachments as: .zip

Change History (37)

@dwieeb
5 years ago

#1 @wpmuguru
4 years ago

  • Type changed from defect (bug) to enhancement

#2 @jeremyfelt
3 years ago

  • Keywords dev-feedback added
  • Milestone changed from Awaiting Review to 3.7

Does it make sense to handle it this way? The patch should probably be refreshed if so, but it would be a quick add. I could also see using an earlier, already available action to redirect the request.

@jeremyfelt
3 years ago

#3 @jeremyfelt
3 years ago

  • Severity changed from minor to normal

17630.diff uses network_site_url() instead of site_url() to align with all other uses of the wp_signup_location filter.

#4 follow-up: @nacin
3 years ago

I sense this could trigger an infinite redirect?

#5 in reply to: ↑ 4 @jeremyfelt
3 years ago

Replying to nacin:

I sense this could trigger an infinite redirect?

Because of the strict comparison? I thought that at first, but then figured these should always be full URLs/strings.

Though I guess it could be filtered to wp-signup.php?custom=thing and throw everything off.

@jeremyfelt
3 years ago

#6 @jeremyfelt
3 years ago

17630.1.diff strips any query string from the filtered URL before comparing to the network signup URL and only redirects if the combination of domain/path is different.

@jeremyfelt
3 years ago

#7 @jeremyfelt
3 years ago

17630.2.diff adds docs to the new wp_signup_location filter added in the patch.

#8 @SergeyBiryukov
3 years ago

die() in line 49 should probably be exit(), for consistency with [16847].

@jeremyfelt
3 years ago

#9 follow-up: @jeremyfelt
3 years ago

17630.3.diff uses exit; rather than die();, see [16847].

Interesting enough, there are a few other uses of wp_redirect() in wp-signup.php that still use die();.

#10 in reply to: ↑ 9 @nacin
3 years ago

  • Milestone changed from 3.7 to Future Release

Replying to jeremyfelt:

Interesting enough, there are a few other uses of wp_redirect() in wp-signup.php that still use die();.

Doesn't matter. [16847] was to enforce we were killing the script after a redirect, not that we were using exit over die. Though exit does seem to be our preferred statement.

This just hasn't been tested. I fear infinite redirects and other side effects. Someone filtering wp_signup_location can *also* choose to redirect wp-signup.php if they wanted to.

#11 @jeremyfelt
2 years ago

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

@chriscct7
9 months ago

#12 @chriscct7
9 months ago

Refreshed patch

#13 @jeremyfelt
9 months ago

  • Keywords needs-patch added; has-patch dev-feedback removed
  • Milestone changed from Future Release to 4.4

I think we'd be better off with an action here rather than correctly trying to guess and match a default location to avoid redirects.

wp-activate.php has an activate_header hook that can be used for this. The signup_header hook in wp-signup.php only runs inside wp_head, which is too late. We could do something like pre_signup_header.

Let's come up with a good action name or close this ticket. :)

#14 @DrewAPicture
9 months ago

@jeremyfelt If you'd rather go the action route, why not just rely on the existing signup_header hook and call this good?

#15 @jeremyfelt
9 months ago

signup_header only fires inside wp_head. At that point, headers are already sent and its too late to redirect. The corresponding action in wp-activate.php is activate_wp_head.

Naming!

Last edited 9 months ago by jeremyfelt (previous) (diff)

#16 follow-up: @DrewAPicture
9 months ago

How about before_signup_header to match before_signup_form?

#17 in reply to: ↑ 16 @jeremyfelt
9 months ago

  • Keywords good-first-bug added

Replying to DrewAPicture:

How about before_signup_header to match before_signup_form?

I like that. It should probably go just above get_header() for sanity.

This would be a good first bug with the action and associated docs. :)

#18 follow-up: @sujin2f
9 months ago

@jeremyfelt I don't understand why did you want to set an action because there is wp_signup_location filter already. If we make a new action, people will have to set one filter and one action in order to change the signup location. Just for preventing redirection loop?

I think we'd be better off with an action here rather than correctly trying to guess and match a default location to avoid redirects.

@pbearne
9 months ago

Patch for extra action

#19 @pbearne
9 months ago

Added the action just before the head

#20 @welcher
9 months ago

  • Keywords has-patch added; needs-patch removed

@pbearne
9 months ago

code tidy with patch

#21 @pbearne
9 months ago

For my entertainment, I cleaned the code in this file while I had it open :-)

Set the ifs' to yodia
Made the comments and default match
WhiteSpace etc.

I know this should be its own ticket, but I don't it here as it does include the current patches

Enjoy

#22 follow-ups: @DrewAPicture
9 months ago

@pbearne: As a matter of course, we don't update files to follow coding standards unless the code is already being touched for a legitimate reason. This helps keep the blame records clean. See the refactoring article in the Core Contributor Handbook for more on that.

17630.2.patch looks pretty good. Some notes:

  • The @since version on the wp_signup_location hook doc should be updated to 4.4.0
  • Looks like there's an extra space before the closing quote for the before_signup_header action tag
  • Let's remove the extra whitespacing

#23 in reply to: ↑ 22 @jeremyfelt
9 months ago

Per comment 13, I think we're okay to skip the re-use of the wp_signup_location filter here (which was added in 4.3 for wp-login) and instead add a header action that can be used to redirect if this file is ever hit. Sticking with before_signup_header should be good in this case.

#24 @jeremyfelt
9 months ago

  • Owner set to jeremyfelt
  • Status changed from new to reviewing

#25 in reply to: ↑ 18 @jeremyfelt
9 months ago

Replying to sujin2f:

@jeremyfelt I don't understand why did you want to set an action because there is wp_signup_location filter already. If we make a new action, people will have to set one filter and one action in order to change the signup location. Just for preventing redirection loop?

Correct. Someone would probably have the easiest time redirecting wp-signup.php manually at the server level, but for those needing to force it via plugin, a second action to short-circuit the file should be fine. Reliance on the current wp_signup_location filter is a bit iffy, mostly because we would need to compare it with network_site_url( 'wp-signup.php' ) which could also be filtered. We could make an attempt at comparing it with $_SERVER['SCRIPT_NAME'], but I'm not sure how reliable that would be.

The use of a new action allows for a general short-circuit of the wp-signup.php load and would also help anyone trying to replace the signup experience without changing the location. It also aligns us a bit more with what actions are available in wp-activate.php.

#26 in reply to: ↑ 22 @jeremyfelt
9 months ago

  • Owner changed from jeremyfelt to pbearne

@pbearne I'm going to assign you as the owner to refresh the patch per Drew's comment. Note that it's not applying to trunk cleanly at the moment. Let's also remove the use of wp_signup_location filter entirely and stick to the new action. Thanks!

#27 @pbearne
9 months ago

@jeremyfelt here you go I have tested the patch applies against trunk so we should be good

@pbearne
9 months ago

removed extra space from action name and removed not needed filter

#28 @jeremyfelt
8 months ago

  • Resolution set to fixed
  • Status changed from reviewing to closed

In 35159:

MS: Introduce action before_signup_header.

This aligns wp-signup.php a bit more with wp-activate.php` and, among other things, allows a plugin to redirect signup requests.

Props pbearne.
Fixes #17630.

Note: See TracTickets for help on using tickets.