search

cyu / rack-cors

Rack Middleware for handling Cross-Origin Resource Sharing (CORS), which makes cross-origin AJAX possible.
MIT License
3.26k stars 263 forks source link

Remove/Replace Rack::Utils::HeaderHash #251

Closed everton closed 1 year ago

everton commented 1 year ago

The Rack::Utils::HeaderHash was removed by Rack on the commit:

https://github.com/rack/rack/commit/a5762cf60af474b89b555ff8d151be5a021701f8#diff-85960cbd609e7f51a9881d1876b82d748f4ba8312a31e3329a5bb9bf791c60be

In the (previously existing) deprecation warning, they suggest "switch to Rack::Headers"; Testing it using this class apparently works, but I'm not sure if you feel it is the proper change since I don't know if the original motivation to use the HeaderHash still exists today.

everton commented 1 year ago

The original class was added in response to #218, but I'm not sure the original problem still exists since some efforts to downcase the header keys were already made.

Just removing the line with the code (replace Rack::Utils::HeaderHash.new(h) by return h in lib/rack/cors/resource.rb) apparently works (tested only locally and in a very simple case and no tests were broken).

Is this a possible solution, or are there other implications that should require using the Rack: :Headers class?

cyu commented 1 year ago

@everton thanks for the detailed report. It looks like the HeaderHash is no longer needed in rack 3 since it lower cases header keys on its own. I see two ways of fixing this:

  1. Upgrading the gemspec to require rack 3 and above, and remove the HeaderHash use
  2. Use a defined? check on HeaderHash and use it if defined. This way we can maintain rack 2 compatibility

Is there another possibility I'm missing?

everton commented 1 year ago

Just removing the class would not break the previous compatibility (your code didn't have this class until last year and it was working).

What would happen is that the problem described would be reintroduced, which sincerely seems to be ok, because if anyone had any problem with it they will have 2 options:

  1. Fix the rack-cors version while they can't upgrade the rack itself
  2. Upgrade the rack and use the latest version of both gems
cyu commented 1 year ago

@everton Ok, happy to take this change if you will create a PR.

everton commented 1 year ago

Added PR #252, where I just removed the Rack::Utils::HeaderHash.new, making it equal to the original code where the hash "h" was just returned without any transformation.

jeffwidman commented 1 year ago

Should this be closed now that #252 is merged?

everton commented 1 year ago

Sure, thanks for remembering this.