Skip to content

Commit 3b98e4b

Browse files
dakronedakrone
committed
Fix redirects to an empty location
In some cases a server can respond with a redirect to an invalid location (like "https:///"). With an async request, this leads to the request hanging forever. This adds an option to do rudimentary validation of the redirect host in the redirect stragety itself, with an option (`:validate-redirects`) to turn it off. Resolves dakrone#416
1 parent c9fce15 commit 3b98e4b

File tree

3 files changed

+121
-33
lines changed

3 files changed

+121
-33
lines changed

‎README.org‎

Lines changed: 5 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -637,7 +637,11 @@ Redirect Options:
637637
- =:graceful= - Similar to =:default=, but does not throw exceptions when max redirects is reached. This is the redirects behaviour in 2.x
638638
- =nil= - When nil, assumes =:default=
639639

640-
You may also pass in an instance of RedirectStrategy if you want a behaviour that's not implemented
640+
You may also pass in an instance of RedirectStrategy (in the =:redirect-strategy= key) if you want a
641+
behavior that's not implemented.
642+
643+
Additionally, clj-http will attempt to validate that a redirect host is not invalid, you can disable
644+
this by setting =:validate-redirects false= in the request (the default is true)
641645

642646
NOTE: The options =:force-redirects= and =:follow-redirects= (present in clj-http 2.x are no longer
643647
used). You can use =:graceful= to mostly emulate the old redirect behaviour.

‎src/clj_http/core.clj‎

Lines changed: 67 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -10,7 +10,8 @@
1010
(java.util Locale)
1111
(org.apache.http HttpEntity HeaderIterator HttpHost HttpRequest
1212
HttpEntityEnclosingRequest HttpResponse
13-
HttpRequestInterceptor HttpResponseInterceptor)
13+
HttpRequestInterceptor HttpResponseInterceptor
14+
ProtocolException)
1415
(org.apache.http.auth UsernamePasswordCredentials AuthScope
1516
NTCredentials)
1617
(org.apache.http.client HttpRequestRetryHandler RedirectStrategy
@@ -23,6 +24,7 @@
2324
CloseableHttpResponse
2425
HttpUriRequest HttpRequestBase)
2526
(org.apache.http.client.protocol HttpClientContext)
27+
(org.apache.http.client.utils URIUtils)
2628
(org.apache.http.config RegistryBuilder)
2729
(org.apache.http.conn.routing HttpRoute HttpRoutePlanner)
2830
(org.apache.http.conn.ssl BrowserCompatHostnameVerifier
@@ -61,33 +63,70 @@
6163
(headers/assoc-join hs k v))
6264
(headers/header-map)))))
6365

