fix(17): T-17-15 refuse an unsafe frontend_url on save and import

- Settings.BeforeValidate answers a cabana validation error on frontend_url, so the settings save is a 422 and the row is unchanged - sitemap:import-winter stops on a frontend_url that models.AllowedFrontendURL refuses, before the dry-run branch and before any database use; the value is printed quoted - errors.frontend_url phrase in en and pl - TestSettingsBeforeValidate, TestWinterSettingsFrontendURL and the save and import refusals in the Postgres smokes
2026-10-13 00:59:24 +02:00
parent b9804ddf17
commit 1bb89ce006
9 changed files with 204 additions and 8 deletions
--- a/README.md
+++ b/README.md
@@ -109,6 +109,8 @@ A save also refuses, per row, a missing `reference` for a registry type that tak
 | `enable_priority_tiers` | on | Tier priorities replace item priorities. |
 | `priority_homepage`, `priority_services`, `priority_blog`, `priority_categories`, `priority_legal` | `1.0`, `0.8`, `0.6`, `0.4`, `0.2` | The tier values, stored as text. |

+A save refuses a `frontend_url` outside the accepted form with a 422 keyed `frontend_url` (phrase `golem15.sitemap::lang.errors.frontend_url`), and `sitemap:import-winter` stops with an error. The PHP plugin stored any text and wrote it unescaped into `<loc>`, the hreflang links and the stylesheet instruction.
+
 The updates manifest is read only when it is a regular file of at most 5 MiB (`classes.ManifestMaxBytes`) holding a JSON object; paths match with the leading slash added and the trailing slash removed (`/` stays). Any error omits `lastmod` and is logged without the file's content. The manifest is a frontend build output, for example a script that writes the git commit date of each static page's source:

 ```json
@@ -191,7 +193,7 @@ The WinterCMS forms are adapted to the admin's YAML:
 | `models.DefinitionItem`, `models.DefinitionItems` | One item and the stored list, with the tolerant PHP-shaped decoding. |
 | `models.ItemTypeURL` | The built-in item type `url`. |
 | `models.Text`, `models.DecodeText` | A text field's value; the tolerant decoding of one stored text value. |
-| `models.Settings`, `models.DefaultSettings` | The settings row and its PHP defaults; `Settings.Priority` reads a tier value. |
+| `models.Settings`, `models.DefaultSettings` | The settings row and its PHP defaults; `Settings.Priority` reads a tier value, `Settings.BeforeValidate` refuses a `frontend_url` outside the accepted form on a save. |
 | `models.AllowedFrontendURL` | Whether a value may be the `frontend_url` setting: empty, or an absolute `http`/`https` URL with a host and without whitespace, control characters, `"`, `<`, `>` or `&`. |
 | `models.PriorityHomepage`, `models.PriorityServices`, `models.PriorityBlog`, `models.PriorityCategories`, `models.PriorityLegal`, `models.PriorityDefaults` | The tier setting keys and their defaults. |
 | `controllers.DefinitionsControllerID`, `controllers.PermissionAccessDefinitions` | `golem15.sitemap.definitions` and `golem15.sitemap.access_definitions`. |
@@ -214,7 +216,7 @@ The WinterCMS forms are adapted to the admin's YAML:
 | Command | Arguments and flags | Effect |
 |---------|---------------------|--------|
 | `sitemap:import` | `FILE`, `--theme`, `--append` | The PHP `ImportCommand`: reads a JSON array of `{url, changefreq, priority}` entries (`monthly` and `0.5` by default). Without `--append` the definition keeps its non-URL items first and the imported URLs replace its URL items; with it the URLs follow every existing item. The definition is always `default`; a `--theme` other than `default` is reported. An entry whose URL `classes.AllowedItemURL` refuses stops the import. |
