PROPOSAL: CORS

[kwhitley]

The Issue

1. The existing CORS procedure is a bit fiddly (requiring a user to manually register an upstream middleware, and a downstream corsify response-modifier).

2. The existing CORS procedure has a race-condition. In order to mirror the origin in the allow-origin response (a good practice, and strictly required in some scenarios), we share a scoped global. This can be polluted if many requests come in at once.

The Proposal

I'm proposing a potential rewrite of the process. It should follow the following principles:

• [ ] match as much of the existing spec/API as possible
• [x] be able to mirror origins
• [x] be able to NOT mirror origins (new)
• [x] handle cookies (appears to be a problem mentioned in issues)
• [x] NOT have a race condition

// BEFORE ===================================
import { router, createCors, text, error } from 'itty-router'

const router = Router()
const { preflight, corsify } = createCors()

router
  .all('*', preflight)
  .get('/', () => text('Hey!'))

export default {
  fetch: (request) => router
                        .fetch(request)
                        .catch(error)
                        .then(corsify)
}

// AFTER ====================================
???

[hiendaovinh]

I do understand the origin mirroring but why do we need a global allowOrigin var ? We can just execute in per-request basis, hence, no race-condition.

[zuisong]

We can just execute in per-request basis, hence, no race-condition.

It works for me

import { createCors, text, error,Router,json } from 'itty-router'

const router = Router()

router
  .get('/', () => text('Hey!'))
function ittyServer(request: Request) {
  const { preflight, corsify } = createCors()
  const serverFn = middlewareWrapperHandler(
    (req) => router.fetch(req).catch(error).then(json),
    [
      {
        preRequest: preflight,
        postRequest: corsify,
      },
    ],
  )
  return serverFn(request)
}

function middlewareWrapperHandler(handler: (request: Request) => Promise<Response>, m: Middleware[]) {
  return m.reduce((h, middleware) => {
    return async (request) => {
      const resp = middleware.preRequest(request) ?? (await h(request))
      return middleware.postRequest(resp)
    }
  }, handler)
}

interface Middleware {
  preRequest(req: Request): Response | undefined
  postRequest(req: Response): Response
}

export const app = { fetch: ittyServer }

[kwhitley]

Executing on a per-request basis absolutely does solve the race condition (which exists for origin-mirroring in a scenario where corsify does not have access to the original request).

That said:

• it's a bit less performant (less of an issue than the race-condition in the first place ofc)
• it's a bit verbose (this is my bigger issue with it)

I'm about to start work on a revamp with the following plans:

1. corsify will default to not mirror domains - everyone has expressed this is not the expected default behavior anyway, and is really mostly there to serve an edge case (cross-domain cookies, and the high-security crowd). This will allow us to only use the shared var (as a backup) in the case of mirroring, and only when the request was not provided to corsify... which leads me to:
2. corsify will be modified to accept (response, request). This will allow us to either use it as a drop-in finally stage handler, or more easily handle the mirror edge-case without having to execute createCors on a per-request basis (which is a bit heavy).

Example with stages (v4.3 Router/AutoRouter)

import { AutoRouter, createCors } from 'itty-router'

const { preflight, corsify } = createCors()

const router = AutoRouter({
  before: [preflight],
  finally: [corsify],
}).get('/', () => 'test')

export default router

Example without stages (using v4.3 IttyRouter)

import { Router, createCors, json, error } from 'itty-router'

const { preflight, corsify } = createCors()

const router = IttyRouter()
  .all('*', preflight)
  .get('/', () => 'test')

export default {
  fetch: (request, ...args) => router
    .fetch(request, ...args)
    .then(json)
    .catch(error)
    .then(response => corsify(response, request))
}

[kwhitley]

This is being closed as we believed it warranted a complete rewrite #226, and is thus forcing us to bump to a major release under v5.x.