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

Make CORS a core plugin #1817

Open
3 tasks done
sergejostir opened this issue Feb 26, 2020 · 13 comments
Open
3 tasks done

Make CORS a core plugin #1817

sergejostir opened this issue Feb 26, 2020 · 13 comments

Comments

@sergejostir
Copy link

  • Used appropriate template for the issue type
  • Searched both open and closed issues for duplicates of this issue
  • Title adequately and concisely reflects the feature or the bug

Feature Request

Use Case

Why do you want this?

There is a restify-cors-middleware package (which is recommended by restify documentation), but unfortunately it has been abandoned for the last 2 years. Considering that the CORS is an important aspect of today's web, I think it would be justified to make it a core plugin. For comparison, Fastify has it as a core plugin.

Example API

This should include code snippets and documentation for the proposed feature

Please see https://github.com/Tabcorp/restify-cors-middleware#readme

Are you willing and able to implement this?

"Yes" or, if "no", what can current contributors do to help you create a PR?

Unfortunately not at this point.

@retrohacker
Copy link
Member

retrohacker commented Mar 12, 2020

Heya @sergiz. We spun the CORS module out of our core plugins ~2 years ago in an attempt to improve the quality of that code base. At the time, it hadn't really had active development on it for an extended period of time, and some active contributors came along eager to work on it. The module improved a lot in that time.

It seems that progress has stalled on that plugin in the last two years but if you look at the commit history for restify's core plugins you'll see they haven't been very active since that was spun out two years ago either.

If the module having a "last updated 2 years ago" is the problem (i.e. a perception issue) we could fix that by pulling it into core so the lack of maintenance isn't on display. But the root problem is that plugin (and many others) just don't get much love.

@drwharris
Copy link