-| `sitemap:import-winter` | `--definitions FILE`, `--settings FILE`, `--theme`, `--map old=new` (repeatable), `--page-url reference=/path` (repeatable), `--force`, `--dry-run` | Copies a WinterCMS export into the definition and the settings row. URL items are kept; static and CMS page items are dropped unless `--page-url` names the URL of their reference (for example `--page-url blog=/blog` copies the CMS page item `blog` as a url item `/blog`); `--map` renames types first (for example `--map blog-post=journal-post`); an item of an unknown type, or whose reference is not among its type's current references, is dropped; a url item whose URL `classes.AllowedItemURL` refuses is dropped, and so is a page item whose `--page-url` it refuses; a `urlPattern` that is not a path is cleared. Every dropped, remapped or cleared item and every dropped `cmsPage` is printed. A definition that already has items is refused without `--force`; `--dry-run` writes nothing. Settings missing from the export take the defaults. `--theme` picks a definition when the export holds several. |
+| `sitemap:import-winter` | `--definitions FILE`, `--settings FILE`, `--theme`, `--map old=new` (repeatable), `--page-url reference=/path` (repeatable), `--force`, `--dry-run` | Copies a WinterCMS export into the definition and the settings row. URL items are kept; static and CMS page items are dropped unless `--page-url` names the URL of their reference (for example `--page-url blog=/blog` copies the CMS page item `blog` as a url item `/blog`); `--map` renames types first (for example `--map blog-post=journal-post`); an item of an unknown type, or whose reference is not among its type's current references, is dropped; a url item whose URL `classes.AllowedItemURL` refuses is dropped, and so is a page item whose `--page-url` it refuses; a `urlPattern` that is not a path is cleared. Every dropped, remapped or cleared item and every dropped `cmsPage` is printed. A definition that already has items is refused without `--force`; `--dry-run` writes nothing. Settings missing from the export take the defaults; a `frontend_url` which `models.AllowedFrontendURL` refuses stops the import before anything is written, also with `--dry-run`. `--theme` picks a definition when the export holds several. |

 Both commands open the database themselves when the application has not published one.

@@ -229,7 +231,7 @@ mysql -N -B --raw <database> < scripts/winter-export-settings.sql > sitemap-sett

 ## Dependencies

- SummerCMS modules: backpack, bonfire, compass, festival, lagoon, pact, party, signpost, surf; bouncer and cabana in tests.
+- SummerCMS modules: backpack, bonfire, cabana, compass, festival, lagoon, pact, party, signpost, surf; bouncer in tests. cabana is a regular dependency: `models` and `controllers` return its validation error.
 - Third-party: `gorm.io/gorm`, `github.com/go-gormigrate/gormigrate/v2`. Test-only: `github.com/testcontainers/testcontainers-go`, `github.com/jackc/pgx/v5`.

 ## Testing
@@ -239,7 +241,7 @@ go test -short ./...   # goldens and config checks, no Docker
 go test ./...          # also the Postgres smoke tests
 ```

-`TestSitemapGolden` (`classes`) feeds `testdata/tracer.json`, `urlto.json` and `full.json` through the Go port and compares bytes with the goldens; it also checks the 50000 URL cap. `TestSitemapTiersConfig` checks the tier rule refusals, a host's replacement of the rule list and the manifest's fail-safe reads. The Postgres smokes (`TestSitemapTracer`, `TestSitemapImportSmoke`, `TestSitemapAdminSmoke`) boot the plugin with a test content plugin and drive the routes, the import commands and the admin API.
+`TestSitemapGolden` (`classes`) feeds `testdata/tracer.json`, `urlto.json` and `full.json` through the Go port and compares bytes with the goldens; it also checks the 50000 URL cap. `TestSitemapTiersConfig` checks the tier rule refusals, a host's replacement of the rule list and the manifest's fail-safe reads. `TestRewriteUnsafeFrontendURL` (`classes`), `TestAllowedFrontendURL` and `TestSettingsBeforeValidate` (`models`) and `TestWinterSettingsFrontendURL` (`console`) are the Docker-free checks of the `frontend_url` rules: the accepted form, the rewrite that is not applied for any other value, the save hook and the import refusal. The Postgres smokes (`TestSitemapTracer`, `TestSitemapImportSmoke`, `TestSitemapAdminSmoke`, `TestImportWinter`, `TestSitemapCacheAndThrottle`) boot the plugin with a test content plugin and drive the routes, the import commands and the admin API; they cover the 422 of a refused `frontend_url` save and a value stored straight in the row, which the sitemap does not apply.

 The goldens are generated from the PHP plugin's generation code and the vendored Winter URL generator of any WinterCMS application, and committed, so CI needs no PHP:

--- a/console/import_winter.go
+++ b/console/import_winter.go
@@ -37,7 +37,8 @@ var droppedTypes = map[string]bool{
 // are checked. PageURLs turns a static or CMS page item into a url item:
 // its key is the item's reference (the PHP page file name, for example
 // blog), its value the page's URL (/blog). Force replaces a definition that
-// has items; DryRun reports without writing.
+// has items; DryRun reports without writing. A frontend_url that
+// models.AllowedFrontendURL refuses stops the import and nothing is written.
 type WinterOptions struct {
 	DefinitionsFile string
 	SettingsFile    string
@@ -71,7 +72,8 @@ type WinterReport struct {
 // (signpost.ValidItemPattern) is cleared, and every dropped, remapped or
 // cleared item is printed. A definition that already has items is refused
 // without --force. Settings keys missing from the export take the PHP
-// defaults.
+// defaults. A frontend_url that models.AllowedFrontendURL refuses stops the
+// import and nothing is written.
 func ImportWinter(app *backpack.App) bonfire.Command {
 	return bonfire.Command{
 		Name:        "sitemap:import-winter",
@@ -418,7 +420,8 @@ func describeItem(label string, item models.DefinitionItem) string {
 }

 // readWinterSettings reads the exported settings object onto the PHP
-// defaults.
+// defaults. A frontend_url that models.AllowedFrontendURL refuses is an
+// error; the value is quoted so its control characters are not printed raw.
 func readWinterSettings(file string, report *WinterReport) (models.Settings, error) {
 	raw, err := os.ReadFile(file)
 	if err != nil {
@@ -460,5 +463,8 @@ func readWinterSettings(file string, report *WinterReport) (models.Settings, err
 		}
 		report.Lines = append(report.Lines, fmt.Sprintf("setting '%s' ignored (not a sitemap setting of this port)", key))
 	}
+	if url := models.Text(s.FrontendURL); !models.AllowedFrontendURL(url) {
+		return models.Settings{}, fmt.Errorf("%s: setting frontend_url %q must be empty or an absolute http:// or https:// URL without whitespace, control characters, quotes or the characters <, > and &", file, url)
+	}
 	return s, nil
 }
--- a/console/import_winter_test.go
+++ b/console/import_winter_test.go
@@ -0,0 +1,92 @@
+package console
+
+import (
+	"bytes"
+	"context"
+	"encoding/json"
+	"os"
+	"path/filepath"
+	"strings"
+	"testing"
+
+	"git.golem15.com/golem15/sm-sitemap-plugin/models"
+	"git.golem15.com/golem15/summercms/modules/bonfire"
+)
+
+// TestWinterSettingsFrontendURL pins the import check of T-17-15: a settings
+// export whose frontend_url models.AllowedFrontendURL refuses stops
+// sitemap:import-winter with an error naming the setting, with and without
+// --dry-run and before the database is used (the test passes none). An
+// accepted, empty, null or missing frontend_url is read as before.
+func TestWinterSettingsFrontendURL(t *testing.T) {
+	dir := t.TempDir()
+	n := 0
+	settingsFile := func(fields map[string]any) string {
+		t.Helper()
+		// json.Marshal encodes the quotes of the attack value.
+		raw, err := json.Marshal(fields)
+		if err != nil {
+			t.Fatal(err)
+		}
+		n++
+		path := filepath.Join(dir, "settings-"+string(rune('a'+n))+".json")
+		if err := os.WriteFile(path, raw, 0o644); err != nil {
+			t.Fatal(err)
+		}
+		return path
+	}
+
+	for _, value := range []string{
+		"javascript:alert(document.domain)//",
+		`https://x"/><y a="`,
+		"0",
+		" https://www.example.test",
+	} {
+		file := settingsFile(map[string]any{"frontend_url": value, "enable_priority_tiers": "1"})
+		var report WinterReport
+		if _, err := readWinterSettings(file, &report); err == nil || !strings.Contains(err.Error(), "setting frontend_url") {
+			t.Errorf("readWinterSettings(%q) = %v, want an error naming setting frontend_url", value, err)
+		}
+		for _, dry := range []bool{false, true} {
+			var buf bytes.Buffer
+			out := bonfire.NewOutput(strings.NewReader(""), &buf, &buf)
+			report, err := ImportWinterFiles(context.Background(), nil, nil, WinterOptions{SettingsFile: file, DryRun: dry}, out)
+			if err == nil || !strings.Contains(err.Error(), "setting frontend_url") {
+				t.Errorf("ImportWinterFiles(%q, dry run %v) = %v, want an error naming setting frontend_url", value, dry, err)
+			}
+			if report.Settings {
+				t.Errorf("ImportWinterFiles(%q, dry run %v) reports the settings as copied", value, dry)
+			}
+			if strings.Contains(buf.String(), "would be copied") || strings.Contains(buf.String(), "Settings copied") {
+				t.Errorf("ImportWinterFiles(%q, dry run %v) printed a copy line:\n%s", value, dry, buf.String())
+			}
+		}
+	}
+
+	// The error quotes the value, so a control character is not printed raw.
+	var report WinterReport
+	_, err := readWinterSettings(settingsFile(map[string]any{"frontend_url": "https://www.example.test\x1b[2J"}), &report)
+	if err == nil || strings.Contains(err.Error(), "\x1b") || !strings.Contains(err.Error(), `\x1b`) {
+		t.Errorf("a control character in frontend_url: %q", err)
+	}
+
+	for name, tc := range map[string]struct {
+		fields map[string]any
+		want   string
+	}{
+		"valid":   {map[string]any{"frontend_url": "https://www.example.test/"}, "https://www.example.test/"},
+		"empty":   {map[string]any{"frontend_url": ""}, ""},
+		"null":    {map[string]any{"frontend_url": nil}, ""},
+		"missing": {map[string]any{"enable_priority_tiers": "1"}, ""},
+	} {
+		var report WinterReport
+		s, err := readWinterSettings(settingsFile(tc.fields), &report)
+		if err != nil {
+			t.Errorf("%s: readWinterSettings = %v", name, err)
+			continue
+		}
+		if got := models.Text(s.FrontendURL); got != tc.want {
+			t.Errorf("%s: frontend_url = %q, want %q", name, got, tc.want)
+		}
+	}
+}
--- a/import_test.go
+++ b/import_test.go
@@ -213,5 +213,19 @@ func TestImportWinter(t *testing.T) {
 		if err := e.gdb.Find(&rows).Error; err != nil || models.Text(rows[0].FrontendURL) != "https://other.example.test" {
 			t.Fatalf("the dry run changed the settings: %+v %v", rows, err)
 		}
+
+		// T-17-15: a frontend_url that is not a plain http(s) URL stops the
+		// import, also as a dry run, and the row keeps its value.
+		quoted, _ := json.Marshal(map[string]string{"frontend_url": `https://x"/><y a="`})
+		unsafe := writeFile(t, dir, "settings4.json", string(quoted))
+		for _, dryRun := range []bool{false, true} {
+			report, out, err := run(console.WinterOptions{SettingsFile: unsafe, DryRun: dryRun})
+			if err == nil || !strings.Contains(err.Error(), "frontend_url") || report.Settings {
+				t.Fatalf("unsafe frontend_url (dry run %v): %+v %v\n%s", dryRun, report, err, out)
+			}
+			if err := e.gdb.Find(&rows).Error; err != nil || len(rows) != 1 || models.Text(rows[0].FrontendURL) != "https://other.example.test" {
+				t.Fatalf("a refused import (dry run %v) changed the settings: %+v %v", dryRun, rows, err)
+			}
+		}
 	})
 }
