From c9612cac428b8763586ebf65ce75b699db8abc59 Mon Sep 17 00:00:00 2001 From: Julian lechner Date: Thu, 24 Sep 2026 16:31:32 +0200 Subject: [PATCH] fix(retrieval): guard owned runtime files and links --- TODO.md | 2 +- scripts/lib/semantic-retrieval.py | 31 +++++++++++++++++++++++++++++-- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/TODO.md b/TODO.md index 1b58713..43ce916 100644 --- a/TODO.md +++ b/TODO.md @@ -4,7 +4,7 @@ - [x] Validate every path read from `.ai-config-manifest.tsv` and reject links before backup, overwrite, or removal. Both manifest writers join unchecked recorded paths to the destination; a `../` entry or linked target can reach files outside the managed directory. (`scripts/lib/manifest.ps1:20`, `scripts/lib/manifest.ps1:136`, `scripts/lib/manifest.sh:20`, `scripts/lib/manifest.sh:170`) - [x] Keep an installed semantic retrieval runtime when an `auto` benchmark says no. `benchmark()` deletes the whole runtime and ownership state even when `state['clients']` still contains active clients. (`scripts/lib/semantic-retrieval.py:117`, `scripts/lib/semantic-retrieval.py:142`) -- [ ] Check semantic retrieval ownership and links before copying or deleting its directory. An existing owned runtime's source files are overwritten without comparing recorded hashes; a replaced target symlink is followed on enable, and disabling the last client removes foreign files inside the runtime. (`scripts/lib/semantic-retrieval.py:82`, `scripts/lib/semantic-retrieval.py:100`) +- [x] Check semantic retrieval ownership and links before copying or deleting its directory. An existing owned runtime's source files are overwritten without comparing recorded hashes; a replaced target symlink is followed on enable, and disabling the last client removes foreign files inside the runtime. (`scripts/lib/semantic-retrieval.py:82`, `scripts/lib/semantic-retrieval.py:100`) - [ ] Preserve pre-existing MCPorter server entries. `ensure_mcporter()` adds a server and claims ownership without checking existing configuration; if the add succeeds for an existing name, a later removal deletes that user's entry. (`scripts/lib/managed-extensions.py:395`, `scripts/lib/managed-extensions.py:263`) - [ ] Preserve pre-existing global CLI packages. `ensure_cli()` and `ensure_qmd()` install and claim npm packages without checking whether they were already installed; later removal can uninstall a user's package. (`scripts/lib/managed-extensions.py:342`, `scripts/lib/managed-extensions.py:378`) - [x] Fetch skill archives by an immutable commit and record that same commit. The archive uses GitHub `HEAD`, while `resolvedRevision` comes from a separate later request, so the recorded revision may not identify the installed payload. (`scripts/lib/managed-extensions.py:94`, `scripts/lib/managed-extensions.py:140`, `scripts/lib/managed-extensions.py:409`) diff --git a/scripts/lib/semantic-retrieval.py b/scripts/lib/semantic-retrieval.py index 76a35f2..5bd30e0 100644 --- a/scripts/lib/semantic-retrieval.py +++ b/scripts/lib/semantic-retrieval.py @@ -78,6 +78,23 @@ def save_state(path, state, dry_run): temporary.unlink(missing_ok=True) +def check_runtime(target, state): + if target.is_symlink(): + raise ValueError('Semantic retrieval directory must not be a symbolic link.') + if not target.exists(): + return + if not target.is_dir(): + raise ValueError('Semantic retrieval target is not a directory.') + for name, expected in state['files'].items(): + path = target / name + if path.is_symlink() or not path.is_file() or digest(path) != expected: + raise ValueError(f'Owned semantic retrieval file changed: {name}') + allowed = set(state['files']) | {'.venv', 'model-cache'} + for path in target.iterdir(): + if path.is_symlink() or path.name not in allowed: + raise ValueError(f'Foreign or linked semantic retrieval path: {path.name}') + + def sync(root, home, clients, enabled, dry_run, update, summary=False): base = home / '.my-ai-configuration' target = base / 'semantic-retrieval' @@ -96,13 +113,16 @@ def sync(root, home, clients, enabled, dry_run, update, summary=False): if state['clients']: save_state(state_path, state, False) return - if target.is_symlink(): - raise ValueError('Semantic retrieval directory must not be a symbolic link.') + if not state['files']: + state_path.unlink(missing_ok=True) + return + check_runtime(target, state) if target.exists() and target.is_dir(): shutil.rmtree(target) state_path.unlink(missing_ok=True) return + check_runtime(target, state) if target.exists() and not state['files']: raise ValueError('Semantic retrieval directory exists without setup ownership; refusing to overwrite it.') if dry_run: @@ -136,9 +156,15 @@ def benchmark(root, home, dry_run, summary=False): return True state = load_state(state_path) source = root / 'shared' / 'retrieval' + if state['files']: + check_runtime(target, state) + elif target.is_symlink(): + raise ValueError('Semantic retrieval directory must not be a symbolic link.') if target.exists() and not state['files']: if not all((target / name).is_file() and digest(target / name) == digest(source / name) for name in FILES): raise ValueError('Semantic retrieval directory exists without setup ownership; refusing to overwrite it.') + state['files'] = {name: digest(target / name) for name in FILES} + check_runtime(target, state) target.mkdir(parents=True, exist_ok=True) for name in FILES: shutil.copy2(source / name, target / name) @@ -152,6 +178,7 @@ def benchmark(root, home, dry_run, summary=False): if outcome['recommended'] or state['clients']: save_state(state_path, state, False) else: + check_runtime(target, state) shutil.rmtree(target) state_path.unlink(missing_ok=True) if not summary: