Keep reserved prefixes out of the SPA fallback under dot segments
Independent review Not signed off — 1 high (#4)
Independent review Not signed off — 1 high (#4)
Address review findings F1 and F2: - F1: check reserved prefixes against the request path before and after cleaning, so /auth/../quests/42 and /api%2F..%2Fquests/42 get the JSON 404 instead of index.html. Add frontend and router tests for both. - F2: add an unhashed asset (robots.txt) to the frontend fixture and assert its status, body and immutable Cache-Control. Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
This commit is contained in:
@@ -48,7 +48,8 @@ func Assets() fs.FS {
|
|||||||
// NewHandler returns a handler that serves files from fsys. A GET or HEAD for a
|
// NewHandler returns a handler that serves files from fsys. A GET or HEAD for a
|
||||||
// path that is not a file in fsys is answered with index.html so client-side
|
// path that is not a file in fsys is answered with index.html so client-side
|
||||||
// routes load the app. Paths equal to or under a reserved prefix (for example
|
// routes load the app. Paths equal to or under a reserved prefix (for example
|
||||||
// "/api") are never answered with index.html; they are passed to notFound.
|
// "/api") are never answered with index.html, whether before or after the path
|
||||||
|
// is cleaned; they are passed to notFound.
|
||||||
// fsys must contain index.html.
|
// fsys must contain index.html.
|
||||||
func NewHandler(fsys fs.FS, logger *slog.Logger, notFound http.Handler, reserved ...string) (http.Handler, error) {
|
func NewHandler(fsys fs.FS, logger *slog.Logger, notFound http.Handler, reserved ...string) (http.Handler, error) {
|
||||||
if _, err := fs.Stat(fsys, indexFile); err != nil {
|
if _, err := fs.Stat(fsys, indexFile); err != nil {
|
||||||
@@ -66,7 +67,9 @@ type handler struct {
|
|||||||
|
|
||||||
func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
|
func (h *handler) ServeHTTP(w http.ResponseWriter, r *http.Request) {
|
||||||
urlPath := path.Clean("/" + r.URL.Path)
|
urlPath := path.Clean("/" + r.URL.Path)
|
||||||
if h.isReserved(urlPath) {
|
// Check the uncleaned path too, so dot segments cannot escape a reserved
|
||||||
|
// prefix into the fallback ("/auth/../quests/42" cleans to "/quests/42").
|
||||||
|
if h.isReserved(r.URL.Path) || h.isReserved(urlPath) {
|
||||||
h.notFound.ServeHTTP(w, r)
|
h.notFound.ServeHTTP(w, r)
|
||||||
return
|
return
|
||||||
}
|
}
|
||||||
|
|||||||
@@ -12,6 +12,7 @@ import (
|
|||||||
const (
|
const (
|
||||||
indexBody = "<!doctype html><title>fixture index</title>"
|
indexBody = "<!doctype html><title>fixture index</title>"
|
||||||
assetBody = "console.log('fixture asset')"
|
assetBody = "console.log('fixture asset')"
|
||||||
|
robotsBody = "User-agent: *"
|
||||||
swBody = "self.addEventListener('fetch', () => {})"
|
swBody = "self.addEventListener('fetch', () => {})"
|
||||||
manifestBody = `{"name":"MealQuest"}`
|
manifestBody = `{"name":"MealQuest"}`
|
||||||
notFoundBody = "reserved not found"
|
notFoundBody = "reserved not found"
|
||||||
@@ -23,6 +24,7 @@ func fixture() fstest.MapFS {
|
|||||||
"sw.js": {Data: []byte(swBody)},
|
"sw.js": {Data: []byte(swBody)},
|
||||||
"manifest.webmanifest": {Data: []byte(manifestBody)},
|
"manifest.webmanifest": {Data: []byte(manifestBody)},
|
||||||
"assets/index-AbC123.js": {Data: []byte(assetBody)},
|
"assets/index-AbC123.js": {Data: []byte(assetBody)},
|
||||||
|
"robots.txt": {Data: []byte(robotsBody)},
|
||||||
}
|
}
|
||||||
}
|
}
|
||||||
|
|
||||||
@@ -50,6 +52,7 @@ func TestServe(t *testing.T) {
|
|||||||
wantCache string
|
wantCache string
|
||||||
}{
|
}{
|
||||||
{"hashed asset", http.MethodGet, "/assets/index-AbC123.js", assetBody, cacheImmutable},
|
{"hashed asset", http.MethodGet, "/assets/index-AbC123.js", assetBody, cacheImmutable},
|
||||||
|
{"unhashed asset", http.MethodGet, "/robots.txt", robotsBody, cacheImmutable},
|
||||||
{"root", http.MethodGet, "/", indexBody, cacheNoCache},
|
{"root", http.MethodGet, "/", indexBody, cacheNoCache},
|
||||||
{"index.html", http.MethodGet, "/index.html", indexBody, cacheNoCache},
|
{"index.html", http.MethodGet, "/index.html", indexBody, cacheNoCache},
|
||||||
{"service worker", http.MethodGet, "/sw.js", swBody, cacheNoCache},
|
{"service worker", http.MethodGet, "/sw.js", swBody, cacheNoCache},
|
||||||
@@ -97,7 +100,7 @@ func TestReservedPrefixesNotServed(t *testing.T) {
|
|||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
h := newTestHandler(t)
|
h := newTestHandler(t)
|
||||||
for _, path := range []string{"/api", "/api/unknown", "/auth/login", "/media/x.jpg", "/health", "/health/deep", "/x/../api/unknown"} {
|
for _, path := range []string{"/api", "/api/unknown", "/auth/login", "/media/x.jpg", "/health", "/health/deep", "/x/../api/unknown", "/auth/../quests/42", "/api%2F..%2Fquests/42"} {
|
||||||
t.Run(path, func(t *testing.T) {
|
t.Run(path, func(t *testing.T) {
|
||||||
t.Parallel()
|
t.Parallel()
|
||||||
|
|
||||||
|
|||||||
@@ -102,6 +102,8 @@ func TestReservedPathsNotFound(t *testing.T) {
|
|||||||
{http.MethodGet, "/auth/callback"},
|
{http.MethodGet, "/auth/callback"},
|
||||||
{http.MethodGet, "/media/photo.jpg"},
|
{http.MethodGet, "/media/photo.jpg"},
|
||||||
{http.MethodGet, "/health/deep"},
|
{http.MethodGet, "/health/deep"},
|
||||||
|
{http.MethodGet, "/auth/../quests/42"},
|
||||||
|
{http.MethodGet, "/api%2F..%2Fquests/42"},
|
||||||
}
|
}
|
||||||
for _, tt := range tests {
|
for _, tt := range tests {
|
||||||
t.Run(tt.method+" "+tt.path, func(t *testing.T) {
|
t.Run(tt.method+" "+tt.path, func(t *testing.T) {
|
||||||
@@ -135,6 +137,7 @@ func TestFrontend(t *testing.T) {
|
|||||||
{"/", fixtureIndex, "no-cache"},
|
{"/", fixtureIndex, "no-cache"},
|
||||||
{"/assets/index-AbC123.js", fixtureAsset, "public, max-age=31536000, immutable"},
|
{"/assets/index-AbC123.js", fixtureAsset, "public, max-age=31536000, immutable"},
|
||||||
{"/quests/42", fixtureIndex, "no-cache"},
|
{"/quests/42", fixtureIndex, "no-cache"},
|
||||||
|
{"/quests/42/edit", fixtureIndex, "no-cache"},
|
||||||
}
|
}
|
||||||
for _, tt := range tests {
|
for _, tt := range tests {
|
||||||
t.Run(tt.path, func(t *testing.T) {
|
t.Run(tt.path, func(t *testing.T) {
|
||||||
|
|||||||
Reference in New Issue
Block a user