--- a/lang/en/lang.yaml
+++ b/lang/en/lang.yaml
@@ -40,6 +40,7 @@ item:
 errors:
  url_scheme: Use a path such as /about, or a link that starts with http:// or https://.
  url_pattern: Use a path that starts with a single /, for example /blog/:slug.
+  frontend_url: 'Use a full address that starts with http:// or https://, without spaces, quotes or the characters < > &. Leave it empty to keep the backend URL.'
 definition:
  not_found: No sitemap definition was found. Try creating one first.
  title: Sitemap definition
--- a/lang/pl/lang.yaml
+++ b/lang/pl/lang.yaml
@@ -40,6 +40,7 @@ item:
 errors:
  url_scheme: Podaj ścieżkę, np. /o-nas, albo link zaczynający się od http:// lub https://.
  url_pattern: Podaj ścieżkę zaczynającą się od jednego /, np. /blog/:slug.
+  frontend_url: 'Podaj pełny adres zaczynający się od http:// lub https://, bez spacji, cudzysłowów i znaków < > &. Zostaw puste, aby zachować adres backendu.'
 definition:
  not_found: Nie znaleziono definicji mapy strony. Musisz ją utworzyć.
  title: Definicja mapy strony
--- a/models/settings.go
+++ b/models/settings.go
@@ -5,8 +5,14 @@ import (
 	"strings"
 	"unicode"
 	"unicode/utf8"
+
+	"git.golem15.com/golem15/summercms/modules/cabana"
+	"gorm.io/gorm"
 )

