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/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: diff --git a/.github/workflows/test.yml b/.github/workflows/test.yml new file mode 100644 index 0000000..237951c --- /dev/null +++ b/.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 diff --git a/pressable-basic-authentication.php b/pressable-basic-authentication.php index 22cee88..30472db 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(); + + // 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'] ) && 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 diff --git a/tests/hook-registration-test.php b/tests/hook-registration-test.php new file mode 100644 index 0000000..024fae2 --- /dev/null +++ b/tests/hook-registration-test.php @@ -0,0 +1,166 @@ + $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 );