Restore update checks and magic login on sites where the API key lives in the environment

flywp.php

@@ -14,6 +14,32 @@
     exit;
 }
 
+/*
+ * Stand down when this plugin is already loaded from somewhere else.
+ *
+ * A site can carry two copies. On Bedrock the plugin directory belongs to Composer, so FlyWP
+ * keeps its own copy outside the checkout and mounts it in under a second slug. If both are
+ * active, `FlyWP_Plugin`, `flywp()` and the bundled Composer autoloader's init class are each
+ * declared twice and the site fatals.
+ *
+ * Whichever copy runs second gives way. That is deliberately all this decides: it needs no
+ * knowledge of what the other copy is called, so a customer whose Composer install lands in a
+ * directory named anything at all is still protected. Which copy *should* win is settled by
+ * FlyWP before either is activated, by activating exactly one.
+ *
+ * Both conditions are checked because they become true at different moments, and this has to
+ * run before `vendor/autoload.php` -- requiring a second copy of the autoloader is itself one
+ * of the redeclarations being avoided.
+ *
+ * Known gap: a copy that stands down here has not called `register_activation_hook()`, so if
+ * WordPress activates it during this same request its `activate()` never runs. Both copies
+ * register the same rewrite endpoint and the next request loads the survivor normally, so
+ * nothing is lost today -- but an activation-only step added later would not run.
+ */
+if ( defined( 'FLYWP_VERSION' ) || class_exists( 'FlyWP_Plugin', false ) ) {
+    return;
+}
+
 require __DIR__ . '/vendor/autoload.php';
 
 use WeDevs\WpUtils\ContainerTrait;
