Skip to content

fix(security): tighten conversation-history file perms to 0600#188

Merged
rafeegnash merged 1 commit into
masterfrom
fix/22-conversation-history-perms
Jun 4, 2026
Merged

fix(security): tighten conversation-history file perms to 0600#188
rafeegnash merged 1 commit into
masterfrom
fix/22-conversation-history-perms

Conversation

@rafeegnash
Copy link
Copy Markdown
Collaborator

Summary

Six of the ten provider conversation-history modules were writing the Q&A log world-readable (0644). The files routinely contain account IDs, ARNs, IPs, role names, and — for iam — entire policy fragments copied verbatim into the LLM answer.

Approach

  • New `internal/secfile` package with three primitives:
    • `EnsurePrivateDir(dir)` — `MkdirAll 0o700` then `Chmod 0o700` (idempotent tighten of pre-existing loose dirs)
    • `WritePrivate(path, data)` — `WriteFile 0o600` then `Chmod 0o600` (defends against `WriteFile` not re-applying mode when the file pre-exists)
    • `ReadPrivate(path)` — open → fd-based `Chmod` → `ReadAll` (no TOCTOU window between read and repair)
  • All ten conversation modules routed through `secfile` — chmod-on-load auto-repairs files installed by older 0644 binaries, so users don't need to `chmod` anything manually.
  • AST-based drift guard (`internal/secfile/drift_test.go`): walks the repo, finds every `conversation.go`, and fails the build if any of them calls `os.WriteFile` / `os.ReadFile` / `os.MkdirAll` directly. The test caught one provider missed by the original audit — `internal/verda/conversation.go` — which is now also patched.
  • End-to-end assertion in `internal/flyio/conversation_test.go::TestSaveLoadRoundTrip` that the on-disk file mode is `0600` and dir is `0700`.

Files touched

File Was Now
`internal/cloudflare/conversation.go` 0644 secfile
`internal/k8s/conversation.go` 0644 secfile
`internal/iam/conversation.go` 0644 secfile
`internal/flyio/conversation.go` 0644 secfile
`internal/railway/conversation.go` 0644 secfile
`internal/vercel/conversation.go` 0644 secfile
`internal/sentry/conversation.go` 0o600 / 0o755 dir secfile
`internal/linear/conversation.go` 0o600 / 0o755 dir secfile
`internal/notion/conversation.go` 0o600 / 0o755 dir secfile
`internal/verda/conversation.go` 0o755 dir, read-only secfile

Notes for reviewers

  • Windows: `Chmod` is short-circuited via `runtime.GOOS == "windows"` because Unix mode bits aren't meaningful there. Default ACLs already create owner-only.
  • Out of scope: `internal/convhistory` extraction (feat(k8s): add k8s ask command for natural language cluster queries #25) and the path-traversal in `sanitizeFilename` (feat: terraform support #23). Both are intentionally deferred so this hotfix stays narrow.
  • Sibling bug: `clanker-cloud`'s `backend/internal/{vercel,flyio,railway}/conversation.go` are copy-paste duplicates that also write `0644` — will file a separate issue against that repo after this lands.

Test plan

  • `go build ./...` clean
  • `go vet ./...` clean
  • `go test -race -count=1 ./...` — all packages pass
  • `secfile` unit tests: new file is 0600, existing loose file tightened to 0600, dir tightened to 0700, ReadPrivate repairs perms
  • `flyio` round-trip test: end-to-end file-on-disk is 0600, dir is 0700
  • Drift test fails the build if any `conversation.go` regresses to `os.WriteFile` / `os.ReadFile` / `os.MkdirAll`

Closes #22

Six of the ten provider conversation-history modules were writing the
Q&A log world-readable (0644). The files routinely contain account
IDs, ARNs, IPs, role names, and (in iam) entire policy fragments
copied verbatim into the LLM answer — anything a non-owner on a
shared host could grep.

Add a tiny internal/secfile package that encapsulates three
primitives — EnsurePrivateDir (0700), WritePrivate (0600), and
ReadPrivate (open → fd-based chmod → ReadAll). The fd-based chmod
removes the TOCTOU window that a path-based Chmod would leave open.

All ten conversation.go modules (cloudflare, k8s, iam, flyio,
railway, vercel, sentry, linear, notion, verda) now route Save /
Load / MkdirAll through secfile. The chmod-on-load path also
auto-repairs files installed by older 0644 binaries — users don't
need to chmod the existing files manually.

Lock the door behind us: a parse-based drift test walks the repo,
finds every conversation.go, and fails the build if any of them
calls os.WriteFile / os.ReadFile / os.MkdirAll directly. A tenth
provider added next quarter cannot reintroduce this leak by
copy-pasting the historic pattern. The test caught one mistake
immediately — internal/verda/conversation.go was missed by the
original audit and is now also patched.

Mode bits are not meaningful on Windows; the Chmod calls inside
secfile short-circuit there. Default ACLs already create
owner-only on Windows.

Closes #22
@rafeegnash rafeegnash merged commit 15917e4 into master Jun 4, 2026
5 checks passed
@rafeegnash rafeegnash deleted the fix/22-conversation-history-perms branch June 4, 2026 15:39
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant