Skip to content

Commit 33c6be5

Browse files
authored
Fix callable save_to dispatch (UnboundLocalError) and stop leaking secrets in KeyError (#18)
* Fix save_to callable dispatch and stop leaking secrets in KeyError Two same-file fixes in base.py. 1. `ask_user_for_key(save_to=<callable>)` raised UnboundLocalError. `SaveTo = Optional[Union[MutableMapping, KTSaver]]` advertises callable savers, and `user_gettable(save_to=...)` is re-exported from `__init__`, but the save block bound `save_to_func` only inside `if hasattr(save_to, '__setitem__')` with no `else` -- so a callable saver blew up with UnboundLocalError, and inside `user_gettable` that UnboundLocalError was swallowed into a KeyError by FuncBasedGettableContainer, silently dropping the value. Replaced with an explicit `_resolve_saver` dispatch (the single place the SaveTo union is interpreted): `__setitem__` when present, else the callable itself, else a TypeError naming the offending type. Resolution is idempotent and happens up front, so an unusable save_to is reported when it is specified rather than after the user has already typed a value we then cannot save. Behaviour is unchanged for existing callers: the mapping branch still dispatches on `hasattr(.., '__setitem__')` rather than `isinstance(.., MutableMapping)`, so write-only stores keep working. 2. FuncBasedGettableContainer.__getitem__ leaked credentials. The KeyError message interpolated both the rejected value `v` and the upstream exception text `e`. `v` is often a credential (that is why it is being validated) and SDK exception text routinely echoes the rejected token. These KeyErrors propagate through ChainMap lookups and are typically logged, sending the secret to log aggregators. Now `raise KeyError(k) from e` (and `raise KeyError(k)` for the invalid value case): the message is just the key, `args[0]` stays the key per the Mapping convention, and the upstream exception remains reachable via `__cause__`. Tests: new config2py/tests/test_base.py covers dict saver, __setitem__ only store, callable saver, the TypeError branch, save_condition, both user_gettable paths, and asserts the secret appears in neither `str()` nor `repr()` of the raised KeyError. Eight of them failed before this change. Two doctests in the FuncBasedGettableContainer docstring asserted the old leaky messages and were updated. Closes #14 Claude-Session: https://claude.ai/code/session_01Kug7UUbVeCQgruvNXUq63c * fix: make app-folder resolution and its tests cross-platform Fixes the four Windows CI failures (all pre-existing: this PR had only touched base.py and tests/test_base.py) and one real Windows bug found while investigating. Code fix -- Windows 'cache' collided with 'data'/'state': APP_FOLDER_STANDARDS described the Windows cache root as FolderSpec("LOCALAPPDATA", join(%LOCALAPPDATA%, "Temp")). Since the fallback is only used when the env var is *absent*, and LOCALAPPDATA is always set on Windows, the "Temp" default was dead code: cache resolved to %LOCALAPPDATA%, exactly the data/state root. Clearing an app's cache would have deleted the user's data. FolderSpec gains a `subpath` field that expresses "a kind that lives inside another kind's root", so cache is now the documented %LOCALAPPDATA%\Temp. The table also snapshotted os.getenv() into its own fallbacks at import time, which made those fallbacks untestable and unreachable. They are now declarative literals (~\AppData\Roaming etc.), so a missing env var yields a real absolute root instead of "" -- an empty root would have degraded os.path.join(root, app_name) into a *relative* path. The single os.name branch is now app_folder_standards(os_name), so either platform's table can be resolved from any platform, and system_default_for_app_data_folder takes it via an optional keyword. That makes Windows behaviour testable on Linux/macOS -- see the new tests/test_platform_standards.py, which loops over both platforms. Test fixes -- assertions that were POSIX-shaped, not wrong behaviour: - test_app_data.py steered app folders with XDG_CONFIG_HOME/XDG_DATA_HOME. XDG is a POSIX standard and is correctly ignored on Windows, so the redirection silently did nothing there: two tests failed on the location assertion and five others passed while writing into the runner's real user profile. They now use config2py's own CONFIG2PY_<KIND>_DIR override, which is honoured on every platform, and compare resolved Path objects rather than strings. Location is now asserted in 5 tests, up from 2. - test_sync_store.py::test_store_repr asserted a separator literal (`temp_file in repr(store)`); pathlib's repr always spells paths with forward slashes, so the native Windows form never appears. It now asserts the store's filepath identity plus the separator-free filename. Doctest fix: get_app_folder's doctest expected '.../.config/config2py'. Rather than skipping it, it now asserts the properties that hold on every platform: the result is absolute, is named after the app, and sits directly inside the 'config' root. That is strictly more coverage than the ELLIPSIS form. Also corrected get_app_rootdir's docstring, which advertised the override variables under wrong names (CONFIG2PY_*_FOLDER instead of CONFIG2PY_*_DIR) and implied XDG works everywhere. pytest config: testpaths named "tests", which does not exist (tests live in config2py/tests/), so pytest warned and fell back to recursive discovery from the CWD. Set to ["config2py"]. doctest_optionflags now match what the wads run-tests-uv action passes on the command line (ELLIPSIS, IGNORE_EXCEPTION_DETAIL) -- it overrides the file, so NORMALIZE_WHITESPACE here meant local runs and CI applied different rules. Collected tests: 106 -> 127 (+20 platform tests, +1 doctest), no warnings. Dependents gate (29 suites) unchanged before/after: 25 pass, 2 pre-existing failures (smart-cv, yp), 1 no-tests, 1 tests-disabled. Claude-Session: https://claude.ai/code/session_01Kug7UUbVeCQgruvNXUq63c * test: pin the documented app-folder roots for both platforms Restores coverage that the previous commit dropped. Making the `get_app_folder` doctest platform-agnostic was right, but the form it replaced ('.../.config/config2py') was the only assertion in the suite that pinned a *literal* XDG root; the properties that replaced it (absolute, basenamed after the app, dirname == get_app_rootdir('config')) all hold by construction of `os.path.join(root, app_name)`, so they no longer notice if the root itself moves. The rest of test_platform_standards.py is structural -- non-empty, no cache/data collision, right variable family -- and a silently retargeted default satisfies all of it. DOCUMENTED_STANDARDS now spells out both tables exactly as get_app_rootdir's docstring advertises them, so relocating a user's config/data/cache/state/runtime folder has to be a deliberate edit in two places rather than a side effect of an unrelated change. Verified non-tautological: repointing the POSIX 'config' default at '~/.conf' turns it red. Collected tests: 127 -> 129. Claude-Session: https://claude.ai/code/session_01Kug7UUbVeCQgruvNXUq63c
1 parent 3bd4b26 commit 33c6be5

7 files changed

Lines changed: 601 additions & 56 deletions

File tree

config2py/base.py

Lines changed: 143 additions & 15 deletions
Original file line numberDiff line numberDiff line change
@@ -246,10 +246,21 @@ class FuncBasedGettableContainer:
246246
``FuncBasedGettableContainer`` raises a ``KeyError``, to conform to the
247247
``Mapping`` protocol.
248248
249-
>>> gc['no_a_key'] # doctest: +ELLIPSIS +IGNORE_EXCEPTION_DETAIL
249+
>>> gc['no_a_key']
250250
Traceback (most recent call last):
251251
...
252-
KeyError: 'There was an exception ... : "I don\'t handle that: no_a_key"'
252+
KeyError: 'no_a_key'
253+
254+
The ``KeyError`` message is just the key: neither the upstream exception text nor
255+
the rejected value is interpolated into it, since getters commonly wrap credential
256+
checks and these errors commonly end up in logs. The upstream exception is still
257+
available, through the standard exception chain:
258+
259+
>>> try:
260+
... gc['no_a_key']
261+
... except KeyError as e:
262+
... print(type(e.__cause__).__name__, e.__cause__, sep=': ')
263+
RuntimeError: I don't handle that: no_a_key
253264
254265
Note that by default, ``FuncBasedGettableContainer`` will catch all ``Exception``
255266
exceptions, but you can specify a different set of exceptions to catch.
@@ -274,7 +285,7 @@ class FuncBasedGettableContainer:
274285
>>> gc['no_a_key']
275286
Traceback (most recent call last):
276287
...
277-
KeyError: 'Value for key no_a_key is not valid: None'
288+
KeyError: 'no_a_key'
278289
279290
"""
280291

@@ -296,12 +307,17 @@ def __getitem__(self, k: KT) -> VT:
296307
try:
297308
v = self.getter(k)
298309
except self.config_not_found_exceptions as e:
299-
raise KeyError(
300-
f"There was an exception when computing key: {k} with the function "
301-
f"{self.getter}. The exception was: {e}"
302-
)
310+
# Note: The upstream exception text is deliberately NOT interpolated into
311+
# the message. Getters here often wrap credential validation, and SDKs
312+
# routinely echo the rejected secret back in their exception text
313+
# ("authentication failed for token sk-..."). These KeyErrors are caught
314+
# and logged by callers, so anything in the message ends up in logs.
315+
# The upstream exception remains reachable via ``__cause__``.
316+
raise KeyError(k) from e
303317
if not self.val_is_valid(v):
304-
raise KeyError(f"Value for key {k} is not valid: {v}")
318+
# Same reasoning: ``v`` is the *rejected* value, which is precisely the
319+
# thing that is often a secret (that's why it's being validated).
320+
raise KeyError(k)
305321
return v
306322

307323
# TODO: Is this used to indicate that the getter couldn't find a key.
@@ -358,6 +374,66 @@ def sources_chainmap(
358374
SaveTo = Optional[Union[MutableMapping, KTSaver]]
359375

360376

377+
def _resolve_saver(save_to: SaveTo) -> Optional[KTSaver]:
378+
"""Resolve a ``SaveTo`` specification into a ``(key, value)`` saver function.
379+
380+
This is the single place where the ``SaveTo`` union is interpreted, so every
381+
function that accepts a ``save_to`` agrees on what it means.
382+
383+
``None`` means "don't save", and is passed through as such:
384+
385+
>>> _resolve_saver(None) is None
386+
True
387+
388+
Anything with a ``__setitem__`` (any ``MutableMapping``, but also the write-only
389+
stores that ``dol`` makes) saves through that ``__setitem__``:
390+
391+
>>> d = {}
392+
>>> save = _resolve_saver(d)
393+
>>> save('some_key', 'some_value')
394+
>>> d
395+
{'some_key': 'some_value'}
396+
397+
A callable is used as the saver itself:
398+
399+
>>> saved = []
400+
>>> save = _resolve_saver(lambda k, v: saved.append((k, v)))
401+
>>> save('some_key', 'some_value')
402+
>>> saved
403+
[('some_key', 'some_value')]
404+
405+
Resolution is idempotent, so an already-resolved saver can be passed around
406+
(and re-resolved) freely:
407+
408+
>>> _resolve_saver(save) is save
409+
True
410+
411+
Anything else is an error, named as such instead of failing obscurely (or
412+
silently dropping the value) at save time:
413+
414+
>>> _resolve_saver(42) # doctest: +ELLIPSIS
415+
Traceback (most recent call last):
416+
...
417+
TypeError: save_to must be None, a MutableMapping ... Got type: int
418+
419+
"""
420+
if save_to is None:
421+
return None
422+
elif hasattr(save_to, "__setitem__"):
423+
# Note: We test for ``__setitem__`` rather than ``isinstance(.., MutableMapping)``
424+
# so that write-only stores (which don't implement the full MutableMapping
425+
# interface) keep working. Mappings win over callables when an object is both.
426+
return save_to.__setitem__
427+
elif callable(save_to):
428+
return save_to
429+
else:
430+
raise TypeError(
431+
"save_to must be None, a MutableMapping (or anything with a __setitem__), "
432+
"or a callable taking (key, value). "
433+
f"Got type: {type(save_to).__name__}"
434+
)
435+
436+
361437
def is_not_empty(val) -> bool:
362438
if isinstance(val, str):
363439
return val != ""
@@ -374,22 +450,60 @@ def ask_user_for_key(
374450
user_asker=ask_user_for_input,
375451
egress: Callable | None = None,
376452
):
453+
"""Ask the user for the value of ``key``, optionally saving it.
454+
455+
:param key: The key to ask the user for. If ``None``, a "curried" version of
456+
``ask_user_for_key`` is returned, so you can specify the key later.
457+
:param prompt_template: A template string to prompt the user with. It should
458+
contain a placeholder for the key, e.g. ``"Enter a value for {}: "``.
459+
:param save_to: Where to save the user's response: a ``MutableMapping`` (or
460+
anything with a ``__setitem__``), or a ``(key, value)`` saver function.
461+
If ``None``, the response is not saved. See ``_resolve_saver``.
462+
:param save_condition: A function of the value, deciding whether to save it.
463+
:param user_asker: A function that takes a prompt string and returns the user's
464+
response.
465+
:param egress: A ``(key, value)`` function to apply to the user's response before
466+
returning (and saving) it.
467+
468+
The value can be saved to any ``MutableMapping``:
469+
470+
>>> store = {}
471+
>>> ask_user_for_key('some_key', save_to=store, user_asker=lambda prompt: 'val')
472+
'val'
473+
>>> store
474+
{'some_key': 'val'}
475+
476+
... or to a ``(key, value)`` function, when saving isn't a simple write:
477+
478+
>>> saved = []
479+
>>> ask_user_for_key(
480+
... 'some_key',
481+
... save_to=lambda k, v: saved.append((k, v)),
482+
... user_asker=lambda prompt: 'val',
483+
... )
484+
'val'
485+
>>> saved
486+
[('some_key', 'val')]
487+
488+
"""
489+
# Note: We resolve ``save_to`` up front (and carry the resolved saver into the
490+
# curried form) so that an unusable ``save_to`` is reported when it's specified,
491+
# not after the user has already typed a value we then can't save.
492+
saver = _resolve_saver(save_to)
377493
if key is None:
378494
return partial(
379495
ask_user_for_key,
380496
prompt_template=prompt_template,
381-
save_to=save_to,
497+
save_to=saver,
382498
save_condition=save_condition,
383499
user_asker=user_asker,
384500
egress=egress,
385501
)
386502
val = user_asker(prompt_template.format(key))
387503
if isinstance(egress, Callable):
388504
val = egress(key, val)
389-
if save_to is not None and save_condition(val):
390-
if hasattr(save_to, "__setitem__"):
391-
save_to_func = save_to.__setitem__
392-
save_to_func(key, val)
505+
if saver is not None and save_condition(val):
506+
saver(key, val)
393507
return val
394508

395509

@@ -405,8 +519,9 @@ def user_gettable(
405519
"""
406520
Create a ``GettableContainer`` that asks the user for a value, optionally saving it.
407521
408-
:param save_to: A ``MutableMapping`` to save the user's response to. If ``None``,
409-
the user's response is not saved.
522+
:param save_to: Where to save the user's response: a ``MutableMapping`` (or
523+
anything with a ``__setitem__``), or a ``(key, value)`` saver function.
524+
If ``None``, the user's response is not saved.
410525
:param prompt_template: A template string to prompt the user with. It should
411526
contain a placeholder for the key, e.g. ``"Enter a value for {}: "``.
412527
:param egress: A function to apply to the user's response before returning it.
@@ -440,6 +555,19 @@ def user_gettable(
440555
>>> d # doctest: +SKIP
441556
{'some': 'store', 'SOME_KEY': 'SOME_VAL'}
442557
558+
When saving isn't a simple write (say you need to encrypt, or write to two
559+
places), ``save_to`` can be a ``(key, value)`` function instead:
560+
561+
>>> saved = []
562+
>>> s = user_gettable(
563+
... save_to=lambda k, v: saved.append((k, v)),
564+
... user_asker=lambda prompt: 'SOME_VAL',
565+
... )
566+
>>> s['SOME_KEY']
567+
'SOME_VAL'
568+
>>> saved
569+
[('SOME_KEY', 'SOME_VAL')]
570+
443571
"""
444572
getter = ask_user_for_key(
445573
prompt_template=prompt_template,

config2py/tests/test_app_data.py

Lines changed: 45 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -7,18 +7,42 @@
77

88
import pytest
99

10-
from config2py.util import ensure_seeded, AppData
10+
from config2py.util import ensure_seeded, AppData, config2py_env_var
11+
12+
13+
def _redirect_app_root(folder_kind: str, target):
14+
"""Redirect a config2py app-root folder kind at *target* for the test.
15+
16+
Uses config2py's own ``CONFIG2PY_<KIND>_DIR`` override, which is honoured on
17+
every platform. The XDG_* variables must NOT be used here: they are a POSIX
18+
standard and are ignored on Windows, so tests keyed on them silently write
19+
into the real user profile instead of the temp dir.
20+
"""
21+
env_var = getattr(config2py_env_var, folder_kind)
22+
return patch.dict(os.environ, {env_var: str(target)})
23+
24+
25+
def _same_path(a, b) -> bool:
26+
"""Compare two paths by identity-on-disk, not by their string spelling.
27+
28+
``resolve()`` on both sides normalises the differences that make naive
29+
string comparison fail per-platform: macOS ``/var`` -> ``/private/var``
30+
symlinks, and Windows 8.3 short names (``RUNNER~1`` -> ``runneradmin``).
31+
"""
32+
return Path(a).resolve() == Path(b).resolve()
1133

1234

1335
# ---------------------------------------------------------------------------
1436
# Helpers — mock importlib.resources for isolated testing
1537
# ---------------------------------------------------------------------------
1638

39+
1740
def _mock_importlib_files(seed_store: dict):
1841
"""Return a mock for importlib.resources.files that reads from *seed_store*.
1942
2043
*seed_store* maps ``(subpackage, filename)`` to ``bytes`` content.
2144
"""
45+
2246
def fake_files(package_path: str):
2347
# Extract subpackage from e.g. "mypkg._seed_data.resources"
2448
parts = package_path.split(".")
@@ -36,6 +60,7 @@ def __truediv__(self, filename):
3660
return mock_ref
3761

3862
return FakeTraversable()
63+
3964
return fake_files
4065

4166

@@ -110,6 +135,7 @@ def test_returns_path_object(self, tmp_path, mock_seeds_for_ensure):
110135
def test_custom_seed_data_dir(self, tmp_path):
111136
"""Ensure the seed_data_dir parameter is used in the package path."""
112137
calls = []
138+
113139
def fake_files(pkg_path):
114140
calls.append(pkg_path)
115141
mock = MagicMock()
@@ -121,7 +147,10 @@ def fake_files(pkg_path):
121147
target = tmp_path / "file.txt"
122148
with patch("importlib.resources.files", side_effect=fake_files):
123149
ensure_seeded(
124-
target, "mypkg", "resources", "file.txt",
150+
target,
151+
"mypkg",
152+
"resources",
153+
"file.txt",
125154
seed_data_dir="my_seeds",
126155
)
127156
assert calls[0] == "mypkg.my_seeds.resources"
@@ -145,21 +174,22 @@ def test_custom_package_name(self):
145174
assert app.package_name == "my_app"
146175

147176
def test_app_folder_creates_directory(self, tmp_path):
148-
with patch.dict(os.environ, {"XDG_DATA_HOME": str(tmp_path)}):
177+
with _redirect_app_root("data", tmp_path):
149178
app = AppData("testapp")
150179
folder = app.app_folder(folder_kind="data")
151180
assert folder.is_dir()
152181
assert folder.name == "testapp"
182+
assert _same_path(folder, tmp_path / "testapp")
153183

154184
def test_app_folder_config(self, tmp_path):
155-
with patch.dict(os.environ, {"XDG_CONFIG_HOME": str(tmp_path)}):
185+
with _redirect_app_root("config", tmp_path):
156186
app = AppData("testapp")
157187
folder = app.app_folder(folder_kind="config")
158188
assert folder.is_dir()
159-
assert folder == tmp_path / "testapp"
189+
assert _same_path(folder, tmp_path / "testapp")
160190

161191
def test_get_resource_seeds_when_missing(self, tmp_path):
162-
with patch.dict(os.environ, {"XDG_DATA_HOME": str(tmp_path)}):
192+
with _redirect_app_root("data", tmp_path):
163193
with patch(
164194
"importlib.resources.files",
165195
side_effect=_mock_importlib_files(SEED_STORE),
@@ -168,10 +198,12 @@ def test_get_resource_seeds_when_missing(self, tmp_path):
168198
path = app.get_resource("hello.txt")
169199
assert path.exists()
170200
assert path.read_bytes() == b"hello world\nline two\n"
171-
assert "resources" in str(path)
201+
assert _same_path(
202+
path, tmp_path / "testapp" / "resources" / "hello.txt"
203+
)
172204

173205
def test_get_resource_preserves_user_edits(self, tmp_path):
174-
with patch.dict(os.environ, {"XDG_DATA_HOME": str(tmp_path)}):
206+
with _redirect_app_root("data", tmp_path):
175207
with patch(
176208
"importlib.resources.files",
177209
side_effect=_mock_importlib_files(SEED_STORE),
@@ -184,7 +216,7 @@ def test_get_resource_preserves_user_edits(self, tmp_path):
184216
assert path2.read_text() == "edited by user"
185217

186218
def test_get_config_seeds_when_missing(self, tmp_path):
187-
with patch.dict(os.environ, {"XDG_CONFIG_HOME": str(tmp_path)}):
219+
with _redirect_app_root("config", tmp_path):
188220
with patch(
189221
"importlib.resources.files",
190222
side_effect=_mock_importlib_files(SEED_STORE),
@@ -194,16 +226,17 @@ def test_get_config_seeds_when_missing(self, tmp_path):
194226
assert path.exists()
195227
data = json.loads(path.read_text())
196228
assert data["tempo"] == 120
229+
assert _same_path(path, tmp_path / "testapp" / "defaults.json")
197230

198231
def test_get_artifact_dir_creates_subdir(self, tmp_path):
199-
with patch.dict(os.environ, {"XDG_DATA_HOME": str(tmp_path)}):
232+
with _redirect_app_root("data", tmp_path):
200233
app = AppData("testapp")
201234
midi_dir = app.get_artifact_dir("midi")
202235
assert midi_dir.is_dir()
203-
assert midi_dir == tmp_path / "testapp" / "artifacts" / "midi"
236+
assert _same_path(midi_dir, tmp_path / "testapp" / "artifacts" / "midi")
204237

205238
def test_get_artifact_dir_multiple_kinds(self, tmp_path):
206-
with patch.dict(os.environ, {"XDG_DATA_HOME": str(tmp_path)}):
239+
with _redirect_app_root("data", tmp_path):
207240
app = AppData("testapp")
208241
for kind in ("midi", "audio", "exports"):
209242
d = app.get_artifact_dir(kind)

0 commit comments

Comments
 (0)