diff --git a/lib/Db/ExAppMapper.php b/lib/Db/ExAppMapper.php index a50ab1d0..5b337d35 100644 --- a/lib/Db/ExAppMapper.php +++ b/lib/Db/ExAppMapper.php @@ -205,30 +205,24 @@ public function updateExApp(ExApp $exApp, array $fields): int { } /** + * Insert routes for an ExApp. Caller is responsible for validating shape — see + * {@see \OCA\AppAPI\Service\ExAppRouteHelper::normalizeAndValidate()}, which is invoked + * by ExAppService::getAppInfo() before reaching this method. + * * @throws Exception */ public function registerExAppRoutes(ExApp $exApp, array $routes): int { $qb = $this->db->getQueryBuilder(); $count = 0; foreach ($routes as $route) { - if (isset($route['bruteforce_protection']) && is_string($route['bruteforce_protection'])) { - $route['bruteforce_protection'] = json_decode($route['bruteforce_protection'], false); - } - if (!isset($route['headers_to_exclude'])) { - $route['headers_to_exclude'] = []; - } $qb->insert('ex_apps_routes') ->values([ 'appid' => $qb->createNamedParameter($exApp->getAppid()), 'url' => $qb->createNamedParameter($route['url']), 'verb' => $qb->createNamedParameter($route['verb']), - 'access_level' => $qb->createNamedParameter($route['access_level']), - 'headers_to_exclude' => $qb->createNamedParameter(is_array($route['headers_to_exclude']) ? json_encode($route['headers_to_exclude']) : '[]'), - 'bruteforce_protection' => $qb->createNamedParameter( - isset($route['bruteforce_protection']) && is_array($route['bruteforce_protection']) - ? json_encode($route['bruteforce_protection']) - : '[]' - ), + 'access_level' => $qb->createNamedParameter($route['access_level'], IQueryBuilder::PARAM_INT), + 'headers_to_exclude' => $qb->createNamedParameter(json_encode($route['headers_to_exclude'])), + 'bruteforce_protection' => $qb->createNamedParameter(json_encode($route['bruteforce_protection'])), ]); $count += $qb->executeStatement(); } diff --git a/lib/Service/ExAppRouteHelper.php b/lib/Service/ExAppRouteHelper.php new file mode 100644 index 00000000..188859d3 --- /dev/null +++ b/lib/Service/ExAppRouteHelper.php @@ -0,0 +1,158 @@ +[401]`), access_level as `PUBLIC|USER|ADMIN`. + * + * The helper produces a canonical structure: access_level as 0/1/2, bruteforce_protection as int[], + * headers_to_exclude as string[]. Anything that cannot be reconciled to that shape is rejected with a + * descriptive message — devs see the actual problem instead of the values being silently coerced to `[]`. + */ +class ExAppRouteHelper { + private const ACCESS_LEVEL_BY_NAME = [ + 'PUBLIC' => 0, + 'USER' => 1, + 'ADMIN' => 2, + ]; + + /** + * @param array $routes raw route entries from getAppInfo's shape-collapse step + * @return array normalized routes ready for ExAppMapper::registerExAppRoutes + * @throws InvalidArgumentException on the first malformed field; message identifies the route and field + */ + public static function normalizeAndValidate(array $routes): array { + $normalized = []; + foreach ($routes as $index => $route) { + if (!is_array($route)) { + throw new InvalidArgumentException(sprintf('route #%d: entry must be an object, got %s', $index, get_debug_type($route))); + } + $normalized[] = self::normalizeRoute($route, $index); + } + return $normalized; + } + + private static function normalizeRoute(array $route, int $index): array { + $url = $route['url'] ?? null; + if (!is_string($url) || trim($url) === '') { + throw new InvalidArgumentException(sprintf("route #%d: 'url' must be a non-empty string, got %s", $index, self::describe($url))); + } + $ident = sprintf("route '%s'", $url); + + $verb = $route['verb'] ?? null; + if (!is_string($verb) || trim($verb) === '') { + throw new InvalidArgumentException(sprintf("%s: 'verb' must be a non-empty string (e.g. 'GET' or 'GET,POST'), got %s", $ident, self::describe($verb))); + } + + return [ + 'url' => $url, + 'verb' => $verb, + 'access_level' => self::normalizeAccessLevel($route['access_level'] ?? null, $ident), + 'bruteforce_protection' => self::normalizeIntList($route['bruteforce_protection'] ?? null, $ident, 'bruteforce_protection'), + 'headers_to_exclude' => self::normalizeStringList($route['headers_to_exclude'] ?? null, $ident, 'headers_to_exclude'), + ]; + } + + private static function normalizeAccessLevel(mixed $raw, string $ident): int { + if (is_string($raw)) { + if (!array_key_exists($raw, self::ACCESS_LEVEL_BY_NAME)) { + throw new InvalidArgumentException(sprintf("%s: invalid 'access_level' '%s' (allowed: PUBLIC, USER, ADMIN)", $ident, $raw)); + } + return self::ACCESS_LEVEL_BY_NAME[$raw]; + } + if (is_int($raw)) { + if (!in_array($raw, self::ACCESS_LEVEL_BY_NAME, true)) { + throw new InvalidArgumentException(sprintf("%s: invalid 'access_level' %d (allowed: 0=PUBLIC, 1=USER, 2=ADMIN)", $ident, $raw)); + } + return $raw; + } + throw new InvalidArgumentException(sprintf("%s: 'access_level' is required and must be one of PUBLIC|USER|ADMIN (or 0|1|2), got %s", $ident, self::describe($raw))); + } + + /** + * Accept array, a JSON-encoded array of ints (from XML body), null, or empty string. + * Reject anything else. + */ + private static function normalizeIntList(mixed $raw, string $ident, string $field): array { + $list = self::decodeListOrNull($raw, $ident, $field); + if ($list === null) { + return []; + } + $out = []; + foreach ($list as $index => $value) { + if (!is_int($value)) { + throw new InvalidArgumentException(sprintf("%s: '%s' must contain only integers (e.g. HTTP status codes), entry at index %d is %s", $ident, $field, $index, self::describe($value))); + } + $out[] = $value; + } + return $out; + } + + /** + * Accept array, a JSON-encoded array of strings (from XML body), null, or empty string. + * Reject anything else. + */ + private static function normalizeStringList(mixed $raw, string $ident, string $field): array { + $list = self::decodeListOrNull($raw, $ident, $field); + if ($list === null) { + return []; + } + $out = []; + foreach ($list as $index => $value) { + if (!is_string($value)) { + throw new InvalidArgumentException(sprintf("%s: '%s' must contain only strings (header names), entry at index %d is %s", $ident, $field, $index, self::describe($value))); + } + $out[] = $value; + } + return $out; + } + + /** + * Resolve the raw list field to either a PHP list (caller validates element types) + * or null (= field is unset / explicitly empty). Throw for anything else, including + * associative arrays / JSON objects — those usually indicate the developer authored XML + * sub-elements (`401`) instead of the + * documented JSON-string body (`[401]`), and dropping the + * keys silently would hide that mistake. + */ + private static function decodeListOrNull(mixed $raw, string $ident, string $field): ?array { + if ($raw === null || $raw === '' || $raw === []) { + return null; + } + if (is_array($raw)) { + if (!array_is_list($raw)) { + throw new InvalidArgumentException(sprintf("%s: '%s' must be a JSON array (list), got an associative object with keys %s — use a JSON-encoded array body in info.xml (e.g. '[401,429]')", $ident, $field, json_encode(array_keys($raw)))); + } + return $raw; + } + if (is_string($raw)) { + $decoded = json_decode($raw, true); + if (!is_array($decoded) || !array_is_list($decoded)) { + throw new InvalidArgumentException(sprintf("%s: '%s' must be a JSON array, got string '%s'", $ident, $field, $raw)); + } + return $decoded; + } + throw new InvalidArgumentException(sprintf("%s: '%s' must be an array (or a JSON-encoded array string), got %s", $ident, $field, self::describe($raw))); + } + + private static function describe(mixed $value): string { + if (is_string($value)) { + return sprintf("'%s' (string)", $value); + } + return get_debug_type($value); + } +} diff --git a/lib/Service/ExAppService.php b/lib/Service/ExAppService.php index 48c7e426..2682a368 100644 --- a/lib/Service/ExAppService.php +++ b/lib/Service/ExAppService.php @@ -9,6 +9,7 @@ namespace OCA\AppAPI\Service; +use InvalidArgumentException; use OCA\AppAPI\AppInfo\Application; use OCA\AppAPI\Db\ExApp; use OCA\AppAPI\Db\ExAppMapper; @@ -287,15 +288,6 @@ public function getAppInfo(string $appId, ?string $infoXml, ?string $jsonInfo, ? } else { $appInfo['external-app']['routes'] = [$appInfo['external-app']['routes']['route']]; } - // update routes, map string access_level to int - $appInfo['external-app']['routes'] = array_map(function ($route) use ($appId) { - $route['access_level'] = $this->mapExAppRouteAccessLevelNameToNumber($route['access_level']); - if ($route['access_level'] !== -1) { - return $route; - } else { - $this->logger->error(sprintf('Invalid access level `%s` for route `%s` in ExApp `%s`', $route['access_level'], $route['url'], $appId)); - } - }, $appInfo['external-app']['routes']); } // Advanced deploy options if (isset($appInfo['external-app']['environment-variables']['variable'])) { @@ -350,21 +342,22 @@ public function getAppInfo(string $appId, ?string $infoXml, ?string $jsonInfo, ? } } } + if (isset($appInfo['external-app']['routes'])) { + if (!is_array($appInfo['external-app']['routes'])) { + return ['error' => sprintf("ExApp '%s' has invalid route definition. 'routes' must be a list of route objects, got %s", $appId, get_debug_type($appInfo['external-app']['routes']))]; + } + try { + $appInfo['external-app']['routes'] = ExAppRouteHelper::normalizeAndValidate($appInfo['external-app']['routes']); + } catch (InvalidArgumentException $e) { + return ['error' => sprintf("ExApp '%s' has invalid route definition. %s", $appId, $e->getMessage())]; + } + } return $appInfo; } - public function mapExAppRouteAccessLevelNameToNumber(string $accessLevel): int { - return match($accessLevel) { - 'PUBLIC' => 0, - 'USER' => 1, - 'ADMIN' => 2, - default => -1, - }; - } - public function setAppDeployProgress(ExApp $exApp, int $progress, string $error = ''): void { if ($progress < 0 || $progress > 100) { - throw new \InvalidArgumentException('Invalid ExApp deploy status progress value'); + throw new InvalidArgumentException('Invalid ExApp deploy status progress value'); } $status = $exApp->getStatus(); if ($progress !== 0 && isset($status['deploy']) && $status['deploy'] === 100) { diff --git a/tests/php/Service/ExAppRouteHelperTest.php b/tests/php/Service/ExAppRouteHelperTest.php new file mode 100644 index 00000000..70bf4afc --- /dev/null +++ b/tests/php/Service/ExAppRouteHelperTest.php @@ -0,0 +1,220 @@ + [ + [[ + 'url' => '/api/.*', + 'verb' => 'GET,POST', + 'access_level' => 1, + 'bruteforce_protection' => [401, 403], + 'headers_to_exclude' => ['Cookie', 'Authorization'], + ]], + [[ + 'url' => '/api/.*', + 'verb' => 'GET,POST', + 'access_level' => 1, + 'bruteforce_protection' => [401, 403], + 'headers_to_exclude' => ['Cookie', 'Authorization'], + ]], + ], + 'XML shape — access_level string + JSON-encoded list bodies' => [ + [[ + 'url' => '/.*', + 'verb' => 'GET', + 'access_level' => 'USER', + 'bruteforce_protection' => '[401,429]', + 'headers_to_exclude' => '["Cookie"]', + ]], + [[ + 'url' => '/.*', + 'verb' => 'GET', + 'access_level' => 1, + 'bruteforce_protection' => [401, 429], + 'headers_to_exclude' => ['Cookie'], + ]], + ], + 'XML shape — empty element bodies (SimpleXML produces empty arrays)' => [ + // arrives as [] after simplexml→json roundtrip + [[ + 'url' => '/.*', + 'verb' => 'POST', + 'access_level' => 'PUBLIC', + 'bruteforce_protection' => [], + 'headers_to_exclude' => [], + ]], + [[ + 'url' => '/.*', + 'verb' => 'POST', + 'access_level' => 0, + 'bruteforce_protection' => [], + 'headers_to_exclude' => [], + ]], + ], + 'JSON path — developer sends empty strings for list fields' => [ + // Defensive: --json-info '{"routes":[{"headers_to_exclude":""}]}' + [[ + 'url' => '/.*', + 'verb' => 'POST', + 'access_level' => 'PUBLIC', + 'bruteforce_protection' => '', + 'headers_to_exclude' => '', + ]], + [[ + 'url' => '/.*', + 'verb' => 'POST', + 'access_level' => 0, + 'bruteforce_protection' => [], + 'headers_to_exclude' => [], + ]], + ], + 'omitted optional fields default to empty' => [ + [[ + 'url' => '/.*', + 'verb' => 'GET', + 'access_level' => 'ADMIN', + ]], + [[ + 'url' => '/.*', + 'verb' => 'GET', + 'access_level' => 2, + 'bruteforce_protection' => [], + 'headers_to_exclude' => [], + ]], + ], + 'multiple routes preserve order' => [ + [ + ['url' => '/a', 'verb' => 'GET', 'access_level' => 'USER'], + ['url' => '/b', 'verb' => 'POST', 'access_level' => 'ADMIN'], + ], + [ + ['url' => '/a', 'verb' => 'GET', 'access_level' => 1, 'bruteforce_protection' => [], 'headers_to_exclude' => []], + ['url' => '/b', 'verb' => 'POST', 'access_level' => 2, 'bruteforce_protection' => [], 'headers_to_exclude' => []], + ], + ], + 'empty input list' => [[], []], + ]; + } + + /** + * Every rejection case names the offending field in the message — that is the whole + * point of fail-fast registration: developers must be able to read the OCC output and + * know exactly which route entry to fix in their info.xml or --json-info payload. + */ + #[DataProvider('invalidRouteProvider')] + public function testNormalizeAndValidateRejects(array $input, string $expectedMessageFragment): void { + try { + ExAppRouteHelper::normalizeAndValidate($input); + self::fail('Expected InvalidArgumentException, none thrown'); + } catch (InvalidArgumentException $e) { + self::assertStringContainsString($expectedMessageFragment, $e->getMessage()); + } + } + + public static function invalidRouteProvider(): array { + return [ + 'entry is not an array' => [ + ['not-an-object'], + 'route #0: entry must be an object', + ], + 'missing url' => [ + [['verb' => 'GET', 'access_level' => 'USER']], + "route #0: 'url' must be a non-empty string", + ], + 'empty url' => [ + [['url' => '', 'verb' => 'GET', 'access_level' => 'USER']], + "'url' must be a non-empty string", + ], + 'missing verb' => [ + [['url' => '/.*', 'access_level' => 'USER']], + "'verb' must be a non-empty string", + ], + 'verb is array' => [ + [['url' => '/.*', 'verb' => ['GET', 'POST'], 'access_level' => 'USER']], + "'verb' must be a non-empty string", + ], + 'missing access_level' => [ + [['url' => '/.*', 'verb' => 'GET']], + "'access_level' is required", + ], + 'unknown access_level string' => [ + [['url' => '/.*', 'verb' => 'GET', 'access_level' => 'SUPERUSER']], + "invalid 'access_level' 'SUPERUSER'", + ], + 'out-of-range access_level int' => [ + [['url' => '/.*', 'verb' => 'GET', 'access_level' => 99]], + "invalid 'access_level' 99", + ], + 'bruteforce_protection contains a string' => [ + [['url' => '/.*', 'verb' => 'GET', 'access_level' => 'USER', 'bruteforce_protection' => [401, 'oops']]], + "'bruteforce_protection' must contain only integers", + ], + 'bruteforce_protection is JSON of strings' => [ + [['url' => '/.*', 'verb' => 'GET', 'access_level' => 'USER', 'bruteforce_protection' => '["401"]']], + "'bruteforce_protection' must contain only integers", + ], + 'bruteforce_protection is malformed JSON' => [ + [['url' => '/.*', 'verb' => 'GET', 'access_level' => 'USER', 'bruteforce_protection' => '{not-json']], + "'bruteforce_protection' must be a JSON array", + ], + 'bruteforce_protection is scalar int' => [ + [['url' => '/.*', 'verb' => 'GET', 'access_level' => 'USER', 'bruteforce_protection' => 401]], + "'bruteforce_protection' must be an array", + ], + 'headers_to_exclude contains an int' => [ + [['url' => '/.*', 'verb' => 'GET', 'access_level' => 'USER', 'headers_to_exclude' => ['Cookie', 42]]], + "'headers_to_exclude' must contain only strings", + ], + 'headers_to_exclude is JSON of ints' => [ + [['url' => '/.*', 'verb' => 'GET', 'access_level' => 'USER', 'headers_to_exclude' => '[1,2]']], + "'headers_to_exclude' must contain only strings", + ], + 'second route is the broken one' => [ + [ + ['url' => '/ok', 'verb' => 'GET', 'access_level' => 'USER'], + ['url' => '/bad', 'verb' => 'GET', 'access_level' => 'WRONG'], + ], + "route '/bad': invalid 'access_level' 'WRONG'", + ], + 'bruteforce_protection is JSON object' => [ + [['url' => '/.*', 'verb' => 'GET', 'access_level' => 'USER', 'bruteforce_protection' => '{"x":401}']], + "'bruteforce_protection' must be a JSON array", + ], + 'bruteforce_protection is associative array (XML sub-element shape)' => [ + [['url' => '/.*', 'verb' => 'GET', 'access_level' => 'USER', 'bruteforce_protection' => ['status-code' => 401]]], + "'bruteforce_protection' must be a JSON array (list)", + ], + 'headers_to_exclude is associative array' => [ + [['url' => '/.*', 'verb' => 'GET', 'access_level' => 'USER', 'headers_to_exclude' => ['header' => 'Cookie']]], + "'headers_to_exclude' must be a JSON array (list)", + ], + ]; + } +}