Skip to content

feat: validate ExApp routes at registration and reject malformed data #885

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
oleksandr-nc wants to merge 1 commit into
base: main
Choose a base branch
from
+397 −32
Open

feat: validate ExApp routes at registration and reject malformed data #885

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
20 changes: 7 additions & 13 deletions lib/Db/ExAppMapper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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();
}
Expand Down
158 changes: 158 additions & 0 deletions lib/Service/ExAppRouteHelper.php
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,158 @@
<?php

declare(strict_types=1);

/**
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
* SPDX-License-Identifier: AGPL-3.0-or-later
*/

namespace OCA\AppAPI\Service;

use InvalidArgumentException;

/**
* Normalize and validate ExApp routes from info.xml / --json-info before they are persisted.
*
* Two input shapes feed this helper:
* - JSON (`--json-info`): values are already typed (access_level: int, bruteforce_protection: int[], headers_to_exclude: string[]).
* - XML (`--info-xml` / appstore): JSON-encoded lists arrive as strings inside element bodies
* (`<bruteforce_protection>[401]</bruteforce_protection>`), 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<int>, 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<string>, 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 (`<bruteforce_protection><status>401</status></...>`) instead of the
* documented JSON-string body (`<bruteforce_protection>[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);
}
}
31 changes: 12 additions & 19 deletions lib/Service/ExAppService.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand Down Expand Up @@ -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'])) {
Expand Down Expand Up @@ -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) {
Expand Down
Loading
Loading