Skip to content

Commit 008c181

Browse files
sferiksferik
committed
Move cookie jar from Redirector to Session
Redirector is now a pure redirect-following engine: it handles URI resolution, verb changes, loop detection, max hops, and nothing else. Session#request creates a per-request CookieJar (preserving thread safety) and handles loading, storing, applying, and deleting cookies at each step of the redirect chain.
1 parent 0dd249f commit 008c181

7 files changed

Lines changed: 188 additions & 186 deletions

File tree

‎CHANGELOG.md‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -58,6 +58,9 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0
5858
`HTTP::CookieJar`. The `cookies` option has been removed from `Options`;
5959
`Chainable#cookies` now sets the `Cookie` header directly with no implicit
6060
merging — the last `.cookies()` call wins (#536)
61+
- Cookie jar management during redirects moved from `Redirector` to `Session`.
62+
`Redirector` is now a pure redirect-following engine with no cookie
63+
awareness; `Session#request` manages cookies across redirect hops
6164

6265
### Removed
6366

‎lib/http/redirector.rb‎

Lines changed: 0 additions & 50 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,5 @@
11
# frozen_string_literal: true
22

3-
require "http/cookie_jar"
43
require "http/headers"
54

65
module HTTP
@@ -73,8 +72,6 @@ def perform(request, response, &)
7372
@request = request
7473
@response = response
7574
@visited = []
76-
collect_cookies_from_request
77-
collect_cookies_from_response
7875

7976
follow_redirects(&) while REDIRECT_CODES.include?(@response.code)
8077

@@ -96,20 +93,8 @@ def follow_redirects
9693
@response.flush
9794

9895
@request = redirect_to(redirect_uri)
99-
apply_cookies_to_request
10096
@on_redirect&.call @response, @request
10197
@response = yield @request
102-
collect_cookies_from_response
103-
end
104-
105-
# Apply cookies to the current request
106-
#
107-
# @api private
108-
# @return [void]
109-
def apply_cookies_to_request
110-
return if cookie_jar.empty?
111-
112-
@request.headers.set(Headers::COOKIE, cookie_jar.map { |c| "#{c.name}=#{c.value}" }.join("; "))
11398
end
11499

115100
# Extracts the redirect URI from the Location header
@@ -121,41 +106,6 @@ def redirect_uri
121106
location.join unless location.empty?
122107
end
123108

124-
# Returns the cookie jar for tracking cookies
125-
#
126-
# @api private
127-
# @return [HTTP::CookieJar]
128-
def cookie_jar
129-
@cookie_jar ||= CookieJar.new
130-
end
131-
132-
# Collects cookies from the current request
133-
#
134-
# @api private
135-
# @return [void]
136-
def collect_cookies_from_request
137-
request_cookie_header = @request.headers["Cookie"]
138-
cookies = Cookie.cookie_value_to_hash(request_cookie_header.to_s)
139-
140-
cookies.each do |key, value|
141-
cookie_jar.add(Cookie.new(key, value, path: @request.uri.path, domain: @request.host))
142-
end
143-
end
144-
145-
# Carries cookies from response to the next request
146-
#
147-
# @api private
148-
# @return [void]
149-
def collect_cookies_from_response
150-
@response.cookies.each do |cookie|
151-
if cookie.value == ""
152-
cookie_jar.delete(cookie)
153-
else
154-
cookie_jar.add(cookie)
155-
end
156-
end
157-
end
158-
159109
# Check if we reached max amount of redirect hops
160110
#
161111
# @api private

‎lib/http/session.rb‎

Lines changed: 95 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -2,6 +2,10 @@
22

33
require "forwardable"
44

5+
require "http/cookie_jar"
6+
require "http/headers"
7+
require "http/redirector"
8+
59
module HTTP
610
# Thread-safe options builder for configuring HTTP requests.
711
#
@@ -50,6 +54,7 @@ def initialize(default_options = {})
5054
# Make an HTTP request by creating a new {Client}
5155
#
5256
# A fresh {Client} is created for each request, ensuring thread safety.
57+
# Manages cookies across redirect hops when following redirects.
5358
#
5459
# @example
5560
# session = HTTP::Session.new
@@ -61,7 +66,18 @@ def initialize(default_options = {})
6166
# @return [HTTP::Response] the response
6267
# @api public
6368
def request(verb, uri, opts = {})
64-
make_client(default_options).request(verb, uri, opts)
69+
cookie_jar = CookieJar.new
70+
client = make_client(default_options)
71+
merged = default_options.merge(opts)
72+
73+
req = client.build_request(verb, uri, opts)
74+
load_cookies(cookie_jar, req)
75+
res = client.perform(req, merged)
76+
store_cookies(cookie_jar, res)
77+
78+
return res unless merged.follow
79+
80+
perform_redirects(cookie_jar, client, req, res, merged)
6581
end
6682

6783
# Build an HTTP request without executing it
@@ -78,5 +94,83 @@ def request(verb, uri, opts = {})
7894
def build_request(verb, uri, opts = {})
7995
make_client(default_options).build_request(verb, uri, opts)
8096
end
97+
98+
private
99+
100+
# Follow redirects with cookie management
101+
#
102+
# @param jar [HTTP::CookieJar] the cookie jar
103+
# @param client [HTTP::Client] the client to perform requests
104+
# @param req [HTTP::Request] the original request
105+
# @param res [HTTP::Response] the initial redirect response
106+
# @param opts [HTTP::Options] the merged options
107+
# @return [HTTP::Response] the final non-redirect response
108+
# @api private
109+
def perform_redirects(jar, client, req, res, opts)
110+
Redirector.new(opts.follow).perform(req, res) do |redirect_req| # steep:ignore
111+
wrapped = wrap_redirect(redirect_req, opts)
112+
apply_cookies(jar, wrapped)
113+
response = client.perform(wrapped, opts)
114+
store_cookies(jar, response)
115+
response
116+
end
117+
end
118+
119+
# Load cookies from the request's Cookie header into the jar
120+
#
121+
# @param jar [HTTP::CookieJar] the cookie jar
122+
# @param request [HTTP::Request] the request
123+
# @return [void]
124+
# @api private
125+
def load_cookies(jar, request)
126+
header = request.headers[Headers::COOKIE]
127+
cookies = HTTP::Cookie.cookie_value_to_hash(header.to_s)
128+
129+
cookies.each do |name, value|
130+
jar.add(HTTP::Cookie.new(name, value, path: request.uri.path, domain: request.host))
131+
end
132+
end
133+
134+
# Store cookies from the response's Set-Cookie headers into the jar
135+
#
136+
# @param jar [HTTP::CookieJar] the cookie jar
137+
# @param response [HTTP::Response] the response
138+
# @return [void]
139+
# @api private
140+
def store_cookies(jar, response)
141+
response.cookies.each do |cookie|
142+
if cookie.value == ""
143+
jar.delete(cookie)
144+
else
145+
jar.add(cookie)
146+
end
147+
end
148+
end
149+
150+
# Apply cookies from the jar to the request's Cookie header
151+
#
152+
# @param jar [HTTP::CookieJar] the cookie jar
153+
# @param request [HTTP::Request] the request
154+
# @return [void]
155+
# @api private
156+
def apply_cookies(jar, request)
157+
if jar.empty?
158+
request.headers.delete(Headers::COOKIE)
159+
else
160+
request.headers.set(Headers::COOKIE, jar.map { |c| "#{c.name}=#{c.value}" }.join("; "))
161+
end
162+
end
163+
164+
# Apply feature middleware to a redirect request
165+
#
166+
# @param request [HTTP::Request] the redirect request
167+
# @param opts [HTTP::Options] the merged options
168+
# @return [HTTP::Request] the wrapped request
169+
# @api private
170+
def wrap_redirect(request, opts)
171+
opts.features.inject(request) do |req, (_name, feature)|
172+
feature.wrap_request(req)
173+
end
174+
end
81175
end
82176
end

‎sig/http.rbs‎

Lines changed: 8 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -117,6 +117,14 @@ module HTTP
117117
def request: (Symbol verb, untyped uri, ?untyped opts) -> Response
118118
def build_request: (Symbol verb, untyped uri, ?untyped opts) -> Request
119119
def persistent?: () -> bool
120+
121+
private
122+
123+
def perform_redirects: (untyped jar, Client client, Request req, Response res, Options opts) -> Response
124+
def load_cookies: (untyped jar, Request request) -> void
125+
def store_cookies: (untyped jar, Response response) -> void
126+
def apply_cookies: (untyped jar, Request request) -> void
127+
def wrap_redirect: (Request request, Options opts) -> Request
120128
end
121129

122130
class Headers
@@ -905,7 +913,6 @@ module HTTP
905913
@request: Request
906914
@response: Response
907915
@visited: Array[untyped]
908-
@cookie_jar: untyped
909916

910917
attr_reader strict: bool
911918
attr_reader max_hops: Integer
@@ -916,10 +923,6 @@ module HTTP
916923
private
917924

918925
def follow_redirects: () { (Request) -> Response } -> void
919-
def apply_cookies_to_request: () -> void
920-
def cookie_jar: () -> untyped
921-
def collect_cookies_from_request: () -> void
922-
def collect_cookies_from_response: () -> void
923926
def too_many_hops?: () -> bool
924927
def endless_loop?: () -> bool
925928
def redirect_uri: () -> String?

‎test/http/redirector_test.rb‎

Lines changed: 2 additions & 130 deletions
Original file line numberDiff line numberDiff line change
@@ -14,16 +14,8 @@ def simple_response(status, body = "", headers = {})
1414
)
1515
end
1616

