Key-transform wrappers hand GDReader.get_url and GDStore.upload the unmapped key (latent; upload is destructive) #4

Summary

GDReader.get_url and GDStore.upload take a key, but they resolve it against the leaf
store's own namespace. If a user puts a dol key transform on top of a GDReader/GDStore
(KeyCodecs.prefixed, wrap_kvs(id_of_key=...), mk_relative_path_store, …), the dunder
protocol (__getitem__/__setitem__/__delitem__/__contains__/__iter__) is mapped
correctly but these two methods receive the outer, unmapped key. upload then performs a
destructive in-place write against the wrong Drive file, and get_url returns a link to the
wrong file and grants anyone/reader on it.

Verdict: latent, not live. pydrivedol ships no dol wrappers of its own (see
"How pydrivedol scopes today" below) — it does not even depend on dol. The defect is only
reachable when a user composes a key codec over these stores, which is precisely the use the
README advertises ("follows the design patterns of the dol package"). Filing so the hazard is
recorded before someone hits it against a real Drive.

Part of the family-wide audit: i2mint/dol#83 (umbrella), root cause i2mint/dol#18.

Mechanism

dol wraps by delegation (has-a), not inheritance. The wrapper holds the leaf in
self.store and maps keys in the dunder methods only. Every other non-dunder attribute reaches
the leaf bound to the leaf, via one of two routes in dol/base.py:

• 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 descriptor for every
  name in dir(wrapped); its __get__ also returns getattr(instance.store, attr).

Either way the method runs with self = the leaf and the key = the outer key. Both routes were
verified here; under Route B type(w).__dict__['get_url'] is a DelegatedAttribute and the
outcome is identical.

Affected symbols

__getitem__, __setitem__, __delitem__, __contains__, __iter__, __len__ are all
correct — the wrapper maps those. _refresh_cache, folder_url, folder_id, convert_office
take no key and are unaffected. The module-level functions (get_bytes,
xlsx_to_google_sheet, drive_from_service_account) are not methods and are not affected.

How pydrivedol scopes today

Scoping is entirely constructor-side, with no dol machinery involved:

• GDReader.__init__ (pydrivedol/base.py:316) takes folder_url and stores
  self.folder_id = _extract_folder_id(folder_url).
• Keys are folder-relative paths, materialized by _list_files into the _files dict
  (path -> Drive file id) at pydrivedol/base.py:379.
• GDReader(Mapping) / GDStore(GDReader, MutableMapping) are plain collections.abc
  classes — not dol.KvReader/KvPersister/Store subclasses.
• There is no _id_of_key anywhere in the package.

Grep over the package for mk_relative_path_store, KeyCodecs, prefixless_view, filt_iter,
wrap_kvs, PrefixRelativizationMixin, Store, KvReader, kv_wrap, Pipe: 0 hits in
code (only prose mentions of dol in the README). pyproject.toml dependencies are
pydrive2 and requests only.

So the scoping style is: prefix fixed at construction, key→id resolved through a private
_files dict; no key-transform layer of its own.

Reproduction

Runnable as-is. It exercises the real pydrivedol code — only the Drive backend is faked,
via the package's own drive= injection point, so no pydrive2 and no network are needed.
The folder layout (a file b at the root and a file b inside subfolder a) is ordinary.

"""Real pydrivedol code + a fake in-memory Drive backend (pydrive2 not needed)."""
import re
import pydrivedol.base as B
from dol import KeyCodecs

B._PYDRIVE2_AVAILABLE = True  # we inject drive=, so skip the pydrive2 guard
FOLDER = "https://drive.google.com/drive/folders/ROOT"
GFOLDER = "application/vnd.google-apps.folder"


class FakeFile(dict):
    def __init__(self, drive, **kw):
        super().__init__(**kw)
        self._drive = drive

    def __getitem__(self, k):
        if k == "alternateLink":
            return f"link-to:{self['id']}"
        return super().__getitem__(k)

    def GetContentString(self, mimetype=None):
        return self._drive.files[self["id"]]["content"].decode("latin-1")

    def SetContentString(self, s):
        self["content"] = s.encode("latin-1")

    def SetContentFile(self, path):
        self["content"] = open(path, "rb").read()

    def InsertPermission(self, perm):
        self._drive.permission_log.append((self["id"], perm))

    def Upload(self, param=None):
        self._drive.files[self["id"]] = self

    def Delete(self):
        self._drive.files.pop(self["id"], None)


class FakeDrive:
    def __init__(self):
        self.files, self._n, self.permission_log = {}, 0, []

    def add(self, id, title, parent, mimeType="text/plain", content=b""):
        self.files[id] = FakeFile(self, id=id, title=title, mimeType=mimeType,
                                  parents=[{"id": parent}], content=content)

    def ListFile(self, q):
        query, out = q["q"], []
        parent = re.search(r"'([^']+)' in parents", query).group(1)
        title = re.search(r"title='([^']+)'", query)
        mime = re.search(r"mimeType='([^']+)'", query)
        for f in self.files.values():
            if not any(p["id"] == parent for p in f["parents"]):
                continue
            if title and f["title"] != title.group(1):
                continue
            if mime and f["mimeType"] != mime.group(1):
                continue
            out.append(f)
        return type("L", (), {"GetList": lambda s: out})()

    def CreateFile(self, meta):
        if meta.get("id") in self.files:
            return self.files[meta["id"]]
        self._n += 1
        return FakeFile(self, id=meta.get("id") or f"NEW{self._n}",
                        title=meta.get("title", ""), parents=meta.get("parents", []),
                        mimeType=meta.get("mimeType", "text/plain"), content=b"")


