Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Fix/non preflight options #63

Open
wants to merge 4 commits into
base: master
Choose a base branch
from

Conversation

sripberger
Copy link

@sripberger sripberger commented Aug 8, 2019

Not all OPTIONS requests are CORS preflight requests from browsers. This middleware correctly checks to make sure that Access-Control-Request-Method is present before fully treating a request as a preflight-- however, it does so after skipping over any of the usual logic for "actual" requests.

The result is that access control headers will not be set for any OPTIONS requests, even though they will still continue into downstream middlewares like other "actual" requests:

cors/index.js

Lines 101 to 107 in 71c4d00

// If there is no Access-Control-Request-Method header or if parsing failed,
// do not set any additional headers and terminate this set of steps.
// The request is outside the scope of this specification.
if (!ctx.get('Access-Control-Request-Method')) {
// this not preflight request, ignore it
return await next();
}

This blocks any and all cross-origin OPTIONS requests that are not preflights, breaking features in other popular Koa middlewares-- such as the automatic 'allowed methods' middlewares from koa-router-- when accessed cross-origin.

I've fixed the problem by moving this check into the if statement that separates "actual" requests from preflight ones.

I've also removed some unnecessary await keywords, rationale explained here:

https://eslint.org/docs/rules/no-return-await

I can of course live without this second bit if you guys have some reason I'm not aware of for wanting to return await in these scenarios. Any performance change will be far below noticeable. I just figured I might as well, and can roll it back if I have to. 👍

Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant