dol#83 audit: redisdol — key-codec escape via RedisFactories (latent); RedisList value-codec claim refuted as stated

[thorwhalen]

Audit of redisdol against the wrapper-delegation bug class tracked in i2mint/dol#83 (root cause: i2mint/dol#18).

Headline: redisdol is NOT affected today. It applies exactly one dol wrapper, and that wrapper carries no key transform. The value-side allegation about RedisList.append/extend/insert does not hold as stated and is refuted below. Two real latent hazards did survive verification, and the more serious of the two is not the one that was alleged.

Mechanism (why any of this matters)

dol wraps stores by delegation. A wrapper maps keys for __getitem__ / __setitem__ / __delitem__ / __contains__ / __iter__, but every other (non-dunder) attribute is fetched leaf-bound, so it never sees the mapping:

• Route A — instance-wraps and mk_relative_path_store subclasses: Store.__getattr__ returns getattr(self.store, attr).
• Route B — class-wraps: delegate_to installs a DelegatedAttribute for every attr in dir(wrapped); its __get__ also returns getattr(instance.store, attr).

There is a value-side mirror: anything that reads or writes values straight off the leaf bypasses obj_of_data / data_of_obj.

How redisdol wraps its stores

Grep over the package for mk_relative_path_store, KeyCodecs, prefixless_view, filt_iter, wrap_kvs, PrefixRelativizationMixin, KeyTemplate, kv_wrap, Pipe:

construct	count	what it wraps
wrap_kvs	1 — redisdol/stores.py:13	RedisStoreWithNumericLists(RedisPersister), with obj_of_data=_convert_numeric_list only
everything else	0	—

So: no key transform exists anywhere in redisdol, and the single wrapper is value-side only. Nothing in this bug class can be confirmed-live here. Everything below is either latent (fires only if a user layers a codec on top) or refuted.

For completeness, redisdol/stores.py is not imported by redisdol/__init__.py, so RedisStoreWithNumericLists is only reachable via from redisdol.stores import ....

Findings

symbol	location	verdict	severity
RedisFactories.from_source / .from_sourced_object	redisdol/base.py:16-26	latent	wrong-scope
RedisList.append	redisdol/base.py:205	latent	value-side
RedisList.extend	redisdol/base.py:208	latent	value-side
RedisList.__setitem__ (not alleged; same mechanism)	redisdol/base.py:197	latent	value-side
RedisList.insert	redisdol/base.py:211	refuted	—
RedisStoreWithNumericLists severs list write-through	redisdol/stores.py:13	confirmed-live	value-side

1. RedisFactories factory methods escape the wrapper entirely (latent, wrong-scope)

This is the real find, and it was not in the original allegation. RedisBytesCollection inherits from_source / from_sourced_object from RedisFactories (redisdol/base.py:15-26). They are public non-dunder attributes on the leaf, so both delegation routes hand back the leaf-bound classmethod. cls is therefore the unwrapped class, and the store you get back is scoped to the entire Redis keyspace, with the caller's key codec silently gone.

# LATENT — requires the user to apply a key codec. Runnable against a live redis server.
from dol import KeyCodecs
from redisdol import RedisPersister

W = KeyCodecs.prefixed('p:')(RedisPersister)   # Route B (class-wrap)
w = W()
w['mine'] = 'x'

f = w.from_sourced_object(w.store)             # looks like "another view of my store"
type(f) is RedisPersister                      # True  <-- the LEAF class, not W
list(w)                                        # [b'mine']            (scoped)
list(f)                                        # [b'p:mine', ...]     every key in the DB

Route A behaves identically: with an instance-wrap, w.from_sourced_object(w.store) also returns a bare RedisPersister.

User-visible consequence. A caller who builds a scoped store (a per-tenant / per-app prefix) and then uses either factory to derive a sibling store gets an object that looks like the same store but is not scoped at all. Reads leak every key in the database; __setitem__ writes outside the prefix; del f[k] deletes a key the scoped store could never have addressed. Nothing warns — the returned object has the same class name (RedisPersister) as the wrapper, so even type(...) in a REPL looks right.

2. RedisList write methods bypass data_of_obj (latent, value-side)

RedisList (redisdol/base.py:165) is a MutableSequence handed out as a value by RedisCollection.__getitem__ (redisdol/base.py:218-223). Its list name is fixed at construction (RedisList.__init__(self, source, name)), so there is no key argument and no key-side exposure at all — under a key codec the handle is built inside the already-remapped __getitem__, so _name correctly holds the leaf key (verified). This is a value-side issue only.

append (rpushx), extend (rpush) and __setitem__ (lset) write straight to Redis. If a store-level obj_of_data lets the RedisList through to the caller, those writes skip data_of_obj, while writes through the store apply it:

# LATENT — the codec below is the user's, not redisdol's. Runnable against a live redis server.
from dol import wrap_kvs
from redisdol import RedisPersister

def encode(obj):   # data_of_obj
    return [str(x).upper() for x in obj] if isinstance(obj, (list, tuple)) else obj

def decode(data):  # obj_of_data — passes the live RedisList straight through
    return data

s = wrap_kvs(RedisPersister, data_of_obj=encode, obj_of_data=decode)()

s['k'] = ['a', 'b']
list(s['k'])            # [b'A', b'B']              <- data_of_obj applied
rl = s['k']             # a live RedisList handle
rl.append('c')
list(s['k'])            # [b'A', b'B', b'c']        <- 'c' NOT encoded
rl.extend(['d'])
list(s['k'])            # [b'A', b'B', b'c', b'd']  <- 'd' NOT encoded
rl[0] = 'z'
list(s['k'])            # [b'z', b'B', b'c', b'd']  <- same hole in __setitem__
del s['k']

User-visible consequence. Mixed encodings inside a single Redis list: elements written through the store are encoded, elements written through the handle are raw. Nothing is destroyed, but a later decode over the list yields garbage for the raw elements, and the corruption is invisible until read-back.

Note the severity ceiling: this needs a user-supplied obj_of_data that returns the RedisList unchanged. The one codec redisdol ships does the opposite (see finding 4), so the package as published never reaches this state.

3. RedisList.insert — REFUTED

redisdol/base.py:211-213:

def insert(self, i, v):  # TODO: Implement
    raise NotImplementedError("Might have to do with self._source.linsert")

It is an unimplemented stub. It performs no write at all, so it cannot bypass a codec. The allegation is wrong about this symbol and should not be repeated in any weaker form.

4. A value codec silently converts a live handle into a detached copy (confirmed-live, value-side)

_convert_numeric_list (redisdol/stores.py:8-10) turns the RedisList into a plain list. So RedisStoreWithNumericLists never hands out a RedisList, and write-through — which the unwrapped RedisPersister does provide — silently disappears:

# CONFIRMED LIVE — no user codec involved. Runnable against a live redis server.
from redisdol import RedisPersister
from redisdol.stores import RedisStoreWithNumericLists

p = RedisPersister()
p['nums'] = [1, 2, 3]
p['nums'].append(4)          # RedisList handle -> writes through
list(p['nums'])              # [b'1', b'2', b'3', b'4']

w = RedisStoreWithNumericLists()
w['nums'] = [1, 2, 3]
type(w['nums'])              # <class 'list'>   <-- detached copy
w['nums'].append(7)
w['nums']                    # [1, 2, 3]        <-- the append went nowhere
del w['nums']

User-visible consequence. Two stores in the same package, differing only by a value codec, disagree about whether a returned sequence is live. Code written against RedisPersister that mutates the returned list keeps working syntactically after switching to RedisStoreWithNumericLists and silently stops persisting. This is the inverse of the alleged bug: the codec does not get bypassed, it removes the object that could have bypassed it.

Remediation

The usual escape hatch does not apply to anything in this package. For the record it is inner_most_key(wrapped_self(self), k) (inner_most_key from dol.dig — it is not exported from dol; wrapped_self is), and it comes with two traps: it walks the whole chain including the leaf's own _id_of_key, so it replaces self._id_of_key(k) and must never be composed with it (double transform); and it returns None silently when no layer defines _id_of_key, so an isinstance(result, str) check is mandatory. None of that helps here — no symbol in redisdol takes a key argument: the factories take a connection, and RedisList's name is bound at construction.

redisdol's fix is to shrink the delegated surface, the same move azuredol made with BlobHandle:

1. RedisFactories (redisdol/base.py:15-26) — drop it from the store classes. Two classmethods that reuse a Redis connection do not need to live on the store's attribute surface, where every wrapper will re-export them leaf-bound. Make them module-level functions taking the class explicitly, e.g. store_from_source(source, cls=RedisBytesPersister) / store_from_sourced_object(obj, cls=...). Renaming with a leading underscore is not a fix — both delegation routes forward underscore-prefixed non-dunder attributes too. If the classmethods must stay for back-compat, they should at minimum document that they return an unwrapped store over the full keyspace.
2. RedisList (redisdol/base.py:165) — give it an explicit codec pair at construction, e.g. RedisList(source, name, *, encode=identity, decode=identity), applied in __setitem__ / append / extend and inverted in __getitem__, with RedisCollection.__getitem__ passing the store's codecs down. Failing that, document RedisList as a raw handle and warn that a value codec must not be layered over a store that returns one.
3. RedisList.insert (redisdol/base.py:211) — implement it via LINSERT (needs the ref value at index i, i.e. lindex then linsert(name, 'BEFORE', ref, v)) or drop the MutableSequence claim; a MutableSequence whose insert always raises is a broken ABC contract.

Adjacent defects found while verifying (different mechanism — not part of dol#83)

These are unrelated to wrapper delegation and probably deserve their own issue(s), but they were confirmed during this audit and are worse than anything above:

• _convert_numeric_list is not total (redisdol/stores.py:8-10). No else branch, so it returns None for every non-list value. RedisStoreWithNumericLists()['a_string_key'] returns None where the leaf holds b'hello'; .get(k, 'DEFAULT') also returns None; dict(store) is silently poisoned. The doctest at redisdol/stores.py:18-23 only exercises the list case, which is why this has gone unnoticed.
• RedisCollection.__getitem__ does not raise KeyError (redisdol/base.py:218-223). No else branch, so a missing key returns None instead of raising. This breaks the Mapping contract: store.get(missing, 'DEFAULT') returns None, not 'DEFAULT'.
• RedisList.append uses RPUSHX (redisdol/base.py:205-206). RPUSHX is a no-op when the key does not exist, so RedisList(source, 'brand_new').append(x) silently discards x and returns 0. extend (RPUSH) creates the key correctly, so the two disagree. RedisList is exported from redisdol/__init__.py, so this is reachable from the public API.

Verification notes

No Redis server and no redis package were available in the audit environment, so the real redisdol source was executed against an in-memory stand-in for redis.Redis implementing keys/type/get/set/__contains__/__delitem__/exists/llen/lindex/lrange/lset/rpush/rpushx with standard Redis semantics. All outputs shown above are the stand-in's, and every command used is deterministic under the real server. The snippets themselves are written for a live server and should reproduce as-is. Line numbers are against redisdol 0.0.4.

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.