possibilities/micro-cors

v1 - Bring us up to spec 🎉

tim-phillips opened this issue · 9 comments

tim-phillips commented

In order to bring this library up to the CORS spec, we need to accomplish the following tasks. Please review the spec and add to this list!!

Simple Cross-Origin Request, Actual Request, and Redirects

  • If the Origin header in the request is not present, stop adding headers and run handler
  • If the Origin header in the request does not match exactly, stop adding headers and run handler
  • If allowCredentials is true, Set Access-Control-Allow-Origin to the value of the Origin header (is the client responsible for rejecting if Origin is *?)

Preflight Request

  • Return empty response and don't run the handler #48
  • Return 204 as empty response? Allow user to change response code?
  • If the Origin header in the request is not present, stop adding headers and return empty
  • If the Origin header in the request does not match exactly, stop adding headers and return empty

Dynamic Access-Control-Allow-Origin

  • Allow user to set multiple origins in config #53
  • Correctly use Vary header #58

Specification

Supplementary reading

cc @bukinoshita @infernalmaster @lemol

tim-phillips commented

EDIT: see #47 (comment)

What do you all think about the Vary header? Is that something we should handle or leave up to the user?

If the server specifies a single origin rather than the "*" wildcard, then the server should also include Origin in the Vary response header — to indicate to clients that server responses will differ based on the value of the Origin request header.

https://developer.mozilla.org/en-US/docs/Web/HTTP/CORS#Access-Control-Allow-Origin

Resources that wish to enable themselves to be shared with multiple Origins but do not respond uniformly with "*" must in practice generate the Access-Control-Allow-Origin header dynamically in response to every request they wish to allow. As a consequence, authors of such resources should send a Vary: Origin HTTP header or provide other appropriate control directives to prevent caching of such responses, which may be inaccurate if re-used across-origins.

https://www.w3.org/TR/cors/#resource-implementation

metamas commented

@tim-phillips Seems like ya'll lost steam on this. I'd like to help. I've recently started building a Next project and will need to handle some CORS stuff in my API Routes (which use micro). Spent a few days studying the MDN docs on CORS. Feel like I have a solid understanding of it all.

If I start working on some PRs, will you be available to review and help clarify anything I'm confused about?

tim-phillips commented

@metamas Yes!

We lost steam because the steps are not clear right now. We need to review the spec and create actionable issues to bring this lib to spec.

It can be useful before you create a PR to create an Issue to discuss what you're planning, or we can chat about it in this thread if you think that's a better place for it.

metamas commented

@tim-phillips Disregard what I wrote before editing this comment. I initially misunderstood your message.

So, what needs to happen is a thorough review of how the current state of the lib compares to the spec. Is that correct?

tim-phillips commented

@metamas Yes, precisely. This lib does work fine, it just doesn’t follow the spec exactly.

ramiel commented

I would propose the following:

  • If allow credentials is true
    • Set Access-Control-Allow-Origin to the value of Origin header (never *)
    • Set Access-Control-Allow-Credentials to true
  • If allow credentials is false
    • Set Access-Control-Allow-Origin with the value of Origin header or *

that partially covers part of the spec

  • 👍1
metamas commented

@tim-phillips I'll fork and approach this by simply reading through the spec. As I do so, I'll check that the project has tests for specified behavior. If it does, I'll check their validity. If it doesn't, I'll add tests and work on getting them to pass (if they don't already). I'll also return here to add to this list.

Ok. So that's my intention. Now, I just need to manage enough free-time to get it done. 😅

Any suggestions on my approach?

tim-phillips commented

@ramiel Added to OP. While allow credentials is true, do you know if the client/browser is responsible for rejecting when the Origin is *?

@metamas I like the test driven approach.

pudgereyem commented

I opened up a PR with some changes to the v1 branch that you can see here; #76

Appreciate anyone taking the time to look it through. Keen on getting v1 in a shape to get merged to master.