ENG-7125: Fix Basic Auth And User Switching Plugin Conflict

.gitattributes

@@ -14,4 +14,6 @@
 /README.md export-ignore
 /composer.json export-ignore
 /composer.lock export-ignore
-/.DS_Store export-ignore
+/.DS_Store export-ignore
+# Tests are not part of the distributed plugin.
+/tests export-ignore

.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:

.github/workflows/test.yml

@@ -0,0 +1,33 @@
+name: Tests
+
+on:
+  pull_request:
+  push:
+    branches:
+      - main
+
+permissions:
+  contents: read
+
+jobs:
+  test:
+    name: Lint and hook registration
+    runs-on: ubuntu-latest
+
+    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
+
+      - 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

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();
+
+		// Force authentication.
+		$this->force_basic_authentication();
+	}
 
-		// Skip if we're in CLI mode.
-		if ( $this->is_cli_request() ) {
+	/**
+	 * 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();
 	}
 
 	/**
@@ -109,6 +130,27 @@ private function force_basic_authentication() {
 			$this->send_auth_headers();
 		}
 
+		// 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.
+		//
+		// Skipping wp_set_current_user() as well as the cookies is correct, not a
+		// 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
+		// exclude; below the cookie calls it would do nothing at all.
+		if ( isset( $_GET['basic-auth-logout'] ) ) {
+			return;
+		}
+
 		// Log the user in programmatically.
 		wp_set_current_user( $user->ID );
 		wp_set_auth_cookie( $user->ID );
@@ -270,6 +312,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'] ) &&

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
 

tests/hook-registration-test.php

@@ -0,0 +1,166 @@
+<?php
+/**
+ * Regression test for the Basic Auth / User Switching logout conflict.
+ *
+ * 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` callback races that setup and fatals whichever way
+ * the load order happens to fall. The logout must therefore stay on `init`, which
+ * runs after every `plugins_loaded` callback has completed.
+ *
+ * Deliberately dependency-free: the repo has no composer/PHPUnit setup, and this
+ * asserts hook wiring rather than request behaviour, so it needs neither WordPress
+ * nor a database. Run it with: php tests/hook-registration-test.php
+ *
+ * @package HostingBasicAuthentication
+ */
+
+// This file defines ABSPATH itself, so the usual `defined( 'ABSPATH' ) || exit`
+// plugin guard cannot protect it. It sits inside the plugin directory, which the
+// web server serves directly without loading WordPress -- so without this guard it
+// answers 200 on a site where Basic Authentication returns 401 for everything else.
+if ( 'cli' !== php_sapi_name() ) {
+	exit( 1 );
+}
+
+define( 'ABSPATH', __DIR__ );
+
+$GLOBALS['hooks'] = array();
+
+function add_action( $hook, $callback, $priority = 10, $accepted_args = 1 ) {
+	$GLOBALS['hooks'][] = array( 'hook' => $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'
+);
+
+// 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 !== $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 !== $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 ) {
+	echo count( $failures ) . " failure(s)\n";
+	exit( 1 );
+}
+
+echo "All checks passed\n";
+exit( 0 );