Rack::CORS middleware #59

Closed
wants to merge 1 commit into
from

Conversation

Projects
None yet
4 participants

Attached is some very simple middleware to allow Cross Origin Resource Sharing for a flexible list of domains. The list of domains that is passed as an argument to the middleware can contain the same kind of wildcards that shell globs can (underneath the hood, I use File.fnmatch? to check Origins against patterns).

Add Rack::CORS, middleware for flexibly enabling Cross Origin Resourc…
…e Sharing for a list of domains or domain patterns
+ # ['http://*.mysite.com'] -- allow all requests from any subdomain on localhost
+ def initialize(app, domain_patterns = [])
+ @app = app
+ @@domain_patterns = domain_patterns
@rkh

rkh Dec 10, 2012

Member

Could you not use a class variable here?

@RafeKettler

RafeKettler Dec 13, 2012

Yes. In practice, they'd be no different. I don't actually remember why I had it written this way in the first place, though. Feel free to change it

@jjb

jjb Nov 2, 2014

Contributor

@RafeKettler please rewrite this with an instance variable, thanks!

Contributor

mpalmer commented Oct 31, 2014

domain_patterns should be an instance variable, which can be fixed up on merge. Other than that, anything that makes CORS easier to deal with gets my vote. I'm especially keen on the presence of test cases.

@mpalmer mpalmer added needs-cleanup and removed needs-cleanup labels Oct 31, 2014

Contributor

jjb commented Nov 2, 2014

might be interesting to compare to the popular font_assets gem https://github.com/ericallam/font_assets/blob/master/lib/font_assets/middleware.rb

Contributor

jjb commented Nov 2, 2014

@ericallam could we get your feedback on this PR?

notably, this PR doesn't set

"Access-Control-Allow-Methods" => "GET",
"Access-Control-Allow-Headers" => "x-requested-with",
"Access-Control-Max-Age" => "3628800"

and maybe there are other best practices.

@mpalmer mpalmer added this to the 1.3 milestone Jun 22, 2015

@mpalmer mpalmer self-assigned this Jun 22, 2015

Contributor

mpalmer commented Jun 22, 2015

LGTM.

@mpalmer mpalmer removed the needs-cleanup label Jun 22, 2015

Contributor

jjb commented Jun 23, 2015

There are two very popular CORS gems, https://github.com/cyu/rack-cors and https://github.com/ericallam/font_assets. We should avoid being redundant with these.

rack-cors is actively developed. Does this PR do anything that rack-cors doesn't do? Maybe when this PR was submitted in 2012 rack-cors wasn't sufficient.

I would vote to close this and not merge it unless @RafeKettler or someone else knows that this code does something that rack-cors does not do (and even if that's the case, perhaps it would be more appropriate as a patch to rack-cors).

Contributor

mpalmer commented Jun 23, 2015

Good call, @jjb. I wasn't aware of the other available options.

I'll leave this open another couple of days, and if nobody comes up with a reason why this PR is needed, I'll drop it.

@mpalmer mpalmer removed this from the 1.3 milestone Jun 23, 2015

Contributor

mpalmer commented Jun 26, 2015

Given no objections, and having looked at the other available options and found them quite tasty, I'm closing this PR.

@mpalmer mpalmer closed this Jun 26, 2015

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment