diff --git a/internal/web/render/render.go b/internal/web/render/render.go index b4f2e14e..b9a02414 100644 --- a/internal/web/render/render.go +++ b/internal/web/render/render.go @@ -245,12 +245,25 @@ func (r *Renderer) RenderPage(w io.Writer, req *http.Request, name string, data // HTTPError writes an error page with the appropriate status code. If the // named error template doesn't exist a plain-text fallback is written. +// +// CX1: when callers pass an empty `message` (the common case — see the +// many `HTTPError(w, r, 404, "")` callsites), fall back to the request +// path so the rendered template ("The page `` doesn't exist...") +// names the URL the user tried to reach instead of "The page “ doesn't +// exist..." Most useful for the post-login redirect → 404 path, where +// users hit a deleted-repo URL and the page text was previously +// uninformative. We use the path (not the full URL) to keep query +// strings — which may carry sensitive tokens — out of the rendered page. func (r *Renderer) HTTPError(w http.ResponseWriter, req *http.Request, status int, message string) { pageName := errorPageFor(status) w.Header().Set("Content-Type", "text/html; charset=utf-8") w.Header().Set("Cache-Control", "no-store") w.WriteHeader(status) + if message == "" && req != nil { + message = req.URL.Path + } + data := struct { Title string Status int @@ -265,8 +278,13 @@ func (r *Renderer) HTTPError(w http.ResponseWriter, req *http.Request, status in RequestID: middleware.RequestIDFromContext(req.Context()), } if err := r.Render(w, pageName, data); err != nil { + // Content-Type was already set to text/html above and headers + // are flushed by the time we hit this fallback, so escape the + // user-controllable bits (message, request_id) before writing. _, _ = fmt.Fprintf(w, "%d %s\n%s\n(request_id=%s)\n", - status, http.StatusText(status), message, data.RequestID) + status, http.StatusText(status), + template.HTMLEscapeString(message), + template.HTMLEscapeString(data.RequestID)) } } diff --git a/internal/web/render/render_test.go b/internal/web/render/render_test.go index a9a7dc3c..e5dbc137 100644 --- a/internal/web/render/render_test.go +++ b/internal/web/render/render_test.go @@ -248,6 +248,91 @@ func TestNew_AcceptsRefsResolvedByPartials(t *testing.T) { } } +// CX1: when a caller passes an empty message to HTTPError (the dominant +// pattern at 404 sites — `HTTPError(w, r, 404, "")`), the renderer falls +// back to the request path so the rendered "page `` doesn't exist" +// message names the URL the user tried to reach. The pre-fix behavior +// rendered "page “ doesn't exist", a dead-end for users redirected to +// a deleted repo after login. +func TestHTTPError_EmptyMessageFallsBackToRequestPath(t *testing.T) { + t.Parallel() + fsys := fstest.MapFS{ + "_layout.html": &fstest.MapFile{Data: []byte( + `{{ define "layout" }}{{ template "body" . }}{{ end }}`, + )}, + "errors/404.html": &fstest.MapFile{Data: []byte( + `{{ define "body" }}missing: {{ .Message }}{{ end }}`, + )}, + } + r, err := New(fsys, Options{}) + if err != nil { + t.Fatalf("New: %v", err) + } + req := httptest.NewRequest("GET", "/octocat/hello-world", nil) + rw := httptest.NewRecorder() + r.HTTPError(rw, req, 404, "") + if !strings.Contains(rw.Body.String(), "missing: /octocat/hello-world") { + t.Errorf("empty message should fall back to req.URL.Path; body=%q", rw.Body.String()) + } +} + +// HTTPError must not leak query strings into the rendered page — they +// can carry session tokens (e.g. ?return_to=...&token=...). Only the +// path falls back. +func TestHTTPError_FallbackOmitsQueryString(t *testing.T) { + t.Parallel() + fsys := fstest.MapFS{ + "_layout.html": &fstest.MapFile{Data: []byte( + `{{ define "layout" }}{{ template "body" . }}{{ end }}`, + )}, + "errors/404.html": &fstest.MapFile{Data: []byte( + `{{ define "body" }}missing: {{ .Message }}{{ end }}`, + )}, + } + r, err := New(fsys, Options{}) + if err != nil { + t.Fatalf("New: %v", err) + } + req := httptest.NewRequest("GET", "/octocat/hello-world?token=secret", nil) + rw := httptest.NewRecorder() + r.HTTPError(rw, req, 404, "") + body := rw.Body.String() + if !strings.Contains(body, "/octocat/hello-world") { + t.Errorf("path missing from fallback; body=%q", body) + } + if strings.Contains(body, "secret") || strings.Contains(body, "token=") { + t.Errorf("query string leaked into rendered page; body=%q", body) + } +} + +// Explicit non-empty messages are preserved verbatim — the fallback +// only triggers when the caller didn't supply one. +func TestHTTPError_ExplicitMessageWins(t *testing.T) { + t.Parallel() + fsys := fstest.MapFS{ + "_layout.html": &fstest.MapFile{Data: []byte( + `{{ define "layout" }}{{ template "body" . }}{{ end }}`, + )}, + "errors/404.html": &fstest.MapFile{Data: []byte( + `{{ define "body" }}missing: {{ .Message }}{{ end }}`, + )}, + } + r, err := New(fsys, Options{}) + if err != nil { + t.Fatalf("New: %v", err) + } + req := httptest.NewRequest("GET", "/some/path", nil) + rw := httptest.NewRecorder() + r.HTTPError(rw, req, 404, "custom message") + body := rw.Body.String() + if !strings.Contains(body, "missing: custom message") { + t.Errorf("explicit message not rendered; body=%q", body) + } + if strings.Contains(body, "/some/path") { + t.Errorf("explicit message should suppress path fallback; body=%q", body) + } +} + // RenderPage preserves any Viewer / CSRFToken the handler set itself, // rather than overwriting them. The auto-inject is for handlers that // hand RenderPage an empty map; explicit handlers stay in control.