From fe9c772e8b92c412b7f9454a264b937ab6d4d6ba Mon Sep 17 00:00:00 2001 From: Jackie Li Date: Wed, 21 Oct 2026 09:42:58 +0100 Subject: [PATCH 1/2] fix!: URLFor escapes each value for the part of the URL it fills URLFor path-escaped every placeholder value, including ones after ? or # in a composed template such as []any{Page{}, "?q={q}"}. url.PathEscape leaves &, = and + alone, so "A&B" broke the query, and a value escaped first with url.QueryEscape came out escaped twice: no input round-tripped. parseSegments now records whether each segment sits in the path, query or fragment, switching only at the template's own first ? and #. Values are held unescaped and escaped once when the URL is joined: path values as before (wildcards keep their slashes), query and fragment values as one query component, with spaces as %20 so existing URLs keep their form. Values auto-filled from the current request come from r.PathValue, already decoded, and were inserted unescaped: an id of "a/b?c" became two path segments and a query. They are now escaped like passed values. The segment cache is now filled only at Mount, once per route. URLFor used to cache every pattern it saw, so a template composed per request ("?" + v.Encode()) grew the cache without bound. BREAKING CHANGE: pass placeholder values raw. A value escaped with url.QueryEscape is now escaped again and reads back escaped, and a placeholder can no longer inject raw query text such as "&sort=asc"; put that text in the template instead. Claude-Session: https://claude.ai/code/session_01NpjJeP32ycvAHNFXRgTviG --- docs/lint.md | 3 +- docs/urlfor.md | 4 + parse.go | 42 ++-- skills/structpages/reference.md | 10 +- struct_pages.go | 5 +- tools/lint/analyzer.go | 14 +- tools/lint/buildtags_integration_test.go | 17 +- url_for.go | 120 ++++++---- url_for_escape_test.go | 272 +++++++++++++++++++++++ url_for_test.go | 6 +- 10 files changed, 415 insertions(+), 78 deletions(-) create mode 100644 url_for_escape_test.go diff --git a/docs/lint.md b/docs/lint.md index 886cb96..9d70f24 100644 --- a/docs/lint.md +++ b/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. diff --git a/docs/urlfor.md b/docs/urlfor.md index 66c7908..b29d56e 100644 --- a/docs/urlfor.md +++ b/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): diff --git a/parse.go b/parse.go index 69f18d6..4da97a8 100644 --- a/parse.go +++ b/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 } diff --git a/skills/structpages/reference.md b/skills/structpages/reference.md index 8cb447a..b88c452 100644 --- a/skills/structpages/reference.md +++ b/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,6 +367,7 @@ 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`, ``). 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) | @@ -371,7 +375,7 @@ concrete page passed to it, through chains of helpers and across packages. 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, diff --git a/struct_pages.go b/struct_pages.go index fda8c25..0edc807 100644 --- a/struct_pages.go +++ b/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 } } diff --git a/tools/lint/analyzer.go b/tools/lint/analyzer.go index 4f4a665..87d81a3 100644 --- a/tools/lint/analyzer.go +++ b/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:]) + } } } diff --git a/tools/lint/buildtags_integration_test.go b/tools/lint/buildtags_integration_test.go index d368ba7..6987c88 100644 --- a/tools/lint/buildtags_integration_test.go +++ b/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{}, diff --git a/url_for.go b/url_for.go index 707b313..ae1c422 100644 --- a/url_for.go +++ b/url_for.go @@ -17,21 +17,16 @@ var ( currentPageCtx = ctxkey.New[*PageNode]("structpages.currentPage", nil) ) -// encodePathSegment converts a value to string and URL-encodes it for use in path segments. -// For wildcard segments ({path...}), slashes are preserved as they're part of the path. -func encodePathSegment(value any, isWildcard bool) string { - s := fmt.Sprint(value) - if isWildcard { - // For wildcards, split on slashes, escape each part, then rejoin - // This preserves slashes while properly encoding each path segment - parts := strings.Split(s, "/") - for i, part := range parts { - parts[i] = url.PathEscape(part) - } - return strings.Join(parts, "/") - } - return url.PathEscape(s) -} +// urlPart is the part of a URL a template segment sits in. Each part escapes +// placeholder values its own way: a path value must not add a segment, and a +// query or fragment value must not add a parameter or end the part. +type urlPart uint8 + +const ( + partPath urlPart = iota + partQuery + partFragment +) func withPcCtx(pc *parseContext) MiddlewareFunc { return func(next http.Handler, node *PageNode) http.Handler { @@ -97,6 +92,10 @@ func extractURLParams(next http.Handler, node *PageNode) http.Handler { // in url_for.go for the full detection order) but require the call site // to track parameter position or name conventions. // +// Pass values unescaped: each is escaped for the part of the URL its +// placeholder sits in (path, query or fragment), so "A&B" in "?q={q}" +// reads back as "A&B". Template strings are used as is. +// // You can pass []any as the page to join multiple path segments // together — strings are concatenated as-is, which is the form used to // append a query-string template to a typed page lookup. You can also @@ -167,12 +166,12 @@ func applyURLPrefix(prefix, path string) string { // //nolint:gocognit,gocyclo // This function handles multiple cases for flexible argument passing func formatPathSegments(ctx context.Context, pattern string, args ...any) (string, error) { - // Use cached segment parsing if parseContext is available + // Mounted routes are parsed once at Mount; see cacheRouteSegments pc := pcCtx.Value(ctx) var segments []segment var err error if pc != nil { - segments, err = pc.getSegmentsCached(pattern) + segments, err = pc.segmentsFor(pattern) } else { segments, err = parseSegments(pattern) } @@ -209,15 +208,7 @@ func formatPathSegments(ctx context.Context, pattern string, args ...any) (strin } } if allFilled { - s := "" - for _, segment := range segments { - if segment.param { - s += segment.value - } else { - s += segment.name - } - } - return s, nil + return joinSegments(segments), nil } } return pattern, fmt.Errorf("pattern %s: no arguments provided", pattern) @@ -238,7 +229,7 @@ func formatPathSegments(ctx context.Context, pattern string, args ...any) (strin for _, idx := range indicies { name := segments[idx].name if value, ok := arg[name]; ok { - segments[idx].value = encodePathSegment(value, segments[idx].wildcard) + segments[idx].value = fmt.Sprint(value) segments[idx].filled = true } // If value not in args map, it should keep the pre-filled value from context @@ -256,7 +247,7 @@ func formatPathSegments(ctx context.Context, pattern string, args ...any) (strin for i, arg := range args { // Always override with provided args when count matches exactly idx := indicies[i] - segments[idx].value = encodePathSegment(arg, segments[idx].wildcard) + segments[idx].value = fmt.Sprint(arg) segments[idx].filled = true } case len(args)%2 == 0 && len(args) >= 2: @@ -290,7 +281,7 @@ func formatPathSegments(ctx context.Context, pattern string, args ...any) (strin for _, idx := range indicies { name := segments[idx].name if value, ok := m[name]; ok { - segments[idx].value = encodePathSegment(value, segments[idx].wildcard) + segments[idx].value = fmt.Sprint(value) segments[idx].filled = true } else if !segments[idx].filled { // Only error if no value from context either @@ -316,7 +307,7 @@ func formatPathSegments(ctx context.Context, pattern string, args ...any) (strin argIdx := 0 for _, idx := range indicies { if !segments[idx].filled && argIdx < len(args) { - segments[idx].value = encodePathSegment(args[argIdx], segments[idx].wildcard) + segments[idx].value = fmt.Sprint(args[argIdx]) segments[idx].filled = true argIdx++ } @@ -324,30 +315,73 @@ func formatPathSegments(ctx context.Context, pattern string, args ...any) (strin } } - s := "" - for _, segment := range segments { - if segment.param { - s += segment.value + return joinSegments(segments), nil +} + +// joinSegments assembles the URL, escaping each placeholder's value for the +// part of the URL it sits in. Values are held unescaped until here, whether +// passed by the caller or taken from the current request (r.PathValue +// results, already decoded). +func joinSegments(segments []segment) string { + var sb strings.Builder + for _, seg := range segments { + if seg.param { + sb.WriteString(seg.escape()) } else { - s += segment.name + sb.WriteString(seg.name) } } - - return s, nil + return sb.String() } type segment struct { name string param bool wildcard bool - value string - filled bool // true if value has been explicitly set (even to empty string) + part urlPart // the part of the URL the segment sits in + value string // unescaped; joinSegments escapes it for part + filled bool // true if value has been explicitly set (even to empty string) +} + +// escape URL-encodes a placeholder value for the part of the URL it fills. +// Path values are escaped per segment; wildcards ({path...}) keep their +// slashes, escaping each segment between them. Query and fragment values are +// escaped as a single query component, so &, =, + and # stay data. +func (seg segment) escape() string { + if seg.part != partPath { + // QueryEscape encodes a literal + as %2B, so every + it emits stands + // for a space; %20 keeps spaces encoded the same way in every part. + return strings.ReplaceAll(url.QueryEscape(seg.value), "+", "%20") + } + if seg.wildcard { + parts := strings.Split(seg.value, "/") + for i, part := range parts { + parts[i] = url.PathEscape(part) + } + return strings.Join(parts, "/") + } + return url.PathEscape(seg.value) } func parseSegments(pattern string) (segments []segment, err error) { if pattern == "" { return } + // The part advances on the template's own ? and #, never on a value's: + // the first ? starts the query, the first # starts the fragment. + part := partPath + static := func(text string) { + segments = append(segments, segment{name: text, part: part}) + for _, c := range text { + switch { + case c == '#': + part = partFragment + return + case c == '?' && part == partPath: + part = partQuery + } + } + } rest := pattern for i := 0; ; i++ { if rest == "" { @@ -355,11 +389,11 @@ func parseSegments(pattern string) (segments []segment, err error) { } start := strings.Index(rest, "{") if start == -1 { - segments = append(segments, segment{name: rest}) + static(rest) break } if start > 0 { - segments = append(segments, segment{name: rest[:start]}) + static(rest[:start]) } rest = rest[start+1:] // move over the '{' end := strings.Index(rest, "}") @@ -369,12 +403,12 @@ func parseSegments(pattern string) (segments []segment, err error) { name := rest[:end] rest = rest[end+1:] if name == "$" { // skip {$} segments - segments = append(segments, segment{name: "{$}"}) + segments = append(segments, segment{name: "{$}", part: part}) continue } wildcard := strings.HasSuffix(name, "...") name = strings.TrimSuffix(name, "...") - segments = append(segments, segment{name: name, param: true, wildcard: wildcard}) + segments = append(segments, segment{name: name, param: true, wildcard: wildcard, part: part}) } return segments, nil } diff --git a/url_for_escape_test.go b/url_for_escape_test.go new file mode 100644 index 0000000..c53af2f --- /dev/null +++ b/url_for_escape_test.go @@ -0,0 +1,272 @@ +package structpages + +import ( + "context" + "net/http" + "net/http/httptest" + "net/url" + "strings" + "testing" + + "github.com/google/go-cmp/cmp" +) + +// decodedURL is a generated URL taken apart and decoded, so tests compare +// what a server reads back rather than one choice of encoding. +type decodedURL struct { + Segments []string // decoded path segments, split on the raw path's slashes + Query url.Values + Fragment string +} + +func decodeURL(t *testing.T, s string) decodedURL { + t.Helper() + u, err := url.Parse(s) + if err != nil { + t.Fatalf("url.Parse(%q): %v", s, err) + } + q, err := url.ParseQuery(u.RawQuery) + if err != nil { + t.Fatalf("url.ParseQuery(%q): %v", u.RawQuery, err) + } + raws := strings.Split(strings.TrimPrefix(u.EscapedPath(), "/"), "/") + segs := make([]string, 0, len(raws)) + for _, raw := range raws { + seg, err := url.PathUnescape(raw) + if err != nil { + t.Fatalf("url.PathUnescape(%q): %v", raw, err) + } + segs = append(segs, seg) + } + return decodedURL{Segments: segs, Query: q, Fragment: u.Fragment} +} + +func TestFormatPathSegments_queryValuesRoundTrip(t *testing.T) { + tests := []struct { + name string + pattern string + args []any + query url.Values + }{ + {"ampersand", "/search?q={q}", []any{"A&B"}, url.Values{"q": {"A&B"}}}, + {"equals", "/search?q={q}", []any{"x=1"}, url.Values{"q": {"x=1"}}}, + {"plus", "/search?q={q}", []any{"c+d"}, url.Values{"q": {"c+d"}}}, + {"percent", "/search?q={q}", []any{"50%"}, url.Values{"q": {"50%"}}}, + {"hash", "/search?q={q}", []any{"a#b"}, url.Values{"q": {"a#b"}}}, + {"space", "/search?q={q}", []any{"Gas Lift"}, url.Values{"q": {"Gas Lift"}}}, + {"non-ascii", "/search?q={q}", []any{"مرحبا é"}, url.Values{"q": {"مرحبا é"}}}, + {"two values", "/search?q={q}&filter={f}", []any{map[string]any{"q": "foo&bar", "f": "a=b"}}, + url.Values{"q": {"foo&bar"}, "filter": {"a=b"}}}, + {"inside a longer value", "/search?q=pre-{x}", []any{"a&b"}, url.Values{"q": {"pre-a&b"}}}, + {"as a key", "/search?{k}=1", []any{"a&b"}, url.Values{"a&b": {"1"}}}, + {"empty", "/search?q={q}", []any{""}, url.Values{"q": {""}}}, + {"not a string", "/search?q={q}", []any{42}, url.Values{"q": {"42"}}}, + {"wildcard in query", "/search?p={p...}", []any{"a/b&c"}, url.Values{"p": {"a/b&c"}}}, + {"after {$}", "/{$}?q={q}", []any{"a b&c"}, url.Values{"q": {"a b&c"}}}, + {"caller pre-escaped", "/alerts?category={c}", []any{url.QueryEscape("Health & Safety")}, + url.Values{"category": {"Health+%26+Safety"}}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := formatPathSegments(context.Background(), tt.pattern, tt.args...) + if err != nil { + t.Fatalf("formatPathSegments: %v", err) + } + d := decodeURL(t, got) + if diff := cmp.Diff(tt.query, d.Query); diff != "" { + t.Errorf("%s: query mismatch (-want +got):\n%s", got, diff) + } + if d.Fragment != "" { + t.Errorf("%s: fragment = %q, want none", got, d.Fragment) + } + }) + } +} + +func TestFormatPathSegments_spaceEncodedAlikeInEveryPart(t *testing.T) { + got, err := formatPathSegments(context.Background(), "/p/{a}?q={b}#{c}", "a b", "a b", "a b") + if err != nil { + t.Fatalf("formatPathSegments: %v", err) + } + if want := "/p/a%20b?q=a%20b#a%20b"; got != want { + t.Errorf("got %q, want %q", got, want) + } +} + +func TestFormatPathSegments_pathAndFragmentRoundTrip(t *testing.T) { + tests := []struct { + name string + pattern string + args []any + want decodedURL + raw string // exact output, where the path encoding is pinned + }{ + {name: "path space", pattern: "/users/{id}", args: []any{"a b"}, + raw: "/users/a%20b"}, + {name: "path slash stays one segment", pattern: "/users/{id}", args: []any{"a/b"}, + raw: "/users/a%2Fb"}, + {name: "path question mark", pattern: "/users/{id}", args: []any{"a?b"}, + want: decodedURL{Segments: []string{"users", "a?b"}, Query: url.Values{}}}, + {name: "path hash", pattern: "/users/{id}", args: []any{"a#b"}, + want: decodedURL{Segments: []string{"users", "a#b"}, Query: url.Values{}}}, + {name: "path plus is literal", pattern: "/users/{id}", args: []any{"c+d"}, + raw: "/users/c+d"}, + {name: "wildcard keeps slashes", pattern: "/files/{path...}", args: []any{"a b/c&d"}, + want: decodedURL{Segments: []string{"files", "a b", "c&d"}, Query: url.Values{}}}, + {name: "path and query", pattern: "/users/{id}?tab={t}", args: []any{"a&b", "x&y"}, + want: decodedURL{Segments: []string{"users", "a&b"}, Query: url.Values{"tab": {"x&y"}}}}, + {name: "fragment", pattern: "/docs/{s}#{a}", args: []any{"api", "a b"}, + want: decodedURL{Segments: []string{"docs", "api"}, Query: url.Values{}, Fragment: "a b"}}, + {name: "path, query and fragment", pattern: "/docs/{s}?q={q}#{a}", args: []any{"api", "a#b", "c&d"}, + want: decodedURL{Segments: []string{"docs", "api"}, Query: url.Values{"q": {"a#b"}}, Fragment: "c&d"}}, + {name: "question mark inside the fragment", pattern: "/docs#x?{a}", args: []any{"c&d"}, + want: decodedURL{Segments: []string{"docs"}, Query: url.Values{}, Fragment: "x?c&d"}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + got, err := formatPathSegments(context.Background(), tt.pattern, tt.args...) + if err != nil { + t.Fatalf("formatPathSegments: %v", err) + } + if tt.raw != "" { + if got != tt.raw { + t.Errorf("got %q, want %q", got, tt.raw) + } + return + } + if diff := cmp.Diff(tt.want, decodeURL(t, got)); diff != "" { + t.Errorf("%s: mismatch (-want +got):\n%s", got, diff) + } + }) + } +} + +func TestFormatPathSegments_queryValueEveryArgForm(t *testing.T) { + for name, args := range map[string][]any{ + "map": {map[string]any{"q": "A&B"}}, + "positional": {"A&B"}, + "pairs": {"q", "A&B"}, + } { + t.Run(name, func(t *testing.T) { + got, err := formatPathSegments(context.Background(), "/search?q={q}", args...) + if err != nil { + t.Fatalf("formatPathSegments: %v", err) + } + if diff := cmp.Diff(url.Values{"q": {"A&B"}}, decodeURL(t, got).Query); diff != "" { + t.Errorf("%s: query mismatch (-want +got):\n%s", got, diff) + } + }) + } +} + +// Values filled from the current request come from r.PathValue, already +// decoded, so URLFor must escape them like values the caller passes. +func TestFormatPathSegments_requestValuesEscaped(t *testing.T) { + tests := []struct { + name string + pattern string + params map[string]string + args []any + want decodedURL + }{ + {name: "path value with reserved characters", pattern: "/items/{id}", + params: map[string]string{"id": "a/b?c#d"}, + want: decodedURL{Segments: []string{"items", "a/b?c#d"}, Query: url.Values{}}}, + {name: "wildcard keeps slashes", pattern: "/files/{path...}", + params: map[string]string{"path": "a b/c?d"}, + want: decodedURL{Segments: []string{"files", "a b", "c?d"}, Query: url.Values{}}}, + {name: "mixed with a passed query value", pattern: "/items/{id}?q={q}", + params: map[string]string{"id": "a&b?"}, args: []any{map[string]any{"q": "x&y"}}, + want: decodedURL{Segments: []string{"items", "a&b?"}, Query: url.Values{"q": {"x&y"}}}}, + } + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + ctx := urlParamsCtx.WithValue(context.Background(), tt.params) + got, err := formatPathSegments(ctx, tt.pattern, tt.args...) + if err != nil { + t.Fatalf("formatPathSegments: %v", err) + } + if diff := cmp.Diff(tt.want, decodeURL(t, got)); diff != "" { + t.Errorf("%s: mismatch (-want +got):\n%s", got, diff) + } + }) + } +} + +type escItemPage struct{} + +func (escItemPage) ServeHTTP(w http.ResponseWriter, r *http.Request) { + u, err := URLFor(r.Context(), []any{escItemPage{}, "?q={q}"}, map[string]any{"q": "A&B"}) + if err != nil { + w.WriteHeader(http.StatusInternalServerError) + _, _ = w.Write([]byte(err.Error())) + return + } + _, _ = w.Write([]byte(u)) +} + +func TestURLFor_requestValuesRoundTrip(t *testing.T) { + type pages struct { + item escItemPage `route:"/items/{id} Item"` + } + for _, tt := range []struct { + name string + opts []Option + path string + wantPath string + want decodedURL + }{ + {name: "escaped request value", path: "/items/a%26b%2Fc%3Fd", + want: decodedURL{Segments: []string{"items", "a&b/c?d"}, Query: url.Values{"q": {"A&B"}}}}, + {name: "under WithURLPrefix", opts: []Option{WithURLPrefix("/admin")}, path: "/items/a%20b", + wantPath: "/admin/items/a%20b", + want: decodedURL{Segments: []string{"admin", "items", "a b"}, Query: url.Values{"q": {"A&B"}}}}, + } { + t.Run(tt.name, func(t *testing.T) { + mux := http.NewServeMux() + if _, err := Mount(mux, &pages{}, "/", "Test", tt.opts...); err != nil { + t.Fatalf("Mount: %v", err) + } + rec := httptest.NewRecorder() + mux.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, tt.path, http.NoBody)) + if rec.Code != http.StatusOK { + t.Fatalf("status %d: %s", rec.Code, rec.Body.String()) + } + got := rec.Body.String() + if tt.wantPath != "" && !strings.HasPrefix(got, tt.wantPath+"?") { + t.Errorf("got %q, want path %q", got, tt.wantPath) + } + if diff := cmp.Diff(tt.want, decodeURL(t, got)); diff != "" { + t.Errorf("%s: mismatch (-want +got):\n%s", got, diff) + } + }) + } +} + +// A template composed per request, such as []any{page, "?" + v.Encode()}, +// is distinct per value set, so URLFor must not cache it: only Mount adds +// to the segment cache, once per route. +func TestURLFor_composedTemplatesNotCached(t *testing.T) { + type pages struct { + item escItemPage `route:"/items/{id} Item"` + } + sp, err := Mount(http.NewServeMux(), &pages{}, "/", "Test") + if err != nil { + t.Fatalf("Mount: %v", err) + } + pc := sp.pc + before := len(pc.segmentCache) + ctx := pcCtx.WithValue(context.Background(), pc) + for i := range 100 { + v := url.Values{"n": {strings.Repeat("x", i)}} + if _, err := URLFor(ctx, []any{escItemPage{}, "?" + v.Encode()}, "1"); err != nil { + t.Fatalf("URLFor: %v", err) + } + } + if got, err := URLFor(ctx, escItemPage{}, "a b"); err != nil || got != "/items/a%20b" { + t.Fatalf("URLFor = %q, %v; want /items/a%%20b", got, err) + } + if after := len(pc.segmentCache); after != before { + t.Errorf("segment cache grew from %d to %d entries", before, after) + } +} diff --git a/url_for_test.go b/url_for_test.go index daab82a..72a0c67 100644 --- a/url_for_test.go +++ b/url_for_test.go @@ -198,8 +198,8 @@ func TestUrlFor(t *testing.T) { }, { name: "with args", - page: []any{product{}, "?page={page}{extra}"}, - args: []any{"page", "1", "extra", "&sort=asc"}, + page: []any{product{}, "?page={page}", "&sort=asc"}, + args: []any{"page", "1"}, expected: "/product?page=1&sort=asc", }, { @@ -1237,7 +1237,7 @@ func TestURLFor_withQueryStringComposition(t *testing.T) { name: "query string with encoded special chars", parts: []any{"/search", "?q={query}&filter={filter}"}, args: []any{"foo&bar", "a=b"}, - expected: "/search?q=foo&bar&filter=a=b", // & and = allowed in query values + expected: "/search?q=foo%26bar&filter=a%3Db", // & and = in a value stay data }, { name: "mixed path params and query params", From 1ded9dceccb6730f8c4277a9a7d3ff50bf8cb048 Mon Sep 17 00:00:00 2001 From: Jackie Li Date: Wed, 21 Oct 2026 09:43:06 +0100 Subject: [PATCH 2/2] feat(lint): flag URLFor values pre-escaped with net/url The new escape category reports url.QueryEscape, url.PathEscape and url.Values.Encode calls passed as URLFor values (positional, key/value or in a literal map), which URLFor now escapes again. It needs no page tree, so it runs outside the per-tree check, and covers gsx url filters through the generated code. Claude-Session: https://claude.ai/code/session_01NpjJeP32ycvAHNFXRgTviG --- tools/lint/escape.go | 69 +++++++++++++++++++++++++++ tools/lint/escape_integration_test.go | 30 ++++++++++++ tools/lint/testdata/escape/go.mod | 9 ++++ tools/lint/testdata/escape/go.sum | 4 ++ tools/lint/testdata/escape/main.go | 38 +++++++++++++++ 5 files changed, 150 insertions(+) create mode 100644 tools/lint/escape.go create mode 100644 tools/lint/escape_integration_test.go create mode 100644 tools/lint/testdata/escape/go.mod create mode 100644 tools/lint/testdata/escape/go.sum create mode 100644 tools/lint/testdata/escape/main.go diff --git a/tools/lint/escape.go b/tools/lint/escape.go new file mode 100644 index 0000000..49e0810 --- /dev/null +++ b/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" +} diff --git a/tools/lint/escape_integration_test.go b/tools/lint/escape_integration_test.go new file mode 100644 index 0000000..3e64e98 --- /dev/null +++ b/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) + } +} diff --git a/tools/lint/testdata/escape/go.mod b/tools/lint/testdata/escape/go.mod new file mode 100644 index 0000000..ed27e4c --- /dev/null +++ b/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 => ../../../.. diff --git a/tools/lint/testdata/escape/go.sum b/tools/lint/testdata/escape/go.sum new file mode 100644 index 0000000..2108c87 --- /dev/null +++ b/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= diff --git a/tools/lint/testdata/escape/main.go b/tools/lint/testdata/escape/main.go new file mode 100644 index 0000000..9c4815f --- /dev/null +++ b/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 +}