Okhttp: Silent "error" when a 302 redirect url is malformed

[jpearl]

I'm seeing a case in handling redirects where it _appears_ that okhttp isn't following a redirect. After digging through okhttp with the failing url, okhttp is functioning "correctly" - the redirect url is invalid. Therefore, okhttp doesn't follow the redirect, and returns a response with response code 302.

A test illustrating this:

Request request = new Request.Builder().url(originalUrl).build();
Response response = okhttp.newCall(request).execute();

log(response.code());
log(response.isSuccessful());

results in:

302
false

Example for originalUrl:

<originalUrl>, <host>, and <paths> have been used as substitutes for real urls, hosts, and paths

<-- 302 Found <originalUrl>
Date: Sun, 07 Jan 2018 17:04:43 GMT
Server: Apache/2.4.10 (Debian) PHP/5.6.24-0+deb8u1 OpenSSL/1.0.1t
X-Powered-By: PHP/5.6.24-0+deb8u1
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
Location: http://.<host>/<path>
Access-Control-Allow-Origin: *
Connection: close
Content-Type: text/html; charset=UTF-8
<-- END HTTP

And yes, followRedirects and followSslRedirects are enabled

What's going on here is that the 302 redirect location is an illegally formatted URL - in my case, something like http://.<somehost>/<path> (note the leading . to the hostname). OkHttp's RetryAndFollowUpInterceptor reads this Location, and tries to parse it with HttpUrl. That, correctly, fails. At this point, the retry interceptor just returns and allows the 302 to propagate up:
https://github.com/square/okhttp/blob/cd84d79c3574de81f23802f832f2ead6ad38a735/okhttp/src/main/java/okhttp3/internal/http/RetryAndFollowUpInterceptor.java#L307-L312

Now, any sane client should be able to detect that the end response is not successful and handle it gracefully. But through reading through app logs, there's no indication as to what the actual failure was. The failed url parsing on the redirect location is simply swallowed here deep in OkHttp, without any mechanism to inform the client that the error occurred.

Should a failed HttpUrl parse be bubbled up as an exception?

[JakeWharton]

We've been talking about throwing-variants of parse and resolve for quite
some time.

On Sun, Jan 7, 2018 at 6:40 PM jpearl [e-mail] wrote:

I'm seeing a case in handling redirects where it appears that okhttp
isn't following a redirect. After digging through okhttp with the failing
url, okhttp is functioning "correctly" - the redirect url is invalid.
Therefore, okhttp doesn't follow the redirect, and returns a response with
response code 302.

A test illustrating this:

Request request = new Request.Builder().url(originalUrl).build();
Response response = okhttp.newCall(request).execute();

log(response.code());
log(response.isSuccessful());

results in:

302
false

Example for originalUrl:

, , and have been used as substitutes for real urls, hosts, and paths

<-- 302 Found
Date: Sun, 07 Jan 2018 17:04:43 GMT
Server: Apache/2.4.10 (Debian) PHP/5.6.24-0+deb8u1 OpenSSL/1.0.1t
X-Powered-By: PHP/5.6.24-0+deb8u1
Expires: Thu, 19 Nov 1981 08:52:00 GMT
Cache-Control: no-store, no-cache, must-revalidate, post-check=0, pre-check=0
Location: http://./
Access-Control-Allow-Origin: *
Connection: close
Content-Type: text/html; charset=UTF-8
<-- END HTTP

And yes, followRedirects and followSslRedirects are enabled

What's going on here is that the 302 redirect location is an illegally
formatted URL - in my case, something like http://./
(note the leading . to the hostname). OkHttp's RetryAndFollowUpInterceptor
reads this Location, and tries to parse it with HttpUrl. That, correctly,
fails. At this point, the retry interceptor just returns and allows the 302
to propagate up:

https://github.com/square/okhttp/blob/cd84d79c3574de81f23802f832f2ead6ad38a735/okhttp/src/main/java/okhttp3/internal/http/RetryAndFollowUpInterceptor.java#L307-L312

Now, any sane client should be able to detect that the end response is not
successful and handle it gracefully. But through reading through app logs,
there's no indication as to what the actual failure was. The failed url
parsing on the redirect location is simply swallowed here deep in OkHttp,
without any mechanism to inform the client that the error occurred.

Should a failed HttpUrl parse be bubbled up as an exception?

—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
https://github.com/square/okhttp/issues/3767, or mute the thread
https://github.com/notifications/unsubscribe-auth/AAEEESgLDo63oT9JcEG17PkOhLQyfklyks5tIVXmgaJpZM4RV1aO
.

[swankjesse]

I think this is asking a slightly different question – given a redirect that we can’t follow should the call throw?

