Skip to content

Commit b6eebd5

Browse files
fix[faustwp]: (#1872) use home_url() in handle_generate_endpoint (#2339)
* fix[faustwp]: (#1872) use home_url() in handle_generate_endpoint On Bedrock-style installs where WordPress core lives under /wp/, site_url('/generate') returns https://example.com/wp/generate but $_SERVER['REQUEST_URI'] is still /generate, so the preg_match in handle_generate_endpoint() never fires and the auth-code redirect silently breaks. home_url() returns the public-facing URL (what REQUEST_URI actually contains), which matches on both standard and Bedrock installs. Per @josephfusco's guidance on #2315, which was closed in favor of this simpler fix — a new filter is not needed, just the correct URL helper. * test[faustwp]: regression test for handle_generate_endpoint() Bedrock case (#1872) Adds AuthCallbacksTests.php covering handle_generate_endpoint() with four scenarios: - Standard install reaches wp_safe_redirect (login redirect path). - Bedrock layout (site_url=/wp/x, home_url=/x) still matches REQUEST_URI=/x. This is the regression guard against future reverts to site_url(): verified to fail when callbacks.php is reverted to site_url() and pass with home_url(). - Unrelated REQUEST_URI early-returns without redirecting. - /generate without redirect_uri early-returns. Uses Patchwork to redefine wp_safe_redirect so the bare 'exit;' is bypassed and the captured redirect target can be asserted. Patchwork is already loaded via phpunit.xml.dist's LOAD_PATCHWORK=1. Also adds tests/mu-plugins/mu-bedrock-simulator.php -- a tiny mu-plugin that makes site_url() return /wp/<path> while home_url() stays at /<path>, for manual browser reproduction inside the docker-compose stack without requiring a full roots/bedrock install. * test[faustwp]: tighten AuthCallbacks regression test fidelity Peer-review polish on the earlier test commit: - Use update_option('siteurl', ...) instead of an add_filter('site_url') to drive Bedrock-style URL divergence. This matches what real Bedrock configures via WP_SITEURL in wp-config.php, so every consumer of get_option('siteurl') and site_url() sees the divergent value, not just callers that go through the site_url filter. Removes the remove_all_filters() tearDown that nuked unrelated core filters. - Replace the RuntimeException + magic-string catch trick with a dedicated RedirectAttempted exception class. Eliminates the risk of accidentally swallowing unrelated runtime errors during the redirect-capture flow. - Tighten assertions on the Bedrock-divergence test: now also asserts the captured redirect contains 'wp-login.php' (matching the standard-install test) and adds two sanity-check assertions that confirm the simulated divergence is actually in place before the handler is invoked. Total assertions in the suite increase from 5 to 8. - Document the simulator mu-plugin's narrower scope vs the test's full fidelity: the mu-plugin filters site_url() output (sufficient for browser reproduction), while the PHPUnit suite updates the option directly. Verified via temporary revert of callbacks.php to site_url(): the Bedrock divergence test now fails with the polished message; restoring home_url() greens the suite on both single-site and multisite. * test[faustwp]: widen AuthCallbacks coverage and document home_url() filter caveat Three small additions on top of the regression-test commits: 1. Assert that the redirect_uri query arg is preserved through to the wp-login redirect_to param, in both the standard and Bedrock cases. A future change that successfully matched the request but mangled or dropped redirect_uri would previously slip through. 2. Add test_wp_in_subdirectory_install_matches_with_home_url for the Codex-documented 'Giving WordPress its own directory' pattern -- WP core in /wordpress, public site at root. Different prefix from Bedrock, same shape of divergence. Proves the fix is not a /wp-special case. Renames set_bedrock_siteurl() to set_split_install_siteurl($suffix) so both tests share the helper. 3. Document the home_url() filter dependency in handle_generate_endpoint()'s docblock. Multilingual plugins (WPML, Polylang, TranslatePress) that filter home_url() to prepend locale paths can still cause this match to miss for non-locale-prefixed frontend callers. Notes the likely follow-up direction (REST route migration) so the next person to touch this knows what they're inheriting. Suite is now 5 tests / 14 assertions. Verified red-on-revert: both the Bedrock and WP-in-subdirectory tests fail cleanly when callbacks.php is reverted to site_url(); restoring home_url() greens both single-site and multisite suites. --------- Co-authored-by: latenighthackathon <latenighthackathon@users.noreply.github.com> Co-authored-by: Joe Fusco <hello@josephfus.co>
1 parent 6b71092 commit b6eebd5

4 files changed

Lines changed: 311 additions & 1 deletion

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"@faustwp/wordpress-plugin": patch
3+
---
4+
5+
fix[faustwp]: use home_url() in handle_generate_endpoint so Bedrock-style installs (where WordPress core lives under /wp/) match against the public REQUEST_URI

plugins/faustwp/includes/auth/callbacks.php

Lines changed: 8 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -19,10 +19,17 @@
1919
*
2020
* Generate an authorization code and redirect to the requested url.
2121
*
22+
* Note: matches REQUEST_URI against the filtered output of home_url(). Plugins
23+
* that filter home_url() to prepend locale paths (WPML, Polylang, TranslatePress)
24+
* may cause this match to fail when the frontend calls '/generate' directly
25+
* without a locale prefix. If that combination surfaces in support, the
26+
* follow-up will likely migrate '/generate' to a proper REST route so locale
27+
* filters cannot affect the match.
28+
*
2229
* @return void
2330
*/
2431
function handle_generate_endpoint() {
25-
$search_pattern = ':^' . site_url( '/generate', 'relative' ) . ':';
32+
$search_pattern = ':^' . home_url( '/generate', 'relative' ) . ':';
2633

2734
if ( ! preg_match( $search_pattern, $_SERVER['REQUEST_URI'] ) ) { // phpcs:ignore WordPress.Security
2835
return;
Lines changed: 240 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,240 @@
1+
<?php
2+
/**
3+
* Integration tests for plugins/faustwp/includes/auth/callbacks.php.
4+
*
5+
* @package FaustWP
6+
*/
7+
8+
namespace WPE\FaustWP\Tests\Integration;
9+
10+
use WPE\FaustWP\Auth;
11+
12+
/**
13+
* Regression tests for handle_generate_endpoint().
14+
*
15+
* Guards the home_url() vs site_url() behavior in handle_generate_endpoint() so
16+
* the fix for #1872 -- Bedrock-style installs where WordPress core lives under
17+
* /wp/ -- cannot be silently reverted.
18+
*
19+
* The Bedrock case is reproduced by overriding the `siteurl` option directly
20+
* (matching what Bedrock configures via WP_SITEURL in wp-config.php) rather
21+
* than filtering `site_url`. That way every consumer of `get_option('siteurl')`
22+
* and `site_url()` sees the divergent value, which is faithful to real Bedrock.
23+
*
24+
* @group auth
25+
*/
26+
class AuthCallbacksTests extends \WP_UnitTestCase {
27+
28+
/**
29+
* Redirect URL captured by the Patchwork-redefined wp_safe_redirect.
30+
*
31+
* Static so the redefine closure can write to it without binding $this.
32+
*
33+
* @var string|null
34+
*/
35+
public static $captured_redirect = null;
36+
37+
/**
38+
* Original siteurl option value, restored in tearDown so tests stay isolated
39+
* even if WP_UnitTestCase's transaction rollback misses something.
40+
*
41+
* @var string
42+
*/
43+
private $original_siteurl = '';
44+
45+
/**
46+
* Snapshots of superglobal values taken in setUp and restored in tearDown.
47+
*
48+
* `phpunit.xml.dist` sets backupGlobals="false" so anything we mutate on
49+
* $_SERVER / $_GET persists across test classes. The WP test bootstrap and
50+
* other test classes downstream of this one (e.g. TelemetryCallbacksTests,
51+
* which triggers wp-cron) depend on REQUEST_URI being defined. Snapshot it
52+
* here so our tearDown leaves the world exactly as we found it.
53+
*
54+
* @var array{request_uri: string|null, get: array<string, mixed>}
55+
*/
56+
private $original_globals = array(
57+
'request_uri' => null,
58+
'get' => array(),
59+
);
60+
61+
public function setUp(): void {
62+
parent::setUp();
63+
64+
self::$captured_redirect = null;
65+
$this->original_siteurl = get_option( 'siteurl' );
66+
$this->original_globals = array(
67+
'request_uri' => isset( $_SERVER['REQUEST_URI'] ) ? (string) $_SERVER['REQUEST_URI'] : null,
68+
'get' => $_GET,
69+
);
70+
71+
// handle_generate_endpoint() calls wp_safe_redirect() and then a bare exit;.
72+
// Redefine wp_safe_redirect via Patchwork to throw a dedicated exception,
73+
// which bypasses the exit and lets us assert which redirect target was
74+
// reached. A dedicated exception class avoids the string-matching trap of
75+
// a generic RuntimeException (which could collide with unrelated errors).
76+
\Patchwork\redefine(
77+
'wp_safe_redirect',
78+
static function ( $location ) {
79+
AuthCallbacksTests::$captured_redirect = $location;
80+
throw new RedirectAttempted( (string) $location );
81+
}
82+
);
83+
}
84+
85+
public function tearDown(): void {
86+
\Patchwork\restoreAll();
87+
update_option( 'siteurl', $this->original_siteurl );
88+
89+
// Restore superglobals to whatever the WP test bootstrap (or a prior test
90+
// class) had them at. Unsetting REQUEST_URI here would break the cron path
91+
// in subsequent test classes -- it leaked into TelemetryCallbacksTests on CI.
92+
if ( null === $this->original_globals['request_uri'] ) {
93+
unset( $_SERVER['REQUEST_URI'] );
94+
} else {
95+
$_SERVER['REQUEST_URI'] = $this->original_globals['request_uri'];
96+
}
97+
$_GET = $this->original_globals['get'];
98+
99+
self::$captured_redirect = null;
100+
parent::tearDown();
101+
}
102+
103+
/**
104+
* Reconfigure the WordPress siteurl option to a split-install layout where WP
105+
* core lives in a subdirectory of the public site.
106+
*
107+
* Default suffix '/wp' mirrors Bedrock (what `composer create-project roots/bedrock`
108+
* sets via WP_SITEURL in wp-config.php). Pass '/wordpress' for the Codex-documented
109+
* "Giving WordPress its own directory" pattern, or any other prefix to exercise
110+
* other split-install configurations.
111+
*
112+
* @param string $suffix Path appended to the home option to form siteurl. Defaults to '/wp'.
113+
*/
114+
private function set_split_install_siteurl( string $suffix = '/wp' ): void {
115+
$home = (string) get_option( 'home' );
116+
update_option( 'siteurl', rtrim( $home, '/' ) . $suffix );
117+
}
118+
119+
/**
120+
* Invoke handle_generate_endpoint() and return the captured wp_safe_redirect
121+
* target, or null if the function returned before reaching the redirect.
122+
*
123+
* @return string|null
124+
*/
125+
private function invoke_handler() {
126+
try {
127+
Auth\handle_generate_endpoint();
128+
} catch ( RedirectAttempted $e ) {
129+
return self::$captured_redirect;
130+
}
131+
return null;
132+
}
133+
134+
/**
135+
* Standard install: REQUEST_URI matches the default search pattern; the
136+
* function proceeds to wp_safe_redirect (login-redirect branch). The
137+
* redirect_uri query arg from the original request must be carried through
138+
* into the wp-login redirect_to param so the user lands back at the right
139+
* frontend after authenticating.
140+
*/
141+
public function test_standard_install_matches_and_redirects(): void {
142+
$_SERVER['REQUEST_URI'] = '/generate?redirect_uri=https://frontend.example/';
143+
$_GET['redirect_uri'] = 'https://frontend.example/';
144+
145+
$redirect = $this->invoke_handler();
146+
147+
$this->assertNotNull( $redirect, 'Standard install must reach wp_safe_redirect.' );
148+
$this->assertStringContainsString( 'wp-login.php', $redirect );
149+
$this->assertStringContainsString(
150+
'frontend.example',
151+
$redirect,
152+
'redirect_uri value must be preserved in the wp-login redirect_to param.'
153+
);
154+
}
155+
156+
/**
157+
* Bedrock-shaped install: the siteurl option includes /wp while home does not.
158+
* With home_url() in callbacks.php, REQUEST_URI=/generate still matches.
159+
*
160+
* Regression guard for #1872: this test fails if callbacks.php is reverted
161+
* to site_url() because the regex becomes /wp/generate and stops matching
162+
* the public REQUEST_URI.
163+
*/
164+
public function test_bedrock_divergence_matches_with_home_url(): void {
165+
$this->set_split_install_siteurl( '/wp' );
166+
167+
// Sanity-check the divergence we just configured: site_url carries /wp,
168+
// home_url does not. If these fail, the test environment itself is broken
169+
// before we even exercise the handler.
170+
$this->assertStringEndsWith( '/wp/generate', site_url( '/generate', 'relative' ) );
171+
$this->assertStringEndsWith( '/generate', home_url( '/generate', 'relative' ) );
172+
173+
$_SERVER['REQUEST_URI'] = '/generate?redirect_uri=https://frontend.example/';
174+
$_GET['redirect_uri'] = 'https://frontend.example/';
175+
176+
$redirect = $this->invoke_handler();
177+
178+
$this->assertNotNull(
179+
$redirect,
180+
'Bedrock layout (siteurl includes /wp, home does not) must still match REQUEST_URI=/generate when home_url() is used.'
181+
);
182+
$this->assertStringContainsString( 'wp-login.php', $redirect );
183+
$this->assertStringContainsString( 'frontend.example', $redirect );
184+
}
185+
186+
/**
187+
* "Giving WordPress its own directory" install: the Codex-documented pattern
188+
* where WP core is installed in a subdirectory (e.g. /wordpress) but the
189+
* site is served from the public root. Different prefix from Bedrock, same
190+
* shape of divergence -- proves the fix generalises beyond '/wp' specifically.
191+
*
192+
* Ref: https://wordpress.org/documentation/article/giving-wordpress-its-own-directory/
193+
*/
194+
public function test_wp_in_subdirectory_install_matches_with_home_url(): void {
195+
$this->set_split_install_siteurl( '/wordpress' );
196+
197+
$this->assertStringEndsWith( '/wordpress/generate', site_url( '/generate', 'relative' ) );
198+
$this->assertStringEndsWith( '/generate', home_url( '/generate', 'relative' ) );
199+
200+
$_SERVER['REQUEST_URI'] = '/generate?redirect_uri=https://frontend.example/';
201+
$_GET['redirect_uri'] = 'https://frontend.example/';
202+
203+
$redirect = $this->invoke_handler();
204+
205+
$this->assertNotNull(
206+
$redirect,
207+
'WP-in-subdirectory layout (siteurl includes /wordpress, home does not) must still match REQUEST_URI=/generate.'
208+
);
209+
$this->assertStringContainsString( 'wp-login.php', $redirect );
210+
}
211+
212+
/**
213+
* Unrelated REQUEST_URI must early-return without touching wp_safe_redirect.
214+
*/
215+
public function test_unrelated_request_uri_is_a_noop(): void {
216+
$_SERVER['REQUEST_URI'] = '/some-other-path';
217+
$_GET = array();
218+
219+
$this->assertNull( $this->invoke_handler() );
220+
}
221+
222+
/**
223+
* /generate without a redirect_uri query arg must early-return.
224+
*/
225+
public function test_generate_without_redirect_uri_is_a_noop(): void {
226+
$_SERVER['REQUEST_URI'] = '/generate';
227+
$_GET = array();
228+
229+
$this->assertNull( $this->invoke_handler() );
230+
}
231+
}
232+
233+
/**
234+
* Thrown by the Patchwork redefine of wp_safe_redirect inside AuthCallbacksTests
235+
* to bypass the bare `exit;` that immediately follows wp_safe_redirect() in
236+
* handle_generate_endpoint(). Keeping this as a dedicated subclass (rather than
237+
* a generic RuntimeException with a magic string) means the catch in
238+
* invoke_handler() can't accidentally swallow unrelated runtime errors.
239+
*/
240+
class RedirectAttempted extends \Exception {}
Lines changed: 58 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,58 @@
1+
<?php
2+
/**
3+
* Bedrock-style site_url() simulator -- manual-verification helper for #1872.
4+
*
5+
* Drop this file into wp-content/mu-plugins/ inside the docker-compose.yml stack
6+
* to make site_url() return /wp/<path> while home_url() stays at /<path>. This
7+
* mirrors the URL divergence of a Bedrock-style WordPress layout without
8+
* requiring a full `composer create-project roots/bedrock` install.
9+
*
10+
* Scope and faithfulness:
11+
*
12+
* This mu-plugin filters the OUTPUT of site_url(). It does NOT change the
13+
* underlying `siteurl` option value. That is sufficient for end-to-end browser
14+
* verification of #1872, where the only thing that matters is the regex match
15+
* inside handle_generate_endpoint().
16+
*
17+
* For test-time fidelity that more closely matches a real Bedrock configuration
18+
* (where every consumer of `get_option('siteurl')` and `site_url()` sees the
19+
* divergent value), the PHPUnit suite updates the siteurl option directly --
20+
* see tests/integration/AuthCallbacksTests.php::set_bedrock_siteurl().
21+
*
22+
* Activate inside the Docker container:
23+
*
24+
* docker compose -f plugins/faustwp/docker-compose.yml exec wordpress sh -c \
25+
* 'mkdir -p /var/www/html/wp-content/mu-plugins \
26+
* && cp /var/www/html/wp-content/plugins/faustwp/tests/mu-plugins/mu-bedrock-simulator.php /var/www/html/wp-content/mu-plugins/'
27+
*
28+
* Then visit:
29+
*
30+
* http://localhost:8080/generate?redirect_uri=https://example.test/
31+
*
32+
* - canary (pre-fix): handler early-returns; WordPress's normal routing kicks in
33+
* - this branch (post-fix): handler matches, redirects to wp-login.php
34+
*
35+
* Not loaded by PHPUnit (the testsuite config in phpunit.xml.dist only scans
36+
* ./tests/integration/ and ./tests/unit/). This file exists for manual browser
37+
* reproduction only.
38+
*
39+
* @package FaustWP\Tests
40+
*/
41+
42+
if ( ! defined( 'ABSPATH' ) ) {
43+
exit;
44+
}
45+
46+
add_filter(
47+
'site_url',
48+
static function ( $url ) {
49+
// Relative URL (scheme='relative'): /<path> -> /wp/<path>
50+
if ( '' !== $url && '/' === $url[0] && ( ! isset( $url[1] ) || '/' !== $url[1] ) ) {
51+
return '/wp' . $url;
52+
}
53+
// Absolute URL: <scheme>://<host>/<path> -> <scheme>://<host>/wp/<path>
54+
return preg_replace( '#(https?://[^/]+)(/.*)?#', '$1/wp$2', $url );
55+
},
56+
10,
57+
1
58+
);

0 commit comments

Comments
 (0)