+// errFrontendURL is the phrase key of the frontend_url refusal.
+const errFrontendURL = "golem15.sitemap::lang.errors.frontend_url"
+
 // The settings keys of the five priority tiers, in the order PHP checked
 // them. A tier rule names one of them.
 const (
@@ -34,6 +40,8 @@ var PriorityDefaults = map[string]string{
 // UpdatesManifestPath names a JSON file of URL path to last-modified date;
 // EnablePriorityTiers turns the tier priorities on, and the priority fields
 // hold each tier's value as text. A null priority reads as its PHP default.
+// The PHP settings form validated nothing; the port checks FrontendURL in
+// BeforeValidate.
 type Settings struct {
 	ID                    uint    `gorm:"column:id;primaryKey"`
 	FrontendURL           *string `gorm:"column:frontend_url"`
@@ -58,9 +66,21 @@ func (Settings) Fillable() []string {
 	}
 }

-// Rules is empty: the PHP settings form validated nothing.
+// Rules is empty: the PHP settings form validated nothing. The one check of
+// the port, on frontend_url, is in BeforeValidate.
 func (Settings) Rules() map[string]string { return map[string]string{} }

+// BeforeValidate is the lagoon.HasBeforeValidate hook cabana calls on a
+// settings save, after the fill and before the row is written: a
+// frontend_url that AllowedFrontendURL refuses is a validation error on
+// frontend_url, so the save answers 422 and the row is left as it was.
+func (s Settings) BeforeValidate(*gorm.DB) error {
+	if AllowedFrontendURL(Text(s.FrontendURL)) {
+		return nil
+	}
+	return &cabana.ValidationError{Details: map[string]any{"frontend_url": []string{errFrontendURL}}}
+}
+
 // AllowedFrontendURL reports whether u may be the frontend_url setting: the
 // empty string, or an absolute http or https URL with a host and without
 // whitespace, control characters, a double quote, <, > or &. The PHP plugin
--- a/models/settings_test.go
+++ b/models/settings_test.go
@@ -1,11 +1,18 @@
 package models_test

 import (
+	"errors"
+	"reflect"
 	"testing"

 	"git.golem15.com/golem15/sm-sitemap-plugin/models"
+	"git.golem15.com/golem15/summercms/modules/cabana"
+	"git.golem15.com/golem15/summercms/modules/lagoon"
 )

+// cabana calls the hook on a settings save through this interface.
+var _ lagoon.HasBeforeValidate = (*models.Settings)(nil)
+
 // TestAllowedFrontendURL pins the one definition of a frontend_url that may
 // be saved, imported and applied (T-17-15): empty, or an absolute http(s)
 // URL with a host and without whitespace, control characters, a double
@@ -59,3 +66,32 @@ func TestAllowedFrontendURL(t *testing.T) {
 		}
 	}
 }
