325 lines
16 KiB
Markdown
325 lines
16 KiB
Markdown
# Full Codebase Cleanup And Hardening Plan
|
|
|
|
This document is a decision-complete implementation plan for the current full-codebase audit findings. It is a planning document only. Implementation should proceed in stages and should not change unrelated behavior.
|
|
|
|
The goal is to polish and harden the public and internal code paths without expanding Scriptorium's scope. Keep the existing package boundaries unless a stage explicitly calls for a helper extraction.
|
|
|
|
## Guiding Decisions
|
|
|
|
- Treat configured `fs.FS` roots as real containment boundaries for public source options.
|
|
- Keep local directory-backed CLI behavior compatible unless this plan explicitly names a change.
|
|
- Keep HTTP `serve` minimal, but safe by default against accidental large request, file, and response bodies.
|
|
- Do not add HTTP authentication in this cleanup pass.
|
|
- Do not add new dependencies.
|
|
- Document only implemented behavior outside `docs/roadmap/`.
|
|
|
|
## Stage 1: Enforce Source-Root Containment For `fs.FS` Prompt And Schema Sources
|
|
|
|
Problem:
|
|
|
|
Public source options such as `WithPromptFS(fsys, root)` and `WithSchemaFS(fsys, root)` describe `root` as the source boundary, but prompt `content_file` and schema path resolution can clean `..` paths above that root when the supplied `fs.FS` is broader than the configured root.
|
|
|
|
Decision:
|
|
|
|
For public `fs.FS` source options, the configured root is a containment boundary. Prompt `content_file` paths and schema paths must resolve inside that root. Absolute paths and relative traversal that escape the root are invalid.
|
|
|
|
Implementation:
|
|
|
|
1. Add a small shared internal helper for `fs.FS` path resolution.
|
|
- Prefer `internal/filecatalog` if the helper naturally belongs with existing clean/display path utilities.
|
|
- Inputs should include a source root and a user path.
|
|
- It should trim whitespace, clean slash paths with `path.Clean`, reject empty paths where the caller requires a file, reject absolute paths, and reject any path whose clean form escapes the clean root.
|
|
- Use path-component checks, not string-prefix checks alone.
|
|
- Return both the resolved `fs.FS` path and a display path when useful for errors.
|
|
2. Update `internal/promptdef` `fsRepository` content-file loading.
|
|
- `content_file: ./local.tmpl` beside the prompt should continue to work.
|
|
- Nested prompt files should keep the existing relative-to-prompt-file behavior.
|
|
- `content_file` values that escape the configured `WithPromptFS` root should return a prompt-load error.
|
|
3. Update `internal/validate` `FSValidator` schema resolution.
|
|
- `WithSchemaFS(fsys, root)` should allow schema paths inside `root`.
|
|
- `WithSchemaFile(path)` should keep the existing single-file behavior: prompt `schema_path` must match the selected file's base name.
|
|
- Schema paths that escape the configured root should return validation/schema-load errors.
|
|
4. Preserve directory-backed compatibility unless a failing test reveals an inconsistency that must be fixed.
|
|
- `WithPromptFile(path)` should continue resolving `content_file` values relative to the selected prompt file's directory.
|
|
- `Config.SchemaDir` / CLI `--schema-dir` should keep documented behavior, including absolute `schema_path` support, because these are operator-controlled local filesystem paths.
|
|
|
|
Tests:
|
|
|
|
- Add `internal/promptdef` tests for `fs.FS` `content_file` traversal:
|
|
- sibling file inside root succeeds;
|
|
- nested file inside root succeeds;
|
|
- `../outside.tmpl` from a prompt under the root is rejected;
|
|
- absolute-style `/outside.tmpl` is rejected.
|
|
- Add public package tests through `WithPromptFS` proving escaped `content_file` returns `ErrPromptLoad`.
|
|
- Add `internal/validate` tests for `WithSchemaFS` traversal:
|
|
- schema inside root succeeds;
|
|
- `../outside.schema.json` is rejected;
|
|
- absolute-style paths are rejected.
|
|
- Keep existing `WithSchemaFile` tests passing.
|
|
|
|
Documentation:
|
|
|
|
- Update `docs/consumers/pkg-scriptorium.md` to state that `WithPromptFS` and `WithSchemaFS` roots are containment boundaries.
|
|
- Update `docs/config.md` only if directory-backed behavior changes. Otherwise leave its local-directory schema-path behavior intact.
|
|
- Update `docs/internal/adapters.md` if shared source-resolution behavior is documented there.
|
|
|
|
## Stage 2: Add HTTP Request, Artifact, And Response Size Limits
|
|
|
|
Problem:
|
|
|
|
HTTP `serve` decodes request bodies directly from `r.Body`, reads file artifacts fully into memory, and serializes generated artifact bodies fully into the response. This is acceptable for trusted small local use, but it is not hardened against accidental or hostile large inputs.
|
|
|
|
Decision:
|
|
|
|
Add configurable HTTP size limits with conservative defaults. Limits apply only to HTTP `serve`; CLI `run` and `render` keep existing direct filesystem behavior.
|
|
|
|
Default limits:
|
|
|
|
- `server.max_request_bytes`: 16 MiB.
|
|
- `server.max_artifact_bytes`: 16 MiB.
|
|
- `server.max_response_bytes`: 16 MiB.
|
|
|
|
Use `0` to disable a specific limit only where this is consistent with existing config style. Negative values are invalid config.
|
|
|
|
Implementation:
|
|
|
|
1. Add default constants in `internal/defaults`.
|
|
2. Extend app config in `internal/config`.
|
|
- Add `server.max_request_bytes`.
|
|
- Add `server.max_artifact_bytes`.
|
|
- Add `server.max_response_bytes`.
|
|
- Apply built-in defaults, config-file values, and CLI overrides according to existing precedence.
|
|
- Reject negative values.
|
|
3. Add `serve` CLI overrides.
|
|
- `--max-request-bytes`
|
|
- `--max-artifact-bytes`
|
|
- `--max-response-bytes`
|
|
- Keep these flags scoped to `serve`.
|
|
4. Extend HTTP handler construction.
|
|
- Add `httpadapter.HandlerOptions` with request and response limit fields.
|
|
- Keep `httpadapter.NewHandler(runner)` as a default constructor for existing tests and callers.
|
|
- Add `httpadapter.NewHandlerWithOptions(runner, options)` for `serve` wiring.
|
|
- `NewHandler(runner)` should apply built-in defaults.
|
|
- `NewHandlerWithOptions(runner, options)` should use the supplied values exactly, so `0` means disabled after config validation.
|
|
5. Limit request decoding.
|
|
- Wrap `r.Body` with `http.MaxBytesReader` when `max_request_bytes > 0`.
|
|
- Return `413 request_too_large` when decoding fails due to size.
|
|
- Continue returning `400 invalid_json` for malformed JSON.
|
|
- Ensure the decoder rejects trailing JSON tokens if it does not already.
|
|
6. Limit HTTP file artifact reads.
|
|
- Add a max-bytes option to the restricted HTTP artifact reader.
|
|
- Use `os.Open`, `Stat`, and `io.LimitReader` or equivalent instead of unbounded `os.ReadFile` for restricted HTTP file reads.
|
|
- If a file exceeds the configured limit, return a specific artifact error that maps to `413 artifact_too_large`.
|
|
- Keep inline artifact bodies covered by the request-body limit.
|
|
7. Limit HTTP response artifact bodies.
|
|
- Build the response DTO, marshal it to JSON bytes, and compare the final encoded response size against `max_response_bytes` when the limit is positive.
|
|
- Return `413 response_too_large` when the encoded response exceeds the configured limit.
|
|
- Do not truncate successful artifacts silently.
|
|
- Apply the same encoded-response check when `include_raw_output` is true.
|
|
|
|
Tests:
|
|
|
|
- Config tests:
|
|
- defaults are applied;
|
|
- config file values load;
|
|
- CLI overrides win;
|
|
- negative values are rejected.
|
|
- CLI tests:
|
|
- `serve` parses the three flags;
|
|
- `run` and `render` do not gain these flags.
|
|
- HTTP tests:
|
|
- oversized request body returns `413 request_too_large`;
|
|
- malformed JSON below the limit still returns `400 invalid_json`;
|
|
- successful request below the limit still works;
|
|
- oversized generated artifact returns the configured too-large error;
|
|
- `include_raw_output` does not bypass response limits.
|
|
- Artifact tests:
|
|
- restricted file reader accepts files at or below the limit;
|
|
- restricted file reader rejects files above the limit;
|
|
- unlimited mode with `0` keeps existing behavior.
|
|
|
|
Documentation:
|
|
|
|
- Update `docs/config.md` with the new server limit fields and defaults.
|
|
- Update `docs/cli.md` with the new `serve` flags.
|
|
- Update `docs/integrations/http-api.md` with `413` errors.
|
|
- Update `docs/operations.md` with sizing guidance.
|
|
- Update `docs/troubleshooting.md` with common size-limit failures.
|
|
- Update `docs/internal/adapters.md` with the HTTP limit boundary.
|
|
|
|
## Stage 3: Harden `OpenAICompatibleProfile` ExtraParams Copying
|
|
|
|
Problem:
|
|
|
|
`OpenAICompatibleProfile` currently deep-copies `ExtraParams` through the general internal copy helper. Cyclic caller-provided maps can recurse indefinitely before `WithProfiles` can validate and return `ErrInvalidConfig`.
|
|
|
|
Decision:
|
|
|
|
`OpenAICompatibleProfile` is a convenience constructor, not a validator. It must not recursively walk caller-provided `ExtraParams`. Validation and safe deep copying belong in `WithProfiles` through the existing public JSON validation path.
|
|
|
|
Implementation:
|
|
|
|
1. Change `OpenAICompatibleProfile` to use a shallow map copy for `ExtraParams`.
|
|
- Copy only the top-level `map[string]any`.
|
|
- Do not recursively copy nested values.
|
|
- Do not call `copyAnyMap` from this constructor.
|
|
2. Keep `WithProfiles` validation and deep-copy behavior unchanged.
|
|
- Unsupported values, non-finite numbers, non-string map keys, and cycles should still return `ErrInvalidConfig`.
|
|
3. Review `copyAnyMap` call sites.
|
|
- Keep it for trusted internal-to-public conversions where values come from already-decoded JSON-like data.
|
|
- Do not use it for untrusted public caller input before validation.
|
|
4. Add a short comment near the constructor if needed to clarify that recursive validation is intentionally deferred.
|
|
|
|
Tests:
|
|
|
|
- Add a public package test where `OpenAICompatibleProfile` receives cyclic `ExtraParams`.
|
|
- The constructor must return promptly.
|
|
- `NewEngine(..., WithProfiles(profile))` must return `ErrInvalidConfig`.
|
|
- Add a test proving non-cyclic nested `ExtraParams` still work through `WithProfiles`.
|
|
- Keep existing mutation-isolation tests passing.
|
|
|
|
Documentation:
|
|
|
|
- No user-facing behavior change is required if existing docs already state that `WithProfiles` validates `ExtraParams`.
|
|
- Update docs only if constructor behavior is currently described as validating or deep-copying recursively.
|
|
|
|
## Stage 4: Redact Provider Non-2xx Response Bodies From Default Errors
|
|
|
|
Problem:
|
|
|
|
The OpenAI-compatible client includes a trimmed provider response body snippet in `ErrUnexpectedStatus`. CLI and library callers may log this error. Provider error bodies can include prompt fragments, schema details, request IDs, or other sensitive operational data.
|
|
|
|
Decision:
|
|
|
|
Default errors should include the provider status code but not the response body. Do not add a debug mode in this pass unless an existing debug/logging surface already supports it.
|
|
|
|
Implementation:
|
|
|
|
1. Change the non-2xx error returned by `internal/llm.OpenAICompatibleClient`.
|
|
- Keep wrapping `ErrUnexpectedStatus`.
|
|
- Include `status=<code>`.
|
|
- Do not include response body text.
|
|
2. Drain and close the response body safely enough for normal HTTP client reuse.
|
|
- It is acceptable to read and discard a small bounded amount if needed.
|
|
- Do not store or return the discarded content.
|
|
3. Review tests that assert the old body-snippet behavior and update them.
|
|
4. Review CLI, HTTP, and public package error mapping.
|
|
- HTTP should remain generic and not leak provider details.
|
|
- CLI/library errors should retain enough status context to diagnose provider failures.
|
|
|
|
Tests:
|
|
|
|
- Update `internal/llm` non-2xx tests:
|
|
- `errors.Is(err, ErrUnexpectedStatus)` remains true;
|
|
- the status code appears in the error string;
|
|
- the provider response body does not appear in the error string.
|
|
- Add a regression test with a body containing distinctive sensitive-looking text and assert it is absent.
|
|
|
|
Documentation:
|
|
|
|
- Update `docs/integrations/openai-compatible-chat.md` to remove the claim that `ErrUnexpectedStatus` includes a response body snippet.
|
|
- Update troubleshooting docs only if they currently instruct users to inspect provider body snippets.
|
|
|
|
## Stage 5: Clarify Artifact-Root Symlink Semantics
|
|
|
|
Problem:
|
|
|
|
The restricted HTTP artifact reader uses lexical path containment before reading the file. Symlinks inside the artifact root are followed by the operating system. This is documented, but the phrase "must stay inside the root" can be overread as a strict realpath guarantee.
|
|
|
|
Decision:
|
|
|
|
Keep symlink-following behavior for this cleanup pass, but make the code and docs explicit that containment is lexical and relies on the artifact root not being writable by untrusted users. This avoids a potentially breaking change for deployments that intentionally use symlinks.
|
|
|
|
Implementation:
|
|
|
|
1. Rename or comment the restricted reader's path-resolution helper to make the lexical nature clear.
|
|
2. Add tests documenting current symlink behavior where the platform supports symlinks.
|
|
- A symlink inside the root to a file outside the root is followed.
|
|
- The test should skip cleanly if symlink creation is unavailable.
|
|
3. Keep traversal rejection tests for `..` and absolute paths outside the root.
|
|
4. Do not introduce `filepath.EvalSymlinks` in this pass.
|
|
|
|
Documentation:
|
|
|
|
- Update `docs/config.md`, `docs/operations.md`, `docs/integrations/http-api.md`, and `docs/internal/adapters.md` to say:
|
|
- lexical traversal outside the root is rejected;
|
|
- symlinks inside the root are followed;
|
|
- the artifact root must not be writable by untrusted users.
|
|
- Avoid wording that implies strict realpath containment unless the implementation changes to enforce it.
|
|
|
|
Future option:
|
|
|
|
If strict filesystem containment becomes required, add an opt-in or replacement mode that resolves both the configured root and requested file with `filepath.EvalSymlinks` before reading, rejects symlink escapes, and documents any compatibility impact.
|
|
|
|
## Stage 6: Align CLI Help And Documentation
|
|
|
|
Problem:
|
|
|
|
The `serve` usage text omits `--artifact-root`, even though the flag exists. New limit flags from Stage 2 also need to appear consistently in CLI help and docs.
|
|
|
|
Decision:
|
|
|
|
Keep CLI help concise but complete for supported flags.
|
|
|
|
Implementation:
|
|
|
|
1. Update `printUsage` in `internal/adapter/cli`.
|
|
- Include `--artifact-root DIR` in the `serve` usage line.
|
|
- Include the new size-limit flags from Stage 2.
|
|
- Keep the line readable; splitting long usage text into multiple lines is acceptable if tests are updated.
|
|
2. Update CLI tests that assert usage output.
|
|
3. Confirm `docs/cli.md` matches actual flags.
|
|
|
|
Tests:
|
|
|
|
- Add or update CLI usage tests to assert that `serve` help mentions:
|
|
- `--artifact-root`;
|
|
- `--max-request-bytes`;
|
|
- `--max-artifact-bytes`;
|
|
- `--max-response-bytes`.
|
|
|
|
Documentation:
|
|
|
|
- Update `docs/cli.md` and any command examples affected by line wrapping or flag additions.
|
|
|
|
## Stage 7: Final Verification
|
|
|
|
Run the full verification set after all stages:
|
|
|
|
```bash
|
|
go test ./...
|
|
go vet ./...
|
|
go run ./examples/go-library/prepare
|
|
go run ./cmd/scriptorium render \
|
|
--config ./examples/config.yml \
|
|
--prompt generic.markdown_summary \
|
|
--input transcript=./examples/fixtures/transcript.md \
|
|
--input glossary=./examples/fixtures/glossary.yml \
|
|
--format json
|
|
```
|
|
|
|
Also run targeted packages while implementing each stage:
|
|
|
|
```bash
|
|
go test ./internal/promptdef ./internal/validate ./internal/filecatalog
|
|
go test ./internal/artifact ./internal/adapter/http ./internal/adapter/cli ./internal/config
|
|
go test .
|
|
```
|
|
|
|
## Non-Goals
|
|
|
|
- Do not collapse internal packages merely to reduce package count.
|
|
- Do not add HTTP authentication or authorization.
|
|
- Do not remove HTTP file inputs.
|
|
- Do not change public request/result type names or method signatures.
|
|
- Do not change CLI `run` or `render` local file-input behavior.
|
|
- Do not silently truncate request, artifact, provider, or response bodies.
|
|
- Do not accept raw API keys through config files, profile files, CLI flags, or HTTP payloads.
|
|
|
|
## Assumptions
|
|
|
|
- Public `fs.FS` roots are intended to be narrower than the supplied filesystem and should therefore be enforced.
|
|
- The initial HTTP size-limit defaults are intentionally conservative and can be tuned by operators.
|
|
- Provider response-body diagnostics are less important than safe default error handling.
|
|
- Symlink compatibility is more important than strict realpath containment for the immediate cleanup pass, provided documentation is explicit.
|