search

httprb / http

HTTP (The Gem! a.k.a. http.rb) - a fast Ruby HTTP client with a chainable API, streaming support, and timeouts
MIT License
3.01k stars 321 forks source link

Recognize cookies set by redirect #712

Closed tkellogg closed 2 years ago

tkellogg commented 2 years ago

When a redirect response arrives, the browser will recognize Set-Cookie headers the same as any other request/response. This PR enables the Redirector to correctly follow the cookie hand-offs.

I encountered this situation out in the wild. This PR, as-is, seems to fix the issue. I haven't bothered to find the relevant spec.

tkellogg commented 2 years ago

Ah, there's still a problem with this — the cookies set by redirects still aren't set in the final response object. Gonna need a bit more work

tarcieri commented 2 years ago

A bit of sleuthing shows this behavior is implemented by all major browsers, so it seems like we should support it: https://blog.dubbelboer.com/2012/11/25/302-cookie.html

tarcieri commented 2 years ago

Looks like some unrelated rubocop failures. I thought we had rubocop pinned to prevent that...

tkellogg commented 2 years ago

@tarcieri This is ready now, as far as I'm concerned. Let me know if anything needs fixing

tarcieri commented 2 years ago

@tkellogg some rubocop failures

tarcieri commented 2 years ago

I think these are fixed by the rubocop update?

tarcieri commented 2 years ago

Aah nope, oh well. #718 fixes them

ixti commented 2 years ago

I have some plans of removing redirector as is, and introducing session object instead. Session will be a http client wrapper that will hold cookies. This PR has these lines:

      # I wish we could just do @response.cookes = cookie_jar
      cookie_jar.each do |cookie|
        @response.cookies.add(cookie)
      end

I believe this is wrong and comes from the idea of using redirector as a session. The response should not hold cookie jar of previous requests.

tarcieri commented 2 years ago

A Session type to encapsulate this state would be great. See also #306.