From 963513d5b6cae9642843a485713a634b8f685765 Mon Sep 17 00:00:00 2001 From: Danil Pismenny Date: Sun, 6 Sep 2026 09:28:58 +0300 Subject: [PATCH] feat: recognize a payload projection when doctor reads a document MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Репозиторий, владеющий `template/`, не является downstream-установкой самого себя. Копировать payload в установленное дерево там незачем, поэтому такой репозиторий может представлять generic-документы симлинками в `template/`. `doctor` считал это дефектом: `manifest.managed_unreadable` и `governance.unsafe_symlink` на каждый такой документ. `lint` при этом те же файлы читал как обычные и не возражал — инструменты расходились между собой. `internal/projection` отвечает на один вопрос: разрешается ли путь внутри корня репозитория И указывает ли он ровно на payload-файл, стоящий за этим путём, в репозитории, который действительно несёт `template/memory-bank`. Симлинк наружу, симлинк на другой файл, битая ссылка и обычный файл проекцией не считаются и сообщаются как прежде. Объём сознательно минимален. Прошлые редакции этой ветки трогали мутационный путь и обход каталожных симлинков; три раунда ревью показали, что там своя поверхность риска — от тихой порчи lock до факториального роста числа документов при перекрёстных ссылках, причём обход менял поведение для всех репозиториев, а не только для тех, что несут payload. Обе части вынесены в feat/projection-ownership-support. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01RtxeRwhVf1y8zvxUtk6xrS --- CHANGELOG.md | 16 +++++ internal/doctor/doctor.go | 26 +++++-- internal/doctor/doctor_test.go | 43 ++++++++++++ internal/doctor/governance.go | 16 ++++- internal/projection/projection.go | 94 +++++++++++++++++++++++++ internal/projection/projection_test.go | 96 ++++++++++++++++++++++++++ 6 files changed, 283 insertions(+), 8 deletions(-) create mode 100644 internal/projection/projection.go create mode 100644 internal/projection/projection_test.go diff --git a/CHANGELOG.md b/CHANGELOG.md index a8221af..81a764a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -7,6 +7,22 @@ and this project follows [Semantic Versioning](https://semver.org/spec/v2.0.0.ht ## [Unreleased] +### Added + +- Recognize a payload projection: a symlink whose resolved target stays inside + the repository and lands on the `template/` file backing that exact path. A + repository that owns the payload is not a downstream installation of itself, + so it may reference generic documents instead of copying them. `doctor` then + inspects the payload the link resolves to, instead of reporting + `manifest.managed_unreadable` and `governance.unsafe_symlink` for every such + document. + +Recognition requires the repository to actually carry a `template/memory-bank` +tree. Symlinks that leave the repository root, or point at anything other than +the payload file backing the path, remain unsafe and are reported exactly as +before. Directory traversal in `lint` and `doctor` is unchanged, and `init`, +`pull` and the ownership lock are untouched. + ## [2.2.0] - 2026-08-15 ### Changed diff --git a/internal/doctor/doctor.go b/internal/doctor/doctor.go index 3251ed9..c5b9204 100644 --- a/internal/doctor/doctor.go +++ b/internal/doctor/doctor.go @@ -13,6 +13,7 @@ import ( "github.com/dapi/memory-bank-cli/internal/lint" "github.com/dapi/memory-bank-cli/internal/ownership" + "github.com/dapi/memory-bank-cli/internal/projection" "gopkg.in/yaml.v3" ) @@ -140,7 +141,7 @@ func (report *Report) checkIdentityAndDrift(agentFile, scopeRoot string) { } report.add(Finding{Code: "agent.entrypoint_missing", Severity: severity, Group: "agent_integration", Path: agentFile, Message: message, Remediation: fmt.Sprintf("Create the agent instruction file and link it to %s/README.md.", scopeRoot)}) } else if !strings.Contains(string(contents), scopeRoot+"/README.md") { - report.add(Finding{Code: "agent.memory_bank_link_missing", Severity: Error, Group: "agent_integration", Path: agentFile, Message: fmt.Sprintf("Agent instructions do not route readers to %s/README.md.", scopeRoot), Remediation: fmt.Sprintf("Add a repository-relative link to %s/README.md or run memory-bank-cli pull with the same --agent-file.", scopeRoot)}) + report.add(Finding{Code: "agent.memory_bank_link_missing", Severity: Error, Group: "agent_integration", Path: agentFile, Message: fmt.Sprintf("Agent instructions do not route readers to %s/README.md.", scopeRoot), Remediation: fmt.Sprintf("Add a repository-relative link to %s/README.md or run memory-bank-cli pull with the same --agent-file.", scopeRoot)}) } if exists && lockErr == nil { agentReport, err := ownership.InspectAgentInstructions(report.RepoRoot, agentFile) @@ -170,12 +171,12 @@ func (report *Report) checkManagedDrift(lock ownership.Lock) { if os.IsNotExist(err) { code, message = "manifest.managed_missing", "Managed file recorded by the lock is missing." } - report.add(Finding{Code: code, Severity: Error, Group: "manifest", Path: filePath, Message: message, Remediation: "Restore the file from the pinned template with memory-bank-cli pull."}) + report.add(Finding{Code: code, Severity: Error, Group: "manifest", Path: filePath, Message: message, Remediation: "Restore the file from the pinned template with memory-bank-cli pull."}) continue } digest := fmt.Sprintf("sha256:%x", sha256.Sum256(data)) if digest != contract.PayloadDigest { - report.add(Finding{Code: "manifest.managed_content_drift", Severity: Error, Group: "manifest", Path: filePath, Message: "Managed file content differs from the lock payload digest.", Remediation: "Review the local change, then restore it through memory-bank-cli pull."}) + report.add(Finding{Code: "manifest.managed_content_drift", Severity: Error, Group: "manifest", Path: filePath, Message: "Managed file content differs from the lock payload digest.", Remediation: "Review the local change, then restore it through memory-bank-cli pull."}) } mode := observedPayloadMode(info.Mode().Perm()) if contract.PayloadMode != "" && mode != "" && mode != contract.PayloadMode { @@ -209,17 +210,32 @@ func readRegularWithinRoot(repoRoot, relativePath string) ([]byte, fs.FileInfo, return nil, nil, err } if info.Mode()&os.ModeSymlink != 0 { + // A template source repository may project its own payload instead + // of copying it. Such a link resolves inside the repository onto + // the payload file backing this exact path, so reading through it + // yields the payload itself. Any other symlink stays unsafe. + // + // Read the path Resolve verified rather than walking the link + // again: a second traversal could follow a link re-pointed in the + // meantime. + if resolved, projected := projection.Resolve(repoRoot, relativePath); projected { + return readResolvedRegular(resolved, relativePath) + } return nil, nil, fmt.Errorf("unsafe symlink in path %q", relativePath) } } - info, err := os.Stat(current) + return readResolvedRegular(current, relativePath) +} + +func readResolvedRegular(path, relativePath string) ([]byte, fs.FileInfo, error) { + info, err := os.Stat(path) if err != nil { return nil, nil, err } if !info.Mode().IsRegular() { return nil, info, fmt.Errorf("path %q is not a regular file", relativePath) } - data, err := os.ReadFile(current) + data, err := os.ReadFile(path) return data, info, err } diff --git a/internal/doctor/doctor_test.go b/internal/doctor/doctor_test.go index 20de55d..b5d5501 100644 --- a/internal/doctor/doctor_test.go +++ b/internal/doctor/doctor_test.go @@ -789,3 +789,46 @@ func hasFinding(report Report, code string) bool { } return false } + +func writeProjectionFixture(t *testing.T, root, relative, contents string) { + t.Helper() + full := filepath.Join(root, filepath.FromSlash(relative)) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(contents), 0o644); err != nil { + t.Fatal(err) + } +} + +// A governed document that links outside the repository, or dangles, is what +// the unsafe-symlink finding exists for. Walking past it silently would remove +// the check the projection support was supposed to leave intact. +func TestGovernanceStillReportsUnsafeGovernedSymlinks(t *testing.T) { + repo, outside := t.TempDir(), t.TempDir() + writeProjectionFixture(t, outside, "evil.md", "---\nstatus: active\n---\n") + if err := os.MkdirAll(filepath.Join(repo, "memory-bank", "flows"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(filepath.Join(outside, "evil.md"), filepath.Join(repo, "memory-bank", "flows", "evil.md")); err != nil { + t.Skipf("symlinks are unavailable: %v", err) + } + if err := os.Symlink(filepath.Join("..", "missing.md"), filepath.Join(repo, "memory-bank", "flows", "dangling.md")); err != nil { + t.Skipf("symlinks are unavailable: %v", err) + } + + report := Report{RepoRoot: repo, Findings: []Finding{}} + report.checkGovernance("memory-bank") + + found := map[string]bool{} + for _, finding := range report.Findings { + if finding.Code == "governance.unsafe_symlink" { + found[finding.Path] = true + } + } + for _, expected := range []string{"memory-bank/flows/evil.md", "memory-bank/flows/dangling.md"} { + if !found[expected] { + t.Fatalf("no unsafe_symlink finding for %s: %#v", expected, report.Findings) + } + } +} diff --git a/internal/doctor/governance.go b/internal/doctor/governance.go index 3b9d052..74cd5c7 100644 --- a/internal/doctor/governance.go +++ b/internal/doctor/governance.go @@ -12,6 +12,7 @@ import ( "strings" "github.com/dapi/memory-bank-cli/internal/lint" + "github.com/dapi/memory-bank-cli/internal/projection" "gopkg.in/yaml.v3" ) @@ -46,11 +47,20 @@ func (report *Report) checkGovernance(scopeRoot string) { return err } documentPath := filepath.ToSlash(relative) + readPath := fullPath if entry.Type()&os.ModeSymlink != 0 { - report.add(Finding{Code: "governance.unsafe_symlink", Severity: Error, Group: "frontmatter_governance", Path: documentPath, Message: "Governed document is a symlink.", Remediation: "Replace it with a regular repository-owned Markdown file."}) - return nil + // A template source repository may project a governed document from + // its own payload. The document behind such a link is the payload + // itself, so it is governed exactly as the payload is. Read the path + // the check verified rather than walking the link again. + resolved, projected := projection.Resolve(report.RepoRoot, documentPath) + if !projected { + report.add(Finding{Code: "governance.unsafe_symlink", Severity: Error, Group: "frontmatter_governance", Path: documentPath, Message: "Governed document is a symlink.", Remediation: "Replace it with a regular repository-owned Markdown file, or point it at the payload file this path installs from."}) + return nil + } + readPath = resolved } - data, err := os.ReadFile(fullPath) + data, err := os.ReadFile(readPath) if err != nil { return err } diff --git a/internal/projection/projection.go b/internal/projection/projection.go new file mode 100644 index 0000000..1a6a9b3 --- /dev/null +++ b/internal/projection/projection.go @@ -0,0 +1,94 @@ +// Package projection recognises an intentional payload projection: a symlink +// inside a template source repository that points at the repository's own +// payload instead of a copy of it. +// +// A repository that owns template/ is not a downstream installation of itself. +// Duplicating the payload into the installed tree buys nothing there, so such a +// repository may represent generic documents as symlinks into template/. The +// symlink then equals the payload by construction and cannot drift. +// +// This is deliberately narrow. The symlink guards elsewhere in this CLI exist to +// stop a write from escaping the repository root through a redirected path, and +// that protection is unchanged: a link is only a projection when it resolves +// inside the same repository AND lands exactly on the payload file that backs +// the destination path. Everything else stays an unsafe path. +package projection + +import ( + "os" + "path/filepath" + "strings" + + "github.com/dapi/memory-bank-cli/internal/ownership" +) + +// PayloadRoot is the tracked payload tree of a template source repository, so a +// destination path X is backed by PayloadRoot/X. It deliberately reuses the +// canonical constant: two definitions of a security-relevant path would let a +// rename silently turn every projection back into an unsafe symlink. +const PayloadRoot = ownership.CanonicalTemplateRoot + +// Resolve reports whether the repository-relative destination path resolves, +// through one or more symlinks, to the payload file that backs it, and returns +// that resolved payload path. +// +// The returned path is what the destination actually reads. A caller must not +// assume it matches an incoming source payload: the local payload can be older +// than the source being installed, and only comparing content can tell. +// +// It returns false rather than an error for the ordinary negative cases — a +// regular file, a broken link, a missing payload counterpart — because callers +// use this to decide between two legitimate behaviours, not to detect faults. +func Resolve(repoRoot, relative string) (string, bool) { + if repoRoot == "" || relative == "" { + return "", false + } + osRelative := filepath.FromSlash(relative) + if !filepath.IsLocal(osRelative) { + return "", false + } + + // Only a repository that actually carries a payload tree can project from + // it. Without this, an unrelated directory named template/ in a downstream + // repository would be enough to accept a symlink where the lock expects a + // regular managed file. + if info, err := os.Stat(filepath.Join(repoRoot, PayloadRoot, ownership.DownstreamPayloadRoot)); err != nil || !info.IsDir() { + return "", false + } + resolvedRoot, err := filepath.EvalSymlinks(repoRoot) + if err != nil { + return "", false + } + destination, err := filepath.EvalSymlinks(filepath.Join(repoRoot, osRelative)) + if err != nil { + return "", false + } + payload, err := filepath.EvalSymlinks(filepath.Join(repoRoot, PayloadRoot, osRelative)) + if err != nil { + return "", false + } + if destination != payload || !within(resolvedRoot, destination) { + return "", false + } + return destination, true +} + +// IsPayloadProjection reports whether the destination path is a projection of +// the payload file backing it. +func IsPayloadProjection(repoRoot, relative string) bool { + _, ok := Resolve(repoRoot, relative) + return ok +} + +// within reports whether target is the root itself or lives below it. Both +// paths must already be resolved, so no symlink can move the target afterwards. +func within(root, target string) bool { + relative, err := filepath.Rel(root, target) + if err != nil { + return false + } + if relative == ".." || strings.HasPrefix(relative, ".."+string(filepath.Separator)) { + return false + } + return true +} diff --git a/internal/projection/projection_test.go b/internal/projection/projection_test.go new file mode 100644 index 0000000..00d362a --- /dev/null +++ b/internal/projection/projection_test.go @@ -0,0 +1,96 @@ +package projection + +import ( + "os" + "path/filepath" + "testing" +) + +func symlinkForTest(t *testing.T, target, link string) { + t.Helper() + if err := os.MkdirAll(filepath.Dir(link), 0o755); err != nil { + t.Fatal(err) + } + if err := os.Symlink(target, link); err != nil { + t.Skipf("symlinks are unavailable: %v", err) + } +} + +func writeForTest(t *testing.T, root, relative, contents string) string { + t.Helper() + full := filepath.Join(root, filepath.FromSlash(relative)) + if err := os.MkdirAll(filepath.Dir(full), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(full, []byte(contents), 0o644); err != nil { + t.Fatal(err) + } + return full +} + +func TestRecognisesFileProjection(t *testing.T) { + repo := t.TempDir() + writeForTest(t, repo, "template/memory-bank/dna/rule.md", "payload\n") + symlinkForTest(t, "../../template/memory-bank/dna/rule.md", filepath.Join(repo, "memory-bank/dna/rule.md")) + + if !IsPayloadProjection(repo, "memory-bank/dna/rule.md") { + t.Fatal("symlink into the repository's own payload must be recognised as a projection") + } +} + +func TestRecognisesProjectionThroughDirectorySymlink(t *testing.T) { + repo := t.TempDir() + writeForTest(t, repo, "template/memory-bank/flows/routing.md", "payload\n") + symlinkForTest(t, "../template/memory-bank/flows", filepath.Join(repo, "memory-bank/flows")) + + if !IsPayloadProjection(repo, "memory-bank/flows/routing.md") { + t.Fatal("a directory symlink into the payload projects the files below it") + } +} + +func TestRejectsSymlinkEscapingTheRepository(t *testing.T) { + repo, outside := t.TempDir(), t.TempDir() + writeForTest(t, repo, "template/memory-bank/dna/rule.md", "payload\n") + writeForTest(t, outside, "rule.md", "payload\n") + symlinkForTest(t, filepath.Join(outside, "rule.md"), filepath.Join(repo, "memory-bank/dna/rule.md")) + + if IsPayloadProjection(repo, "memory-bank/dna/rule.md") { + t.Fatal("a link leaving the repository root must never count as a projection") + } +} + +func TestRejectsSymlinkToADifferentPayloadFile(t *testing.T) { + repo := t.TempDir() + writeForTest(t, repo, "template/memory-bank/dna/rule.md", "payload\n") + writeForTest(t, repo, "template/memory-bank/dna/other.md", "payload\n") + symlinkForTest(t, "../../template/memory-bank/dna/other.md", filepath.Join(repo, "memory-bank/dna/rule.md")) + + if IsPayloadProjection(repo, "memory-bank/dna/rule.md") { + t.Fatal("only the payload file backing this exact destination is a projection") + } +} + +func TestRejectsRegularFileAndBrokenLink(t *testing.T) { + repo := t.TempDir() + writeForTest(t, repo, "template/memory-bank/dna/rule.md", "payload\n") + writeForTest(t, repo, "memory-bank/dna/rule.md", "local override\n") + symlinkForTest(t, "../../template/memory-bank/dna/missing.md", filepath.Join(repo, "memory-bank/dna/broken.md")) + + if IsPayloadProjection(repo, "memory-bank/dna/rule.md") { + t.Fatal("a regular file is an override, not a projection") + } + if IsPayloadProjection(repo, "memory-bank/dna/broken.md") { + t.Fatal("a broken link is not a projection") + } +} + +func TestRejectsPathsOutsideTheRepositoryRoot(t *testing.T) { + repo := t.TempDir() + writeForTest(t, repo, "template/memory-bank/dna/rule.md", "payload\n") + + for _, relative := range []string{"", "../escape.md", "/absolute.md"} { + if IsPayloadProjection(repo, relative) { + t.Fatalf("path %q must not be treated as a projection", relative) + } + } +}