refactor(BridgeFactory): make methods only accept valid class names (#2897)

This moves the responsibility for getting a valid class name
to the users of BridgeFactory, avoiding the repeated sanitation.
Improper use can also be checked statically.

actions/ConnectivityAction.php

@@ -26,6 +26,13 @@ class ConnectivityAction implements ActionInterface
 {
     public $userData = [];
 
+    private BridgeFactory $bridgeFactory;
+
+    public function __construct()
+    {
+        $this->bridgeFactory = new \BridgeFactory();
+    }
+
     public function execute()
     {
         if (!Debug::isEnabled()) {
@@ -39,7 +46,13 @@ class ConnectivityAction implements ActionInterface
 
         $bridgeName = $this->userData['bridge'];
 
-        $this->reportBridgeConnectivity($bridgeName);
+        $bridgeClassName = $this->bridgeFactory->sanitizeBridgeName($bridgeName);
+
+        if ($bridgeClassName === null) {
+            throw new \InvalidArgumentException('Bridge name invalid!');
+        }
+
+        $this->reportBridgeConnectivity($bridgeClassName);
     }
 
     /**
@@ -52,14 +65,12 @@ class ConnectivityAction implements ActionInterface
      *   "successful": true/false
      * }
      *
-     * @param string $bridgeName Name of the bridge to generate the report for
+     * @param class-string<BridgeInterface> $bridgeClassName Name of the bridge to generate the report for
      * @return void
      */
-    private function reportBridgeConnectivity($bridgeName)
+    private function reportBridgeConnectivity($bridgeClassName)
     {
-        $bridgeFactory = new \BridgeFactory();
+        if (!$this->bridgeFactory->isWhitelisted($bridgeClassName)) {
-
-        if (!$bridgeFactory->isWhitelisted($bridgeName)) {
             header('Content-Type: text/html');
             returnServerError('Bridge is not whitelisted!');
         }
@@ -67,12 +78,12 @@ class ConnectivityAction implements ActionInterface
         header('Content-Type: text/json');
 
         $retVal = [
-            'bridge' => $bridgeName,
+            'bridge' => $bridgeClassName,
             'successful' => false,
             'http_code' => 200,
         ];
 
-        $bridge = $bridgeFactory->create($bridgeName);
+        $bridge = $this->bridgeFactory->create($bridgeClassName);
 
         if ($bridge === false) {
             echo json_encode($retVal);

actions/DetectAction.php

@@ -26,12 +26,12 @@ class DetectAction implements ActionInterface
 
         $bridgeFactory = new \BridgeFactory();
 
-        foreach ($bridgeFactory->getBridgeNames() as $bridgeName) {
+        foreach ($bridgeFactory->getBridgeClassNames() as $bridgeClassName) {
-            if (!$bridgeFactory->isWhitelisted($bridgeName)) {
+            if (!$bridgeFactory->isWhitelisted($bridgeClassName)) {
                 continue;
             }
 
-            $bridge = $bridgeFactory->create($bridgeName);
+            $bridge = $bridgeFactory->create($bridgeClassName);
 
             if ($bridge === false) {
                 continue;
@@ -43,7 +43,7 @@ class DetectAction implements ActionInterface
                 continue;
             }
 
-            $bridgeParams['bridge'] = $bridgeName;
+            $bridgeParams['bridge'] = $bridgeClassName;
             $bridgeParams['format'] = $format;
 
             header('Location: ?action=display&' . http_build_query($bridgeParams), true, 301);

actions/DisplayAction.php

@@ -28,21 +28,25 @@ class DisplayAction implements ActionInterface
 
     public function execute()
     {
-        $bridge = array_key_exists('bridge', $this->userData) ? $this->userData['bridge'] : null;
+        $bridgeFactory = new \BridgeFactory();
+
+        $bridgeClassName = isset($this->userData['bridge']) ? $bridgeFactory->sanitizeBridgeName($this->userData['bridge']) : null;
+
+        if ($bridgeClassName === null) {
+            throw new \InvalidArgumentException('Bridge name invalid!');
+        }
 
         $format = $this->userData['format']
             or returnClientError('You must specify a format!');
 
-        $bridgeFactory = new \BridgeFactory();
-
         // whitelist control
-        if (!$bridgeFactory->isWhitelisted($bridge)) {
+        if (!$bridgeFactory->isWhitelisted($bridgeClassName)) {
             throw new \Exception('This bridge is not whitelisted', 401);
             die;
         }
 
         // Data retrieval
-        $bridge = $bridgeFactory->create($bridge);
+        $bridge = $bridgeFactory->create($bridgeClassName);
         $bridge->loadConfiguration();
 
         $noproxy = array_key_exists('_noproxy', $this->userData)

actions/ListAction.php

@@ -22,20 +22,20 @@ class ListAction implements ActionInterface
 
         $bridgeFactory = new \BridgeFactory();
 
-        foreach ($bridgeFactory->getBridgeNames() as $bridgeName) {
+        foreach ($bridgeFactory->getBridgeClassNames() as $bridgeClassName) {
-            $bridge = $bridgeFactory->create($bridgeName);
+            $bridge = $bridgeFactory->create($bridgeClassName);
 
             if ($bridge === false) { // Broken bridge, show as inactive
-                $list->bridges[$bridgeName] = [
+                $list->bridges[$bridgeClassName] = [
                     'status' => 'inactive'
                 ];
 
                 continue;
             }
 
-            $status = $bridgeFactory->isWhitelisted($bridgeName) ? 'active' : 'inactive';
+            $status = $bridgeFactory->isWhitelisted($bridgeClassName) ? 'active' : 'inactive';
 
-            $list->bridges[$bridgeName] = [
+            $list->bridges[$bridgeClassName] = [
                 'status' => $status,
                 'uri' => $bridge->getURI(),
                 'donationUri' => $bridge->getDonationURI(),

lib/BridgeCard.php

@@ -25,16 +25,16 @@ final class BridgeCard
     /**
      * Get the form header for a bridge card
      *
-     * @param string $bridgeName The bridge name
+     * @param class-string<BridgeInterface> $bridgeClassName The bridge name
      * @param bool $isHttps If disabled, adds a warning to the form
      * @return string The form header
      */
-    private static function getFormHeader($bridgeName, $isHttps = false, $parameterName = '')
+    private static function getFormHeader($bridgeClassName, $isHttps = false, $parameterName = '')
     {
         $form = <<<EOD
 			<form method="GET" action="?">
 				<input type="hidden" name="action" value="display" />
-				<input type="hidden" name="bridge" value="{$bridgeName}" />
+				<input type="hidden" name="bridge" value="{$bridgeClassName}" />
 EOD;
 
         if (!empty($parameterName)) {
@@ -54,7 +54,7 @@ This bridge is not fetching its content through a secure connection</div>';
     /**
      * Get the form body for a bridge
      *
-     * @param string $bridgeName The bridge name
+     * @param class-string<BridgeInterface> $bridgeClassName The bridge name
      * @param array $formats A list of supported formats
      * @param bool $isActive Indicates if a bridge is enabled or not
      * @param bool $isHttps Indicates if a bridge uses HTTPS or not
@@ -63,14 +63,14 @@ This bridge is not fetching its content through a secure connection</div>';
      * @return string The form body
      */
     private static function getForm(
-        $bridgeName,
+        $bridgeClassName,
         $formats,
         $isActive = false,
         $isHttps = false,
         $parameterName = '',
         $parameters = []
     ) {
-        $form = self::getFormHeader($bridgeName, $isHttps, $parameterName);
+        $form = self::getFormHeader($bridgeClassName, $isHttps, $parameterName);
 
         if (count($parameters) > 0) {
             $form .= '<div class="parameters">';
@@ -85,7 +85,7 @@ This bridge is not fetching its content through a secure connection</div>';
                 }
 
                 $idArg = 'arg-'
-                    . urlencode($bridgeName)
+                    . urlencode($bridgeClassName)
                     . '-'
                     . urlencode($parameterName)
                     . '-'
@@ -297,16 +297,16 @@ This bridge is not fetching its content through a secure connection</div>';
     /**
      * Gets a single bridge card
      *
-     * @param string $bridgeName The bridge name
+     * @param class-string<BridgeInterface> $bridgeClassName The bridge name
      * @param array $formats A list of formats
      * @param bool $isActive Indicates if the bridge is active or not
      * @return string The bridge card
      */
-    public static function displayBridgeCard($bridgeName, $formats, $isActive = true)
+    public static function displayBridgeCard($bridgeClassName, $formats, $isActive = true)
     {
         $bridgeFactory = new \BridgeFactory();
 
-        $bridge = $bridgeFactory->create($bridgeName);
+        $bridge = $bridgeFactory->create($bridgeClassName);
 
         if ($bridge == false) {
             return '';
@@ -340,20 +340,20 @@ This bridge is not fetching its content through a secure connection</div>';
         }
 
         $card = <<<CARD
-			<section id="bridge-{$bridgeName}" data-ref="{$name}">
+			<section id="bridge-{$bridgeClassName}" data-ref="{$name}">
 				<h2><a href="{$uri}">{$name}</a></h2>
 				<p class="description">{$description}</p>
-				<input type="checkbox" class="showmore-box" id="showmore-{$bridgeName}" />
+				<input type="checkbox" class="showmore-box" id="showmore-{$bridgeClassName}" />
-				<label class="showmore" for="showmore-{$bridgeName}">Show more</label>
+				<label class="showmore" for="showmore-{$bridgeClassName}">Show more</label>
 CARD;
 
         // If we don't have any parameter for the bridge, we print a generic form to load it.
         if (count($parameters) === 0) {
-            $card .= self::getForm($bridgeName, $formats, $isActive, $isHttps);
+            $card .= self::getForm($bridgeClassName, $formats, $isActive, $isHttps);
 
         // Display form with cache timeout and/or noproxy options (if enabled) when bridge has no parameters
         } elseif (count($parameters) === 1 && array_key_exists('global', $parameters)) {
-            $card .= self::getForm($bridgeName, $formats, $isActive, $isHttps, '', $parameters['global']);
+            $card .= self::getForm($bridgeClassName, $formats, $isActive, $isHttps, '', $parameters['global']);
         } else {
             foreach ($parameters as $parameterName => $parameter) {
                 if (!is_numeric($parameterName) && $parameterName === 'global') {
@@ -368,11 +368,11 @@ CARD;
                     $card .= '<h5>' . $parameterName . '</h5>' . PHP_EOL;
                 }
 
-                $card .= self::getForm($bridgeName, $formats, $isActive, $isHttps, $parameterName, $parameter);
+                $card .= self::getForm($bridgeClassName, $formats, $isActive, $isHttps, $parameterName, $parameter);
             }
         }
 
-        $card .= '<label class="showless" for="showmore-' . $bridgeName . '">Show less</label>';
+        $card .= '<label class="showless" for="showmore-' . $bridgeClassName . '">Show less</label>';
         if ($donationUri !== '' && $donationsAllowed) {
             $card .= '<p class="maintainer">' . $maintainer . ' ~ <a href="' . $donationUri . '">Donate</a></p>';
         } else {

lib/BridgeFactory.php

@@ -3,7 +3,9 @@
 final class BridgeFactory
 {
     private $folder;
-    private $bridgeNames = [];
+    /** @var array<class-string<BridgeInterface>> */
+    private $bridgeClassNames = [];
+    /** @var array<class-string<BridgeInterface>> */
     private $whitelist = [];
 
     public function __construct(string $folder = PATH_LIB_BRIDGES)
@@ -12,8 +14,8 @@ final class BridgeFactory
 
         // create names
         foreach (scandir($this->folder) as $file) {
-            if (preg_match('/^([^.]+)Bridge\.php$/U', $file, $m)) {
+            if (preg_match('/^([^.]+Bridge)\.php$/U', $file, $m)) {
-                $this->bridgeNames[] = $m[1];
+                $this->bridgeClassNames[] = $m[1];
             }
         }
 
@@ -26,34 +28,48 @@ final class BridgeFactory
             $contents = '';
         }
         if ($contents === '*') { // Whitelist all bridges
-            $this->whitelist = $this->getBridgeNames();
+            $this->whitelist = $this->getBridgeClassNames();
         } else {
             foreach (explode("\n", $contents) as $bridgeName) {
-                $this->whitelist[] = $this->sanitizeBridgeName($bridgeName);
+                $bridgeClassName = $this->sanitizeBridgeName($bridgeName);
+                if ($bridgeClassName !== null) {
+                    $this->whitelist[] = $bridgeClassName;
+                }
             }
         }
     }
 
+    /**
+     * @param class-string<BridgeInterface> $name
+     */
     public function create(string $name): BridgeInterface
     {
-        if (preg_match('/^[A-Z][a-zA-Z0-9-]*$/', $name)) {
+        return new $name();
-            $className = sprintf('%sBridge', $this->sanitizeBridgeName($name));
-            return new $className();
-        }
-        throw new \InvalidArgumentException('Bridge name invalid!');
     }
 
-    public function getBridgeNames(): array
+    /**
+     * @return array<class-string<BridgeInterface>>
+     */
+    public function getBridgeClassNames(): array
     {
-        return $this->bridgeNames;
+        return $this->bridgeClassNames;
     }
 
-    public function isWhitelisted($name): bool
+    /**
+     * @param class-string<BridgeInterface>|null $name
+     */
+    public function isWhitelisted(string $name): bool
     {
-        return in_array($this->sanitizeBridgeName($name), $this->whitelist);
+        return in_array($name, $this->whitelist);
     }
 
-    private function sanitizeBridgeName($name)
+    /**
+     * Tries to turn a potentially human produced bridge name into a class name.
+     *
+     * @param mixed $name
+     * @return class-string<BridgeInterface>|null
+     */
+    public function sanitizeBridgeName($name): ?string
     {
         if (!is_string($name)) {
             return null;
@@ -64,21 +80,21 @@ final class BridgeFactory
             $name = $matches[1];
         }
 
-        // Trim trailing 'Bridge' if exists
+        // Append 'Bridge' suffix if not present.
-        if (preg_match('/(.+)(?:Bridge)/i', $name, $matches)) {
+        if (!preg_match('/(Bridge)$/i', $name)) {
-            $name = $matches[1];
+            $name = sprintf('%sBridge', $name);
         }
 
         // Improve performance for correctly written bridge names
-        if (in_array($name, $this->getBridgeNames())) {
+        if (in_array($name, $this->getBridgeClassNames())) {
-            $index = array_search($name, $this->getBridgeNames());
+            $index = array_search($name, $this->getBridgeClassNames());
-            return $this->getBridgeNames()[$index];
+            return $this->getBridgeClassNames()[$index];
         }
 
         // The name is valid if a corresponding bridge file is found on disk
-        if (in_array(strtolower($name), array_map('strtolower', $this->getBridgeNames()))) {
+        if (in_array(strtolower($name), array_map('strtolower', $this->getBridgeClassNames()))) {
-            $index = array_search(strtolower($name), array_map('strtolower', $this->getBridgeNames()));
+            $index = array_search(strtolower($name), array_map('strtolower', $this->getBridgeClassNames()));
-            return $this->getBridgeNames()[$index];
+            return $this->getBridgeClassNames()[$index];
         }
 
         Debug::log('Invalid bridge name specified: "' . $name . '"!');

lib/BridgeList.php

@@ -66,20 +66,20 @@ EOD;
         $inactiveBridges = '';
 
         $bridgeFactory = new \BridgeFactory();
-        $bridgeNames = $bridgeFactory->getBridgeNames();
+        $bridgeClassNames = $bridgeFactory->getBridgeClassNames();
 
         $formatFactory = new FormatFactory();
         $formats = $formatFactory->getFormatNames();
 
-        $totalBridges = count($bridgeNames);
+        $totalBridges = count($bridgeClassNames);
 
-        foreach ($bridgeNames as $bridgeName) {
+        foreach ($bridgeClassNames as $bridgeClassName) {
-            if ($bridgeFactory->isWhitelisted($bridgeName)) {
+            if ($bridgeFactory->isWhitelisted($bridgeClassName)) {
-                $body .= BridgeCard::displayBridgeCard($bridgeName, $formats);
+                $body .= BridgeCard::displayBridgeCard($bridgeClassName, $formats);
                 $totalActiveBridges++;
             } elseif ($showInactive) {
                 // inactive bridges
-                $inactiveBridges .= BridgeCard::displayBridgeCard($bridgeName, $formats, false) . PHP_EOL;
+                $inactiveBridges .= BridgeCard::displayBridgeCard($bridgeClassName, $formats, false) . PHP_EOL;
             }
         }
 

tests/Actions/ActionImplementationTest.php

@@ -40,7 +40,7 @@ class ActionImplementationTest extends TestCase
 
         $this->setAction($path);
 
-        $methods = get_class_methods($this->obj);
+        $methods = array_diff(get_class_methods($this->obj), ['__construct']);
         sort($methods);
 
         $this->assertEquals($allowedMethods, $methods);

tests/Actions/ListActionTest.php

@@ -49,7 +49,7 @@ class ListActionTest extends TestCase
         $bridgeFactory = new BridgeFactory();
 
         $this->assertEquals(
-            count($bridgeFactory->getBridgeNames()),
+            count($bridgeFactory->getBridgeClassNames()),
             count($items['bridges']),
             'Number of bridges doesn\'t match'
         );