From 5f646210f9dc45ef6de08c7b0156f10ec6b6bd55 Mon Sep 17 00:00:00 2001 From: loukieluke <84424020+loukieluke@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:27:59 -0500 Subject: [PATCH 1/8] ENG-7125: Defer Basic Auth logout handling to `init` wp_logout() fires the `wp_logout` action, whose subscribers may rely on constants their own plugin defines in a `plugins_loaded` callback. Calling it from this plugin's own `plugins_loaded` priority-1 callback races that setup, and User Switching's `wp_logout` subscriber fatals on its own not-yet-defined cookie constants whenever this plugin wins the race. Reproduced against WordPress 7.1 / PHP 8.3 with Basic Auth 1.0.2 and User Switching 1.12.2: an anonymous `GET /?basic-auth-logout=1` returns a hard 500 (`Undefined constant "USER_SWITCHING_SECURE_COOKIE"`), with no credentials required. 93 production sites have both plugins active. Enforcement stays on `plugins_loaded` priority 1 -- only the logout moves. Moving all of init() would let other plugins' `plugins_loaded` callbacks run unauthenticated on a gated site. Two consequences worth noting: - Requests with no credentials now stop at the auth challenge and never reach wp_logout(), closing the unauthenticated CSRF/DoS trigger. - maybe_redirect_from_login_page() now runs before the logout, so it early-returns on a logout request; without that guard a logout URL lacking `action=logout` would redirect to the home page still logged in. Co-Authored-By: Claude Opus 5 (1M context) --- pressable-basic-authentication.php | 72 +++++++++++++++++++++--------- readme.txt | 2 +- 2 files changed, 52 insertions(+), 22 deletions(-) diff --git a/pressable-basic-authentication.php b/pressable-basic-authentication.php index 22cee88..64bb894 100644 --- a/pressable-basic-authentication.php +++ b/pressable-basic-authentication.php @@ -8,7 +8,7 @@ /* Plugin Name: Hosting Basic Authentication Description: Forces all users to authenticate using Basic Authentication before accessing any page. -Version: 1.0.2 +Version: 1.0.3 License: GPL2 Text Domain: hosting-basic-authentication */ @@ -30,6 +30,18 @@ public function __construct() { // Hook into WordPress before anything is outputted. add_action( 'plugins_loaded', array( $this, 'init' ), 1 ); + // Logout is handled on `init`, deliberately later than the rest of `init()`. + // wp_logout() fires the `wp_logout` action, and its subscribers may rely on + // constants their own plugin defines in a `plugins_loaded` callback. Firing it + // from `plugins_loaded` priority 1 races that setup, and which plugin wins the + // race depends on load order -- `active_plugins` ordering, anything filtering + // it, and network-activated plugins, which load earlier still. User Switching + // defines its cookie constants that way, so wherever this plugin happens to run + // first, User Switching's `wp_logout` subscriber fatals on a constant it has + // not defined yet. Hooking to `init` drops the dependency on load order + // entirely: every `plugins_loaded` callback has completed by then. + add_action( 'init', array( $this, 'handle_logout_request' ), 1 ); + // Add filter for logout URL. add_filter( 'logout_url', array( $this, 'modify_logout_url' ), 10, 2 ); @@ -41,36 +53,45 @@ public function __construct() { * Initialize the plugin */ public function init() { - // Skip if we're doing AJAX. - if ( $this->is_ajax_request() ) { + if ( $this->skip_request() ) { return; } - // Skip if we're doing CRON. - if ( $this->is_cron_request() ) { - return; - } + // Redirect from wp-login.php when already authenticated via Basic Auth + $this->maybe_redirect_from_login_page(); - // Skip if we're in CLI mode. - if ( $this->is_cli_request() ) { + // Force authentication. + $this->force_basic_authentication(); + } + + /** + * Handles the Basic Auth logout request. + * + * Hooked to `init` rather than running with the rest of init() on + * `plugins_loaded` -- see the hook registration in the constructor for why. + */ + public function handle_logout_request() { + if ( $this->skip_request() ) { return; } - // Skip requests to excluded endpoints - if ($this->should_skip_auth()) { - return; - } - - // Handle logout request. - if ( isset( $_GET['basic-auth-logout'] ) ) { - $this->handle_basic_auth_logout(); + if ( ! isset( $_GET['basic-auth-logout'] ) ) { + return; } - // Redirect from wp-login.php when already authenticated via Basic Auth - $this->maybe_redirect_from_login_page(); + $this->handle_basic_auth_logout(); + } - // Force authentication. - $this->force_basic_authentication(); + /** + * Whether this request is outside the scope of Basic Authentication. + * + * @return bool + */ + private function skip_request() { + return $this->is_ajax_request() + || $this->is_cron_request() + || $this->is_cli_request() + || $this->should_skip_auth(); } /** @@ -270,6 +291,15 @@ public function modify_logout_url( $logout_url, $redirect ) { public function maybe_redirect_from_login_page() { global $pagenow; + // A request that asks to log out is never redirected away from the logout. This + // guard only matters on wp-login.php, and only for a logout URL that omits + // `action=logout` -- the URL modify_logout_url() builds always carries it. Since + // the logout moved to `init`, this method now runs first, and without the guard + // such a request would redirect to the home page still logged in, with no error. + if ( isset( $_GET['basic-auth-logout'] ) ) { + return; + } + // Check if we're on the login page and have Basic Auth credentials if ( 'wp-login.php' === $pagenow && ! empty( $_SERVER['PHP_AUTH_USER'] ) && diff --git a/readme.txt b/readme.txt index 9f457d5..4aca433 100644 --- a/readme.txt +++ b/readme.txt @@ -4,7 +4,7 @@ Tags: pressable, basic auth, authentication, security Requires at least: 6.7 Tested up to: 6.8 Requires PHP: 8.1 -Stable tag: 1.0.2 +Stable tag: 1.0.3 License: GPLv2 or later License URI: http://www.gnu.org/licenses/gpl-2.0.html From be74abd6c56073b5bdf19d97d476bd94446a0bfa Mon Sep 17 00:00:00 2001 From: loukieluke <84424020+loukieluke@users.noreply.github.com> Date: Fri, 11 Sep 2026 17:27:59 -0500 Subject: [PATCH 2/8] ENG-7125: Add hook-registration regression test and CI Pins the hook wiring the fix depends on: the logout handler is registered on `init`, init() stays on `plugins_loaded` priority 1 so enforcement cannot silently drift late, init() never invokes the logout handler, and the handler is public as a hook callback must be. Dependency-free by design -- the repo has no composer/PHPUnit setup, and asserting hook wiring rather than request behaviour needs neither WordPress nor a database. The logout method name is asserted from both sides, matching on the trailing "(": a negative check alone would pass vacuously if the method were renamed, silently losing the coverage the test exists for. The test guards on php_sapi_name(): it defines ABSPATH itself, so the usual `defined( 'ABSPATH' ) || exit` idiom cannot protect it, and it sits in a directory the web server serves directly. Without the guard it answered 200 on a site where Basic Auth returns 401 for everything else. `/tests export-ignore` keeps it out of the release zip. Co-Authored-By: Claude Opus 5 (1M context) --- .gitattributes | 4 +- .github/workflows/test.yml | 25 ++++++ tests/hook-registration-test.php | 137 +++++++++++++++++++++++++++++++ 3 files changed, 165 insertions(+), 1 deletion(-) create mode 100644 .github/workflows/test.yml create mode 100644 tests/hook-registration-test.php diff --git a/.gitattributes b/.gitattributes index 28a7274..ac24691 100644 --- a/.gitattributes +++ b/.gitattributes @@ -14,4 +14,6 @@ /README.md export-ignore /composer.json export-ignore /composer.lock export-ignore -/.DS_Store export-ignore \ No newline at end of file +/.DS_Store export-ignore +# Tests are not part of the distributed plugin. +/tests export-ignore diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..30ec83f --- /dev/null +++ b/.github/workflows/test.yml @@ -0,0 +1,25 @@ +name: Tests + +on: + pull_request: + push: + branches: + - main + +jobs: + test: + name: Lint and hook registration + runs-on: ubuntu-latest + + steps: + - name: Checkout code + uses: actions/checkout@v4 + + - name: Report PHP version + run: php -v + + - name: Lint PHP files + run: find . -path ./vendor -prune -o -name '*.php' -print0 | xargs -0 -n1 -- php -l + + - name: Hook registration regression test + run: php tests/hook-registration-test.php diff --git a/tests/hook-registration-test.php b/tests/hook-registration-test.php new file mode 100644 index 0000000..118b05f --- /dev/null +++ b/tests/hook-registration-test.php @@ -0,0 +1,137 @@ + $hook, 'callback' => $callback, 'priority' => $priority ); +} + +function add_filter( $hook, $callback, $priority = 10, $accepted_args = 1 ) { + $GLOBALS['hooks'][] = array( 'hook' => $hook, 'callback' => $callback, 'priority' => $priority ); +} + +require __DIR__ . '/../pressable-basic-authentication.php'; + +$failures = array(); + +/** + * Records a single assertion. + * + * @param bool $passed Whether the assertion held. + * @param string $description What was asserted. + */ +function check( $passed, $description ) { + global $failures; + + if ( $passed ) { + echo " PASS $description\n"; + return; + } + + $failures[] = $description; + echo " FAIL $description\n"; +} + +/** + * Finds the hook a given method of the plugin class was registered against. + * + * @param string $method Method name. + * @return array|null The recorded registration, or null when unregistered. + */ +function registration_for( $method ) { + foreach ( $GLOBALS['hooks'] as $registration ) { + if ( is_array( $registration['callback'] ) && $registration['callback'][1] === $method ) { + return $registration; + } + } + + return null; +} + +/** + * Returns the source of one method of the plugin class. + * + * @param string $method Method name. + * @return string + */ +function source_of( $method ) { + if ( ! method_exists( 'Pressable_Basic_Auth', $method ) ) { + return ''; + } + + $reflected = new ReflectionMethod( 'Pressable_Basic_Auth', $method ); + $lines = file( $reflected->getFileName() ); + + return implode( + '', + array_slice( $lines, $reflected->getStartLine() - 1, $reflected->getEndLine() - $reflected->getStartLine() + 1 ) + ); +} + +echo "Basic Auth hook registration\n"; + +$logout = registration_for( 'handle_logout_request' ); +check( null !== $logout, 'the logout handler is registered' ); +check( null !== $logout && 'init' === $logout['hook'], "the logout handler is hooked to 'init'" ); + +$boot = registration_for( 'init' ); +check( null !== $boot && 'plugins_loaded' === $boot['hook'], "init() is still hooked to 'plugins_loaded'" ); +check( null !== $boot && 1 === $boot['priority'], 'init() still runs at priority 1, so enforcement stays early' ); + +// Asserted from both sides on purpose. The negative check alone would pass +// vacuously if handle_basic_auth_logout() were renamed -- silently losing the +// coverage this test exists for -- so the positive check pins the name as live. +// Both match on the trailing "(" so a rename to a superstring (…_logout_renamed) +// does not satisfy either check. +check( + false !== strpos( source_of( 'handle_logout_request' ), 'handle_basic_auth_logout(' ), + 'the init callback reaches the logout handler' +); + +check( + false === strpos( source_of( 'init' ), 'handle_basic_auth_logout(' ), + 'init() does not invoke the logout handler, so wp_logout() cannot fire on plugins_loaded' +); + +check( + // Short-circuits so a missing method reports a failure rather than throwing a + // ReflectionException, which would abort the run and hide any later check. + method_exists( 'Pressable_Basic_Auth', 'handle_logout_request' ) + && ( new ReflectionMethod( 'Pressable_Basic_Auth', 'handle_logout_request' ) )->isPublic(), + 'the logout handler is public, as a hook callback must be' +); + +echo "\n"; + +if ( $failures ) { + echo count( $failures ) . " failure(s)\n"; + exit( 1 ); +} + +echo "All checks passed\n"; +exit( 0 ); From 84307771d89ad1030280bd1495ec701d9b5f8ced Mon Sep 17 00:00:00 2001 From: loukieluke <84424020+loukieluke@users.noreply.github.com> Date: Mon, 14 Sep 2026 09:33:39 -0500 Subject: [PATCH 3/8] ENG-7125: Harden the test workflow against the PR code it runs The job lints and executes PHP from the checked-out pull request, so leaving GITHUB_TOKEN in the local git config gives that code access to the credential. Disable credential persistence and declare an explicit read-only permission set instead of inheriting the default. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/test.yml | 8 ++++++++ 1 file changed, 8 insertions(+) diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml index 30ec83f..237951c 100644 --- a/.github/workflows/test.yml +++ b/.github/workflows/test.yml @@ -6,6 +6,9 @@ on: branches: - main +permissions: + contents: read + jobs: test: name: Lint and hook registration @@ -14,6 +17,11 @@ jobs: steps: - name: Checkout code uses: actions/checkout@v4 + with: + # The job lints and executes PHP straight from the checked-out pull + # request, so the GITHUB_TOKEN must not be left in the local git + # config where that code could read it. + persist-credentials: false - name: Report PHP version run: php -v From 02bea5a9b7989c59981657c5408c3932bd8ccf3c Mon Sep 17 00:00:00 2001 From: loukieluke <84424020+loukieluke@users.noreply.github.com> Date: Mon, 14 Sep 2026 12:36:58 -0500 Subject: [PATCH 4/8] ENG-7125: Don't establish a session for a request that is logging out Moving the logout to `init` put force_basic_authentication() in front of it, so a logout request carrying live Basic credentials but no valid WP session was authenticated and given cookies that wp_logout() discarded microseconds later. The user saw no difference, but set_current_user, set_auth_cookie and set_logged_in_cookie fired on what was only ever a logout -- a hook an audit or session-tracking plugin can reasonably record as a login. The guard sits after wp_authenticate() rather than at the top of the method, so missing and invalid credentials still get their 401 and an anonymous logout still cannot reach wp_logout(). Only the cookie-setting is skipped. Co-Authored-By: Claude Opus 5 (1M context) --- pressable-basic-authentication.php | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/pressable-basic-authentication.php b/pressable-basic-authentication.php index 64bb894..3e5e219 100644 --- a/pressable-basic-authentication.php +++ b/pressable-basic-authentication.php @@ -130,6 +130,17 @@ private function force_basic_authentication() { $this->send_auth_headers(); } + // A request that is about to log out still has to clear the authentication gate + // above -- that is what keeps an anonymous logout from reaching wp_logout() -- but + // it must not be given a session that handle_logout_request() discards moments + // later on `init`. Establishing one anyway fires set_current_user, set_auth_cookie + // and set_logged_in_cookie on what is only ever a logout, which an audit or + // session-tracking plugin can reasonably record as a real login. Returning here + // leaves the 401 paths untouched and skips only the cookie-setting. + if ( isset( $_GET['basic-auth-logout'] ) ) { + return; + } + // Log the user in programmatically. wp_set_current_user( $user->ID ); wp_set_auth_cookie( $user->ID ); From c7b180e395ec94dbb14ab9e326cd6a01bdf1681c Mon Sep 17 00:00:00 2001 From: loukieluke <84424020+loukieluke@users.noreply.github.com> Date: Mon, 14 Sep 2026 12:36:58 -0500 Subject: [PATCH 5/8] ENG-7125: Gate the release on the lint and hook-registration checks test.yml and main.yml both trigger on push to main, and separate workflow files have no implicit ordering -- only same-workflow `needs:` or an explicit `workflow_run:` creates one, and main.yml has neither. So a commit breaking `php -l` or the hook-registration test would still have published a Release with the checks failing alongside it. Running both in the release job ahead of `git archive` closes that without touching packaging mechanics: the archive step and build.rb are unchanged, and `/tests export-ignore` still keeps the zip to LICENSE, the plugin and readme.txt. Co-Authored-By: Claude Opus 5 (1M context) --- .github/workflows/main.yml | 11 +++++++++++ 1 file changed, 11 insertions(+) diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 4ee246e..16032c8 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -18,6 +18,17 @@ jobs: - name: Checkout code uses: actions/checkout@v3 + # The release is published straight from this job, and no ordering exists + # between separate workflow files, so the checks in test.yml cannot gate it + # from outside -- a push that breaks them would still ship a Release. These + # run here, ahead of the archive, so a failure stops the release rather than + # being reported next to one. + - name: Lint PHP files + run: find . -path ./vendor -prune -o -name '*.php' -print0 | xargs -0 -n1 -- php -l + + - name: Hook registration regression test + run: php tests/hook-registration-test.php + - name: Install Ruby uses: ruby/setup-ruby@v1 with: From ee057c90dc6c119520ea5485ea9048820cb97855 Mon Sep 17 00:00:00 2001 From: loukieluke <84424020+loukieluke@users.noreply.github.com> Date: Mon, 14 Sep 2026 12:52:38 -0500 Subject: [PATCH 6/8] ENG-7125: Pin the logout guard's position and correct what it claims to skip The comment said the guard skips "only the cookie-setting", which is wrong. Skipping wp_set_current_user() also leaves get_current_user_id() at 0 for the wp_logout() that follows, so subscribers receive 0 and wp_destroy_current_session() reaps no token. That matches what 1.0.2 did, since its logout ran ahead of any of this -- but the comment claimed something narrower than the code does. The position of the guard was also unpinned. Moved above the credential checks it would still suppress the spurious session, so the symptom would look fixed, while skipping the 401 and letting an anonymous ?basic-auth-logout=1 reach wp_logout() again -- the unauthenticated trigger this plugin's logout move closed. Two assertions now bracket it between wp_authenticate() and wp_set_auth_cookie(); both mutations fail the suite. Co-Authored-By: Claude Opus 5 (1M context) --- pressable-basic-authentication.php | 16 ++++++++++++---- tests/hook-registration-test.php | 20 ++++++++++++++++++++ 2 files changed, 32 insertions(+), 4 deletions(-) diff --git a/pressable-basic-authentication.php b/pressable-basic-authentication.php index 3e5e219..b453ca4 100644 --- a/pressable-basic-authentication.php +++ b/pressable-basic-authentication.php @@ -133,10 +133,18 @@ private function force_basic_authentication() { // A request that is about to log out still has to clear the authentication gate // above -- that is what keeps an anonymous logout from reaching wp_logout() -- but // it must not be given a session that handle_logout_request() discards moments - // later on `init`. Establishing one anyway fires set_current_user, set_auth_cookie - // and set_logged_in_cookie on what is only ever a logout, which an audit or - // session-tracking plugin can reasonably record as a real login. Returning here - // leaves the 401 paths untouched and skips only the cookie-setting. + // later on `init`. Establishing one fires set_auth_cookie and set_logged_in_cookie + // on what is only ever a logout, which an audit or session-tracking plugin can + // reasonably record as a real login. + // + // This skips the whole session setup, not merely the cookies. Without + // wp_set_current_user() the wp_logout() that follows sees get_current_user_id() as + // 0, so it passes 0 to `wp_logout` subscribers and reaps no session token. That is + // what 1.0.2 did too -- its logout ran before any of this -- so it restores the + // released behaviour rather than inventing a third one. + // + // Position matters: ahead of the credential checks this would also skip the 401, + // reopening the anonymous logout path. tests/hook-registration-test.php pins it. if ( isset( $_GET['basic-auth-logout'] ) ) { return; } diff --git a/tests/hook-registration-test.php b/tests/hook-registration-test.php index 118b05f..ad24715 100644 --- a/tests/hook-registration-test.php +++ b/tests/hook-registration-test.php @@ -126,6 +126,26 @@ function source_of( $method ) { 'the logout handler is public, as a hook callback must be' ); +// The guard that skips session setup on a logout request has to sit AFTER the +// credential checks. Moved above them it would still suppress the spurious session +// -- so the symptom it was added for would look fixed -- while also skipping the +// 401, letting an anonymous ?basic-auth-logout=1 reach wp_logout() again. That is +// precisely the unauthenticated trigger this plugin's logout move closed, so the +// wrong placement is worse than no guard at all and is pinned here by position. +$force = source_of( 'force_basic_authentication' ); +$guard_at = strpos( $force, "\$_GET['basic-auth-logout']" ); +$validated_at = strpos( $force, 'wp_authenticate(' ); + +check( + false !== $guard_at && false !== $validated_at && $guard_at > $validated_at, + 'the logout guard sits after wp_authenticate(), so a logout still requires credentials' +); + +check( + false !== $guard_at && false !== strpos( $force, 'wp_set_auth_cookie(' ) && $guard_at < strpos( $force, 'wp_set_auth_cookie(' ), + 'the logout guard sits before wp_set_auth_cookie(), so a logout establishes no session' +); + echo "\n"; if ( $failures ) { From cec2a0c908c815dba1a42bc417d720641ec5d20b Mon Sep 17 00:00:00 2001 From: loukieluke <84424020+loukieluke@users.noreply.github.com> Date: Mon, 14 Sep 2026 12:56:57 -0500 Subject: [PATCH 7/8] ENG-7125: Anchor the logout guard on the last credential challenge Bracketing the guard between wp_authenticate() and wp_set_auth_cookie() left a gap: a guard placed between wp_authenticate() and the is_wp_error() handling sits inside those bounds and satisfies both checks, while returning before the challenge that rejects INVALID credentials -- so a wrong password plus the logout param would reach wp_logout(). Anchoring on the last send_auth_headers() instead puts every credential-failure path ahead of the guard. Position alone also said nothing about the guard still returning, so a body that no longer does is now rejected too. All four mutations fail the suite: the guard moved above either challenge, moved between the two, stripped of its return, and removed outright. The rationale comment no longer explains the skipped session by reference to what an earlier release did; it explains it from the state the code is actually in -- this path is only reachable when no session exists, so there is no user to name. Co-Authored-By: Claude Opus 5 (1M context) --- pressable-basic-authentication.php | 26 +++++++++++------------ tests/hook-registration-test.php | 33 +++++++++++++++++++----------- 2 files changed, 34 insertions(+), 25 deletions(-) diff --git a/pressable-basic-authentication.php b/pressable-basic-authentication.php index b453ca4..27a16ae 100644 --- a/pressable-basic-authentication.php +++ b/pressable-basic-authentication.php @@ -130,21 +130,21 @@ private function force_basic_authentication() { $this->send_auth_headers(); } - // A request that is about to log out still has to clear the authentication gate - // above -- that is what keeps an anonymous logout from reaching wp_logout() -- but - // it must not be given a session that handle_logout_request() discards moments - // later on `init`. Establishing one fires set_auth_cookie and set_logged_in_cookie - // on what is only ever a logout, which an audit or session-tracking plugin can - // reasonably record as a real login. + // A request asking to log out must still clear the authentication gate above -- + // that is what keeps an anonymous caller from reaching wp_logout() -- but it must + // not be given a session that handle_logout_request() discards moments later on + // `init`. Establishing one fires set_auth_cookie and set_logged_in_cookie on what + // is only ever a logout, which an audit or session-tracking plugin can reasonably + // record as a real login. // - // This skips the whole session setup, not merely the cookies. Without - // wp_set_current_user() the wp_logout() that follows sees get_current_user_id() as - // 0, so it passes 0 to `wp_logout` subscribers and reaps no session token. That is - // what 1.0.2 did too -- its logout ran before any of this -- so it restores the - // released behaviour rather than inventing a third one. + // Skipping wp_set_current_user() as well as the cookies is correct, not a + // shortcut: execution only reaches here when no WordPress session exists (a live + // one returns above), so there is no logged-in user for the following wp_logout() + // to name or whose session token it could reap. It reports 0 because 0 is true. // - // Position matters: ahead of the credential checks this would also skip the 401, - // reopening the anonymous logout path. tests/hook-registration-test.php pins it. + // Placement is load-bearing in both directions. Above the credential handling this + // would skip the 401 as well, readmitting the unauthenticated caller it exists to + // exclude; below the cookie calls it would do nothing at all. if ( isset( $_GET['basic-auth-logout'] ) ) { return; } diff --git a/tests/hook-registration-test.php b/tests/hook-registration-test.php index ad24715..024fae2 100644 --- a/tests/hook-registration-test.php +++ b/tests/hook-registration-test.php @@ -126,26 +126,35 @@ function source_of( $method ) { 'the logout handler is public, as a hook callback must be' ); -// The guard that skips session setup on a logout request has to sit AFTER the -// credential checks. Moved above them it would still suppress the spurious session -// -- so the symptom it was added for would look fixed -- while also skipping the -// 401, letting an anonymous ?basic-auth-logout=1 reach wp_logout() again. That is -// precisely the unauthenticated trigger this plugin's logout move closed, so the -// wrong placement is worse than no guard at all and is pinned here by position. -$force = source_of( 'force_basic_authentication' ); -$guard_at = strpos( $force, "\$_GET['basic-auth-logout']" ); -$validated_at = strpos( $force, 'wp_authenticate(' ); +// The guard that skips session setup on a logout request is bracketed rather than +// merely ordered, because both neighbours are hazards. Above the credential +// handling it would suppress the spurious session -- so the symptom it was added +// for would look fixed -- while also skipping the 401, readmitting the +// unauthenticated caller the logout move excluded. Below the cookie calls it would +// be inert. Anchoring on the LAST send_auth_headers() rather than wp_authenticate() +// is deliberate: between the two, a guard still clears every ordering check yet lets +// INVALID credentials bypass the challenge. +$force = source_of( 'force_basic_authentication' ); +$guard_at = strpos( $force, "\$_GET['basic-auth-logout']" ); +$last_challenge = strrpos( $force, 'send_auth_headers(' ); +$cookie_at = strpos( $force, 'wp_set_auth_cookie(' ); check( - false !== $guard_at && false !== $validated_at && $guard_at > $validated_at, - 'the logout guard sits after wp_authenticate(), so a logout still requires credentials' + false !== $guard_at && false !== $last_challenge && $guard_at > $last_challenge, + 'the logout guard sits after every credential challenge, so a logout still requires valid credentials' ); check( - false !== $guard_at && false !== strpos( $force, 'wp_set_auth_cookie(' ) && $guard_at < strpos( $force, 'wp_set_auth_cookie(' ), + false !== $guard_at && false !== $cookie_at && $guard_at < $cookie_at, 'the logout guard sits before wp_set_auth_cookie(), so a logout establishes no session' ); +// Position alone would be satisfied by a guard whose body no longer returns. +check( + 1 === preg_match( '/if \(\s*isset\(\s*\$_GET\[.basic-auth-logout.\]\s*\)\s*\)\s*\{\s*return;\s*\}/', $force ), + 'the logout guard actually returns, rather than only appearing in the right place' +); + echo "\n"; if ( $failures ) { From ead980aadfbb39c38c0434a5671bd3fa6c478a50 Mon Sep 17 00:00:00 2001 From: loukieluke <84424020+loukieluke@users.noreply.github.com> Date: Mon, 14 Sep 2026 13:00:45 -0500 Subject: [PATCH 8/8] ENG-7125: Correct two inaccuracies in the logout-guard rationale The comment claimed the guard is reached only when no WordPress session exists. is_user_logged_in() proves nobody is logged IN, which is not the same thing -- an expired logged_in cookie still parses into a token -- so the claim was stronger than the condition supports. It now says what the condition actually establishes and makes no assertion about tokens. It also said wp_logout() "reports 0 because 0 is true", which in a PHP file reads as a claim about truthiness, where 0 is false. The point was that 0 is the honest answer; it now says so by naming get_current_user_id() as the source. Co-Authored-By: Claude Opus 5 (1M context) --- pressable-basic-authentication.php | 8 +++++--- 1 file changed, 5 insertions(+), 3 deletions(-) diff --git a/pressable-basic-authentication.php b/pressable-basic-authentication.php index 27a16ae..30472db 100644 --- a/pressable-basic-authentication.php +++ b/pressable-basic-authentication.php @@ -138,9 +138,11 @@ private function force_basic_authentication() { // record as a real login. // // Skipping wp_set_current_user() as well as the cookies is correct, not a - // shortcut: execution only reaches here when no WordPress session exists (a live - // one returns above), so there is no logged-in user for the following wp_logout() - // to name or whose session token it could reap. It reports 0 because 0 is true. + // shortcut: execution only reaches here when nobody is logged in -- a live session + // returns above -- so there is no established identity for the following + // wp_logout() to report. It passes whatever get_current_user_id() actually holds, + // which is 0, instead of one this method manufactured moments earlier purely to + // tear it down again. // // Placement is load-bearing in both directions. Above the credential handling this // would skip the 401 as well, readmitting the unauthenticated caller it exists to