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

v1 - Bring us up to spec 🎉 #51

Open
3 of 9 tasks
tim-phillips opened this issue Dec 5, 2018 · 9 comments
Open
3 of 9 tasks

v1 - Bring us up to spec 🎉 #51

tim-phillips opened this issue Dec 5, 2018 · 9 comments
Labels

Comments

@tim-phillips
Copy link
Collaborator

tim-phillips commented Dec 5, 2018

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

Dynamic Access-Control-Allow-Origin

Specification

Supplementary reading

cc @bukinoshita @infernalmaster @lemol

@tim-phillips tim-phillips added the v1 label Dec 5, 2018
@tim-phillips tim-phillips changed the title v1 - Bring us up to spec v1 - Bring us up to spec 🎉 Dec 5, 2018
@tim-phillips

This comment has been minimized.

@metamas
Copy link

metamas commented Sep 8, 2019

@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
Copy link
Collaborator Author

@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
Copy link

metamas commented Sep 10, 2019

@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
Copy link
Collaborator Author

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

@ramiel
Copy link

ramiel commented Sep 12, 2019

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

@metamas
Copy link

metamas commented Sep 14, 2019

@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
Copy link
Collaborator Author

@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
Copy link

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.

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

No branches or pull requests

4 participants