-
Notifications
You must be signed in to change notification settings - Fork 328
Recognize cookies set by redirect #712
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
Conversation
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.
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 |
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 |
Looks like some unrelated rubocop failures. I thought we had rubocop pinned to prevent that... |
My previous commit only carried cookies between redirect requests and not to the final response. This corrects that. All tests are now at the higher level, and the code change is smaller, overall.
I had previously thought it happened automatically, but I guess not.
@tarcieri This is ready now, as far as I'm concerned. Let me know if anything needs fixing |
@tkellogg some rubocop failures |
I think these are fixed by the rubocop update? |
There were two failures after merging #712 under the latest rubocop
Aah nope, oh well. #718 fixes them |
There were two failures after merging #712 under the latest rubocop
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. |
A |
Changelog: ## [5.3.1] - 2025-06-09 ### Changed - Revert switch to the native llhttp on MRI, as it's not compatible with standalone bundles ([#802](httprb/http#802)) ## [5.3.0] - 2025-06-09 ### Added - (backported) Add .retriable feature to Http - (backported) Add more specific ConnectionError classes - (backported) New feature: RaiseError ### Changed - (backported) Drop depenency on base64 - (backported) Cache header normalization to reduce object allocation - (backported) Use native llhttp on MRI ## [5.2.0] - 2024-02-05 ### Added - Add `Connection#finished_request?` ([#743](httprb/http#743)) - Add `Instrumentation#on_error` ([#746](httprb/http#746)) - Add `base64` dependency (suppresses warnings on Ruby 3.0) ([#759](httprb/http#759)) - Add `PURGE` HTTP verb ([#757](httprb/http#757)) - Add Ruby-3.3 support ### Changed - **BREAKING** Process features in reverse order ([#766](httprb/http#766)) - **BREAKING** Downcase Content-Type charset name ([#753](httprb/http#753)) - **BREAKING** Make URI normalization more conservative ([#758](httprb/http#758)) ### Fixed - Close sockets on initialize failure ([#762](httprb/http#762)) - Prevent CRLF injection due to broken URL normalizer ([#765](httprb/http#765)) [unreleased]: httprb/http@v5.3.0...5-x-stable [5.3.0]: httprb/http@v5.2.0...v5.3.0 [5.2.0]: httprb/http@v5.1.1...v5.2.0 ## 5.1.1 (2022-12-17) * [#731](httprb/http#731) Strip brackets from IPv6 addresses in `HTTP::URI`. ([@jeraki]) * [#722](httprb/http#722) Add `on_redirect` callback. ([@benubois]) ## 5.1.0 (2022-06-17) * Drop ruby-2.5 support. * [#715](httprb/http#715) Set default encoding to UTF-8 for `application/json`. ([@drwl]) * [#712](httprb/http#712) Recognize cookies set by redirect. ([@tkellogg]) * [#707](httprb/http#707) Distinguish connection timeouts. ([@YuLeven]) ## 5.0.4 (2021-10-07) * [#698](httprb/http#698) Fix `HTTP::Timeout::Global#connect_ssl`. ([@tarcieri]) ## 5.0.3 (2021-10-06) * [#695](httprb/http#695) Revert DNS resolving feature. ([@PhilCoggins]) * [#694](httprb/http#694) Fix cookies extraction. ([@flosacca]) ## 5.0.2 (2021-09-10) * [#686](httprb/http#686) Correctly reset the parser. ([@bryanp]) * [#684](httprb/http#684) Don't set Content-Length for GET, HEAD, DELETE, or CONNECT requests without a BODY. ([@jyn514]) * [#679](httprb/http#679) Use features on redirected requests. ([@nomis]) * [#678](httprb/http#678) Restore `HTTP::Response` `:uri` option for backwards compatibility. ([@schwern]) * [#676](httprb/http#676) Update addressable because of CVE-2021-32740. ([@matheussilvasantos]) * [#653](httprb/http#653) Avoid force encodings on frozen strings. ([@bvicenzo]) * [#638](httprb/http#638) DNS failover handling. ([@midnight-wonderer]) ## 5.0.1 (2021-06-26) * [#670](httprb/http#670) Revert `Response#parse` behavior introduced in [#540]. ([@DannyBen]) * [#669](httprb/http#669) Prevent bodies from being resubmitted when following unsafe redirects. ([@odinhb]) * [#664](httprb/http#664) Bump llhttp-ffi to 0.3.0. ([@bryanp]) ## 5.0.0 (2021-05-12) * [#656](httprb/http#656) Handle connection timeouts in `Features` ([@semenyukdmitry]) * [#651](httprb/http#651) Replace `http-parser` with `llhttp` ([@bryanp]) * [#647](httprb/http#647) Add support for `MKCALENDAR` HTTP verb ([@meanphil]) * [#632](httprb/http#632) Respect the SSL context's `verify_hostname` value ([@colemannugent]) * [#625](httprb/http#625) Fix inflator with empty responses ([@LukaszMaslej]) * [#599](httprb/http#599) Allow passing `HTTP::FormData::{Multipart,UrlEncoded}` object directly. ([@ixti]) * [#593](httprb/http#593) [#592](httprb/http#592) Support informational (1XX) responses. ([@ixti]) * [#590](httprb/http#590) [#589](httprb/http#589) Fix response headers paring. ([@Bonias]) * [#587](httprb/http#587) [#585](httprb/http#585) Fix redirections when server responds with multiple Location headers. ([@ixti]) * [#581](httprb/http#581) [#582](httprb/http#582) Add Ruby 2.7.x support. ([@janko]) * [#577](httprb/http#577) Fix `Chainable#timeout` with frozen Hash. ([@antonvolkoff]) * [#576](httprb/http#576) [#524](httprb/http#524) **BREAKING CHANGE** Preserve header names casing. ([@joshuaflanagan]) * [#540](httprb/http#540) [#538](httprb/http#538) **BREAKING CHANGE** Require explicit MIME type for Response#parse ([@ixti]) * [#532](httprb/http#532) Fix pipes support in request bodies. ([@ixti]) * [#530](httprb/http#530) Improve header fields name/value validation. ([@Bonias]) * [#506](httprb/http#506) [#521](httprb/http#521) Skip auto-deflate when there is no body. ([@Bonias]) * [#489](httprb/http#489) Fix HTTP parser. ([@ixti], [@fxposter]) * [#546](httprb/http#546) **BREAKING CHANGE** Provide initiating `HTTP::Request` object on `HTTP::Response`. ([@joshuaflanagan]) * [#571](httprb/http#571) Drop Ruby 2.3.x support. ([@ixti]) * [3ed0c31](httprb/http@3ed0c31) Drop Ruby 2.4.x support.
When a redirect response arrives, the browser will recognize
Set-Cookie
headers the same as any other request/response. This PRenables 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.