The issue is that that module has a dependancy on an older version of restify so it spits errors out. Ive submitted a pull request to update the dependancies and update the dependancy to retify 8 (Tabcorp/restify-cors-middleware#74) but its sat there unanswered for months. Those developers are no longer maintaining the repo which affectively means there is not actively supported CORS for restify. its not a great looks and has me just about ready to remove restify and replace it with express. They are also wanting to support Node < 8.

@retrohacker
Copy link
Member

retrohacker commented Mar 12, 2020

Ah I didn't realize the module was broken. We can pull it back into this repo to get back into a usable state. If someone wants to open a PR bringing it back into the core modules I'll gladly merge it.

FWIW, Fastify and Express are great projects and have a more active community. Restify is very much focused on stability and most of the work that goes into it is stuff that directly impacts maintainers' deployments of this project.

@nagad814
Copy link

So Restify has no CROS implementation as of this moment and is it recommended to use other servers instead of Restify. PS: why was it removed in the first place?

@retrohacker
Copy link
Member

@nagad814
Copy link

nagad814 commented Sep 28, 2020

thank you, I understand my way forward. @retrohacker you said in the above link : will bring it back the plugin if it is required. What does constitute as "if required".

@retrohacker
Copy link
Member

Basically now. If you open a PR for bringing CORS back into the repo, with passing tests, we can review it.

#1817 (comment)

Though note: The current maintainers aren't using CORS in our deployments of restify. Judging by the current, broken, state of CORS in this project, I suspect you will be the only active user deploying restify w/ CORS. If you like the restify project and are willing to personally invest in keeping CORS working for your use case, this might make sense for you. There are many web frameworks that provide CORS support. I suspect they will have a larger community supporting your use case.

@deguich
Copy link

deguich commented Sep 29, 2020

I suspect you will be the only active user deploying restify w/ CORS

Strange comment, cors is a mandatory module nowaday when developping a production rest server with authentication.

I had the same problem and tried several cors module. I used the expressjs cors middleware for restify projects :
https://github.com/expressjs/cors

Example for an accept all origins rest server :

import corsMiddleware from 'cors';
const cors = corsMiddleware({
  credentials: true,
  preflightMaxAge: 5,
  origin: function (origin, callback) {
      callback(null, origin)
  }
});
server.pre(cors);

My projects are not in production yet, so I'm not sure about the full compatibility of this cors module. But It could be good to work on restify compatibility and documentation with a well maintain cors module like espressjs/cors. It seems better to me than developing a new one.

That was just another opinion ...

Thanks for your work

@drwharris
Copy link

I suspect you will be the only active user deploying restify w/ CORS

Strange comment, cors is a mandatory module nowaday when developping a production rest server with authentication.

I had the same problem and tried several cors module. I used the expressjs cors middleware for restify projects :
https://github.com/expressjs/cors

Example for an accept all origins rest server :

import corsMiddleware from 'cors';
const cors = corsMiddleware({
  credentials: true,
  preflightMaxAge: 5,
  origin: function (origin, callback) {
      callback(null, origin)
  }
});
server.pre(cors);

My projects are not in production yet, so I'm not sure about the full compatibility of this cors module. But It could be good to work on restify compatibility and documentation with a well maintain cors module like espressjs/cors. It seems better to me than developing a new one.

That was just another opinion ...

Thanks for your work

You are 100% correct and the responses recently posted are the reason I have moved on. Fantastic, lightweight, robust service but without CORS as part of core. That's just insanity.

@sergejostir
Copy link
Author

sergejostir commented Sep 29, 2020

I suspect you will be the only active user deploying restify w/ CORS

Strange comment, cors is a mandatory module nowaday when developping a production rest server with authentication.
I had the same problem and tried several cors module. I used the expressjs cors middleware for restify projects :
https://github.com/expressjs/cors
Example for an accept all origins rest server :

import corsMiddleware from 'cors';
const cors = corsMiddleware({
  credentials: true,
  preflightMaxAge: 5,
  origin: function (origin, callback) {
      callback(null, origin)
  }
});
server.pre(cors);

My projects are not in production yet, so I'm not sure about the full compatibility of this cors module. But It could be good to work on restify compatibility and documentation with a well maintain cors module like espressjs/cors. It seems better to me than developing a new one.
That was just another opinion ...
Thanks for your work

You are 100% correct and the responses recently posted are the reason I have moved on. Fantastic, lightweight, robust service but without CORS as part of core. That's just insanity.

I completely understand them. Restify is primarly made for use of its maintainers and it's not the main goal to suit as wide audience as possible. However, they are willing to add new stuff that they don't need, if someone is willing to maintain it.

I have moved to an alternative recently, but for anyone wondering, the cors package out there (https://github.com/Tabcorp/restify-cors-middleware) is still working, it's just throwing a warning on install since the max version it officially supports is restify 7.x.

@retrohacker
Copy link
Member

Thanks @sergiz ❤️

You're 💯% spot on. Our use of restify is entirely internal IPC as far as I know. Any communication with the outside world happens via proxy and authentication is handled out of process. CORS is not something our architecture calls for. The module suffered from bit rot inside our code base as a result.

For those who need CORS, Tabcorp's module is a good implementation, albeit stale. A little elbow grease and you will be up and running. If you want to contribute that work back to the project, we would be happy to take the time to review it. But, without a community of active contributors using that code path, we are likely to repeat this cycle every few years.

@cekvenich2
Copy link

So what is the status of CORS now?

@carycodes
Copy link

As someone who ran into this problem recently, I can answer the "what's the status now" question.

restify still has no built-in CORS middleware. It is not a priority for the maintainers, because they do not personally use/need it.

restify-cors-middleware has now been abandoned for 5+ years.

Having said that, the CORS spec hasn't meaningfully changed in the last 5 years, and the restify-cors-middleware does appear to still be working--the problem is that it explicitly requires a peer dependency of restify no later than version 7 (which was current at the last time the middleware library got an update). Simply forking the middleware and removing that peer dependency requirement should give you a working CORS middleware.

More generally, the maintainers above have made it pretty clear that restify is a fairly targeted library for the things they need to do. The CORS issue itself is surmountable, but if you are creating a web service for which CORS is necessary, you are far enough outside the intended usage of this library that you should perhaps consider an alternative framework.

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

No branches or pull requests

7 participants