diff --git a/README.md b/README.md index 5a8db5e..cf2b763 100644 --- a/README.md +++ b/README.md @@ -25,7 +25,7 @@ v0.1 served one `/sitemap.xml` in the WinterCMS byte format from a definition of - `/sitemap.xml` is now a sitemap index; its URL is unchanged, so robots.txt `Sitemap:` lines and Search Console submissions keep working. - The files are `/sitemap--.xml` at the site root. Go serves them at `/_sitemap/files/{file}`; the site's SEO layer (vue-seo-layer) or its proxy maps the root names there. - `/sitemap.xsl` answers 404, and no `` or `` is written. -- The definitions screen and the old settings tile are one settings singleton. The url items of the definition move to the additional addresses, `frontend_url` to the site address and `frontend_default_locale` to the default locale (migration `202610280002`). Reference items are dropped: the content types list their records. The `golem15_sitemap_definitions` table and the Phase 17 settings columns stay in the database, unread, so a rollback loses nothing. +- The definitions screen and the old settings tile are one settings singleton. The url items of the definition move to the additional addresses (`models.ConvertLegacyItems`): a relative path such as `o-nas` gets its leading slash, as v0.1 resolved it against the site root, spaces and the characters `"`, `<`, `>` and `\` are percent-encoded, and a duplicate is copied once. An item that still cannot be an additional address (an absolute URL with `&`, another scheme, a malformed item) is not copied and logged at startup as `golem15.sitemap: definition url item not copied` with the item, its value and the reason; the other items are copied. `frontend_url` moves to the site address and `frontend_default_locale` to the default locale (migration `202610280002`). Reference items are dropped: the content types list their records. The `golem15_sitemap_definitions` table and the Phase 17 settings columns stay in the database, unread, so a rollback loses nothing. - The config keys `golem15.sitemap.cache_seconds` and `golem15.sitemap.priority_tiers` and the festival events `BeforeAddItem` and `AddItem` are gone. ## Features @@ -270,6 +270,7 @@ The files table lists the image and News files too. A save refuses an invalid si | `models.TypeSetting` | A row of `golem15_sitemap_types`. | | `models.AllowedFrontendURL`, `models.AllowedAddress`, `models.ParseDate`, `models.ValidDate` | The value checks. | | `models.LegacyItem`, `models.ParseLegacyItems`, `models.ParseLegacyItem`, `models.LegacyEntries`, `models.LegacyJSON`, `models.DecodeText`, `models.Text` | Reading Phase 17 and WinterCMS definition data. | +| `models.ConvertLegacyItems`, `models.DroppedItem` | The url items of a Phase 17 definition as additional addresses, and the items that could not be copied with their reasons. | | `controllers.SettingsControllerID`, `controllers.PermissionAccessDefinitions` | `golem15.sitemap.settings` and `golem15.sitemap.access_definitions`. | | `console.Rebuild`, `console.Import`, `console.ImportFile`, `console.ImportWinter`, `console.ImportWinterFiles`, `console.WinterOptions`, `console.WinterReport` | The commands. | | `api.KeyFile`, `api.KeyFilePattern` | The IndexNow key file handler (package `controllers/api`). | diff --git a/models/legacy.go b/models/legacy.go index 19e1336..a1229be 100644 --- a/models/legacy.go +++ b/models/legacy.go @@ -4,8 +4,11 @@ import ( "bytes" "encoding/json" "fmt" + "regexp" "strconv" "strings" + "unicode" + "unicode/utf8" ) // Text returns *p, "" for nil. @@ -175,3 +178,101 @@ func phpNumberString(n json.Number) string { } return strconv.FormatFloat(f, 'f', -1, 64) } + +// DroppedItem is a url item of a Phase 17 definition that ConvertLegacyItems +// could not copy: its PHP label (index or key; "" for a definition that is +// not a list of items), the value as stored and the reason. +type DroppedItem struct { + Label string + URL string + Reason string +} + +// legacyScheme matches a value that starts with a URL scheme. +var legacyScheme = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9+.\-]*:`) + +// ConvertLegacyItems turns the url items of a Phase 17 definition into +// additional addresses (D-32, P33-66; pure). The entries are parsed one by +// one: a malformed item is dropped with its parse error and the others are +// kept; items of another type (records the content types list) are neither +// kept nor reported. A value without a scheme and without a leading / gets +// one, as the Phase 17 sitemap resolved it against the site root; spaces +// and the bytes " < > \ are percent-encoded. The result must pass +// AllowedAddress; one that does not is dropped with the reason (an +// absolute URL with & among them). A duplicate is kept once and not +// reported. +func ConvertLegacyItems(data []byte) ([]ExtraURL, []DroppedItem) { + rows := []ExtraURL{} + var dropped []DroppedItem + data = LegacyJSON(data) + if len(data) == 0 || bytes.Equal(data, []byte("null")) { + return rows, nil + } + raws, labels, ok := LegacyEntries(data) + if !ok { + return rows, []DroppedItem{{Reason: "the definition data is not a list of items"}} + } + seen := map[string]bool{} + for i, raw := range raws { + item, err := ParseLegacyItem(raw) + if err != nil { + dropped = append(dropped, DroppedItem{Label: labels[i], Reason: err.Error()}) + continue + } + if item.Type != LegacyItemURL { + continue + } + u, reason := convertLegacyURL(item.URL) + if reason != "" { + dropped = append(dropped, DroppedItem{Label: labels[i], URL: item.URL, Reason: reason}) + continue + } + if seen[u] { + continue + } + seen[u] = true + rows = append(rows, ExtraURL{URL: u}) + } + return rows, dropped +} + +// convertLegacyURL is one url item's value as an additional address, or +// the reason it cannot be one. +func convertLegacyURL(raw string) (string, string) { + u := strings.TrimSpace(raw) + if u == "" { + return "", "the item has no address" + } + if !utf8.ValidString(u) { + return "", "the address is not valid UTF-8" + } + if !legacyScheme.MatchString(u) && !strings.HasPrefix(u, "/") { + u = "/" + u + } + var b strings.Builder + for _, r := range u { + switch { + case r == '"' || r == '<' || r == '>' || r == '\\' || unicode.IsSpace(r): + for _, c := range []byte(string(r)) { + fmt.Fprintf(&b, "%%%02X", c) + } + default: + b.WriteRune(r) + } + } + u = b.String() + if AllowedAddress(u) { + return u, "" + } + switch { + case strings.HasPrefix(u, "//"): + return "", "an address starting with // has no scheme" + case strings.HasPrefix(u, "/"): + return "", "the path holds a control character" + case strings.Contains(u, "&"): + return "", "an absolute address may not hold &" + case strings.ContainsFunc(u, unicode.IsControl): + return "", "the address holds a control character" + } + return "", "only a path or an absolute http(s) address can be an additional address" +} diff --git a/models/legacy_test.go b/models/legacy_test.go new file mode 100644 index 0000000..7b2ae8e --- /dev/null +++ b/models/legacy_test.go @@ -0,0 +1,60 @@ +package models_test + +import ( + "strings" + "testing" + + "git.golem15.com/golem15/sm-sitemap-plugin/models" +) + +// TestConvertLegacyItems pins L2 WR-04 (D-32, P33-66): every url item the +// Phase 17 sitemap served is copied, a relative path with a leading slash +// and spaces and unsafe bytes percent-encoded; a duplicate is kept once +// and not reported; an item that still is no allowed address, and a +// malformed one, is dropped with its reason while the others are kept; +// items of another type are neither kept nor reported. +func TestConvertLegacyItems(t *testing.T) { + data := `[{"type":"url","url":"o-nas"},` + + `{"type":"url","url":"/blog/zażółć gęślą"},` + + `{"type":"url","url":"/o-nas"},` + + `{"type":"url","url":"https://site.test/x?a=1&b=2"},` + + `{"type":"url","url":["not", "text"]},` + + `{"type":"acme-post","reference":"7"},` + + `{"url":"/a\"bd\\e"},` + + `{"type":"url","url":"https://site.test/kontakt"},` + + `{"type":"url","url":"javascript:alert(1)"}]` + rows, dropped := models.ConvertLegacyItems([]byte(data)) + var got []string + for _, r := range rows { + got = append(got, r.URL) + if r.LastMod != "" { + t.Errorf("a copied address has a lastmod: %+v", r) + } + } + want := "/o-nas /blog/zażółć%20gęślą /a%22b%3Cc%3Ed%5Ce https://site.test/kontakt" + if strings.Join(got, " ") != want { + t.Fatalf("rows = %q, want %q", strings.Join(got, " "), want) + } + if len(dropped) != 3 { + t.Fatalf("dropped = %+v, want the & URL, the malformed item and javascript:", dropped) + } + if d := dropped[0]; d.Label != "3" || d.URL != "https://site.test/x?a=1&b=2" || !strings.Contains(d.Reason, "&") { + t.Errorf("dropped & URL = %+v", d) + } + if d := dropped[1]; d.Label != "4" || d.URL != "" || !strings.Contains(d.Reason, "url") { + t.Errorf("dropped malformed item = %+v", d) + } + if d := dropped[2]; d.Label != "8" || d.URL != "javascript:alert(1)" || d.Reason == "" { + t.Errorf("dropped javascript: = %+v", d) + } + + if rows, dropped := models.ConvertLegacyItems([]byte(`{"a":{"url":"/x"},"b":{"url":"/x"}}`)); len(rows) != 1 || rows[0].URL != "/x" || len(dropped) != 0 { + t.Errorf("keyed object = %+v %+v", rows, dropped) + } + if rows, dropped := models.ConvertLegacyItems([]byte(`"not a list"`)); len(rows) != 0 || len(dropped) != 1 || dropped[0].Label != "" { + t.Errorf("not a list = %+v %+v", rows, dropped) + } + if rows, dropped := models.ConvertLegacyItems(nil); len(rows) != 0 || dropped != nil { + t.Errorf("empty = %+v %+v", rows, dropped) + } +} diff --git a/sitemap_index_test.go b/sitemap_index_test.go index e8aacde..9993325 100644 --- a/sitemap_index_test.go +++ b/sitemap_index_test.go @@ -349,7 +349,11 @@ func TestMigrateDefinitions(t *testing.T) { `{"type":"url","url":"/o-nas"},` + `{"type":"url","url":"javascript:alert(1)"},` + `{"type":"url","url":"/"},` + - `{"type":"url","url":"https://www.example.test/kontakt"}]` + `{"type":"url","url":"https://www.example.test/kontakt"},` + + `{"type":"url","url":"kontakt-pl"},` + + `{"type":"url","url":"/blog/zażółć gęślą"},` + + `{"type":"url","url":"https://www.example.test/x?a=1&b=2"},` + + `{"type":"url","url":{"bad":true}}]` for _, stmt := range []struct { sql string args []any @@ -369,7 +373,10 @@ func TestMigrateDefinitions(t *testing.T) { if err != nil { t.Fatal(err) } - if got := extraURLs(s.Extra()); got != "/ /o-nas https://www.example.test/kontakt" { + // L2 WR-04: one malformed item no longer drops the definition, a + // relative path gets its slash and a space is percent-encoded; the & + // URL and javascript: are dropped (and logged). + if got := extraURLs(s.Extra()); got != "/ /o-nas https://www.example.test/kontakt /kontakt-pl /blog/zażółć%20gęślą" { t.Fatalf("extra_urls = %q", got) } for _, e := range s.Extra() { diff --git a/updates/202610280002_update_golem15_sitemap_settings.go b/updates/202610280002_update_golem15_sitemap_settings.go index 259e578..73c9528 100644 --- a/updates/202610280002_update_golem15_sitemap_settings.go +++ b/updates/202610280002_update_golem15_sitemap_settings.go @@ -5,6 +5,7 @@ import ( "strings" "git.golem15.com/golem15/sm-sitemap-plugin/models" + "git.golem15.com/golem15/summercms/modules/diary" "github.com/go-gormigrate/gormigrate/v2" "gorm.io/gorm" ) @@ -57,15 +58,22 @@ func init() { Register(updateSettings...) } +// migrationLog is the migrations' logger (component sitemap). +var migrationLog = diary.For("sitemap") + // copyDefinitions moves the Phase 17 data into the new columns (D-32): the // url items of the definition /sitemap.xml served (theme default, the -// first one) become the additional addresses as given, without a lastmod -// (an address that models.AllowedAddress refuses, or one already copied, -// is skipped); frontend_url becomes site_url when models.AllowedFrontendURL -// accepts it, and frontend_default_locale default_locale. Reference items -// are dropped: the content types list their records. The definitions table -// and the Phase 17 columns are left as they are, so a rollback (which drops -// the new columns) keeps every value. +// first one) become the additional addresses, without a lastmod, through +// models.ConvertLegacyItems (P33-66): a relative path gets its leading +// slash, spaces and the bytes " < > \ are percent-encoded, a duplicate is +// kept once, and each url item that still cannot be an address (or is +// malformed) is logged at warn level with its label, value and reason +// while the other items are copied. frontend_url becomes site_url when +// models.AllowedFrontendURL accepts it, and frontend_default_locale +// default_locale. Reference items are dropped: the content types list +// their records. The definitions table and the Phase 17 columns are left +// as they are, so a rollback (which drops the new columns) keeps every +// value. func copyDefinitions(tx *gorm.DB) error { var data []string if err := tx.Raw(`SELECT COALESCE(data, '') FROM golem15_sitemap_definitions WHERE theme = 'default' ORDER BY id ASC LIMIT 1`).Scan(&data).Error; err != nil { @@ -73,20 +81,14 @@ func copyDefinitions(tx *gorm.DB) error { } rows := []models.ExtraURL{} if len(data) > 0 { - items, _, err := models.ParseLegacyItems([]byte(data[0])) - if err != nil { - // A definition the Phase 17 code could not read either: nothing - // is copied, and the row stays for a manual look. - items = nil - } - seen := map[string]bool{} - for _, item := range items { - u := strings.TrimSpace(item.URL) - if item.Type != models.LegacyItemURL || seen[u] || !models.AllowedAddress(u) { + var dropped []models.DroppedItem + rows, dropped = models.ConvertLegacyItems([]byte(data[0])) + for _, d := range dropped { + if d.Label == "" && d.URL == "" { + migrationLog.Warn("golem15.sitemap: definition not copied", "reason", d.Reason) continue } - seen[u] = true - rows = append(rows, models.ExtraURL{URL: u}) + migrationLog.Warn("golem15.sitemap: definition url item not copied", "item", d.Label, "url", d.URL, "reason", d.Reason) } } raw, err := json.Marshal(rows)