Skip to content
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

Handling of early 403 FORBIDDEN with Connection: close whilst streaming #1001

Closed
fakraemer opened this issue Jul 24, 2014 · 4 comments · Fixed by #6295
Closed

Handling of early 403 FORBIDDEN with Connection: close whilst streaming #1001

fakraemer opened this issue Jul 24, 2014 · 4 comments · Fixed by #6295
Labels
enhancement Feature not a bug
Milestone

Comments

@fakraemer
Copy link

We are using okhttp against an HTTP server (Finagle) which will cut off the connection early if the client is not authorised for a particular resource.

HTTP/1.1 403 Forbidden\r\n
[Expert Info (Chat/Sequence): HTTP/1.1 403 Forbidden\r\n]
Request Version: HTTP/1.1
Status Code: 403
Response Phrase: Forbidden
Content-Length: 0\r\n
Connection: close\r\n
\r\n

The client does not handle this behaviour well, as in it escapes with

java.net.SocketException: Connection reset
    at java.net.SocketOutputStream.socketWrite(SocketOutputStream.java:118)
    at java.net.SocketOutputStream.write(SocketOutputStream.java:159)
    at okio.Okio$1.write(Okio.java:78)
    at okio.AsyncTimeout$1.write(AsyncTimeout.java:155)
    at okio.RealBufferedSink.emitCompleteSegments(RealBufferedSink.java:133)
    at okio.RealBufferedSink.write(RealBufferedSink.java:45)
    at com.squareup.okhttp.internal.http.HttpConnection$FixedLengthSink.write(HttpConnection.java:302)
    at okio.RealBufferedSink.emitCompleteSegments(RealBufferedSink.java:133)
    at okio.RealBufferedSink.writeAll(RealBufferedSink.java:83)
    at com.squareup.okhttp.internal.http.URLConnectionTest$2.writeTo(URLConnectionTest.java:2235)
    at com.squareup.okhttp.Call.getResponse(Call.java:202)
    at com.squareup.okhttp.Call.execute(Call.java:80) 

I'm not an expert on the HTTP/1.1 spec, but given from what I've read so far, I believe the server is in the right and I'd expect the client to parse the response headers and expose them. The sections I'm referring to are

8.1.2 Overall Operation
... Persistent connections provide a mechanism by which a client and a server can signal the close of a TCP connection. This signaling takes > place using the Connection header field (section 14.10). Once a close has been signaled, the client MUST NOT send any more requests on that connection.

and further

8.1.2.1 Negotiation
... If the server chooses to close the connection immediately after sending the
response, it SHOULD send a Connection header including the connection-token close.
An HTTP/1.1 client MAY expect a connection to remain open, but would decide to keep it open based on whether the response from a server contains a Connection header with the connection-token close.

I tried to recreate the problem in your test infrastructure, please see URLConnectionTest#connectionCloseEarlyInResponseWhilstStreaming (based on 7f763c1).

@swankjesse
Copy link
Collaborator

Comnection reuse is a red herring here. Your situation has two different results:

  • The request body was truncated by the server. That's an error.
  • The response of 403.

OkHttp gives up as soon as the error is encountered. It's unlikely that we can do anything better.

That said, I'm curious about what other user agents do here. Does curl return the 403? What about Chrome?

@fakraemer
Copy link
Author

Hi Jesse,

Thanks for your reply. I'm not sure if I've done this correctly, but it seems curl can cope with the server responding early with a 403.

