From 75da5bd81018b8e51acf7432b9d70e4e86c83747 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:25:03 +0000 Subject: [PATCH 1/3] fix: two delegation bugs in dol's own code (content_url, filesys key validation) Both are instances of the same root cause (#83): a wrapper delegates a non-dunder method to an inner layer bound to that layer, so it receives the OUTER, unmapped key. 1. `content_url(store, ref_or_key)` did a flat `getattr(store, 'url_for')(key)`. On a wrapped store that returns the method bound to an inner layer, so the backend got the outer key and returned a URL for a different object than `store[key]` reads: wrapped = KeyCodecs.prefixed('a/')(Served)({'a/k': b'v'}) wrapped['k'] # b'v' -> reads a/k content_url(wrapped, 'k') # 'https://cdn/k' -> WRONG, addresses k It now walks the .store chain to the layer that actually defines `url_for`, applying each OUTER layer's `_id_of_key` on the way and stopping there -- because that layer applies its own. Resolving all the way would double-apply the provider's transform, which matters for a backend that owns a prefix (an s3dol store, the case `dol/content.py`'s docstring names). Uses `inspect.getattr_static` to tell a real `url_for` from a class-wrap's forwarding `DelegatedAttribute`. None of the existing tests caught this: they are all identity-keyed. 2. `Files(d).is_valid_key(k)` returned **False** for a key that demonstrably exists, breaking the invariant `all(s.is_valid_key(k) for k in s)`. `Files` is `mk_relative_path_store(FileBytesPersister)`, so `is_valid_key`/`validate_key` were reached through `Store.__getattr__` with the RELATIVE key while the leaf matched them against a pattern built from the absolute path. `mk_relative_path_store` now maps the key first -- the same thing its `with_key_validation=True` branch already did by hand. Tests: 4 new cases (class-wrap, instance-wrap, Pipe, no-double-apply, keyless wraps unchanged) plus a filesys case asserting the `all(is_valid_key(k) for k in s)` invariant. Verified: full suite 513 passed / 3 skipped. Dependents gate byte-for-byte identical to HEAD (xdol 4, pdfdol 23+1, chromadol 2, focal 1, dol_cookbook 1, ftpdol 1); the rest fail on absent backend SDKs both before and after. (2) is also what `focal` inherits via `LocalBinaryStore(Files)`. --- dol/content.py | 56 ++++++++++++++++++++++++++- dol/paths.py | 20 ++++++++++ dol/tests/test_content.py | 79 +++++++++++++++++++++++++++++++++++++++ dol/tests/test_filesys.py | 26 +++++++++++++ 4 files changed, 179 insertions(+), 2 deletions(-) diff --git a/dol/content.py b/dol/content.py index e2868234..57414ae3 100644 --- a/dol/content.py +++ b/dol/content.py @@ -192,6 +192,44 @@ def _url_of(ref_or_key: Any) -> Optional[str]: return None +def _url_for_provider_and_key(store: Any, key: str): + """The layer that owns ``url_for``, and ``key`` expressed in *that layer's* key space. + + ``getattr(store, 'url_for')`` on a wrapped store returns the method bound to an inner + layer, so handing it the outer key addresses the wrong object. Walk the ``.store`` chain + to find the layer that actually defines ``url_for``, applying every *outer* layer's + ``_id_of_key`` on the way -- and stopping there, because that layer applies its own. + + Returns ``(None, key)`` when no layer provides ``url_for``. + """ + import inspect + + from dol.base import DelegatedAttribute + + def _really_defines_url_for(obj) -> bool: + # ``getattr_static`` does not invoke descriptors, so a class-wrap's + # ``DelegatedAttribute`` is visible as itself rather than as the inner bound method. + # A layer that only *forwards* ``url_for`` is not the provider. + try: + attr = inspect.getattr_static(obj, "url_for") + except AttributeError: + return False + return not isinstance(attr, DelegatedAttribute) + + layer, k = store, key + while layer is not None: + if _really_defines_url_for(layer): + return layer, k + inner = getattr(layer, "store", None) + if inner is None: + return None, k + id_of_key = getattr(layer, "_id_of_key", None) + if callable(id_of_key): + k = id_of_key(k) + layer = inner + return None, k + + def content_url(store: Any, ref_or_key: Any) -> Optional[str]: """A fetchable URL for content, resolved **on demand**. @@ -206,12 +244,26 @@ def content_url(store: Any, ref_or_key: Any) -> Optional[str]: True >>> content_url({}, ContentRef('k1', url='https://carried/k1')) # ref carries its own 'https://carried/k1' + + The key is resolved **through any wrapping layers**, so a URL addresses the same object + ``store[key]`` reads. Without this, a key-transforming wrap would hand the backend the + outer key and silently return a URL for a different object: + + >>> from dol import KeyCodecs + >>> wrapped = KeyCodecs.prefixed('a/')(Served)({'a/k1': b'v'}) + >>> wrapped['k1'] + b'v' + >>> content_url(wrapped, 'k1') + 'https://cdn.example/a/k1' """ carried = _url_of(ref_or_key) if carried: return carried - url_for = getattr(store, "url_for", None) - return url_for(_key_of(ref_or_key)) if callable(url_for) else None + provider, key = _url_for_provider_and_key(store, _key_of(ref_or_key)) + if provider is None: + return None + url_for = getattr(provider, "url_for", None) + return url_for(key) if callable(url_for) else None def _ref( diff --git a/dol/paths.py b/dol/paths.py index e4d386b5..6fa9265f 100644 --- a/dol/paths.py +++ b/dol/paths.py @@ -1207,6 +1207,26 @@ def _id_of_key(self, k): cls._id_of_key = _id_of_key + # Key-validation methods must see the INNER (absolute) key. + # + # ``is_valid_key``/``validate_key`` are defined on the wrapped class and reached through + # ``Store.__getattr__``, which hands the leaf the OUTER (relativized) key. The leaf then + # matches it against a pattern built from the absolute path, so + # ``Files(d).is_valid_key('a.txt')`` was False for a key that demonstrably exists. Map the + # key first, exactly as the ``with_key_validation`` branch above already does by hand. + for _method_name in ("is_valid_key", "validate_key"): + if hasattr(store_cls, _method_name) and _method_name not in cls.__dict__: + + def _key_mapped(self, k, *args, __name=_method_name, **kwargs): + return getattr(self.store, __name)(self._id_of_key(k), *args, **kwargs) + + _key_mapped.__name__ = _method_name + _key_mapped.__qualname__ = f"{cls.__name__}.{_method_name}" + _key_mapped.__doc__ = ( + f"``{_method_name}`` on the inner key -- see ``mk_relative_path_store``." + ) + setattr(cls, _method_name, _key_mapped) + # if __module__ is not None: # cls.__module__ = __module__ diff --git a/dol/tests/test_content.py b/dol/tests/test_content.py index f71c8022..db05d084 100644 --- a/dol/tests/test_content.py +++ b/dol/tests/test_content.py @@ -169,3 +169,82 @@ def test_backend_injection_dict_vs_class(): cas = with_content_addressing(backing) ref = cas.add(b"shared-bytes") assert backing[ref.item_id] == b"shared-bytes" # writes land in the injected backend + + +# ------------------------------------------------------------------------------------- +# content_url must resolve the key through wrapping layers +# +# It used to do a flat ``getattr(store, 'url_for')(key)``. On a wrapped store that returns +# the method bound to an inner layer, so the backend received the OUTER key and returned a +# URL for a different object than ``store[key]`` reads. + + +class _Served(dict): + def url_for(self, key): + return f"https://cdn/{key}" + + +def test_content_url_resolves_through_a_key_wrap(): + from dol import KeyCodecs, Pipe, content_url + + # class-wrap: url_for reaches the leaf via a DelegatedAttribute + wrapped = KeyCodecs.prefixed("a/")(_Served)({"a/k": b"v"}) + assert wrapped["k"] == b"v" + assert content_url(wrapped, "k") == "https://cdn/a/k" + + # instance-wrap: url_for reaches the leaf via Store.__getattr__ + wrapped = KeyCodecs.prefixed("a/")(_Served({"a/k": b"v"})) + assert content_url(wrapped, "k") == "https://cdn/a/k" + + # stacked + stacked = Pipe(KeyCodecs.prefixed("a/"), KeyCodecs.prefixed("b/"))( + _Served({"a/b/k": b"v"}) + ) + assert content_url(stacked, "k") == "https://cdn/a/b/k" + + +def test_content_url_unchanged_for_unwrapped_and_keyless_wraps(): + from dol import KeyCodecs, content_url, filt_iter, wrap_kvs + + assert content_url(_Served({"k": b"v"}), "k") == "https://cdn/k" + assert content_url({}, "k") is None + # wraps that do not change keys must not change the URL + assert ( + content_url(wrap_kvs(_Served({"k": b"v"}), obj_of_data=lambda v: v), "k") + == "https://cdn/k" + ) + assert ( + content_url(filt_iter(_Served({"k": b"v"}), filt=lambda k: True), "k") + == "https://cdn/k" + ) + + +def test_content_url_does_not_double_apply_the_providers_own_transform(): + """A backend whose own ``url_for`` applies ``self._id_of_key`` (e.g. a store that owns + a prefix) must receive the key in ITS key space, not the fully-resolved one.""" + from dol import KeyCodecs, content_url + from dol.base import KvReader + + class Prefixed(KvReader): + def __init__(self, prefix=""): + self.prefix = prefix + + def _id_of_key(self, k): + return f"{self.prefix}{k}" + + def _key_of_id(self, i): + return i[len(self.prefix) :] + + def __iter__(self): + yield from () + + def __getitem__(self, k): + return b"v" + + def url_for(self, k): + return f"https://s3/{self._id_of_key(k)}" + + assert content_url(Prefixed("logs/"), "f") == "https://s3/logs/f" + assert content_url(KeyCodecs.prefixed("x/")(Prefixed("logs/")), "f") == ( + "https://s3/logs/x/f" + ) diff --git a/dol/tests/test_filesys.py b/dol/tests/test_filesys.py index c66411a4..727689dc 100644 --- a/dol/tests/test_filesys.py +++ b/dol/tests/test_filesys.py @@ -247,3 +247,29 @@ def test_subfolder_stores(): assert set(folder2_store.keys()) == {"this.txt", "over.json"} assert folder2_store["this.txt"] == b"that" assert folder2_store["over.json"] == b"there" + + +def test_is_valid_key_sees_the_inner_key(tmpdir): + """``Files`` is ``mk_relative_path_store(FileBytesPersister)``, so ``is_valid_key`` was + reached through ``Store.__getattr__`` with the RELATIVE key while the leaf matched it + against a pattern built from the absolute path -- returning False for keys that exist.""" + import os + + from dol import Files, TextFiles + + rootdir = str(tmpdir) + os.makedirs(os.path.join(rootdir, "sub"), exist_ok=True) + with open(os.path.join(rootdir, "a.txt"), "w") as fp: + fp.write("x") + with open(os.path.join(rootdir, "sub", "b.txt"), "w") as fp: + fp.write("y") + + s = Files(rootdir) + assert sorted(s) == ["a.txt", "sub/b.txt"] + # the invariant that was broken: every key the store yields is a valid key + assert all(s.is_valid_key(k) for k in s) + assert s.is_valid_key("a.txt") + assert s.is_valid_key("sub/b.txt") + s.validate_key("a.txt") # must not raise + assert TextFiles(rootdir).is_valid_key("a.txt") + assert s["a.txt"] == b"x" # reads unaffected From 48e1f146a81e343819c229d66f1184b5436e9ffa Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:30:11 +0000 Subject: [PATCH 2/3] test: make the is_valid_key test separator-agnostic (Windows CI) --- dol/tests/test_filesys.py | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/dol/tests/test_filesys.py b/dol/tests/test_filesys.py index 727689dc..1f50495e 100644 --- a/dol/tests/test_filesys.py +++ b/dol/tests/test_filesys.py @@ -265,11 +265,12 @@ def test_is_valid_key_sees_the_inner_key(tmpdir): fp.write("y") s = Files(rootdir) - assert sorted(s) == ["a.txt", "sub/b.txt"] + nested_key = os.path.join("sub", "b.txt") # separator differs on Windows + assert sorted(s) == ["a.txt", nested_key] # the invariant that was broken: every key the store yields is a valid key assert all(s.is_valid_key(k) for k in s) assert s.is_valid_key("a.txt") - assert s.is_valid_key("sub/b.txt") + assert s.is_valid_key(nested_key) s.validate_key("a.txt") # must not raise assert TextFiles(rootdir).is_valid_key("a.txt") assert s["a.txt"] == b"x" # reads unaffected From 0db03ed806aad36cd1ca9c7bcaf09c81efa6de10 Mon Sep 17 00:00:00 2001 From: Thor Whalen <1906276+thorwhalen@users.noreply.github.com> Date: Mon, 10 Aug 2026 16:49:55 +0000 Subject: [PATCH 3/3] fix: address adversarial review of the content_url walk Three defects found by an independent refutation pass, all in the new `_url_for_provider_and_key`, plus its cost. None were caught by the tests. 1. UNBOUNDED WALK. `while layer is not None` had no cycle detection and no depth bound, so `content_url(MagicMock(), k)` hung forever -- every `.store` on a mock mints a fresh child -- as did a store whose `.store` returns itself. Master was bounded by Python's recursion limit; this was not. Any downstream test passing a mock store to `content_url` would have wedged CI. Now bounded by an id-based `seen` set (cycles) AND a depth cap (chains that never repeat and never end). 2. STATIC LOOKUP BROKE DUCK TYPING. A store providing `url_for` via `__getattr__` (proxies, lazy-client wrappers) is invisible to a static lookup, so the walk found no provider and `content_url` returned None -- which callers read as "no URL exists". That is exactly the silent-wrong-answer failure mode this PR set is against. Now falls back to plain `getattr(store, 'url_for')` with the original key when the walk finds nothing, making the behaviour a strict superset of master's. 3. COLLISION WITH `with_key_validation=True`. That option redefines `_id_of_key` to RAISE `KeyError` on an invalid key, and the new `is_valid_key`/`validate_key` wrappers called it -- so the predicate raised for precisely the input it exists to answer "no" for, and `validate_key`'s exception type changed from the leaf's to `KeyError`. The wrappers now use `PrefixRelativizationMixin._id_of_key`, the unvalidated mapping. Also: `content_url` was 14.5x slower (3.4us -> 49.5us). Two causes, both fixed -- `import inspect` + `from dol.base import ...` ran on every call (hoisted to module level), and `getattr_static` was called even for unwrapped stores. Checking `.store` before the static lookup restores the unwrapped fast path (3.2us, master parity), and replacing `getattr_static` with a hand-rolled MRO scan makes the wrapped path ~7x cheaper. The scan agrees with `getattr_static` on every shape that matters (instance attribute, plain method, __slots__, class-wrap, instance-wrap, __getattr__-provided), and anything it misses falls through to the plain-getattr fallback, so a miss costs correctness nothing. Documented the `DelegatedAttribute` duplication (`dol.base` vs the unused `dol.util` copy) that the isinstance check depends on. Tests: 3 new cases -- pathological chains terminate, a dynamically-provided `url_for` is still found, and `with_key_validation=True` coexists with the wrappers. 516 passed / 3 skipped. --- dol/content.py | 87 +++++++++++++++++++++++++++------------ dol/paths.py | 8 +++- dol/tests/test_content.py | 31 ++++++++++++++ dol/tests/test_paths.py | 28 +++++++++++++ 4 files changed, 127 insertions(+), 27 deletions(-) diff --git a/dol/content.py b/dol/content.py index 57414ae3..5e8a98a7 100644 --- a/dol/content.py +++ b/dol/content.py @@ -63,6 +63,7 @@ ) from dol.base import KvPersister +from dol.base import DelegatedAttribute as _DelegatedAttribute #: The ``_tag`` discriminator value carried on the JSON wire form (cross-language parity). CONTENT_REF_TAG = "ContentRef" @@ -192,6 +193,36 @@ def _url_of(ref_or_key: Any) -> Optional[str]: return None +_MAX_STORE_CHAIN_DEPTH = 100 # no real wrap stack is remotely this deep + + +def _statically_defines_url_for(obj) -> bool: + """True if ``obj`` itself provides ``url_for`` (as opposed to forwarding it inward). + + Looks the attribute up **without invoking descriptors**, so a class-wrap's + ``DelegatedAttribute`` is seen as itself rather than as the inner bound method -- a layer + that only *forwards* ``url_for`` is not the provider. + + This is a hand-rolled ``inspect.getattr_static``. It agrees with it on every shape that + matters here (instance attribute, plain method, ``__slots__``, class-wrap, instance-wrap, + ``__getattr__``-provided) and is ~7x faster, which matters because ``content_url`` is + called per object. Anything this misses falls through to the plain-``getattr`` fallback + in :func:`content_url`, so a miss costs correctness nothing. + + NOTE: ``DelegatedAttribute`` is defined twice in dol -- ``dol.base`` (the one the wrap + machinery constructs) and an unused duplicate in ``dol.util``. If a wrap path ever + switches to the other copy this check silently stops working, so they must not diverge. + """ + d = getattr(obj, "__dict__", None) + if d is not None and "url_for" in d: + return not isinstance(d["url_for"], _DelegatedAttribute) + for klass in type(obj).__mro__: + attr = klass.__dict__.get("url_for") + if attr is not None: + return not isinstance(attr, _DelegatedAttribute) + return False + + def _url_for_provider_and_key(store: Any, key: str): """The layer that owns ``url_for``, and ``key`` expressed in *that layer's* key space. @@ -200,34 +231,31 @@ def _url_for_provider_and_key(store: Any, key: str): to find the layer that actually defines ``url_for``, applying every *outer* layer's ``_id_of_key`` on the way -- and stopping there, because that layer applies its own. - Returns ``(None, key)`` when no layer provides ``url_for``. + Returns ``(None, key)`` -- with ``key`` unchanged -- when the walk finds no provider, so + the caller can fall back to plain ``getattr``. + + The walk is bounded two ways. ``seen`` catches a chain that points back at itself; the + depth cap catches one that never repeats and never ends, which is what a ``MagicMock`` + does (every ``.store`` mints a fresh child). """ - import inspect - - from dol.base import DelegatedAttribute - - def _really_defines_url_for(obj) -> bool: - # ``getattr_static`` does not invoke descriptors, so a class-wrap's - # ``DelegatedAttribute`` is visible as itself rather than as the inner bound method. - # A layer that only *forwards* ``url_for`` is not the provider. - try: - attr = inspect.getattr_static(obj, "url_for") - except AttributeError: - return False - return not isinstance(attr, DelegatedAttribute) - - layer, k = store, key - while layer is not None: - if _really_defines_url_for(layer): - return layer, k + layer, k, seen = store, key, set() + for _ in range(_MAX_STORE_CHAIN_DEPTH): + if layer is None or id(layer) in seen: + break + seen.add(id(layer)) inner = getattr(layer, "store", None) if inner is None: - return None, k + # Innermost layer: nothing left to forward to, so it is the provider if it has + # ``url_for`` at all. Checking ``.store`` first keeps the unwrapped case -- + # by far the common one -- on a single cheap ``getattr``. + return layer, k + if _statically_defines_url_for(layer): + return layer, k id_of_key = getattr(layer, "_id_of_key", None) if callable(id_of_key): k = id_of_key(k) layer = inner - return None, k + return None, key def content_url(store: Any, ref_or_key: Any) -> Optional[str]: @@ -259,11 +287,18 @@ def content_url(store: Any, ref_or_key: Any) -> Optional[str]: carried = _url_of(ref_or_key) if carried: return carried - provider, key = _url_for_provider_and_key(store, _key_of(ref_or_key)) - if provider is None: - return None - url_for = getattr(provider, "url_for", None) - return url_for(key) if callable(url_for) else None + outer_key = _key_of(ref_or_key) + provider, key = _url_for_provider_and_key(store, outer_key) + if provider is not None: + url_for = getattr(provider, "url_for", None) + if callable(url_for): + return url_for(key) + # Fallback to plain duck typing, with the key as given. The walk above looks attributes + # up statically and so cannot see a ``url_for`` supplied by ``__getattr__`` (a proxy, a + # lazy-client wrapper). Returning None there would be a silent wrong answer; this keeps + # such stores working exactly as they did before. + url_for = getattr(store, "url_for", None) + return url_for(outer_key) if callable(url_for) else None def _ref( diff --git a/dol/paths.py b/dol/paths.py index 6fa9265f..63c92b38 100644 --- a/dol/paths.py +++ b/dol/paths.py @@ -1218,7 +1218,13 @@ def _id_of_key(self, k): if hasattr(store_cls, _method_name) and _method_name not in cls.__dict__: def _key_mapped(self, k, *args, __name=_method_name, **kwargs): - return getattr(self.store, __name)(self._id_of_key(k), *args, **kwargs) + # Use the mixin's *unvalidated* mapping, not ``self._id_of_key``. Under + # ``with_key_validation=True`` the latter is redefined above to RAISE + # ``KeyError`` on an invalid key -- which would make ``is_valid_key`` raise + # for exactly the input it exists to answer "no" for, and would change + # ``validate_key``'s exception type. + _id = PrefixRelativizationMixin._id_of_key(self, k) + return getattr(self.store, __name)(_id, *args, **kwargs) _key_mapped.__name__ = _method_name _key_mapped.__qualname__ = f"{cls.__name__}.{_method_name}" diff --git a/dol/tests/test_content.py b/dol/tests/test_content.py index db05d084..cdf350df 100644 --- a/dol/tests/test_content.py +++ b/dol/tests/test_content.py @@ -248,3 +248,34 @@ def url_for(self, k): assert content_url(KeyCodecs.prefixed("x/")(Prefixed("logs/")), "f") == ( "https://s3/logs/x/f" ) + + +def test_content_url_terminates_on_pathological_chains(): + """The chain walk must be bounded. A ``MagicMock`` mints a fresh child for every + ``.store``, and a self-referential store cycles -- both used to hang forever.""" + from unittest.mock import MagicMock + + from dol import content_url + + content_url(MagicMock(), "k") # must simply return + + class SelfStore: + @property + def store(self): + return self + + assert content_url(SelfStore(), "k") is None + + +def test_content_url_still_finds_a_dynamically_provided_url_for(): + """``url_for`` supplied via ``__getattr__`` is invisible to a static lookup. Returning + None there would be a silent wrong answer, so we fall back to plain duck typing.""" + from dol import content_url + + class Dyn(dict): + def __getattr__(self, name): + if name == "url_for": + return lambda key: f"https://dyn/{key}" + raise AttributeError(name) + + assert content_url(Dyn({"k": 1}), "k") == "https://dyn/k" diff --git a/dol/tests/test_paths.py b/dol/tests/test_paths.py index 976590b6..9a1472fc 100644 --- a/dol/tests/test_paths.py +++ b/dol/tests/test_paths.py @@ -91,3 +91,31 @@ def test_string_template_simple(): VersionedFile = st.dict_to_namedtuple({"i01_": "life", "version": 42}) assert VersionedFile == namedtuple("VersionedFile", ["i01_", "version"])("life", 42) assert st.namedtuple_to_dict(VersionedFile) == {"i01_": "life", "version": 42} + + +def test_key_validation_wrappers_coexist_with_with_key_validation(): + """``with_key_validation=True`` redefines ``_id_of_key`` to RAISE on an invalid key, so + the key-mapping wrappers must use the unvalidated mapping -- otherwise ``is_valid_key`` + raises for exactly the input it exists to answer 'no' for.""" + from dol.paths import mk_relative_path_store + + class Leaf(dict): + _prefix = "/ROOT/" + + def is_valid_key(self, k): + return k.startswith("/ROOT/") and k.endswith(".txt") + + def validate_key(self, k): + if not self.is_valid_key(k): + raise ValueError(k) + + s = mk_relative_path_store(Leaf, with_key_validation=True)() + assert s.is_valid_key("b.txt") is True + assert s.is_valid_key("b.json") is False # a bool, not a raise + s.validate_key("b.txt") + try: + s.validate_key("b.json") + except ValueError: + pass # the LEAF's exception type, not KeyError from the validating _id_of_key + else: + raise AssertionError("validate_key should have raised ValueError")