fix(17): CR-01 refuse non-http sitemap URLs on save, import and output

A url item stored as javascript://%0aalert(1)%0a//x passed Url::to and
reached <loc>, which sitemap.xsl renders as a link on the site origin.

- The definitions form refuses, per row, a url that is not a path or an
  http(s) URL (classes.AllowedItemURL) and a urlPattern that is not a
  path (signpost.ValidItemPattern), keyed data.<i>.url / .urlPattern.
- Generator.addItem drops a loc or alternate that is not http(s).
- sitemap:import-winter drops such url items and clears such patterns
  with a report line; sitemap:import refuses such an entry.
This commit is contained in:
2026-10-12 21:28:28 +02:00
parent 4e55939a1f
commit c42bc265c8
12 changed files with 257 additions and 11 deletions

View File

@@ -56,9 +56,10 @@ Each URL is written in the PHP order (`classes.Generator`):
2. A URL item's address goes through the port of Winter's `Url::to` (`classes.URLTo`): root plus the path trimmed of slashes, `/` keeps its slash, every path segment re-encoded like `rawurlencode`, valid absolute, `mailto:`, `tel:` and fragment URLs kept.
3. `lastmod` is the record's modification time, else the updates manifest's date for the URL path, else omitted. It is written as `2006-01-02T15:04:05+00:00` in UTC.
4. `classes.BeforeAddItem` may drop the URL.
5. The priority is the first matching tier's setting when tiers are on, else the item's priority, written as stored text (`1.0` stays `1.0`).
6. `loc`, `lastmod`, `changefreq` and `priority` are written, then the alternates as `xhtml:link` elements, only when there are more than one.
7. `classes.AddItem` may append XML before `</url>`.
5. A URL that does not come out as an `http` or `https` URL (for example a `javascript:` URL an import or a registry type let through) is dropped, and so is such an alternate. The PHP plugin wrote any URL `Url::to` kept; the stylesheet renders `<loc>` as a link on the site's origin.
6. The priority is the first matching tier's setting when tiers are on, else the item's priority, written as stored text (`1.0` stays `1.0`).
7. `loc`, `lastmod`, `changefreq` and `priority` are written, then the alternates as `xhtml:link` elements, only when there are more than one.
8. `classes.AddItem` may append XML before `</url>`.
A registry item is resolved with its URL pattern, its nesting flag and the settings' frontend default locale as the unprefixed locale. A resolved item with a URL is written, then its sub-items once each with their own URL. A list type (a resolved item without a URL, such as all posts) writes each sub-item once per alternate, with that alternate's URL as `loc` and every alternate as `hreflang`, which is what the PHP list types returned.
@@ -91,6 +92,8 @@ location = /sitemap.xsl {
The admin's type dropdown offers `url` and every registry type in registry order with its label. The reference dropdown depends on the row's type and lists the type's references (`signpost.ReferenceOptions`); `url` and types without references take none, and a reference the type does not offer is refused on save.
A save also refuses, per row, a `url` that is not a path (relative or starting with a single `/`) or an absolute `http`/`https` URL (`classes.AllowedItemURL`: no other scheme, no protocol-relative `//host`, no bare fragment, no surrounding whitespace or control characters), and a `urlPattern` that is not a path starting with a single `/` (`signpost.ValidItemPattern`). The errors are keyed `data.<row>.url` and `data.<row>.urlPattern`.
### Settings
| Setting | Default | Meaning |
@@ -188,6 +191,7 @@ The WinterCMS forms are adapted to the admin's YAML:
| `classes.Serialize`, `classes.URLEntry`, `classes.Alternate` | The libxml-format writer and its entries. |
| `classes.EscapeText`, `classes.EscapeAttr` | libxml's text and attribute escaping. |
| `classes.URLTo`, `classes.BuildURL`, `classes.URLPath` | Ports of Winter `Url::to`, `Url::buildUrl` and `parse_url(..., PHP_URL_PATH)`. |
| `classes.AllowedItemURL` | Whether a url item's address may be stored: a path or an absolute `http`/`https` URL. |
| `classes.Rewrite`, `classes.BackendURL` | The `frontend_url` rewrite and the backend URL it replaces. |
| `classes.TiersConfigKey`, `classes.TierConfig`, `classes.TierRule`, `classes.Tiers`, `classes.CompileTiers`, `classes.LoadTiers` | The tier rules. |
| `classes.Manifest`, `classes.LoadManifest`, `classes.ManifestMaxBytes` | The updates manifest. |
@@ -200,8 +204,8 @@ 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. |
| `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. Every dropped or remapped 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` | `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. |
Both commands open the database themselves when the application has not published one.

View File

@@ -196,7 +196,8 @@ func (g *Generator) manifest() Manifest {
// missing URL filled through Url::to, lastmod from the record or the
// manifest, BeforeAddItem, an empty URL dropped and the URL passed through
// Url::to again, the count, the tier or item priority, the elements and
// AddItem.
// AddItem. Unlike PHP, a URL or alternate that does not come out as an
// http or https URL is dropped.
func (g *Generator) addItem(ctx context.Context, item *models.DefinitionItem, info itemInfo) error {
if g.count >= MaxURLs {
return nil
@@ -229,6 +230,12 @@ func (g *Generator) addItem(ctx context.Context, item *models.DefinitionItem, in
return nil
}
loc := URLTo(g.Root, info.url)
if !httpURL(loc) {
// Only http and https URLs are written: a javascript: or other
// scheme that an import, a registry type or a pattern let through
// would be a clickable link in sitemap.xsl.
return nil
}
g.count++
priority, ok := g.Tiers.Priority(loc, g.Settings)
if !ok {
@@ -242,7 +249,11 @@ func (g *Generator) addItem(ctx context.Context, item *models.DefinitionItem, in
}
if len(info.alternates) > 1 {
for _, a := range info.alternates {
entry.Alternates = append(entry.Alternates, Alternate{Hreflang: a.Hreflang, Href: URLTo(g.Root, a.Href)})
href := URLTo(g.Root, a.Href)
if !httpURL(href) {
continue
}
entry.Alternates = append(entry.Alternates, Alternate{Hreflang: a.Hreflang, Href: href})
}
}
if g.Events != nil {

View File

@@ -149,3 +149,35 @@ func TestURLCap(t *testing.T) {
t.Fatalf("no items = %q %v", empty, err)
}
}
// TestOnlyHTTPLocs pins the CR-01 output filter: a URL that Url::to keeps
// but that is not http or https (a javascript: URL FILTER_VALIDATE_URL
// accepts, mailto:, a bare fragment) is not written and not counted. A
// protocol-relative URL comes out of Url::to on the root host.
func TestOnlyHTTPLocs(t *testing.T) {
gen := &classes.Generator{Root: "https://api.example.test"}
xml, err := gen.Generate(context.Background(), urlItems(
"javascript://%0aalert(1)%0a//x",
"JavaScript://example.test/%0aalert(1)",
"//evil.test/x",
"mailto:a@example.test",
"#top",
"/ok",
"https://other.example.test/page",
))
if err != nil {
t.Fatal(err)
}
if n := strings.Count(xml, "<url>"); n != 3 {
t.Fatalf("%d url elements, want 3:\n%s", n, xml)
}
if !strings.Contains(xml, "<loc>https://api.example.test/evil.test/x</loc>") {
t.Fatalf("the protocol-relative URL did not stay on the root host:\n%s", xml)
}
if strings.Contains(strings.ToLower(xml), "javascript") || strings.Contains(xml, "mailto:") || strings.Contains(xml, "#top") {
t.Fatalf("a non-http URL was written:\n%s", xml)
}
if !strings.Contains(xml, "<loc>https://api.example.test/ok</loc>") || !strings.Contains(xml, "<loc>https://other.example.test/page</loc>") {
t.Fatalf("an http(s) URL is missing:\n%s", xml)
}
}

View File

@@ -531,3 +531,36 @@ func URLPath(raw string) string {
}
return u.path
}
// schemePrefix matches a URL that starts with a scheme and its colon.
var schemePrefix = regexp.MustCompile(`^[A-Za-z][A-Za-z0-9+.\-]*:`)
// AllowedItemURL reports whether a url item's address may be stored: empty,
// a path (relative or starting with a single "/"), or an absolute http or
// https URL. Any other scheme (javascript:, data:, mailto: and the like), a
// protocol-relative "//host" or "/\host", a bare fragment, surrounding
// whitespace and control characters are refused. Url::to keeps any URL
// FILTER_VALIDATE_URL accepts, so without this a javascript: URL reached
// <loc>, which sitemap.xsl renders as a link.
func AllowedItemURL(u string) bool {
if u == "" {
return true
}
if strings.TrimSpace(u) != u || strings.ContainsFunc(u, func(r rune) bool { return r < 0x20 || r == 0x7f }) {
return false
}
if httpURL(u) {
return true
}
if schemePrefix.MatchString(u) || u[0] == '#' || u[0] == '\\' {
return false
}
return !strings.HasPrefix(u, "//") && !strings.HasPrefix(u, `/\`)
}
// httpURL reports whether u starts with http:// or https://
// (case-insensitive).
func httpURL(u string) bool {
lower := strings.ToLower(u)
return strings.HasPrefix(lower, "http://") || strings.HasPrefix(lower, "https://")
}

View File

@@ -55,3 +55,30 @@ func TestURLTo(t *testing.T) {
}
}
}
// TestAllowedItemURL pins the save rule of a url item's address (CR-01).
func TestAllowedItemURL(t *testing.T) {
for u, want := range map[string]bool{
"": true,
"/": true,
"/about": true,
"about/team?x=1": true,
"https://example.test/a": true,
"HTTP://example.test/a": true,
"javascript://%0aalert(1)%0a//x": false,
"javascript:alert(1)": false,
"data:text/html,x": false,
"mailto:a@example.test": false,
"//evil.test/x": false,
`/\evil.test/x`: false,
`\\evil.test`: false,
"#top": false,
" /about": false,
"/about\n": false,
"java\tscript:alert(1)": false,
} {
if got := classes.AllowedItemURL(u); got != want {
t.Errorf("AllowedItemURL(%q) = %v, want %v", u, got, want)
}
}
}

View File

@@ -8,6 +8,7 @@ import (
"fmt"
"os"
"git.golem15.com/golem15/sm-sitemap-plugin/classes"
"git.golem15.com/golem15/sm-sitemap-plugin/models"
"git.golem15.com/golem15/summercms/modules/backpack"
"git.golem15.com/golem15/summercms/modules/bonfire"
@@ -22,7 +23,8 @@ import (
// its url items; with --append the URLs are added after every existing item.
// The definition is always the one /sitemap.xml serves (theme default);
// --theme is accepted for PHP compatibility and a theme other than default
// is reported.
// is reported. An entry whose url is not a path or an http(s) URL
// (classes.AllowedItemURL) is refused, as the admin form refuses it.
func Import(app *backpack.App) bonfire.Command {
return bonfire.Command{
Name: "sitemap:import",
@@ -67,6 +69,9 @@ func ImportFile(ctx context.Context, db *gorm.DB, file, theme string, appendMode
if fields["url"] == nil || err != nil || phpEmpty(fields["url"]) {
return fmt.Errorf("Entry %s is missing a 'url' field", labels[i])
}
if !classes.AllowedItemURL(models.Text(url)) {
return fmt.Errorf("Entry %s has a url that is not a path or an http(s) URL", labels[i])
}
item := models.DefinitionItem{Type: models.ItemTypeURL, URL: url}
item.Changefreq = textOr(fields["changefreq"], "monthly")
item.Priority = textOr(fields["priority"], "0.5")

View File

@@ -12,6 +12,7 @@ import (
"sort"
"strings"
"git.golem15.com/golem15/sm-sitemap-plugin/classes"
"git.golem15.com/golem15/sm-sitemap-plugin/models"
"git.golem15.com/golem15/summercms/modules/backpack"
"git.golem15.com/golem15/summercms/modules/bonfire"
@@ -65,9 +66,12 @@ type WinterReport struct {
// their URL, --map old=new renames applied first),
// an item of a registry type that takes references is dropped when its
// reference is not among the type's current references, an item of an
// unknown type is dropped, and every dropped or remapped item is printed. A
// definition that already has items is refused without --force. Settings
// keys missing from the export take the PHP defaults.
// unknown type is dropped, a url item whose URL is not a path or an http(s)
// URL (classes.AllowedItemURL) is dropped, a URL pattern that is not a path
// (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.
func ImportWinter(app *backpack.App) bonfire.Command {
return bonfire.Command{
Name: "sitemap:import-winter",
@@ -328,7 +332,16 @@ func mapItems(ctx context.Context, reg *signpost.Registry, data json.RawMessage,
if page, ok := fields["cmsPage"].(string); ok && page != "" {
report.Lines = append(report.Lines, fmt.Sprintf("%s: cmsPage '%s' dropped (CMS pages are not supported)", label, page))
}
if p := models.Text(item.URLPattern); p != "" && !signpost.ValidItemPattern(p) {
report.Lines = append(report.Lines, fmt.Sprintf("%s: urlPattern '%s' cleared (not a path starting with a single /)", label, p))
item.URLPattern = nil
}
if url, ok := pageURLs[models.Text(item.Reference)]; ok && droppedTypes[item.Type] {
if !classes.AllowedItemURL(url) {
report.Lines = append(report.Lines, fmt.Sprintf("%s: dropped (--page-url '%s' is not a path or an http(s) URL)", label, url))
report.Dropped++
continue
}
report.Lines = append(report.Lines, fmt.Sprintf("%s: copied as type 'url' with url '%s'", label, url))
report.Remapped++
item.Type = models.ItemTypeURL
@@ -349,6 +362,11 @@ func mapItems(ctx context.Context, reg *signpost.Registry, data json.RawMessage,
}
}
if item.Type == models.ItemTypeURL {
if !classes.AllowedItemURL(models.Text(item.URL)) {
report.Lines = append(report.Lines, fmt.Sprintf("%s: dropped (the url is not a path or an http(s) URL)", label))
report.Dropped++
continue
}
out = append(out, item)
report.Kept++
continue

View File

@@ -4,9 +4,12 @@ package controllers
import (
"context"
"errors"
"fmt"
"git.golem15.com/golem15/sm-sitemap-plugin/classes"
"git.golem15.com/golem15/sm-sitemap-plugin/models"
"git.golem15.com/golem15/summercms/modules/backpack"
"git.golem15.com/golem15/summercms/modules/cabana"
"git.golem15.com/golem15/summercms/modules/pact"
"git.golem15.com/golem15/summercms/modules/signpost"
"gorm.io/gorm"
@@ -23,6 +26,12 @@ const (
// typeURLLabel is the label of the built-in url item type.
const typeURLLabel = "golem15.sitemap::lang.item.url"
// The phrase keys of the item row refusals.
const (
errURLScheme = "golem15.sitemap::lang.errors.url_scheme"
errURLPattern = "golem15.sitemap::lang.errors.url_pattern"
)
// definitionsAdminController edits the one sitemap definition of the
// application (theme default): a singleton form whose items are a repeater
// with a type dropdown (url plus the registry types) and a reference
@@ -96,6 +105,41 @@ func (c definitionsAdminController) DependentOptions(ctx context.Context, _ stri
return signpost.ReferenceOptions(ctx, c.registry(), typ)
}
// FormBeforeCreate checks the item rows (see checkItems).
func (c definitionsAdminController) FormBeforeCreate(_ context.Context, model any) error {
return c.checkItems(model)
}
// FormBeforeUpdate checks the item rows (see checkItems).
func (c definitionsAdminController) FormBeforeUpdate(_ context.Context, model any) error {
return c.checkItems(model)
}
// checkItems refuses, per row, a url outside classes.AllowedItemURL
// (data.<i>.url) and a URL pattern that is not a path
// (signpost.ValidItemPattern, data.<i>.urlPattern). The PHP plugin checked
// neither; a javascript: URL would reach <loc>, which sitemap.xsl renders as
// a link.
func (definitionsAdminController) checkItems(model any) error {
def, ok := model.(*models.Definition)
if !ok || def == nil {
return nil
}
details := map[string]any{}
for i, item := range def.Items() {
if !classes.AllowedItemURL(models.Text(item.URL)) {
details[fmt.Sprintf("data.%d.url", i)] = []string{errURLScheme}
}
if p := models.Text(item.URLPattern); p != "" && !signpost.ValidItemPattern(p) {
details[fmt.Sprintf("data.%d.urlPattern", i)] = []string{errURLPattern}
}
}
if len(details) > 0 {
return &cabana.ValidationError{Details: details}
}
return nil
}
var (
_ pact.AdminController = definitionsAdminController{}
_ pact.AdminPermissioned = definitionsAdminController{}
@@ -103,4 +147,6 @@ var (
_ pact.AdminSingleton = definitionsAdminController{}
_ pact.DropdownOptionsProvider = definitionsAdminController{}
_ pact.DependentOptionsProvider = definitionsAdminController{}
_ pact.FormBeforeCreate = definitionsAdminController{}
_ pact.FormBeforeUpdate = definitionsAdminController{}
)

View File

@@ -0,0 +1,39 @@
package controllers
import (
"errors"
"testing"
"git.golem15.com/golem15/sm-sitemap-plugin/models"
"git.golem15.com/golem15/summercms/modules/cabana"
)
func ptr(s string) *string { return &s }
// TestCheckItems pins the CR-01 save check: per row, a url outside
// classes.AllowedItemURL and a URL pattern that is not a path are refused
// under data.<i>.url and data.<i>.urlPattern.
func TestCheckItems(t *testing.T) {
c := definitionsAdminController{}
def := &models.Definition{}
def.SetItems([]models.DefinitionItem{
{Type: models.ItemTypeURL, URL: ptr("/about")},
{Type: models.ItemTypeURL, URL: ptr("javascript://%0aalert(1)%0a//x")},
{Type: "acme-post", URLPattern: ptr("//evil.test/:slug")},
{Type: "acme-post", URLPattern: ptr("/blog/:slug")},
})
err := c.checkItems(def)
var verr *cabana.ValidationError
if !errors.As(err, &verr) {
t.Fatalf("checkItems = %v, want a validation error", err)
}
if len(verr.Details) != 2 || verr.Details["data.1.url"] == nil || verr.Details["data.2.urlPattern"] == nil {
t.Fatalf("details = %v", verr.Details)
}
ok := &models.Definition{}
ok.SetItems([]models.DefinitionItem{{Type: models.ItemTypeURL, URL: ptr("https://example.test/")}})
if err := c.checkItems(ok); err != nil {
t.Fatalf("a valid definition was refused: %v", err)
}
}

View File

@@ -164,6 +164,31 @@ func TestImportWinter(t *testing.T) {
}
})
t.Run("url and pattern checks", func(t *testing.T) {
bad := writeFile(t, dir, "bad-urls.jsonl", row(7, "t", `[{"type":"url","url":"javascript://%0aalert(1)%0a//x"},`+
`{"type":"url","url":"/ok"},`+
`{"type":"acme-post","reference":"7","urlPattern":"//evil.test/:slug"}]`))
report, out, err := run(console.WinterOptions{DefinitionsFile: bad, Force: true})
if err != nil {
t.Fatalf("import: %v\n%s", err, out)
}
if report.Kept != 2 || report.Dropped != 1 {
t.Fatalf("report = %+v\n%s", report, out)
}
for _, line := range []string{
"item 0 (type 'url', url 'javascript://%0aalert(1)%0a//x'): dropped (the url is not a path or an http(s) URL)",
"item 2 (type 'acme-post', reference '7'): urlPattern '//evil.test/:slug' cleared (not a path starting with a single /)",
} {
if !strings.Contains(out, line) {
t.Errorf("output lacks %q:\n%s", line, out)
}
}
got := stored(t)
if len(got) != 2 || models.Text(got[0].URL) != "/ok" || got[1].URLPattern != nil {
t.Fatalf("stored = %+v", got)
}
})
t.Run("settings upsert", func(t *testing.T) {
first := writeFile(t, dir, "settings.json", `{"frontend_url":"https://www.example.test","enable_priority_tiers":"1","priority_blog":0.7,"unknown":"x"}`)
report, out, err := run(console.WinterOptions{SettingsFile: first})

View File

@@ -37,6 +37,9 @@ item:
save_definition: Saving Definition...
load_indicator: Emptying Definition...
empty_confirm: Empty this definition?
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.
definition:
not_found: No sitemap definition was found. Try creating one first.
title: Sitemap definition

View File

@@ -37,6 +37,9 @@ item:
save_definition: Zapisywanie definicji...
load_indicator: Czyszczenie definicji...
empty_confirm: Wyczyścić tę definicję?
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.
definition:
not_found: Nie znaleziono definicji mapy strony. Musisz ją utworzyć.
title: Definicja mapy strony