Skip to content

Commit c7d0d68

Browse files
benoitcbenoitc
authored
Merge pull request #801 from benoitc/fix/redirect-url-resolution
fix: properly resolve relative redirect URLs per RFC 3986
2 parents 8b8b79e + e28e097 commit c7d0d68

2 files changed

Lines changed: 319 additions & 6 deletions

File tree

‎src/hackney.erl‎

Lines changed: 57 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -966,15 +966,66 @@ resolve_redirect_url(CurrentURL, Location) when is_binary(Location) ->
966966
case Location of
967967
<<"http://", _/binary>> -> hackney_url:parse_url(Location);
968968
<<"https://", _/binary>> -> hackney_url:parse_url(Location);
969+
<<"ws://", _/binary>> -> hackney_url:parse_url(Location);
970+
<<"wss://", _/binary>> -> hackney_url:parse_url(Location);
971+
<<"//", _/binary>> ->
972+
%% Network-path reference (RFC 3986 Section 4.2)
973+
%% Use the current scheme with the new authority and path
974+
Scheme = CurrentURL#hackney_url.scheme,
975+
SchemePrefix = atom_to_binary(Scheme, utf8),
976+
hackney_url:parse_url(<<SchemePrefix/binary, ":", Location/binary>>);
969977
<<"/", _/binary>> ->
970-
%% Relative path - use current host
971-
CurrentURL#hackney_url{path = Location, qs = <<>>};
978+
%% Absolute-path reference - parse query string from Location
979+
{Path, Qs} = parse_path_qs(Location),
980+
CurrentURL#hackney_url{path = Path, qs = Qs};
972981
_ ->
973-
%% Relative path without leading slash
982+
%% Relative-path reference (RFC 3986 Section 5.2.3)
983+
%% Parse query string from Location first
984+
{RelPath, Qs} = parse_path_qs(Location),
974985
CurrentPath = CurrentURL#hackney_url.path,
975-
BasePath = filename:dirname(binary_to_list(CurrentPath)),
976-
NewPath = iolist_to_binary([BasePath, "/", Location]),
977-
CurrentURL#hackney_url{path = NewPath, qs = <<>>}
986+
NewPath = merge_paths(CurrentPath, RelPath),
987+
CurrentURL#hackney_url{path = NewPath, qs = Qs}
988+
end.
989+
990+
%% @private Parse path and query string from a path-like string
991+
parse_path_qs(PathLike) ->
992+
case binary:split(PathLike, <<"?">>) of
993+
[Path] -> {Path, <<>>};
994+
[Path, Qs] -> {Path, Qs}
995+
end.
996+
997+
%% @private Merge a relative path with a base path per RFC 3986 Section 5.2.3
998+
merge_paths(BasePath, RelPath) when is_binary(BasePath), is_binary(RelPath) ->
999+
case BasePath of
1000+
<<>> ->
1001+
%% Empty base path - prepend /
1002+
<<"/", RelPath/binary>>;
1003+
<<"/">> ->
1004+
%% Root path - just prepend /
1005+
<<"/", RelPath/binary>>;
1006+
_ ->
1007+
%% Get directory part of base path (everything up to and including last /)
1008+
BaseDir = base_directory(BasePath),
1009+
iolist_to_binary([BaseDir, RelPath])
1010+
end.
1011+
1012+
%% @private Get the directory part of a path (up to and including last /)
1013+
%% Example: "/a/b/c" -> "/a/b/", "/a/b/" -> "/a/b/", "/" -> "/", "" -> ""
1014+
base_directory(Path) ->
1015+
case find_last_slash(Path) of
1016+
-1 -> <<>>;
1017+
Pos -> binary:part(Path, 0, Pos + 1)
1018+
end.
1019+
1020+
%% @private Find the position of the last "/" in a binary
1021+
find_last_slash(Bin) ->
1022+
find_last_slash(Bin, byte_size(Bin) - 1).
1023+
1024+
find_last_slash(_Bin, Pos) when Pos < 0 -> -1;
1025+
find_last_slash(Bin, Pos) ->
1026+
case binary:at(Bin, Pos) of
1027+
$/ -> Pos;
1028+
_ -> find_last_slash(Bin, Pos - 1)
9781029
end.
9791030

