From 2a00f221ddaa33a2dbc9b8d10f4873478636a300 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 22 Sep 2026 16:11:01 +0000 Subject: [PATCH 1/2] fix: validate document keys before they reach request URLs Keys are placed into pyArango document URLs unencoded. `_make_key` now checks the joined key against ArangoDB's document-key alphabet (minus `%`, which the server would percent-decode), rejects `.`/`..` and keys over 254 bytes, and raises KeyError otherwise. New public helper: `validate_document_key`. CI: live-server tests and doctests are skipped when no ArangoDB server is reachable (they previously errored at collection), and docsrc/ is excluded from test collection. Closes #2 Co-Authored-By: Claude Opus 5 --- arangodol/__init__.py | 39 +++++++++++++++++++++++++++++++- arangodol/tests/_live_server.py | 17 ++++++++++++++ arangodol/tests/test_arangodb.py | 7 ++++-- arangodol/tests/test_keys.py | 36 +++++++++++++++++++++++++++++ conftest.py | 20 ++++++++++++++++ pyproject.toml | 1 + 6 files changed, 117 insertions(+), 3 deletions(-) create mode 100644 arangodol/tests/_live_server.py create mode 100644 arangodol/tests/test_keys.py create mode 100644 conftest.py diff --git a/arangodol/__init__.py b/arangodol/__init__.py index 16e602c..9d698ec 100644 --- a/arangodol/__init__.py +++ b/arangodol/__init__.py @@ -1,12 +1,49 @@ """ arango with a simple (dict-like or list-like) interface """ +import re + from dol.base import Persister from pyArango.connection import Connection from pyArango.theExceptions import DocumentNotFoundError +# ArangoDB's document-key alphabet, minus ``%``: pyArango places keys in request +# URLs without encoding them, so a ``%`` would be percent-decoded by the server +# and address a different document than the one stored. +_VALID_KEY_CHARS = re.compile(r"[A-Za-z0-9_\-:.@()+,=;$!*']+") +_MAX_KEY_BYTES = 254 +_DOT_SEGMENTS = {".", ".."} + + +def validate_document_key(key): + """Return ``key`` if it is a safe ArangoDB document key, else raise ``KeyError``. + + Keys end up in request URLs, so anything outside ArangoDB's key alphabet (such + as ``/``) or a dot segment could address a different collection or database. + + >>> validate_document_key('Robot::0a1b') + 'Robot::0a1b' + >>> validate_document_key('a/b') + Traceback (most recent call last): + ... + KeyError: "Invalid ArangoDB document key: 'a/b'" + >>> validate_document_key('..') + Traceback (most recent call last): + ... + KeyError: "Invalid ArangoDB document key: '..'" + """ + if ( + not isinstance(key, str) + or key in _DOT_SEGMENTS + or len(key.encode("utf-8")) > _MAX_KEY_BYTES + or not _VALID_KEY_CHARS.fullmatch(key) + ): + raise KeyError(f"Invalid ArangoDB document key: {key!r}") + return key + + class ArangoDbPersister(Persister): """ A basic ArangoDB persister. @@ -111,7 +148,7 @@ def _make_key(self, keys_dict): """ key_values = [keys_dict[key_label] for key_label in self._key_fields] key_str = self._key_fields_separator.join(key_values) - return key_str + return validate_document_key(key_str) def _split_key(self, joined_key_str): """ diff --git a/arangodol/tests/_live_server.py b/arangodol/tests/_live_server.py new file mode 100644 index 0000000..10a75bf --- /dev/null +++ b/arangodol/tests/_live_server.py @@ -0,0 +1,17 @@ +"""Detect whether the local ArangoDB test server is reachable (for skipping live tests).""" + +import socket +from urllib.parse import urlparse + +DB_URL = "http://127.0.0.1:8529" +DB_PASSWORD = "somepassword" +_CONNECT_TIMEOUT_S = 1 + + +def server_is_up(url=DB_URL, *, timeout=_CONNECT_TIMEOUT_S): + parsed = urlparse(url) + try: + with socket.create_connection((parsed.hostname, parsed.port), timeout=timeout): + return True + except OSError: + return False diff --git a/arangodol/tests/test_arangodb.py b/arangodol/tests/test_arangodb.py index b3495eb..706bca6 100644 --- a/arangodol/tests/test_arangodb.py +++ b/arangodol/tests/test_arangodb.py @@ -7,13 +7,16 @@ pytest tests/test_arangodb.py """ +import pytest + from arangodol import ArangoDbPersister from arangodol import ArangoDbTupleKeyStore +from ._live_server import DB_PASSWORD, DB_URL, server_is_up from .base_test import BasePersisterTest, BaseKeyTupleStoreTest -DB_URL = "http://127.0.0.1:8529" -DB_PASSWORD = "somepassword" +if not server_is_up(DB_URL): + pytest.skip(f"No ArangoDB server at {DB_URL}", allow_module_level=True) class TestArangoDbPersister(BasePersisterTest): diff --git a/arangodol/tests/test_keys.py b/arangodol/tests/test_keys.py new file mode 100644 index 0000000..5d9eda3 --- /dev/null +++ b/arangodol/tests/test_keys.py @@ -0,0 +1,36 @@ +"""Document keys are validated before they reach a request URL (no server needed).""" + +import pytest + +from arangodol import ArangoDbPersister, validate_document_key + + +def _persister(key_fields=("first_name", "last_name"), separator="::"): + # Bypass __init__ (which connects to a server); only key handling is tested. + p = ArangoDbPersister.__new__(ArangoDbPersister) + p._key_fields = key_fields + p._key_fields_separator = separator + return p + + +@pytest.mark.parametrize( + "key", ["abc", "Robot::0a1b", "a-b_c:d.e@f(g)+h,i=j;k$l!m*n'o", "x" * 254] +) +def test_valid_keys_pass(key): + assert validate_document_key(key) == key + + +@pytest.mark.parametrize( + "key", + ["", ".", "..", "a/b", "../other", "a%2Fb", "a b", "a?b", "a#b", "x" * 255, 42], +) +def test_invalid_keys_raise_key_error(key): + with pytest.raises(KeyError): + validate_document_key(key) + + +def test_make_key_validates_joined_key(): + p = _persister() + assert p._make_key({"first_name": "a", "last_name": "b"}) == "a::b" + with pytest.raises(KeyError): + p._make_key({"first_name": "..", "last_name": "/x"}) diff --git a/conftest.py b/conftest.py new file mode 100644 index 0000000..6b70b85 --- /dev/null +++ b/conftest.py @@ -0,0 +1,20 @@ +"""Skip the doctests that need a live ArangoDB server when none is reachable.""" + +import pytest +from _pytest.doctest import DoctestItem + +from arangodol.tests._live_server import DB_URL, server_is_up + +_LIVE_SERVER_DOCTESTS = { + "arangodol.ArangoDbPersister", + "arangodol.ArangoDbTupleKeyStore", +} + + +def pytest_collection_modifyitems(config, items): + if server_is_up(DB_URL): + return + skip = pytest.mark.skip(reason=f"No ArangoDB server at {DB_URL}") + for item in items: + if isinstance(item, DoctestItem) and item.name in _LIVE_SERVER_DOCTESTS: + item.add_marker(skip) diff --git a/pyproject.toml b/pyproject.toml index ef1b76d..a610c17 100644 --- a/pyproject.toml +++ b/pyproject.toml @@ -139,6 +139,7 @@ coverage_report_format = [ "xml", ] exclude_paths = [ + "docsrc", "examples", "scrap", ] From 40022478da6e0977be21345d26fd568ccba45134 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Tue, 22 Sep 2026 16:13:37 +0000 Subject: [PATCH 2/2] review: validate before any backend call in __setitem__; more key tests Co-Authored-By: Claude Opus 5 --- arangodol/__init__.py | 10 ++++++---- arangodol/tests/test_keys.py | 28 ++++++++++++++++++++++++++++ 2 files changed, 34 insertions(+), 4 deletions(-) diff --git a/arangodol/__init__.py b/arangodol/__init__.py index 9d698ec..5470581 100644 --- a/arangodol/__init__.py +++ b/arangodol/__init__.py @@ -9,9 +9,10 @@ from pyArango.theExceptions import DocumentNotFoundError -# ArangoDB's document-key alphabet, minus ``%``: pyArango places keys in request -# URLs without encoding them, so a ``%`` would be percent-decoded by the server -# and address a different document than the one stored. +# ArangoDB's document-key alphabet, minus ``%``. pyArango places keys in request +# URLs without encoding them and the server percent-decodes the path, so allowing +# ``%`` would let encoded ``/`` or ``..`` address other collections or databases. +# Do not add ``%`` back to match ArangoDB's documented alphabet. _VALID_KEY_CHARS = re.compile(r"[A-Za-z0-9_\-:.@()+,=;$!*']+") _MAX_KEY_BYTES = 254 _DOT_SEGMENTS = {".", ".."} @@ -180,11 +181,12 @@ def __getitem__(self, keys_dict): return data def __setitem__(self, keys_dict, values_dict): + key = self._make_key(keys_dict) # validate before touching the backend try: doc = self.__fetchitem__(keys_dict) except KeyError: doc = self._collection.createDocument() - doc._key = self._make_key(keys_dict) + doc._key = key for k, v in values_dict.items(): doc[k] = v diff --git a/arangodol/tests/test_keys.py b/arangodol/tests/test_keys.py index 5d9eda3..76322bb 100644 --- a/arangodol/tests/test_keys.py +++ b/arangodol/tests/test_keys.py @@ -34,3 +34,31 @@ def test_make_key_validates_joined_key(): assert p._make_key({"first_name": "a", "last_name": "b"}) == "a::b" with pytest.raises(KeyError): p._make_key({"first_name": "..", "last_name": "/x"}) + + +@pytest.mark.parametrize( + "key", ["%", "a\\b", "abc\n", "é", "a\x00", "a%2e%2e", "a%2F..%2Fx"] +) +def test_more_invalid_keys(key): + with pytest.raises(KeyError): + validate_document_key(key) + + +class _BackendMustNotBeCalled: + def __getitem__(self, key): + raise AssertionError(f"backend reached with key {key!r}") + + def createDocument(self, *args, **kwargs): + raise AssertionError("backend reached (createDocument)") + + +def test_invalid_key_never_reaches_backend(): + p = _persister(key_fields=("k",)) + p._collection = _BackendMustNotBeCalled() + bad = {"k": "../x"} + with pytest.raises(KeyError): + p[bad] + with pytest.raises(KeyError): + del p[bad] + with pytest.raises(KeyError): + p[bad] = {"v": 1}