dd if=/dev/zero bs=10485760 count=1|curl -v -H "Content-Type: application/octet-stream" -H "Content-Length: 10485760" -H "Connection: Keep-Alive" -H "Expect:" -H "Transfer-Encoding:" -T - http://localhost:8080/context
* About to connect() to localhost port 8080 (#0)
*   Trying 127.0.0.1...
* Connected to localhost (127.0.0.1) port 8080 (#0)
> PUT /context HTTP/1.1
> User-Agent: curl/7.29.0
> Host: localhost:8080
> Accept: */*
> Content-Type: application/octet-stream
> Content-Length: 10485760
> Connection: Keep-Alive
> 
< HTTP/1.1 403 Forbidden
< Content-Length: 0
< Connection: close
< 
* we are done reading and this is set to close, stop send
* Closing connection 0

And this is what I got from the tcpdump

PUT /context HTTP/1.1
User-Agent: curl/7.29.0
Host: localhost:8080
Accept: */*
Content-Type: application/octet-stream
Content-Length: 10485760
Connection: Keep-Alive

...

HTTP/1.1 403 Forbidden
Content-Length: 0
Connection: close

Note that I changed the URLs and truncated the request body shown here, ... standing for the zeros I generated. The entire conversation is 147770 bytes long, so since the input is 10485760 bytes I can assume that we got truncated early and could not stream out the full body.

Cheers,
Fabs

@swankjesse swankjesse added the enhancement Feature not a bug label Sep 28, 2014
@swankjesse swankjesse added this to the Icebox milestone Sep 28, 2014
@ricellis
Copy link

There are some other cases where the spec allows a server to close a connection before sending a request body is complete, such as 413 Request Entity Too Large. It would be nice to read those responses and stop sending data rather than throw a SocketTimeoutException. This is particularly true because we also can't avoid starting the send by using Expect: 100-Continue due to #675.

I think the relevant part of the spec relating to this is actually 8.2.2.

For the 413 case I was looking at OkHttp (3.4.2) does:

Exception in thread "main" java.net.SocketTimeoutException: timeout
    at okio.Okio$3.newTimeoutException(Okio.java:210)
    at okio.AsyncTimeout.exit(AsyncTimeout.java:288)
    at okio.AsyncTimeout$1.write(AsyncTimeout.java:184)
    at okio.RealBufferedSink.emitCompleteSegments(RealBufferedSink.java:171)
    at okio.RealBufferedSink.write(RealBufferedSink.java:41)
    at okhttp3.internal.http.Http1xStream$FixedLengthSink.write(Http1xStream.java:282)
    at okio.RealBufferedSink.emitCompleteSegments(RealBufferedSink.java:171)
    at okio.RealBufferedSink.write(RealBufferedSink.java:91)
    at okhttp3.RequestBody$2.writeTo(RequestBody.java:96)
    at okhttp3.internal.http.CallServerInterceptor.intercept(CallServerInterceptor.java:47)
... interceptor chain omitted...
    at okhttp3.RealCall.getResponseWithInterceptorChain(RealCall.java:170)
    at okhttp3.RealCall.execute(RealCall.java:60)
...
Caused by: java.net.SocketException: Broken pipe (Write failed)
    at java.net.SocketOutputStream.socketWrite0(Native Method)
    at java.net.SocketOutputStream.socketWrite(SocketOutputStream.java:109)
    at java.net.SocketOutputStream.write(SocketOutputStream.java:153)
    at sun.security.ssl.OutputRecord.writeBuffer(OutputRecord.java:431)
    at sun.security.ssl.OutputRecord.write(OutputRecord.java:417)
    at sun.security.ssl.SSLSocketImpl.writeRecordInternal(SSLSocketImpl.java:876)
    at sun.security.ssl.SSLSocketImpl.writeRecord(SSLSocketImpl.java:847)
    at sun.security.ssl.AppOutputStream.write(AppOutputStream.java:123)
    at okio.Okio$1.write(Okio.java:78)
    at okio.AsyncTimeout$1.write(AsyncTimeout.java:180)
    ... 27 more

whereas cURL does manage to read the response status and body and stop the send:

< HTTP/1.1 413 Request Entity Too Large
* HTTP error before end of send, stop sending
< 
{"error":"too_large","reason":"the request entity is too large"}
* Closing connection 0

bessbd pushed a commit to bessbd/okhttp that referenced this issue Jun 8, 2020
As per https://tools.ietf.org/html/rfc2616#section-8.2.2 An
HTTP/1.1 (or later) client sending a message-body SHOULD
monitor the network connection for an error status while it
is transmitting the request.

Currently, okhttp implements a send request, then read response
if/when request was sent successfully.
This hides early responses such as 413 Payload Too Large
errors from clients.

This commit fixes that by adding a response read in case of an
exception happening while the request is being sent.

This closes square#1001 .
@frannoriega
Copy link

Today (6 years after this issue was opened), I experienced the same problem.
I managed to create the following network interceptor which allowed me to work around the issue, using okhttp 4.6.0.

import okhttp3.Connection;
import okhttp3.Interceptor;
import okhttp3.MediaType;
import okhttp3.Protocol;
import okhttp3.Response;
import okhttp3.ResponseBody;
import okhttp3.internal.http1.HeadersReader;
import okio.Buffer;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;

import java.io.IOException;
import java.io.InputStream;
import java.net.ProtocolException;
import java.net.Socket;
import java.net.SocketException;
import java.util.Objects;

import javax.annotation.Nonnull;

public class SocketClosedRecoveryInterceptor implements Interceptor {

	@Nonnull
	@Override
	public Response intercept(@Nonnull Chain chain) throws IOException {
		try {
			return chain.proceed(chain.request());
		} catch (SocketException e) {
			Connection connection = Objects.requireNonNull(
					chain,
					"The interceptor chain should have a non-null connection").connection();
			Socket socket = Objects.requireNonNull(
					connection,
					"The interceptor connection should have a non-null socket").socket();
			InputStream inputStream = Objects.requireNonNull(
					socket,
					"The interceptor socket should have a non-null input stream").getInputStream();
			try (Buffer buffer = new Buffer()) {
				buffer.write(inputStream.readNBytes(inputStream.available()));
				HeadersReader headersReader = new HeadersReader(buffer);
				StatusLine statusLine = StatusLine.parse(headersReader.readLine());
				Response.Builder builder = new Response.Builder()
						.request(chain.request())
						.protocol(statusLine.protocol)
						.code(statusLine.code)
						.message(statusLine.message)
						.headers(headersReader.readHeaders())
						.body(ResponseBody.create(buffer, MediaType.get("text/plain"), buffer.size()));
				return builder.build();
			}
		}
	}

	/**
	 * An HTTP response status line like "HTTP/1.1 200 OK". Copied from
	 * {@link okhttp3.internal.http.StatusLine}, since it was package-private.
	 */
	private static final class StatusLine {
		/** Numeric status code, 307: Temporary Redirect. */
		public static final int HTTP_TEMP_REDIRECT = 307;
		public static final int HTTP_PERM_REDIRECT = 308;
		public static final int HTTP_CONTINUE = 100;

		public final Protocol protocol;
		public final int code;
		public final String message;

		public StatusLine(Protocol protocol, int code, String message) {
			this.protocol = protocol;
			this.code = code;
			this.message = message;
		}

		public static StatusLine get(Response response) {
			return new StatusLine(response.protocol(), response.code(), response.message());
		}

		public static StatusLine parse(String statusLine) throws IOException {
			// H T T P / 1 . 1   2 0 0   T e m p o r a r y   R e d i r e c t
			// 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0 1 2 3 4 5 6 7 8 9 0

			// Parse protocol like "HTTP/1.1" followed by a space.
			int codeStart;
			Protocol protocol;
			if (statusLine.startsWith("HTTP/1.")) {
				if (statusLine.length() < 9 || statusLine.charAt(8) != ' ') {
					throw new ProtocolException("Unexpected status line: " + statusLine);
				}
				int httpMinorVersion = statusLine.charAt(7) - '0';
				codeStart = 9;
				if (httpMinorVersion == 0) {
					protocol = Protocol.HTTP_1_0;
				} else if (httpMinorVersion == 1) {
					protocol = Protocol.HTTP_1_1;
				} else {
					throw new ProtocolException("Unexpected status line: " + statusLine);
				}
			} else if (statusLine.startsWith("ICY ")) {
				// Shoutcast uses ICY instead of "HTTP/1.0".
				protocol = Protocol.HTTP_1_0;
				codeStart = 4;
			} else {
				throw new ProtocolException("Unexpected status line: " + statusLine);
			}

			// Parse response code like "200". Always 3 digits.
			if (statusLine.length() < codeStart + 3) {
				throw new ProtocolException("Unexpected status line: " + statusLine);
			}
			int code;
			try {
				code = Integer.parseInt(statusLine.substring(codeStart, codeStart + 3));
			} catch (NumberFormatException e) {
				throw new ProtocolException("Unexpected status line: " + statusLine);
			}

			// Parse an optional response message like "OK" or "Not Modified". If it
			// exists, it is separated from the response code by a space.
			String message = "";
			if (statusLine.length() > codeStart + 3) {
				if (statusLine.charAt(codeStart + 3) != ' ') {
					throw new ProtocolException("Unexpected status line: " + statusLine);
				}
				message = statusLine.substring(codeStart + 4);
			}

			return new StatusLine(protocol, code, message);
		}

		@Override
		public String toString() {
			StringBuilder result = new StringBuilder();
			result.append(protocol == Protocol.HTTP_1_0 ? "HTTP/1.0" : "HTTP/1.1");
			result.append(' ').append(code);
			if (message != null) {
				result.append(' ').append(message);
			}
			return result.toString();
		}
	}
}

My hypothesis is that the underlying SocketInputStream that OkHTTP is using, throws the SocketException: connection reset whenever there is an attempt to read more bytes than what are actually available.

What I did was basically read only those available bytes (inputStream.readNBytes(inputStream.available())).
The problem is that by doing that, I was working with the raw bytes from the response (outside of OkHTTP types), so I had to parse to create the Response (I copied the code that OkHTTP was using to parse what I needed, the StatusLine class, which is package-private) .

I was mostly interested in getting the status code and message, but I went the extra mile to try to get a text/plain response body.

So far, I tested this with the troublesome requests and it is working. I will keep testing this and see if the hypothesis is correct.
I wanted to share what I've found so far (it could be a useful workaround for others having the same issue) and restart a discussion to see if this can be handled by OkHTTP.

bessbd pushed a commit to bessbd/okhttp that referenced this issue Sep 28, 2020
As per https://tools.ietf.org/html/rfc2616#section-8.2.2 An
HTTP/1.1 (or later) client sending a message-body SHOULD
monitor the network connection for an error status while it
is transmitting the request.

Currently, okhttp implements a send request, then read response
if/when request was sent successfully.
This hides early responses such as 413 Payload Too Large
errors from clients.

This commit fixes that by adding a response read in case of an
exception happening while the request is being sent.

This closes square#1001 .
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement Feature not a bug
Projects
None yet
4 participants