64-
(def graceful-redirect-strategy
65-
(reify RedirectStrategy
66-
(getRedirect [this request response context]
67-
(.getRedirect DefaultRedirectStrategy/INSTANCE request response context))
68-
69-
(isRedirected [this request response context]
70-
(let [^HttpClientContext typed-context context
71-
max-redirects (-> (.getRequestConfig typed-context)
72-
.getMaxRedirects)
73-
num-redirects (count (.getRedirectLocations typed-context))]
74-
(if (<= max-redirects num-redirects)
75-
false
76-
(.isRedirected DefaultRedirectStrategy/INSTANCE
77-
request response typed-context))))))
78-
79-
(defn get-redirect-strategy [redirect-strategy]
66+
(defn graceful-redirect-strategy
67+
"Similar to the default redirect strategy, however, does not throw an error
68+
when the maximum number of redirects has been reached. Still supports
69+
validating that the new redirect host is not empty."
70+
[req]
71+
(let [validate? (opt req :validate-redirects)]
72+
(reify RedirectStrategy
73+
(getRedirect [this request response context]
74+
(let [new-request (.getRedirect DefaultRedirectStrategy/INSTANCE
75+
request response context)]
76+
(when (or validate? (nil? validate?))
77+
(let [uri (.getURI new-request)
78+
new-host (URIUtils/extractHost uri)]
79+
(when (nil? new-host)
80+
(throw
81+
(ProtocolException.
82+
(str "Redirect URI does not specify a valid host name: "
83+
uri))))))
84+
new-request))
85+
86+
(isRedirected [this request response context]
87+
(let [^HttpClientContext typed-context context
88+
max-redirects (-> (.getRequestConfig typed-context)
89+
.getMaxRedirects)
90+
num-redirects (count (.getRedirectLocations typed-context))]
91+
(if (<= max-redirects num-redirects)
92+
false
93+
(.isRedirected DefaultRedirectStrategy/INSTANCE
94+
request response typed-context)))))))
95+
96+
(defn default-redirect-strategy [^RedirectStrategy original req]
97+
"Proxies calls to whatever original redirect strategy is passed in, however,
98+
if :validate-redirects is set in the request, checks that the redirected host
99+
is not empty."
100+
(let [validate? (opt req :validate-redirects)]
101+
(reify RedirectStrategy
102+
(getRedirect [this request response context]
103+
(let [new-request (.getRedirect original request response context)]
104+
(when (or validate? (nil? validate?))
105+
(let [uri (.getURI new-request)
106+
new-host (URIUtils/extractHost uri)]
107+
(when (nil? new-host)
108+
(throw
109+
(ProtocolException.
110+
(str "Redirect URI does not specify a valid host name: "
111+
uri))))))
112+
new-request))
113+
114+
(isRedirected [this request response context]
115+
(.isRedirected original request response context)))))
116+
117+
(defn get-redirect-strategy [{:keys [redirect-strategy] :as req}]
80118
(case redirect-strategy
81119
:none (reify RedirectStrategy
82120
(getRedirect [this request response context] nil)
83121
(isRedirected [this request response context] false))
84122

85-
;; Like default, but does not
86-
:graceful graceful-redirect-strategy
123+
;; Like default, but does not throw exceptions when max redirects is
124+
;; reached.
125+
:graceful (graceful-redirect-strategy req)
87126

88-
:default (DefaultRedirectStrategy/INSTANCE)
89-
:lax (LaxRedirectStrategy.)
90-
nil (DefaultRedirectStrategy/INSTANCE)
127+
:default (default-redirect-strategy DefaultRedirectStrategy/INSTANCE req)
128+
:lax (default-redirect-strategy (LaxRedirectStrategy.) req)
129+
nil (default-redirect-strategy DefaultRedirectStrategy/INSTANCE req)
91130

92131
;; use directly as reifed RedirectStrategy
93132
redirect-strategy))
@@ -154,17 +193,16 @@
154193
(DefaultProxyRoutePlanner. (construct-http-host proxy-host proxy-port))
155194
(SystemDefaultRoutePlanner. (ProxySelector/getDefault)))))
156195