def fresh():
    """Drive folder holding BOTH 'b' and 'a/b' -- an ordinary layout."""
    d = FakeDrive()
    d.add("F_ROOT_B", "b", "ROOT", content=b"IMPORTANT ROOT DATA")
    d.add("F_A", "a", "ROOT", mimeType=GFOLDER)
    d.add("F_A_B", "b", "F_A", content=b"inner data")
    return d


def state(d):
    return {f["id"]: f["content"] for f in d.files.values() if f["mimeType"] != GFOLDER}


d = fresh()
w = KeyCodecs.prefixed("a/")(B.GDStore(FOLDER, drive=d))   # user-applied key codec
print("leaf keys        ", sorted(w.store))
print("w['b']           ", w["b"])

# 1. get_url -> wrong file, and grants anyone/reader on it
print("w.get_url('b')   ", w.get_url("b"))
print("permissions      ", d.permission_log)

# 2. upload -> destroys the wrong file's content
d = fresh()
w = KeyCodecs.prefixed("a/")(B.GDStore(FOLDER, drive=d))
print("before upload    ", state(d))
w.upload("b", b"OVERWRITTEN")
print("after  upload    ", state(d))
print("w['b'] still     ", w["b"])

# contrast: __setitem__ is mapped correctly
d = fresh()
w = KeyCodecs.prefixed("a/")(B.GDStore(FOLDER, drive=d))
w["b"] = b"via setitem"
print("after  setitem   ", state(d))

Output:

leaf keys         ['a/b', 'b']
w['b']            b'inner data'                       <- correct, reads a/b
w.get_url('b')    link-to:F_ROOT_B                    <- WRONG file (wanted F_A_B)
permissions       [('F_ROOT_B', {'type': 'anyone', 'value': 'anyone', 'role': 'reader'})]
before upload     {'F_ROOT_B': b'IMPORTANT ROOT DATA', 'F_A_B': b'inner data'}
after  upload     {'F_ROOT_B': b'OVERWRITTEN', 'F_A_B': b'inner data'}   <- wrong file destroyed
w['b'] still      b'inner data'                       <- the write vanished from the store
after  setitem    {'F_ROOT_B': b'IMPORTANT ROOT DATA', 'F_A_B': b'via setitem'}   <- correct

User-visible consequence

upload — data destruction. upload never checks whether the key exists. It resolves the
unmapped key to parent_id = self._get_or_create_folders(key) + filename, queries that
parent for a file with the same title, and if one is found passes its file_id to
_upload_converting, which calls gfile.Upload() on that existing id. That is an in-place
content replacement of an unrelated Drive file: the file keeps its id, so every existing
share link, embed, and permission grant continues to resolve — and now serves the caller's
bytes. The previous content is gone (recoverable only from Drive version history, if the
account has it). Meanwhile the key the caller actually asked for is never written, and upload
returns a URL, so the write looks successful. With no name collision the damage is milder
but still silent: a stray file plus stray intermediate folders are created at the wrong level
and the intended key stays unwritten.

get_url — wrong link plus an unintended public grant. get_url does check
key not in self._files, so with no collision it raises KeyError: File not found: <key> —
loud and safe. On collision it silently returns the other file's alternateLink and first
calls InsertPermission({'type': 'anyone', 'role': 'reader'}) on it, i.e. it makes a file the
caller never named readable by anyone with the link. The caller then hands that link out
believing it points at their own object.

Related, not part of this bug class — worth noting because it sets the blast radius above:
get_url is a method on a read-only Mapping that unconditionally mutates sharing state, and
its default is the most permissive one (permission_type='anyone', permission_role='reader').
There is no way to just read a file's link without granting. Even used correctly that is
surprising; combined with the wrong-key resolution above it turns a naming coincidence into a
data exposure. Consider splitting into a non-mutating url_for(key) and an explicit
share(key, ...), and defaulting permission_type to something non-public.

Suggested remediation

Preferred — shrink the surface. Both methods are per-object operations wearing a
per-collection costume. Move them onto a handle bound to a key at construction time (the shape
azuredol uses for BlobHandle), so there is no loose key argument to get out of sync:

store.handle('a/b').url(...)     # instead of store.get_url('a/b')
store.handle('a/b').upload(...)  # instead of store.upload('a/b', ...)

The handle is obtained through the store's normal key path, so any wrapper maps the key exactly
once, and the per-write options (path=, convert=, google_mimetype=) live on the handle
where they belong. Note convert_office=True at construction already covers the common
convert= case through plain __setitem__, so the residual need for a loose upload is small.

Minimal fix if the current signatures must stay — resolve the incoming key through the
outer store before touching Drive:

from dol import wrapped_self
from dol.dig import inner_most_key   # NOT exported from dol's top level

def _leaf_key(self, k):
    inner = inner_most_key(wrapped_self(self), k)
    return inner if isinstance(inner, str) else k

Then start get_url, upload, and _get_or_create_folders with key = self._leaf_key(key).

Two traps, both live for this package:

1. inner_most_key walks the whole chain including the leaf's own _id_of_key. It
   replaces any self._id_of_key(k) call — never compose the two, or the key is
   transformed twice. pydrivedol has no _id_of_key today, so there is nothing to compose with
   yet; that changes the moment someone makes these classes dol.Store subclasses.
2. It returns None — silently, no exception — when no layer in the chain defines
   _id_of_key. That is the unwrapped case for pydrivedol, i.e. the normal one:
   inner_most_key(wrapped_self(GDStore(...)), 'b') returns None today (verified). The
   isinstance(inner, str) guard above is mandatory, not defensive styling.

Independently of which fix is chosen, upload should validate the resolved key the way
get_url and __delitem__ do, rather than blind-writing to whatever the path resolves to.

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.