17-
def redirect_response(status, location, set_cookie = {})
18-
res = simple_response status, "", "Location" => location
19-
set_cookie.each do |name, value|
20-
res.headers.add("Set-Cookie", "#{name}=#{value}; path=/; httponly; secure; SameSite=none; Secure")
21-
end
22-
res
23-
end
24-
25-
def cookie_jar_from(redirector)
26-
redirector.instance_variable_get(:@cookie_jar)
17+
def redirect_response(status, location)
18+
simple_response status, "", "Location" => location
2719
end
2820

2921
describe "#strict" do
@@ -114,68 +106,6 @@ def cookie_jar_from(redirector)
114106
assert_equal "http://example.com/123", res.to_s
115107
end
116108

117-
it "forwards accumulated cookies through redirect chain" do
118-
req = HTTP::Request.new verb: :head, uri: "http://example.com"
119-
hops = [
120-
redirect_response(301, "http://example.com/1", { "foo" => "42" }),
121-
redirect_response(301, "http://example.com/2", { "bar" => "53", "deleted" => "foo" }),
122-
redirect_response(301, "http://example.com/3", { "baz" => "64", "deleted" => "" }),
123-
redirect_response(301, "http://example.com/4", { "baz" => "65" }),
124-
simple_response(200, "bar")
125-
]
126-
127-
request_cookies = [
128-
{ "foo" => "42" },
129-
{ "foo" => "42", "bar" => "53", "deleted" => "foo" },
130-
{ "foo" => "42", "bar" => "53", "baz" => "64" },
131-
{ "foo" => "42", "bar" => "53", "baz" => "65" }
132-
]
133-
134-
res = redirector.perform(req, hops.shift) do |request|
135-
req_cookie = HTTP::Cookie.cookie_value_to_hash(request.headers["Cookie"] || "")
136-
137-
assert_equal request_cookies.shift, req_cookie
138-
hops.shift
139-
end
140-
141-
assert_equal "bar", res.to_s
142-
end
143-
144-
it "forwards original request cookies through redirect chain" do
145-
req = HTTP::Request.new verb: :head, uri: "http://example.com"
146-
req.headers.set("Cookie", "foo=42; deleted=baz")
147-
hops = [
148-
redirect_response(301, "http://example.com/1", { "bar" => "64", "deleted" => "" }),
149-
simple_response(200, "bar")
150-
]
151-
152-
request_cookies = [
153-
{ "foo" => "42", "bar" => "64" },
154-
{ "foo" => "42", "bar" => "64" }
155-
]
156-
157-
redirector.perform(req, hops.shift) do |request|
158-
req_cookie = HTTP::Cookie.cookie_value_to_hash(request.headers["Cookie"] || "")
159-
160-
assert_equal request_cookies.shift, req_cookie
161-
hops.shift
162-
end
163-
end
164-
165-
it "collects request cookies with correct path" do
166-
req = HTTP::Request.new verb: :head, uri: "http://example.com/some/path"
167-
req.headers.set("Cookie", "foo=bar")
168-
hops = [simple_response(200, "done")]
169-
170-
redirector.perform(req, redirect_response(301, "http://example.com/other")) do
171-
hops.shift
172-
end
173-
174-
cookie = cookie_jar_from(redirector).detect { |c| c.name == "foo" }
175-
176-
assert_equal "/some/path", cookie.path
177-
end
178-
179109
context "with on_redirect callback" do
180110
let(:options) do
181111
{
@@ -567,64 +497,6 @@ def empty_body
567497
end
568498
end
569499

570-
it "collects cookies from initial request headers" do
571-
req = HTTP::Request.new verb: :get, uri: "http://example.com"
572-
req.headers.set("Cookie", "initial=cookie")
573-
hops = [
574-
redirect_response(301, "http://example.com/1"),
575-
simple_response(200, "done")
576-
]
577-
578-
redirector.perform(req, hops.shift) do |request|
579-
cookie_header = request.headers["Cookie"]
580-
581-
assert_includes cookie_header, "initial=cookie"
582-
hops.shift
583-
end
584-
end
585-
586-
it "collects cookies from response Set-Cookie headers" do
587-
req = HTTP::Request.new verb: :get, uri: "http://example.com"
588-
hops = [
589-
redirect_response(301, "http://example.com/1", { "resp_cookie" => "value1" }),
590-
simple_response(200, "done")
591-
]
592-
593-
redirector.perform(req, hops.shift) do |request|
594-
cookie_header = request.headers["Cookie"]
595-
596-
assert_includes cookie_header, "resp_cookie=value1"
597-
hops.shift
598-
end
599-
end
600-
601-
it "deletes cookies with empty value during redirect" do
602-
req = HTTP::Request.new verb: :get, uri: "http://example.com"
603-
hops = [
604-
redirect_response(301, "http://example.com/1", { "mycookie" => "present" }),
605-
redirect_response(301, "http://example.com/2", { "mycookie" => "" }),
606-
simple_response(200, "done")
607-
]
608-
609-
redirector.perform(req, hops.shift) { hops.shift }
610-
cookie_names = cookie_jar_from(redirector).map(&:name)
611-
612-
refute_includes cookie_names, "mycookie"
613-
end
614-
615-
it "does not set Cookie header when cookie jar is empty" do
616-
req = HTTP::Request.new verb: :get, uri: "http://example.com"
617-
hops = [
618-
redirect_response(301, "http://example.com/1"),
619-
simple_response(200, "done")
620-
]
621-
622-
redirector.perform(req, hops.shift) do |request|
623-
assert_nil request.headers["Cookie"]
624-
hops.shift
625-
end
626-
end
627-
628500
context "with max_hops: 2 and an endless redirect loop" do
629501
let(:options) { { max_hops: 2 } }
630502

0 commit comments

Comments
 (0)