search

RefugeRestrooms / refugerestrooms

REFUGE restrooms indexes and maps safe restroom locations for trans, intersex, and gender nonconforming individuals.
http://www.refugerestrooms.org
GNU Affero General Public License v3.0
892 stars 262 forks source link

Remove rack-jsonp for rack-cors #341

Open dannypaz opened 7 years ago

dannypaz commented 7 years ago

Please see the following comment https://github.com/RefugeRestrooms/refugerestrooms/pull/307#issuecomment-299679611

After we have updated RR to Rails 5, we should remove our dep on rack-jsonp and replace with rack-cors

Reasoning:

  1. More Secure
  2. Still relatively maintained (Feb of this year vs. 3 years ago

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.

DeeDeeG commented 7 years ago

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 rack-cors and use it in a few places. Also only to access the API. See these search results in our repo: search results for "cors"

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?

DeeDeeG commented 7 years ago

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.

mi-wood commented 6 years ago

@DeeDeeG did you ever code this up, or should I give it a try?

DeeDeeG commented 6 years ago

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.

DeeDeeG commented 6 years ago

By the way, as far as I can see this is a really simple issue.

Steps:

1) Remove rack-jsonp gem from the Gemfile 2) Deal with any errors (eg when starting rails, or when navigating the site) (I don't expect there will be any errors, but you never know.) 3) Check that nothing broke

DeeDeeG commented 6 years ago

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 127.0.0.1:3000/api/docs.

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 jsonp -> cors would fix it? (Probably not though.)

More notes: