-
Notifications
You must be signed in to change notification settings - Fork 260
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
Remove rack-jsonp for rack-cors #341
Comments
I did some quick research on this. Short version:
Long version: In this repo, searching for "jsonp" only returns this file: app/controllers/api/v1/base.rb The full search results: search results for "jsonp" ... which makes me think we are only using jsonp for accessing our API. Also, we already bring in so if we can manage to switch accessing the API to use only rack-cors, this issue would be fixed. We could also drop our dependency on rack-jsonp. (I'm not sure if switching jsonp to cors is a big deal, considering that we own the domain the API sits on, and the domain we make the requests from. If either got hacked hypothetically, I'm not sure CORS would help us, vs. jsonp. (Also I think hypothetically getting hacked would be a problem regardless of how we get at the API.) The benefits aren't super clear to me, other than just streamlining and modernizing our code and dependencies. Not sure if it's deep-down a technical win or just kind of neutral-impact. I hope CORS isn't slower, also, due to having to do more requests back and forth. But we'll see I guess.) FWIW CORS is well-supported by all modern browsers, for our purposes: http://caniuse.com/#feat=cors In any case, it still kinda seems like the right thing to do to move away from jsonp if we don't need it, and we're already using CORS, so why not only use CORS? |
I am tempted to try just removing everything jsonp-related from the project, test it and see if anything breaks. This should be tested in Opera Mini or something else really old, just to see if older browsers will have issues. |
@DeeDeeG did you ever code this up, or should I give it a try? |
Nope, I never got to it, so if you want to, go ahead! Noting that the API uses either JSONP or CORS, so the thing I expect to break would be the API. I think that ALSO means that the API reference page should kinda break, meaning that (I assume) testing the API reference page should be the easiest way to confirm this doesn't break anything. |
By the way, as far as I can see this is a really simple issue. Steps:
|
I tried this out here: https://github.com/DeeDeeG/refugerestrooms/tree/no-rack-jsonp, but there's still some stuff to fix. This does currently break the api page at Also, I noticed this file is the only thing left in my branch with the word "jsonp" anywhere in it: public/api/docs/lib/jquery-1.8.0.min.js It has jsonp mentioned a bunch of times. Wonder if find+replace More notes:
|
Please see the following comment #307 (comment)
After we have updated RR to Rails 5, we should remove our dep on
rack-jsonp
and replace withrack-cors
Reasoning:
TLDR from @DeeDeeG:
I read on Wikipedia that jsonp is insecure, and is being replaced by CORS (link to wiki article). Can we use rack-cors (link) instead? It's been updated as recently as February this year, as opposed to rack-jsonp, which was updated in 2014.
The text was updated successfully, but these errors were encountered: