Skip to content

Commit cfa6da1

Browse files
authored
Merge pull request #885 from nextcloud/feat/validate-exapp-routes-on-register
feat: validate ExApp routes at registration and reject malformed data
2 parents 2c9093f + 8d57f2e commit cfa6da1

4 files changed

Lines changed: 397 additions & 32 deletions

File tree

lib/Db/ExAppMapper.php

Lines changed: 7 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -205,30 +205,24 @@ public function updateExApp(ExApp $exApp, array $fields): int {
205205
}
206206

207207
/**
208+
* Insert routes for an ExApp. Caller is responsible for validating shape — see
209+
* {@see \OCA\AppAPI\Service\ExAppRouteHelper::normalizeAndValidate()}, which is invoked
210+
* by ExAppService::getAppInfo() before reaching this method.
211+
*
208212
* @throws Exception
209213
*/
210214
public function registerExAppRoutes(ExApp $exApp, array $routes): int {
211215
$qb = $this->db->getQueryBuilder();
212216
$count = 0;
213217
foreach ($routes as $route) {
214-
if (isset($route['bruteforce_protection']) && is_string($route['bruteforce_protection'])) {
215-
$route['bruteforce_protection'] = json_decode($route['bruteforce_protection'], false);
216-
}
217-
if (!isset($route['headers_to_exclude'])) {
218-
$route['headers_to_exclude'] = [];
219-
}
220218
$qb->insert('ex_apps_routes')
221219
->values([
222220
'appid' => $qb->createNamedParameter($exApp->getAppid()),
223221
'url' => $qb->createNamedParameter($route['url']),
224222
'verb' => $qb->createNamedParameter($route['verb']),
225-
'access_level' => $qb->createNamedParameter($route['access_level']),
226-
'headers_to_exclude' => $qb->createNamedParameter(is_array($route['headers_to_exclude']) ? json_encode($route['headers_to_exclude']) : '[]'),
227-
'bruteforce_protection' => $qb->createNamedParameter(
228-
isset($route['bruteforce_protection']) && is_array($route['bruteforce_protection'])
229-
? json_encode($route['bruteforce_protection'])
230-
: '[]'
231-
),
223+
'access_level' => $qb->createNamedParameter($route['access_level'], IQueryBuilder::PARAM_INT),
224+
'headers_to_exclude' => $qb->createNamedParameter(json_encode($route['headers_to_exclude'])),
225+
'bruteforce_protection' => $qb->createNamedParameter(json_encode($route['bruteforce_protection'])),
232226
]);
233227
$count += $qb->executeStatement();
234228
}

lib/Service/ExAppRouteHelper.php

Lines changed: 158 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,158 @@
1+
<?php
2+
3+
declare(strict_types=1);
4+
5+
/**
6+
* SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors
7+
* SPDX-License-Identifier: AGPL-3.0-or-later
8+
*/
9+
10+
namespace OCA\AppAPI\Service;
11+
12+
use InvalidArgumentException;
13+
14+
/**
15+
* Normalize and validate ExApp routes from info.xml / --json-info before they are persisted.
16+
*
17+
* Two input shapes feed this helper:
18+
* - JSON (`--json-info`): values are already typed (access_level: int, bruteforce_protection: int[], headers_to_exclude: string[]).
19+
* - XML (`--info-xml` / appstore): JSON-encoded lists arrive as strings inside element bodies
20+
* (`<bruteforce_protection>[401]</bruteforce_protection>`), access_level as `PUBLIC|USER|ADMIN`.
21+
*
22+
* The helper produces a canonical structure: access_level as 0/1/2, bruteforce_protection as int[],
23+
* headers_to_exclude as string[]. Anything that cannot be reconciled to that shape is rejected with a
24+
* descriptive message — devs see the actual problem instead of the values being silently coerced to `[]`.
25+
*/
26+
class ExAppRouteHelper {
27+
private const ACCESS_LEVEL_BY_NAME = [
28+
'PUBLIC' => 0,
29+
'USER' => 1,
30+
'ADMIN' => 2,
31+
];
32+
33+
/**
34+
* @param array $routes raw route entries from getAppInfo's shape-collapse step
35+
* @return array normalized routes ready for ExAppMapper::registerExAppRoutes
36+
* @throws InvalidArgumentException on the first malformed field; message identifies the route and field
37+
*/
38+
public static function normalizeAndValidate(array $routes): array {
39+
$normalized = [];
40+
foreach ($routes as $index => $route) {
41+
if (!is_array($route)) {
42+
throw new InvalidArgumentException(sprintf('route #%d: entry must be an object, got %s', $index, get_debug_type($route)));
43+
}
44+
$normalized[] = self::normalizeRoute($route, $index);
45+
}
46+
return $normalized;
47+
}
48+
49+
private static function normalizeRoute(array $route, int $index): array {
50+
$url = $route['url'] ?? null;
51+
if (!is_string($url) || trim($url) === '') {
52+
throw new InvalidArgumentException(sprintf("route #%d: 'url' must be a non-empty string, got %s", $index, self::describe($url)));
53+
}
54+
$ident = sprintf("route '%s'", $url);
55+
56+
$verb = $route['verb'] ?? null;
57+
if (!is_string($verb) || trim($verb) === '') {
58+
throw new InvalidArgumentException(sprintf("%s: 'verb' must be a non-empty string (e.g. 'GET' or 'GET,POST'), got %s", $ident, self::describe($verb)));
59+
}
60+
61+
return [
62+
'url' => $url,
63+
'verb' => $verb,
64+
'access_level' => self::normalizeAccessLevel($route['access_level'] ?? null, $ident),
65+
'bruteforce_protection' => self::normalizeIntList($route['bruteforce_protection'] ?? null, $ident, 'bruteforce_protection'),
66+
'headers_to_exclude' => self::normalizeStringList($route['headers_to_exclude'] ?? null, $ident, 'headers_to_exclude'),
67+
];
68+
}
69+
70+
private static function normalizeAccessLevel(mixed $raw, string $ident): int {
71+
if (is_string($raw)) {
72+
if (!array_key_exists($raw, self::ACCESS_LEVEL_BY_NAME)) {
73+
throw new InvalidArgumentException(sprintf("%s: invalid 'access_level' '%s' (allowed: PUBLIC, USER, ADMIN)", $ident, $raw));
74+
}
75+
return self::ACCESS_LEVEL_BY_NAME[$raw];
76+
}
77+
if (is_int($raw)) {
78+
if (!in_array($raw, self::ACCESS_LEVEL_BY_NAME, true)) {
79+
throw new InvalidArgumentException(sprintf("%s: invalid 'access_level' %d (allowed: 0=PUBLIC, 1=USER, 2=ADMIN)", $ident, $raw));
80+
}
81+
return $raw;
82+
}
83+
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)));
84+
}
85+
86+
/**
87+
* Accept array<int>, a JSON-encoded array of ints (from XML body), null, or empty string.
88+
* Reject anything else.
89+
*/
90+
private static function normalizeIntList(mixed $raw, string $ident, string $field): array {
91+
$list = self::decodeListOrNull($raw, $ident, $field);
92+
if ($list === null) {
93+
return [];
94+
}
95+
$out = [];
96+
foreach ($list as $index => $value) {
97+
if (!is_int($value)) {
98+
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)));
99+
}
100+
$out[] = $value;
101+
}
102+
return $out;
103+
}
104+
105+
/**
106+
* Accept array<string>, a JSON-encoded array of strings (from XML body), null, or empty string.
107+
* Reject anything else.
108+
*/
109+
private static function normalizeStringList(mixed $raw, string $ident, string $field): array {
110+
$list = self::decodeListOrNull($raw, $ident, $field);
111+
if ($list === null) {
112+
return [];
113+
}
114+
$out = [];
115+
foreach ($list as $index => $value) {
116+
if (!is_string($value)) {
117+
throw new InvalidArgumentException(sprintf("%s: '%s' must contain only strings (header names), entry at index %d is %s", $ident, $field, $index, self::describe($value)));
118+
}
119+
$out[] = $value;
120+
}
121+
return $out;
122+
}
123+
124+
/**
125+
* Resolve the raw list field to either a PHP list (caller validates element types)
126+
* or null (= field is unset / explicitly empty). Throw for anything else, including
127+
* associative arrays / JSON objects — those usually indicate the developer authored XML
128+
* sub-elements (`<bruteforce_protection><status>401</status></...>`) instead of the
129+
* documented JSON-string body (`<bruteforce_protection>[401]</...>`), and dropping the
130+
* keys silently would hide that mistake.
131+
*/
132+
private static function decodeListOrNull(mixed $raw, string $ident, string $field): ?array {
133+
if ($raw === null || $raw === '' || $raw === []) {
134+
return null;
135+
}
136+
if (is_array($raw)) {
137+
if (!array_is_list($raw)) {
138+
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))));
139+
}
140+
return $raw;
141+
}
142+
if (is_string($raw)) {
143+
$decoded = json_decode($raw, true);
144+
if (!is_array($decoded) || !array_is_list($decoded)) {
145+
throw new InvalidArgumentException(sprintf("%s: '%s' must be a JSON array, got string '%s'", $ident, $field, $raw));
146+
}
147+
return $decoded;
148+
}
149+
throw new InvalidArgumentException(sprintf("%s: '%s' must be an array (or a JSON-encoded array string), got %s", $ident, $field, self::describe($raw)));
150+
}
151+
152+
private static function describe(mixed $value): string {
153+
if (is_string($value)) {
154+
return sprintf("'%s' (string)", $value);
155+
}
156+
return get_debug_type($value);
157+
}
158+
}