157-
(defn http-client [{:keys [redirect-strategy retry-handler uri
158-
request-interceptor response-interceptor
159-
proxy-host proxy-port http-builder-fns]
196+
(defn http-client [{:keys [retry-handler uri request-interceptor
197+
response-interceptor proxy-host proxy-port
198+
http-builder-fns]
160199
:as req}
161200
conn-mgr http-url proxy-ignore-host]
162201
;; have to let first, otherwise we get a reflection warning on (.build)
163202
(let [^HttpClientBuilder builder (-> (HttpClients/custom)
164203
(.setConnectionManager conn-mgr)
165204
(.setRedirectStrategy
166-
(get-redirect-strategy
167-
redirect-strategy))
205+
(get-redirect-strategy req))
168206
(add-retry-handler retry-handler)
169207
;; By default, get the proxy settings
170208
;; from the jvm or system properties
@@ -188,17 +226,15 @@
188226
(http-builder-fn builder req))
189227
(.build builder)))
190228

191-
(defn http-async-client [{:keys [redirect-strategy uri
192-
request-interceptor response-interceptor
229+
(defn http-async-client [{:keys [uri request-interceptor response-interceptor
193230
proxy-host proxy-port async-http-builder-fns]
194231
:as req}
195232
conn-mgr http-url proxy-ignore-host]
196233
;; have to let first, otherwise we get a reflection warning on (.build)
197234
(let [^HttpAsyncClientBuilder builder (-> (HttpAsyncClients/custom)
198235
(.setConnectionManager conn-mgr)
199236
(.setRedirectStrategy
200-
(get-redirect-strategy
201-
redirect-strategy))
237+
(get-redirect-strategy req))
202238
;; By default, get the proxy
203239
;; settings from the jvm or system
204240
;; properties

‎test/clj_http/test/core_test.clj‎

Lines changed: 49 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -8,14 +8,15 @@
88
[clojure.test :refer :all]
99
[ring.adapter.jetty :as ring])
1010
(:import (java.io ByteArrayInputStream)
11+
(java.util.concurrent TimeoutException TimeUnit)
1112
(org.apache.http.params CoreConnectionPNames CoreProtocolPNames)
1213
(org.apache.http.message BasicHeader BasicHeaderIterator)
1314
(org.apache.http.client.methods HttpPost)
1415
(org.apache.http.client.protocol HttpClientContext)
1516
(org.apache.http.client.config RequestConfig)
1617
(org.apache.http.client.params CookiePolicy ClientPNames)
1718
(org.apache.http HttpRequest HttpResponse HttpConnection
18-
HttpInetConnection HttpVersion)
19+
HttpInetConnection HttpVersion ProtocolException)
1920
(org.apache.http.protocol HttpContext ExecutionContext)
2021
(org.apache.http.impl.client DefaultHttpClient)
2122
(org.apache.http.client.params ClientPNames)
@@ -48,6 +49,8 @@
4849
[:get "/redirect"]
4950
{:status 302
5051
:headers {"location" "http://localhost:18080/redirect"}}
52+
[:get "/bad-redirect"]
53+
{:status 301 :headers {"location" "https:///"}}
5154
[:get "/redirect-to-get"]
5255
{:status 302
5356
:headers {"location" "http://localhost:18080/get"}}
@@ -706,3 +709,48 @@
706709
(is (.contains (get-in @resp [:headers "got"]) "\"foo\" \"bar\"")
707710
"Headers should have included the new default headers")
708711
(is (not (realized? error)))))
712+
713+
(deftest ^:integration test-bad-redirects
714+
(run-server)
715+
(try
716+
(client/get (localhost "/bad-redirect"))
717+
(is false "should have thrown an exception")
718+
(catch ProtocolException e
719+
(is (.contains
720+
(.getMessage e)
721+
"Redirect URI does not specify a valid host name: https:///"))))
722+
;; async version
723+
(let [e (atom nil)
724+
latch (promise)]
725+
(try
726+
(.get
727+
(client/get (localhost "/bad-redirect") {:async true}
728+
(fn [resp]
729+
(is false
730+
(str "should not have been called but got" resp)))
731+
(fn [err]
732+
(reset! e err)
733+
(deliver latch true)
734+
nil)))
735+
(catch Exception error
736+
(is (.contains
737+
(.getMessage error)
738+
"Redirect URI does not specify a valid host name: https:///"))))
739+
@latch
740+
(is (.contains
741+
(.getMessage @e)
742+
"Redirect URI does not specify a valid host name: https:///")))
743+
(try
744+
(.get (client/get
745+
(localhost "/bad-redirect")
746+
{:async true
747+
:validate-redirects false}
748+
(fn [resp]
749+
(is false
750+
(str "should not have been called but got" resp)))
751+
(fn [err]
752+
(is false
753+
(str "should not have been called but got" err))))
754+
1 TimeUnit/SECONDS)
755+
(is false "should have thrown a timeout exception")
756+
(catch TimeoutException te)))

0 commit comments

Comments
 (0)