From 3a5e71aa2bce61a56e8a026fd8e1fcf67f2bd40f Mon Sep 17 00:00:00 2001 From: Ben Keith Date: Mon, 16 Sep 2024 18:46:45 -0400 Subject: [PATCH 1/4] Add tests for #269 and #380 --- tests/php/test-core.php | 92 +++++++++++++++++++++++++++++++++++++++++ 1 file changed, 92 insertions(+) diff --git a/tests/php/test-core.php b/tests/php/test-core.php index c44a5050..f5902968 100644 --- a/tests/php/test-core.php +++ b/tests/php/test-core.php @@ -682,4 +682,96 @@ function( $requested_path, $redirected_to, $status_code ) use ( &$redirect_to, & remove_filter('srm_match_query_params', '__return_true'); } + /** + * Test that redirection to an external domain works with a regular expression without substitution + * + * @link https://github.com/10up/safe-redirect-manager/issues/269 + * @since 2.1.2 + */ + public function testRedirectToExternalDomainWithNonSubstitutingRegex269() { + $_SERVER['REQUEST_URI'] = '/be/hhhh'; + $redirected = true; + $redirect_to = 'http://xu-osp-plugins.local/404-regex'; + $expected = 'http://xu-osp-plugins.local/404-regex'; + + srm_create_redirect( '(go|be)\/h{0,}$', $redirect_to, 301, true ); + + add_action( + 'srm_do_redirect', + function( $requested_path, $redirected_to, $status_code ) use ( &$redirect_to, &$redirected, &$expected ) { + if ( $redirected_to === $expected ) { + $redirected = true; + } + }, + 10, + 3 + ); + + SRM_Redirect::factory()->maybe_redirect(); + $this->assertTrue( $redirected, 'Expected that a non-substituting regular expression would trigger a redirect to http://xu-osp-plugins.local/404-regex, but instead redirected to ' . $redirect_to ); + } + + /** + * Test that redirection to an external domain works with a regular expression with substitution + * + * @link https://github.com/10up/safe-redirect-manager/issues/380 + * @since 2.2.0 + */ + public function testRedirectToExternalDomainWithSubstitutingRegex380() { + $_SERVER['REQUEST_URI'] = '/test/1234'; + $redirected = true; + $redirect_from = '/test/(.*)'; + $redirect_to = 'http://example.org/$1'; + $expected = 'http://example.org/1234'; + + srm_create_redirect( $redirect_from, $redirect_to, 301, true ); + + add_action( + 'srm_do_redirect', + function( $requested_path, $redirected_to, $status_code ) use ( &$redirect_to, &$redirected, &$expected ) { + if ( $redirected_to === $expected ) { + $redirected = true; + } else { + $redirect_to = $redirected_to; + } + }, + 10, + 3 + ); + + SRM_Redirect::factory()->maybe_redirect(); + $this->assertTrue( $redirected, 'Expected that a substituting regular expression would trigger a redirect to http://example.org/1234, but instead redirected to ' . $redirect_to ); + } + + /** + * Test that redirection to a local URL works with a regular expression with substitution + * + * @link https://github.com/10up/safe-redirect-manager/issues/380 + * @since 2.2.0 + */ + public function testRedirectToPathWithSubstitutingRegex380() { + $_SERVER['REQUEST_URI'] = '/test/1234'; + $redirected = true; + $redirect_from = '/test/(.*)'; + $redirect_to = '/result/$1'; + $expected = '/result/1234'; + + srm_create_redirect( $redirect_from, $redirect_to, 301, true ); + + add_action( + 'srm_do_redirect', + function( $requested_path, $redirected_to, $status_code ) use ( &$redirect_to, &$redirected, &$expected ) { + if ( $redirected_to === $expected ) { + $redirected = true; + } else { + $redirect_to = $redirected_to; + } + }, + 10, + 3 + ); + + SRM_Redirect::factory()->maybe_redirect(); + $this->assertTrue( $redirected, 'Expected that a substituting regular expression would trigger a redirect to /result/1234, but instead redirected to ' . $redirect_to ); + } } From 30c4c84d764a01481a5a18248aa5b9213bbfb77a Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Tue, 17 Sep 2024 10:04:15 +1000 Subject: [PATCH 2/4] Update tests to ensure failure on develop. --- tests/php/test-core.php | 80 ++++++++++++++++++++++++++--------------- 1 file changed, 52 insertions(+), 28 deletions(-) diff --git a/tests/php/test-core.php b/tests/php/test-core.php index f5902968..0c2c131c 100644 --- a/tests/php/test-core.php +++ b/tests/php/test-core.php @@ -690,25 +690,35 @@ function( $requested_path, $redirected_to, $status_code ) use ( &$redirect_to, & */ public function testRedirectToExternalDomainWithNonSubstitutingRegex269() { $_SERVER['REQUEST_URI'] = '/be/hhhh'; - $redirected = true; + $redirect_from = '(go|be)\/h{0,}$'; $redirect_to = 'http://xu-osp-plugins.local/404-regex'; - $expected = 'http://xu-osp-plugins.local/404-regex'; + $status = 301; + $use_regex = true; - srm_create_redirect( '(go|be)\/h{0,}$', $redirect_to, 301, true ); + $expected_redirect = 'http://xu-osp-plugins.local/404-regex'; + $expected_status = 301; + + $actual_request = ''; + $actual_redirect = ''; + $actual_status = 0; + + srm_create_redirect( $redirect_from, $redirect_to, $status, $use_regex ); add_action( 'srm_do_redirect', - function( $requested_path, $redirected_to, $status_code ) use ( &$redirect_to, &$redirected, &$expected ) { - if ( $redirected_to === $expected ) { - $redirected = true; - } + function( $requested_path, $redirected_to, $status_code ) use ( &$actual_request, &$actual_redirect, &$actual_status ) { + $actual_request = $requested_path; + $actual_redirect = $redirected_to; + $actual_status = $status_code; }, 10, 3 ); SRM_Redirect::factory()->maybe_redirect(); - $this->assertTrue( $redirected, 'Expected that a non-substituting regular expression would trigger a redirect to http://xu-osp-plugins.local/404-regex, but instead redirected to ' . $redirect_to ); + $this->assertSame( $_SERVER['REQUEST_URI'], $actual_request, 'The requested path does not meet the expectation' ); + $this->assertSame( $expected_redirect, $actual_redirect, 'The redirect destination does not meet the expectation' ); + $this->assertSame( $expected_status, $actual_status, 'The redirect status does npt meet the expectation.' ); } /** @@ -719,28 +729,35 @@ function( $requested_path, $redirected_to, $status_code ) use ( &$redirect_to, & */ public function testRedirectToExternalDomainWithSubstitutingRegex380() { $_SERVER['REQUEST_URI'] = '/test/1234'; - $redirected = true; $redirect_from = '/test/(.*)'; $redirect_to = 'http://example.org/$1'; - $expected = 'http://example.org/1234'; + $status = 301; + $use_regex = true; + + $expected_redirect = 'http://example.org/1234'; + $expected_status = 301; - srm_create_redirect( $redirect_from, $redirect_to, 301, true ); + $actual_request = ''; + $actual_redirect = ''; + $actual_status = 0; + + srm_create_redirect( $redirect_from, $redirect_to, $status, $use_regex ); add_action( 'srm_do_redirect', - function( $requested_path, $redirected_to, $status_code ) use ( &$redirect_to, &$redirected, &$expected ) { - if ( $redirected_to === $expected ) { - $redirected = true; - } else { - $redirect_to = $redirected_to; - } + function( $requested_path, $redirected_to, $status_code ) use ( &$actual_request, &$actual_redirect, &$actual_status ) { + $actual_request = $requested_path; + $actual_redirect = $redirected_to; + $actual_status = $status_code; }, 10, 3 ); SRM_Redirect::factory()->maybe_redirect(); - $this->assertTrue( $redirected, 'Expected that a substituting regular expression would trigger a redirect to http://example.org/1234, but instead redirected to ' . $redirect_to ); + $this->assertSame( $_SERVER['REQUEST_URI'], $actual_request, 'The requested path does not meet the expectation' ); + $this->assertSame( $expected_redirect, $actual_redirect, 'The redirect destination does not meet the expectation' ); + $this->assertSame( $expected_status, $actual_status, 'The redirect status does npt meet the expectation.' ); } /** @@ -751,27 +768,34 @@ function( $requested_path, $redirected_to, $status_code ) use ( &$redirect_to, & */ public function testRedirectToPathWithSubstitutingRegex380() { $_SERVER['REQUEST_URI'] = '/test/1234'; - $redirected = true; $redirect_from = '/test/(.*)'; $redirect_to = '/result/$1'; - $expected = '/result/1234'; + $status = 301; + $use_regex = true; + + $expected_redirect = '/result/1234'; + $expected_status = 301; + + $actual_request = ''; + $actual_redirect = ''; + $actual_status = 0; - srm_create_redirect( $redirect_from, $redirect_to, 301, true ); + srm_create_redirect( $redirect_from, $redirect_to, $status, $use_regex ); add_action( 'srm_do_redirect', - function( $requested_path, $redirected_to, $status_code ) use ( &$redirect_to, &$redirected, &$expected ) { - if ( $redirected_to === $expected ) { - $redirected = true; - } else { - $redirect_to = $redirected_to; - } + function( $requested_path, $redirected_to, $status_code ) use ( &$actual_request, &$actual_redirect, &$actual_status ) { + $actual_request = $requested_path; + $actual_redirect = $redirected_to; + $actual_status = $status_code; }, 10, 3 ); SRM_Redirect::factory()->maybe_redirect(); - $this->assertTrue( $redirected, 'Expected that a substituting regular expression would trigger a redirect to /result/1234, but instead redirected to ' . $redirect_to ); + $this->assertSame( $_SERVER['REQUEST_URI'], $actual_request, 'The requested path does not meet the expectation' ); + $this->assertSame( $expected_redirect, $actual_redirect, 'The redirect destination does not meet the expectation' ); + $this->assertSame( $expected_status, $actual_status, 'The redirect status does npt meet the expectation.' ); } } From 5e7bb7ea620ee7b823beced7984b96c64abbe8d7 Mon Sep 17 00:00:00 2001 From: Ben Keith Date: Mon, 16 Sep 2024 18:14:18 -0400 Subject: [PATCH 3/4] Allow regular expression parsing if the redirect target is a valid URL Fixes https://github.com/10up/safe-redirect-manager/issues/380 --- inc/classes/class-srm-redirect.php | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-) diff --git a/inc/classes/class-srm-redirect.php b/inc/classes/class-srm-redirect.php index ceb69745..b22abbbf 100644 --- a/inc/classes/class-srm-redirect.php +++ b/inc/classes/class-srm-redirect.php @@ -176,8 +176,10 @@ public function match_redirect( $requested_path ) { // check if requested path is the same as the redirect from path if ( $enable_regex ) { + // for regexes, check whether the requested path matches the $redirect_from regular expression $match_query_params = false; $matched_path = preg_match( '@' . $redirect_from . '@' . $regex_flag, $requested_path ); + // and then return the matching path } else { if ( $case_insensitive ) { $redirect_from = strtolower( $redirect_from ); @@ -207,6 +209,8 @@ public function match_redirect( $requested_path ) { } } + // If the requested path matches a redirect rule... + // this variable is not used after this boolean. if ( $matched_path ) { /** * Whitelist redirect host @@ -223,9 +227,14 @@ public function match_redirect( $requested_path ) { } // Allow for regex replacement in $redirect_to - if ( $enable_regex && ! filter_var( $redirect_to, FILTER_VALIDATE_URL ) ) { + if ( $enable_regex ) { $redirect_to = preg_replace( '@' . $redirect_from . '@' . $regex_flag, $redirect_to, $requested_path ); - $redirect_to = '/' . ltrim( $redirect_to, '/' ); + + // If $redirect_to does not look like a valid URL, + // assume that it needs to be turned into a path relative to root with a leading slash. + if ( ! filter_var( $redirect_to, FILTER_VALIDATE_URL ) ) { + $redirect_to = '/' . ltrim( $redirect_to, '/' ); + } } // re-add the query params if they've not already been added by the wildcard From aba231be8f754157c14fcfc9de5687a7dce37069 Mon Sep 17 00:00:00 2001 From: Peter Wilson Date: Tue, 17 Sep 2024 10:27:06 +1000 Subject: [PATCH 4/4] CS: Whitespace fixes. --- tests/php/test-core.php | 18 +++++++++--------- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/tests/php/test-core.php b/tests/php/test-core.php index 0c2c131c..4553a6c8 100644 --- a/tests/php/test-core.php +++ b/tests/php/test-core.php @@ -698,7 +698,7 @@ public function testRedirectToExternalDomainWithNonSubstitutingRegex269() { $expected_redirect = 'http://xu-osp-plugins.local/404-regex'; $expected_status = 301; - $actual_request = ''; + $actual_request = ''; $actual_redirect = ''; $actual_status = 0; @@ -707,9 +707,9 @@ public function testRedirectToExternalDomainWithNonSubstitutingRegex269() { add_action( 'srm_do_redirect', function( $requested_path, $redirected_to, $status_code ) use ( &$actual_request, &$actual_redirect, &$actual_status ) { - $actual_request = $requested_path; + $actual_request = $requested_path; $actual_redirect = $redirected_to; - $actual_status = $status_code; + $actual_status = $status_code; }, 10, 3 @@ -737,7 +737,7 @@ public function testRedirectToExternalDomainWithSubstitutingRegex380() { $expected_redirect = 'http://example.org/1234'; $expected_status = 301; - $actual_request = ''; + $actual_request = ''; $actual_redirect = ''; $actual_status = 0; @@ -746,9 +746,9 @@ public function testRedirectToExternalDomainWithSubstitutingRegex380() { add_action( 'srm_do_redirect', function( $requested_path, $redirected_to, $status_code ) use ( &$actual_request, &$actual_redirect, &$actual_status ) { - $actual_request = $requested_path; + $actual_request = $requested_path; $actual_redirect = $redirected_to; - $actual_status = $status_code; + $actual_status = $status_code; }, 10, 3 @@ -776,7 +776,7 @@ public function testRedirectToPathWithSubstitutingRegex380() { $expected_redirect = '/result/1234'; $expected_status = 301; - $actual_request = ''; + $actual_request = ''; $actual_redirect = ''; $actual_status = 0; @@ -785,9 +785,9 @@ public function testRedirectToPathWithSubstitutingRegex380() { add_action( 'srm_do_redirect', function( $requested_path, $redirected_to, $status_code ) use ( &$actual_request, &$actual_redirect, &$actual_status ) { - $actual_request = $requested_path; + $actual_request = $requested_path; $actual_redirect = $redirected_to; - $actual_status = $status_code; + $actual_status = $status_code; }, 10, 3