diff --git a/docs/cli.md b/docs/cli.md index c0f558d..fe12565 100644 --- a/docs/cli.md +++ b/docs/cli.md @@ -83,9 +83,12 @@ Notes: - `--prompt-dir `: prompt definition directory. - `--profile-dir `: custom profile definition directory. - `--schema-dir `: schema base directory for `json_schema` validation. +- `--artifact-root `: base directory for HTTP `file` input references. Notes: - `serve` does not accept runtime model override flags such as `--model` or `--llm-base-url`. +- HTTP `file` input references are rejected unless an artifact root is configured through `server.artifact_root` or `--artifact-root`. +- `--artifact-root` affects only `serve`; `run` and `render` file input paths are unchanged. ## Input And Variable Syntax diff --git a/docs/config.md b/docs/config.md index 703c309..2c88438 100644 --- a/docs/config.md +++ b/docs/config.md @@ -34,6 +34,7 @@ schema_dir: /opt/scriptorium/schemas server: addr: 127.0.0.1:8080 + artifact_root: /var/lib/scriptorium/artifacts defaults: render_format: text @@ -47,12 +48,14 @@ Top-level fields: - `profile_dir` (optional): default custom profile definition directory. - `schema_dir` (optional): base directory for schema files used by `json_schema` validation. - `server.addr` (optional): default listen address for `serve`. +- `server.artifact_root` (optional): base directory for HTTP `file` input references. - `defaults.render_format` (optional): default `render` output format (`text` or `json`). Built-in defaults: - `schema_dir`: `.` - `server.addr`: `:8080` +- `server.artifact_root`: unset; HTTP `file` input references are rejected until configured. - `defaults.render_format`: `text` Validation behavior: @@ -60,6 +63,15 @@ Validation behavior: - Config decoding is strict; unknown YAML fields are rejected. - Raw API key fields are not supported in `config.yml`. +HTTP artifact root behavior: + +- `server.artifact_root` applies only to `serve`. +- HTTP `inline` input references work without an artifact root. +- HTTP `file` input references are resolved against `server.artifact_root` and must stay inside it. +- Relative traversal and absolute paths outside the root are rejected. +- Symlinks inside the root are followed by the operating system; do not make the artifact root writable by untrusted users. +- CLI `run` and `render` file inputs keep their normal direct filesystem path behavior. + ## Prompt Definition Files Prompt definitions are YAML files anywhere under `prompt_dir`, including nested subdirectories. @@ -269,6 +281,9 @@ Rules: - Invalid generated JSON causes validation status `failed` (not a runtime error). Supported artifact reference types for request inputs are `file` and `inline`. +For HTTP `serve`, `file` references require `server.artifact_root` and must stay +inside that root. CLI `run` and `render` file inputs are not restricted by +`server.artifact_root`. ## Secrets Handling diff --git a/docs/integrations/http-api.md b/docs/integrations/http-api.md index 7bc3681..e3578e5 100644 --- a/docs/integrations/http-api.md +++ b/docs/integrations/http-api.md @@ -70,6 +70,12 @@ Input reference types currently supported by runtime artifact loading: - `file` - `inline` +HTTP `file` references require `server.artifact_root` or `serve --artifact-root`. +Relative file URIs resolve inside that root. Absolute file URIs are accepted +only when they remain inside the root. Requests that escape the root, including +`..` traversal and absolute paths outside the root, return +`400 artifact_not_allowed`. `inline` references do not require an artifact root. + Model override notes: - Numeric model override fields distinguish omitted values from explicit zero values. For example, omitting `temperature` preserves the selected profile/default value, while `"temperature": 0` explicitly sets the effective temperature to zero. @@ -194,6 +200,7 @@ Current error mapping (non-exhaustive): - `400 profile_required`: no explicit `profile_id` and prompt has no `default_profile` - `400 prompt_load_failed`: prompt definition invalid/unloadable - `400 profile_load_failed`: profile invalid/unloadable +- `400 artifact_not_allowed`: file input artifact is outside the configured artifact root or file refs are not enabled - `400 artifact_read_failed`: input artifact loading failed - `400 prompt_render_failed`: template render failed - `400 api_key_env_missing`: named API-key environment variable is missing diff --git a/docs/internal/adapters.md b/docs/internal/adapters.md index f9f8100..9089a9b 100644 --- a/docs/internal/adapters.md +++ b/docs/internal/adapters.md @@ -85,6 +85,7 @@ Primary app settings consumed by adapters: - `profile_dir` (optional custom profile source) - `schema_dir` - `server.addr` +- `server.artifact_root` (HTTP `serve` file input root) - `defaults.render_format` Execution profile/request settings used through runner: @@ -116,6 +117,10 @@ Artifact refs: - Supported reference types: `inline`, `file`. - Unsupported types return `ErrUnsupportedRefType`. +- CLI `run` and `render` use direct filesystem file reads for `file` references. +- HTTP `serve` uses a restricted artifact reader: `inline` references work without a root, while `file` references require `server.artifact_root` or `--artifact-root` and must stay inside that root. +- HTTP file paths are resolved with clean absolute paths and containment checks, not string-prefix checks. +- Symlinks inside the root are followed by the operating system; the configured root must not be writable by untrusted users. LLM adapter: diff --git a/docs/operations.md b/docs/operations.md index 1d6d690..f39b179 100644 --- a/docs/operations.md +++ b/docs/operations.md @@ -35,6 +35,7 @@ Built-in defaults relevant to operations: - `schema_dir: .` - `server.addr: :8080` +- `server.artifact_root`: unset; HTTP `file` input references are rejected until configured - `defaults.render_format: text` ## Normal CLI Workflow @@ -75,11 +76,14 @@ Current inbound API behavior: - Route: `POST /v1/runs` - JSON request parsing rejects unknown fields. - Validation content failures still return `200 OK` with `validation.status: "failed"`. +- `inline` input references work without filesystem configuration. +- `file` input references require `server.artifact_root` or `serve --artifact-root`; relative paths resolve inside that root and paths outside it are rejected. Security caveat: - `serve` has no built-in authentication or authorization. - Deploy only behind trusted controls (private network boundary, authenticated reverse proxy, API gateway, or equivalent). +- Keep the HTTP artifact root as narrow as practical and do not make it writable by untrusted users. ## Output, Logs, And Exit Codes diff --git a/examples/config.yml b/examples/config.yml index 759e95b..353b2cc 100644 --- a/examples/config.yml +++ b/examples/config.yml @@ -4,6 +4,7 @@ schema_dir: ./examples/schemas server: addr: :8080 + artifact_root: . defaults: render_format: text diff --git a/internal/adapter/cli/run.go b/internal/adapter/cli/run.go index 18b5e8d..6aa8568 100644 --- a/internal/adapter/cli/run.go +++ b/internal/adapter/cli/run.go @@ -74,10 +74,11 @@ type renderConfig struct { type serveConfig struct { configPath string - addr string - promptDir string - profileDir string - schemaDir string + addr string + promptDir string + profileDir string + schemaDir string + artifactRoot string } type commonCommandSettings struct { @@ -85,6 +86,7 @@ type commonCommandSettings struct { profileDir string schemaDir string serverAddr string + artifactRoot string defaultRenderFormat renderformat.PreparedRunOutputFormat } @@ -202,7 +204,13 @@ func serveCommand(args []string, stderr io.Writer) int { return ExitRuntimeError } - runner := newRunner(cfg.promptDir, cfg.profileDir, cfg.schemaDir, llmClient) + artifactReader, err := artifactadapter.NewRestrictedCompositeReader(cfg.artifactRoot) + if err != nil { + fmt.Fprintf(stderr, "artifact root error: %v\n", err) + return ExitRuntimeError + } + + runner := newRunnerWithArtifactReader(cfg.promptDir, cfg.profileDir, cfg.schemaDir, llmClient, artifactReader) h := httpadapter.NewHandler(runner) srv := &http.Server{ @@ -281,6 +289,7 @@ func parseServeArgs(args []string) (*serveConfig, error) { fs.StringVar(&cfg.promptDir, "prompt-dir", "", "directory containing prompt definition YAML files") fs.StringVar(&cfg.profileDir, "profile-dir", "", "directory containing execution profile YAML files") fs.StringVar(&cfg.schemaDir, "schema-dir", "", "base directory for validation schemas") + fs.StringVar(&cfg.artifactRoot, "artifact-root", "", "base directory for HTTP file input artifacts") if err := fs.Parse(args); err != nil { return nil, err @@ -290,10 +299,11 @@ func parseServeArgs(args []string) (*serveConfig, error) { } settings, err := resolveCommonSettings(fs, cfg.configPath, appconfig.CLIOverrides{ - PromptDir: cfg.promptDirIfSet(fs), - ProfileDir: cfg.profileDirIfSet(fs), - SchemaDir: cfg.schemaDirIfSet(fs), - ServerAddr: cfg.addrIfSet(fs), + PromptDir: cfg.promptDirIfSet(fs), + ProfileDir: cfg.profileDirIfSet(fs), + SchemaDir: cfg.schemaDirIfSet(fs), + ServerAddr: cfg.addrIfSet(fs), + ArtifactRoot: cfg.artifactRootIfSet(fs), }) if err != nil { return nil, err @@ -303,6 +313,7 @@ func parseServeArgs(args []string) (*serveConfig, error) { cfg.profileDir = settings.profileDir cfg.schemaDir = settings.schemaDir cfg.addr = settings.serverAddr + cfg.artifactRoot = settings.artifactRoot if err := validateRequiredLibraryDirs(cfg.promptDir); err != nil { return nil, err @@ -313,6 +324,9 @@ func parseServeArgs(args []string) (*serveConfig, error) { cfg.profileDir = filepath.Clean(cfg.profileDir) } cfg.schemaDir = filepath.Clean(cfg.schemaDir) + if strings.TrimSpace(cfg.artifactRoot) != "" { + cfg.artifactRoot = filepath.Clean(cfg.artifactRoot) + } return cfg, nil } @@ -429,6 +443,13 @@ func (c *serveConfig) addrIfSet(fs *flag.FlagSet) string { return "" } +func (c *serveConfig) artifactRootIfSet(fs *flag.FlagSet) string { + if flagWasSet(fs, "artifact-root") { + return c.artifactRoot + } + return "" +} + func registerConfigPathFlag(fs *flag.FlagSet, target *string) { fs.StringVar( target, @@ -466,6 +487,7 @@ func resolveCommonSettings(fs *flag.FlagSet, configPath string, overrides appcon profileDir: settings.ProfileDir, schemaDir: settings.SchemaDir, serverAddr: settings.ServerAddr, + artifactRoot: settings.ArtifactRoot, defaultRenderFormat: settings.DefaultRenderFormat, }, nil } @@ -478,10 +500,17 @@ func validateRequiredLibraryDirs(promptDir string) error { } func newRunner(promptDir, profileDir, schemaDir string, llmClient llm.Client) *usecase.Runner { + return newRunnerWithArtifactReader(promptDir, profileDir, schemaDir, llmClient, artifactadapter.NewCompositeReader()) +} + +func newRunnerWithArtifactReader(promptDir, profileDir, schemaDir string, llmClient llm.Client, artifactReader artifactadapter.Reader) *usecase.Runner { + if artifactReader == nil { + artifactReader = artifactadapter.NewCompositeReader() + } return usecase.NewRunner( promptdef.NewFilesystemRepository(promptDir), builtin.NewRepositoryWithDirectory(profileDir), - artifactadapter.NewCompositeReader(), + artifactReader, prompt.NewGoRenderer(), llmClient, validate.NewStandardValidator(schemaDir), diff --git a/internal/adapter/cli/run_test.go b/internal/adapter/cli/run_test.go index 1dbd9d4..cc141e7 100644 --- a/internal/adapter/cli/run_test.go +++ b/internal/adapter/cli/run_test.go @@ -435,11 +435,13 @@ profile_dir: ./from-config/profiles schema_dir: ./from-config/schemas server: addr: 127.0.0.1:9000 + artifact_root: ./from-config/artifacts `) cfg, err := parseServeArgs([]string{ "--config", configPath, "--addr", ":7777", + "--artifact-root", "./from-cli/artifacts", }) if err != nil { t.Fatalf("expected valid args, got %v", err) @@ -457,6 +459,9 @@ server: if cfg.addr != ":7777" { t.Fatalf("expected CLI addr override, got %q", cfg.addr) } + if cfg.artifactRoot != filepath.Clean("./from-cli/artifacts") { + t.Fatalf("expected CLI artifact root override, got %q", cfg.artifactRoot) + } } func TestParseServeArgsWithConfigProvidesRequiredDirectoriesAndAddr(t *testing.T) { @@ -466,6 +471,7 @@ profile_dir: ./from-config/profiles schema_dir: ./from-config/schemas server: addr: 127.0.0.1:9000 + artifact_root: ./from-config/artifacts `) cfg, err := parseServeArgs([]string{ @@ -487,6 +493,9 @@ server: if cfg.addr != "127.0.0.1:9000" { t.Fatalf("expected addr from config, got %q", cfg.addr) } + if cfg.artifactRoot != filepath.Clean("./from-config/artifacts") { + t.Fatalf("expected artifact root from config, got %q", cfg.artifactRoot) + } } func TestRunAndRenderBuildEquivalentRuntimeOverrideRequestsForSharedFlags(t *testing.T) { diff --git a/internal/adapter/http/handler.go b/internal/adapter/http/handler.go index 242c64a..5c06e04 100644 --- a/internal/adapter/http/handler.go +++ b/internal/adapter/http/handler.go @@ -7,6 +7,7 @@ import ( "net/http" "strings" + "gitea.maximumdirect.net/eric/scriptorium/internal/artifact" "gitea.maximumdirect.net/eric/scriptorium/internal/domain" "gitea.maximumdirect.net/eric/scriptorium/internal/profile" "gitea.maximumdirect.net/eric/scriptorium/internal/promptdef" @@ -187,6 +188,8 @@ func mapRunError(err error) (int, string, string) { return http.StatusBadRequest, "prompt_load_failed", "failed to load prompt definition" case errors.Is(err, usecase.ErrProfileLoad): return http.StatusBadRequest, "profile_load_failed", "failed to load execution profile" + case errors.Is(err, artifact.ErrFileNotAllowed), errors.Is(err, artifact.ErrFileOutsideRoot): + return http.StatusBadRequest, "artifact_not_allowed", "file input artifact is not allowed" case errors.Is(err, usecase.ErrArtifactLoad): return http.StatusBadRequest, "artifact_read_failed", "failed to read input artifact" case errors.Is(err, usecase.ErrPromptRender): diff --git a/internal/adapter/http/handler_test.go b/internal/adapter/http/handler_test.go index 7e2fdea..6b60238 100644 --- a/internal/adapter/http/handler_test.go +++ b/internal/adapter/http/handler_test.go @@ -7,11 +7,14 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" + "path/filepath" "reflect" "strings" "testing" "time" + "gitea.maximumdirect.net/eric/scriptorium/internal/artifact" "gitea.maximumdirect.net/eric/scriptorium/internal/domain" "gitea.maximumdirect.net/eric/scriptorium/internal/llm" "gitea.maximumdirect.net/eric/scriptorium/internal/profile" @@ -61,6 +64,12 @@ func (handlerRenderer) Render(ctx context.Context, definition *domain.PromptDefi return &domain.RenderedPrompt{Messages: []domain.RenderedMessage{{Role: "user", Content: "hi"}}}, nil } +type handlerLLMClient struct{} + +func (handlerLLMClient) Generate(ctx context.Context, req domain.GenerateRequest) (*domain.GenerateResponse, error) { + return &domain.GenerateResponse{Content: "ok"}, nil +} + func TestHandlerPostRunsSuccessWithExplicitProfileID(t *testing.T) { start := time.Now().UTC() end := start.Add(2 * time.Second) @@ -184,6 +193,87 @@ func TestHandlerPostRunsSuccessWithExplicitProfileID(t *testing.T) { } } +func TestHandlerInlineRefsWorkWithoutArtifactRoot(t *testing.T) { + h := newArtifactRootHandler(t, "") + + req := httptest.NewRequest(http.MethodPost, "/v1/runs", bytes.NewBufferString(`{ + "prompt_id":"p", + "inputs":{"x":{"type":"inline","body":"inline body"}} + }`)) + w := httptest.NewRecorder() + + h.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d body=%s", w.Code, w.Body.String()) + } +} + +func TestHandlerFileRefsWithoutArtifactRootAreRejected(t *testing.T) { + h := newArtifactRootHandler(t, "") + + req := httptest.NewRequest(http.MethodPost, "/v1/runs", bytes.NewBufferString(`{ + "prompt_id":"p", + "inputs":{"x":{"type":"file","uri":"input.txt"}} + }`)) + w := httptest.NewRecorder() + + h.ServeHTTP(w, req) + + assertHTTPErrorCode(t, w, http.StatusBadRequest, "artifact_not_allowed") +} + +func TestHandlerFileRefsUnderArtifactRootWork(t *testing.T) { + root := t.TempDir() + if err := os.WriteFile(filepath.Join(root, "input.txt"), []byte("allowed"), 0o644); err != nil { + t.Fatal(err) + } + h := newArtifactRootHandler(t, root) + + req := httptest.NewRequest(http.MethodPost, "/v1/runs", bytes.NewBufferString(`{ + "prompt_id":"p", + "inputs":{"x":{"type":"file","uri":"input.txt"}} + }`)) + w := httptest.NewRecorder() + + h.ServeHTTP(w, req) + + if w.Code != http.StatusOK { + t.Fatalf("expected 200, got %d body=%s", w.Code, w.Body.String()) + } +} + +func TestHandlerFileRefsOutsideArtifactRootAreRejected(t *testing.T) { + root := t.TempDir() + outside := t.TempDir() + if err := os.WriteFile(filepath.Join(outside, "secret.txt"), []byte("denied"), 0o644); err != nil { + t.Fatal(err) + } + h := newArtifactRootHandler(t, root) + + tests := []struct { + name string + uri string + }{ + {name: "relative traversal", uri: filepath.Join("..", filepath.Base(outside), "secret.txt")}, + {name: "absolute outside root", uri: filepath.Join(outside, "secret.txt")}, + } + for _, tc := range tests { + t.Run(tc.name, func(t *testing.T) { + body := fmt.Sprintf(`{ + "prompt_id":"p", + "inputs":{"x":{"type":"file","uri":%q}} + }`, tc.uri) + req := httptest.NewRequest(http.MethodPost, "/v1/runs", bytes.NewBufferString(body)) + w := httptest.NewRecorder() + + h.ServeHTTP(w, req) + + assertHTTPErrorCode(t, w, http.StatusBadRequest, "artifact_not_allowed") + }) + } +} + func TestHandlerPostRunsSuccessUsingPromptDefaultProfile(t *testing.T) { r := &fakeRunner{result: &domain.RunResult{ Artifact: domain.Artifact{Body: []byte("ok")}, @@ -676,3 +766,48 @@ func TestHandlerValidationFailureStillSuccessAndRawOutputOptIn(t *testing.T) { func wrap(stage error, cause error) error { return fmt.Errorf("%w: %w", stage, cause) } + +func newArtifactRootHandler(t *testing.T, root string) *Handler { + t.Helper() + + reader, err := artifact.NewRestrictedCompositeReader(root) + if err != nil { + t.Fatalf("expected restricted artifact reader: %v", err) + } + runner := usecase.NewRunner( + handlerPromptRepo{def: &domain.PromptDefinition{ + ID: "p", + Version: "1", + DefaultProfile: "exec", + Templates: []domain.PromptMessageTemplate{{Role: "user", Content: "hi"}}, + OutputFormat: domain.FormatText, + Validation: domain.OutputContract{Format: domain.FormatText, ValidationMode: domain.ValidationNone}, + }}, + handlerProfileRepo{profile: &domain.ExecutionProfile{ + ID: "exec", + Endpoint: "http://example.invalid/v1", + Model: "model", + }}, + reader, + handlerRenderer{}, + handlerLLMClient{}, + nil, + ) + return NewHandler(runner) +} + +func assertHTTPErrorCode(t *testing.T, w *httptest.ResponseRecorder, status int, code string) { + t.Helper() + + if w.Code != status { + t.Fatalf("expected %d, got %d body=%s", status, w.Code, w.Body.String()) + } + var resp map[string]any + if err := json.Unmarshal(w.Body.Bytes(), &resp); err != nil { + t.Fatalf("invalid JSON response: %v", err) + } + errBody := resp["error"].(map[string]any) + if errBody["code"] != code { + t.Fatalf("expected code %q, got %#v", code, errBody["code"]) + } +} diff --git a/internal/artifact/reader.go b/internal/artifact/reader.go index 5863316..7c70c2e 100644 --- a/internal/artifact/reader.go +++ b/internal/artifact/reader.go @@ -10,12 +10,15 @@ import ( "mime" "os" "path/filepath" + "strings" ) var ( ErrUnsupportedRefType = errors.New("unsupported artifact reference type") ErrMissingInlineBody = errors.New("missing body for inline artifact") ErrMissingFilePath = errors.New("missing file path for file artifact") + ErrFileNotAllowed = errors.New("file artifact references are not allowed") + ErrFileOutsideRoot = errors.New("file artifact path is outside artifact root") ) // Reader resolves artifact references into actual artifacts. @@ -26,7 +29,7 @@ type Reader interface { // CompositeReader routes artifact resolution based on the reference type. type CompositeReader struct { inlineReader *inlineReader - fileReader *fileReader + fileReader Reader } func NewCompositeReader() Reader { @@ -36,6 +39,17 @@ func NewCompositeReader() Reader { } } +func NewRestrictedCompositeReader(root string) (Reader, error) { + fileReader, err := newRestrictedFileReader(root) + if err != nil { + return nil, err + } + return &CompositeReader{ + inlineReader: &inlineReader{}, + fileReader: fileReader, + }, nil +} + func (c *CompositeReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domain.Artifact, error) { select { case <-ctx.Done(): @@ -89,21 +103,99 @@ func (r *fileReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domain. return nil, ErrMissingFilePath } - data, err := os.ReadFile(ref.URI) - if err != nil { - return nil, fmt.Errorf("failed to read file %s: %w", ref.URI, err) + return readFileArtifact(ref.URI) +} + +type deniedFileReader struct{} + +func (r deniedFileReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domain.Artifact, error) { + select { + case <-ctx.Done(): + return nil, ctx.Err() + default: } - contentType := mime.TypeByExtension(filepath.Ext(ref.URI)) + if ref.URI == "" { + return nil, ErrMissingFilePath + } + return nil, ErrFileNotAllowed +} + +type restrictedFileReader struct { + root string +} + +func newRestrictedFileReader(root string) (Reader, error) { + cleanRoot := strings.TrimSpace(root) + if cleanRoot == "" { + return deniedFileReader{}, nil + } + absRoot, err := filepath.Abs(filepath.Clean(cleanRoot)) + if err != nil { + return nil, fmt.Errorf("resolve artifact root: %w", err) + } + return &restrictedFileReader{root: absRoot}, nil +} + +func (r *restrictedFileReader) Read(ctx context.Context, ref domain.ArtifactRef) (*domain.Artifact, error) { + select { + case <-ctx.Done(): + return nil, ctx.Err() + default: + } + + if ref.URI == "" { + return nil, ErrMissingFilePath + } + + path, err := r.resolve(ref.URI) + if err != nil { + return nil, err + } + return readFileArtifact(path) +} + +func (r *restrictedFileReader) resolve(rawPath string) (string, error) { + cleanPath := filepath.Clean(strings.TrimSpace(rawPath)) + var candidate string + if filepath.IsAbs(cleanPath) { + candidate = cleanPath + } else { + candidate = filepath.Join(r.root, cleanPath) + } + + absCandidate, err := filepath.Abs(candidate) + if err != nil { + return "", fmt.Errorf("resolve artifact path: %w", err) + } + absCandidate = filepath.Clean(absCandidate) + + rel, err := filepath.Rel(r.root, absCandidate) + if err != nil { + return "", fmt.Errorf("compare artifact path to root: %w", err) + } + if rel == ".." || strings.HasPrefix(rel, ".."+string(filepath.Separator)) || filepath.IsAbs(rel) { + return "", ErrFileOutsideRoot + } + return absCandidate, nil +} + +func readFileArtifact(path string) (*domain.Artifact, error) { + data, err := os.ReadFile(path) + if err != nil { + return nil, fmt.Errorf("failed to read file %s: %w", path, err) + } + + contentType := mime.TypeByExtension(filepath.Ext(path)) if contentType == "" { contentType = defaults.ContentTypeTextPlain } return &domain.Artifact{ - Name: filepath.Base(ref.URI), + Name: filepath.Base(path), ContentType: contentType, Body: data, - URI: ref.URI, + URI: path, Size: int64(len(data)), Hash: fmt.Sprintf("%x", sha256.Sum256(data)), }, nil diff --git a/internal/artifact/reader_test.go b/internal/artifact/reader_test.go index 442e811..7b8cff0 100644 --- a/internal/artifact/reader_test.go +++ b/internal/artifact/reader_test.go @@ -4,6 +4,7 @@ import ( "context" "errors" "os" + "path/filepath" "testing" "gitea.maximumdirect.net/eric/scriptorium/internal/domain" @@ -56,6 +57,81 @@ func TestCompositeReader_Read(t *testing.T) { }) } +func TestRestrictedCompositeReader(t *testing.T) { + ctx := context.Background() + root := t.TempDir() + outside := t.TempDir() + + if err := os.WriteFile(filepath.Join(root, "input.txt"), []byte("allowed"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Mkdir(filepath.Join(root, "nested"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(outside, "secret.txt"), []byte("denied"), 0o644); err != nil { + t.Fatal(err) + } + + reader, err := NewRestrictedCompositeReader(root) + if err != nil { + t.Fatalf("expected restricted reader construction, got %v", err) + } + + t.Run("accepts relative contained path", func(t *testing.T) { + art, err := reader.Read(ctx, domain.ArtifactRef{Type: domain.ArtifactRefFile, URI: "nested/../input.txt"}) + if err != nil { + t.Fatalf("expected contained relative path to succeed, got %v", err) + } + if string(art.Body) != "allowed" { + t.Fatalf("unexpected artifact body: %q", string(art.Body)) + } + }) + + t.Run("accepts absolute contained path", func(t *testing.T) { + art, err := reader.Read(ctx, domain.ArtifactRef{Type: domain.ArtifactRefFile, URI: filepath.Join(root, "input.txt")}) + if err != nil { + t.Fatalf("expected contained absolute path to succeed, got %v", err) + } + if art.Name != "input.txt" { + t.Fatalf("unexpected artifact name: %q", art.Name) + } + }) + + t.Run("rejects relative traversal outside root", func(t *testing.T) { + _, err := reader.Read(ctx, domain.ArtifactRef{Type: domain.ArtifactRefFile, URI: filepath.Join("..", filepath.Base(outside), "secret.txt")}) + if !errors.Is(err, ErrFileOutsideRoot) { + t.Fatalf("expected ErrFileOutsideRoot, got %v", err) + } + }) + + t.Run("rejects absolute path outside root", func(t *testing.T) { + _, err := reader.Read(ctx, domain.ArtifactRef{Type: domain.ArtifactRefFile, URI: filepath.Join(outside, "secret.txt")}) + if !errors.Is(err, ErrFileOutsideRoot) { + t.Fatalf("expected ErrFileOutsideRoot, got %v", err) + } + }) +} + +func TestRestrictedCompositeReaderWithoutRootDeniesFileRefs(t *testing.T) { + reader, err := NewRestrictedCompositeReader("") + if err != nil { + t.Fatalf("expected restricted reader construction, got %v", err) + } + + art, err := reader.Read(context.Background(), domain.ArtifactRef{Type: domain.ArtifactRefInline, Body: "inline"}) + if err != nil { + t.Fatalf("expected inline ref to work without artifact root, got %v", err) + } + if string(art.Body) != "inline" { + t.Fatalf("unexpected inline body: %q", string(art.Body)) + } + + _, err = reader.Read(context.Background(), domain.ArtifactRef{Type: domain.ArtifactRefFile, URI: "input.txt"}) + if !errors.Is(err, ErrFileNotAllowed) { + t.Fatalf("expected ErrFileNotAllowed, got %v", err) + } +} + func TestFileReader_Read(t *testing.T) { content := []byte("test file content") tmpFile, err := os.CreateTemp("", "artifact_test_*.txt") diff --git a/internal/config/config.go b/internal/config/config.go index 59527c6..3334924 100644 --- a/internal/config/config.go +++ b/internal/config/config.go @@ -40,7 +40,8 @@ type Config struct { } type ServerConfig struct { - Addr string `yaml:"addr"` + Addr string `yaml:"addr"` + ArtifactRoot string `yaml:"artifact_root"` } type DefaultsConfig struct { @@ -53,6 +54,7 @@ type AppSettings struct { ProfileDir string SchemaDir string ServerAddr string + ArtifactRoot string DefaultRenderFormat renderformat.PreparedRunOutputFormat } @@ -62,6 +64,7 @@ type CLIOverrides struct { ProfileDir string SchemaDir string ServerAddr string + ArtifactRoot string RenderFormat string } @@ -142,6 +145,9 @@ func ApplyCLIOverrides(base AppSettings, overrides CLIOverrides) (AppSettings, e if v := strings.TrimSpace(overrides.ServerAddr); v != "" { out.ServerAddr = v } + if v := strings.TrimSpace(overrides.ArtifactRoot); v != "" { + out.ArtifactRoot = filepath.Clean(v) + } if rawFormat := strings.TrimSpace(overrides.RenderFormat); rawFormat != "" { parsed, err := renderformat.ParsePreparedRunOutputFormat(rawFormat) if err != nil { @@ -181,6 +187,9 @@ func applyConfig(base AppSettings, cfg Config) (AppSettings, error) { if v := strings.TrimSpace(cfg.Server.Addr); v != "" { out.ServerAddr = v } + if v := strings.TrimSpace(cfg.Server.ArtifactRoot); v != "" { + out.ArtifactRoot = filepath.Clean(v) + } if rawFormat := strings.TrimSpace(cfg.Defaults.RenderFormat); rawFormat != "" { parsed, err := renderformat.ParsePreparedRunOutputFormat(rawFormat) if err != nil { diff --git a/internal/config/config_test.go b/internal/config/config_test.go index 0a813f0..1278846 100644 --- a/internal/config/config_test.go +++ b/internal/config/config_test.go @@ -92,6 +92,7 @@ profile_dir: ./profiles schema_dir: ./schemas server: addr: 127.0.0.1:9090 + artifact_root: ./artifacts defaults: render_format: json `) @@ -113,6 +114,9 @@ defaults: if got.ServerAddr != "127.0.0.1:9090" { t.Fatalf("unexpected server.addr: %q", got.ServerAddr) } + if got.ArtifactRoot != filepath.Clean("./artifacts") { + t.Fatalf("unexpected server.artifact_root: %q", got.ArtifactRoot) + } if got.DefaultRenderFormat != renderformat.PreparedRunFormatJSON { t.Fatalf("unexpected defaults.render_format: %q", got.DefaultRenderFormat) } @@ -185,6 +189,7 @@ func TestApplyCLIOverridesAppliesPrecedence(t *testing.T) { ProfileDir: "/from/config/profiles", SchemaDir: "/from/config/schemas", ServerAddr: ":1234", + ArtifactRoot: "/from/config/artifacts", DefaultRenderFormat: renderformat.PreparedRunFormatJSON, } @@ -193,6 +198,7 @@ func TestApplyCLIOverridesAppliesPrecedence(t *testing.T) { ProfileDir: "./profiles-cli", SchemaDir: "./schemas-cli", ServerAddr: ":8081", + ArtifactRoot: "./artifacts-cli", RenderFormat: "text", }) if err != nil { @@ -211,6 +217,9 @@ func TestApplyCLIOverridesAppliesPrecedence(t *testing.T) { if got.ServerAddr != ":8081" { t.Fatalf("unexpected server addr: %q", got.ServerAddr) } + if got.ArtifactRoot != filepath.Clean("./artifacts-cli") { + t.Fatalf("unexpected artifact root: %q", got.ArtifactRoot) + } if got.DefaultRenderFormat != renderformat.PreparedRunFormatText { t.Fatalf("unexpected render format: %q", got.DefaultRenderFormat) }