From 707db5394ce4366d794510a60e62fcd2cab83d22 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Thu, 13 Aug 2026 03:06:28 +0000 Subject: [PATCH] Validate canonical comparison manifests --- docs/integrations/comparison-bundle.md | 12 +- docs/internal/comparison-publication.md | 6 + internal/comparison/comparison.go | 6 +- internal/comparison/comparison_test.go | 17 ++- internal/comparison/publish.go | 159 ++++++++++++++++++++++++ internal/comparison/publish_test.go | 44 +++++++ 6 files changed, 234 insertions(+), 10 deletions(-) diff --git a/docs/integrations/comparison-bundle.md b/docs/integrations/comparison-bundle.md index e2d8814..a874f22 100644 --- a/docs/integrations/comparison-bundle.md +++ b/docs/integrations/comparison-bundle.md @@ -66,7 +66,8 @@ contiguous from one, profile IDs are distinct and nonblank, and `succeeded + failed == total`. A successful result has `status: "succeeded"`, `validationStatus: "passed"`, -a unique Markdown `reportPath`, and no `error`. A failed result has +a `reportPath` exactly equal to the canonical `NN-profile-slug.md` filename for +its position, total, and logical profile ID, and no `error`. A failed result has `status: "failed"`, no `reportPath`, and an `error` object with nonblank `category` and `message`. Its validation status is absent, `failed`, or `skipped`. Error messages are valid UTF-8 and no longer than 1,024 bytes. @@ -81,10 +82,11 @@ larger workspace. Weatherreporter recognizes a replaceable bundle only when it exactly satisfies the current version, schema, file set, file types, relative-path rules, and -data-package digest. It rejects unknown manifest fields, multiple JSON values, -extra entries, symlinks, and future or otherwise unsupported versions. Treat a -bundle that fails recognition as an ordinary directory, not as a compatible -bundle. +data-package digest. JSON field names are case-sensitive canonical names and a +field may appear only once in each manifest object. It rejects unknown, +case-variant, or duplicate fields; multiple JSON values; extra entries; +symlinks; and future or otherwise unsupported versions. Treat a bundle that +fails recognition as an ordinary directory, not as a compatible bundle. The manifest contains safe operational provenance, but `data-package.yml` and the generated Markdown can contain sensitive weather or location context. Do diff --git a/docs/internal/comparison-publication.md b/docs/internal/comparison-publication.md index f427280..f7f42d3 100644 --- a/docs/internal/comparison-publication.md +++ b/docs/internal/comparison-publication.md @@ -6,6 +6,12 @@ and only the Markdown files for successful profiles. The durable layout, schema, and compatibility rules are owned by the [comparison bundle contract](../integrations/comparison-bundle.md). +Recognition first token-validates the manifest's object fields, rejecting +unknown, case-variant, and duplicate names before decoding its typed schema. +Manifest validation derives each successful report filename from its ordered +position, total profile count, and logical profile ID; logical-bundle and +filesystem validation then require that exact path and file set. + Destination planning is read-only. It requires an exact absolute target that is neither the filesystem root nor the working directory, rejects unsafe symlinks and non-directories, accepts a missing or empty directory, and permits diff --git a/internal/comparison/comparison.go b/internal/comparison/comparison.go index fe5f738..39fd598 100644 --- a/internal/comparison/comparison.go +++ b/internal/comparison/comparison.go @@ -283,7 +283,11 @@ func (manifest Manifest) Validate() error { if result.ValidationStatus != "passed" { return fmt.Errorf("successful result %d did not pass validation", result.Position) } - if result.Error != nil || !isReportPath(result.ReportPath) { + expectedPath, err := ReportFilename(result.Position, manifest.Total, result.ProfileID) + if err != nil { + return fmt.Errorf("successful result %d has invalid report identity: %w", result.Position, err) + } + if result.Error != nil || result.ReportPath != expectedPath { return fmt.Errorf("successful result %d has invalid report details", result.Position) } if _, duplicate := reportPaths[result.ReportPath]; duplicate { diff --git a/internal/comparison/comparison_test.go b/internal/comparison/comparison_test.go index 5671780..5468d3e 100644 --- a/internal/comparison/comparison_test.go +++ b/internal/comparison/comparison_test.go @@ -1,7 +1,6 @@ package comparison import ( - "encoding/json" "strings" "testing" "time" @@ -165,9 +164,9 @@ func TestManifestEncodingAndRoundTrip(t *testing.T) { t.Fatalf("EncodeManifest() =\n%s\nwant\n%s", encoded, want) } - var decoded Manifest - if err := json.Unmarshal(encoded, &decoded); err != nil { - t.Fatalf("json.Unmarshal() error = %v", err) + decoded, err := decodeManifest(encoded) + if err != nil { + t.Fatalf("decodeManifest() error = %v", err) } if err := decoded.Validate(); err != nil { t.Fatalf("decoded manifest validation error = %v", err) @@ -192,6 +191,10 @@ func TestManifestValidateRejectsInvariants(t *testing.T) { {name: "duplicate profile", mutate: func(manifest *Manifest) { manifest.Results[1].ProfileID = manifest.Results[0].ProfileID }}, {name: "unsupported status", mutate: func(manifest *Manifest) { manifest.Results[1].Status = "skipped" }}, {name: "successful result without report", mutate: func(manifest *Manifest) { manifest.Results[0].ReportPath = "" }}, + {name: "arbitrary report name", mutate: func(manifest *Manifest) { manifest.Results[0].ReportPath = "arbitrary.md" }}, + {name: "wrong report position", mutate: func(manifest *Manifest) { manifest.Results[0].ReportPath = "02-weather-light.md" }}, + {name: "wrong report ordinal width", mutate: func(manifest *Manifest) { manifest.Results[0].ReportPath = "1-weather-light.md" }}, + {name: "wrong report profile slug", mutate: func(manifest *Manifest) { manifest.Results[0].ReportPath = "01-weather.md" }}, {name: "successful result with error", mutate: func(manifest *Manifest) { manifest.Results[0].Error = &SafeError{Category: "application", Message: "bad"} }}, @@ -247,6 +250,12 @@ func TestLogicalBundleValidate(t *testing.T) { if err := bundle.Validate(); err == nil { t.Fatal("LogicalBundle.Validate() accepted unordered report position") } + bundle.Reports[0].Position = 1 + bundle.Reports[0].Path = "arbitrary.md" + bundle.Manifest.Results[0].ReportPath = "arbitrary.md" + if err := bundle.Validate(); err == nil { + t.Fatal("LogicalBundle.Validate() accepted a noncanonical report path") + } } func TestSHA256AndTruncateErrorMessage(t *testing.T) { diff --git a/internal/comparison/publish.go b/internal/comparison/publish.go index 14e4e54..1415635 100644 --- a/internal/comparison/publish.go +++ b/internal/comparison/publish.go @@ -239,6 +239,9 @@ func readBundleFile(directory, name string) ([]byte, error) { } func decodeManifest(data []byte) (Manifest, error) { + if err := validateManifestJSON(data); err != nil { + return Manifest{}, unrecognizedBundleError("decode manifest", err) + } decoder := json.NewDecoder(bytes.NewReader(data)) decoder.DisallowUnknownFields() var manifest Manifest @@ -255,6 +258,162 @@ func decodeManifest(data []byte) (Manifest, error) { return manifest, nil } +var manifestFields = map[string]jsonValueValidator{ + "schemaVersion": nil, + "comparisonId": nil, + "startedAt": nil, + "finishedAt": nil, + "reportId": nil, + "validPeriod": validateValidPeriodJSON, + "timezone": nil, + "promptId": nil, + "promptVersion": nil, + "promptHash": nil, + "dataPackage": validateDataPackageJSON, + "total": nil, + "succeeded": nil, + "failed": nil, + "results": validateResultsJSON, +} + +var validPeriodFields = map[string]jsonValueValidator{ + "start": nil, + "end": nil, +} + +var dataPackageFields = map[string]jsonValueValidator{ + "path": nil, + "sha256": nil, +} + +var resultFields = map[string]jsonValueValidator{ + "position": nil, + "profileId": nil, + "backendId": nil, + "modelName": nil, + "status": nil, + "validationStatus": nil, + "reportPath": nil, + "error": validateSafeErrorJSON, +} + +var safeErrorFields = map[string]jsonValueValidator{ + "category": nil, + "message": nil, +} + +type jsonValueValidator func(*json.Decoder) error + +func validateManifestJSON(data []byte) error { + decoder := json.NewDecoder(bytes.NewReader(data)) + if err := validateJSONObject(decoder, manifestFields); err != nil { + return err + } + if _, err := decoder.Token(); err != io.EOF { + if err == nil { + return fmt.Errorf("manifest has multiple JSON values") + } + return err + } + return nil +} + +func validateValidPeriodJSON(decoder *json.Decoder) error { + return validateJSONObject(decoder, validPeriodFields) +} + +func validateDataPackageJSON(decoder *json.Decoder) error { + return validateJSONObject(decoder, dataPackageFields) +} + +func validateResultsJSON(decoder *json.Decoder) error { + token, err := decoder.Token() + if err != nil { + return err + } + if delimiter, ok := token.(json.Delim); !ok || delimiter != '[' { + return fmt.Errorf("results must be an array") + } + for decoder.More() { + if err := validateJSONObject(decoder, resultFields); err != nil { + return err + } + } + token, err = decoder.Token() + if err != nil { + return err + } + if delimiter, ok := token.(json.Delim); !ok || delimiter != ']' { + return fmt.Errorf("results has an invalid array terminator") + } + return nil +} + +func validateSafeErrorJSON(decoder *json.Decoder) error { + token, err := decoder.Token() + if err != nil { + return err + } + if token == nil { + return nil + } + if delimiter, ok := token.(json.Delim); !ok || delimiter != '{' { + return fmt.Errorf("error must be an object") + } + return validateJSONObjectBody(decoder, safeErrorFields) +} + +func validateJSONObject(decoder *json.Decoder, fields map[string]jsonValueValidator) error { + token, err := decoder.Token() + if err != nil { + return err + } + if delimiter, ok := token.(json.Delim); !ok || delimiter != '{' { + return fmt.Errorf("manifest value must be an object") + } + return validateJSONObjectBody(decoder, fields) +} + +func validateJSONObjectBody(decoder *json.Decoder, fields map[string]jsonValueValidator) error { + seen := make(map[string]struct{}, len(fields)) + for decoder.More() { + token, err := decoder.Token() + if err != nil { + return err + } + name, ok := token.(string) + if !ok { + return fmt.Errorf("manifest field name is invalid") + } + validator, known := fields[name] + if !known { + return fmt.Errorf("manifest field %q is not canonical", name) + } + if _, duplicate := seen[name]; duplicate { + return fmt.Errorf("manifest field %q is duplicated", name) + } + seen[name] = struct{}{} + if validator == nil { + var value json.RawMessage + if err := decoder.Decode(&value); err != nil { + return err + } + continue + } + if err := validator(decoder); err != nil { + return err + } + } + token, err := decoder.Token() + if err != nil { + return err + } + if delimiter, ok := token.(json.Delim); !ok || delimiter != '}' { + return fmt.Errorf("manifest object has an invalid terminator") + } + return nil +} + func unrecognizedBundleError(action string, err error) error { if err == nil { return fmt.Errorf("%w: %s", ErrUnrecognizedBundle, action) diff --git a/internal/comparison/publish_test.go b/internal/comparison/publish_test.go index 5394f37..4a88624 100644 --- a/internal/comparison/publish_test.go +++ b/internal/comparison/publish_test.go @@ -138,6 +138,14 @@ func TestRecognizeBundleRejectsUnsafeAndMalformedContents(t *testing.T) { t.Fatal(err) } }}, + {name: "duplicate manifest field", mutate: func(t *testing.T, directory string) { + t.Helper() + appendManifestField(t, directory, `"schemaVersion": "weatherreporter.comparison.v1"`) + }}, + {name: "case variant manifest field", mutate: func(t *testing.T, directory string) { + t.Helper() + appendManifestField(t, directory, `"SchemaVersion": "weatherreporter.comparison.v1"`) + }}, {name: "traversal report path", mutate: func(t *testing.T, directory string) { t.Helper() path := filepath.Join(directory, ManifestFilename) @@ -158,6 +166,29 @@ func TestRecognizeBundleRejectsUnsafeAndMalformedContents(t *testing.T) { t.Fatal(err) } }}, + {name: "noncanonical report path", mutate: func(t *testing.T, directory string) { + t.Helper() + path := filepath.Join(directory, ManifestFilename) + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + var manifest Manifest + if err := json.Unmarshal(data, &manifest); err != nil { + t.Fatal(err) + } + if err := os.Rename(filepath.Join(directory, manifest.Results[0].ReportPath), filepath.Join(directory, "arbitrary.md")); err != nil { + t.Fatal(err) + } + manifest.Results[0].ReportPath = "arbitrary.md" + data, err = json.Marshal(manifest) + if err != nil { + t.Fatal(err) + } + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatal(err) + } + }}, } for _, test := range tests { @@ -171,6 +202,19 @@ func TestRecognizeBundleRejectsUnsafeAndMalformedContents(t *testing.T) { } } +func appendManifestField(t *testing.T, directory, field string) { + t.Helper() + path := filepath.Join(directory, ManifestFilename) + data, err := os.ReadFile(path) + if err != nil { + t.Fatal(err) + } + data = append(data[:len(data)-2], []byte(",\n "+field+"\n}\n")...) + if err := os.WriteFile(path, data, 0o600); err != nil { + t.Fatal(err) + } +} + func TestPlanDestinationAcceptsRecognizedReplacement(t *testing.T) { directory := publishTestBundle(t, testBundle()) plan, err := PlanDestination(filepath.Dir(directory), directory, true)