[wp-trac] [WordPress Trac] #33821: redirect_canonical does not consider port in $compare_original

[WordPress Trac]

#33821: redirect_canonical does not consider port in $compare_original
-------------------------------------------------+-------------------------
 Reporter:  willshouse                           |       Owner:  (none)
     Type:  defect (bug)                         |      Status:  reopened
 Priority:  normal                               |   Milestone:  Future
                                                 |  Release
Component:  Canonical                            |     Version:  2.3
 Severity:  normal                               |  Resolution:
 Keywords:  has-patch needs-unit-tests has-      |     Focuses:
  test-info                                      |
-------------------------------------------------+-------------------------

Comment (by SirLouen):

 Replying to [comment:45 wojtekn]:
 > What if we added a few more unit tests to confirm that behavior for
 those cases doesn't change:
 >
 > - Example.com → example.com (should not redirect)
 > - www.example.com → example.com (should redirect)
 > - example.com → www.example.com (should redirect)

 Yes, that could be useful.

 But here,

 {{{
 /*
          * Ignore differences in host capitalization, as this can lead to
 infinite redirects.
          * Only redirect no-www <=> yes-www.
          */
         if ( $original_host_low === $redirect_host_low
                 || ( 'www.' . $original_host_low !== $redirect_host_low
                         && 'www.' . $redirect_host_low !==
 $original_host_low )
         ) {
                 if ( $original_port === $redirect_port ) {
                         $redirect['host'] = $original['host'];
                 }
         }
 }}}

 What I can interpret is:
 If we redirect
 Example.com:8080 => Example.com:18889
 But then can I get redirected to => example.com:18889
 Leading into a redirect loop? (because there is host `strtolower`
 normalization)

 In case there isn't such risk also from a code review, shouldn't we
 integrate and document it?

 {{{
 /*
  * Ignore differences in host capitalization, as this can lead to infinite
 redirects.
  * Only redirect no-www <=> yes-www.
  * Only when ports are identical, we would rather not rewrite ports.
  */
 if ( $original_host_low === $redirect_host_low
         || ( 'www.' . $original_host_low !== $redirect_host_low
         && 'www.' . $redirect_host_low !== $original_host_low )
         && ( $original_port === $redirect_port ) {
 ) {
                 $redirect['host'] = $original['host'];
         }
 }
 }}}

 So basically in case there could be no problems with the double
 redirection and a future loop, ideally some unit tests covering several
 examples for each scenario would be great to prove this right and conclude
 this ticket.

-- 
Ticket URL: <https://core.trac.wordpress.org/ticket/33821#comment:46>
WordPress Trac <https://core.trac.wordpress.org/>
WordPress publishing platform