diff --git a/.gitattributes b/.gitattributes index ac24691..28a7274 100644 --- a/.gitattributes +++ b/.gitattributes @@ -14,6 +14,4 @@ /README.md export-ignore /composer.json export-ignore /composer.lock export-ignore -/.DS_Store export-ignore -# Tests are not part of the distributed plugin. -/tests export-ignore +/.DS_Store export-ignore \ No newline at end of file diff --git a/.github/workflows/main.yml b/.github/workflows/main.yml index 16032c8..4ee246e 100644 --- a/.github/workflows/main.yml +++ b/.github/workflows/main.yml @@ -18,17 +18,6 @@ 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 deleted file mode 100644 index 237951c..0000000 --- a/.github/workflows/test.yml +++ /dev/null @@ -1,33 +0,0 @@ -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 30472db..91e10c5 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.3 +Version: 1.0.4 License: GPL2 Text Domain: hosting-basic-authentication */ @@ -30,18 +30,6 @@ 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 ); @@ -53,45 +41,36 @@ public function __construct() { * Initialize the plugin */ public function init() { - if ( $this->skip_request() ) { + // Skip if we're doing AJAX. + if ( $this->is_ajax_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(); - } - - /** - * 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() ) { + // Skip if we're doing CRON. + if ( $this->is_cron_request() ) { return; } - if ( ! isset( $_GET['basic-auth-logout'] ) ) { + // Skip if we're in CLI mode. + if ( $this->is_cli_request() ) { return; } - $this->handle_basic_auth_logout(); - } + // Skip requests to excluded endpoints + if ($this->should_skip_auth()) { + return; + } - /** - * 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(); + // Handle logout request. + if ( isset( $_GET['basic-auth-logout'] ) ) { + $this->handle_basic_auth_logout(); + } + + // Redirect from wp-login.php when already authenticated via Basic Auth + $this->maybe_redirect_from_login_page(); + + // Force authentication. + $this->force_basic_authentication(); } /** @@ -130,27 +109,6 @@ 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 ); @@ -312,15 +270,6 @@ 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 4aca433..ebf5922 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.3 +Stable tag: 1.0.4 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 deleted file mode 100644 index 024fae2..0000000 --- a/tests/hook-registration-test.php +++ /dev/null @@ -1,166 +0,0 @@ - $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 );