fix!: URLFor escapes each value for the part of the URL it fills

docs/lint.md

@@ -77,6 +77,7 @@ linter can't tell where the tree is mounted, so its pages get no tree-based chec
 | `ref` | `structpages.Ref(...)` strings that don't resolve to a page-tree node — including refs stored in struct fields and vars (e.g. a nav table). |
 | `idfor` | `structpages.ID` / `IDTarget` method expressions whose receiver is not mounted as a page, or whose method is missing on a chain leaf. The same for the argument of `RenderTarget.Is`, which matches the target an `ID(ctx, X)` element produces: a method expression or method value (`p.Content`) whose receiver isn't mounted, or an argument that isn't a function at all, such as `t.Is(p.Content())`, which calls the component instead of naming it, or a `Ref`/string, which structpages' own target selectors never match (a custom `RenderTarget` might). |
 | `params` | `URLFor` param keys that don't appear in the route pattern (including placeholders in a `?q={q}` fragment) — literal `map[string]any` keys and the constant keys of the key/value form, `URLFor(ctx, p, "id", x)` / `url("id", x)`. |
+| `escape` | A `URLFor` value already escaped with `net/url` — `url.QueryEscape(x)`, `url.PathEscape(x)` or `v.Encode()` passed positionally, as a key/value pair or in a literal map, `url("q", url.QueryEscape(q))` in gsx. `URLFor` escapes every value for its place in the URL, so it would be escaped twice. Pass the value raw; put an encoded `url.Values` in the template instead, `[]any{page, "?" + v.Encode()}`. Needs no page tree. |
 | `url-attr` | URL-bearing HTML attributes in `.gsx` and `.templ` files (`href`, `action`, `formaction`, `hx-{get,post,put,patch,delete}`, `hx-{push,replace}-url`) whose values are hard-coded internal paths, string concats, `fmt.Sprint*` calls, or gsx `` f`/path/@{x}` `` templates. Allows `https://`, `mailto:`, `#`, and protocol-relative `//…` externals. |
 | `route-literal` | `.go` string literals whose value exactly equals a mounted route — e.g. `return "/admin/queues"` — where you should resolve by page type via `URLFor`. Deliberately narrow: exact concrete-route matches only; comparisons (`==`/`switch`), `Ref(...)` args and the route argument of a `Mount`/`Parse` call (or of a helper forwarding to one), which defines the route, are skipped; `_test.go` and generated files are skipped. |
 | `selector` | A `structpages.Mount` call with no `structpages.WithTargetSelector(...)` option whose tree serves htmx templates. There is no default target selector since v0.9.0, so every htmx partial request to that tree renders the full `Page`. Pass `WithTargetSelector(structpages.HTMXRenderTarget)` for htmx 1/2 or `WithTargetSelector(structpages.HTMXv4RenderTarget)` for htmx 4. See [How `selector` decides](#how-selector-decides). |
@@ -199,7 +200,7 @@ Limits:
 ### gsx templates
 
 gsx lowers the `url`, `id` and `target` filters to real `structpages.URLFor` / `ID` / `IDTarget` calls in
-the generated `.x.go`, so `urlfor`, `ref`, `params` and `idfor` check every `{ page{} |> url("id", x) }`
+the generated `.x.go`, so `urlfor`, `ref`, `params`, `escape` and `idfor` check every `{ page{} |> url("id", x) }`
 exactly like hand-written Go. Findings are reported at the `.gsx` line through gsx's `//line` directives.
 `url-attr` parses the `.gsx` sources directly with gsx's own parser.
 

docs/urlfor.md

@@ -75,6 +75,8 @@ url, err := structpages.URLFor(ctx,
 )
 ```
 
+Each value is escaped for the part of the URL its placeholder sits in, so pass it raw. A path value can't add a segment (`a/b` becomes `a%2Fb`), and a query or fragment value can't add a parameter or end the query: `&`, `=`, `+`, `#` and `%` all stay data, so `q: "A&B"` reads back as `A&B`. Don't `url.QueryEscape` a value first, or it gets escaped twice and the handler reads `A%26B`. The part changes only at the template's own `?` and `#`, never at one in a value. Text you write into the template itself is used as is, so query text you've already encoded goes there: `[]any{MyList{}, "?" + values.Encode()}`.
+
 In markup the same form builds pagination links, one per page:
 
 ```gsx
@@ -115,6 +117,8 @@ href, err := structpages.URLFor(ctx, siblingPage{})
 
 Only the current route's params auto-fill; sibling routes with different param names do not.
 
+Auto-filled values come from `r.PathValue`, already decoded, and are escaped again like passed values: on a request for `/items/a%2Fb`, `{id}` fills as `a%2Fb`.
+
 ## Dynamic references with Ref
 
 When the target page can't be referenced by static type — a cross-package import would cycle, a configuration-driven menu stores its targets as data, or a Go type alias collapses two routes onto one `reflect.Type` — use `Ref` (a string type):

parse.go

@@ -740,33 +740,33 @@ func (p *parseContext) findPageNodeByQualifiedRef(ref string) (*PageNode, error)
 	return current, nil
 }
 
-// getSegmentsCached returns cached segments for a pattern, parsing and caching if not already cached
-// Returns a copy of the cached segments to avoid mutation issues
-func (p *parseContext) getSegmentsCached(pattern string) ([]segment, error) {
-	// Try read lock first for cache hit
-	p.segmentCacheMu.RLock()
-	if cached, ok := p.segmentCache[pattern]; ok {
-		p.segmentCacheMu.RUnlock()
-		// Return a copy to avoid mutations affecting the cache
-		result := make([]segment, len(cached))
-		copy(result, cached)
-		return result, nil
-	}
-	p.segmentCacheMu.RUnlock()
-
-	// Cache miss - parse and store
-	segments, err := parseSegments(pattern)
+// cacheRouteSegments parses a mounted route and caches its segments.
+// Only Mount adds to the cache, once per route, so it stays bounded by
+// the page tree: URLFor templates composed at request time, such as
+// []any{page, "?" + v.Encode()}, can be distinct per request and are
+// never cached.
+func (p *parseContext) cacheRouteSegments(route string) ([]segment, error) {
+	segments, err := parseSegments(route)
 	if err != nil {
 		return nil, err
 	}
-
 	p.segmentCacheMu.Lock()
-	p.segmentCache[pattern] = segments
+	p.segmentCache[route] = segments
 	p.segmentCacheMu.Unlock()
+	return segments, nil
+}
 
-	// Return a copy to avoid mutations affecting the cache
-	result := make([]segment, len(segments))
-	copy(result, segments)
+// segmentsFor returns a pattern's segments: a copy of the cached segments
+// of a mounted route, so callers can fill them in, or else freshly parsed.
+func (p *parseContext) segmentsFor(pattern string) ([]segment, error) {
+	p.segmentCacheMu.RLock()
+	cached, ok := p.segmentCache[pattern]
+	p.segmentCacheMu.RUnlock()
+	if !ok {
+		return parseSegments(pattern)
+	}
+	result := make([]segment, len(cached))
+	copy(result, cached)
 	return result, nil
 }
 

skills/structpages/reference.md

@@ -111,8 +111,11 @@ Detected in this order:
 - **Auto-fill**: unfilled placeholders that are path params of the current request's route are taken from
   the request. Params of other routes are not.
 
-Explicitly passed path values are escaped per segment; `{path...}` wildcards keep their slashes.
-Auto-filled values are taken from `r.PathValue` (already decoded) and inserted as is. `{$}` is removed and
+Pass values raw: each is escaped for the part of the URL its placeholder sits in. Path values are escaped per
+segment, and `{path...}` wildcards keep their slashes. Query and fragment values are escaped as one component,
+so `&`, `=`, `+` and `#` stay data. Don't `url.QueryEscape` first, since that escapes twice. Template text is used as
+is, so put pre-encoded query text there (`"?" + values.Encode()`). Auto-filled values come from `r.PathValue`
+(already decoded) and are escaped the same way. `{$}` is removed and
 `WithURLPrefix` is prepended.
 
 ## ID and IDTarget
@@ -364,14 +367,15 @@ concrete page passed to it, through chains of helpers and across packages.
 | `urlfor` | `URLFor` targets: unmounted or ambiguous type, unknown chain child, typed value after a fragment |
 | `ref` | `Ref` strings (including ones stored in variables or struct fields) that resolve to no node |
 | `params` | `URLFor` param keys that are not placeholders in the pattern (fragment included): literal map keys, and constant keys of the key/value form, `url("itemId", 7)` |
+| `escape` | `URLFor` values pre-escaped with `url.QueryEscape`/`url.PathEscape`/`Values.Encode()`, which URLFor escapes again: pass raw; an encoded query goes in the template (`"?" + v.Encode()`) |
 | `idfor` | `ID`/`IDTarget` method expressions whose receiver is not mounted, or whose method is missing on a chain leaf; `RenderTarget.Is` arguments whose receiver is not mounted, or that aren't a function at all (`t.Is(p.Content())` calls instead of naming; a `Ref`/string never matches) |
 | `selector` | a `Mount` with no `WithTargetSelector` whose pages' `.gsx`/`.templ` templates use htmx (`hx-get`…`hx-select`, `<hx-partial>`). Flagged only when every option is a direct `structpages` call (inline or a literal slice); a variable or `opts...` spread isn't. `_test.go` skipped |
 | `route-literal` | `.go` string literals equal to a concrete mounted route (comparisons, `Ref` args, tests and generated files skipped) |
 | `url-attr` | hard-coded internal URLs in URL-bearing attributes (`href`, `action`, `formaction`, `hx-get`/`post`/`put`/`patch`/`delete`, `hx-push-url`/`hx-replace-url`) of `.gsx` and `.templ` files: literals, `+` concatenation, `fmt.Sprint*`, and gsx `` f`/items/@{id}` `` templates |
 
 gsx templates get two passes. gsx lowers `|> url`, `|> id` and `|> target` to real
 `URLFor`/`ID`/`IDTarget` calls in the generated `.x.go`, which the analyzer checks like hand-written Go
-(`urlfor`, `ref`, `params`, `idfor`). Findings point at the `.gsx` through gsx's `//line` directives.
+(`urlfor`, `ref`, `params`, `escape`, `idfor`). Findings point at the `.gsx` through gsx's `//line` directives.
 `url-attr` parses the `.gsx` itself. Keep the `.x.go` files generated before linting.
 
 The key/value check follows URLFor's own rules. With as many args as placeholders the call is positional,

struct_pages.go

@@ -379,8 +379,9 @@ func (sp *StructPages) registerPageItem(mux Mux, page *PageNode, mw []Middleware
 	// Pre-parse route segments for performance (done once at Mount time)
 	fullRoute := page.FullRoute()
 	if page.routeSegments == nil {
-		if segments, err := sp.pc.getSegmentsCached(fullRoute); err == nil {
-			// Store without copying since we own this PageNode
+		if segments, err := sp.pc.cacheRouteSegments(fullRoute); err == nil {
+			// Shared with the cache: both only read it; segmentsFor and
+			// getRouteSegments hand out copies
 			page.routeSegments = segments
 		}
 	}

tools/lint/analyzer.go

@@ -117,11 +117,21 @@ func (ctx *checkCtx) inSomeTree(check func()) {
 //
 // The region check is the exception: it reports a bug that occurs in
 // any tree that mounts both pages involved, so it looks for one such
-// tree rather than for one tree that passes (see checkRegion).
+// tree rather than for one tree that passes (see checkRegion). The
+// escape check needs no tree at all.
 func visitCall(ctx *checkCtx, call *ast.CallExpr) {
 	ctx.inSomeTree(func() { checkCall(ctx, call) })
-	if fn, args, ok := structpagesCall(ctx.pass.TypesInfo, call); ok && (fn.Name() == "ID" || fn.Name() == "IDTarget") {
+	fn, args, ok := structpagesCall(ctx.pass.TypesInfo, call)
+	if !ok {
+		return
+	}
+	switch fn.Name() {
+	case "ID", "IDTarget":
 		checkRegion(ctx, call, args, fn.Name())
+	case "URLFor":
+		if len(args) > 0 {
+			checkPreEscaped(ctx, call, args[1:])
+		}
 	}
 }
 

tools/lint/buildtags_integration_test.go

@@ -1,6 +1,8 @@
 package lint
 
 import (
+	"fmt"
+	"path/filepath"
 	"strings"
 	"testing"
 
@@ -11,9 +13,17 @@ import (
 // urlforDiags loads testdata/buildtags under the given build flags and returns
 // the [urlfor] diagnostics the analyzer reports.
 func urlforDiags(t *testing.T, buildFlags ...string) []string {
+	t.Helper()
+	return categoryDiags(t, "testdata/buildtags", "urlfor", buildFlags...)
+}
+
+// categoryDiags loads the testdata module in dir under the given build flags
+// and returns the analyzer's diagnostics of one category, each prefixed with
+// its file:line.
+func categoryDiags(t *testing.T, dir, category string, buildFlags ...string) []string {
 	t.Helper()
 	cfg := &packages.Config{
-		Dir: "testdata/buildtags",
+		Dir: dir,
 		Mode: packages.NeedName | packages.NeedTypes | packages.NeedSyntax |
 			packages.NeedTypesInfo | packages.NeedDeps | packages.NeedImports |
 			packages.NeedFiles | packages.NeedCompiledGoFiles | packages.NeedModule,
@@ -41,8 +51,9 @@ func urlforDiags(t *testing.T, buildFlags ...string) []string {
 			Pkg:       pkg.Types,
 			TypesInfo: pkg.TypesInfo,
 			Report: func(d analysis.Diagnostic) {
-				if strings.HasPrefix(d.Message, "[urlfor]") {
-					diags = append(diags, d.Message)
+				if strings.HasPrefix(d.Message, "["+category+"]") {
+					p := pkg.Fset.Position(d.Pos)
+					diags = append(diags, fmt.Sprintf("%s:%d: %s", filepath.Base(p.Filename), p.Line, d.Message))
 				}
 			},
 			ResultOf: map[*analysis.Analyzer]any{},

tools/lint/escape.go

@@ -0,0 +1,69 @@
+package lint
+
+import (
+	"go/ast"
+	"go/types"
+)
+
+// checkPreEscaped reports URLFor values already escaped with net/url.
+// URLFor escapes every value for the part of the URL its placeholder
+// sits in, so a value escaped first is escaped twice: the handler reads
+// url.QueryEscape("A&B") back as "A%26B", not "A&B". args starts after
+// the page argument. Values are checked positionally, as key/value
+// pairs, and as the values of a literal map; a call spreading a slice
+// (args...) hides its values and is skipped.
+//
+// The check needs no page tree: whichever tree serves the call, the
+// value is escaped again.
+func checkPreEscaped(ctx *checkCtx, call *ast.CallExpr, args []ast.Expr) {
+	if call.Ellipsis.IsValid() {
+		return
+	}
+	for _, arg := range args {
+		if lit, ok := unparen(arg).(*ast.CompositeLit); ok && isStringAnyMap(ctx.pass.TypesInfo, lit) {
+			for _, e := range lit.Elts {
+				if kv, ok := e.(*ast.KeyValueExpr); ok {
+					reportPreEscaped(ctx, kv.Value)
+				}
+			}
+			continue
+		}
+		reportPreEscaped(ctx, arg)
+	}
+}
+
+func reportPreEscaped(ctx *checkCtx, expr ast.Expr) {
+	call, ok := unparen(expr).(*ast.CallExpr)
+	if !ok {
+		return
+	}
+	fn := calleeFunc(ctx.pass.TypesInfo, call)
+	if fn == nil || fn.Pkg() == nil || fn.Pkg().Path() != "net/url" {
+		return
+	}
+	sig, _ := fn.Type().(*types.Signature)
+	switch {
+	case sig != nil && sig.Recv() == nil && (fn.Name() == "QueryEscape" || fn.Name() == "PathEscape"):
+		ctx.report(expr.Pos(), "escape",
+			"URLFor: url.%s value is escaped again by URLFor, "+
+				"which escapes each value for its place in the URL; pass it unescaped",
+			fn.Name())
+	case sig != nil && sig.Recv() != nil && fn.Name() == "Encode" && isURLValues(sig.Recv().Type()):
+		ctx.report(expr.Pos(), "escape",
+			"URLFor: url.Values.Encode() as a value is escaped again by URLFor, so its & and = become data; "+
+				"put the encoded query in the template instead: []any{page, \"?\" + v.Encode()}")
+	}
+}
+
+// isURLValues reports whether t is net/url.Values or a pointer to it.
+func isURLValues(t types.Type) bool {
+	if p, ok := t.(*types.Pointer); ok {
+		t = p.Elem()
+	}
+	named, ok := t.(*types.Named)
+	if !ok {
+		return false
+	}
+	obj := named.Obj()
+	return obj.Pkg() != nil && obj.Pkg().Path() == "net/url" && obj.Name() == "Values"
+}

tools/lint/escape_integration_test.go

@@ -0,0 +1,30 @@
+package lint
+
+import (
+	"strings"
+	"testing"
+
+	"github.com/google/go-cmp/cmp"
+)
+
+// TestEscape_Integration covers values escaped with net/url before URLFor,
+// which escapes them again: in every argument form, through a literal map,
+// and not for raw values, a pre-encoded template, a spread slice or a
+// suppressed line.
+func TestEscape_Integration(t *testing.T) {
+	got := categoryDiags(t, "testdata/escape", "escape")
+	for i, d := range got {
+		got[i], _, _ = strings.Cut(d, " is escaped again")
+	}
+	want := []string{
+		"main.go:25: [escape] URLFor: url.QueryEscape value",
+		"main.go:26: [escape] URLFor: url.QueryEscape value",
+		"main.go:27: [escape] URLFor: url.PathEscape value",
+		"main.go:28: [escape] URLFor: url.Values.Encode() as a value",
+		"main.go:29: [escape] URLFor: url.QueryEscape value",
+		"main.go:32: [escape] URLFor: url.QueryEscape value",
+	}
+	if diff := cmp.Diff(want, got); diff != "" {
+		t.Errorf("diagnostics (-want +got):\n%s", diff)
+	}
+}

tools/lint/testdata/escape/go.mod

@@ -0,0 +1,9 @@
+module escape
+
+go 1.24.0
+
+require github.com/jackielii/structpages v0.0.0
+
+require github.com/jackielii/ctxkey v1.0.1 // indirect
+
+replace github.com/jackielii/structpages => ../../../..

tools/lint/testdata/escape/go.sum

@@ -0,0 +1,4 @@
+github.com/google/go-cmp v0.7.0 h1:wk8382ETsv4JYUZwIsn6YpYiWiBsYLSJiTsyBybVuN8=
+github.com/google/go-cmp v0.7.0/go.mod h1:pXiqmnSA92OHEEa9HXL2W4E7lf9JzCmGVUdgjX3N/iU=
+github.com/jackielii/ctxkey v1.0.1 h1:CcgbR+fQbrzZJWxI/7Ec4EhzUbmTU1sfI1gV7MAgjIg=
+github.com/jackielii/ctxkey v1.0.1/go.mod h1:fo4HOwrvSnc3n8o5qZ5L+FVcSyQn+d67CCnlEbH24uc=

tools/lint/testdata/escape/main.go

@@ -0,0 +1,38 @@
+// Command escape passes URLFor values already escaped with net/url, which
+// URLFor escapes again. The [escape] check flags each such value.
+package main
+
+import (
+	"context"
+	"net/http"
+	"net/url"
+
+	"github.com/jackielii/structpages"
+)
+
+type searchPage struct{}
+
+type pages struct {
+	Search searchPage `route:"/search/{id} Search"`
+}
+
+func main() {
+	mux := http.NewServeMux()
+	structpages.Mount(mux, pages{}, "/", "App")
+}
+
+func links(ctx context.Context, q string, v url.Values, args []any) {
+	structpages.URLFor(ctx, []any{searchPage{}, "?q={q}"}, "id", "1", "q", url.QueryEscape(q))                 // flagged
+	structpages.URLFor(ctx, []any{searchPage{}, "?q={q}"}, map[string]any{"id": "1", "q": url.QueryEscape(q)}) // flagged
+	structpages.URLFor(ctx, searchPage{}, url.PathEscape(q))                                                   // flagged
+	structpages.URLFor(ctx, []any{searchPage{}, "?{q}"}, "1", v.Encode())                                      // flagged
+	structpages.URLFor(ctx, []any{searchPage{}, "?q={q}"}, "id", "1", "q", (url.QueryEscape(q)))               // flagged
+
+	var sp *structpages.StructPages
+	sp.URLFor([]any{searchPage{}, "?q={q}"}, "id", "1", "q", url.QueryEscape(q)) // flagged
+
+	structpages.URLFor(ctx, []any{searchPage{}, "?q={q}"}, "id", "1", "q", q)
+	structpages.URLFor(ctx, []any{searchPage{}, "?" + v.Encode()}, "1")
+	structpages.URLFor(ctx, searchPage{}, args...)
+	structpages.URLFor(ctx, searchPage{}, url.PathEscape(q)) //structpages:lint:ignore escape
+}