kwhitley / itty-router

A little router.
MIT License
1.79k stars 78 forks source link

Itty Router hangs when sending "Not Allowed" method with body. #28

Closed RuthSargent closed 3 years ago

RuthSargent commented 3 years ago

We are using itty-router and have set our own routing up to return 405 response on certain methods. We are sending a POST request with a body to a 3rd party service, however when sending the same request with a not allowed method, the request hangs. If we dont attach a body, we receive the 405 response.

More context on how we have set up our routing is as follows:

router .post('/api/v1/sign-up', middleware1, middleware2, middleware3) .post( '', () => new Response('Not found', { status: 404, statusText: 'Not found' }), ) .head( '', () => new Response('Method not allowed', { status: 405, statusText: 'Method not allowed', }), ) .get( '', () => new Response('Method not allowed', { status: 405, statusText: 'Method not allowed', }), ) .put( '', () => new Response('Method not allowed', { status: 405, statusText: 'Method not allowed', }), ) ...

kwhitley commented 3 years ago

I'll take a look this morning!

At a glance though, I can't imagine that itty would have anything to do with it, as it contains no concept of methods and has no checks for status codes (it's about the most agnostic pass-through library ever, haha). I'll see what I can find!

kwhitley commented 3 years ago

But also, if you're blocking methods, I'd def recommend using a wildcard route. An empty string will essentially match only to "/"

*UPDATE: I think this is the problem... just make sure those blocked methods are route matching to `'', not''`**

kwhitley commented 3 years ago

Conversely if you only want to allow a specific method/call, add a single .all() route to the end to catch everything else:

import { Router } from 'itty-router'
import { text, missing, status } from 'itty-router-extras'

const router = Router()

router
  .post('/your-allowed-route', () => text('Success!'))  // return successfully for your approved route
  .post('*', () => missing('Not found'))                // catch other POST routes
  .all('*', () => status(405, 'Method not allowed'))    // and any other method will get a 405
RuthSargent commented 3 years ago

Thanks for your reply. I'll give the wildcard route a go.

EDIT: I had to update our version of itty-router to use the wildcard route and our code is working as it previously was. Unfortunately though it has not solved the original problem. We get a method not allowed response with all other routes if we do not attach a body, but if a body is present then it hangs.

kwhitley commented 3 years ago

Gotcha, I'll set up a test case for this... super weird. Can you show me a copy of the Request (as much as you're able) you're feeding through the router?

Doubly odd because itty-router doesn't touch, look at, etc the body... so it may be an upstream environment issue... or an uncaught error.

Just to confirm:

POST /api/v1/sign-up (with body) --> OK
ANY METHOD, ANY PATH (no body) --> 405
ANY METHOD, ANY PATH (with body) --> hangs/fails to return

That said, if anything is throwing in the handlers/middleware, itty core Router does NOT handle this automatically, resulting in a route that "fails to respond" (hangs). I'd recommend (if you haven't already), adding a catch to the handle to get more insight (and more importantly to make sure it always returns a Response). Wherever you call router.handle, try this:

router.handle(request, ...args).catch(err => new Response(err.message, { status: 500 }))

Conversely, if you're leveraging the utility functions in itty-router-extras, it introduces a "throwable" router that will safely catch thrown errors that you may try as well (just a Router that calls the catch on the handle for you):

import { ThrowableRouter } from 'itty-router-extras'

const router = ThrowableRouter()

// all else is the same, but no need to call the final catch

router.handle(request, ...args) // will automatically return a Response for errors
RuthSargent commented 3 years ago

This is our handler code

import { Request, Router } from 'itty-router'

import middleware …

declare global {

const SECRET_KEY: string

const API_KEY: string

}

const router = Router()

router

.post(

'/api/email/v1/sign-up',

middleware,

SendEmail,

)

.post(

'*',

() => new Response('Not found', { status: 404, statusText: 'Not found' }),

)

.all('*', () => new Response('Method not allowed', {

status: 405,

statusText: 'Method not allowed'

}))

export const handleRequest = (request: Request | Request) =>

router.handle(request)

We made a request using postman to the above endpoint with any other method than post. Body is a json object containing an email. Eg

{

‘email’: @.***’

}

Request hangs unless we remove the body from the request.

From: "Kevin R. Whitley" @.> Reply to: kwhitley/itty-router @.> Date: Thursday, 15 April 2021 at 21:52 To: kwhitley/itty-router @.> Cc: Ruth Sargent @.>, Author @.***> Subject: Re: [kwhitley/itty-router] Itty Router hangs when sending "Not Allowed" method with body. (#28)

Gotcha, I'll set up a test case for this... super weird. Can you show me a copy of the Request (as much as you're able) you're feeding through the router?

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHubhttps://github.com/kwhitley/itty-router/issues/28#issuecomment-820723182, or unsubscribehttps://github.com/notifications/unsubscribe-auth/ASLB3AXY2LT5M2MD3Y4VL7TTI5G2PANCNFSM4245J4QQ.

kwhitley commented 3 years ago

Hmmm, I don't know what to tell you, because I use PATCH/PUT all the time with itty (with JSON body)...

Internally, itty does not reference/care-about any method names (aside from the pseudo "all" method/channel), nor does it check for anything like a body. It simply takes a Request, and route matches it against the method name (whatever it is) and the path, running associated middleware for the match. It does this until the first return it encounters within any matching route.

Wish I could help further, but I really don't believe there's anything within the very tiny code that even could be causing this... my guess is that the issue falls outside the scope of this lib, perhaps in the worker/service (maybe it has restrictions against body in non-POST requests?) or something like that.

I'm closing this for now, but if we get more info that suggests itty is actually the problem, I'll be more than happy to reopen :)

RuthSargent commented 3 years ago

Thanks for looking into it. We will explore further on this end.