From 1dccf5f140e7c2d1ef855b4c7bf920e098f2a7d1 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Mon, 10 Aug 2026 17:35:52 +0000 Subject: [PATCH] Validate portable workspace identifiers --- docs/config.md | 17 ++++-- docs/internal/artifacts.md | 14 ++++- docs/internal/manifest.md | 5 ++ docs/roadmap/implementation.md | 2 +- internal/app/campaign_config_path.go | 9 +-- internal/app/paths_test_helpers_test.go | 16 +++++ internal/app/post_publish_cleanup_test.go | 2 +- internal/artifactpolicy/policy_fuzz_test.go | 32 ++++++++++ internal/artifacts/artifact_resolver.go | 6 +- internal/artifacts/artifact_resolver_test.go | 6 +- internal/artifacts/catalog.go | 12 ++-- internal/artifacts/catalog_test.go | 20 ++++++ internal/artifacts/current_state.go | 13 ++++ internal/artifacts/identity_paths_test.go | 40 ++++++++++++ internal/artifacts/local.go | 6 ++ internal/artifacts/paths.go | 44 ++++++++++--- internal/artifacts/paths_model_test.go | 12 ++-- internal/artifacts/paths_test_helpers_test.go | 21 +++++++ internal/artifacts/publish_identity.go | 6 ++ internal/config/identity_validation_test.go | 25 ++++++++ internal/config/validate.go | 14 +++++ internal/manifest/identity_validation_test.go | 36 +++++++++++ internal/manifest/store.go | 40 ++++++++++++ internal/pathsafe/opaque_segment.go | 34 +++++++++++ internal/pathsafe/opaque_segment_test.go | 61 +++++++++++++++++++ internal/previouscache/previouscache.go | 5 +- internal/stage/analyze_test.go | 12 ++-- internal/stage/paths_test_helpers_test.go | 16 +++++ internal/stage/prepare_previous_test.go | 2 +- internal/stage/prepare_test.go | 4 +- internal/stage/run_local.go | 6 ++ 31 files changed, 490 insertions(+), 48 deletions(-) create mode 100644 internal/app/paths_test_helpers_test.go create mode 100644 internal/artifactpolicy/policy_fuzz_test.go create mode 100644 internal/artifacts/identity_paths_test.go create mode 100644 internal/artifacts/paths_test_helpers_test.go create mode 100644 internal/config/identity_validation_test.go create mode 100644 internal/manifest/identity_validation_test.go create mode 100644 internal/pathsafe/opaque_segment.go create mode 100644 internal/pathsafe/opaque_segment_test.go create mode 100644 internal/stage/paths_test_helpers_test.go diff --git a/docs/config.md b/docs/config.md index 6c1db0d..dc4a398 100644 --- a/docs/config.md +++ b/docs/config.md @@ -38,6 +38,15 @@ If local session discovery fails and a `session_id` is known, Narratio attempts using configured object storage. +### Identity segments + +Campaign IDs (`campaign_id` and `default_campaign_id`), session IDs, previous +session IDs, and Narratio run IDs are opaque portable segments. They must use +only ASCII letters, digits, `.`, `_`, and `-`; empty values, `.`/`..`, path +separators, drive forms, whitespace, control characters, and non-ASCII text are +rejected. Narratio does not trim or rewrite these values. Existing manifests or +remote state with an unsafe legacy identity must be migrated before use. + ## Validation and Merge Rules - YAML decode is strict (`KnownFields(true)`): unknown fields fail load. @@ -272,7 +281,7 @@ For each artifact input `pipeline.scriptorium.artifacts..inputs..inputs.` - previous-session source ID format: `narratio.previous_session.artifact.` -All formats are validated by strict source-policy rules. Extraction sources are -registered only from `pipeline.notarius.outputs`; the Notarius index has no -selectable source ID. +All formats are validated by strict source-policy rules. Configured artifact and +extraction keys use `^[a-z][a-z0-9_]*$`; source parsers never normalize an +unrecognized token into a valid source. Extraction sources are registered only +from `pipeline.notarius.outputs`; the Notarius index has no selectable source +ID. ## Runtime Catalog @@ -131,6 +133,12 @@ Caller policy is intentionally outside artifacts helpers: - spool/cache paths; - S3 session/run/current-state key layout. +Campaign, session, and Narratio run IDs are validated as portable opaque +segments at configuration and artifact boundaries before they can be used in a +workspace or S3 namespace. Previous-artifact destinations remain typed, +multi-segment relative paths and are confined beneath `previous/artifacts`; they +are not treated as opaque identifiers. + See [Workspace Internals](workspace.md) for how callers consume local helpers and [Operations](../operations.md#local-state-layout) for the authoritative physical layout. diff --git a/docs/internal/manifest.md b/docs/internal/manifest.md index ec631ab..f3e0e33 100644 --- a/docs/internal/manifest.md +++ b/docs/internal/manifest.md @@ -17,6 +17,11 @@ Explain the session-progress and invocation-audit models implemented by - durable `artifacts` records - per-stage `stages` map +Session, campaign, and run identities in local and downloaded manifests must be +portable opaque segments. Unsafe legacy identities are rejected with migration +guidance rather than being normalized into a different workspace or remote +namespace. + The model admits these stage states: - `pending` diff --git a/docs/roadmap/implementation.md b/docs/roadmap/implementation.md index 2431214..108edea 100644 --- a/docs/roadmap/implementation.md +++ b/docs/roadmap/implementation.md @@ -17,7 +17,7 @@ All stages are pending when this plan is created. | Stage | Summary | Primary findings | Status | | ---: | --- | --- | --- | | 1 | Align data classification and group workspace modes | RSK-004 | Completed | -| 2 | Enforce safe identifiers and fuzz path/source contracts | COR-002, TST-013 | Pending | +| 2 | Enforce safe identifiers and fuzz path/source contracts | COR-002, TST-013 | Completed | | 3 | Consolidate crash-durable atomic file replacement | RSK-002, DUP-001, DUP-005 | Pending | | 4 | Add confined destination and download/install capabilities | COR-003, DUP-003, TST-003 | Pending | | 5 | Confine recursive cleanup and replace sentinel locks | RSK-003 | Pending | diff --git a/internal/app/campaign_config_path.go b/internal/app/campaign_config_path.go index d5b0278..b931495 100644 --- a/internal/app/campaign_config_path.go +++ b/internal/app/campaign_config_path.go @@ -6,6 +6,7 @@ import ( "strings" "gitea.maximumdirect.net/eric/narratio/internal/config" + "gitea.maximumdirect.net/eric/narratio/internal/pathsafe" ) func resolveCampaignConfigPath(pipelineCfg *config.PipelineConfig, campaignIDFlag, campaignFileFlag string) (string, error) { @@ -33,12 +34,8 @@ func resolveCampaignConfigPath(pipelineCfg *config.PipelineConfig, campaignIDFla } func validateCampaignIDToken(campaignID string) error { - if filepath.IsAbs(campaignID) || - strings.Contains(campaignID, "/") || - strings.Contains(campaignID, `\`) || - campaignID == "." || - campaignID == ".." { - return fmt.Errorf("campaign id %q must be a single path segment", campaignID) + if err := pathsafe.ValidateOpaqueSegment(campaignID); err != nil { + return fmt.Errorf("campaign id %q must be a single path segment and opaque identifier: %w", campaignID, err) } return nil } diff --git a/internal/app/paths_test_helpers_test.go b/internal/app/paths_test_helpers_test.go new file mode 100644 index 0000000..7a90326 --- /dev/null +++ b/internal/app/paths_test_helpers_test.go @@ -0,0 +1,16 @@ +package app + +import ( + "testing" + + "gitea.maximumdirect.net/eric/narratio/internal/artifacts" +) + +func mustPreviousArtifactPathForCampaign(t *testing.T, root, campaign, sessionID, relative string) string { + t.Helper() + path, err := artifacts.SessionPreviousArtifactPathForCampaign(root, campaign, sessionID, relative) + if err != nil { + t.Fatalf("SessionPreviousArtifactPathForCampaign() error = %v", err) + } + return path +} diff --git a/internal/app/post_publish_cleanup_test.go b/internal/app/post_publish_cleanup_test.go index b64e83a..f11fd83 100644 --- a/internal/app/post_publish_cleanup_test.go +++ b/internal/app/post_publish_cleanup_test.go @@ -277,7 +277,7 @@ func cleanupFixtureConfig(t *testing.T) (*config.Config, cleanupSeed) { runWorkDir := artifacts.SessionRunRootForCampaign(cfg.Pipeline.Workspace.Root, cfg.Session.Campaign, cfg.Session.SessionID, runID) otherRunDir := artifacts.SessionRunRootForCampaign(cfg.Pipeline.Workspace.Root, cfg.Session.Campaign, cfg.Session.SessionID, "20260516T010204Z-5e6f7a8b") spoolAudioDir := artifacts.SessionSpoolAudioDir(cfg.Pipeline.Spool.Root, cfg.Session.Campaign, cfg.Session.SessionID, runID) - previousCachePath := artifacts.SessionPreviousArtifactPathForCampaign( + previousCachePath := mustPreviousArtifactPathForCampaign(t, cfg.Pipeline.Workspace.Root, cfg.Session.Campaign, cfg.Session.SessionID, diff --git a/internal/artifactpolicy/policy_fuzz_test.go b/internal/artifactpolicy/policy_fuzz_test.go new file mode 100644 index 0000000..bf29854 --- /dev/null +++ b/internal/artifactpolicy/policy_fuzz_test.go @@ -0,0 +1,32 @@ +package artifactpolicy + +import "testing" + +func FuzzArtifactSourceParsers(f *testing.F) { + for _, seed := range []string{ + "narratio.artifact.session_recap", + "narratio.extraction.npc_registry", + "narratio.previous_session.artifact.session_recap", + "narratio.artifact../escape", + `narratio.artifact.key\name`, + "narratio.artifact.café", + } { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, source string) { + for _, parse := range []func(string) (string, bool){ParseConfiguredSource, ParseExtractionSource, ParsePreviousSessionSource} { + if key, ok := parse(source); ok && !IsConfiguredKey(key) { + t.Fatalf("parser accepted non-configured key %q from %q", key, source) + } + } + classified, err := ClassifySource(source) + if err == nil { + if classified.ID == "" { + t.Fatalf("ClassifySource(%q) returned an empty identifier", source) + } + if classified.ConfiguredKey != "" && !IsConfiguredKey(classified.ConfiguredKey) { + t.Fatalf("ClassifySource(%q) returned invalid configured key %q", source, classified.ConfiguredKey) + } + } + }) +} diff --git a/internal/artifacts/artifact_resolver.go b/internal/artifacts/artifact_resolver.go index 04103f7..579d8d2 100644 --- a/internal/artifacts/artifact_resolver.go +++ b/internal/artifacts/artifact_resolver.go @@ -391,7 +391,11 @@ func previousSessionCacheCandidatePaths(paths SessionPaths, canonicalRelPath str out := make([]string, 0, len(relCandidates)) seen := map[string]struct{}{} for _, rel := range relCandidates { - abs := filepath.Clean(SessionPreviousArtifactPath(paths, rel)) + abs, err := SessionPreviousArtifactPath(paths, rel) + if err != nil { + continue + } + abs = filepath.Clean(abs) if _, ok := seen[abs]; ok { continue } diff --git a/internal/artifacts/artifact_resolver_test.go b/internal/artifacts/artifact_resolver_test.go index d524863..d2fd5d1 100644 --- a/internal/artifacts/artifact_resolver_test.go +++ b/internal/artifacts/artifact_resolver_test.go @@ -436,8 +436,8 @@ func TestResolvePreviousSessionArtifactWithCatalogPrefersManifestInputRecord(t * workspace := t.TempDir() paths := buildSessionPaths(workspace, "campaign", "session") - manifestBackedPath := SessionPreviousArtifactPath(paths, "artifacts/session_recap.md") - fallbackPath := SessionPreviousArtifactPath(paths, "session_recap.md") + manifestBackedPath := mustSessionPreviousArtifactPath(t, paths, "artifacts/session_recap.md") + fallbackPath := mustSessionPreviousArtifactPath(t, paths, "session_recap.md") if err := os.MkdirAll(filepath.Dir(manifestBackedPath), 0o755); err != nil { t.Fatalf("MkdirAll() error = %v", err) } @@ -487,7 +487,7 @@ func TestResolvePreviousSessionArtifactWithCatalogFallsBackToPreparedCachePath(t workspace := t.TempDir() paths := buildSessionPaths(workspace, "campaign", "session") - fallbackPath := SessionPreviousArtifactPath(paths, "session_recap.md") + fallbackPath := mustSessionPreviousArtifactPath(t, paths, "session_recap.md") if err := os.MkdirAll(filepath.Dir(fallbackPath), 0o755); err != nil { t.Fatalf("MkdirAll() error = %v", err) } diff --git a/internal/artifacts/catalog.go b/internal/artifacts/catalog.go index 5e4a7f3..6f46852 100644 --- a/internal/artifacts/catalog.go +++ b/internal/artifacts/catalog.go @@ -92,8 +92,8 @@ func (c *ArtifactCatalog) RegisterExtractionArtifacts(configured map[string]Extr for _, key := range keys { trimmed := strings.TrimSpace(key) - if trimmed == "" { - return fmt.Errorf("extraction artifact keys must be non-empty") + if !artifactpolicy.IsConfiguredKey(trimmed) { + return fmt.Errorf("extraction artifact key %q must match ^[a-z][a-z0-9_]*$", key) } if _, exists := c.extractionIndex[trimmed]; exists { return fmt.Errorf("duplicate extraction artifact key %q", trimmed) @@ -152,16 +152,16 @@ func (c *ArtifactCatalog) RegisterConfiguredArtifacts( selectedSet := map[string]struct{}{} for _, key := range selected { trimmed := strings.TrimSpace(key) - if trimmed == "" { - return fmt.Errorf("selected artifact keys must be non-empty") + if !artifactpolicy.IsConfiguredKey(trimmed) { + return fmt.Errorf("selected artifact key %q must match ^[a-z][a-z0-9_]*$", key) } selectedSet[trimmed] = struct{}{} } for _, key := range keys { trimmed := strings.TrimSpace(key) - if trimmed == "" { - return fmt.Errorf("configured artifact keys must be non-empty") + if !artifactpolicy.IsConfiguredKey(trimmed) { + return fmt.Errorf("configured artifact key %q must match ^[a-z][a-z0-9_]*$", key) } def := configured[key] sourceID := ConfiguredArtifactSourceID(trimmed) diff --git a/internal/artifacts/catalog_test.go b/internal/artifacts/catalog_test.go index 2dd9dd2..84ff8d0 100644 --- a/internal/artifacts/catalog_test.go +++ b/internal/artifacts/catalog_test.go @@ -114,6 +114,26 @@ func TestArtifactCatalogRejectsSelectedUnknownArtifact(t *testing.T) { } } +func TestArtifactCatalogRejectsUnsafeArtifactKeys(t *testing.T) { + unsafe := []string{"", "artifact/name", `artifact\name`, "artifact-name", "café"} + for _, key := range unsafe { + t.Run("configured", func(t *testing.T) { + catalog := NewArtifactCatalog() + err := catalog.RegisterConfiguredArtifacts(map[string]ConfiguredArtifactDefinition{key: {OutputPath: "artifacts/output.md"}}, nil) + if err == nil { + t.Fatalf("RegisterConfiguredArtifacts(%q) error = nil, want rejection", key) + } + }) + t.Run("extraction", func(t *testing.T) { + catalog := NewArtifactCatalog() + err := catalog.RegisterExtractionArtifacts(map[string]ExtractionArtifactDefinition{key: {LaneID: "lane"}}) + if err == nil { + t.Fatalf("RegisterExtractionArtifacts(%q) error = nil, want rejection", key) + } + }) + } +} + func TestArtifactCatalogRejectsConfiguredSourceConflictAcrossRegistrations(t *testing.T) { catalog := NewArtifactCatalog() if err := catalog.RegisterConfiguredArtifacts( diff --git a/internal/artifacts/current_state.go b/internal/artifacts/current_state.go index 3c81418..0fe0966 100644 --- a/internal/artifacts/current_state.go +++ b/internal/artifacts/current_state.go @@ -86,6 +86,9 @@ func LoadCurrentRunPointer(ctx context.Context, store storage.ObjectStore, curre if runID == "" { return "", fmt.Errorf("current run pointer %q is empty", key) } + if err := ValidateRunIdentity(runID); err != nil { + return "", fmt.Errorf("current run pointer %q contains an unsafe legacy run id; migrate remote state before use: %w", key, err) + } return runID, nil } @@ -163,6 +166,16 @@ func ValidateCurrentStateIdentity(state *CurrentState, validation CurrentStateVa manifestSessionID := strings.TrimSpace(state.Manifest.SessionID) manifestCampaign := strings.TrimSpace(state.Manifest.Campaign) manifestRunID := strings.TrimSpace(state.Manifest.RunID) + if manifestCampaign != "" { + if err := ValidateSessionIdentity(manifestCampaign, manifestSessionID); err != nil { + return fmt.Errorf("current manifest contains unsafe legacy identities; migrate remote state before use: %w", err) + } + } + if manifestRunID != "" { + if err := ValidateRunIdentity(manifestRunID); err != nil { + return fmt.Errorf("current manifest contains an unsafe legacy run id; migrate remote state before use: %w", err) + } + } if expectedSessionID != "" && manifestSessionID != expectedSessionID { return fmt.Errorf( diff --git a/internal/artifacts/identity_paths_test.go b/internal/artifacts/identity_paths_test.go new file mode 100644 index 0000000..0eeed85 --- /dev/null +++ b/internal/artifacts/identity_paths_test.go @@ -0,0 +1,40 @@ +package artifacts + +import ( + "os" + "path/filepath" + "testing" +) + +func TestEnsureLayoutForRejectsUnsafeIdentityBeforeFilesystemAccess(t *testing.T) { + workspace := t.TempDir() + store := NewLocalStore(workspace) + for _, identity := range []struct { + name string + campaign string + session string + }{ + {name: "campaign traversal", campaign: "../outside", session: "session"}, + {name: "session separator", campaign: "campaign", session: `session\other`}, + {name: "session drive", campaign: "campaign", session: `C:\outside`}, + {name: "unicode", campaign: "café", session: "session"}, + } { + t.Run(identity.name, func(t *testing.T) { + if _, err := store.EnsureLayoutFor(identity.campaign, identity.session); err == nil { + t.Fatal("EnsureLayoutFor() error = nil, want unsafe identity rejection") + } + if _, err := os.Stat(filepath.Join(workspace, "work")); !os.IsNotExist(err) { + t.Fatalf("workspace path was created for rejected identity, stat err = %v", err) + } + }) + } +} + +func TestSessionPreviousArtifactPathRejectsEscapingRelativePath(t *testing.T) { + paths := buildSessionPaths(t.TempDir(), "campaign", "session") + for _, relative := range []string{"../escape.json", "/absolute.json", `C:\escape.json`} { + if _, err := SessionPreviousArtifactPath(paths, relative); err == nil { + t.Fatalf("SessionPreviousArtifactPath(%q) error = nil, want confinement failure", relative) + } + } +} diff --git a/internal/artifacts/local.go b/internal/artifacts/local.go index 2e3b800..80346fc 100644 --- a/internal/artifacts/local.go +++ b/internal/artifacts/local.go @@ -51,6 +51,9 @@ func (s *LocalStore) EnsureLayoutFor(campaign, sessionID string) (SessionPaths, if campaign == "" { return SessionPaths{}, fmt.Errorf("campaign is required") } + if err := ValidateSessionIdentity(campaign, sessionID); err != nil { + return SessionPaths{}, err + } return s.ensureLayout(s.SessionPathsFor(campaign, sessionID)) } @@ -62,6 +65,9 @@ func (s *LocalStore) ensureLayout(paths SessionPaths) (SessionPaths, error) { if strings.TrimSpace(paths.SessionID) == "" { return SessionPaths{}, fmt.Errorf("sessionID is required") } + if err := ValidateSessionIdentity(paths.CampaignID, paths.SessionID); err != nil { + return SessionPaths{}, err + } dirs := []string{ paths.Root, diff --git a/internal/artifacts/paths.go b/internal/artifacts/paths.go index 4dc6399..0da16bc 100644 --- a/internal/artifacts/paths.go +++ b/internal/artifacts/paths.go @@ -7,6 +7,7 @@ import ( "strings" "gitea.maximumdirect.net/eric/narratio/internal/config" + "gitea.maximumdirect.net/eric/narratio/internal/pathsafe" ) // SessionPaths contains canonical local paths for one session work directory. @@ -63,11 +64,15 @@ func SessionPreviousArtifactsDirForCampaign(rootDir, campaign, sessionID string) return filepath.Join(SessionPreviousDirForCampaign(rootDir, campaign, sessionID), config.PathArtifactsDirSegment) } -// SessionPreviousArtifactPathForCampaign returns a path under previous/artifacts for one artifact. -func SessionPreviousArtifactPathForCampaign(rootDir, campaign, sessionID, artifactRelativePath string) string { - return filepath.Join( +// SessionPreviousArtifactPathForCampaign returns a confined path under +// previous/artifacts for one artifact. +func SessionPreviousArtifactPathForCampaign(rootDir, campaign, sessionID, artifactRelativePath string) (string, error) { + if err := ValidateSessionIdentity(campaign, sessionID); err != nil { + return "", err + } + return pathsafe.JoinSlashRelativeUnderRoot( SessionPreviousArtifactsDirForCampaign(rootDir, campaign, sessionID), - filepath.FromSlash(previousArtifactCacheRelativePath(artifactRelativePath)), + previousArtifactCacheRelativePath(artifactRelativePath), ) } @@ -190,9 +195,34 @@ func SessionPreviousArtifactsDir(paths SessionPaths) string { return paths.PreviousArtifactsDir } -// SessionPreviousArtifactPath returns a path under previous/artifacts for already-resolved session paths. -func SessionPreviousArtifactPath(paths SessionPaths, artifactRelativePath string) string { - return filepath.Join(paths.PreviousArtifactsDir, filepath.FromSlash(previousArtifactCacheRelativePath(artifactRelativePath))) +// SessionPreviousArtifactPath returns a confined path under previous/artifacts +// for already-resolved session paths. +func SessionPreviousArtifactPath(paths SessionPaths, artifactRelativePath string) (string, error) { + if err := ValidateSessionIdentity(paths.CampaignID, paths.SessionID); err != nil { + return "", err + } + return pathsafe.JoinSlashRelativeUnderRoot(paths.PreviousArtifactsDir, previousArtifactCacheRelativePath(artifactRelativePath)) +} + +// ValidateSessionIdentity validates the opaque components used in workspace +// and object-store session paths. +func ValidateSessionIdentity(campaign, sessionID string) error { + if err := pathsafe.ValidateOpaqueSegment(campaign); err != nil { + return fmt.Errorf("campaign identity: %w", err) + } + if err := pathsafe.ValidateOpaqueSegment(sessionID); err != nil { + return fmt.Errorf("session identity: %w", err) + } + return nil +} + +// ValidateRunIdentity validates the opaque component used below a session's +// run directory and object-store prefix. +func ValidateRunIdentity(runID string) error { + if err := pathsafe.ValidateOpaqueSegment(runID); err != nil { + return fmt.Errorf("run identity: %w", err) + } + return nil } func previousArtifactCacheRelativePath(artifactRelativePath string) string { diff --git a/internal/artifacts/paths_model_test.go b/internal/artifacts/paths_model_test.go index 1a73ed5..293a24b 100644 --- a/internal/artifacts/paths_model_test.go +++ b/internal/artifacts/paths_model_test.go @@ -96,15 +96,15 @@ func TestSessionPreviousPathsForCampaign(t *testing.T) { t.Fatalf("SessionPreviousArtifactsDirForCampaign() = %q, want %q", artifactsDir, wantArtifactsDir) } - artifactPath := SessionPreviousArtifactPathForCampaign(root, "forsaken", "2026-04-19", "session_recap.md") + artifactPath := mustSessionPreviousArtifactPathForCampaign(t, root, "forsaken", "2026-04-19", "session_recap.md") wantArtifactPath := filepath.Join(root, "work", "forsaken", "2026-04-19", "previous", "artifacts", "session_recap.md") if artifactPath != wantArtifactPath { - t.Fatalf("SessionPreviousArtifactPathForCampaign() = %q, want %q", artifactPath, wantArtifactPath) + t.Fatalf("mustSessionPreviousArtifactPathForCampaign(t, ) = %q, want %q", artifactPath, wantArtifactPath) } - previousRelativeArtifactPath := SessionPreviousArtifactPathForCampaign(root, "forsaken", "2026-04-19", "artifacts/session_recap.md") + previousRelativeArtifactPath := mustSessionPreviousArtifactPathForCampaign(t, root, "forsaken", "2026-04-19", "artifacts/session_recap.md") if previousRelativeArtifactPath != wantArtifactPath { - t.Fatalf("SessionPreviousArtifactPathForCampaign(previous-relative) = %q, want %q", previousRelativeArtifactPath, wantArtifactPath) + t.Fatalf("mustSessionPreviousArtifactPathForCampaign(t, previous-relative) = %q, want %q", previousRelativeArtifactPath, wantArtifactPath) } } @@ -120,13 +120,13 @@ func TestSessionPreviousPathsFromSessionPaths(t *testing.T) { t.Fatalf("SessionPreviousArtifactsDir() = %q, want %q", got, paths.PreviousArtifactsDir) } - got := SessionPreviousArtifactPath(paths, "quest_log.json") + got := mustSessionPreviousArtifactPath(t, paths, "quest_log.json") want := filepath.Join(paths.PreviousArtifactsDir, "quest_log.json") if got != want { t.Fatalf("SessionPreviousArtifactPath() = %q, want %q", got, want) } - got = SessionPreviousArtifactPath(paths, "artifacts/session_recap.md") + got = mustSessionPreviousArtifactPath(t, paths, "artifacts/session_recap.md") want = filepath.Join(paths.PreviousArtifactsDir, "session_recap.md") if got != want { t.Fatalf("SessionPreviousArtifactPath(previous-relative) = %q, want %q", got, want) diff --git a/internal/artifacts/paths_test_helpers_test.go b/internal/artifacts/paths_test_helpers_test.go new file mode 100644 index 0000000..cca7bb8 --- /dev/null +++ b/internal/artifacts/paths_test_helpers_test.go @@ -0,0 +1,21 @@ +package artifacts + +import "testing" + +func mustSessionPreviousArtifactPath(t *testing.T, paths SessionPaths, relative string) string { + t.Helper() + path, err := SessionPreviousArtifactPath(paths, relative) + if err != nil { + t.Fatalf("SessionPreviousArtifactPath() error = %v", err) + } + return path +} + +func mustSessionPreviousArtifactPathForCampaign(t *testing.T, root, campaign, sessionID, relative string) string { + t.Helper() + path, err := SessionPreviousArtifactPathForCampaign(root, campaign, sessionID, relative) + if err != nil { + t.Fatalf("SessionPreviousArtifactPathForCampaign() error = %v", err) + } + return path +} diff --git a/internal/artifacts/publish_identity.go b/internal/artifacts/publish_identity.go index aee094b..c3b3e0e 100644 --- a/internal/artifacts/publish_identity.go +++ b/internal/artifacts/publish_identity.go @@ -39,6 +39,9 @@ func ResolvePublishSessionPrefix(cfg *config.Config, m *manifest.Manifest) (stri if cfg.Pipeline.Storage.S3 == nil { return "", fmt.Errorf("pipeline.storage.s3 configuration is required") } + if err := ValidateSessionIdentity(campaign, sessionID); err != nil { + return "", err + } sessionPrefix := S3SessionPrefix(cfg.Pipeline.Storage.S3.RootPrefix, campaign, sessionID) if strings.TrimSpace(sessionPrefix) == "" { @@ -68,6 +71,9 @@ func ResolvePublishRunPrefix(cfg *config.Config, m *manifest.Manifest) (string, if runID == "" { return "", fmt.Errorf("run id is required") } + if err := ValidateRunIdentity(runID); err != nil { + return "", err + } return S3RunPrefix(sessionPrefix, runID), nil } diff --git a/internal/config/identity_validation_test.go b/internal/config/identity_validation_test.go new file mode 100644 index 0000000..490f0f7 --- /dev/null +++ b/internal/config/identity_validation_test.go @@ -0,0 +1,25 @@ +package config + +import ( + "strings" + "testing" +) + +func TestIdentityValidationRejectsUnsafeSegments(t *testing.T) { + unsafe := []string{"../escape", `campaign\name`, `C:\campaign`, "café", "campaign name", ".", ".."} + for _, value := range unsafe { + t.Run("campaign/"+strings.ReplaceAll(value, "/", "_"), func(t *testing.T) { + cfg := &CampaignConfig{CampaignID: value, Inputs: CampaignInputsConfig{ + SpeakersFile: "speakers.yml", AutocorrectFile: "autocorrect.yml", GlossaryFile: "glossary.yml", PlayersFile: "players.yml", PartyFile: "party.yml", + }} + if err := validateCampaign(cfg); err == nil { + t.Fatalf("validateCampaign(%q) error = nil, want rejection", value) + } + }) + t.Run("session/"+strings.ReplaceAll(value, "/", "_"), func(t *testing.T) { + if err := validateSessionIdentifier("session.session_id", value, true); err == nil { + t.Fatalf("validateSessionIdentifier(%q) error = nil, want rejection", value) + } + }) + } +} diff --git a/internal/config/validate.go b/internal/config/validate.go index 5c21380..4f0c7d6 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -53,6 +53,9 @@ func validateCampaign(cfg *CampaignConfig) error { if CampaignID(cfg) == "" { return fmt.Errorf("campaign.campaign_id is required") } + if err := pathsafe.ValidateOpaqueSegment(cfg.CampaignID); err != nil { + return fmt.Errorf("campaign.campaign_id: %w", err) + } if strings.TrimSpace(cfg.Inputs.SpeakersFile) == "" { return fmt.Errorf("campaign.inputs.speakers_file is required") } @@ -75,6 +78,11 @@ func validatePipeline(cfg *PipelineConfig) error { if strings.TrimSpace(cfg.Workspace.Root) == "" { return fmt.Errorf("pipeline.workspace.root is required") } + if campaignID := cfg.Campaigns.DefaultCampaignID; campaignID != "" { + if err := pathsafe.ValidateOpaqueSegment(campaignID); err != nil { + return fmt.Errorf("pipeline.campaigns.default_campaign_id: %w", err) + } + } if err := validateSecrets(cfg.Secrets); err != nil { return err } @@ -717,6 +725,9 @@ func validateSession(cfg *SessionConfig) error { if strings.TrimSpace(cfg.Campaign) == "" { return fmt.Errorf("session.campaign is required") } + if err := pathsafe.ValidateOpaqueSegment(cfg.Campaign); err != nil { + return fmt.Errorf("session.campaign: %w", err) + } if strings.TrimSpace(cfg.Inputs.SpeakersFile) == "" { return fmt.Errorf("session.inputs.speakers_file is required") @@ -762,6 +773,9 @@ func validateSessionIdentifier(fieldName, value string, required bool) error { } return nil } + if err := pathsafe.ValidateOpaqueSegment(value); err != nil { + return fmt.Errorf("%s: %w", fieldName, err) + } return nil } diff --git a/internal/manifest/identity_validation_test.go b/internal/manifest/identity_validation_test.go new file mode 100644 index 0000000..6440d1e --- /dev/null +++ b/internal/manifest/identity_validation_test.go @@ -0,0 +1,36 @@ +package manifest + +import ( + "context" + "os" + "path/filepath" + "strings" + "testing" + "time" +) + +func TestLocalStoreRejectsUnsafeLegacyIdentity(t *testing.T) { + now := time.Now().UTC() + path := filepath.Join(t.TempDir(), "manifest.json") + data := []byte(`{"session_id":"../escape","campaign":"campaign","created_at":"` + now.Format(time.RFC3339Nano) + `","updated_at":"` + now.Format(time.RFC3339Nano) + `","stages":{}}`) + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatalf("WriteFile() error = %v", err) + } + + _, err := (&LocalStore{}).Load(context.Background(), path) + if err == nil || !strings.Contains(err.Error(), "migrate the legacy manifest") { + t.Fatalf("Load() error = %v, want migration guidance", err) + } +} + +func TestLocalStoreRefusesUnsafeIdentityOnSave(t *testing.T) { + now := time.Now().UTC() + m := New("session", now) + m.Campaign = "../outside" + path := filepath.Join(t.TempDir(), "manifest.json") + + err := (&LocalStore{}).Save(context.Background(), path, m) + if err == nil || !strings.Contains(err.Error(), "migrate the legacy manifest") { + t.Fatalf("Save() error = %v, want migration guidance", err) + } +} diff --git a/internal/manifest/store.go b/internal/manifest/store.go index 89b73f3..7c58618 100644 --- a/internal/manifest/store.go +++ b/internal/manifest/store.go @@ -10,6 +10,7 @@ import ( "time" "gitea.maximumdirect.net/eric/narratio/internal/fileops" + "gitea.maximumdirect.net/eric/narratio/internal/pathsafe" ) // Store persists manifests to and from durable storage. @@ -79,6 +80,9 @@ func (s *LocalStore) Save(ctx context.Context, path string, m *Manifest) error { if m.CreatedAt.IsZero() { return fmt.Errorf("save manifest: created_at is required") } + if err := validateManifestIdentities(m.SessionID, m.Campaign, m.RunID); err != nil { + return fmt.Errorf("save manifest: %w", err) + } m.UpdatedAt = time.Now().UTC() if m.Stages == nil { @@ -202,6 +206,9 @@ func (s *LocalStore) SaveRun(ctx context.Context, path string, m *RunManifest) e if m.CreatedAt.IsZero() { return fmt.Errorf("save run manifest: created_at is required") } + if err := validateManifestIdentities(m.SessionID, m.Campaign, m.RunID); err != nil { + return fmt.Errorf("save run manifest: %w", err) + } m.UpdatedAt = time.Now().UTC() if m.Stages == nil { @@ -230,6 +237,9 @@ func validateLoadedManifest(m *Manifest) error { if m.UpdatedAt.IsZero() { return fmt.Errorf("updated_at is required") } + if err := validateManifestIdentities(m.SessionID, m.Campaign, m.RunID); err != nil { + return err + } return nil } @@ -265,10 +275,40 @@ func validateLoadedRunManifest(m *RunManifest) error { if m.UpdatedAt.IsZero() { return fmt.Errorf("updated_at is required") } + if err := validateManifestIdentities(m.SessionID, m.Campaign, m.RunID); err != nil { + return err + } return nil } +func validateManifestIdentities(sessionID, campaign, runID string) error { + identities := []struct { + field string + value string + required bool + }{ + {field: "session_id", value: sessionID, required: true}, + {field: "campaign", value: campaign}, + {field: "run_id", value: runID}, + } + for _, identity := range identities { + value := strings.TrimSpace(identity.value) + if value == "" && !identity.required { + continue + } + if err := pathsafe.ValidateOpaqueSegment(identity.value); err != nil { + return fmt.Errorf( + "manifest %s %q is not a portable opaque identifier; migrate the legacy manifest before use: %w", + identity.field, + identity.value, + err, + ) + } + } + return nil +} + func normalizeRunManifest(m *RunManifest) { if m.Stages == nil { m.Stages = map[string]*RunStageRecord{} diff --git a/internal/pathsafe/opaque_segment.go b/internal/pathsafe/opaque_segment.go new file mode 100644 index 0000000..b732afd --- /dev/null +++ b/internal/pathsafe/opaque_segment.go @@ -0,0 +1,34 @@ +package pathsafe + +import ( + "errors" + "fmt" +) + +var ( + ErrOpaqueSegmentRequired = errors.New("opaque identifier is required") + ErrOpaqueSegmentInvalid = errors.New("opaque identifier must contain only ASCII letters, digits, '.', '_', or '-'") +) + +// ValidateOpaqueSegment validates an identity token that occupies exactly one +// portable local-path and object-key segment. It deliberately does not trim or +// normalize input: callers must reject rather than silently rewrite identity. +func ValidateOpaqueSegment(value string) error { + if value == "" { + return ErrOpaqueSegmentRequired + } + if value == "." || value == ".." { + return fmt.Errorf("%w: dot segments are not allowed", ErrOpaqueSegmentInvalid) + } + for index := 0; index < len(value); index++ { + character := value[index] + if (character >= 'a' && character <= 'z') || + (character >= 'A' && character <= 'Z') || + (character >= '0' && character <= '9') || + character == '.' || character == '_' || character == '-' { + continue + } + return fmt.Errorf("%w: invalid byte at offset %d", ErrOpaqueSegmentInvalid, index) + } + return nil +} diff --git a/internal/pathsafe/opaque_segment_test.go b/internal/pathsafe/opaque_segment_test.go new file mode 100644 index 0000000..7c09f1a --- /dev/null +++ b/internal/pathsafe/opaque_segment_test.go @@ -0,0 +1,61 @@ +package pathsafe + +import "testing" + +func TestValidateOpaqueSegment(t *testing.T) { + valid := []string{"campaign", "2026-05-03", "run_01", "a.b-c_D9"} + for _, value := range valid { + t.Run("valid/"+value, func(t *testing.T) { + if err := ValidateOpaqueSegment(value); err != nil { + t.Fatalf("ValidateOpaqueSegment(%q) error = %v", value, err) + } + }) + } + + invalid := []string{"", ".", "..", "campaign/name", `campaign\name`, "/campaign", `C:\campaign`, "campaign name", "café", "campaign\x00name", "campaign\nname"} + for _, value := range invalid { + t.Run("invalid", func(t *testing.T) { + if err := ValidateOpaqueSegment(value); err == nil { + t.Fatalf("ValidateOpaqueSegment(%q) error = nil, want rejection", value) + } + }) + } +} + +func FuzzValidateOpaqueSegment(f *testing.F) { + for _, seed := range []string{"campaign", "2026-05-03", "..", `C:\campaign`, "café", "a/b", "a\x00b"} { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, value string) { + if err := ValidateOpaqueSegment(value); err == nil { + if value == "" || value == "." || value == ".." { + t.Fatalf("accepted reserved segment %q", value) + } + for index := 0; index < len(value); index++ { + character := value[index] + if !((character >= 'a' && character <= 'z') || (character >= 'A' && character <= 'Z') || + (character >= '0' && character <= '9') || character == '.' || character == '_' || character == '-') { + t.Fatalf("accepted invalid byte %q in %q", character, value) + } + } + } + }) +} + +func FuzzNormalizeRelativeDestination(f *testing.F) { + for _, seed := range []string{"reports/result.json", `reports\result.json`, "../escape", `C:\escape`, "/absolute", "a/./b", "a/../b"} { + f.Add(seed) + } + f.Fuzz(func(t *testing.T, value string) { + normalized, err := NormalizeRelativeDestination(value) + if err != nil { + return + } + if normalized == "" || normalized == "." || normalized == ".." || normalized[0] == '/' { + t.Fatalf("NormalizeRelativeDestination(%q) = %q, want confined relative path", value, normalized) + } + if len(normalized) >= 3 && normalized[:3] == "../" { + t.Fatalf("NormalizeRelativeDestination(%q) escaped with %q", value, normalized) + } + }) +} diff --git a/internal/previouscache/previouscache.go b/internal/previouscache/previouscache.go index 06836ca..5251ce3 100644 --- a/internal/previouscache/previouscache.go +++ b/internal/previouscache/previouscache.go @@ -166,7 +166,10 @@ func BuildPlan( continue } - localPath := artifacts.SessionPreviousArtifactPath(paths, selectedRel) + localPath, err := artifacts.SessionPreviousArtifactPath(paths, selectedRel) + if err != nil { + return nil, fmt.Errorf("resolve previous-session artifact path: %w", err) + } localRel, err := relativeToSession(paths, localPath) if err != nil { return nil, err diff --git a/internal/stage/analyze_test.go b/internal/stage/analyze_test.go index fa7cb68..983783a 100644 --- a/internal/stage/analyze_test.go +++ b/internal/stage/analyze_test.go @@ -385,7 +385,7 @@ func TestAnalyzeIncludesCanonicalPreviousRecapWhenPreparedCacheExists(t *testing paths := sessionPathsForEnv(env, m.SessionID) writeAnalyzeFile(t, filepath.Join(paths.TranscriptsDir, "polished.json"), `{"segments":[]}`) - previousRecapPath := artifacts.SessionPreviousArtifactPath(paths, "artifacts/session_recap.md") + previousRecapPath := mustPreviousArtifactPath(t, paths, "artifacts/session_recap.md") writeAnalyzeFile(t, previousRecapPath, "previous recap\n") env.Config.Pipeline.Scriptorium.Artifacts["session_recap"] = config.ScriptoriumArtifactConfig{ @@ -776,7 +776,7 @@ func TestAnalyzeResolvesCanonicalPreviousSessionArtifactFromManifestInput(t *tes paths := sessionPathsForEnv(env, m.SessionID) writeAnalyzeFile(t, filepath.Join(paths.TranscriptsDir, "final.trimmed.json"), `{"segments":[]}`) - previousPath := artifacts.SessionPreviousArtifactPath(paths, "artifacts/session_recap.md") + previousPath := mustPreviousArtifactPath(t, paths, "artifacts/session_recap.md") writeAnalyzeFile(t, previousPath, "previous recap\n") m.Inputs = append(m.Inputs, manifest.InputRecord{ Kind: "previous_artifact", @@ -832,7 +832,7 @@ func TestAnalyzeRenderDebugWithCanonicalPreviousSessionInput(t *testing.T) { writeAnalyzeFile(t, filepath.Join(paths.TranscriptsDir, "final.trimmed.json"), `{"segments":[]}`) env.Config.Pipeline.Scriptorium.RenderDebug = true - previousPath := artifacts.SessionPreviousArtifactPath(paths, "artifacts/session_recap.md") + previousPath := mustPreviousArtifactPath(t, paths, "artifacts/session_recap.md") writeAnalyzeFile(t, previousPath, "previous recap\n") m.Inputs = append(m.Inputs, manifest.InputRecord{ Kind: "previous_artifact", @@ -866,7 +866,7 @@ func TestAnalyzeDoesNotCallObjectStoreForCanonicalPreviousSessionInput(t *testin paths := sessionPathsForEnv(env, m.SessionID) writeAnalyzeFile(t, filepath.Join(paths.TranscriptsDir, "final.trimmed.json"), `{"segments":[]}`) - previousPath := artifacts.SessionPreviousArtifactPath(paths, "artifacts/session_recap.md") + previousPath := mustPreviousArtifactPath(t, paths, "artifacts/session_recap.md") writeAnalyzeFile(t, previousPath, "previous recap\n") m.Inputs = append(m.Inputs, manifest.InputRecord{ Kind: "previous_artifact", @@ -1552,13 +1552,13 @@ func setupAnalyzeEnv(t *testing.T) (*Env, *manifest.Manifest, *scriptorium.FakeR }, Session: &config.SessionConfig{ SessionID: "2026-05-03", - Campaign: "Icewind Dale", + Campaign: "icewind-dale", Date: "2026-05-03", }, } store := artifacts.NewLocalStore(workspace) - if _, err := store.EnsureLayoutFor("sample-campaign", "2026-05-03"); err != nil { + if _, err := store.EnsureLayoutFor("icewind-dale", "2026-05-03"); err != nil { t.Fatalf("EnsureLayout() error = %v", err) } diff --git a/internal/stage/paths_test_helpers_test.go b/internal/stage/paths_test_helpers_test.go new file mode 100644 index 0000000..9c9765c --- /dev/null +++ b/internal/stage/paths_test_helpers_test.go @@ -0,0 +1,16 @@ +package stage + +import ( + "testing" + + "gitea.maximumdirect.net/eric/narratio/internal/artifacts" +) + +func mustPreviousArtifactPath(t *testing.T, paths artifacts.SessionPaths, relative string) string { + t.Helper() + path, err := artifacts.SessionPreviousArtifactPath(paths, relative) + if err != nil { + t.Fatalf("SessionPreviousArtifactPath() error = %v", err) + } + return path +} diff --git a/internal/stage/prepare_previous_test.go b/internal/stage/prepare_previous_test.go index 0a50e7b..48756c2 100644 --- a/internal/stage/prepare_previous_test.go +++ b/internal/stage/prepare_previous_test.go @@ -46,7 +46,7 @@ func TestHydratePreviousSessionArtifactsDownloadsManifestAndRequiredArtifact(t * if _, err := os.Stat(sessionPaths.PreviousManifestPath); err != nil { t.Fatalf("previous manifest missing: %v", err) } - recapPath := artifacts.SessionPreviousArtifactPath(sessionPaths, "artifacts/session_recap.md") + recapPath := mustPreviousArtifactPath(t, sessionPaths, "artifacts/session_recap.md") if _, err := os.Stat(recapPath); err != nil { t.Fatalf("previous artifact missing: %v", err) } diff --git a/internal/stage/prepare_test.go b/internal/stage/prepare_test.go index baa4204..c01fcb3 100644 --- a/internal/stage/prepare_test.go +++ b/internal/stage/prepare_test.go @@ -508,7 +508,7 @@ func TestPrepareStageHydratesRequiredPreviousArtifactAndRecordsInputs(t *testing if _, err := os.Stat(paths.PreviousManifestPath); err != nil { t.Fatalf("expected previous manifest: %v", err) } - recapPath := artifacts.SessionPreviousArtifactPath(paths, "artifacts/session_recap.md") + recapPath := mustPreviousArtifactPath(t, paths, "artifacts/session_recap.md") if _, err := os.Stat(recapPath); err != nil { t.Fatalf("expected previous artifact: %v", err) } @@ -556,7 +556,7 @@ func TestPrepareStageRerunOverwritesPreviousCache(t *testing.T) { if _, err := (prepareStage{}).Run(context.Background(), env, m); err != nil { t.Fatalf("first prepare run error = %v", err) } - recapPath := artifacts.SessionPreviousArtifactPath(paths, "artifacts/session_recap.md") + recapPath := mustPreviousArtifactPath(t, paths, "artifacts/session_recap.md") firstBytes, err := os.ReadFile(recapPath) if err != nil { t.Fatalf("read first hydrated artifact: %v", err) diff --git a/internal/stage/run_local.go b/internal/stage/run_local.go index ab29b5b..388b952 100644 --- a/internal/stage/run_local.go +++ b/internal/stage/run_local.go @@ -54,6 +54,12 @@ func resolveRunStageLayout( if runID == "" || campaign == "" { return runStageLayout{}, nil } + if err := artifacts.ValidateSessionIdentity(campaign, sessionID); err != nil { + return runStageLayout{}, err + } + if err := artifacts.ValidateRunIdentity(runID); err != nil { + return runStageLayout{}, err + } root := artifacts.SessionRunStageDirForCampaign( env.Config.Pipeline.Workspace.Root,