[JakeWharton]

What's the alternative? I definitely think delivering a 302 that can't be
followed it weird.

What do browsers do?

On Sun, Jan 7, 2018 at 10:21 PM Jesse Wilson [e-mail]
wrote:

I think this is asking a slightly different question – given a redirect
that we can’t follow should the call throw?

—
You are receiving this because you commented.

Reply to this email directly, view it on GitHub
https://github.com/square/okhttp/issues/3767#issuecomment-355877879, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEEEet-tq8i96LcuzCuFU9I4orLHodsks5tIYnDgaJpZM4RV1aO
.

[jpearl]

@swankjesse is right in that I think the first question is what is the expected behavior? I could see a reasonable argument going something along the lines of "well, we already provide an API for Response.isSuccessful(), so the precedent already exists for a failure condition that isn't bubbled up through an exception". Obviously the flip side of that argument is why are there different classifications of errors - those that throw and those that don't, and are those classifications clearly defined as to what type of error falls in each bucket?

Then of course I see a few arguments for the throws, including my original confusion that spurred this, which is that I thought OkHttp was broken and that it wasn't following the redirect, because it gave me no indication at all that it tried, nor that it was the redirect that was the root cause of the overall call failure.

Of course to @JakeWharton's point, a throws on parse and resolve would seem to be one sane option to raising the exception, especially if that's already been a desire.

My one request though on any new exception is to avoid putting URLs into the exception message. URLs, especially for content, can be considered customer sensitive data that we won't log. For example, that's why we use parse and not Request.Builder.url(String)

[JakeWharton]

To clarify, I don't mean changing the behavior of the existing methods, but
rather to have some other version like parseOrThrow or safeParse or
something...

On Sun, Jan 7, 2018 at 10:49 PM jpearl [e-mail] wrote:

@swankjesse https://github.com/swankjesse is right in that I think the
first question is what is the expected behavior? I could see a reasonable
argument going something along the lines of "well, we already provide an
API for Response.isSuccessful(), so the precedent already exists for a
failure condition that isn't bubbled up through an exception". Obviously
the flip side of that argument is why are there different classifications
of errors - those that throw and those that don't, and are those
classifications clearly defined as to what type of error falls in each
bucket?

Then of course I see a few arguments for the throws, including my original
confusion that spurred this, which is that I thought OkHttp was broken and
that it wasn't following the redirect, because it gave me no indication at
all that it tried, nor that it was the redirect that was the root cause of
the overall call failure.

Of course to @JakeWharton https://github.com/jakewharton's point, a
throws on parse and resolve would seem to be one sane option to raising
the exception, especially if that's already been a desire.

My one request though on any new exception is to avoid putting URLs into
the exception message. URLs, especially for content, can be considered
customer sensitive data that we won't log. For example, that's why we use
parse
https://github.com/square/okhttp/blob/cd84d79c3574de81f23802f832f2ead6ad38a735/okhttp/src/main/java/okhttp3/HttpUrl.java#L899
and not Request.Builder.url(String)
https://github.com/square/okhttp/blob/cd84d79c3574de81f23802f832f2ead6ad38a735/okhttp/src/main/java/okhttp3/Request.java#L142-L144

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/square/okhttp/issues/3767#issuecomment-355879971, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEEERTPb5yhwZlrKmqaNCov2WPJ4xFEks5tIZBSgaJpZM4RV1aO
.

[swankjesse]

[swankjesse]

That’s an attempt to redirect to a file: URL. I didn’t get anything satisfying attempting to get Chrome and Firefox to redirect to broken URLs; they appear to just hang.

I don’t think OkHttp should throw when it has a response, even if that response is a malformed redirect. Throwing prevents the application layer from inspecting the HTTP response body, and that might have some clues around what went wrong.

[Mezner]

Will changing this to not throw break consumers? Should we introduce new methods for safely parsing, and then consider changing this implementation in a larger revision?

[JakeWharton]

Existing HttpUrl methods would not be charged in behavior

On Mon, Jan 8, 2018, 8:45 AM Russell Myers [e-mail] wrote:

Will changing this to not throw break consumers? Should we introduce new
methods for safely parsing, and then consider changing this implementation
in a larger revision?

—
You are receiving this because you were mentioned.

Reply to this email directly, view it on GitHub
https://github.com/square/okhttp/issues/3767#issuecomment-355969939, or mute
the thread
https://github.com/notifications/unsubscribe-auth/AAEEEc64oaMV3KjCMgNC6QKgRMU_2tnPks5tIhvdgaJpZM4RV1aO
.

[swankjesse]

I think returning the 302 is our best option here.