fix: the definitions migration keeps and logs every url item

Migration 202610280002 converts the Phase 17 url items through
models.ConvertLegacyItems: a relative path gets its leading slash,
spaces and the bytes " < > \ are percent-encoded, and a malformed item
no longer drops the whole definition. Each url item that cannot be
copied is logged at warn level with its label, value and reason.

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-<slug>-<n>.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 `<priority>` or `<changefreq>` 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`). |

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"
+}

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\"b<c>d\\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)
+	}
+}

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() {

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)