diff --git a/README.md b/README.md index b212734..e640c5d 100644 --- a/README.md +++ b/README.md @@ -719,6 +719,8 @@ image has to be present on every node that can run the pod. 5. List in the `` of each route in your `info.xml` every HTTP method that route serves, separated by commas (e.g. `GET,POST`). As in AppAPI's PHP proxy, the first route whose URL and verb both match a request decides its access level, and `HEAD` is allowed wherever `GET` is listed. HaRP answers a method that no route of the path lists with `405 Method Not Allowed`. HaRP 0.4.7 and older, and HaRP with an AppAPI version that does not send the verbs, match routes by URL only: there the first route whose URL matches decides for every method, so use different URLs where methods need different access levels. +6. Write the URLs of your routes for the decoded path, as your ExApp reads it, and start them with `/` or `^/`. Since HaRP 0.4.8 the route check of a user's request uses its path decoded once and with `//` merged. HaRP forwards a user's request with its path as sent, except that escaped letters, digits and `-._~` are decoded, `//` is merged, a raw `\` becomes `%5C` and a `%` that starts no escape becomes `%25`; requests from AppAPI keep their path. If another reading of the path matches a route with a higher access level, that level applies, because some servers read the path that way (in another letter case, without a trailing slash, without `;` parameters, with `%2F` kept inside its segment, or decoded without merging `//`) and an ExApp that passes the decoded path on to another server makes an escaped `?` or `#` end the path and an escaped `;` start a parameter. So that a directory URL like `/img/` keeps the level of its route, write that route as `^/img(/.*)?$`. Paths with `.` or `..` segments are refused. Paths with control characters, and the `/heartbeat`, `/init` and `/enabled` endpoints in any spelling, are refused unless AppAPI sends the request. + That's it! Your ExApp is now adapted to Nextcloud 32. ## Nextcloud 32: Migrating Existing ExApps from DSP to HaRP diff --git a/haproxy_agent.py b/haproxy_agent.py index 2825e56..57da035 100644 --- a/haproxy_agent.py +++ b/haproxy_agent.py @@ -22,7 +22,7 @@ from enum import IntEnum from ipaddress import IPv4Address, IPv6Address, ip_address from typing import Any, Literal, Self -from urllib.parse import quote +from urllib.parse import quote, unquote import aiohttp from aiohttp import web @@ -43,9 +43,17 @@ HARP_VERSION = "0.4.7" APPID_PATTERN = re.compile(r"(?:^|/)exapps/([^/]+)") -# Matches `..` path segments in any literal/encoded form: separators can be `/` or %2F, -# the dots themselves can be `.` or %2E (any case combination, e.g. `..`, `%2e.`, `%2E%2e`). -DOT_DOT_SEGMENT_PATTERN = re.compile(r"(?:^|/|%2[fF])(?:\.|%2[eE]){2}(?:/|%2[fF]|$)") +# Matches `.` and `..` path segments in any literal/encoded form: separators can be `/`, %2F or `\` (a slash for WHATWG +# URL parsers), the dots `.` or %2E (any case), and the segment may go on with `;` or %3B (Tomcat drops parameters), +# or end at %3F or %23 (a wrapper ExApp that decodes the path cuts it there), as in AppAPI's PHP proxy. +DOT_SEGMENT_PATTERN = re.compile(r"(?:^|[/\\]|%2[fF])(?:\.|%2[eE]){1,2}(?:[/;\\]|%2[fF]|%3[bBfF]|%23|$)") +# An escaped RFC 3986 unreserved character (letters, digits, `-._~`): it means the same escaped or not. +UNRESERVED_ESCAPE_PATTERN = re.compile(r"%(?:[46][1-9A-Fa-f]|[57][0-9Aa]|3[0-9]|2[DdEe]|5[Ff]|7[Ee])") +LONE_PERCENT_PATTERN = re.compile(r"%(?![0-9A-Fa-f]{2})") +# The reply to HAProxy carries the forwarded path and must fit one SPOE frame (16380 bytes). +MAX_FORWARDED_PATH_LENGTH = 14 * 1024 +CONTROL_CHARACTER_PATTERN = re.compile(r"[\x00-\x1f\x7f]") +LIFECYCLE_ENDPOINTS = ("heartbeat", "init", "enabled") SHARED_KEY = os.environ.get("HP_SHARED_KEY") # ExApp containers get this token instead of the shared key (see `with_exapp_frp_token`). It is accepted only for # the FRP login, so an ExApp cannot use the control API. Deployed containers carry it: the derivation must not change. @@ -691,8 +699,37 @@ async def get_session(pass_cookie: str) -> NcUser | None: ############################################################################### -def find_route( - routes: list[ExAppRoute], target_path: str, method: str, exapp_id: str +def _unescaped(match: re.Match) -> str: + return chr(int(match[0][1:], 16)) + + +def exapp_paths(target_path: str) -> tuple[str, list[str]]: + """The path to forward to the ExApp, and the ways servers read it, starting with the path its routes are checked on. + + Servers differ on escaped unreserved characters, a `%` that starts no escape, repeated slashes and a raw backslash + (a slash for WHATWG URL parsers), so the forwarded path has none of them. The checked path is the forwarded one + decoded once, with `%2F` as a slash, and `//` merged, as for servers that decode before they route. + """ + forwarded = UNRESERVED_ESCAPE_PATTERN.sub(_unescaped, LONE_PERCENT_PATTERN.sub("%25", target_path)) + forwarded = re.sub("//+", "/", forwarded.replace("\\", "%5C")) + if len(forwarded) > MAX_FORWARDED_PATH_LENGTH: + return forwarded, [forwarded] # refused by the caller + decoded = unquote(forwarded) + checked = re.sub("//+", "/", decoded) + readings = [checked, decoded] # Starlette keeps the `//` of a decoded `%2F` next to a `/` + if "%2F" in forwarded or "%2f" in forwarded: + readings.append(unquote(re.sub("%2[fF]", "%252F", forwarded))) # Express, aiohttp and Go keep it in its segment + if ";" in forwarded: + readings.append(re.sub("//+", "/", unquote(re.sub(";[^/]*", "", forwarded)))) # Tomcat drops `;` parameters + if "?" in checked or "#" in checked or ";" in checked: + resent = re.split("[?#]", checked, maxsplit=1)[0] # a wrapper ExApp re-sending the decoded path cuts it there + readings += [resent, re.sub("//+", "/", re.sub(";[^/]*", "", resent))] # Tomcat behind it drops `;` parameters + readings += [reading[:-1] for reading in readings if len(reading) > 1 and reading.endswith("/")] # Express, Spring + return forwarded, list(dict.fromkeys(readings)) + + +def first_route( + routes: list[ExAppRoute], path: str, flags: int, method: str, exapp_id: str ) -> tuple[ExAppRoute | None, list[ExAppRoute]]: """The first route matching both path and method, as in AppAPI's PHP proxy, or None and the routes of the path. @@ -701,7 +738,7 @@ def find_route( path_routes = [] for route in routes: try: - if not re.match(route.url, target_path): + if not re.match(route.url, path, flags): continue except re.error as err: LOGGER.error("Invalid regex %s in route for exapp %s: %s", route.url, exapp_id, err) @@ -712,6 +749,25 @@ def find_route( return None, path_routes +def find_route( + routes: list[ExAppRoute], readings: list[str], method: str, exapp_id: str +) -> tuple[ExAppRoute | None, list[ExAppRoute]]: + """The route that decides the request: the first one matching the checked path, case-sensitively, and the method. + + A route that another reading of the path (`exapp_paths`, the checked path first) or the path in another letter case + (Express, AppAPI's PHP proxy) matches can only raise the access level. + """ + route, path_routes = first_route(routes, readings[0], 0, method, exapp_id) + if route is None: + return None, path_routes + for reading in readings: + for flags in (0, re.IGNORECASE) if reading != readings[0] else (re.IGNORECASE,): + other, _ = first_route(routes, reading, flags, method, exapp_id) + if other is not None and other.access_level > route.access_level: + route = other + return route, path_routes + + def allowed_methods(routes: list[ExAppRoute]) -> str: """The `Allow` header of a 405: the methods of these routes, with HEAD wherever GET is.""" methods = set().union(*(route.methods() or frozenset() for route in routes)) @@ -754,8 +810,8 @@ async def _exapps_msg( LOGGER.warning("IP %s is banned due to excessive failed attempts.", client_ip_str) return reply.set_txn_var("bad_request", 1) - if DOT_DOT_SEGMENT_PATTERN.search(path): - LOGGER.warning("Rejecting path with parent-directory segment: %s (client_ip=%s)", path, client_ip_str) + if DOT_SEGMENT_PATTERN.search(path): + LOGGER.warning("Rejecting path with a dot segment: %s (client_ip=%s)", path, client_ip_str) await record_failure_unless_trusted(client_ip_str, request_headers) return reply.set_txn_var("bad_request", 1) @@ -767,10 +823,10 @@ async def _exapps_msg( exapp_id = match.group(1) exapp_id_lower = exapp_id.lower() target_path = path.removeprefix(f"/exapps/{exapp_id}") - reply = reply.set_txn_var("target_path", target_path) # Special handling for AppAPI requests if exapp_id == "app_api": + reply = reply.set_txn_var("target_path", target_path) return await handle_app_api_request(target_path, request_headers, client_ip_str, reply) exapp_route_bruteforce_protection = None @@ -829,12 +885,20 @@ async def _exapps_msg( route_allowed = False if authorization_app_api: route_allowed = True # We skip routes checking for AppAPI signed requests - elif target_path in ("/heartbeat", "/init", "/enabled"): - LOGGER.error("Only requests from AppAPI allowed to the internal endpoints.") - await record_ip_failure(client_ip_str) - return reply.set_txn_var("bad_request", 1) else: - route, path_routes = find_route(exapp_record.routes, target_path, method, exapp_id) + target_path, readings = exapp_paths(target_path) + # Control characters too: Starlette's `$` also matches before a trailing newline (`/enabled%0A`). + if ( + len(target_path) > MAX_FORWARDED_PATH_LENGTH + or CONTROL_CHARACTER_PATTERN.search(readings[0]) + or any(reading.strip("/").lower() in LIFECYCLE_ENDPOINTS for reading in readings) + ): + LOGGER.error( + "Refusing a request to an internal endpoint, with a control character or too long: %.200s", target_path + ) + await record_ip_failure(client_ip_str) + return reply.set_txn_var("bad_request", 1) + route, path_routes = find_route(exapp_record.routes, readings, method, exapp_id) if route is None and path_routes: # Not counted toward the IP ban: link checkers, monitors and CORS preflights send such requests. So it is # decided before the user lookup, or random credentials would get checked by Nextcloud without limit. @@ -896,6 +960,7 @@ async def _exapps_msg( return reply.set_txn_var("not_found", 1) LOGGER.info("Rerouting request to %s:%s with path=%s", exapp_record.resolved_host, exapp_record.port, target_path) + reply = reply.set_txn_var("target_path", target_path) reply = reply.set_txn_var("target_ip", exapp_record.resolved_host) reply = reply.set_txn_var("target_port", exapp_record.port) reply = reply.set_txn_var("exapp_token", authorization_app_api) diff --git a/tests/unit/test_route_paths.py b/tests/unit/test_route_paths.py new file mode 100644 index 0000000..6883703 --- /dev/null +++ b/tests/unit/test_route_paths.py @@ -0,0 +1,223 @@ +# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: AGPL-3.0-or-later +"""Route checks decide on the path as the ExApp reads it, and HaRP forwards a path that every server reads alike.""" + +import asyncio +import ipaddress + +import pytest + +import haproxy_agent as agent + +from .test_route_verbs import IP, decide, exapp, outcome, request, strikes, txn_vars + +ADMIN_AREA = exapp( + {"url": "^/admin/.*", "verb": "GET", "access_level": 2}, + {"url": "^/api/admin/.*", "verb": "GET", "access_level": 2}, + {"url": "^/settings$", "verb": "GET", "access_level": 2}, + {"url": ".*", "verb": "GET,POST,PUT", "access_level": 1}, +) + + +def forwarded(monkeypatch, path: str, level: int = 2) -> str: + found = decide(monkeypatch, ADMIN_AREA, "GET", path, level) + assert outcome(found) == "ALLOW", found + return found["target_path"] + + +@pytest.mark.parametrize( + "path", + [ + "/api/%61dmin/x", # FastAPI, aiohttp, Flask and Tomcat decode before they route + "/api/admin%2Fx", + "/%61dmin/x", + "//admin/x", # Tomcat and nginx merge slashes + "/api//admin/x", + "/api/%2Fadmin/x", # a wrapper ExApp may pass `/api//admin/x` on to a server that merges slashes + "/Admin/x", # Express routes ignoring case + "/API/ADMIN/x", + "/settings/", # Express also ignores a trailing slash + "/Settings/", + "/admin;x/y", # Tomcat drops `;` parameters + "/admin;/x", + "/;x/admin/x", # and merges the empty segment that is left + "/settings%3Fx", # a wrapper ExApp that re-sends the decoded path cuts it at `?` or `#` + "/settings%23x", + "/settings%3Fx%3Fy", + "/settings/%3Fx", + "/%2Fsettings%3Fx", + "/settings%3Bx", # and Tomcat behind it drops `;` parameters + "/settings;x%3F/y", + "/admin%3Bv=1/x", + "/%3Bx/admin/x", + ], +) +def test_a_stricter_route_is_not_skipped_by_another_spelling_of_its_path(monkeypatch, path): + assert outcome(decide(monkeypatch, ADMIN_AREA, "GET", path, 1)) == "403" + assert outcome(decide(monkeypatch, ADMIN_AREA, "GET", path, 2)) == "ALLOW" + + +def test_a_stricter_route_is_not_skipped_by_an_escaped_slash(monkeypatch): + # Express, aiohttp and Go's ServeMux route `a%2Fb` as one segment, so their `/files/:name` serves it. + record = exapp( + {"url": "^/files/[^/]+$", "verb": "GET", "access_level": 2}, {"url": ".*", "verb": "GET", "access_level": 1} + ) + for path in ("/files/a%2Fb", "/files/a%2fb", "/files/a%2Fb/"): + assert request(monkeypatch, record, "GET", path, 1) == "403" + assert request(monkeypatch, record, "GET", path, 2) == "ALLOW" + + +def test_a_stricter_route_is_not_skipped_by_a_decoded_slash_next_to_another(monkeypatch): + # Starlette reads `/files/%2Fa.txt` as `/files//a.txt` and runs its `/files/{rest:path}` handler. + record = exapp( + {"url": "^/files/[^/]+$", "verb": "GET", "access_level": 0}, + {"url": "^/files/.*", "verb": "GET", "access_level": 2}, + {"url": ".*", "verb": "GET", "access_level": 0}, + ) + assert request(monkeypatch, record, "GET", "/files/a.txt") == "ALLOW" + assert request(monkeypatch, record, "GET", "/files/%2Fa.txt") == "403" + + +def test_the_forwarded_path_reads_as_the_checked_one(monkeypatch): + assert forwarded(monkeypatch, "/files/%6d%4A%39%2d%2e%5f%7e") == "/files/mJ9-._~" + assert forwarded(monkeypatch, "/files/%a.png") == "/files/%25a.png" + assert forwarded(monkeypatch, "/api/%61dmin/x") == "/api/admin/x" + assert forwarded(monkeypatch, "//admin/x") == "/admin/x" + assert forwarded(monkeypatch, "/files/%7Euser") == "/files/~user" + assert forwarded(monkeypatch, "/files/100%") == "/files/100%25" + + +def test_other_escapes_are_forwarded_as_sent(monkeypatch): + for path in ("/files/My%20File.txt", "/files/caf%C3%A9", "/files/caf%c3%a9", "/files/a%2Fb", "/files/%25", "/a;b"): + assert forwarded(monkeypatch, path, level=1) == path + + +def test_a_path_too_long_to_forward_is_refused(monkeypatch): + # Each lone `%` is forwarded as `%25`: the reply to HAProxy would exceed one SPOE frame and stall its connection. + assert decide(monkeypatch, ADMIN_AREA, "GET", "/x/" + "%" * 6000, 1)["bad_request"] == 1 + assert forwarded(monkeypatch, "/x/" + "%" * 4700, level=1) == "/x/" + "%25" * 4700 + + +def test_a_backslash_is_forwarded_escaped(monkeypatch): + # WHATWG URL parsers (Hono, Node's `new URL()`) read a raw `\` as `/`; `%5C` is a `\` for every server. + assert forwarded(monkeypatch, "/\\x/admin/x", level=1) == "/%5Cx/admin/x" + assert forwarded(monkeypatch, "/api\\admin/x", level=1) == "/api%5Cadmin/x" + + +def test_a_stricter_route_is_not_skipped_by_dropping_semicolon_parameters_before_decoding(monkeypatch): + # Tomcat drops `;` parameters before it decodes, so `%3B` stays: `/;/files/%3B/a` runs its `/files/*` servlet. + record = exapp( + {"url": "^/files/[^/]+$", "verb": "GET", "access_level": 0}, + {"url": "^/files/.*", "verb": "GET", "access_level": 2}, + {"url": ".*", "verb": "GET", "access_level": 0}, + ) + assert request(monkeypatch, record, "GET", "/;/files/%3B/a") == "403" + + +def test_a_route_for_a_name_with_spaces_or_accents_matches_the_encoded_request(monkeypatch): + record = exapp({"url": "^/files/Café menu\\.pdf$", "verb": "GET", "access_level": 0}) + assert request(monkeypatch, record, "GET", "/files/Caf%C3%A9%20menu.pdf") == "ALLOW" + + +def test_visionatrix_double_slash_api_calls_reach_its_api_route(monkeypatch): + # Its UI joins the base URL `/exapps/visionatrix/` and `/vapi/...`. + record = exapp( + {"url": "^/vapi/.*", "verb": "GET", "access_level": 1}, {"url": ".*", "verb": "GET", "access_level": 2} + ) + found = decide(monkeypatch, record, "GET", "//vapi/flows/installed", 1) + assert outcome(found) == "ALLOW" + assert found["target_path"] == "/vapi/flows/installed" + + +@pytest.mark.parametrize( + "path", + [ + "/p/../admin/x", + "/p/..;/admin/x", + "/p/%2e%2e;/admin/x", + "/p/..%3Bx/admin/x", + "/./admin/x", + "/p/.;x/", + "/%2F.%2Fb", + "/public/..\\admin/x", # WHATWG URL parsers read `\` as `/` and then resolve the dots + "/p\\..\\admin/x", + "/p/%2E%2E/admin/x", + "/p%2f..%2fadmin/x", + "/admin/x/..", + "/p/..%3F/admin/x", # a wrapper ExApp that decodes the path cuts it at `?` or `#` + "/p/.%23x", + ], +) +def test_dot_segments_are_refused_in_every_form(monkeypatch, path): + found = decide(monkeypatch, ADMIN_AREA, "GET", path, 2) + assert found["bad_request"] == 1 + assert strikes() == 1 + + +@pytest.mark.parametrize( + ("method", "path"), + [ + ("POST", "/Init/"), # Express: any case, trailing slash + ("PUT", "/%65nabled"), # Python servers decode + ("PUT", "//enabled"), + ("GET", "/HEARTBEAT"), + ("GET", "/heartbeat/"), + ("PUT", "/enabled%0A"), # Starlette's `$` also matches before a trailing newline + ("PUT", "/enabled;x"), # Tomcat + ("PUT", "/;x/enabled"), + ], +) +def test_the_internal_endpoints_are_refused_in_every_spelling(monkeypatch, method, path): + found = decide(monkeypatch, ADMIN_AREA, method, path, 2) + assert found["bad_request"] == 1 + assert strikes() == 1 + + +def test_a_route_matching_only_in_another_letter_case_does_not_lower_the_access_level(monkeypatch): + record = exapp( + {"url": "^/static/.*", "verb": "GET", "access_level": 0}, {"url": ".*", "verb": "GET", "access_level": 2} + ) + assert request(monkeypatch, record, "GET", "/static/app.js") == "ALLOW" + assert request(monkeypatch, record, "GET", "/STATIC/app.js") == "403" + + +def test_a_path_without_a_route_in_its_letter_case_is_still_refused(monkeypatch): + record = exapp({"url": "^/x$", "verb": "GET", "access_level": 0}) + assert request(monkeypatch, record, "GET", "/X") == "404" + + +def test_a_directory_path_is_not_refused_for_lack_of_a_route_without_its_slash(monkeypatch): + record = exapp({"url": "^/api/.*", "verb": "GET", "access_level": 1}) + assert request(monkeypatch, record, "GET", "/api/", 1) == "ALLOW" + + +def test_the_route_of_the_first_reading_decides_between_routes_of_the_same_level(monkeypatch): + record = exapp( + {"url": "^/a/x/$", "verb": "GET", "access_level": 2, "bruteforce_protection": [401]}, + {"url": "^/A/x$", "verb": "GET", "access_level": 2}, + {"url": ".*", "verb": "GET", "access_level": 1}, + ) + found = decide(monkeypatch, record, "GET", "/A/x/", 2) + assert outcome(found) == "ALLOW" + assert found["statuses_to_trigger_bp"] == "[401]" + + +def test_a_request_to_appapi_keeps_its_path(): + # The Docker Engine API through the FRP tunnel, as AppAPI sends it. + headers = "harp-shared-key: unit-test-shared-key\ndocker-engine-port: 24001" + reply = asyncio.run(agent.exapps_msg("/exapps/app_api/v1.44/_ping", headers, ipaddress.ip_address(IP), "", "GET")) + assert txn_vars(reply)["target_path"] == "/v1.44/_ping" + + +def test_a_request_signed_by_appapi_is_forwarded_as_sent(monkeypatch): + signed = "\n".join( + [ + "harp-shared-key: unit-test-shared-key", + "authorization-app-api: dXNlcjpzZWNyZXQ=", + "ex-app-version: 1", + "ex-app-id: app", + "ex-app-host: 127.0.0.1", + "ex-app-port: 23000", + ] + ) + assert decide(monkeypatch, ADMIN_AREA, "GET", "//x;y/%61", headers=signed)["target_path"] == "//x;y/%61"