ZijianHe / koa-router

Router middleware for koa.
MIT License
4.85k stars 408 forks source link

allowedMethods not working if ctx.throw is used #387

Open IlyaSemenov opened 6 years ago

IlyaSemenov commented 6 years ago

If there is a middleware added after router.allowedMethods() that invokes ctx.throw, it prevents allowedMethods from responding to OPTIONS requests for legitimate URLs.

Experiment 1

Run server:

const Koa = require('koa')
const Router = require('koa-router')

const app = new Koa()
const router = new Router()

router.get('/test', ctx => {
  ctx.body = 'foo'
})

app
  .use(router.routes())
  .use(router.allowedMethods())
  .use(ctx => {
    ctx.status = 404
    ctx.body = "boom"
  })

app.listen(3000)

The OPTIONS handler for a legitimate route works as expected:

$ curl localhost:3000/test -XOPTIONS -v
...
< HTTP/1.1 200 OK
< Allow: HEAD
< Allow: GET

Experiment 2

Replace ctx.status = 404 with ctx.throw(404):

const Koa = require('koa')
const Router = require('koa-router')

const app = new Koa()
const router = new Router()

router.get('/test', ctx => {
  ctx.body = 'foo'
})

app
  .use(router.routes())
  .use(router.allowedMethods())
  .use(ctx => {
    ctx.throw(404, "boom")
  })

app.listen(3000)

The OPTIONS handler for a legitimate route will give 404:

$ curl localhost:3000/test -XOPTIONS -v
...
< HTTP/1.1 404 Not Found
< Content-Type: text/plain; charset=utf-8
< Content-Length: 4
boom

Expected result

Both experiment should produce identical results (OPTIONS handler replies with proper Allow: headers).

Frankly, I believe this issue is a sign of a more serious problem. allowedMethods() should check for matches immediately and should not run any subsequent middleware at all in case of successful match. This is the same as normal route handlers work: they do not run next middleware in chain when they can handle request themselves fully.

mosinve commented 6 years ago

as a workaround i used this small middleware instead of .use(router.allowedMethods()):

app.use(async (ctx, next) => {
    ctx.set('Access-Control-Allow-Origin', '*')
    ctx.set('Access-Control-Allow-Headers', 'Origin, X-Requested-With, Content-Type, Accept')
    if (ctx.method === 'OPTIONS') {
      const route = router.match(ctx.path)
      const methods = route.path.reduce((acc, path) => acc + path.methods.join(','), '')
      ctx.set('Access-Control-Allow-Methods', methods)
      ctx.status = 204
    } else {
      try {
        await next()
      } catch (e) {
        ctx.status = e.status
        ctx.body = e.message
      }
    }
  })