diff --git a/TODO.md b/TODO.md index 350931e..1b58713 100644 --- a/TODO.md +++ b/TODO.md @@ -2,7 +2,7 @@ ## Data loss and security -- [ ] 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] 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`) - [ ] 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`) diff --git a/scripts/lib/manifest.ps1 b/scripts/lib/manifest.ps1 index de1a254..dd6dfa0 100644 --- a/scripts/lib/manifest.ps1 +++ b/scripts/lib/manifest.ps1 @@ -3,6 +3,24 @@ function Get-ManagedManifestPath { return (Join-Path $Destination '.ai-config-manifest.tsv') } +function Resolve-ManagedPath { + param([Parameter(Mandatory=$true)][string]$Destination, [Parameter(Mandatory=$true)][string]$Relative) + $parts = $Relative -split '/', 0, 'SimpleMatch' + if ($Relative.StartsWith('/') -or $Relative.Contains('\') -or $Relative.Contains(':') -or $Relative.Contains("`t") -or $Relative.Contains("`r") -or $Relative.Contains("`n") -or @($parts | Where-Object { $_ -in @('', '.', '..') }).Count -gt 0) { + throw "Invalid managed path: $Relative" + } + $target = $Destination + foreach ($part in $parts) { $target = Join-Path $target $part } + $probe = $target + while ($true) { + $item = Get-Item -LiteralPath $probe -Force -ErrorAction SilentlyContinue + if ($item -and ($item.Attributes -band [IO.FileAttributes]::ReparsePoint)) { throw "Managed path contains a link: $probe" } + if ($probe -eq $Destination) { break } + $probe = Split-Path $probe -Parent + } + return $target +} + function Get-Sha256Hash { param([Parameter(Mandatory=$true)][string]$Path) # Lowercase to match sha256sum's output, since the manifest must be readable by @@ -22,7 +40,10 @@ function Read-ManagedManifest { $entries = @{} if (Test-Path -LiteralPath $ManifestPath) { # Lowercase to tolerate an older manifest written before hashes were normalized. - Import-Csv -LiteralPath $ManifestPath -Delimiter ([char]9) | ForEach-Object { $entries[$_.path] = $_.sha256.ToLowerInvariant() } + Import-Csv -LiteralPath $ManifestPath -Delimiter ([char]9) | ForEach-Object { + $null = Resolve-ManagedPath -Destination (Split-Path $ManifestPath -Parent) -Relative $_.path + $entries[$_.path] = $_.sha256.ToLowerInvariant() + } } return $entries } @@ -63,6 +84,7 @@ function Sync-ManagedDestination { ) if (-not $Summary) { Write-Output "SOURCE $Source -> $Destination" } $manifestPath = Get-ManagedManifestPath $Destination + $null = Resolve-ManagedPath -Destination $Destination -Relative '.ai-config-manifest.tsv' $oldManifest = Read-ManagedManifest $manifestPath $newManifest = @{} $backupRoot = Join-Path $Destination "backups/$Stamp" @@ -71,7 +93,7 @@ function Sync-ManagedDestination { Get-ChildItem $Source -File -Recurse | ForEach-Object { $relative = $_.FullName.Substring($Source.Length).TrimStart([char[]]@('\','/')).Replace('\','/') if ((Split-Path $Destination -Leaf) -eq '.codex' -and $relative.StartsWith('skills/')) { return } - $target = Join-Path $Destination $relative + $target = Resolve-ManagedPath -Destination $Destination -Relative $relative $rawContent = Get-Content -LiteralPath $_.FullName -Raw -Encoding UTF8 $filterFlashbang = -not $FlashbangEnabled -and $relative -in @('settings.json', 'config.toml') if ($filterFlashbang) { @@ -121,7 +143,7 @@ function Sync-ManagedDestination { if ($DryRun) { if (-not $Summary) { Write-Output "DRYRUN $action $target" }; if ($action -eq 'CREATE') { $tally.Created++ } else { $tally.Updated++ }; return } if ($exists) { $wasManaged = $oldManifest.ContainsKey($relative) - $backup = Join-Path $backupRoot $relative + $backup = Resolve-ManagedPath -Destination $Destination -Relative "backups/$Stamp/$relative" New-Item (Split-Path $backup -Parent) -ItemType Directory -Force | Out-Null Copy-Item -LiteralPath $target $backup -Force if (-not $Summary) { Write-Output "BACKUP $target -> $backup" } @@ -134,10 +156,10 @@ function Sync-ManagedDestination { } foreach ($relative in @($oldManifest.Keys | Where-Object { -not $newManifest.ContainsKey($_) })) { - $target = Join-Path $Destination $relative + $target = Resolve-ManagedPath -Destination $Destination -Relative $relative if (-not (Test-Path -LiteralPath $target)) { continue } if ($DryRun) { if (-not $Summary) { Write-Output "DRYRUN REMOVE $target" }; $tally.Removed++; continue } - $backup = Join-Path $backupRoot $relative + $backup = Resolve-ManagedPath -Destination $Destination -Relative "backups/$Stamp/$relative" New-Item (Split-Path $backup -Parent) -ItemType Directory -Force | Out-Null Copy-Item -LiteralPath $target $backup -Force if (-not $Summary) { Write-Output "BACKUP $target -> $backup" } diff --git a/scripts/lib/manifest.sh b/scripts/lib/manifest.sh index 3bc280c..b01b964 100755 --- a/scripts/lib/manifest.sh +++ b/scripts/lib/manifest.sh @@ -16,6 +16,24 @@ manifest_path() { printf '%s/.ai-config-manifest.tsv' "$1" } +managed_target() { + local destination=$1 relative=$2 component target=$destination probe + [[ -n "$relative" && "$relative" != /* && "$relative" != */ && "$relative" != *//* && "$relative" != *\\* && "$relative" != *:* && "$relative" != *$'\t'* && "$relative" != *$'\r'* && "$relative" != *$'\n'* ]] || { printf 'Invalid managed path: %s\n' "$relative" >&2; return 1; } + local -a parts + IFS=/ read -r -a parts <<< "$relative" + for component in "${parts[@]}"; do + [[ -n "$component" && "$component" != . && "$component" != .. ]] || { printf 'Invalid managed path: %s\n' "$relative" >&2; return 1; } + target+=/$component + done + probe=$target + while :; do + [ ! -L "$probe" ] || { printf 'Managed path contains a link: %s\n' "$probe" >&2; return 1; } + [ "$probe" != "$destination" ] || break + probe=$(dirname -- "$probe") + done + printf '%s' "$target" +} + # read_managed_manifest read_managed_manifest() { local path=$1 @@ -31,6 +49,7 @@ read_managed_manifest() { [ "$rel" = path ] && continue fi [ -n "$rel" ] || continue + managed_target "$(dirname -- "$path")" "$rel" > /dev/null || return 1 hash=$(printf '%s' "$hash" | tr '[:upper:]' '[:lower:]') # tolerate uppercase hashes from older manifests # shellcheck disable=SC2034 out_ref[$rel]=$hash @@ -57,8 +76,9 @@ sync_managed_destination() { [ "$summary" = true ] || printf 'SOURCE %s -> %s\n' "$source" "$destination" local manifest manifest=$(manifest_path "$destination") + managed_target "$destination" '.ai-config-manifest.tsv' > /dev/null || return 1 declare -A old_manifest - read_managed_manifest "$manifest" old_manifest + read_managed_manifest "$manifest" old_manifest || return 1 declare -A new_manifest local backup_root="$destination/backups/$stamp" local created=0 updated=0 unchanged=0 removed=0 warned=0 @@ -67,7 +87,7 @@ sync_managed_destination() { local relative target content needs_sub new_hash exists current_hash action backup relative=${source_file#"$source/"} if [ "${destination##*/}" = .codex ] && [[ "$relative" = skills/* ]]; then continue; fi - target="$destination/$relative" + target=$(managed_target "$destination" "$relative") || return 1 content=$(cat -- "$source_file" && printf '\034') || return content=${content%$'\034'} needs_sub=false @@ -150,7 +170,7 @@ sync_managed_destination() { fi if [ "$exists" = true ]; then - backup="$backup_root/$relative" + backup=$(managed_target "$destination" "backups/$stamp/$relative") || return 1 mkdir -p "$(dirname -- "$backup")" cp -- "$target" "$backup" [ "$summary" = true ] || printf 'BACKUP %s -> %s\n' "$target" "$backup" @@ -172,14 +192,14 @@ sync_managed_destination() { local relative target current_hash backup for relative in "${!old_manifest[@]}"; do [ -z "${new_manifest[$relative]+x}" ] || continue - target="$destination/$relative" + target=$(managed_target "$destination" "$relative") || return 1 [ -f "$target" ] || continue if [ "$dry_run" = true ]; then [ "$summary" = true ] || printf 'DRYRUN REMOVE %s\n' "$target" removed=$((removed + 1)) continue fi - backup="$backup_root/$relative" + backup=$(managed_target "$destination" "backups/$stamp/$relative") || return 1 mkdir -p "$(dirname -- "$backup")" cp -- "$target" "$backup" [ "$summary" = true ] || printf 'BACKUP %s -> %s\n' "$target" "$backup"