+
+// TestSettingsBeforeValidate pins the save check of T-17-15: the hook cabana
+// calls on a settings save passes a null, empty or accepted frontend_url and
+// answers a validation error keyed frontend_url for any other value.
+func TestSettingsBeforeValidate(t *testing.T) {
+	text := func(s string) *string { return &s }
+	for _, url := range []*string{nil, text(""), text("https://www.ex.pl/")} {
+		if err := (&models.Settings{FrontendURL: url}).BeforeValidate(nil); err != nil {
+			t.Errorf("BeforeValidate(%q) = %v, want nil", models.Text(url), err)
+		}
+	}
+	want := map[string]any{"frontend_url": []string{"golem15.sitemap::lang.errors.frontend_url"}}
+	for _, url := range []string{
+		"javascript:alert(document.domain)//",
+		`https://x"/><y a="`,
+		"0",
+		" https://www.ex.pl",
+	} {
+		err := (&models.Settings{FrontendURL: &url}).BeforeValidate(nil)
+		var invalid *cabana.ValidationError
+		if !errors.As(err, &invalid) {
+			t.Errorf("BeforeValidate(%q) = %v, want a *cabana.ValidationError", url, err)
+			continue
+		}
+		if !reflect.DeepEqual(invalid.Details, want) {
+			t.Errorf("BeforeValidate(%q) details = %v, want %v", url, invalid.Details, want)
+		}
+	}
+}
--- a/sitemap_smoke_test.go
+++ b/sitemap_smoke_test.go
@@ -620,11 +620,35 @@ func TestSitemapAdminSmoke(t *testing.T) {
 	if err := e.gdb.First(&st, 1).Error; err != nil || models.Text(st.FrontendURL) != "https://www.example.test" {
 		t.Fatalf("settings = %+v %v", st, err)
 	}
+
+	// T-17-15: a frontend URL that is not a plain http(s) URL is a 422 on
+	// frontend_url and the stored row keeps its value.
+	for _, value := range []string{"javascript:alert(document.domain)//", `https://x"/><y a="`, "https://www.example.test "} {
+		rec = e.call(t, http.MethodPut, "/settings/sitemap_settings", token, map[string]any{"frontend_url": value, "enable_priority_tiers": true})
+		if rec.Code != http.StatusUnprocessableEntity || !strings.Contains(rec.Body.String(), `"frontend_url"`) || !strings.Contains(rec.Body.String(), "golem15.sitemap::lang.errors.frontend_url") {
+			t.Fatalf("PUT settings frontend_url=%q: %d %s", value, rec.Code, rec.Body.String())
+		}
+		var kept models.Settings
+		if err := e.gdb.First(&kept, 1).Error; err != nil || models.Text(kept.FrontendURL) != "https://www.example.test" {
+			t.Fatalf("a refused save of frontend_url=%q changed the settings: %+v %v", value, kept, err)
+		}
+	}
+
 	rec = e.call(t, http.MethodGet, "/settings", token, nil)
 	if rec.Code != http.StatusOK || !strings.Contains(rec.Body.String(), `"controller":"golem15.sitemap.definitions"`) {
 		t.Fatalf("settings index: %d %s", rec.Code, rec.Body.String())
 	}

+	// An empty frontend URL saves: the sitemap keeps the backend URL.
+	rec = e.call(t, http.MethodPut, "/settings/sitemap_settings", token, map[string]any{"frontend_url": "", "enable_priority_tiers": true})
+	if rec.Code != http.StatusOK {
+		t.Fatalf("PUT settings with an empty frontend_url: %d %s", rec.Code, rec.Body.String())
+	}
+	var cleared models.Settings
+	if err := e.gdb.First(&cleared, 1).Error; err != nil || models.Text(cleared.FrontendURL) != "" {
+		t.Fatalf("settings after an empty frontend_url = %+v %v", cleared, err)
+	}
+
 	// Without the permission every route answers 403.
 	other := e.admin(t)
 	for _, c := range []struct{ method, path string }{