Non-dunder surface (`mkdir`, `sync_to`, `rootdir`) bypasses dol key-codec wrappers — `sync_to(..., delete_local_files_not_in_remote=True)` is destructive under a wrap

[thorwhalen]

TL;DR

sshdol's stores are clean leaves — they import no dol and wrap nothing themselves. But every non-dunder member of their public surface (mkdir, sync_to, and the documented rootdir attribute) is written against the leaf's own rootdir-relative coordinate space, so it silently ignores any dol key transform placed on top. Two of those are destructive in that situation.

Verdict for this package: LATENT, not live. Nothing sshdol ships today triggers the bug. Every finding requires a user to wrap an SshFiles / SshFilesReader in a dol key codec — which is a realistic thing to do here, since the package bills itself as "built on dol" and sync_to explicitly documents dol.Files as a supported target.

Umbrella: i2mint/dol#83 · Root cause: i2mint/dol#18

Mechanism (30 seconds)

dol wraps stores by delegation (has-a), not inheritance. A key-transform wrapper maps keys for __getitem__, __setitem__, __delitem__, __contains__ and __iter__ — and for nothing else. Every other non-dunder attribute is handed to the leaf together with the caller's outer, unmapped key. Two routes, both in dol/base.py:

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

Both routes are demonstrated below; the outcome is identical.

How sshdol is scoped

Scoping is in-leaf: self.rootdir (sshdol/base.py:241, with sftp.chdir(rootdir) at sshdol/base.py:251) plus the module-level normalize_path (sshdol/base.py:68). Wrapper census inside the package: mk_relative_path_store 0, KeyCodecs 0, prefixless_view 0, filt_iter 0, wrap_kvs 0, PrefixRelativizationMixin 0 — there is not a single import dol in sshdol/, and the only runtime dependency is paramiko. SshFilesReader subclasses collections.abc.Mapping directly.

That is exactly why this is a latent hazard and not a live bug: the leaf is self-consistent. It stops being self-consistent the moment a codec sits above it.

Affected symbols

Symbol	Location	Key-like arg	Verdict	Severity
SshFiles.mkdir(path, exist_ok=False)	sshdol/base.py:684	path (pos. 1)	latent	wrong-scope
SshFiles.sync_to(target, *, delete_local_files_not_in_remote=…) — source side	sshdol/base.py:725, source built at :824	none (scope-wide)	latent	destructive
SshFiles.sync_to — target side, reads target.rootdir	sshdol/base.py:761-767	none (scope-wide)	latent	destructive
SshFilesReader.rootdir (public data attribute)	sshdol/base.py:241	none (scope-wide)	latent	wrong-scope

The private helpers _is_dir, _path_exists, _list_directory, _walk_directory, _check_path_depth and _ensure_directory_exists share the same coordinate space, but they are underscore-private and are only ever called from dunders with already-mapped keys, so they are not listed as findings.

What actually goes wrong

1. mkdir creates the directory in the wrong place, and hands back an unwrapped store.
mkdir runs normalize_path(path) (:698), tests and creates against the leaf-relative path (:701, :715), then does return self[path] (:720). Under a wrap, path is the outer key, so the directory is created at rootdir/<outer_key> instead of rootdir/<inner_key>. Separately, self[path] at :720 is the leaf's __getitem__, so even when the key is right the returned sub-store has lost the key codec — navigating into the new directory silently drops the wrap.

2. sync_to transfers the whole remote tree regardless of the view — and --delete prunes against that whole tree.
sshdol/base.py:824 builds the rsync source as f"{user}@{host}:{self.rootdir.rstrip('/')}/". There is no key-like argument anywhere in the signature, so there is nothing a wrapper could map: the operation is unconditionally scoped to the leaf's entire rootdir. Two user-visible consequences:

• Exfiltration. Remote files the wrap was hiding (a filt_iter filter, or a prefix restricting the view to one subdirectory) are copied down into the local target anyway.
• Local deletion. With delete_local_files_not_in_remote=True (:810) or delete_mode='recycle' (:806), rsync --delete prunes the local target against the full remote tree rather than the view. Under a prefix-shifted view this is at its worst: every previously synced local file sits at a relative path that does not exist in the unshifted source, so rsync deletes all of them and re-lays the tree one directory deeper. Deletion is permanent unless delete_mode='recycle' was used.

3. sync_to writes to the wrong local directory when the target is a wrapped store — and --delete then runs over that whole directory.
sshdol/base.py:761-767 accepts any object exposing a string rootdir that exists on disk, and the README names dol.Files as the intended target. dol.Files is a mk_relative_path_store subclass, so .rootdir resolves through Route A to the leaf. For a bare dol.Files(root) the leaf root and the view root coincide and everything is fine. Add a key codec and they diverge: KeyCodecs.prefixed('mirror/')(…dol.Files('/local/root')) reports .rootdir == '/local/root'. The rsync destination becomes /local/root/, and with delete_local_files_not_in_remote=True rsync --delete permanently deletes every file anywhere under /local/root that is not present on the remote — including files in sibling directories the user's narrowed target view could never even address.

Repro — SYNTHETIC

An SSH server and paramiko are not needed to show the defect, so the transport is stubbed with a local directory and the rsync argv is captured rather than executed. FakeSshFiles copies the shape of sshdol/base.py verbatim — same rootdir + normalize_path scoping, same mkdir body ending in return self[path], same sync_to source/target construction. Every wrapper below is real dol.

"""SYNTHETIC repro of the sshdol key-delegation defect. Needs only `dol`."""
import os, shutil, tempfile
from collections.abc import MutableMapping
import dol
from dol import KeyCodecs, filt_iter


def normalize_path(path):  # verbatim, sshdol/base.py:68
    if not path or path == ".":
        return ""
    path = path[:-1] if path.endswith("/") else path
    return path.replace("\\", "/")


class FakeSshFiles(MutableMapping):
    def __init__(self, rootdir=".", *, user="u", host="h"):
        self.rootdir, self._conn_user, self._conn_host = rootdir, user, host

    def _abs(self, k):
        return os.path.join(self.rootdir, normalize_path(k))

    # dunders: mediated by the wrapper -- these are correct
    def __getitem__(self, k):
        p = self._abs(k)
        return type(self)(p) if os.path.isdir(p) else open(p, "rb").read()

    def __setitem__(self, k, v):
        os.makedirs(os.path.dirname(self._abs(k)), exist_ok=True)
        open(self._abs(k), "wb").write(v)

    def __delitem__(self, k):
        os.remove(self._abs(k))

    def __iter__(self):
        for d, _, fs in os.walk(self.rootdir):
            for f in fs:
                yield os.path.relpath(os.path.join(d, f), self.rootdir)

    def __len__(self):
        return sum(1 for _ in self)

    # non-dunders: NOT mediated -- handed the OUTER key / the leaf's rootdir
    def mkdir(self, path, exist_ok=False):            # sshdol/base.py:684
        path = normalize_path(path)
        os.makedirs(os.path.join(self.rootdir, path), exist_ok=exist_ok)
        return self[path]                             # sshdol/base.py:720

    def sync_to(self, target, *, delete_local_files_not_in_remote=False):
        if not isinstance(target, str) and hasattr(target, "rootdir"):
            target = target.rootdir                   # sshdol/base.py:767
        os.makedirs(target, exist_ok=True)
        cmd = ["rsync", "-a", "-z"] + (["--delete"] if delete_local_files_not_in_remote else [])
        return cmd + [f"{self._conn_user}@{self._conn_host}:{self.rootdir.rstrip('/')}/",  # :824
                      os.path.join(target, "")]


def subdir_view(store, prefix):
    """The natural 'restrict this store to a subdirectory' wrap."""
    return KeyCodecs.prefixed(prefix)(filt_iter(store, filt=lambda k: k.startswith(prefix)))


tmp = tempfile.mkdtemp()
remote = os.path.join(tmp, "remote_root")
for rel in ["sub/a.txt", "sub/b.txt", "private/keys.pem"]:
    os.makedirs(os.path.dirname(os.path.join(remote, rel)), exist_ok=True)
    open(os.path.join(remote, rel), "wb").write(b"x")

# --- 1. mkdir: key-like arg #1 gets the outer key -------------------------
w = subdir_view(FakeSshFiles(remote), "sub/")
print("view keys           :", sorted(w))
print("w['a.txt']  (dunder):", w["a.txt"], "  <- correct")
made = w.mkdir("newdir")
print("w.mkdir('newdir') -> ", os.path.relpath(made.rootdir, remote), "  EXPECTED sub/newdir")

# --- 1b. same via CLASS wrap (Route B) ------------------------------------
WCls = KeyCodecs.prefixed("sub/")(FakeSshFiles)
print("class-wrap mkdir  -> ", os.path.relpath(WCls(remote).mkdir("newdir2").rootdir, remote),
      "  EXPECTED sub/newdir2")

# --- 2. sync_to source: scope-wide, no key arg to map at all --------------
cmd = w.sync_to(os.path.join(tmp, "dst"), delete_local_files_not_in_remote=True)
print("rsync SOURCE        :", cmd[-2])
print("EXPECTED            :", remote + "/sub/   <- private/keys.pem is outside the view")

# --- 3. sync_to target: `.rootdir` read off a wrapped dol.Files -----------
localroot = os.path.join(tmp, "local_root")
os.makedirs(f"{localroot}/onlyhere", exist_ok=True)
os.makedirs(f"{localroot}/mirror", exist_ok=True)
open(f"{localroot}/onlyhere/precious.txt", "wb").write(b"keep me")
tgt = subdir_view(dol.Files(localroot), "mirror/")
cmd = FakeSshFiles(remote).sync_to(tgt, delete_local_files_not_in_remote=True)
print("rsync DEST          :", cmd[-1])
print("EXPECTED            :", os.path.join(localroot, "mirror", ""),
      " <- --delete here wipes local_root/onlyhere/precious.txt")

shutil.rmtree(tmp)

Output (temp dir elided as <tmp>):

view keys           : ['a.txt', 'b.txt']
w['a.txt']  (dunder): b'x'   <- correct
w.mkdir('newdir') ->  newdir   EXPECTED sub/newdir
class-wrap mkdir  ->  newdir2   EXPECTED sub/newdir2
rsync SOURCE        : u@h:<tmp>/remote_root/
EXPECTED            : <tmp>/remote_root/sub/   <- private/keys.pem is outside the view
rsync DEST          : <tmp>/local_root/
EXPECTED            : <tmp>/local_root/mirror/  <- --delete here wipes local_root/onlyhere/precious.txt

Note line 2 against line 3: the dunder path is correct, the method path is not. That is the whole bug.

Remediation

mkdir — map the key at the boundary

Resolve the caller's key through the full wrapper chain before touching the remote:

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

def mkdir(self, path, exist_ok=False):
    inner = inner_most_key(wrapped_self(self), path)
    path = normalize_path(inner if isinstance(inner, str) else path)
    ...

Two traps, both mandatory to respect:

1. inner_most_key walks the entire chain, including the leaf's own _id_of_key. It therefore replaces self._id_of_key(k) — never compose the two, or the key gets transformed twice. (sshdol has no _id_of_key today, so this mainly matters if one is ever added.)
2. It returns None — silently — when no layer in the chain defines _id_of_key, which is the common unwrapped case. The isinstance(inner, str) guard above is not optional.

The second half of mkdir needs attention too: return self[path] (sshdol/base.py:720) hands back a bare leaf SshFiles, so the caller loses the wrap on the way into the new directory. Either return None/the path, or re-apply the caller's wrap.

sync_to — no key to map; shrink the surface instead

sync_to has no key-like argument, so inner_most_key has nothing to work with. The real fix is to stop deriving scope from self.rootdir behind the caller's back:

• Preferred — move it off the store. Make the rsync path an explicit handle constructed with a directory, the way azuredol's BlobHandle does: RemoteDirSync(conn, remote_dir).to_local(local_dir, ...). A free function sync(src_dir, dst_dir, *, ...) taking both endpoints explicitly is unambiguous, cannot be scope-drifted by a wrapper, and is trivially testable.
• If it must stay a method, add an explicit subdir=None (or remote_dir=None) keyword so the caller can state the scope, and resolve the destination from an explicit path rather than sniffing target.rootdir. Better still, refuse to guess: require a str/Path destination and let callers pass store.rootdir themselves, so the leaf-vs-view divergence becomes the caller's visible choice rather than a hidden one.
• Minimum viable safety today, if the API is frozen: detect that self is wrapped (wrapped_self(self) is not self, or an _id_of_key/key filter present anywhere in the chain) and refuse to run — especially with --delete — rather than silently syncing the wrong scope. A loud RuntimeError is far cheaper than deleting a user's local tree.
• Document rootdir. The README's target contract ("an object with a .rootdir") is the delegated-attribute trap written down as prose. It should say that .rootdir reports the leaf's directory and is only meaningful for an unwrapped store.

Tests worth adding

sshdol/tests/test_base.py currently exercises sync_to only on a bare SshFiles (test_sync_to). Add cases that wrap the store in KeyCodecs.prefixed(...) / filt_iter(...) and assert on the constructed rsync argv — factor the argv construction out of sync_to into a pure helper so it can be asserted without a network round-trip — and on the directory mkdir actually created.

---

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.