From e03c69a3aaf163f075b3ad069efb2dcc06b65fa5 Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Mon, 3 Aug 2026 13:17:40 +0000 Subject: [PATCH 1/2] fix(files): never mutate the shared PROPFIND property lists get_propfind_properties() aliased the module-level PROPFIND_PROPERTIES and extended it with +=, and both trashbin_list() implementations did the same, so the shared list grew by 7 entries per call on servers advertising files.locking and by 3 per trashbin_list(). A long-running client ended up sending multi-megabyte PROPFIND bodies, and the server cost is O(properties x resources), so it could starve the Nextcloud instance it was talking to. Build a fresh list in each of the three places and turn both constants into tuples, so any future in-place mutation fails immediately instead of silently corrupting shared state. Closes #453 Signed-off-by: Oleksandr Piskun --- CHANGELOG.md | 6 ++ nc_py_api/files/_files.py | 18 +++-- nc_py_api/files/files.py | 11 ++- nc_py_api/files/files_async.py | 11 ++- tests_unit/test_propfind_properties.py | 104 +++++++++++++++++++++++++ 5 files changed, 136 insertions(+), 14 deletions(-) create mode 100644 tests_unit/test_propfind_properties.py diff --git a/CHANGELOG.md b/CHANGELOG.md index 0ff94929..6b101ff4 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -2,6 +2,12 @@ All notable changes to this project will be documented in this file. +## [0.30.3 - 2026-08-03] + +### Fixed + +- PROPFIND property lists are no longer mutated in place. `get_propfind_properties()` and both `trashbin_list()` implementations extended the shared `PROPFIND_PROPERTIES` constant with `+=`, so it grew on every call (7 entries per call against servers advertising `files.locking`, 3 per `trashbin_list()`). Long-running clients ended up sending multi-megabyte PROPFIND bodies that could exhaust the server's workers. Both property constants are now immutable tuples, so this class of bug cannot come back. #453 Thanks to @ciberkids + ## [0.30.2 - 2026-06-02] ### Changed diff --git a/nc_py_api/files/_files.py b/nc_py_api/files/_files.py index 7d584e46..e6496460 100644 --- a/nc_py_api/files/_files.py +++ b/nc_py_api/files/_files.py @@ -2,6 +2,8 @@ import contextlib import enum +import typing +from collections.abc import Sequence from datetime import datetime, timezone from io import BytesIO from json import dumps, loads @@ -16,7 +18,7 @@ from .._misc import check_capabilities, clear_from_params_empty from . import FsNode, SystemTag -PROPFIND_PROPERTIES = [ +PROPFIND_PROPERTIES: typing.Final[tuple[str, ...]] = ( "d:resourcetype", "d:getlastmodified", "d:creationdate", @@ -34,9 +36,9 @@ "oc:share-types", "oc:favorite", "nc:is-encrypted", -] +) -PROPFIND_LOCKING_PROPERTIES = [ +PROPFIND_LOCKING_PROPERTIES: typing.Final[tuple[str, ...]] = ( "nc:lock", "nc:lock-owner-displayname", "nc:lock-owner", @@ -44,7 +46,7 @@ "nc:lock-owner-editor", # App id of an app owned lock "nc:lock-time", # Timestamp of the log creation time "nc:lock-timeout", # TTL of the lock in seconds staring from the creation time -] +) SEARCH_PROPERTIES_MAP = { "name": "d:displayname", # like, eq @@ -66,8 +68,8 @@ class PropFindType(enum.IntEnum): VERSIONS_FILE_ID = 3 -def get_propfind_properties(capabilities: dict) -> list: - r = PROPFIND_PROPERTIES +def get_propfind_properties(capabilities: dict) -> list[str]: + r = list(PROPFIND_PROPERTIES) if not check_capabilities("files.locking", capabilities): r += PROPFIND_LOCKING_PROPERTIES return r @@ -222,7 +224,7 @@ def build_update_tag_req( def build_listdir_req( - user: str, path: str, properties: list[str], prop_type: PropFindType + user: str, path: str, properties: Sequence[str], prop_type: PropFindType ) -> tuple[ElementTree.Element, str]: root = ElementTree.Element( "d:propfind", @@ -245,7 +247,7 @@ def build_listdir_response( webdav_response: Response, user: str, path: str, - properties: list[str], + properties: Sequence[str], exclude_self: bool, prop_type: PropFindType, ) -> list[FsNode]: diff --git a/nc_py_api/files/files.py b/nc_py_api/files/files.py index 32b14d65..eaf027d9 100644 --- a/nc_py_api/files/files.py +++ b/nc_py_api/files/files.py @@ -2,6 +2,7 @@ import builtins import os +from collections.abc import Sequence from pathlib import Path from urllib.parse import quote @@ -278,8 +279,12 @@ def setfav(self, path: str | FsNode, value: int | bool) -> None: def trashbin_list(self) -> list[FsNode]: """Returns a list of all entries in the TrashBin.""" - properties = PROPFIND_PROPERTIES - properties += ["nc:trashbin-filename", "nc:trashbin-original-location", "nc:trashbin-deletion-time"] + properties = [ + *PROPFIND_PROPERTIES, + "nc:trashbin-filename", + "nc:trashbin-original-location", + "nc:trashbin-deletion-time", + ] return self._listdir( self._session.user, "", properties=properties, depth=1, exclude_self=False, prop_type=PropFindType.TRASHBIN ) @@ -457,7 +462,7 @@ def _listdir( self, user: str, path: str, - properties: list[str], + properties: Sequence[str], depth: int, exclude_self: bool, prop_type: PropFindType = PropFindType.DEFAULT, diff --git a/nc_py_api/files/files_async.py b/nc_py_api/files/files_async.py index 1d1e2648..05e09fae 100644 --- a/nc_py_api/files/files_async.py +++ b/nc_py_api/files/files_async.py @@ -2,6 +2,7 @@ import builtins import os +from collections.abc import Sequence from pathlib import Path from urllib.parse import quote @@ -282,8 +283,12 @@ async def setfav(self, path: str | FsNode, value: int | bool) -> None: async def trashbin_list(self) -> list[FsNode]: """Returns a list of all entries in the TrashBin.""" - properties = PROPFIND_PROPERTIES - properties += ["nc:trashbin-filename", "nc:trashbin-original-location", "nc:trashbin-deletion-time"] + properties = [ + *PROPFIND_PROPERTIES, + "nc:trashbin-filename", + "nc:trashbin-original-location", + "nc:trashbin-deletion-time", + ] return await self._listdir( await self._session.user, "", @@ -466,7 +471,7 @@ async def _listdir( self, user: str, path: str, - properties: list[str], + properties: Sequence[str], depth: int, exclude_self: bool, prop_type: PropFindType = PropFindType.DEFAULT, diff --git a/tests_unit/test_propfind_properties.py b/tests_unit/test_propfind_properties.py new file mode 100644 index 00000000..35959b51 --- /dev/null +++ b/tests_unit/test_propfind_properties.py @@ -0,0 +1,104 @@ +"""Tests that the shared PROPFIND property lists are never mutated in place.""" + +import types + +import pytest + +from nc_py_api.files._files import ( + PROPFIND_LOCKING_PROPERTIES, + PROPFIND_PROPERTIES, + PropFindType, + get_propfind_properties, +) +from nc_py_api.files.files import FilesAPI +from nc_py_api.files.files_async import AsyncFilesAPI + +CAPS_WITH_LOCKING = {"files": {"locking": "1.0"}} +CAPS_WITHOUT_LOCKING = {"files": {}} +TRASHBIN_PROPERTIES = ["nc:trashbin-filename", "nc:trashbin-original-location", "nc:trashbin-deletion-time"] + + +def test_propfind_constants_are_immutable(): + assert isinstance(PROPFIND_PROPERTIES, tuple) + assert isinstance(PROPFIND_LOCKING_PROPERTIES, tuple) + with pytest.raises(AttributeError): + PROPFIND_PROPERTIES.append("nc:not-allowed") + + +def test_get_propfind_properties_does_not_mutate_constants(): + before, before_locking = PROPFIND_PROPERTIES, PROPFIND_LOCKING_PROPERTIES + for _ in range(10): + get_propfind_properties(CAPS_WITH_LOCKING) + get_propfind_properties(CAPS_WITHOUT_LOCKING) + assert before == PROPFIND_PROPERTIES + assert before_locking == PROPFIND_LOCKING_PROPERTIES + + +def test_get_propfind_properties_returns_fresh_list(): + first = get_propfind_properties(CAPS_WITH_LOCKING) + second = get_propfind_properties(CAPS_WITH_LOCKING) + assert first == second + assert first is not second + first.append("nc:added-by-caller") + assert "nc:added-by-caller" not in second + assert "nc:added-by-caller" not in PROPFIND_PROPERTIES + + +def test_get_propfind_properties_locking_capability(): + with_locking = get_propfind_properties(CAPS_WITH_LOCKING) + without_locking = get_propfind_properties(CAPS_WITHOUT_LOCKING) + assert len(with_locking) == len(PROPFIND_PROPERTIES) + len(PROPFIND_LOCKING_PROPERTIES) + assert len(without_locking) == len(PROPFIND_PROPERTIES) + assert set(PROPFIND_LOCKING_PROPERTIES).issubset(with_locking) + assert not set(PROPFIND_LOCKING_PROPERTIES).intersection(without_locking) + + +def test_trashbin_list_does_not_mutate_constants(monkeypatch): + requested = [] + + def _fake_listdir(_self, _user, _path, **kwargs): + requested.append(list(kwargs["properties"])) + return [] + + monkeypatch.setattr(FilesAPI, "_listdir", _fake_listdir) + files_api = FilesAPI(types.SimpleNamespace(user="admin")) + before = PROPFIND_PROPERTIES + for _ in range(3): + files_api.trashbin_list() + assert before == PROPFIND_PROPERTIES + for properties in requested: + assert properties == [*PROPFIND_PROPERTIES, *TRASHBIN_PROPERTIES] + + +async def test_trashbin_list_async_does_not_mutate_constants(monkeypatch): + requested = [] + + async def _fake_listdir(_self, _user, _path, **kwargs): + requested.append(list(kwargs["properties"])) + return [] + + class _StubSession: + @property + async def user(self) -> str: + return "admin" + + monkeypatch.setattr(AsyncFilesAPI, "_listdir", _fake_listdir) + files_api = AsyncFilesAPI(_StubSession()) + before = PROPFIND_PROPERTIES + for _ in range(3): + await files_api.trashbin_list() + assert before == PROPFIND_PROPERTIES + for properties in requested: + assert properties == [*PROPFIND_PROPERTIES, *TRASHBIN_PROPERTIES] + + +def test_trashbin_list_requests_trashbin_prop_type(monkeypatch): + calls = [] + + def _fake_listdir(_self, _user, _path, **kwargs): + calls.append(kwargs["prop_type"]) + return [] + + monkeypatch.setattr(FilesAPI, "_listdir", _fake_listdir) + FilesAPI(types.SimpleNamespace(user="admin")).trashbin_list() + assert calls == [PropFindType.TRASHBIN] From 6104c72340aa8026b42d2c6a1c24a3a27cb4a24d Mon Sep 17 00:00:00 2001 From: Oleksandr Piskun Date: Mon, 3 Aug 2026 14:08:08 +0000 Subject: [PATCH 2/2] test(files): compare PROPFIND constants against real snapshots The mutation checks bound the constants by name instead of copying them, so the comparison could never fail if the constants were ever lists again: on the unfixed code that test passed while the list was being corrupted. Snapshot both constants at import time and assert against those copies, and pin that the helper still returns a list. Signed-off-by: Oleksandr Piskun --- tests_unit/test_propfind_properties.py | 27 +++++++++++++------------- 1 file changed, 14 insertions(+), 13 deletions(-) diff --git a/tests_unit/test_propfind_properties.py b/tests_unit/test_propfind_properties.py index 35959b51..550ef734 100644 --- a/tests_unit/test_propfind_properties.py +++ b/tests_unit/test_propfind_properties.py @@ -16,6 +16,10 @@ CAPS_WITH_LOCKING = {"files": {"locking": "1.0"}} CAPS_WITHOUT_LOCKING = {"files": {}} TRASHBIN_PROPERTIES = ["nc:trashbin-filename", "nc:trashbin-original-location", "nc:trashbin-deletion-time"] +# real copies taken at import: comparing against the live constants would be vacuous +# if they are ever turned back into lists, since the name would alias the same object +EXPECTED_PROPERTIES = tuple(PROPFIND_PROPERTIES) +EXPECTED_LOCKING_PROPERTIES = tuple(PROPFIND_LOCKING_PROPERTIES) def test_propfind_constants_are_immutable(): @@ -26,31 +30,30 @@ def test_propfind_constants_are_immutable(): def test_get_propfind_properties_does_not_mutate_constants(): - before, before_locking = PROPFIND_PROPERTIES, PROPFIND_LOCKING_PROPERTIES for _ in range(10): get_propfind_properties(CAPS_WITH_LOCKING) get_propfind_properties(CAPS_WITHOUT_LOCKING) - assert before == PROPFIND_PROPERTIES - assert before_locking == PROPFIND_LOCKING_PROPERTIES + assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES + assert tuple(PROPFIND_LOCKING_PROPERTIES) == EXPECTED_LOCKING_PROPERTIES def test_get_propfind_properties_returns_fresh_list(): first = get_propfind_properties(CAPS_WITH_LOCKING) second = get_propfind_properties(CAPS_WITH_LOCKING) + assert isinstance(first, list) assert first == second assert first is not second first.append("nc:added-by-caller") assert "nc:added-by-caller" not in second assert "nc:added-by-caller" not in PROPFIND_PROPERTIES + assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES def test_get_propfind_properties_locking_capability(): with_locking = get_propfind_properties(CAPS_WITH_LOCKING) without_locking = get_propfind_properties(CAPS_WITHOUT_LOCKING) - assert len(with_locking) == len(PROPFIND_PROPERTIES) + len(PROPFIND_LOCKING_PROPERTIES) - assert len(without_locking) == len(PROPFIND_PROPERTIES) - assert set(PROPFIND_LOCKING_PROPERTIES).issubset(with_locking) - assert not set(PROPFIND_LOCKING_PROPERTIES).intersection(without_locking) + assert with_locking == [*EXPECTED_PROPERTIES, *EXPECTED_LOCKING_PROPERTIES] + assert without_locking == list(EXPECTED_PROPERTIES) def test_trashbin_list_does_not_mutate_constants(monkeypatch): @@ -62,12 +65,11 @@ def _fake_listdir(_self, _user, _path, **kwargs): monkeypatch.setattr(FilesAPI, "_listdir", _fake_listdir) files_api = FilesAPI(types.SimpleNamespace(user="admin")) - before = PROPFIND_PROPERTIES for _ in range(3): files_api.trashbin_list() - assert before == PROPFIND_PROPERTIES + assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES for properties in requested: - assert properties == [*PROPFIND_PROPERTIES, *TRASHBIN_PROPERTIES] + assert properties == [*EXPECTED_PROPERTIES, *TRASHBIN_PROPERTIES] async def test_trashbin_list_async_does_not_mutate_constants(monkeypatch): @@ -84,12 +86,11 @@ async def user(self) -> str: monkeypatch.setattr(AsyncFilesAPI, "_listdir", _fake_listdir) files_api = AsyncFilesAPI(_StubSession()) - before = PROPFIND_PROPERTIES for _ in range(3): await files_api.trashbin_list() - assert before == PROPFIND_PROPERTIES + assert tuple(PROPFIND_PROPERTIES) == EXPECTED_PROPERTIES for properties in requested: - assert properties == [*PROPFIND_PROPERTIES, *TRASHBIN_PROPERTIES] + assert properties == [*EXPECTED_PROPERTIES, *TRASHBIN_PROPERTIES] def test_trashbin_list_requests_trashbin_prop_type(monkeypatch):