ReviewOS

stacks/bun-router

A catch-all OPTIONS route makes 404 unreachable: unmatched paths answer 405

#908
Closed glennmichael123 opened this 8 days ago ·
8 days ago

Summary

Registering a catch-all OPTIONS route for CORS preflight makes 404 unreachable. Every unmatched path under the wildcard's scope answers 405 Method Not Allowed with Allow: OPTIONS, on every verb, instead of 404 Not Found.

This is not a niche registration: it is the pattern the Stacks framework's own default CORS middleware documents, at storage/framework/defaults/app/Middleware/Cors.ts:65:

route.options('/api/*', () => new Response(null)).middleware('cors')

An app that follows it loses 404 for its entire API surface. I found this on a deployed Stacks app, where every nonexistent /api/* path returned:

{"success":false,"message":"Method Not Allowed","path":"/api/definitely-not-a-route","method":"GET","allowed":["OPTIONS"]}

with Allow: OPTIONS on the response.

Reproduction

@stacksjs/bun-router 0.1.20 (HEAD aaa9bee). Drop this in packages/bun-router/ and run bun _repro-405.ts:

import { Router } from './src/router/index'

async function status(router: Router, path: string, method = 'GET') {
  const res = await router.handleRequest(new Request(`http://localhost${path}`, { method }))
  return res.status
}

// Control: no preflight catch-all.
const control = new Router()
control.get('/users', () => new Response('ok'))
console.log('control  GET /nope                 ->', await status(control, '/nope'))

// The CORS preflight catch-all that Stacks' own Cors.ts documents.
const withCatchAll = new Router()
withCatchAll.get('/users', () => new Response('ok'))
withCatchAll.options('/*', () => new Response(null, { status: 204 }))
console.log('catchall GET /nope                 ->', await status(withCatchAll, '/nope'))
console.log('catchall GET /users (real route)   ->', await status(withCatchAll, '/users'))
console.log('catchall POST /users (real 405)    ->', await status(withCatchAll, '/users', 'POST'))

// The narrower form Cors.ts actually recommends.
const scoped = new Router()
scoped.get('/api/users', () => new Response('ok'))
scoped.options('/api/*', () => new Response(null, { status: 204 }))
console.log('scoped   GET /api/nope             ->', await status(scoped, '/api/nope'))
console.log('scoped   GET /outside-the-prefix   ->', await status(scoped, '/outside-the-prefix'))

Output:

control  GET /nope                 -> 404   <- correct
catchall GET /nope                 -> 405   <- should be 404
catchall GET /users (real route)   -> 200
catchall POST /users (real 405)    -> 405   <- correct, genuine method mismatch
scoped   GET /api/nope             -> 405   <- should be 404
scoped   GET /outside-the-prefix   -> 404   <- correct

The last two lines isolate the cause: the damage stops exactly at the wildcard's prefix. Genuine method mismatches (POST /users) keep working, so this is specifically about unmatched paths.

Root cause

getAllowedMethods() (src/router/router.ts:796) collects every method registered for a path, and a wildcard pattern matches any path. The 405-vs-404 branch then treats a non-empty result as proof the path exists:

const allowedMethods = this.getAllowedMethods(pathname, hostname, true)

if (allowedMethods.length > 0) {
  // 405
}

// unreachable once a wildcard route is registered
if (this.fallbackHandler) { /* ... */ }
// 404

(src/router/server.ts:625 onward; the same decision is duplicated at src/router/router.ts:679.)

A route that matches everything carries no information about whether a specific path exists, so it cannot support that inference. The fallbackHandler and the 404 are both dead code behind it.

Why it matters

  • Clients cannot tell a typo from a wrong verb. A 405 says "this resource exists, use another method", which sends anyone debugging a client toward a verb that does not exist.
  • Allow: OPTIONS is actively misleading and is the header conformant clients and generated SDKs read to retry.
  • fallback() silently stops firing, so custom 404 pages and 404 metrics go quiet. Monitoring that alerts on 404 rate sees zero.

Suggested direction

The fix belongs in the 405-vs-404 decision, not in asking apps to drop preflight routes. Some options, roughly in order of how much I like them:

  1. Exclude catch-all matches when computing allowedMethods for this decision: a pattern that matches every path is not evidence of a path.
  2. Require at least one non-OPTIONS method before answering 405, since a bare preflight registration never indicates a usable resource.
  3. Have getAllowedMethods report whether the match came from a literal/parameterised route or a wildcard, and let the caller decide.

Happy to put up a PR for whichever you prefer.

Secondary, same area

The two 405 responders emit different JSON shapes. router.ts:684 sends { error, path, method, allowed }; server.ts:636 sends { success, message, path, method, allowed }. A client parsing the error key gets a different answer depending on which entry point served the request.

Sign in to comment on this issue.