@@ -111,6 +137,19 @@ public function init_plugin() {
         // one POST path and does nothing on any other request.
         new FlyWP\Frontend\MagicLogin();
 
+        // The router and the API also load ahead of the gate. `Router::register_routes()` is
+        // what registers `fly-api` as a public query variable, and it has to run on every
+        // request; when it does not, `WP::parse_request()` drops the unknown variable and
+        // WordPress serves the front page instead. That is a 200 carrying the theme, which
+        // reads to a caller as a working site rather than a plugin with no key. Loading these
+        // unconditionally means an unkeyed site answers `/fly-api/*` with JSON and says so.
+        //
+        // This exposes nothing new: `Api` registers only the unauthenticated `ping` route
+        // until a valid bearer token is presented, and every other route stays behind either
+        // that check or the key gate below.
+        $this->router = new FlyWP\Router();
+        $this->rest   = new FlyWP\Api();
+
         if ( ! $this->has_key() ) {
             $this->add_action( 'admin_notices', 'admin_notice' );
 
@@ -123,8 +162,6 @@ public function init_plugin() {
             $this->frontend = new FlyWP\Frontend();
         }
 
-        $this->router       = new FlyWP\Router();
-        $this->rest         = new FlyWP\Api();
         $this->fastcgi      = new FlyWP\Fastcgi_Cache();
         $this->opcache      = new FlyWP\Opcache();
         $this->flyapi       = new FlyWP\FlyApi();
@@ -151,7 +188,7 @@ public function admin_notice() {
      * @return bool
      */
     public function has_key() {
-        return FLYWP_API_KEY !== '';
+        return $this->get_key() !== '';
     }
 
     /**
@@ -160,25 +197,16 @@ public function has_key() {
      * @return string
      */
     public function get_key() {
-        return FLYWP_API_KEY;
+        return FlyWP\KeyResolver::resolve( FLYWP_API_KEY, 'FLYWP_API_KEY' );
     }
 
     /**
      * Public key used to verify magic-login tokens.
      *
-     * Set as a constant on classic WordPress. On Bedrock it may only be present in the
-     * environment, so fall back to `getenv()`.
-     *
      * @return string
      */
     public function get_login_public_key() {
-        if ( FLYWP_LOGIN_PUBLIC_KEY !== '' ) {
-            return FLYWP_LOGIN_PUBLIC_KEY;
-        }
-
-        $from_env = getenv( 'FLYWP_LOGIN_PUBLIC_KEY' );
-
-        return $from_env === false ? '' : $from_env;
+        return FlyWP\KeyResolver::resolve( FLYWP_LOGIN_PUBLIC_KEY, 'FLYWP_LOGIN_PUBLIC_KEY' );
     }
 }
 

includes/Api/Ping.php

@@ -14,6 +14,11 @@ public function __construct() {
     /**
      * Handle ping request.
      *
+     * `has_key` is the field that answers "does this integration work". The route itself
+     * no longer does: it is registered whether or not the plugin found an API key, so a
+     * 200 here proves only that the plugin loaded. A caller deciding whether a site is
+     * healthy has to read this field, not the status code.
+     *
      * @return void
      */
     public function handle_ping() {
@@ -22,6 +27,7 @@ public function handle_ping() {
             'wp_version'     => get_bloginfo( 'version' ),
             'php_version'    => PHP_VERSION,
             'plugin_version' => FLYWP_VERSION,
+            'has_key'        => flywp()->has_key(),
         ];
 
         wp_send_json( $response );

includes/KeyResolver.php

@@ -0,0 +1,60 @@
+<?php
+
+namespace FlyWP;
+
+/**
+ * Resolves a FlyWP key that may never have reached a PHP constant.
+ *
+ * Classic WordPress writes these into `wp-config.php`, so the constant is always set and
+ * nothing here does any work. Bedrock has no `wp-config.php`: FlyWP writes the value into
+ * `.env`, and the constant exists only when a config file declares it with `Config::define()`.
+ * `flywp/bedrock-starter` declares it; a repository a customer built from upstream
+ * `roots/bedrock` does not. On those sites the key is present and correct in the environment
+ * while the constant is empty, which used to switch the whole plugin off.
+ *
+ * All three environment sources are read because the loaders disagree on which they fill.
+ * `flywp/bedrock-starter` builds its Dotenv repository from `EnvConstAdapter` and
+ * `PutenvAdapter`, which populates `getenv()` and `$_ENV` but not `$_SERVER`. A repository
+ * using `createImmutable`, or a host with `putenv()` disabled, populates a different subset.
+ *
+ * This is deliberately free of WordPress functions so it can be unit tested without booting
+ * WordPress, which is why `$_SERVER` is unslashed with `stripslashes()` rather than
+ * `wp_unslash()`. The two are the same operation for a string, and the unslashing is required:
+ * `wp_magic_quotes()` runs `add_magic_quotes()` over `$_GET`, `$_POST`, `$_COOKIE` **and
+ * `$_SERVER`** before any plugin loads. A key containing a quote or a backslash would otherwise
+ * arrive here escaped while the bearer token it is compared against is unslashed by
+ * {@see \FlyWP\Api::get_bearer_token()}, and `hash_equals()` would never match.
+ *
+ * `$_ENV` is the one superglobal `wp_magic_quotes()` leaves alone, so it is read as it stands.
+ */
+class KeyResolver {
+
+    /**
+     * Resolve a key from its constant, falling back to the environment.
+     *
+     * @param string $constant_value Value of the matching constant, '' when undeclared.
+     * @param string $name           Environment variable to fall back to.
+     *
+     * @return string The key, or '' when no source carries one.
+     */
+    public static function resolve( $constant_value, $name ) {
+        if ( is_string( $constant_value ) && $constant_value !== '' ) {
+            return $constant_value;
+        }
+
+        $candidates = [
+            getenv( $name ),
+            isset( $_ENV[ $name ] ) ? $_ENV[ $name ] : false,
+            // phpcs:ignore WordPress.Security.ValidatedSanitizedInput.InputNotSanitized,WordPress.Security.ValidatedSanitizedInput.MissingUnslash -- The value IS unslashed, with stripslashes() rather than the wp_unslash() the sniff looks for, because this class stays free of WordPress functions so it can be unit tested. The two are identical for a string. It is not sanitized because it is a credential compared with hash_equals(), and sanitizing would alter it.
+            isset( $_SERVER[ $name ] ) && is_string( $_SERVER[ $name ] ) ? stripslashes( $_SERVER[ $name ] ) : false,
+        ];
+
+        foreach ( $candidates as $candidate ) {
+            if ( is_string( $candidate ) && $candidate !== '' ) {
+                return $candidate;
+            }
+        }
+
+        return '';
+    }
+}

tests/KeyResolverTest.php

@@ -0,0 +1,134 @@
+<?php
+
+namespace FlyWP\Tests;
+
+use FlyWP\KeyResolver;
+use PHPUnit\Framework\TestCase;
+
+/**
+ * The resolver is what keeps the plugin alive on a Bedrock site whose repository never
+ * declared `Config::define( 'FLYWP_API_KEY', ... )`. Before it existed, `has_key()` read the
+ * empty constant, `init_plugin()` returned early, `fly-api` was never registered as a query
+ * variable, and `/fly-api/*` answered with the theme front page and a 200.
+ */
+class KeyResolverTest extends TestCase {
+
+    const NAME = 'FLYWP_TEST_KEY';
+
+    const VALUE = 'site_key_42_abcdefghijklmnopqrstuvwxyz0123456789';
+
+    protected function setUp(): void {
+        parent::setUp();
+
+        $this->clearEnvironment();
+    }
+
+    protected function tearDown(): void {
+        $this->clearEnvironment();
+
+        parent::tearDown();
+    }
+
+    public function test_it_prefers_the_constant_when_one_is_set() {
+        putenv( self::NAME . '=from-getenv' );
+        $_ENV[ self::NAME ]    = 'from-env';
+        $_SERVER[ self::NAME ] = 'from-server';
+
+        $this->assertSame( self::VALUE, KeyResolver::resolve( self::VALUE, self::NAME ) );
+    }
+
+    public function test_it_falls_back_to_getenv_when_the_constant_is_empty() {
+        putenv( self::NAME . '=' . self::VALUE );
+
+        $this->assertSame( self::VALUE, KeyResolver::resolve( '', self::NAME ) );
+    }
+
+    /**
+     * A repository loading its `.env` with `createImmutable` never calls `putenv()`.
+     */
+    public function test_it_falls_back_to_the_env_superglobal() {
+        $_ENV[ self::NAME ] = self::VALUE;
+
+        $this->assertSame( self::VALUE, KeyResolver::resolve( '', self::NAME ) );
+    }
+
+    public function test_it_falls_back_to_the_server_superglobal() {
+        $_SERVER[ self::NAME ] = self::VALUE;
+
+        $this->assertSame( self::VALUE, KeyResolver::resolve( '', self::NAME ) );
+    }
+
+    public function test_it_returns_an_empty_string_when_no_source_has_the_key() {
+        $this->assertSame( '', KeyResolver::resolve( '', self::NAME ) );
+    }
+
+    /**
+     * `getenv()` returns false for an unset name, and an unset `.env` line can leave an empty
+     * string behind. Neither is a key, and neither may win over a later source that has one.
+     */
+    public function test_it_skips_empty_sources_and_keeps_looking() {
+        putenv( self::NAME . '=' );
+        $_ENV[ self::NAME ]    = '';
+        $_SERVER[ self::NAME ] = self::VALUE;
+
+        $this->assertSame( self::VALUE, KeyResolver::resolve( '', self::NAME ) );
+    }
+
+    /**
+     * The login public key is base64 and carries `+`, `/` and `=`. It is returned verbatim:
+     * a resolver that trimmed or escaped it would break signature verification rather than
+     * fail loudly.
+     */
+    public function test_it_returns_a_base64_value_unaltered() {
+        $public_key = 'IMOCUs37RkBOXqbUoMzASCZgByLMIJOeUsw6V5K7uRA=';
+
+        putenv( self::NAME . '=' . $public_key );
+
+        $this->assertSame( $public_key, KeyResolver::resolve( '', self::NAME ) );
+    }
+
+    /**
+     * `wp_magic_quotes()` runs `add_magic_quotes()` over `$_SERVER` before any plugin loads.
+     * A key read from there arrives escaped, while the bearer token it is compared against is
+     * unslashed by `Api::get_bearer_token()`. Without the same treatment here, `hash_equals()`
+     * could never match for a key holding a quote or a backslash.
+     */
+    public function test_it_unslashes_a_key_read_from_the_server_superglobal() {
+        $_SERVER[ self::NAME ] = "a\\'quoted\\\\key";
+
+        $this->assertSame( "a'quoted\\key", KeyResolver::resolve( '', self::NAME ) );
+    }
+
+    /**
+     * `$_ENV` is the one superglobal `wp_magic_quotes()` leaves alone, so it is taken verbatim.
+     */
+    public function test_it_does_not_unslash_the_env_superglobal() {
+        $_ENV[ self::NAME ] = "a\\'value";
+
+        $this->assertSame( "a\\'value", KeyResolver::resolve( '', self::NAME ) );
+    }
+
+    /**
+     * A constant that is not a string cannot be a key, and must not stop the fallback.
+     */
+    public function test_a_non_string_constant_falls_through_to_the_environment() {
+        putenv( self::NAME . '=' . self::VALUE );
+
+        $this->assertSame( self::VALUE, KeyResolver::resolve( false, self::NAME ) );
+    }
+
+    /**
+     * An array in `$_SERVER` is not a key. It must be stepped over rather than reaching
+     * `stripslashes()`, which raises a TypeError on anything but a string.
+     */
+    public function test_it_steps_over_a_non_string_in_the_server_superglobal() {
+        $_SERVER[ self::NAME ] = [ 'not', 'a', 'key' ];
+
+        $this->assertSame( '', KeyResolver::resolve( '', self::NAME ) );
+    }
+
+    private function clearEnvironment() {
+        putenv( self::NAME );
+        unset( $_ENV[ self::NAME ], $_SERVER[ self::NAME ] );
+    }
+}