From a16f66cbc79d4483e6bd24823facc1e8a115f460 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Sun, 5 Jul 2026 00:10:47 +0000 Subject: [PATCH] Enforce fs source containment --- docs/consumers/pkg-scriptorium.md | 2 +- docs/internal/adapters.md | 8 +- engine_test.go | 40 ++++++++- internal/filecatalog/catalog.go | 37 +++++++++ internal/filecatalog/catalog_test.go | 87 ++++++++++++++++++++ internal/promptdef/filesystem_repository.go | 26 ++++-- internal/promptdef/repository_test.go | 63 ++++++++++++++ internal/validate/standard_validator.go | 32 ++++--- internal/validate/standard_validator_test.go | 56 +++++++++++++ 9 files changed, 327 insertions(+), 24 deletions(-) diff --git a/docs/consumers/pkg-scriptorium.md b/docs/consumers/pkg-scriptorium.md index 6e42bda..696398e 100644 --- a/docs/consumers/pkg-scriptorium.md +++ b/docs/consumers/pkg-scriptorium.md @@ -31,7 +31,7 @@ Directory fields on `Config` remain the compatibility path. Explicit source opti - `WithProfileFS(fsys, root)` and `WithProfileFile(path)` - `WithSchemaFS(fsys, root)` and `WithSchemaFile(path)` -Prompt and profile sources load standard Scriptorium YAML with the same strict validation as directory sources. Prompt `content_file` paths resolve relative to the prompt file in the same source. Profile options overlay custom profiles above built-ins. Schema `fs.FS` sources preserve prompt `schema_path` semantics; schema file options expose the file by its base name. +Prompt and profile sources load standard Scriptorium YAML with the same strict validation as directory sources. For `WithPromptFS`, the configured root is a containment boundary: prompt `content_file` paths resolve relative to the prompt file and must remain inside that root. Profile options overlay custom profiles above built-ins. For `WithSchemaFS`, prompt `schema_path` values resolve inside the configured root. Absolute paths and relative traversal outside those `fs.FS` roots are rejected. Schema file options expose the file by its base name. ## In-Memory Profiles diff --git a/docs/internal/adapters.md b/docs/internal/adapters.md index 1d805b6..e34c247 100644 --- a/docs/internal/adapters.md +++ b/docs/internal/adapters.md @@ -11,7 +11,7 @@ This document describes implemented adapter/repository boundaries and their curr - root package `scriptorium`: public Go library facade for preparing and running prompt requests. - `internal/promptdef`: filesystem and `fs.FS` prompt-definition repositories. - `internal/profile`: filesystem, `fs.FS`, and overlay execution-profile repositories. -- `internal/filecatalog`: shared YAML discovery and display-path helpers for prompt/profile repositories. +- `internal/filecatalog`: shared YAML discovery, display-path, and `fs.FS` source-root resolution helpers. - `internal/profile/builtin`: embedded built-in execution-profile repository. - `internal/artifact`: input artifact reader. - `internal/prompt`: Go-template renderer. @@ -46,7 +46,7 @@ Prompt/profile repositories: - Input: prompt/profile YAML files under configured directories or `fs.FS` roots. - Output: normalized domain definitions/profiles or typed errors. -- Shared YAML catalog helpers provide recursive discovery, extension filtering, deterministic ordering, file stems, and `fs.FS` display paths. +- Shared YAML catalog helpers provide recursive discovery, extension filtering, deterministic ordering, file stems, `fs.FS` display paths, and source-root containment checks. - Single-file public sources are represented as `fs.FS` roots containing one YAML file; lookup still uses YAML `id` values. Profile repository composition: @@ -110,6 +110,7 @@ Strict decoding and input checks: - prompt/profile repositories scan nested subdirectories recursively. - prompt/profile lookup uses YAML `id` values; subdirectory paths are organizational only. - prompt `content_file` paths resolve relative to the prompt YAML file within the same source. +- `fs.FS` prompt `content_file` paths and schema paths must remain inside the configured source root; absolute paths and relative traversal outside the root are rejected. - duplicate prompt/profile IDs are invalid and fail instead of using first-match behavior. - duplicate profile IDs across custom and built-in sources are allowed; the custom source overrides the built-in profile. - HTTP DTO decoder rejects unknown JSON fields. @@ -142,7 +143,8 @@ Validator: - `basic`, `json`, `json_schema` content failures return `ValidationFailed` results. - schema load/compile/path failures are runtime errors. -- schema lookup uses explicit `schema_path` values relative to `schema_dir`; it does not recursively search by basename. +- directory-backed schema lookup uses explicit `schema_path` values relative to `schema_dir`; it does not recursively search by basename. +- `fs.FS` schema lookup uses explicit `schema_path` values inside the configured source root. Single-file public schema sources match by base name. HTTP error mapping: diff --git a/engine_test.go b/engine_test.go index a775678..2a9beb8 100644 --- a/engine_test.go +++ b/engine_test.go @@ -881,6 +881,38 @@ output: } } +func TestPrepareWithPromptFSRejectsEscapedContentFile(t *testing.T) { + promptFS := fstest.MapFS{ + "assets/prompts/fs-escape.yaml": &fstest.MapFile{Data: []byte(` +id: fs.escape +version: "1.0.0" +default_profile: local-fast +messages: + - role: user + content_file: ../outside.tmpl +output: + format: text + validation_mode: none + repair_attempts: 0 +`)}, + "assets/outside.tmpl": &fstest.MapFile{Data: []byte(`Outside root.`)}, + } + + engine, err := scriptorium.NewEngine(scriptorium.Config{ + PromptDir: t.TempDir(), + ProfileDir: "./examples/profiles", + SchemaDir: "./examples/schemas", + }, scriptorium.WithPromptFS(promptFS, "assets/prompts")) + if err != nil { + t.Fatalf("expected engine construction to succeed, got %v", err) + } + + _, err = engine.Prepare(context.Background(), scriptorium.RunRequest{PromptID: "fs.escape"}) + if !errors.Is(err, scriptorium.ErrPromptLoad) { + t.Fatalf("expected ErrPromptLoad, got %v", err) + } +} + func TestPrepareWorksWithPromptFile(t *testing.T) { promptDir := t.TempDir() promptPath := filepath.Join(promptDir, "single.yaml") @@ -893,7 +925,7 @@ inputs: required: true messages: - role: user - content: "Summarize {{input \"transcript\"}} from file." + content_file: ./single.tmpl output: format: text validation_mode: none @@ -901,6 +933,9 @@ output: `), 0o644); err != nil { t.Fatal(err) } + if err := os.WriteFile(filepath.Join(promptDir, "single.tmpl"), []byte(`Summarize {{input "transcript"}} from file.`), 0o644); err != nil { + t.Fatal(err) + } engine, err := scriptorium.NewEngine(scriptorium.Config{ ProfileDir: "./examples/profiles", @@ -922,6 +957,9 @@ output: if prepared.PromptID != "single.file.prompt" { t.Fatalf("unexpected prompt id: %q", prepared.PromptID) } + if len(prepared.Messages) != 1 || !strings.Contains(prepared.Messages[0].Content, "from file") { + t.Fatalf("expected content_file body from prompt file, got %+v", prepared.Messages) + } } func TestPrepareWorksWithProfileFSOverBuiltIns(t *testing.T) { diff --git a/internal/filecatalog/catalog.go b/internal/filecatalog/catalog.go index 16c9272..e0c8369 100644 --- a/internal/filecatalog/catalog.go +++ b/internal/filecatalog/catalog.go @@ -2,6 +2,7 @@ package filecatalog import ( "context" + "fmt" "io/fs" "os" "path" @@ -93,6 +94,42 @@ func DisplayPath(root string, name string) string { return cleanName } +// ResolveFSPath resolves userPath from baseDir and keeps it inside root. +func ResolveFSPath(root string, baseDir string, userPath string) (string, string, error) { + cleanRoot := CleanFSRoot(root) + cleanBase := path.Clean(strings.TrimSpace(baseDir)) + if cleanBase == "" { + cleanBase = cleanRoot + } + if !containsFSPath(cleanRoot, cleanBase) { + return "", "", fmt.Errorf("base path %q is outside source root %q", cleanBase, cleanRoot) + } + + cleanUserPath := strings.TrimSpace(userPath) + if cleanUserPath == "" { + return "", "", fmt.Errorf("path is required") + } + cleanUserPath = path.Clean(cleanUserPath) + if path.IsAbs(cleanUserPath) { + return "", "", fmt.Errorf("path %q must be relative", userPath) + } + + resolved := path.Clean(path.Join(cleanBase, cleanUserPath)) + if !containsFSPath(cleanRoot, resolved) { + return "", "", fmt.Errorf("path %q escapes source root %q", userPath, cleanRoot) + } + return resolved, DisplayPath(cleanRoot, resolved), nil +} + +func containsFSPath(root string, name string) bool { + root = CleanFSRoot(root) + name = path.Clean(name) + if root == "." { + return name == "." || (name != ".." && !strings.HasPrefix(name, "../")) + } + return name == root || strings.HasPrefix(name, strings.TrimSuffix(root, "/")+"/") +} + // Stem strips .yaml or .yml from a file name. func Stem(name string) string { name = strings.TrimSuffix(name, ".yaml") diff --git a/internal/filecatalog/catalog_test.go b/internal/filecatalog/catalog_test.go index 0ba8015..6a2a2cc 100644 --- a/internal/filecatalog/catalog_test.go +++ b/internal/filecatalog/catalog_test.go @@ -6,6 +6,7 @@ import ( "os" "path/filepath" "reflect" + "strings" "testing" "testing/fstest" ) @@ -131,6 +132,92 @@ func TestDisplayPath(t *testing.T) { } } +func TestResolveFSPath(t *testing.T) { + tests := []struct { + name string + root string + baseDir string + userPath string + wantPath string + wantDisplay string + wantErr string + }{ + { + name: "sibling inside root", + root: "prompts", + baseDir: "prompts/nested", + userPath: "./messages/user.tmpl", + wantPath: "prompts/nested/messages/user.tmpl", + wantDisplay: "nested/messages/user.tmpl", + }, + { + name: "parent inside root", + root: "prompts", + baseDir: "prompts/nested", + userPath: "../shared/user.tmpl", + wantPath: "prompts/shared/user.tmpl", + wantDisplay: "shared/user.tmpl", + }, + { + name: "escape rejected", + root: "prompts", + baseDir: "prompts/nested", + userPath: "../../outside.tmpl", + wantErr: "escapes source root", + }, + { + name: "absolute path rejected", + root: "prompts", + baseDir: "prompts/nested", + userPath: "/outside.tmpl", + wantErr: "must be relative", + }, + { + name: "empty path rejected", + root: "prompts", + baseDir: "prompts/nested", + userPath: " ", + wantErr: "path is required", + }, + { + name: "dot root allows normal relative path", + root: ".", + baseDir: ".", + userPath: "schemas/events.schema.json", + wantPath: "schemas/events.schema.json", + wantDisplay: "schemas/events.schema.json", + }, + { + name: "dot root rejects parent escape", + root: ".", + baseDir: ".", + userPath: "../outside.tmpl", + wantErr: "escapes source root", + }, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + gotPath, gotDisplay, err := ResolveFSPath(tc.root, tc.baseDir, tc.userPath) + if tc.wantErr != "" { + if err == nil { + t.Fatalf("expected error containing %q", tc.wantErr) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("expected error to contain %q, got %v", tc.wantErr, err) + } + return + } + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if gotPath != tc.wantPath || gotDisplay != tc.wantDisplay { + t.Fatalf("expected path/display %q/%q, got %q/%q", tc.wantPath, tc.wantDisplay, gotPath, gotDisplay) + } + }) + } +} + func TestStemStripsYAMLExtensions(t *testing.T) { tests := []struct { name string diff --git a/internal/promptdef/filesystem_repository.go b/internal/promptdef/filesystem_repository.go index 00234f3..54009e7 100644 --- a/internal/promptdef/filesystem_repository.go +++ b/internal/promptdef/filesystem_repository.go @@ -193,6 +193,11 @@ func loadPromptDefinition(ctx context.Context, fsys fs.FS, root string, id strin if err != nil { return nil, fmt.Errorf("failed to read prompt definition directory: %w", err) } + cleanRoot := filecatalog.CleanFSRoot(root) + rootInfo, err := fs.Stat(fsys, cleanRoot) + if err != nil { + return nil, fmt.Errorf("failed to read prompt definition directory: %w", err) + } var matches []promptDefinitionMatch for _, fullPath := range files { @@ -220,7 +225,7 @@ func loadPromptDefinition(ctx context.Context, fsys fs.FS, root string, id strin continue } - def, err := normalizePromptDefinitionFromFS(raw, fsys, fullPath) + def, err := normalizePromptDefinitionFromFS(raw, fsys, root, fullPath, rootInfo.IsDir()) if err != nil { if fileMatch || strings.TrimSpace(raw.ID) == id { return nil, fmt.Errorf("%w: %s: %v", ErrInvalidPromptDefinition, relPath, err) @@ -295,14 +300,23 @@ func normalizePromptDefinition(raw *promptDefinitionFile, sourcePath string) (*d }) } -func normalizePromptDefinitionFromFS(raw *promptDefinitionFile, fsys fs.FS, sourcePath string) (*domain.PromptDefinition, error) { +func normalizePromptDefinitionFromFS(raw *promptDefinitionFile, fsys fs.FS, root string, sourcePath string, rootIsDir bool) (*domain.PromptDefinition, error) { promptDir := path.Dir(sourcePath) return normalizePromptDefinitionWithContent(raw, func(contentFile string) (string, string, error) { - resolvedPath := strings.TrimSpace(contentFile) - if !path.IsAbs(resolvedPath) { - resolvedPath = path.Join(promptDir, resolvedPath) + var resolvedPath string + if rootIsDir { + var err error + resolvedPath, _, err = filecatalog.ResolveFSPath(root, promptDir, contentFile) + if err != nil { + return "", "", err + } + } else { + resolvedPath = strings.TrimSpace(contentFile) + if !path.IsAbs(resolvedPath) { + resolvedPath = path.Join(promptDir, resolvedPath) + } + resolvedPath = strings.TrimPrefix(path.Clean(resolvedPath), "/") } - resolvedPath = strings.TrimPrefix(path.Clean(resolvedPath), "/") body, err := fs.ReadFile(fsys, resolvedPath) if err != nil { diff --git a/internal/promptdef/repository_test.go b/internal/promptdef/repository_test.go index 9551104..2bbeb12 100644 --- a/internal/promptdef/repository_test.go +++ b/internal/promptdef/repository_test.go @@ -359,6 +359,69 @@ output: } } +func TestFSRepositoryContentFileContainment(t *testing.T) { + t.Run("nested prompt can reference file inside root", func(t *testing.T) { + repo := NewFSRepository(fstest.MapFS{ + "prompts/nested/prompt.yaml": &fstest.MapFile{Data: []byte(` +id: fs-contained-prompt +version: "1.0.0" +messages: + - role: user + content_file: ../shared/user.tmpl +output: + format: markdown + validation_mode: basic + repair_attempts: 0 +`)}, + "prompts/shared/user.tmpl": &fstest.MapFile{Data: []byte(`Inside root.`)}, + }, "prompts") + + got, err := repo.GetPromptDefinition(context.Background(), "fs-contained-prompt", "") + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if len(got.Templates) != 1 || got.Templates[0].Content != "Inside root." { + t.Fatalf("expected contained content file, got %+v", got.Templates) + } + }) + + tests := []struct { + name string + contentFile string + wantErr string + }{ + {name: "parent escape rejected", contentFile: "../outside.tmpl", wantErr: "escapes source root"}, + {name: "absolute path rejected", contentFile: "/outside.tmpl", wantErr: "must be relative"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + repo := NewFSRepository(fstest.MapFS{ + "prompts/prompt.yaml": &fstest.MapFile{Data: []byte(` +id: fs-escaped-prompt +version: "1.0.0" +messages: + - role: user + content_file: ` + tc.contentFile + ` +output: + format: markdown + validation_mode: basic + repair_attempts: 0 +`)}, + "outside.tmpl": &fstest.MapFile{Data: []byte(`Outside root.`)}, + }, "prompts") + + _, err := repo.GetPromptDefinition(context.Background(), "fs-escaped-prompt", "") + if !errors.Is(err, ErrInvalidPromptDefinition) { + t.Fatalf("expected ErrInvalidPromptDefinition, got %v", err) + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("expected error to contain %q, got %v", tc.wantErr, err) + } + }) + } +} + func TestFSRepositoryRejectsDuplicatePromptIDs(t *testing.T) { repo := NewFSRepository(fstest.MapFS{ "one.yaml": &fstest.MapFile{Data: []byte(` diff --git a/internal/validate/standard_validator.go b/internal/validate/standard_validator.go index d08a9ed..5e1ff7f 100644 --- a/internal/validate/standard_validator.go +++ b/internal/validate/standard_validator.go @@ -12,6 +12,7 @@ import ( "strings" "gitea.maximumdirect.net/eric/scriptorium/internal/domain" + "gitea.maximumdirect.net/eric/scriptorium/internal/filecatalog" "github.com/santhosh-tekuri/jsonschema/v6" ) @@ -244,17 +245,24 @@ func (v *FSValidator) resolveSchemaPath(schemaPath string) (string, error) { return "", errors.New("schema filesystem is nil") } - cleanRoot := cleanFSRoot(v.root) + cleanRoot := filecatalog.CleanFSRoot(v.root) rootInfo, err := fs.Stat(v.fsys, cleanRoot) if err != nil { return "", fmt.Errorf("failed to access schema source %q: %w", cleanRoot, err) } - cleanSchemaPath := cleanSchemaFSPath(schemaPath) var resolved string if rootInfo.IsDir() { - resolved = path.Join(cleanRoot, cleanSchemaPath) + resolvedPath, _, err := filecatalog.ResolveFSPath(cleanRoot, cleanRoot, schemaPath) + if err != nil { + return "", err + } + resolved = resolvedPath } else { + cleanSchemaPath, err := cleanSchemaFSPath(schemaPath) + if err != nil { + return "", err + } if cleanSchemaPath != path.Base(cleanRoot) { return "", fmt.Errorf("schema path %q does not match schema file %q", cleanSchemaPath, path.Base(cleanRoot)) } @@ -267,18 +275,16 @@ func (v *FSValidator) resolveSchemaPath(schemaPath string) (string, error) { return resolved, nil } -func cleanSchemaFSPath(schemaPath string) string { +func cleanSchemaFSPath(schemaPath string) (string, error) { cleaned := strings.TrimSpace(schemaPath) - cleaned = strings.TrimPrefix(path.Clean(cleaned), "/") - return cleaned -} - -func cleanFSRoot(root string) string { - root = strings.TrimSpace(root) - if root == "" || root == "." { - return "." + if cleaned == "" { + return "", errors.New("schema path is required for json_schema validation") } - return strings.TrimPrefix(path.Clean(root), "/") + cleaned = path.Clean(cleaned) + if path.IsAbs(cleaned) { + return "", fmt.Errorf("schema path %q must be relative", schemaPath) + } + return cleaned, nil } func fsSchemaResourceURL(schemaName string) string { diff --git a/internal/validate/standard_validator_test.go b/internal/validate/standard_validator_test.go index b68a971..d404a5b 100644 --- a/internal/validate/standard_validator_test.go +++ b/internal/validate/standard_validator_test.go @@ -4,6 +4,7 @@ import ( "context" "os" "path/filepath" + "strings" "testing" "testing/fstest" @@ -276,6 +277,61 @@ func TestFSValidatorJSONSchemaSuccess(t *testing.T) { } } +func TestFSValidatorJSONSchemaPathContainment(t *testing.T) { + t.Run("nested schema inside root succeeds", func(t *testing.T) { + v := NewFSValidator(fstest.MapFS{ + "schemas/nested/events.schema.json": &fstest.MapFile{Data: []byte(`{ + "type": "object", + "required": ["events"], + "properties": { + "events": {"type": "array"} + } +}`)}, + }, "schemas") + + res, err := v.Validate(context.Background(), &domain.Artifact{Body: []byte(`{"events":[]}`)}, domain.OutputContract{ + ValidationMode: domain.ValidationJSONSchema, + SchemaPath: "nested/events.schema.json", + }) + if err != nil { + t.Fatalf("expected no error, got %v", err) + } + if res.Status != domain.ValidationPassed || !res.IsValid { + t.Fatalf("expected passed/valid, got status=%q valid=%v", res.Status, res.IsValid) + } + }) + + tests := []struct { + name string + schemaPath string + wantErr string + }{ + {name: "parent escape rejected", schemaPath: "../outside.schema.json", wantErr: "escapes source root"}, + {name: "absolute path rejected", schemaPath: "/outside.schema.json", wantErr: "must be relative"}, + } + + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + v := NewFSValidator(fstest.MapFS{ + "schemas/events.schema.json": &fstest.MapFile{Data: []byte(`{"type":"object"}`)}, + "outside.schema.json": &fstest.MapFile{Data: []byte(`{"type":"object"}`)}, + "schemas/outside.schema.json": &fstest.MapFile{Data: []byte(`{"type":"object"}`)}, + }, "schemas") + + _, err := v.Validate(context.Background(), &domain.Artifact{Body: []byte(`{"events":[]}`)}, domain.OutputContract{ + ValidationMode: domain.ValidationJSONSchema, + SchemaPath: tc.schemaPath, + }) + if err == nil { + t.Fatal("expected schema path error") + } + if !strings.Contains(err.Error(), tc.wantErr) { + t.Fatalf("expected error to contain %q, got %v", tc.wantErr, err) + } + }) + } +} + func TestFSValidatorSingleSchemaFileUsesBaseName(t *testing.T) { v := NewFSValidator(fstest.MapFS{ "events.schema.json": &fstest.MapFile{Data: []byte(`{