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

Extract CORS to a standalone extension #6309

Closed
4 tasks
hacksparrow opened this issue Sep 9, 2020 · 8 comments
Closed
4 tasks

Extract CORS to a standalone extension #6309

hacksparrow opened this issue Sep 9, 2020 · 8 comments

Comments

@hacksparrow
Copy link
Contributor

hacksparrow commented Sep 9, 2020

CORS should not be baked in into the framework, especially with defaults. Configuring CORS is a error-prone, complicated task, we should let users take 100% responsibility of setting it up themselves; and disabling CORS should simply mean "not using the CORS extension".

Acceptance criteria

  • Remove all CORS related code from the core framework
  • Create a new CORS extension from the removed code
  • Update app generator to use the new extension
  • Update relevant docs
@hacksparrow
Copy link
Contributor Author

Prompted by my review of #6306 and #6290.

@bajtos
Copy link
Member

bajtos commented Sep 10, 2020

Let's keep security label for security vulnerabilities only, to make them stand out from the crowd.

@bajtos
Copy link
Member

bajtos commented Sep 10, 2020

Let's start by discussing the user (app developer) experience. Here is my point of view:

As a LoopBack user building a new API server, I want to quickly scaffold a LoopBack project with the recommended settings and best practices in place. I want a reasonable CORS setup that will allow my API to be consumed by a variety of clients, including single-page web applications (HTML5 SPAs - React, Angular, Vue).

We should keep this in mind when discussing what changes to make.

For example, if we remove all CORS-related code from @loopback/rest, then we can still have CORS enabled in new projects by enabling the new extension in the project template, possibly with a CLI prompt to ask the user how they want to handle CORS.

Remove all CORS related code from the core framework

I'd like us to consider impact of such breaking change on our users.

It may be better to make these changes in phases, e.g.:

  • Phase 1 introduces the new extension but keeps existing CORS support for backwards compatibility, possibly with a deprecation warning.

  • Phase 2 removes CORS support from @loopback/rest. This should happen several months after Phase 1.

See Reacting versioning guidelines for inspiration: https://reactjs.org/docs/faq-versioning.html#breaking-changes

@achrinza
Copy link
Member

From the other thread, +1 for changing to safer CORS defaults.

However, CORS tends to be an important part of a REST API. So I'm not too sure if we should abstract it from @loopback/rest. This may open the gates for "separating for the sake of separating", which would further complicate LoopBack 4.

Since CORS is already managed by @loopback/rest, I would consider it already well-separated from the other parts of the framework.

@hacksparrow
Copy link
Contributor Author

So I'm not too sure if we should abstract it from @loopback/rest.

As long as there is an intuitive way to enable-disable it, and it is disabled by default (can be enabled in scaffolded apps) I am fine with it being in@loopback/rest.

@raymondfeng
Copy link
Contributor

IMO, CORS is designed to protect browsers instead of API servers. Non-browser API clients don't really care about CORS.

I'm leaning to leaving CORS in @loopback/rest and enabling it by default to avoid extra work for most cases. A lot of web applications now use public/private cloud APIs served from other domains. It's also a common practice to separate API endpoints from web servers. The default should support these use cases out of box and we allow developers to further constrain it if needed.

@stale
Copy link

stale bot commented Jul 14, 2021

This issue has been marked stale because it has not seen activity within six months. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository. This issue will be closed within 30 days of being stale.

@stale stale bot added the stale label Jul 14, 2021
@stale
Copy link

stale bot commented Aug 13, 2021

This issue has been closed due to continued inactivity. Thank you for your understanding. If you believe this to be in error, please contact one of the code owners, listed in the CODEOWNERS file at the top-level of this repository.

@stale stale bot closed this as completed Aug 13, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants