fix(ingestion): brain/raw/ readers go through an os.Root; symlinks are inert (#153) (#156)

Co-authored-by: Mathias <mthbqv@gmail.com>

ingestion/internal/extract/callsites_test.go

@@ -0,0 +1,130 @@
+package extract
+
+import (
+	"go/ast"
+	"go/parser"
+	"go/token"
+	"os"
+	"path/filepath"
+	"strconv"
+	"strings"
+	"testing"
+)
+
+const extractPkg = "github.com/mathiasbq/hyperguild/ingestion/internal/extract"
+
+// parseNonTest parses every non-test Go file under the module.
+func parseNonTest(t *testing.T) map[string]*ast.File {
+	t.Helper()
+	fset := token.NewFileSet()
+	out := map[string]*ast.File{}
+	err := filepath.WalkDir(filepath.Join("..", ".."), func(p string, d os.DirEntry, err error) error {
+		if err != nil {
+			return err
+		}
+		if d.IsDir() && (d.Name() == "vendor" || d.Name() == ".git") {
+			return filepath.SkipDir
+		}
+		if d.IsDir() || !strings.HasSuffix(p, ".go") || strings.HasSuffix(p, "_test.go") {
+			return nil
+		}
+		f, perr := parser.ParseFile(fset, p, nil, 0)
+		if perr != nil {
+			return perr
+		}
+		out[filepath.ToSlash(p)] = f
+		return nil
+	})
+	if err != nil {
+		t.Fatal(err)
+	}
+	return out
+}
+
+// PR #156 review: reading a caller-chosen path by name reopens it after the
+// containment check (hyperguild#153 item 3). Outside this package, nothing
+// may reference extract.Text — called OR taken as a value — under whatever
+// name the package is imported (round 2: an alias and a func value slipped
+// past a plain text search).
+func TestNoPackageReferencesExtractTextOutsideExtract(t *testing.T) {
+	for path, f := range parseNonTest(t) {
+		if strings.Contains(path, "internal/extract/") {
+			continue
+		}
+		names := map[string]bool{}
+		for _, imp := range f.Imports {
+			if p, _ := strconv.Unquote(imp.Path.Value); p == extractPkg {
+				name := "extract"
+				if imp.Name != nil {
+					name = imp.Name.Name
+				}
+				if name == "." {
+					t.Errorf("%s dot-imports extract; import it by name so its use stays checkable", path)
+				}
+				names[name] = true
+			}
+		}
+		if len(names) == 0 {
+			continue
+		}
+		ast.Inspect(f, func(n ast.Node) bool {
+			if sel, ok := n.(*ast.SelectorExpr); ok {
+				if id, ok := sel.X.(*ast.Ident); ok && names[id.Name] && sel.Sel.Name == "Text" {
+					t.Errorf("%s references %s.Text; use extract.TextIn through an os.Root", path, id.Name)
+				}
+			}
+			return true
+		})
+	}
+}
+
+// PR #156 review round 2: the FIFO fix (open non-blocking, check the handle)
+// lives in OpenRegular, and reverting any caller to a plain Root.Open stayed
+// green. In the three raw/ reader FILES parsed below (and only those: a
+// wrapper in another file is not seen), no Open/OpenFile/ReadFile selector
+// may appear outside OpenRegular, called or taken as a method value. This is
+// a second layer; the behavioural race tests in internal/api and
+// internal/watcher are what catch a rewrite through any API (round 3).
+// osOpenAllowed names the functions in guarded files that may call os.Open*
+// on a path that is not a raw/ read.
+var osOpenAllowed = map[string]bool{
+	"internal/watcher/watcher.go:appendWatcherLog": true, // ingest log append, not a raw/ read
+	// Text reads by path; outside this package only TextIn's temp copy reaches
+	// it (TestNoPackageReferencesExtractTextOutsideExtract enforces that).
+	"internal/extract/extract.go:Text": true,
+}
+
+func TestRawReaderFilesUseNoOpenOrReadFileOutsideOpenRegular(t *testing.T) {
+	guarded := []string{"internal/api/rawroot.go", "internal/extract/extract.go", "internal/watcher/watcher.go"}
+	files := parseNonTest(t)
+	for _, g := range guarded {
+		var f *ast.File
+		for p, af := range files {
+			if strings.HasSuffix(p, g) {
+				f = af
+			}
+		}
+		if f == nil {
+			t.Fatalf("guarded file %s not found", g)
+		}
+		for _, decl := range f.Decls {
+			fn, ok := decl.(*ast.FuncDecl)
+			if !ok || fn.Name.Name == "OpenRegular" {
+				continue
+			}
+			ast.Inspect(fn, func(n ast.Node) bool {
+				sel, ok := n.(*ast.SelectorExpr)
+				if !ok {
+					return true
+				}
+				if sel.Sel.Name == "Open" || sel.Sel.Name == "OpenFile" || sel.Sel.Name == "ReadFile" {
+					if id, ok := sel.X.(*ast.Ident); ok && id.Name == "os" && osOpenAllowed[g+":"+fn.Name.Name] {
+						return true // a named non-raw write (the ingest log append)
+					}
+					t.Errorf("%s: %s calls .%s; raw/ reads must go through OpenRegular", g, fn.Name.Name, sel.Sel.Name)
+				}
+				return true
+			})
+		}
+	}
+}

ingestion/internal/extract/extract.go

@@ -3,8 +3,10 @@ package extract
 
 import (
 	"fmt"
+	"io"
 	"os"
 	"strings"
+	"syscall"
 )
 
 // Text reads the file at path and returns its plain-text content.
@@ -41,3 +43,70 @@ func fileExt(path string) string {
 	}
 	return ""
 }
+
+// TextIn is Text for a file read through an os.Root (hyperguild#153): the
+// read cannot leave the root, so a path checked earlier cannot be swapped
+// for a symlink pointing elsewhere before it is read. Formats that need an
+// external tool (pdf, docmark) are copied to a temp file from the handle
+// opened through the root, then extracted from the copy.
+func TextIn(r *os.Root, rel string) (string, error) {
+	fi, err := r.Lstat(rel)
+	if err != nil {
+		return "", fmt.Errorf("stat %s: %w", rel, err)
+	}
+	if !fi.Mode().IsRegular() {
+		return "", fmt.Errorf("%s: not a regular file", rel)
+	}
+	f, _, err := OpenRegular(r, rel)
+	if err != nil {
+		return "", fmt.Errorf("open %s: %w", rel, err)
+	}
+	defer func() { _ = f.Close() }()
+	ext := strings.ToLower(fileExt(rel))
+	switch ext {
+	case ".md", ".txt":
+		b, err := io.ReadAll(f)
+		if err != nil {
+			return "", fmt.Errorf("read %s: %w", rel, err)
+		}
+		return string(b), nil
+	case ".pdf", ".docx", ".xlsx", ".pptx", ".png", ".jpg", ".jpeg":
+		tmp, err := os.CreateTemp("", "ingest-*"+ext)
+		if err != nil {
+			return "", fmt.Errorf("temp copy: %w", err)
+		}
+		defer func() { _ = os.Remove(tmp.Name()) }()
+		if _, err := io.Copy(tmp, f); err != nil {
+			_ = tmp.Close()
+			return "", fmt.Errorf("copy %s: %w", rel, err)
+		}
+		if err := tmp.Close(); err != nil {
+			return "", fmt.Errorf("copy %s: %w", rel, err)
+		}
+		return Text(tmp.Name())
+	default:
+		return "", fmt.Errorf("unsupported file extension: %s", ext)
+	}
+}
+
+// OpenRegular opens rel inside r and returns it only if the OPENED handle is
+// a regular file. The open is non-blocking, so a FIFO swapped in after an
+// Lstat (PR #156 review: it hung open(2) forever) is refused promptly rather
+// than stalling the reader. Callers keep their Lstat check, which refuses
+// symlinks outright; this closes the race between that check and the open.
+func OpenRegular(r *os.Root, rel string) (*os.File, os.FileInfo, error) {
+	f, err := r.OpenFile(rel, os.O_RDONLY|syscall.O_NONBLOCK, 0)
+	if err != nil {
+		return nil, nil, err
+	}
+	fi, err := f.Stat()
+	if err != nil {
+		_ = f.Close()
+		return nil, nil, err
+	}
+	if !fi.Mode().IsRegular() {
+		_ = f.Close()
+		return nil, nil, fmt.Errorf("%s: not a regular file", rel)
+	}
+	return f, fi, nil
+}

ingestion/internal/extract/root_test.go

@@ -0,0 +1,64 @@
+package extract
+
+import (
+	"os"
+	"path/filepath"
+	"syscall"
+	"testing"
+	"time"
+
+	"github.com/stretchr/testify/assert"
+	"github.com/stretchr/testify/require"
+)
+
+// hyperguild#153 item 3: ingest checked a path, then extract.Text reopened it
+// by name, so a file swapped for a symlink in between was followed out of
+// raw/. TextIn reads through an os.Root, which refuses to leave the root.
+func TestTextInReadsInsideTheRootAndRefusesEscapes(t *testing.T) {
+	root := t.TempDir()
+	require.NoError(t, os.WriteFile(filepath.Join(root, "ok.md"), []byte("inside"), 0o644))
+	outside := filepath.Join(t.TempDir(), "secret.md")
+	require.NoError(t, os.WriteFile(outside, []byte("SENTINEL-153-TEXTIN"), 0o644))
+	require.NoError(t, os.Symlink(outside, filepath.Join(root, "leak.md")))
+
+	r, err := os.OpenRoot(root)
+	require.NoError(t, err)
+	defer func() { _ = r.Close() }()
+
+	got, err := TextIn(r, "ok.md")
+	require.NoError(t, err)
+	assert.Equal(t, "inside", got)
+
+	got, err = TextIn(r, "leak.md")
+	assert.Error(t, err)
+	assert.NotContains(t, got, "SENTINEL-153-TEXTIN")
+
+	_, err = TextIn(r, "../"+filepath.Base(outside))
+	assert.Error(t, err)
+}
+
+// PR #156 review: a regular file swapped for a FIFO between Lstat and Open
+// blocked open(2) forever. OpenRegular opens non-blocking and checks the type
+// on the opened handle, so a FIFO is refused promptly instead of hanging.
+func TestOpenRegularRefusesAFIFOWithoutBlocking(t *testing.T) {
+	dir := t.TempDir()
+	require.NoError(t, syscall.Mkfifo(filepath.Join(dir, "pipe.md"), 0o644))
+	r, err := os.OpenRoot(dir)
+	require.NoError(t, err)
+	defer func() { _ = r.Close() }()
+
+	done := make(chan error, 1)
+	go func() {
+		f, _, err := OpenRegular(r, "pipe.md")
+		if f != nil {
+			_ = f.Close()
+		}
+		done <- err
+	}()
+	select {
+	case err := <-done:
+		assert.Error(t, err)
+	case <-time.After(2 * time.Second):
+		t.Fatal("OpenRegular blocked on a FIFO")
+	}
+}