hanami / controller

Complete, fast and testable actions for Rack and Hanami
http://hanamirb.org
MIT License
246 stars 111 forks source link

Badly encoded cookie strings from Safari results in an error #414

Closed pat closed 1 year ago

pat commented 1 year ago

I've hit a bit of an edge-case: we're integrating with a third-party that adds their own cookies to our site, and those cookies can contain user data. If that user-entered data includes non-ASCII characters (e.g. "brûlée"), then when that cookie data is passed along, some browsers mangle the accented characters. Safari is definitely screwing this up, but Chrome is fine.

When it comes to parsing the values on the server, Rack has a string in env["HTTP_COOKIE"] that includes "br\xFBl\xE9e", yet says its encoding is ASCII (but should be ISO-8859-1).

The issue as it relates to hanami-controller? Cookie keys are converted to symbols, and because the encoding is incorrect, I get an exception when the response is being put together. (I'm not using these cookies myself, otherwise I suspect the error would crop up sooner).

EncodingError: invalid symbol in encoding UTF-8

vendor/bundle/ruby/3.2.0/gems/dry-transformer-1.0.1/lib/dry/transformer/hash_transformations.rb:67 to_sym
vendor/bundle/ruby/3.2.0/gems/dry-transformer-1.0.1/lib/dry/transformer/hash_transformations.rb:67 block in deep_symbolize_keys
vendor/bundle/ruby/3.2.0/gems/dry-transformer-1.0.1/lib/dry/transformer/hash_transformations.rb:66 each
vendor/bundle/ruby/3.2.0/gems/dry-transformer-1.0.1/lib/dry/transformer/hash_transformations.rb:66 each_with_object
vendor/bundle/ruby/3.2.0/gems/dry-transformer-1.0.1/lib/dry/transformer/hash_transformations.rb:66 deep_symbolize_keys
vendor/bundle/ruby/3.2.0/gems/dry-transformer-1.0.1/lib/dry/transformer/function.rb:48 call
vendor/bundle/ruby/3.2.0/gems/dry-transformer-1.0.1/lib/dry/transformer/function.rb:48 call
vendor/bundle/ruby/3.2.0/gems/hanami-utils-2.0.2/lib/hanami/utils/hash.rb:54 deep_symbolize
vendor/bundle/ruby/3.2.0/gems/hanami-controller-2.0.1/lib/hanami/action/cookie_jar.rb:30 initialize
vendor/bundle/ruby/3.2.0/gems/hanami-controller-2.0.1/lib/hanami/action/response.rb:216 new
vendor/bundle/ruby/3.2.0/gems/hanami-controller-2.0.1/lib/hanami/action/response.rb:216 cookies
vendor/bundle/ruby/3.2.0/gems/hanami-controller-2.0.1/lib/hanami/action/cookies.rb:22 finish
vendor/bundle/ruby/3.2.0/gems/hanami-controller-2.0.1/lib/hanami/action.rb:332 call
vendor/bundle/ruby/3.2.0/gems/hanami-2.0.2/lib/hanami/slice/routing/resolver.rb:78 block in resolve_slice_action
vendor/bundle/ruby/3.2.0/gems/hanami-router-2.0.2/lib/hanami/router.rb:108 call
vendor/bundle/ruby/3.2.0/gems/dry-monitor-1.0.1/lib/dry/monitor/rack/middleware.rb:36 block in call
vendor/bundle/ruby/3.2.0/gems/dry-monitor-1.0.1/lib/dry/monitor/clock.rb:15 measure
vendor/bundle/ruby/3.2.0/gems/dry-monitor-1.0.1/lib/dry/monitor/rack/middleware.rb:36 call
vendor/bundle/ruby/3.2.0/gems/hanami-router-2.0.2/lib/hanami/middleware/app.rb:40 call

My workaround has been to fix env["COOKIE_DATA"] to be UTF-8 via a middleware app (see below), and then everything's fine, but perhaps hanami-controller should handle these situations more gracefully, and not raise an exception? 🤔 I realise it's not hanami-controller's fault that the cookie data is bad, mind you, but I suspect a fix here is easier than a fix in Safari. 😅

class CleanCookies
  def initialize(app)
    @app = app
  end

  def call(env)
    env["HTTP_COOKIE"] = reencode(env["HTTP_COOKIE"])

    app.call(env)
  end

  private

  attr_reader :app

  def reencode(string)
    return nil if string.nil?
    return string if string.encoding == Encoding::UTF_8

    # if string is ASCII then this may fail, so we fall back to ISO-8859-1:
    string.encode(Encoding::UTF_8)
  rescue Encoding::UndefinedConversionError
    string.encode(Encoding::UTF_8, Encoding::ISO_8859_1)
  end
end
jodosha commented 1 year ago

@pat I'm wondering if Rack should handle this situation:

::Rack::Utils.parse_query(string, COOKIE_SEPARATOR)
pat commented 1 year ago

@jodosha to be honest, I'm really not sure where the best place to handle it is - should it be Rack? or Puma? Or reported to Apple's Safari team? Or instead, should cookies be URL-encoded to ensure it's not a problem in the first place?

Maybe it's best to just close this issue? I'm going to talk to the third-party that's generating the cookies, and perhaps anyone who finds the same problem crop up will come across this post and potentially be aided by my middleware. 🤷🏻‍♂️

jodosha commented 1 year ago

@pat Thank you.