Key-transform bypass: `aget` gets the OUTER key — `RelPathAioFileBytesReader.aget('a.txt')` silently reads a file outside the store

[thorwhalen]

Summary

AioFileBytesReader.aget — the only read method this package offers — is handed the outer, untransformed key when the class is wrapped in mk_relative_path_store. Since the package ships exactly such a wrapper (RelPathAioFileBytesReader), this is live today, and it is silent: aget resolves the relative key against the process working directory, so it can return the bytes of a completely unrelated file instead of the one in the store.

This is an instance of the family tracked in i2mint/dol#83 (umbrella), root cause i2mint/dol#18.

Mechanism

dol wraps stores by delegation (has-a), not inheritance. The wrapper maps keys for the dunder protocol (__getitem__ / __setitem__ / __delitem__ / __contains__ / __iter__) — and for nothing else. Every other non-dunder method reaches the leaf bound to the leaf, with the outer key unmapped. Two routes:

• Route A — instance-wraps and mk_relative_path_store subclasses. Store.__getattr__ (dol/base.py:742) returns getattr(self.store, attr) — the leaf-bound method.
• Route B — class-wraps. delegate_to (dol/base.py:416) installs a DelegatedAttribute per attr; its __get__ (dol/base.py:279) also returns getattr(instance.store, attr).

This package hits Route A. mk_relative_path_store builds type(store_cls.__name__, (PrefixRelativizationMixin, Store), {}) (dol/paths.py:1163) — a delegating Store, not a subclass of the leaf. So RelPathAioFileBytesReader(...).aget is AioFileBytesReader.aget bound to the inner store:

s.aget.__self__ is s.store   # True
s.aget.__qualname__          # 'AioFileBytesReader.aget'

aget then does AIOFile(k, ...) (aiofiledol/__init__.py:49) on a key like 'greeting' — a relative path, resolved by the OS against the current working directory.

Why it is silent rather than loud here

Three things line up badly:

1. __getitem__ = None (aiofiledol/__init__.py:28). The mapped read route is deliberately disabled — s['greeting'] raises TypeError: 'NoneType' object is not callable. aget is not a convenience alongside a correct __getitem__; it is the entire read API, and there is no working alternative to fall back on.
2. The key validator is commented out on aget (aiofiledol/__init__.py:30). asetitem keeps @validate_key_and_raise_key_error_on_exception (:84) and therefore fails loudly; aget has no such backstop. (The README still advertises KeyValidationError for an out-of-store key — in current code that call raises FileNotFoundError.)
3. The scoping the leaf does have is absolute-path based. is_valid_key matches self._key_pattern, built from the absolute rootdir — so it also returns the wrong answer for outer keys.

Net effect: aget reads whatever the relative path happens to name in the CWD, or raises FileNotFoundError for a key that list(s) just yielded and k in s just accepted.

Affected symbols

Symbol	Location	Verdict	Severity	Notes
AioFileBytesReader.aget	aiofiledol/__init__.py:31 (AIOFile at :49)	CONFIRMED LIVE	silent wrong result / reads outside the store	Shipped as RelPathAioFileBytesReader (:118) and RelPathFileStringReader (:133)
is_valid_key / validate_key (delegated, from dol.filesys)	wrap site aiofiledol/__init__.py:118	CONFIRMED LIVE	wrong scope	False / KeyValidationError for every valid outer key; inherited defect, same shape as the Files case in dol#83
AioFileBytesPersister.asetitem	aiofiledol/__init__.py:85 (AIOFile at :105)	LATENT	wrong scope (fail-loud, not destructive)	Only if a user relativizes a persister; the package never does. The retained validator turns it into KeyValidationError — nothing is written or destroyed
AioFileStringReader.aget (inherited)	aiofiledol/__init__.py:125	CONFIRMED LIVE	same as aget	Same body, reached via RelPathFileStringReader

Not affected, verified: keys / items / values resolve on the wrapper and map correctly; with_relative_paths returns a correct single-level view here; the mapped __setitem__ route (Store.__setitem__ → leaf __setitem__ at :109 → asetitem with the already-mapped key) writes to the right place when run inside an event loop.

Reproduction (REAL — this package, run as-is)

import asyncio, os, tempfile
from aiofiledol import RelPathAioFileBytesReader

# a real store with a real file in it
root = tempfile.mkdtemp()
with open(os.path.join(root, 'greeting'), 'wb') as f:
    f.write(b'REAL STORE CONTENT')

# an unrelated file with the same *relative* name, in the process CWD
cwd = tempfile.mkdtemp()
os.chdir(cwd)
with open('greeting', 'wb') as f:
    f.write(b'DECOY FROM CWD')

s = RelPathAioFileBytesReader(root)

print(list(s))            # ['greeting']          the key is relative
print('greeting' in s)    # True                  the store agrees it exists

s['greeting']             # TypeError: 'NoneType' object is not callable   (__getitem__ = None)

print(asyncio.run(s.aget('greeting')))
# -> b'DECOY FROM CWD'    WRONG. Expected b'REAL STORE CONTENT'

os.remove('greeting')     # remove the decoy
asyncio.run(s.aget('greeting'))
# -> FileNotFoundError: [Errno 2] No such file or directory: 'greeting'
#    ...for a key that list(s) yields and `in` accepts.

print(s.is_valid_key('greeting'))   # False   WRONG

User-visible consequence

For any user of RelPathAioFileBytesReader / RelPathFileStringReader — the relative-path classes, i.e. the ergonomic ones:

• Reads silently return the wrong bytes. If a file with the same relative name exists under the process CWD, aget returns that file's contents. The store's rootdir is not consulted at all. A store that is supposed to be confined to rootdir reads arbitrary CWD-relative paths — including ../ traversals if keys are ever user-influenced.
• Otherwise reads fail on keys the store says it has. list(s), k in s, len(s) all work; s.aget(k) on the very key just enumerated raises FileNotFoundError. There is no fallback, because __getitem__ is None.
• Key validation lies in the other direction. s.is_valid_key(k) is False and s.validate_key(k) raises for every key the store legitimately contains, so user code that guards with these rejects everything.

No data is destroyed. The write path (asetitem) is latent and fail-loud: the retained validator rejects the unmapped key with KeyValidationError before any file is opened, so nothing is written to a wrong location and nothing is overwritten. This is a read-side confidentiality/correctness bug, not a destructive one.

Suggested remediation

Resolve the key through the whole wrapper chain at the top of each async method. wrapped_self climbs the back-reference registry from the leaf to the outermost wrapper (verified: wrapped_self(s.store) is s), and inner_most_key then walks the chain's _id_of_keys:

from dol import wrapped_self          # exported from dol
from dol.dig import inner_most_key    # NOT exported from dol — import from dol.dig

def _resolve_key(self, k):
    kk = inner_most_key(wrapped_self(self), k)
    return kk if isinstance(kk, str) else k   # str-check is mandatory, see below

class AioFileBytesReader(FileCollection, KvReader):
    async def aget(self, k):
        async with AIOFile(_resolve_key(self, k), **self._read_open_kwargs) as fp:
            return await fp.read()

Two traps, both real:

• inner_most_key walks the ENTIRE chain, including the leaf's own _id_of_key. It replaces self._id_of_key(k) and must never be composed with it, or the key is transformed twice.
• It returns None, silently, when no layer in the chain defines _id_of_key — which is exactly the un-wrapped case (inner_most_key(wrapped_self(bare_leaf), 'a.txt') → None). The isinstance(kk, str) fallback is not optional; without it, an un-relativized AioFileBytesReader would start passing None to AIOFile.

Verified working, both ways round:

# with the fix applied to aget:
RelFixed(root).aget('a.txt')                       # -> b'HI'   (relativized: correct file)
Fixed(root, max_levels=0).aget(f'{root}/a.txt')    # -> b'HI'   (bare: absolute keys still work)

Apply the same treatment to asetitem (:85) to close the latent write-path case. There is precedent for this pattern inside dol itself — dol/filesys.py:766 and :825 already use inner_most_key to recover the full path.

Two smaller items worth folding in while touching this file:

• Restore the @validate_key_and_raise_key_error_on_exception decorator on aget (currently commented out at :30). Once the key is resolved correctly, the validator turns any remaining mismatch into a loud KeyValidationError instead of a wrong read — and it makes the code match the README, which already documents that behaviour.
• __getitem__ = None (:28) removes every escape route. If a synchronous read cannot be supported, consider raising a NotImplementedError with a message pointing at aget, rather than a bare TypeError: 'NoneType' object is not callable.

Notes on the original survey

Two corrections, for the record: mk_relative_path_store appears at 2 call sites, not 3 (the third grep hit is the import at :10); and both call sites wrap readers, which is exactly why the read path is confirmed-live while the write path is only latent.

---

Note on in-flight upstream fixes (added when filing)

Two dol PRs are open and change details referenced above:

• i2mint/dol#84 — inner_most_key and unravel_key
  become importable from dol directly (no more from dol.dig import ...), and
  inner_most_key now raises instead of returning None when no layer of the chain
  defines _id_of_key. If you write a local shim, the isinstance(_id, str) guard becomes
  unnecessary once that lands — but the "it replaces _id_of_key, never composes with it" trap
  still applies.
• i2mint/dol#85 — fixes dol.content_url to
  resolve the key through wrapping layers, and makes mk_relative_path_store install
  key-mapping is_valid_key/validate_key. Any finding above that is inherited from
  dol.filesys.Files is repaired by #85 with no change needed in this repo — this issue will
  be closed with verification once it merges.

Design context for why the ecosystem-wide answer is not "sprinkle wrapped_self everywhere":
i2mint/s3dol#14 and
s3dol ADR-0011.
Short version: wrapped_self is a best-effort guardrail with its own silent failure mode (it
degrades to the raw leaf when nothing holds a reference to the wrapper), so the durable fix is
to have no key-taking methods rather than to harden each one.

[thorwhalen]

Verified: the confirmed-live key-validation row is fixed upstream — no change needed here

i2mint/dol#85 merged. mk_relative_path_store now maps the key inward before consulting the
leaf, so the delegated is_valid_key/validate_key row in the table above is repaired without
touching this repo:

keys: ['greeting']
is_valid_key('greeting') -> True        (was False)
validate_key('greeting') -> no raise    (was KeyValidationError)

Requires dol >= 0.3.59.

Still open, and unaffected by that fix

The other findings are aiofiledol's own, not inherited:

• aget / asetitem treat the key as an absolute filesystem path — the core finding, and
  the reason this one matters more than most in the census: the class sets __getitem__ = None,
  so these two are the entire read/write API, and the package applies mk_relative_path_store
  to itself (3 uses).
• The key validator is commented out on aget (aiofiledol/__init__.py:30) while
  asetitem keeps it, so reads have no backstop and the README's advertised
  KeyValidationError is actually FileNotFoundError.

Neither is repaired by the upstream change. Leaving this issue open, scoped to those.

Note on the remediation sketch in the original report: inner_most_key is now importable
straight from dol (no more from dol.dig import ...) and raises instead of returning
None when the chain has no _id_of_key, so the isinstance(_id, str) guard is no longer
needed — see i2mint/dol#84. The "it replaces _id_of_key, never composes with it" trap still
applies. And before reaching for wrapped_self: it has its own silent failure mode when nothing
holds a reference to the wrapper (i2mint/dol#18), which is why the durable answer for an adapter
is to not have key-taking methods at all — see i2mint/dol#86.