From c0100231a94705e0822ad91a82512075737e8fca Mon Sep 17 00:00:00 2001 From: Claude Opus 5 Date: Thu, 30 Jul 2026 10:16:47 +0600 Subject: [PATCH] answer 404 for a static path the root refuses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Found by /invariants: a symlink under static/ pointing outside the site root answered 500. The guard held — os.Root refused it and no bytes escaped — but the response confirmed the path was there, where every other miss answers 404. Same reasoning as a hidden bundle answering 404 rather than 403 (ADR-0024). serveStatic now stats through the rooted FS first, so a directory, a missing file, and a refused name are one answer. That also folds the old noListing and staticFS into one function, since "cannot serve this" was already their shared job. The test uses a real temp directory rather than a MapFS, because the guard under test belongs to os.Root; verified it fails with 500 against the previous code before keeping it. Splitting web_test.go at the seam the package already had — resolve_test.go for what a path means, web_test.go for what happens once it resolves — because it crossed FILE_LOC_WARN. Same response as content.go at entry 9. --- docs/state.md | 6 +- internal/web/resolve_test.go | 89 +++++++++++++++++++++++++ internal/web/web.go | 27 ++++---- internal/web/web_test.go | 122 ++++++++++++----------------------- 4 files changed, 148 insertions(+), 96 deletions(-) create mode 100644 internal/web/resolve_test.go diff --git a/docs/state.md b/docs/state.md index 078399b..71bda06 100644 --- a/docs/state.md +++ b/docs/state.md @@ -1,6 +1,6 @@ # State -**Verified against:** `430a5ae` on 2026-07-30 — update this line every change. +**Verified against:** `26cc829` on 2026-07-30 — update this line every change. If this file disagrees with the code, the code is right and this file is a bug. ## Inventory @@ -15,9 +15,9 @@ If this file disagrees with the code, the code is right and this file is a bug. | `internal/render/chrome.go` | the engine's own words: phrase table, month names, digits, and the `t`/`num`/`day` template funcs (ADR-0034) | 105 | | `internal/render/templates/` | reference theme: `base.html`, `page.html`, `list.html`, `theme.css` (ADR-0026) | — | | `internal/web/resolve.go` | URL → (key, lang, page, tag) or a canonical redirect: language prefix, `/en/…` fork guard, pagination, tags, trailing slash | 112 | -| `internal/web/web.go` | handler: resolve, look up with fallback, section and tag listings, sequence, `/static/`, degrade on failure | 156 | +| `internal/web/web.go` | handler: resolve, look up with fallback, section and tag listings, sequence, `/static/` (misses and refusals alike answer 404), degrade on failure | 152 | | `cmd/khosra/main.go` | flags, wiring, startup — the only place things are assembled | 53 | -| `*_test.go` | table-driven; symlink escape, permalink, language fallback, aliases, pagination, tags, sequences, chrome, typography, 404 | 1119 | +| `*_test.go` | table-driven, one file per source file; symlink escape (content and static), canonical paths, language fallback, aliases, pagination, tags, sequences, chrome, typography, 404 | 1172 | Serves a bundle at `/{section}/{slug}/`, a paginated listing per section, tag listings global and section-narrowed, sequence navigation and a series archive on any nested bundle, and `static/` verbatim. diff --git a/internal/web/resolve_test.go b/internal/web/resolve_test.go new file mode 100644 index 0000000..2242baa --- /dev/null +++ b/internal/web/resolve_test.go @@ -0,0 +1,89 @@ +package web + +import ( + "net/http" + "net/http/httptest" + "testing" +) + +// These cover what a path *means*: which spelling is canonical and which redirects to it. What happens +// once a path resolves is web_test.go's business. + +func TestSlashlessPathRedirectsPermanently(t *testing.T) { + h := testHandler(t) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/pages/about", nil)) + if rec.Code != http.StatusMovedPermanently { + t.Fatalf("got %d, want 301", rec.Code) + } + if loc := rec.Header().Get("Location"); loc != "/pages/about/" { + t.Errorf("Location = %q", loc) + } +} + +func TestDefaultLanguagePrefixRedirectsToRoot(t *testing.T) { + h := multilingualHandler(t) + for path, want := range map[string]string{ + "/en/pages/about/": "/pages/about/", + "/en/pages/about": "/pages/about/", + } { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil)) + if rec.Code != http.StatusMovedPermanently { + t.Errorf("GET %s = %d, want 301: /en/… must never be live (ADR-0009)", path, rec.Code) + continue + } + if loc := rec.Header().Get("Location"); loc != want { + t.Errorf("GET %s → %q, want %q", path, loc, want) + } + } +} + +func TestPrefixedSlashlessPathRedirectsWithItsPrefix(t *testing.T) { + h := multilingualHandler(t) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/bn/pages/about", nil)) + if rec.Code != http.StatusMovedPermanently { + t.Fatalf("got %d, want 301", rec.Code) + } + if loc := rec.Header().Get("Location"); loc != "/bn/pages/about/" { + t.Errorf("Location = %q, want /bn/pages/about/", loc) + } +} + +func TestUnknownLanguagePrefixIsNotALanguage(t *testing.T) { + h := multilingualHandler(t) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/fr/pages/about/", nil)) + if rec.Code != http.StatusNotFound { + t.Errorf("got %d, want 404: fr is not a language this site has", rec.Code) + } +} + +func TestPageOneIsNeverItsOwnURL(t *testing.T) { + h := listingHandler(t, 3) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/posts/page/1/", nil)) + if rec.Code != http.StatusMovedPermanently { + t.Fatalf("got %d, want 301 (ADR-0028)", rec.Code) + } + if loc := rec.Header().Get("Location"); loc != "/posts/" { + t.Errorf("Location = %q, want /posts/", loc) + } +} + +func TestTagPathsCanonicaliseAndMiss(t *testing.T) { + h := tagHandler(t) + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/tags/monsoon", nil)) + if rec.Code != http.StatusMovedPermanently || rec.Header().Get("Location") != "/tags/monsoon/" { + t.Errorf("slashless tag path = %d %q", rec.Code, rec.Header().Get("Location")) + } + for _, path := range []string{"/tags/nothing/", "/tags/", "/posts/tags/nothing/"} { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil)) + if rec.Code != http.StatusNotFound { + t.Errorf("GET %s = %d, want 404", path, rec.Code) + } + } +} diff --git a/internal/web/web.go b/internal/web/web.go index d5334bd..7e5a9eb 100644 --- a/internal/web/web.go +++ b/internal/web/web.go @@ -21,29 +21,28 @@ func Handler(site *content.Site, r *render.Renderer, siteFS fs.FS) http.Handler serve(w, req, site, r) }) if siteFS != nil { - mux.Handle("GET /static/", http.StripPrefix("/static/", noListing(staticFS(siteFS)))) + if sub, err := fs.Sub(siteFS, "static"); err == nil { + mux.Handle("GET /static/", http.StripPrefix("/static/", serveStatic(sub))) + } } return mux } -// staticFS serves the site root's static/ directory verbatim. It keeps the os.Root guarantee, because +// serveStatic serves the site root's static/ directory verbatim. It keeps the os.Root guarantee, because // the fs.FS it is given is the one rooted there (ADR-0031). -func staticFS(siteFS fs.FS) http.Handler { - sub, err := fs.Sub(siteFS, "static") - if err != nil { - return http.NotFoundHandler() - } - return http.FileServerFS(sub) -} - -// noListing refuses directory paths, so static/ never answers with an index of its own contents. -func noListing(h http.Handler) http.Handler { +// +// Anything it cannot serve answers 404: a directory, a missing file, or a name the root refuses because it +// resolves outside. A listing would expose the tree, and an error page for a refused symlink would confirm +// the path is there — the same reason a hidden bundle answers 404 rather than 403 (ADR-0024). +func serveStatic(sub fs.FS) http.Handler { + files := http.FileServerFS(sub) return http.HandlerFunc(func(w http.ResponseWriter, req *http.Request) { - if req.URL.Path == "" || strings.HasSuffix(req.URL.Path, "/") { + info, err := fs.Stat(sub, strings.TrimPrefix(req.URL.Path, "/")) + if err != nil || info.IsDir() { http.NotFound(w, req) return } - h.ServeHTTP(w, req) + files.ServeHTTP(w, req) }) } diff --git a/internal/web/web_test.go b/internal/web/web_test.go index 878f534..78818a4 100644 --- a/internal/web/web_test.go +++ b/internal/web/web_test.go @@ -4,6 +4,8 @@ import ( "fmt" "net/http" "net/http/httptest" + "os" + "path/filepath" "strings" "testing" "testing/fstest" @@ -48,18 +50,6 @@ func TestServeBundleAtItsPermalink(t *testing.T) { } } -func TestSlashlessPathRedirectsPermanently(t *testing.T) { - h := testHandler(t) - rec := httptest.NewRecorder() - h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/pages/about", nil)) - if rec.Code != http.StatusMovedPermanently { - t.Fatalf("got %d, want 301", rec.Code) - } - if loc := rec.Header().Get("Location"); loc != "/pages/about/" { - t.Errorf("Location = %q", loc) - } -} - func TestUnknownPathsAre404(t *testing.T) { h := testHandler(t) for _, path := range []string{"/", "/nope/", "/pages/nope", "/pages/about/deeper/"} { @@ -124,45 +114,6 @@ func TestMissingVariantFallsBackAndSaysSo(t *testing.T) { } } -func TestDefaultLanguagePrefixRedirectsToRoot(t *testing.T) { - h := multilingualHandler(t) - for path, want := range map[string]string{ - "/en/pages/about/": "/pages/about/", - "/en/pages/about": "/pages/about/", - } { - rec := httptest.NewRecorder() - h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil)) - if rec.Code != http.StatusMovedPermanently { - t.Errorf("GET %s = %d, want 301: /en/… must never be live (ADR-0009)", path, rec.Code) - continue - } - if loc := rec.Header().Get("Location"); loc != want { - t.Errorf("GET %s → %q, want %q", path, loc, want) - } - } -} - -func TestPrefixedSlashlessPathRedirectsWithItsPrefix(t *testing.T) { - h := multilingualHandler(t) - rec := httptest.NewRecorder() - h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/bn/pages/about", nil)) - if rec.Code != http.StatusMovedPermanently { - t.Fatalf("got %d, want 301", rec.Code) - } - if loc := rec.Header().Get("Location"); loc != "/bn/pages/about/" { - t.Errorf("Location = %q, want /bn/pages/about/", loc) - } -} - -func TestUnknownLanguagePrefixIsNotALanguage(t *testing.T) { - h := multilingualHandler(t) - rec := httptest.NewRecorder() - h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/fr/pages/about/", nil)) - if rec.Code != http.StatusNotFound { - t.Errorf("got %d, want 404: fr is not a language this site has", rec.Code) - } -} - func aliasHandler(t *testing.T) http.Handler { t.Helper() fsys := fstest.MapFS{ @@ -269,18 +220,6 @@ func TestPaginationSplitsAndLinks(t *testing.T) { } } -func TestPageOneIsNeverItsOwnURL(t *testing.T) { - h := listingHandler(t, 3) - rec := httptest.NewRecorder() - h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/posts/page/1/", nil)) - if rec.Code != http.StatusMovedPermanently { - t.Fatalf("got %d, want 301 (ADR-0028)", rec.Code) - } - if loc := rec.Header().Get("Location"); loc != "/posts/" { - t.Errorf("Location = %q, want /posts/", loc) - } -} - func TestPagePastTheEndIs404(t *testing.T) { h := listingHandler(t, 3) rec := httptest.NewRecorder() @@ -320,6 +259,47 @@ func TestStaticFilesAreServedAndDirectoriesAreNot(t *testing.T) { } } +func TestAStaticPathThatEscapesTheRootIs404(t *testing.T) { + // A real directory, not a MapFS: the guard being tested belongs to os.Root (ADR-0031), and the point is + // what the *response* is when it refuses — a miss, never an error page that confirms the path. + dir := t.TempDir() + if err := os.MkdirAll(filepath.Join(dir, "static"), 0o755); err != nil { + t.Fatal(err) + } + if err := os.WriteFile(filepath.Join(dir, "static", "ok.css"), []byte("body{}"), 0o644); err != nil { + t.Fatal(err) + } + outside := filepath.Join(dir, "outside.txt") + if err := os.WriteFile(outside, []byte("secret"), 0o644); err != nil { + t.Fatal(err) + } + if err := os.Symlink(outside, filepath.Join(dir, "static", "escape.txt")); err != nil { + t.Skipf("symlinks unavailable: %v", err) + } + fsys, err := content.OpenSite(dir) + if err != nil { + t.Fatal(err) + } + r, err := render.New(fsys) + if err != nil { + t.Fatal(err) + } + h := Handler(content.NewSite(nil), r, fsys) + for path, want := range map[string]int{ + "/static/ok.css": http.StatusOK, + "/static/escape.txt": http.StatusNotFound, + } { + rec := httptest.NewRecorder() + h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil)) + if rec.Code != want { + t.Errorf("GET %s = %d, want %d", path, rec.Code, want) + } + if strings.Contains(rec.Body.String(), "secret") { + t.Fatalf("GET %s served bytes from outside the root", path) + } + } +} + func seriesHandler(t *testing.T) http.Handler { t.Helper() fsys := fstest.MapFS{ @@ -460,19 +440,3 @@ func TestSectionNarrowedTagListing(t *testing.T) { t.Errorf("narrowing to comics should drop the posts entry:\n%s", body) } } - -func TestTagPathsCanonicaliseAndMiss(t *testing.T) { - h := tagHandler(t) - rec := httptest.NewRecorder() - h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, "/tags/monsoon", nil)) - if rec.Code != http.StatusMovedPermanently || rec.Header().Get("Location") != "/tags/monsoon/" { - t.Errorf("slashless tag path = %d %q", rec.Code, rec.Header().Get("Location")) - } - for _, path := range []string{"/tags/nothing/", "/tags/", "/posts/tags/nothing/"} { - rec := httptest.NewRecorder() - h.ServeHTTP(rec, httptest.NewRequest(http.MethodGet, path, nil)) - if rec.Code != http.StatusNotFound { - t.Errorf("GET %s = %d, want 404", path, rec.Code) - } - } -}