9801031
async_request(ConnPid, Method, Path, Headers, Body, AsyncMode, StreamTo, FollowRedirect) ->

‎test/hackney_redirect_tests.erl‎

Lines changed: 262 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,262 @@
1+
%% @doc Tests for redirect URL resolution (issues #711, #693, #568, #617)
2+
-module(hackney_redirect_tests).
3+
-include_lib("eunit/include/eunit.hrl").
4+
-include("hackney_lib.hrl").
5+
6+
-define(PORT, 9877).
7+
8+
%% Setup/teardown for integration tests
9+
setup() ->
10+
%% Use ensure_all_started for proper dependency handling
11+
{ok, _} = application:ensure_all_started(hackney),
12+
{ok, _} = application:ensure_all_started(cowboy),
13+
%% Start the test HTTP server
14+
Dispatch = cowboy_router:compile([{'_', [{"/[...]", test_http_resource, []}]}]),
15+
{ok, _} = cowboy:start_clear(test_redirect_http, [{port, ?PORT}], #{
16+
env => #{dispatch => Dispatch}
17+
}),
18+
ok.
19+
20+
cleanup(_) ->
21+
cowboy:stop_listener(test_redirect_http),
22+
ok.
23+
24+
url(Path) ->
25+
<<"http://localhost:", (integer_to_binary(?PORT))/binary, Path/binary>>.
26+
27+
%% =============================================================================
28+
%% Unit tests for resolve_redirect_url logic
29+
%% =============================================================================
30+
31+
%% Test RFC 3986 Section 5.4 relative reference resolution
32+
%% Base: http://a/b/c/d;p?q
33+
34+
%% Issue #711: Relative paths without leading slash
35+
resolve_relative_path_test_() ->
36+
%% Base URL similar to Wikipedia example
37+
BaseURL = hackney_url:parse_url(<<"https://example.com/api/page/summary/coffee">>),
38+
39+
[
40+
%% Test 1: Simple relative path should replace last segment
41+
{"relative path replaces last segment",
42+
fun() ->
43+
Location = <<"Coffee">>,
44+
Result = resolve_redirect_url(BaseURL, Location),
45+
?assertEqual(<<"/api/page/summary/Coffee">>, Result#hackney_url.path)
46+
end},
47+
48+
%% Test 2: Relative path with query string
49+
{"relative path with query string",
50+
fun() ->
51+
Location = <<"newpage?foo=bar">>,
52+
Result = resolve_redirect_url(BaseURL, Location),
53+
%% Fixed: query string is now parsed from Location
54+
?assertEqual(<<"/api/page/summary/newpage">>, Result#hackney_url.path),
55+
?assertEqual(<<"foo=bar">>, Result#hackney_url.qs)
56+
end},
57+
58+
%% Test 3: Absolute path should work
59+
{"absolute path replaces entire path",
60+
fun() ->
61+
Location = <<"/new/absolute/path">>,
62+
Result = resolve_redirect_url(BaseURL, Location),
63+
?assertEqual(<<"/new/absolute/path">>, Result#hackney_url.path)
64+
end},
65+
66+
%% Test 4: Absolute URL should be parsed completely
67+
{"absolute URL is fully parsed",
68+
fun() ->
69+
Location = <<"https://other.com/different/path">>,
70+
Result = resolve_redirect_url(BaseURL, Location),
71+
?assertEqual("other.com", Result#hackney_url.host),
72+
?assertEqual(<<"/different/path">>, Result#hackney_url.path)
73+
end},
74+
75+
%% Test 5: Network-path reference (//host/path)
76+
{"network-path reference preserves scheme",
77+
fun() ->
78+
Location = <<"//other.com/new/path">>,
79+
Result = resolve_redirect_url(BaseURL, Location),
80+
?assertEqual("other.com", Result#hackney_url.host),
81+
?assertEqual(<<"/new/path">>, Result#hackney_url.path),
82+
?assertEqual(https, Result#hackney_url.scheme)
83+
end},
84+
85+
%% Test 6: Absolute path with query string
86+
{"absolute path with query string",
87+
fun() ->
88+
Location = <<"/new/path?query=value">>,
89+
Result = resolve_redirect_url(BaseURL, Location),
90+
?assertEqual(<<"/new/path">>, Result#hackney_url.path),
91+
?assertEqual(<<"query=value">>, Result#hackney_url.qs)
92+
end}
93+
].
94+
95+
%% Test handling of trailing slashes in base path
96+
trailing_slash_test_() ->
97+
BaseURL = hackney_url:parse_url(<<"https://example.com/a/b/">>),
98+
[
99+
{"trailing slash: relative path appends to directory",
100+
fun() ->
101+
Location = <<"c">>,
102+
Result = resolve_redirect_url(BaseURL, Location),
103+
%% With trailing slash, should append: /a/b/ + c = /a/b/c
104+
?assertEqual(<<"/a/b/c">>, Result#hackney_url.path)
105+
end}
106+
].
107+
108+
%% Test for empty/root paths
109+
empty_path_test_() ->
110+
BaseURL = hackney_url:parse_url(<<"https://example.com">>),
111+
BaseURLSlash = hackney_url:parse_url(<<"https://example.com/">>),
112+
[
113+
{"empty base path",
114+
fun() ->
115+
Location = <<"page">>,
116+
Result = resolve_redirect_url(BaseURL, Location),
117+
%% Fixed: empty base path now prepends /
118+
?assertEqual(<<"/page">>, Result#hackney_url.path)
119+
end},
120+
{"root base path",
121+
fun() ->
122+
Location = <<"page">>,
123+
Result = resolve_redirect_url(BaseURLSlash, Location),
124+
?assertEqual(<<"/page">>, Result#hackney_url.path)
125+
end}
126+
].
127+
128+
%% Helper function that mirrors hackney.erl's resolve_redirect_url
129+
resolve_redirect_url(CurrentURL, Location) when is_binary(Location) ->
130+
case Location of
131+
<<"http://", _/binary>> -> hackney_url:parse_url(Location);
132+
<<"https://", _/binary>> -> hackney_url:parse_url(Location);
133+
<<"//", _/binary>> ->
134+
Scheme = CurrentURL#hackney_url.scheme,
135+
SchemePrefix = atom_to_binary(Scheme, utf8),
136+
hackney_url:parse_url(<<SchemePrefix/binary, ":", Location/binary>>);
137+
<<"/", _/binary>> ->
138+
{Path, Qs} = parse_path_qs(Location),
139+
CurrentURL#hackney_url{path = Path, qs = Qs};
140+
_ ->
141+
{RelPath, Qs} = parse_path_qs(Location),
142+
CurrentPath = CurrentURL#hackney_url.path,
143+
NewPath = merge_paths(CurrentPath, RelPath),
144+
CurrentURL#hackney_url{path = NewPath, qs = Qs}
145+
end.
146+
147+
parse_path_qs(PathLike) ->
148+
case binary:split(PathLike, <<"?">>) of
149+
[Path] -> {Path, <<>>};
150+
[Path, Qs] -> {Path, Qs}
151+
end.
152+
153+
merge_paths(BasePath, RelPath) when is_binary(BasePath), is_binary(RelPath) ->
154+
case BasePath of
155+
<<>> -> <<"/", RelPath/binary>>;
156+
<<"/">> -> <<"/", RelPath/binary>>;
157+
_ ->
158+
BaseDir = base_directory(BasePath),
159+
iolist_to_binary([BaseDir, RelPath])
160+
end.
161+
162+
base_directory(Path) ->
163+
case find_last_slash(Path) of
164+
-1 -> <<>>;
165+
Pos -> binary:part(Path, 0, Pos + 1)
166+
end.
167+
168+
find_last_slash(Bin) ->
169+
find_last_slash(Bin, byte_size(Bin) - 1).
170+
171+
find_last_slash(_Bin, Pos) when Pos < 0 -> -1;
172+
find_last_slash(Bin, Pos) ->
173+
case binary:at(Bin, Pos) of
174+
$/ -> Pos;
175+
_ -> find_last_slash(Bin, Pos - 1)
176+
end.
177+
178+
%% =============================================================================
179+
%% Integration tests for redirect following
180+
%% =============================================================================
181+
182+
redirect_integration_test_() ->
183+
{setup,
184+
fun setup/0,
185+
fun cleanup/1,
186+
[
187+
{"absolute redirect follow",
188+
fun test_absolute_redirect_follow/0},
189+
{"relative redirect with slash",
190+
fun test_relative_redirect_with_slash/0},
191+
%% Issue #711: relative redirect without slash
192+
{"relative redirect without slash",
193+
fun test_relative_redirect_without_slash/0},
194+
%% Issue #693: redirect to non-standard port preserves Host header
195+
{"redirect preserves port in Host header",
196+
fun test_redirect_port_in_host/0}
197+
]}.
198+
199+
test_absolute_redirect_follow() ->
200+
RedirectTarget = url(<<"/get">>),
201+
URL = url(<<"/redirect-to?url=", RedirectTarget/binary>>),
202+
{ok, Status, _Headers, Client} = hackney:request(get, URL, [], <<>>, [{follow_redirect, true}]),
203+
Location = hackney:location(Client),
204+
hackney:close(Client),
205+
?assertEqual(200, Status),
206+
?assertEqual(RedirectTarget, Location).
207+
208+
test_relative_redirect_with_slash() ->
209+
%% Redirect to absolute path /get
210+
URL = url(<<"/redirect-to?url=/get">>),
211+
{ok, Status, _Headers, Client} = hackney:request(get, URL, [], <<>>, [{follow_redirect, true}]),
212+
hackney:close(Client),
213+
?assertEqual(200, Status).
214+
215+
test_relative_redirect_without_slash() ->
216+
%% This tests issue #711
217+
%% If we're at /foo/bar and redirect to "baz", we should end up at /foo/baz
218+
%% We need to set up a specific test case for this
219+
%% For now, verify basic relative redirect works
220+
URL = url(<<"/redirect-to?url=get">>), %% relative path without /
221+
{ok, Status, _Headers, Client} = hackney:request(get, URL, [], <<>>, [{follow_redirect, true}]),
222+
FinalLocation = hackney:location(Client),
223+
hackney:close(Client),
224+
%% The redirect from /redirect-to to "get" should go to /get
225+
io:format("Relative redirect result: status=~p, location=~p~n", [Status, FinalLocation]),
226+
?assertEqual(200, Status).
227+
228+
test_redirect_port_in_host() ->
229+
%% This tests issue #693
230+
%% When redirecting to a URL with non-standard port, Host header should include port
231+
%% For this test, we redirect to our own server on non-standard port
232+
RedirectTarget = <<"http://localhost:", (integer_to_binary(?PORT))/binary, "/get">>,
233+
URL = url(<<"/redirect-to?url=", RedirectTarget/binary>>),
234+
{ok, Status, _Headers, Client} = hackney:request(get, URL, [], <<>>, [{follow_redirect, true}]),
235+
hackney:close(Client),
236+
%% If this works, the Host header was correct
237+
?assertEqual(200, Status).
238+
239+
%% Test that netloc includes port for non-standard ports
240+
netloc_port_test_() ->
241+
[
242+
{"netloc includes port for non-standard HTTP port",
243+
fun() ->
244+
URL = hackney_url:normalize(<<"http://example.com:8080/path">>),
245+
?assertEqual(<<"example.com:8080">>, URL#hackney_url.netloc)
246+
end},
247+
{"netloc excludes port for standard HTTP port",
248+
fun() ->
249+
URL = hackney_url:normalize(<<"http://example.com:80/path">>),
250+
?assertEqual(<<"example.com">>, URL#hackney_url.netloc)
251+
end},
252+
{"netloc includes port for non-standard HTTPS port",
253+
fun() ->
254+
URL = hackney_url:normalize(<<"https://example.com:8443/path">>),
255+
?assertEqual(<<"example.com:8443">>, URL#hackney_url.netloc)
256+
end},
257+
{"netloc excludes port for standard HTTPS port",
258+
fun() ->
259+
URL = hackney_url:normalize(<<"https://example.com:443/path">>),
260+
?assertEqual(<<"example.com">>, URL#hackney_url.netloc)
261+
end}
262+
].

0 commit comments

Comments
 (0)