fix(serve): ask the disk, not the extension, before dropping a catch-all

`getRoute` dropped every catch-all candidate whenever the request path
carried a non-page extension. That guard is #1841 and it protects something
real: `getRoute` runs before the publicDir handler, so an unguarded catch-all
answers `/images/logo.jpg` with the 404 page before the actual file can be
served.

But the extension is a guess about intent, and it is the wrong question for
an app whose catch-all legitimately serves paths with dots in them. A code
browser is exactly that app - `/owner/repo/tree/main/src/index.ts` is a page,
not an asset - and so is a docs site addressing `guide.md`. For those, the
only route that could answer is the one being dropped, and the request falls
through to a fallback that appends `.html`, so the view is handed a file name
nobody asked for and reports it missing.

Asking the disk keeps exactly what #1841 was protecting - a real
`public/images/logo.jpg` still wins over `[...all].stx` - and costs one stat
on a path that was about to be looked up anyway. `isStaticAssetPath` stays,
because the implicit-`.html` retry further down is a different question with
the same shape and still wants the cheap answer.

`publicFileExists` normalizes traversal before the prefix check, the same way
the publicDir handler does, so `..` cannot walk out of the root and report on
a file that is none of the caller's business; it decodes escapes the way the
server will, declines an embedded NUL, and is false for a directory, which is
not a file to serve. All six are tested, along with the regression itself: an
extensioned path publicDir does not have still resolves to the catch-all.

The resolver test that mirrors `getRoute` now mirrors the new one - it stands
the disk up as a list of paths rather than re-deriving the old extension
rule, which would have kept passing while the server did something else.

Author: chrisbbreuer

packages/bun-plugin/src/serve.ts

@@ -14,7 +14,7 @@
  */
 
 import { serve as bunServe, Glob } from 'bun'
-import { watch as fsWatch } from 'node:fs'
+import { existsSync, watch as fsWatch, statSync } from 'node:fs'
 import nodeFs from 'node:fs/promises'
 import nodePath from 'node:path'
 import process from 'node:process'
@@ -297,6 +297,50 @@ export function isStaticAssetPath(requestPath: string): boolean {
   return /\.[a-z0-9]+$/i.test(requestPath) && !/\.(?:stx|md|html)$/i.test(requestPath)
 }
 
+/**
+ * Does a file actually exist under `publicDir` for this request path?
+ *
+ * The extension test above is a guess about intent, and it is the wrong
+ * question to ask before dropping a catch-all: an app whose catch-all
+ * legitimately serves paths that carry extensions - a file browser, a docs
+ * site addressing `guide.md`, anything rendering a repository - has every
+ * such page refused, because the guess says "asset" and the only route that
+ * could answer is the one being dropped.
+ *
+ * Asking the disk instead keeps what stacksjs/stx#1841 was protecting (a real
+ * `public/images/logo.jpg` still wins over `[...all].stx`) and costs one stat
+ * on a path that was going to be looked up moments later anyway.
+ *
+ * Traversal is normalized before the prefix check, the same way the publicDir
+ * handler does it, so `..` cannot walk out of the root and report on a file
+ * that is none of the caller's business.
+ */
+export function publicFileExists(requestPath: string, publicDir: string): boolean {
+  let decoded: string
+  try {
+    decoded = decodeURIComponent(requestPath)
+  }
+  catch {
+    decoded = requestPath
+  }
+
+  if (decoded.includes('\0'))
+    return false
+
+  const publicRoot = nodePath.resolve(process.cwd(), publicDir)
+  const resolved = nodePath.resolve(publicRoot, `.${decoded}`)
+  const inside = resolved === publicRoot || resolved.startsWith(`${publicRoot}${nodePath.sep}`)
+  if (!inside)
+    return false
+
+  try {
+    return existsSync(resolved) && !statSync(resolved).isDirectory()
+  }
+  catch {
+    return false
+  }
+}
+
 /**
  * Escape a string for safe interpolation into HTML text/attribute context.
  * Used so a crafted request path can't inject markup into the 404 page
@@ -2379,7 +2423,11 @@ function __stxOverlay(errs){
     // the path carries a non-page file extension, drop catch-all candidates so
     // the request falls through to publicDir (and then the real 404 page).
     // Specific routes may still match (rare, but legitimate). stacksjs/stx#1841.
-    const isAssetRequest = isStaticAssetPath(normalizedPath)
+    // A catch-all is dropped when publicDir really holds this file, rather
+    // than whenever the path merely looks like an asset: the extension is a
+    // guess about intent, and it refuses every legitimate catch-all page
+    // whose path carries a dot (see publicFileExists).
+    const isAssetRequest = publicFileExists(`/${normalizedPath}`, publicDir)
     const dynamicFiles = files
       .filter((f) => {
       const nf = f.replace(/^\.\//, '').replace(/\\/g, '/')

packages/bun-plugin/test/routes-catch-all.test.ts

@@ -1,5 +1,7 @@
-import { describe, expect, it } from 'bun:test'
-import { buildDynamicRouteRegexes, isStaticAssetPath, routeSpecificity } from '../src/serve'
+import { afterAll, beforeAll, describe, expect, it } from 'bun:test'
+import { mkdirSync, rmSync, writeFileSync } from 'node:fs'
+import { join } from 'node:path'
+import { buildDynamicRouteRegexes, isStaticAssetPath, publicFileExists, routeSpecificity } from '../src/serve'
 
 /**
  * The plugin's dev server compiles file routes itself. Catch-alls were turned
@@ -108,10 +110,11 @@ describe('isStaticAssetPath — catch-all never shadows a static asset (#1841)',
       expect(isStaticAssetPath(p)).toBe(false)
   })
 
-  // Mirrors getRoute: catch-all candidates are dropped for asset requests, so
-  // the resolver returns null (→ publicDir serves the file, then the real 404).
-  const resolveAsset = (target: string, files: string[]): string | null => {
-    const asset = isStaticAssetPath(target)
+  // Mirrors getRoute: catch-all candidates are dropped when publicDir really
+  // holds the file, so the resolver returns null and publicDir serves it.
+  // `exists` stands in for the disk.
+  const resolveAsset = (target: string, files: string[], exists: string[] = []): string | null => {
+    const asset = exists.includes(target)
     const candidates = files
       .filter(f => f.includes('[') && !(asset && /\[\.\.\./.test(f)))
       .sort((a, b) => routeSpecificity(b) - routeSpecificity(a))
@@ -124,9 +127,10 @@ describe('isStaticAssetPath — catch-all never shadows a static asset (#1841)',
     return null
   }
 
-  it('an image path does NOT resolve to the catch-all (falls through to publicDir)', () => {
+  it('an image that publicDir has does NOT resolve to the catch-all', () => {
     const files = ['[...all].stx', 'foo/[id].stx']
-    expect(resolveAsset('images/background-auth.jpg', files)).toBeNull()
+    const target = 'images/background-auth.jpg'
+    expect(resolveAsset(target, files, [target])).toBeNull()
   })
 
   it('a real page miss still resolves to the catch-all', () => {
@@ -138,4 +142,60 @@ describe('isStaticAssetPath — catch-all never shadows a static asset (#1841)',
     const files = ['[...all].stx', 'download/[file].stx']
     expect(resolveAsset('download/report.pdf', files)).toBe('download/[file].stx')
   })
+
+  /*
+   * The regression the disk check exists for. An app whose catch-all serves
+   * paths that carry extensions - a code browser rendering
+   * `/owner/repo/tree/main/src/index.ts` - had every such page refused,
+   * because the extension test called it an asset and dropped the only route
+   * that could answer. Nothing is at that path in publicDir, so nothing
+   * should be dropped.
+   */
+  it('an extensioned path publicDir does not have still resolves to the catch-all', () => {
+    const files = ['[...all].stx', 'foo/[id].stx']
+    expect(resolveAsset('owner/repo/tree/main/src/index.ts', files)).toBe('[...all].stx')
+  })
+})
+
+/**
+ * The disk check itself. It answers for files that are really there, and
+ * refuses to answer for anything outside the root however the path is spelled.
+ */
+describe('publicFileExists', () => {
+  const root = 'test/fixtures/public-exists'
+
+  beforeAll(() => {
+    mkdirSync(join(root, 'images'), { recursive: true })
+    writeFileSync(join(root, 'images', 'logo.jpg'), 'not really a jpeg')
+  })
+
+  afterAll(() => {
+    rmSync(root, { recursive: true, force: true })
+  })
+
+  it('finds a file that is there', () => {
+    expect(publicFileExists('/images/logo.jpg', root)).toBe(true)
+  })
+
+  it('does not find one that is not', () => {
+    expect(publicFileExists('/images/missing.jpg', root)).toBe(false)
+    expect(publicFileExists('/owner/repo/tree/main/src/index.ts', root)).toBe(false)
+  })
+
+  it('is false for a directory, which is not a file to serve', () => {
+    expect(publicFileExists('/images', root)).toBe(false)
+  })
+
+  it('refuses to walk out of the root', () => {
+    expect(publicFileExists('/../../package.json', root)).toBe(false)
+    expect(publicFileExists('/images/../../../package.json', root)).toBe(false)
+  })
+
+  it('reads an escaped path the same way the server will', () => {
+    expect(publicFileExists('/images/%6Cogo.jpg', root)).toBe(true)
+  })
+
+  it('declines a path carrying a NUL', () => {
+    expect(publicFileExists('/images/logo.jpg%00.txt', root)).toBe(false)
+  })
 })