diff --git a/.github/workflows/tests-deploy-docker.yml b/.github/workflows/tests-deploy-docker.yml index 2147edf..5de3d80 100644 --- a/.github/workflows/tests-deploy-docker.yml +++ b/.github/workflows/tests-deploy-docker.yml @@ -549,6 +549,10 @@ jobs: test_req admin "-u usr:password" 403 test_req admin "-u admin:admin" 200 + # A method that no route of the path lists: HaRP answers 405 itself, with HEAD added to the GET route's verb + curl -sS -o /dev/null -D - -X POST http://127.0.0.1/exapps/app-skeleton-python/public | tr -d '\r' \ + | grep -ix 'allow: GET, HEAD' >/dev/null || error + # A chunked request with credentials, sent to HaRP directly because the nginx in front buffers bodies code=$(printf 'body' | curl -sS -m 10 -o /dev/null -w "%{http_code}" -u usr:password -X GET \ -H "Transfer-Encoding: chunked" --data-binary @- http://127.0.0.1:8780/exapps/app-skeleton-python/user) diff --git a/README.md b/README.md index b86c3b6..ca78f66 100644 --- a/README.md +++ b/README.md @@ -717,6 +717,8 @@ image has to be present on every node that can run the pod. > **Note:** On Alpine Linux, `apk add frp` also installs the FRP client, but the Alpine package can be several releases behind the version above. +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. Unlike the PHP proxy, HaRP allows `HEAD` wherever `GET` is listed. It 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. + That's it! Your ExApp is now adapted to Nextcloud 32. ## Nextcloud 32: Migrating Existing ExApps from DSP to HaRP diff --git a/haproxy.cfg.template b/haproxy.cfg.template index 7718674..7088245 100644 --- a/haproxy.cfg.template +++ b/haproxy.cfg.template @@ -47,6 +47,7 @@ frontend ex_apps http-request silent-drop if { var(txn.exapps.bad_request) -m int eq 1 } http-request return status 401 content-type text/plain string "401 Unauthorized" if { var(txn.exapps.unauthorized) -m int eq 1 } http-request return status 403 content-type text/plain string "403 Forbidden" if { var(txn.exapps.forbidden) -m int eq 1 } + http-request return status 405 content-type text/plain string "405 Method Not Allowed" hdr Allow "%[var(txn.exapps.allow)]" if { var(txn.exapps.method_not_allowed) -m int eq 1 } http-request return status 404 content-type text/plain string "404 Not Found" if { var(txn.exapps.not_found) -m int eq 1 } http-request return status 503 content-type text/plain string "503 Service Unavailable (HaRP)" if !{ var(txn.exapps.backend) -m found } use_backend %[var(txn.exapps.backend)] @@ -62,6 +63,7 @@ _HTTPS_FRONTEND_ filter spoe engine exapps-spoe config /etc/haproxy/spoe-age _HTTPS_FRONTEND_ http-request silent-drop if { var(txn.exapps.bad_request) -m int eq 1 } _HTTPS_FRONTEND_ http-request return status 401 content-type text/plain string "401 Unauthorized" if { var(txn.exapps.unauthorized) -m int eq 1 } _HTTPS_FRONTEND_ http-request return status 403 content-type text/plain string "403 Forbidden" if { var(txn.exapps.forbidden) -m int eq 1 } +_HTTPS_FRONTEND_ http-request return status 405 content-type text/plain string "405 Method Not Allowed" hdr Allow "%[var(txn.exapps.allow)]" if { var(txn.exapps.method_not_allowed) -m int eq 1 } _HTTPS_FRONTEND_ http-request return status 404 content-type text/plain string "404 Not Found" if { var(txn.exapps.not_found) -m int eq 1 } _HTTPS_FRONTEND_ http-request return status 503 content-type text/plain string "503 Service Unavailable (HaRP)" if !{ var(txn.exapps.backend) -m found } _HTTPS_FRONTEND_ use_backend %[var(txn.exapps.backend)] diff --git a/haproxy_agent.py b/haproxy_agent.py index ced4eda..2825e56 100644 --- a/haproxy_agent.py +++ b/haproxy_agent.py @@ -342,6 +342,7 @@ class AccessLevel(IntEnum): class ExAppRoute(BaseModel): url: str = Field(..., description="REGEX for URL, e.g. r'^/private/.*'") + verb: str | None = Field(None, description="HTTP methods, e.g. 'GET,POST'. Older AppAPI versions do not send it.") access_level: AccessLevel = Field(..., description="ADMIN(2), USER(1), or PUBLIC(0)") bruteforce_protection: list[int] = Field( [], description="List with HTTP statuses to trigger the bruteforce protection." @@ -355,6 +356,19 @@ def encode_bruteforce_protection_values(self) -> Self: self.str_bruteforce_protection = json.dumps(self.bruteforce_protection) if self.bruteforce_protection else "" return self + def methods(self) -> frozenset[str] | None: + """The methods the route lists, or None (every method) when AppAPI did not send a verb.""" + if self.verb is None: + return None + return frozenset(token.upper() for token in re.split(r"[\s,]+", self.verb) if token) + + def accepts(self, method: str) -> bool: + """HEAD is accepted wherever GET is. Methods are case-sensitive: a backend may treat `get` as not GET.""" + methods = self.methods() + if methods is None: + return True + return method in methods or (method == "HEAD" and "GET" in methods) + class ExApp(BaseModel): exapp_token: str = Field(..., repr=False) # the ExApp's secret; records are written to the log @@ -677,20 +691,57 @@ async def get_session(pass_cookie: str) -> NcUser | None: ############################################################################### +def find_route( + routes: list[ExAppRoute], target_path: str, 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. + + A request without a method (a custom spoe-agent.conf without `method`) gets the first route of the path. + """ + path_routes = [] + for route in routes: + try: + if not re.match(route.url, target_path): + continue + except re.error as err: + LOGGER.error("Invalid regex %s in route for exapp %s: %s", route.url, exapp_id, err) + continue + if not method or route.accepts(method): + return route, path_routes + path_routes.append(route) + return None, 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)) + if "GET" in methods: + methods.add("HEAD") + return ", ".join(sorted(methods)) + + @SPOA_AGENT.handler("exapps_msg") async def exapps_msg( - path: str, headers: str, client_ip: ipaddress.IPv4Address | ipaddress.IPv6Address, pass_cookie: str + path: str, + headers: str, + client_ip: ipaddress.IPv4Address | ipaddress.IPv6Address, + pass_cookie: str, + method: str = "", ) -> AckPayload: """An exception would kill the SPOA connection; a zero-action ACK hangs the stream until `timeout processing`.""" try: - return await _exapps_msg(path, headers, client_ip, pass_cookie) + return await _exapps_msg(path, headers, client_ip, pass_cookie, method) except Exception: LOGGER.exception("Unhandled error while processing request to path=%s", path) return AckPayload().set_txn_var("agent_error", 1) async def _exapps_msg( - path: str, headers: str, client_ip: ipaddress.IPv4Address | ipaddress.IPv6Address, pass_cookie: str + path: str, + headers: str, + client_ip: ipaddress.IPv4Address | ipaddress.IPv6Address, + pass_cookie: str, + method: str = "", ) -> AckPayload: reply = AckPayload() request_headers = parse_headers(headers) @@ -783,6 +834,17 @@ async def _exapps_msg( 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) + 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. + # not_found is also set, so that a custom /haproxy.cfg without the 405 rule answers 404. + allow = allowed_methods(path_routes) + LOGGER.info("No route of %s allows %s %s; allowed: %s", exapp_id, method, target_path, allow) + reply = reply.set_txn_var("allow", allow) + reply = reply.set_txn_var("method_not_allowed", 1) + return reply.set_txn_var("not_found", 1) + nc_user = None if pass_cookie or "authorization" in request_headers: # We also pass requests with "authorization" to the Nextcloud to support App Passwords and Basic Auth. @@ -799,24 +861,14 @@ async def _exapps_msg( LOGGER.exception("Failed to fetch user info from Nextcloud", exc_info=e) return reply.set_txn_var("unauthorized", 1) - for route in exapp_record.routes: - try: - if re.match(route.url, target_path): - if route.access_level == AccessLevel.PUBLIC: - exapp_route_bruteforce_protection = route.str_bruteforce_protection - route_allowed = True - break - - if nc_user and route.access_level <= nc_user.access_level: - exapp_route_bruteforce_protection = route.str_bruteforce_protection - route_allowed = True - break - - LOGGER.error("Access denied for '%s' to %s", nc_user.user_id if nc_user else "", target_path) - await record_ip_failure(client_ip_str) - return reply.set_txn_var("forbidden", 1) - except re.error as err: - LOGGER.error("Invalid regex %s in route for exapp %s: %s", route.url, exapp_id, err) + if route is not None: + if route.access_level == AccessLevel.PUBLIC or (nc_user and route.access_level <= nc_user.access_level): + exapp_route_bruteforce_protection = route.str_bruteforce_protection + route_allowed = True + else: + LOGGER.error("Access denied for '%s' to %s", nc_user.user_id if nc_user else "", target_path) + await record_ip_failure(client_ip_str) + return reply.set_txn_var("forbidden", 1) if not route_allowed: LOGGER.error("No defined route for handling %s", target_path) diff --git a/spoe-agent.conf b/spoe-agent.conf index 830c314..5f25274 100644 --- a/spoe-agent.conf +++ b/spoe-agent.conf @@ -13,7 +13,7 @@ spoe-agent exapps-agent spoe-message exapps_msg event on-frontend-http-request - args path=path headers=req.hdrs client_ip=src pass_cookie=req.cook('oc_sessionPassphrase') + args path=path headers=req.hdrs client_ip=src pass_cookie=req.cook('oc_sessionPassphrase') method=method [exapps-bruteforce-protection-spoe] spoe-agent exapps-agent diff --git a/tests/unit/test_route_verbs.py b/tests/unit/test_route_verbs.py new file mode 100644 index 0000000..18166b7 --- /dev/null +++ b/tests/unit/test_route_verbs.py @@ -0,0 +1,189 @@ +# SPDX-FileCopyrightText: 2026 Nextcloud GmbH and Nextcloud contributors +# SPDX-License-Identifier: AGPL-3.0-or-later +"""Route verbs: first match by path and method, HEAD implied by GET, 405 with Allow for a method no route lists.""" + +import asyncio +import ipaddress + +from haproxyspoa.payloads.ack import AckPayload + +import haproxy_agent as agent + +IP = "192.0.2.7" + +SAME_URL = [ # a public GET route listed before an admin POST route of the same URL + {"url": "^/x$", "verb": "GET", "access_level": 0}, + {"url": "^/x$", "verb": "POST", "access_level": 2}, +] + + +def txn_vars(reply: AckPayload) -> dict: + return {action.name: action.value for action in reply.actions} + + +def exapp(*routes) -> agent.ExApp: + return agent.ExApp.model_validate( + {"exapp_token": "t", "exapp_version": "1", "host": "127.0.0.1", "port": 23000, "routes": list(routes)} + ) + + +def decide(monkeypatch, record, method, path, level=None, lookups=None, headers="") -> dict: + """The txn vars the agent sets for this request; `level` sends credentials of a user with that access level. + + `method=None` calls the handler without it, as for a custom spoe-agent.conf without `method`. + """ + + async def fetch(_app_id): + return record + + async def user(_app_id, _headers): + if lookups is not None: + lookups.append(path) + return agent.NcUser(user_id="u", access_level=level) + + monkeypatch.setattr(agent, "_get_or_fetch_exapp", fetch) + monkeypatch.setattr(agent, "nc_get_user", user) + if level is not None: + headers = "authorization: Basic eDp5" + args = {"path": f"/exapps/app{path}", "headers": headers, "client_ip": ipaddress.ip_address(IP), "pass_cookie": ""} + if method is not None: + args["method"] = method + return txn_vars(asyncio.run(agent.exapps_msg(**args))) # the handler, called the way haproxyspoa calls it + + +def outcome(found: dict) -> str: + if "backend" in found: + return "ALLOW" + for name, code in (("method_not_allowed", "405"), ("forbidden", "403"), ("not_found", "404")): + if found.get(name) == 1: + return code + return repr(found) + + +def request(monkeypatch, record, method, path, level=None) -> str: + return outcome(decide(monkeypatch, record, method, path, level)) + + +def strikes(): + return len(agent.BLACKLIST_CACHE.get(IP, [])) + + +def test_first_match_by_path_and_method(monkeypatch): + assert request(monkeypatch, exapp(*SAME_URL), "GET", "/x") == "ALLOW" + assert request(monkeypatch, exapp(*SAME_URL), "POST", "/x") == "403" + assert strikes() == 1 + assert request(monkeypatch, exapp(*SAME_URL), "POST", "/x", level=2) == "ALLOW" + + +def test_bruteforce_protection_comes_from_the_deciding_route(monkeypatch): + record = exapp( + {"url": "^/login$", "verb": "GET", "access_level": 0}, + {"url": "^/login$", "verb": "POST", "access_level": 0, "bruteforce_protection": [401]}, + ) + found = decide(monkeypatch, record, "GET", "/login") + assert found["backend"] == "ex_apps_backend" + assert "statuses_to_trigger_bp" not in found + found = decide(monkeypatch, record, "POST", "/login") + assert found["backend"] == "ex_apps_backend_w_bruteforce" + assert found["statuses_to_trigger_bp"] == "[401]" + + +def test_unlisted_method_is_405_with_allow_and_no_ban_strike(monkeypatch): + other_url = {"url": "^/other$", "verb": "PUT", "access_level": 0} # not part of the Allow header of /x + found = decide(monkeypatch, exapp(*SAME_URL, other_url), "DELETE", "/x") + assert found["method_not_allowed"] == 1 + assert found["not_found"] == 1 # a custom /haproxy.cfg without the 405 rule answers 404 + assert found["allow"] == "GET, HEAD, POST" + for _ in range(20): # a link checker or monitor cannot get its address banned this way + assert request(monkeypatch, exapp(*SAME_URL), "PATCH", "/x") == "405" + assert strikes() == 0 + assert request(monkeypatch, exapp(*SAME_URL), "GET", "/x") == "ALLOW" + + +def test_unlisted_method_is_refused_before_the_user_lookup(monkeypatch): + # Not counted toward the ban, so it must not let a client make Nextcloud check credentials without limit. + lookups = [] + found = decide(monkeypatch, exapp(*SAME_URL), "DELETE", "/x", level=1, lookups=lookups) + assert outcome(found) == "405" + assert lookups == [] + assert outcome(decide(monkeypatch, exapp(*SAME_URL), "POST", "/x", level=2, lookups=lookups)) == "ALLOW" + assert lookups == ["/x"] + + +def test_head_is_implied_by_get(monkeypatch): + assert request(monkeypatch, exapp(*SAME_URL), "HEAD", "/x") == "ALLOW" + only_post = exapp({"url": "^/y$", "verb": "POST", "access_level": 0}) + assert request(monkeypatch, only_post, "HEAD", "/y") == "405" + + +def test_options_is_not_special(monkeypatch): + assert request(monkeypatch, exapp(*SAME_URL), "OPTIONS", "/x") == "405" + listed = exapp({"url": "^/y$", "verb": "POST,OPTIONS", "access_level": 0}) + assert request(monkeypatch, listed, "OPTIONS", "/y") == "ALLOW" + + +def test_route_with_several_verbs(monkeypatch): + record = exapp({"url": "^/z$", "verb": "GET,POST", "access_level": 0}) + assert request(monkeypatch, record, "POST", "/z") == "ALLOW" + assert request(monkeypatch, record, "PUT", "/z") == "405" + + +def test_verb_as_appapi_before_35_stores_it(monkeypatch): + # AppAPI before 35 stores the text of as it is in info.xml, line breaks and case included. + record = exapp({"url": "^/z$", "verb": "\n get, POST\n ", "access_level": 0}) + assert request(monkeypatch, record, "GET", "/z") == "ALLOW" + assert request(monkeypatch, record, "POST", "/z") == "ALLOW" + assert decide(monkeypatch, record, "PUT", "/z")["allow"] == "GET, HEAD, POST" + + +def test_method_is_case_sensitive(monkeypatch): + assert request(monkeypatch, exapp(*SAME_URL), "get", "/x") == "405" + + +def test_methods_are_compared_as_tokens(monkeypatch): + record = exapp({"url": "^/z$", "verb": "GET,POST", "access_level": 0}) + for method in ("ET", "OS", "GETPOST"): # a client may send any method token + assert request(monkeypatch, record, method, "/z") == "405" + + +def test_route_without_verb_matches_every_method(monkeypatch): + older_appapi = exapp({"url": "^/x$", "access_level": 0}) + assert request(monkeypatch, older_appapi, "DELETE", "/x") == "ALLOW" + + +def test_request_without_method_keeps_the_first_path_match(monkeypatch): + # A custom spoe-agent.conf without `method`: the first route of the path decides, as before. + assert request(monkeypatch, exapp(*SAME_URL), None, "/x") == "ALLOW" + assert request(monkeypatch, exapp(*reversed(SAME_URL)), None, "/x") == "403" + + +def test_request_signed_by_appapi_skips_the_route_check(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 outcome(decide(monkeypatch, exapp(*SAME_URL), "DELETE", "/x", headers=signed)) == "ALLOW" + assert outcome(decide(monkeypatch, exapp(*SAME_URL), "PUT", "/enabled", headers=signed)) == "ALLOW" + + +def test_public_route_still_tells_the_exapp_who_the_user_is(monkeypatch): + found = decide(monkeypatch, exapp(*SAME_URL), "GET", "/x", level=1) + assert found["exapp_token"] == agent.b64encode(b"u:t") + + +def test_unknown_path_is_still_404_and_counted(monkeypatch): + assert request(monkeypatch, exapp(*SAME_URL), "GET", "/nope") == "404" + assert strikes() == 1 + + +def test_route_with_invalid_regex_is_not_a_path_match(monkeypatch): + # AppAPI does not check route regexes, and Python's re rejects some PCRE patterns. + record = exapp({"url": "^/x(", "verb": "PUT", "access_level": 0}) + assert request(monkeypatch, record, "GET", "/x") == "404" + assert strikes() == 1