lib/Service/ExAppService.php

Lines changed: 12 additions & 19 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@
99

1010
namespace OCA\AppAPI\Service;
1111

12+
use InvalidArgumentException;
1213
use OCA\AppAPI\AppInfo\Application;
1314
use OCA\AppAPI\Db\ExApp;
1415
use OCA\AppAPI\Db\ExAppMapper;
@@ -287,15 +288,6 @@ public function getAppInfo(string $appId, ?string $infoXml, ?string $jsonInfo, ?
287288
} else {
288289
$appInfo['external-app']['routes'] = [$appInfo['external-app']['routes']['route']];
289290
}
290-
// update routes, map string access_level to int
291-
$appInfo['external-app']['routes'] = array_map(function ($route) use ($appId) {
292-
$route['access_level'] = $this->mapExAppRouteAccessLevelNameToNumber($route['access_level']);
293-
if ($route['access_level'] !== -1) {
294-
return $route;
295-
} else {
296-
$this->logger->error(sprintf('Invalid access level `%s` for route `%s` in ExApp `%s`', $route['access_level'], $route['url'], $appId));
297-
}
298-
}, $appInfo['external-app']['routes']);
299291
}
300292
// Advanced deploy options
301293
if (isset($appInfo['external-app']['environment-variables']['variable'])) {
@@ -350,21 +342,22 @@ public function getAppInfo(string $appId, ?string $infoXml, ?string $jsonInfo, ?
350342
}
351343
}
352344
}
345+
if (isset($appInfo['external-app']['routes'])) {
346+
if (!is_array($appInfo['external-app']['routes'])) {
347+
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']))];
348+
}
349+
try {
350+
$appInfo['external-app']['routes'] = ExAppRouteHelper::normalizeAndValidate($appInfo['external-app']['routes']);
351+
} catch (InvalidArgumentException $e) {
352+
return ['error' => sprintf("ExApp '%s' has invalid route definition. %s", $appId, $e->getMessage())];
353+
}
354+
}
353355
return $appInfo;
354356
}
355357

356-
public function mapExAppRouteAccessLevelNameToNumber(string $accessLevel): int {
357-
return match($accessLevel) {
358-
'PUBLIC' => 0,
359-
'USER' => 1,
360-
'ADMIN' => 2,
361-
default => -1,
362-
};
363-
}
364-
365358
public function setAppDeployProgress(ExApp $exApp, int $progress, string $error = ''): void {
366359
if ($progress < 0 || $progress > 100) {
367-
throw new \InvalidArgumentException('Invalid ExApp deploy status progress value');
360+
throw new InvalidArgumentException('Invalid ExApp deploy status progress value');
368361
}
369362
$status = $exApp->getStatus();
370363
if ($progress !== 0 && isset($status['deploy']) && $status['deploy'] === 100) {

0 commit comments

Comments
 (0)