diff --git a/README.md b/README.md index 5e1688b..23ee482 100644 --- a/README.md +++ b/README.md @@ -68,7 +68,7 @@ Each URL is written in the PHP order (`classes.Generator`): 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. -When the settings hold a frontend URL, the backend URL (`app.url` when set, else the request's scheme and `Host`) is replaced literally inside every ``, every `xhtml:link href` and the stylesheet href, as the PHP `str_replace` did. The scheme is `https` for a TLS connection, or when the immediate peer is in `http.trusted_proxies` and its `X-Forwarded-Proto` says `https`. +When the settings hold a frontend URL, the backend URL (`app.url` when set, else the request's scheme and `Host`) is replaced literally inside every ``, every `xhtml:link href` and the stylesheet href, as the PHP `str_replace` did. The rewrite runs only for a frontend URL `models.AllowedFrontendURL` accepts, and a `` or `href` that would not stay an `http` or `https` URL keeps its value. The scheme is `https` for a TLS connection, or when the immediate peer is in `http.trusted_proxies` and its `X-Forwarded-Proto` says `https`. `GET /sitemap.xsl` serves the PHP plugin's stylesheet byte for byte, `Content-Type: text/xsl; charset=UTF-8`. @@ -103,7 +103,7 @@ A save also refuses, per row, a missing `reference` for a registry type that tak | Setting | Default | Meaning | |---------|---------|---------| -| `frontend_url` | empty | The public frontend URL that replaces the backend URL in the XML. | +| `frontend_url` | empty | The public frontend URL that replaces the backend URL in the XML: empty, or an absolute `http` or `https` URL with a host and without whitespace, control characters, a double quote, a less-than sign, a greater-than sign or an ampersand (`models.AllowedFrontendURL`). A stored value outside this form is not applied and the sitemap keeps the backend URL. | | `frontend_default_locale` | empty | The locale whose URLs carry no prefix, passed to registry types; empty leaves it to the type. | | `updates_manifest_path` | empty | A JSON file mapping a URL path to an ISO 8601 date, used as `lastmod` for items without their own date. | | `enable_priority_tiers` | on | Tier priorities replace item priorities. | @@ -192,6 +192,7 @@ The WinterCMS forms are adapted to the admin's YAML: | `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.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`. | | `classes.Generator` | Writes a definition's sitemap; `Generate`, `Stylesheet`. | @@ -200,7 +201,7 @@ The WinterCMS forms are adapted to the admin's YAML: | `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.Rewrite`, `classes.BackendURL` | The `frontend_url` rewrite, applied only for a value `models.AllowedFrontendURL` accepts, 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. | | `classes.BeforeAddItem`, `classes.AddItem`, `classes.ItemInfo` | The hooks. | diff --git a/classes/rewrite.go b/classes/rewrite.go index f2f7e72..4125911 100644 --- a/classes/rewrite.go +++ b/classes/rewrite.go @@ -3,6 +3,8 @@ package classes import ( "regexp" "strings" + + "git.golem15.com/golem15/sm-sitemap-plugin/models" ) // The PHP routes.php patterns of the frontend rewrite. @@ -16,30 +18,48 @@ var ( // element, inside every xhtml:link href and in the exact stylesheet href. // Both URLs are right-trimmed of slashes first. An empty frontendURL (or // "0", which PHP reads as false) leaves xml unchanged. +// +// PHP inserted the stored text as it was. Three rules keep that text out of +// the document unless it is a plain http(s) URL (T-17-15): +// +// - a frontendURL that models.AllowedFrontendURL refuses leaves xml +// unchanged, whatever is stored in the settings row; +// - the frontend URL is inserted through EscapeText in loc and through +// EscapeAttr in the hrefs. Both are the identity for every accepted +// value, which keeps the PHP bytes; +// - a loc or xhtml:link href whose rewritten value does not start with +// http:// or https:// keeps its value. The generator applies the same +// filter before the rewrite. func Rewrite(xml, backendURL, frontendURL string) string { if frontendURL == "" || frontendURL == "0" { return xml } + if !models.AllowedFrontendURL(frontendURL) { + return xml + } frontend := strings.TrimRight(frontendURL, "/") backend := strings.TrimRight(backendURL, "/") - replace := func(s string) string { - if backend == "" { - return s - } - return strings.ReplaceAll(s, backend, frontend) + if backend == "" { + return xml } + frontendText, frontendAttr := EscapeText(frontend), EscapeAttr(frontend) xml = locElement.ReplaceAllStringFunc(xml, func(m string) string { inner := m[len("") : len(m)-len("")] - return "" + replace(inner) + "" + rewritten := strings.ReplaceAll(inner, backend, frontendText) + if !httpURL(rewritten) { + return m + } + return "" + rewritten + "" }) xml = xhtmlLinkRef.ReplaceAllStringFunc(xml, func(m string) string { sub := xhtmlLinkRef.FindStringSubmatch(m) - return sub[1] + replace(sub[2]) + sub[3] + rewritten := strings.ReplaceAll(sub[2], backend, frontendAttr) + if !httpURL(rewritten) { + return m + } + return sub[1] + rewritten + sub[3] }) - if backend != "" { - xml = strings.ReplaceAll(xml, `href="`+backend+`/sitemap.xsl"`, `href="`+frontend+`/sitemap.xsl"`) - } - return xml + return strings.ReplaceAll(xml, `href="`+backend+`/sitemap.xsl"`, `href="`+frontendAttr+`/sitemap.xsl"`) } // BackendURL is PHP rtrim(url('/'), '/') for a request whose root URL is diff --git a/classes/rewrite_test.go b/classes/rewrite_test.go index b896399..124ad05 100644 --- a/classes/rewrite_test.go +++ b/classes/rewrite_test.go @@ -1,22 +1,31 @@ package classes_test import ( + "context" + "strings" "testing" "git.golem15.com/golem15/sm-sitemap-plugin/classes" ) +// rewriteBackend and rewriteDoc are the backend URL and the document of the +// rewrite tests: a stylesheet instruction, a loc, hreflang links and the +// backend URL in places the rewrite leaves alone. +const rewriteBackend = "https://api.ex.pl" + +var rewriteDoc = "\n" + + `` + "\n" + + `https://api.ex.pl/a2026https://api.ex.pl1` + + `` + + `https://api.ex.pl/xhttps://other.pl/https://api.ex.plcp` + "\n" + // TestRewrite pins the PHP routes.php frontend_url rewrite: the backend URL // is replaced inside loc, inside xhtml:link href and in the exact stylesheet // href, nowhere else; slashes are trimmed on both; an empty or "0" frontend // URL and equal URLs leave the document unchanged. func TestRewrite(t *testing.T) { - const backend = "https://api.ex.pl" - doc := "\n" + - `` + "\n" + - `https://api.ex.pl/a2026https://api.ex.pl1` + - `` + - `https://api.ex.pl/xhttps://other.pl/https://api.ex.plcp` + "\n" + const backend = rewriteBackend + doc := rewriteDoc want := "\n" + `` + "\n" + `https://www.ex.pl/a2026https://api.ex.pl1` + @@ -50,3 +59,73 @@ func TestRewrite(t *testing.T) { t.Fatalf("other stylesheet = %q", got) } } + +// The two frontend_url values of the T-17-15 reproduction: a script URL the +// stylesheet would render as a link, and a value that closes the attribute +// and the element it is written into. +var frontendAttacks = []string{ + "javascript:alert(document.domain)//", + `https://x"/>", + " https://www.ex.pl", + "https://www.ex.pl\n", + "//www.ex.pl", + "www.ex.pl", + "https:///x", + "data:text/html,x", + }, frontendAttacks...) + for _, frontend := range refused { + if got := classes.Rewrite(rewriteDoc, rewriteBackend, frontend); got != rewriteDoc { + t.Errorf("frontend %q changed the document:\n%s", frontend, got) + } + } + + // The reproduction through the generator. + const root = "https://api.example.test" + xml, err := (&classes.Generator{Root: root}).Generate(context.Background(), urlItems("/blog/x")) + if err != nil { + t.Fatal(err) + } + if !strings.Contains(xml, `href="https://api.example.test/sitemap.xsl"`) { + t.Fatalf("the generated document lacks the stylesheet instruction:\n%s", xml) + } + for _, frontend := range frontendAttacks { + got := classes.Rewrite(xml, classes.BackendURL(root), frontend) + if got != xml { + t.Errorf("frontend %q changed the generated sitemap:\n%s", frontend, got) + } + if !strings.Contains(got, "https://api.example.test/blog/x") { + t.Errorf("frontend %q: the loc is not on the backend URL:\n%s", frontend, got) + } + if lower := strings.ToLower(got); strings.Contains(lower, "javascript") || strings.Contains(lower, "https://www.example.test/blog/x") || !strings.Contains(got, `href="https://www.example.test/sitemap.xsl"`) || strings.Contains(got, "api.example.test") { + t.Errorf("a valid frontend URL did not rewrite the sitemap:\n%s", got) + } + if got := classes.Rewrite(xml, classes.BackendURL(root), "HTTPS://WWW.EXAMPLE.TEST"); !strings.Contains(got, "HTTPS://WWW.EXAMPLE.TEST/blog/x") { + t.Errorf("an upper-case frontend URL did not rewrite the loc:\n%s", got) + } + + // The http(s) output filter after the rewrite: replacing "s://api.ex.pl" + // would write httphttps://www.ex.pl/a, so loc and href keep their values. + entry := `https://api.ex.pl/a` + if got := classes.Rewrite(entry, "s://api.ex.pl", "https://www.ex.pl"); got != entry { + t.Errorf("a rewrite to a non-http(s) URL was written:\n%s", got) + } +} diff --git a/models/settings.go b/models/settings.go index d834a92..5134540 100644 --- a/models/settings.go +++ b/models/settings.go @@ -1,5 +1,12 @@ package models +import ( + "net/url" + "strings" + "unicode" + "unicode/utf8" +) + // The settings keys of the five priority tiers, in the order PHP checked // them. A tier rule names one of them. const ( @@ -54,6 +61,36 @@ func (Settings) Fillable() []string { // Rules is empty: the PHP settings form validated nothing. func (Settings) Rules() map[string]string { return map[string]string{} } +// 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 +// stored any text. The value is written into every loc, every hreflang link +// and the stylesheet instruction of the sitemap, and sitemap.xsl renders +// each loc as a link on the public origin, so a value outside this form +// (a javascript: URL, or text that closes the attribute it is written into) +// would reach the readers of /sitemap.xml (T-17-15). +func AllowedFrontendURL(u string) bool { + if u == "" { + return true + } + if !utf8.ValidString(u) { + return false + } + if strings.ContainsFunc(u, func(r rune) bool { + return unicode.IsControl(r) || unicode.IsSpace(r) || r == '"' || r == '<' || r == '>' || r == '&' + }) { + return false + } + parsed, err := url.Parse(u) + if err != nil { + return false + } + if parsed.Scheme != "http" && parsed.Scheme != "https" { + return false + } + return parsed.Opaque == "" && parsed.Hostname() != "" +} + // DefaultSettings is the settings PHP read when nothing was saved: tiers on // with the default priorities. func DefaultSettings() Settings { diff --git a/models/settings_test.go b/models/settings_test.go new file mode 100644 index 0000000..925bcbe --- /dev/null +++ b/models/settings_test.go @@ -0,0 +1,61 @@ +package models_test + +import ( + "testing" + + "git.golem15.com/golem15/sm-sitemap-plugin/models" +) + +// 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 +// quote, <, > or &. +func TestAllowedFrontendURL(t *testing.T) { + for _, u := range []string{ + "", + "https://www.ex.pl", + "https://www.ex.pl/", + "https://www.ex.pl///", + "http://localhost:3000", + "HTTPS://WWW.EX.PL", + "https://www.ex.pl/app", + "https://www.ex.pl:8443/base/", + "https://[::1]:3000", + // The frontend URLs of the golden inputs. + "https://api.example.test", + "https://www.example.test/", + } { + if !models.AllowedFrontendURL(u) { + t.Errorf("AllowedFrontendURL(%q) = false, want true", u) + } + } + for _, u := range []string{ + "0", + "javascript:alert(document.domain)//", + "JavaScript://www.ex.pl/%0aalert(1)", + `https://x"/>", + " https://www.ex.pl", + "https://www.ex.pl\n", + "https://www.ex.pl\t", + "https://www.ex.pl ", + "https://www.ex.pl\x7f", + "https://www.ex.pl\xff", + "//www.ex.pl", + "www.ex.pl", + "/path", + "https:///x", + "https://:8080", + "https:www.ex.pl", + "http://", + "https://", + "https://%zz", + "data:text/html,x", + "ftp://www.ex.pl", + } { + if models.AllowedFrontendURL(u) { + t.Errorf("AllowedFrontendURL(%q) = true, want false", u) + } + } +} diff --git a/sitemap_smoke_test.go b/sitemap_smoke_test.go index 76a9216..e23e5ec 100644 --- a/sitemap_smoke_test.go +++ b/sitemap_smoke_test.go @@ -424,6 +424,29 @@ func TestSitemapImportSmoke(t *testing.T) { if err := console.ImportFile(ctx, e.gdb, bad, "", true, out); err == nil || err.Error() != "Entry 1 is missing a 'url' field" { t.Fatalf("missing url: %v", err) } + + // T-17-15: a frontend URL written straight into the row (one stored + // before the save check existed) is not applied; the sitemap keeps the + // backend URL and none of the value reaches it. + for _, value := range []string{"javascript:alert(document.domain)//", `https://x"/>https://api.example.test/x") { + t.Errorf("frontend_url %q: the sitemap is not on the backend URL:\n%s", value, body) + } + if strings.Contains(body, "www.example.test") { + t.Errorf("frontend_url %q: the earlier frontend URL is left in the sitemap:\n%s", value, body) + } + } } var adminSeq atomic.Uint64