From dbf8c5b7ae48193033f3a3d6d907f6063a21721f Mon Sep 17 00:00:00 2001 From: Jan Tojnar Date: Fri, 8 Jul 2022 12:54:23 +0200 Subject: [PATCH] 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 | 27 ++++++--- actions/DetectAction.php | 8 +-- actions/DisplayAction.php | 14 +++-- actions/ListAction.php | 10 ++-- lib/BridgeCard.php | 34 ++++++------ lib/BridgeFactory.php | 64 ++++++++++++++-------- lib/BridgeList.php | 12 ++-- tests/Actions/ActionImplementationTest.php | 2 +- tests/Actions/ListActionTest.php | 2 +- 9 files changed, 102 insertions(+), 71 deletions(-) diff --git a/actions/ConnectivityAction.php b/actions/ConnectivityAction.php index dc7f3697..9a78d167 100644 --- a/actions/ConnectivityAction.php +++ b/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 $bridgeClassName Name of the bridge to generate the report for * @return void */ - private function reportBridgeConnectivity($bridgeName) + private function reportBridgeConnectivity($bridgeClassName) { - $bridgeFactory = new \BridgeFactory(); - - if (!$bridgeFactory->isWhitelisted($bridgeName)) { + if (!$this->bridgeFactory->isWhitelisted($bridgeClassName)) { 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); diff --git a/actions/DetectAction.php b/actions/DetectAction.php index bc34c0c7..cc5a7975 100644 --- a/actions/DetectAction.php +++ b/actions/DetectAction.php @@ -26,12 +26,12 @@ class DetectAction implements ActionInterface $bridgeFactory = new \BridgeFactory(); - foreach ($bridgeFactory->getBridgeNames() as $bridgeName) { - if (!$bridgeFactory->isWhitelisted($bridgeName)) { + foreach ($bridgeFactory->getBridgeClassNames() as $bridgeClassName) { + 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); diff --git a/actions/DisplayAction.php b/actions/DisplayAction.php index 5edf5c82..6561ad8a 100644 --- a/actions/DisplayAction.php +++ b/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) diff --git a/actions/ListAction.php b/actions/ListAction.php index 1ad96717..58a79ce5 100644 --- a/actions/ListAction.php +++ b/actions/ListAction.php @@ -22,20 +22,20 @@ class ListAction implements ActionInterface $bridgeFactory = new \BridgeFactory(); - foreach ($bridgeFactory->getBridgeNames() as $bridgeName) { - $bridge = $bridgeFactory->create($bridgeName); + foreach ($bridgeFactory->getBridgeClassNames() as $bridgeClassName) { + $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(), diff --git a/lib/BridgeCard.php b/lib/BridgeCard.php index 9ff7fcce..9e5b151e 100644 --- a/lib/BridgeCard.php +++ b/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 $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; if (!empty($parameterName)) { @@ -54,7 +54,7 @@ This bridge is not fetching its content through a secure connection'; /** * Get the form body for a bridge * - * @param string $bridgeName The bridge name + * @param class-string $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'; * @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 .= '
'; @@ -85,7 +85,7 @@ This bridge is not fetching its content through a secure connection
'; } $idArg = 'arg-' - . urlencode($bridgeName) + . urlencode($bridgeClassName) . '-' . urlencode($parameterName) . '-' @@ -297,16 +297,16 @@ This bridge is not fetching its content through a secure connection'; /** * Gets a single bridge card * - * @param string $bridgeName The bridge name + * @param class-string $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'; } $card = << +

{$name}

{$description}

- - + + 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 .= '
' . $parameterName . '
' . PHP_EOL; } - $card .= self::getForm($bridgeName, $formats, $isActive, $isHttps, $parameterName, $parameter); + $card .= self::getForm($bridgeClassName, $formats, $isActive, $isHttps, $parameterName, $parameter); } } - $card .= ''; + $card .= ''; if ($donationUri !== '' && $donationsAllowed) { $card .= '

' . $maintainer . ' ~ Donate

'; } else { diff --git a/lib/BridgeFactory.php b/lib/BridgeFactory.php index 3e355b7a..34602476 100644 --- a/lib/BridgeFactory.php +++ b/lib/BridgeFactory.php @@ -3,7 +3,9 @@ final class BridgeFactory { private $folder; - private $bridgeNames = []; + /** @var array> */ + private $bridgeClassNames = []; + /** @var array> */ 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)) { - $this->bridgeNames[] = $m[1]; + if (preg_match('/^([^.]+Bridge)\.php$/U', $file, $m)) { + $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 $name + */ public function create(string $name): BridgeInterface { - if (preg_match('/^[A-Z][a-zA-Z0-9-]*$/', $name)) { - $className = sprintf('%sBridge', $this->sanitizeBridgeName($name)); - return new $className(); - } - throw new \InvalidArgumentException('Bridge name invalid!'); + return new $name(); } - public function getBridgeNames(): array + /** + * @return array> + */ + public function getBridgeClassNames(): array { - return $this->bridgeNames; + return $this->bridgeClassNames; } - public function isWhitelisted($name): bool + /** + * @param class-string|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|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 - if (preg_match('/(.+)(?:Bridge)/i', $name, $matches)) { - $name = $matches[1]; + // Append 'Bridge' suffix if not present. + if (!preg_match('/(Bridge)$/i', $name)) { + $name = sprintf('%sBridge', $name); } // Improve performance for correctly written bridge names - if (in_array($name, $this->getBridgeNames())) { - $index = array_search($name, $this->getBridgeNames()); - return $this->getBridgeNames()[$index]; + if (in_array($name, $this->getBridgeClassNames())) { + $index = array_search($name, $this->getBridgeClassNames()); + 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()))) { - $index = array_search(strtolower($name), array_map('strtolower', $this->getBridgeNames())); - return $this->getBridgeNames()[$index]; + if (in_array(strtolower($name), array_map('strtolower', $this->getBridgeClassNames()))) { + $index = array_search(strtolower($name), array_map('strtolower', $this->getBridgeClassNames())); + return $this->getBridgeClassNames()[$index]; } Debug::log('Invalid bridge name specified: "' . $name . '"!'); diff --git a/lib/BridgeList.php b/lib/BridgeList.php index d3056e62..f8d0b1a1 100644 --- a/lib/BridgeList.php +++ b/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) { - if ($bridgeFactory->isWhitelisted($bridgeName)) { - $body .= BridgeCard::displayBridgeCard($bridgeName, $formats); + foreach ($bridgeClassNames as $bridgeClassName) { + if ($bridgeFactory->isWhitelisted($bridgeClassName)) { + $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; } } diff --git a/tests/Actions/ActionImplementationTest.php b/tests/Actions/ActionImplementationTest.php index 3f063682..bf5dc4f9 100644 --- a/tests/Actions/ActionImplementationTest.php +++ b/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); diff --git a/tests/Actions/ListActionTest.php b/tests/Actions/ListActionTest.php index 437f184f..5056050e 100644 --- a/tests/Actions/ListActionTest.php +++ b/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' );