From 42fb4aa82ad37eabc087a6cfb22a31e7afc2cd41 Mon Sep 17 00:00:00 2001 From: Eric Rakestraw Date: Tue, 2 Jun 2026 18:38:16 +0000 Subject: [PATCH] Centralize link URL validation --- docs/internal/link.md | 27 +++++++++++++++++++++ docs/policy/architecture.md | 1 + internal/config/validate.go | 25 +++---------------- internal/config/validate_test.go | 27 ++++++++++++++++++++- internal/link/url.go | 26 ++++++++++++++++++++ internal/link/url_test.go | 39 ++++++++++++++++++++++++++++++ internal/publish/links.go | 4 +++ internal/publish/links_test.go | 20 +++++++++++++++ internal/state/distributor_test.go | 17 +++++++++++++ internal/state/validate.go | 26 +++----------------- 10 files changed, 166 insertions(+), 46 deletions(-) create mode 100644 docs/internal/link.md create mode 100644 internal/link/url.go create mode 100644 internal/link/url_test.go diff --git a/docs/internal/link.md b/docs/internal/link.md new file mode 100644 index 0000000..a3b99fd --- /dev/null +++ b/docs/internal/link.md @@ -0,0 +1,27 @@ +# Link URL Policy + +## Purpose + +`internal/link` defines shared validation for configured and persisted HTTP link URLs. + +## Inputs and outputs + +Input is a URL string. Output is either nil for an accepted URL or a concise validation error that callers wrap with field context. + +## Validation behavior + +Accepted URLs must parse successfully, use `http` or `https`, include a host, and omit query strings and fragments. + +## Boundaries + +This package validates URL shape only. It does not construct destination output URLs, choose primary URLs, infer public URLs from backend configuration, or read configuration files. + +## Tests + +Before changing link URL policy, inspect tests under `internal/link` and callers in `internal/config`, `internal/state`, and `internal/publish`. + +## Invariants + +- Configured `links.base_url`, persisted `links.primary_url`, persisted output `url`, and publish link planning use the same URL policy. +- Callers own field-specific error context. +- URL path construction remains in `internal/publish`. diff --git a/docs/policy/architecture.md b/docs/policy/architecture.md index 3755715..03d18e4 100644 --- a/docs/policy/architecture.md +++ b/docs/policy/architecture.md @@ -202,6 +202,7 @@ Use this current layout unless the project has a documented reason to differ: - `internal/config`: configuration structs, defaults, loading, precedence, and validation. - `internal/bundle`: storage-backed source bundle discovery and validation over the public manifest contract. - `internal/state`: `.distributor.json` parsing, validation, comparison, and output metadata. +- `internal/link`: shared HTTP URL validation for configured and persisted link metadata. - `internal/storage`: backend interfaces, shared path/resource types, backend registry, and storage errors. - `internal/adapters/local`: local filesystem backend. - `internal/adapters/ssh`: SSH/SFTP backend. diff --git a/internal/config/validate.go b/internal/config/validate.go index bca91ad..8968bed 100644 --- a/internal/config/validate.go +++ b/internal/config/validate.go @@ -2,9 +2,10 @@ package config import ( "fmt" - "net/url" "regexp" "strings" + + "gitea.maximumdirect.net/eric/distributor/internal/link" ) var idPattern = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9._-]*$`) @@ -187,7 +188,7 @@ func validateLinks(errs ValidationErrors, context string, links *Links) Validati } if links.BaseURL == "" { errs = append(errs, context+".base_url is required") - } else if err := validateLinkBaseURL(links.BaseURL); err != nil { + } else if err := link.ValidateHTTPURL(links.BaseURL); err != nil { errs = append(errs, context+".base_url "+err.Error()) } switch links.Primary { @@ -198,26 +199,6 @@ func validateLinks(errs ValidationErrors, context string, links *Links) Validati return errs } -func validateLinkBaseURL(value string) error { - parsed, err := url.Parse(value) - if err != nil { - return fmt.Errorf("must be a valid URL") - } - if parsed.Scheme != "http" && parsed.Scheme != "https" { - return fmt.Errorf("must use http or https") - } - if parsed.Host == "" { - return fmt.Errorf("must include a host") - } - if parsed.RawQuery != "" { - return fmt.Errorf("must not include a query string") - } - if parsed.Fragment != "" { - return fmt.Errorf("must not include a fragment") - } - return nil -} - func validateTransferPolicy(errs ValidationErrors, context string, policy TransferPolicy) ValidationErrors { if policy.OnDestinationSame != TransferActionSkip && policy.OnDestinationSame != TransferActionFail { errs = append(errs, context+".on_destination_same must be skip or fail") diff --git a/internal/config/validate_test.go b/internal/config/validate_test.go index 53de7df..a0e7b82 100644 --- a/internal/config/validate_test.go +++ b/internal/config/validate_test.go @@ -1,6 +1,9 @@ package config -import "testing" +import ( + "strings" + "testing" +) func TestValidatePublishTransformPolicy(t *testing.T) { tests := publishTransformPolicyCases() @@ -145,6 +148,28 @@ func TestValidateLinks(t *testing.T) { } } +func TestValidateLinksReportsFieldContext(t *testing.T) { + cfg := Config{Pipelines: []Pipeline{{ + ID: "reports", + Source: Backend{Backend: BackendLocal, Path: "/source"}, + Destinations: []Destination{{ + ID: "web", + Backend: BackendLocal, + Path: "/destination", + Links: &Links{BaseURL: "https://reports.example.com/archive?preview=1", Primary: LinkPrimaryAuto}, + }}, + }}} + ApplyDefaults(&cfg) + err := Validate(cfg) + if err == nil { + t.Fatal("Validate() error = nil, want error") + } + want := "pipelines[0].destinations[0].links.base_url must not include a query string" + if !strings.Contains(err.Error(), want) { + t.Fatalf("Validate() error = %q, want %q", err, want) + } +} + type publishTransformPolicyCase struct { name string publish PublishPolicy diff --git a/internal/link/url.go b/internal/link/url.go new file mode 100644 index 0000000..7eb03e9 --- /dev/null +++ b/internal/link/url.go @@ -0,0 +1,26 @@ +package link + +import ( + "fmt" + "net/url" +) + +func ValidateHTTPURL(value string) error { + parsed, err := url.Parse(value) + if err != nil { + return fmt.Errorf("must be a valid URL") + } + if parsed.Scheme != "http" && parsed.Scheme != "https" { + return fmt.Errorf("must use http or https") + } + if parsed.Host == "" { + return fmt.Errorf("must include a host") + } + if parsed.RawQuery != "" { + return fmt.Errorf("must not include a query string") + } + if parsed.Fragment != "" { + return fmt.Errorf("must not include a fragment") + } + return nil +} diff --git a/internal/link/url_test.go b/internal/link/url_test.go new file mode 100644 index 0000000..678b643 --- /dev/null +++ b/internal/link/url_test.go @@ -0,0 +1,39 @@ +package link + +import ( + "strings" + "testing" +) + +func TestValidateHTTPURL(t *testing.T) { + tests := []struct { + name string + value string + wantErr string + }{ + {name: "http", value: "http://reports.example.com/archive"}, + {name: "https", value: "https://reports.example.com/archive"}, + {name: "missing host", value: "https:///archive", wantErr: "must include a host"}, + {name: "unsupported scheme", value: "ftp://reports.example.com/archive", wantErr: "must use http or https"}, + {name: "query string", value: "https://reports.example.com/archive?preview=1", wantErr: "must not include a query string"}, + {name: "fragment", value: "https://reports.example.com/archive#top", wantErr: "must not include a fragment"}, + {name: "parse failure", value: "http://[::1", wantErr: "must be a valid URL"}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := ValidateHTTPURL(tt.value) + if tt.wantErr == "" { + if err != nil { + t.Fatalf("ValidateHTTPURL() error = %v", err) + } + return + } + if err == nil { + t.Fatal("ValidateHTTPURL() error = nil, want error") + } + if !strings.Contains(err.Error(), tt.wantErr) { + t.Fatalf("ValidateHTTPURL() error = %q, want %q", err, tt.wantErr) + } + }) + } +} diff --git a/internal/publish/links.go b/internal/publish/links.go index 1c1fe7c..706f195 100644 --- a/internal/publish/links.go +++ b/internal/publish/links.go @@ -6,6 +6,7 @@ import ( "strings" "gitea.maximumdirect.net/eric/distributor/internal/config" + "gitea.maximumdirect.net/eric/distributor/internal/link" "gitea.maximumdirect.net/eric/distributor/internal/state" ) @@ -13,6 +14,9 @@ func PlanLinks(req Request, outputs []Output) ([]Output, string, error) { if req.Links == nil { return outputs, "", nil } + if err := link.ValidateHTTPURL(req.Links.BaseURL); err != nil { + return nil, "", fmt.Errorf("link base URL: %w", err) + } linked := make([]Output, 0, len(outputs)) for _, output := range outputs { outputURL, err := OutputURL(req.Links.BaseURL, req.DestinationBundlePath, output.DestinationPath) diff --git a/internal/publish/links_test.go b/internal/publish/links_test.go index c9c5b13..e0dda5c 100644 --- a/internal/publish/links_test.go +++ b/internal/publish/links_test.go @@ -1,6 +1,7 @@ package publish import ( + "strings" "testing" "gitea.maximumdirect.net/eric/distributor/internal/config" @@ -147,3 +148,22 @@ func TestPlanLinksLeavesOutputsUnchangedWithoutConfig(t *testing.T) { t.Fatalf("output URL = %q, want empty", linked[0].URL) } } + +func TestPlanLinksValidatesBaseURLBeforePlanning(t *testing.T) { + _, _, err := PlanLinks(Request{ + DestinationBundlePath: "daily", + Links: &config.Links{ + BaseURL: "https://reports.example.com/archive?preview=1", + Primary: config.LinkPrimaryAuto, + }, + }, []Output{{ + DestinationPath: "report.md", + Kind: state.OutputKindSource, + }}) + if err == nil { + t.Fatal("PlanLinks() error = nil, want error") + } + if !strings.Contains(err.Error(), "link base URL: must not include a query string") { + t.Fatalf("PlanLinks() error = %q, want link base URL context", err) + } +} diff --git a/internal/state/distributor_test.go b/internal/state/distributor_test.go index 074843f..c5bc7f6 100644 --- a/internal/state/distributor_test.go +++ b/internal/state/distributor_test.go @@ -179,6 +179,23 @@ func TestParseRejectsInvalidOutputMetadata(t *testing.T) { } } +func TestValidateReportsURLFieldContext(t *testing.T) { + source := validManifest(t) + state := *withState(t, source, func(s *DistributorState) { + s.Links = &LinkState{PrimaryURL: "https://reports.example.com/archive#top"} + }) + err := Validate(state) + assertStateErrorContains(t, err, "state links.primary_url") + assertStateErrorContains(t, err, "must not include a fragment") + + state = *withState(t, source, func(s *DistributorState) { + s.Outputs[0].URL = "https://reports.example.com/archive?preview=1" + }) + err = Validate(state) + assertStateErrorContains(t, err, "state outputs[0].url") + assertStateErrorContains(t, err, "must not include a query string") +} + func TestParseRejectsMalformedPublishedTimestamp(t *testing.T) { body := strings.Replace(validStateJSON(t), `"published_at": "2026-05-30T11:12:00Z"`, `"published_at": "May 30"`, 1) _, err := Parse([]byte(body)) diff --git a/internal/state/validate.go b/internal/state/validate.go index e3e93c4..31a4782 100644 --- a/internal/state/validate.go +++ b/internal/state/validate.go @@ -2,9 +2,9 @@ package state import ( "fmt" - "net/url" "gitea.maximumdirect.net/eric/distributor/internal/bundle" + "gitea.maximumdirect.net/eric/distributor/internal/link" "gitea.maximumdirect.net/eric/distributor/internal/storage" ) @@ -30,7 +30,7 @@ func Validate(s DistributorState) error { return fmt.Errorf("state source.manifest: %w", err) } if s.Links != nil && s.Links.PrimaryURL != "" { - if err := validateStateURL(s.Links.PrimaryURL); err != nil { + if err := link.ValidateHTTPURL(s.Links.PrimaryURL); err != nil { return fmt.Errorf("state links.primary_url: %w", err) } } @@ -70,7 +70,7 @@ func validateOutput(index int, output OutputFile) error { return fmt.Errorf("state outputs[%d].transform is required for generated output", index) } if output.URL != "" { - if err := validateStateURL(output.URL); err != nil { + if err := link.ValidateHTTPURL(output.URL); err != nil { return fmt.Errorf("state outputs[%d].url: %w", index, err) } } @@ -82,23 +82,3 @@ func validateOutput(index int, output OutputFile) error { } return nil } - -func validateStateURL(value string) error { - parsed, err := url.Parse(value) - if err != nil { - return err - } - if parsed.Scheme != "http" && parsed.Scheme != "https" { - return fmt.Errorf("must use http or https") - } - if parsed.Host == "" { - return fmt.Errorf("must include a host") - } - if parsed.RawQuery != "" { - return fmt.Errorf("must not include a query string") - } - if parsed.Fragment != "" { - return fmt.Errorf("must not include a fragment") - } - return nil -}