diff --git a/README.md b/README.md index 23ee482..22a9207 100644 --- 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 ``, 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 < 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: diff --git a/console/import_winter.go b/console/import_winter.go index eb90176..6a16887 100644 --- 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 } diff --git a/console/import_winter_test.go b/console/import_winter_test.go new file mode 100644 index 0000000..97dcae8 --- /dev/null +++ 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"/> &. Leave it empty to keep the backend URL.' definition: not_found: No sitemap definition was found. Try creating one first. title: Sitemap definition diff --git a/lang/pl/lang.yaml b/lang/pl/lang.yaml index 072abe0..0cc1cb3 100644 --- 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 diff --git a/models/settings.go b/models/settings.go index 5134540..207335b 100644 --- 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 diff --git a/models/settings_test.go b/models/settings_test.go index 925bcbe..43db04e 100644 --- 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"/>