fix(security): allowlist-sanitize history-file identifiers via SafeSlug#189
Merged
Conversation
Seven provider conversation-history modules built filenames with a blocklist sanitiser — strings.NewReplacer over a fixed list of shell metacharacters that let `.` and `..` through untouched. With CLI flags like `--cluster ../../etc/passwd` (k8s) or `--account-id ../..` (cloudflare) wired straight into the filename, an attacker could write the LLM history outside ~/.clanker. The other three providers (sentry, linear, notion) already used the right pattern — an allowlist over [A-Za-z0-9_-] with a "default" fallback for empty input — but the implementation was copy-pasted three times. Move the canonical implementation into internal/secfile as SafeSlug(s string) string. Bound the result to 64 bytes so a verbose input can't blow past common filesystem limits. Replace the sanitiser call in all ten conversation modules and delete every local sanitize/safeSlug helper. Net diff: -150 LoC. Extend the AST-based drift test (from #22) to fail the build if any conversation.go: 1. calls strings.NewReplacer (the blocklist anti-pattern), or 2. declares a top-level func named sanitize*/safeSlug. Either form would silently reintroduce the original CVE. Acceptance cases pass (see internal/secfile/secfile_test.go): ".." -> "default" "../../etc/passwd" -> "etcpasswd" "my-cluster.dev" -> "my-clusterdev" "" -> "default" "123456789012" -> "123456789012" (AWS account IDs preserved) "中文" / "\\x00\\x00" -> "default" (unicode/null stripped) "a" * 200 -> "a" * 64 (length cap) Migration: existing history files written by the old sanitiser may become orphaned (e.g. cloudflare_my.account.json -> code now reads cloudflare_myaccount.json). The orphan is intentional — a rename migration would itself need to handle the unsafe old name. Users silently start a fresh conversation. Closes #23
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Seven of ten provider conversation-history modules built filenames with a blocklist sanitiser — `strings.NewReplacer` over a fixed set of shell metacharacters that let `.` and `..` through untouched. With CLI flags like `--cluster` (k8s) or `--account-id` (cloudflare) wired straight into the filename, an attacker could escape `~/.clanker`.
The other three providers (sentry, linear, notion) already used the right pattern — an allowlist over `[A-Za-z0-9_-]` — but the implementation was copy-pasted three times. Consolidate into one canonical helper.
What changed
Either form would silently reintroduce the original CVE.
Exploit reachability (per provider)
Acceptance cases (see `internal/secfile/secfile_test.go::TestSafeSlug`)
Migration
Existing history files written by the old sanitiser may become orphaned — e.g. `cloudflare_my.account.json` was correct under the old code, but `secfile.SafeSlug` now strips the dot so the new code looks at `cloudflare_myaccount.json`. Intentional: a rename migration would itself need to construct + trust the unsafe old path. Users silently start a fresh conversation. Documented in the commit body.
Out of scope (per fresh-eyes review)
Test plan
Closes #23