itkacher/OkHttpProfiler

Multipart body altering

Opened this issue · 13 comments

Hi there! Thank you for that nice interceptor.
We've faced fancy issue here:
While trying to upload multipart body containing file with OkHttpProfiler connected, our backend notifies body validation error. With no OkHttpProfiler connected everything fires just ok.
During further inspection we're getting next raw body starting:

--79f57c45-d2a7-440b-8646-081067778104Content-Disposition: form-data; name="file"; filename="file"Content-Type: image/jpegContent-Length: 38511<file body going next>

I'm not sure about validity of that boundary appendix and produced Content-Length.
Is it somehow altered by OkHttpProfiler? Thank you!

Hi!
I have had no problems with the multipart body.
I will investigate this issue. I have no idea right now
Thanks for sharing

Is this a real file size? Filename with ".jpg" or ".jpeg" extension here name="file"; filename="file"?
Also, can you provide a log from the logcat (Verbose level) like:

V/OKPRFL_9o0ubvx_RQB: --275a8191-f116-4421-a4cb-b2fc6fd56eb3
    Content-Disposition: form-data; name="file"; filename="Document_1.jpg"
    Content-Type: image/jpeg
    Content-Length: 200248

Only headers of file part
I can't reproduce any altering.

@itkacher thank you for such a quick response.
The thing I discovered for now is switching off late response proceeding solves the issue:

    public static class PrematureOkHttpProfilerInterceptor implements Interceptor {
        private final DataTransfer dataTransfer = new LogDataTransfer();
        private final DateFormat format = new SimpleDateFormat("ddhhmmssSSS", Locale.US);

        @Override
        public Response intercept(Chain chain) throws IOException {
            Request originalRequest = chain.request();
            Response originalResponse = chain.proceed(originalRequest);

            String id = generateId();
            long startTime = System.currentTimeMillis();
            dataTransfer.sendRequest(id, originalRequest);
            try {
                dataTransfer.sendResponse(id, originalResponse);
                dataTransfer.sendDuration(id, System.currentTimeMillis() - startTime);
                return originalResponse;
            } catch (Exception e) {
                dataTransfer.sendException(id, e);
                dataTransfer.sendDuration(id, System.currentTimeMillis() - startTime);
                throw e;
            }
        }

        private String generateId() {
            long timeAndDay = Long.parseLong(format.format(new Date()));
            return Long.toString(timeAndDay, Character.MAX_RADIX);
        }
    }

Output:

POST /api/content/upload

Accept:application/json
Authorization:Bearer ***
Content-Type:application/json

--d3276c57-519a-4fdf-a304-407adb7c9455Content-Disposition: form-data; name="file"; filename="file"Content-Type: image/jpegContent-Length: 0--d3276c57-519a-4fdf-a304-407adb7c9455--

And looses intercepted parts' content.

Filename with ".jpg" or ".jpeg" extension here

Yep, that is intended.
Will provide requested info ASAP.

Failed request with original interceptor:
https://gist.github.com/DummyCo/4f2ddfccd8d8015c91bf55e100e4e4d8
Succeed request with premature chain processing:
https://gist.github.com/DummyCo/1328c57bd81d076986c2a405f9a00bb8

Thanks a lot. I will try to fix soon. Sorry about the issue.

Request header is:
OKPRFL_9obdan0_RQH: Content-Type:application/json
Are you sure, that this is correct?
Because it can be a reason.
I guess it needs to be like this.
Content-Type: multipart/form-data; boundary=cb57c87e-f1dd-492c-b142-2ffca7ecad04
Can you provide the library (OkHttp or Retrofit) and how do exactly you send the file (java/kotlin code), please?

I did some request fixes with encoding, not sure that this is it.
Can you test it, please?
https://github.com/itkacher/OkHttpProfiler/raw/bugfix/7/okhttpprofiler-1.0.5.jar
Just add this file to the libs folder and add to the gradle
implementation fileTree(dir: 'libs', include: ['*.jar'])
Also, can you set the correct type of MultipartBody?

RequestBody requestBody = new MultipartBody.Builder()
                .setType(MultipartBody.FORM)

Because a server needs to know the boundary for correct request parsing.
I will be grateful if you help me with this.
Thanks

Are you sure, that this is correct?

I agree that it's not too healthy to provide wrong Content-Type here but our server consumes it for now, will work on it further anyway, but I'm not too sure that it may cause this particular problem because request is delivered with no issues using series of other custom and opensource interceptors for different purposes. Nevertheless, will fix it ASAP, thank you.

Going to test new version out and notify about the results!

https://developer.mozilla.org/en-US/docs/Web/HTTP/Headers/Content-Type

boundary
For multipart entities the boundary directive is required, which consists of 1 to 70 characters from a set of characters known to be very robust through email gateways, and not ending with white space. It is used to encapsulate the boundaries of the multiple parts of the message. Often the header boundary is prepended by two dashes in the body and the final boundary also have a two dashes appended to it.

This header is important for the multipart form data.
The server parses content between two bound as one part of the request.
In theory, your request shouldn't work at all.
But I agree with you, that this is strange behavior that it works without Interceptor.
I can't reproduce it now, so I need more data for the problem understanding.
Can you provide the code of request generation I will try your case?
I will install a NodeJS server and try to send a request and compare the content of it with/without interceptor.

Thank you for clarifying. I managed to fix headers and some internal stream feeding flow, here are some interesting points:

  • Retrofit version: 2.5.0
  • okhttp version: 3.12.0

REST service upload method signature:

@Multipart
@POST("content/upload")
Observable<ModelResponse<Content>> uploadContent(
		@Part MultipartBody.Part part
);

Request preparing & uploading flow:

InputStreamRequestBody inputStreamRequestBody = new InputStreamRequestBody(
		MediaType.parse(mimeType),
		inputStream
);

CountingRequestBody countingRequestBody = new CountingRequestBody(
		inputStreamRequestBody,
		(bytesWritten, contentLength) -> {
			//...
		}
);

MultipartBody.Part filePart = MultipartBody.Part.createFormData(
		"file",
		"file",
		countingRequestBody
);

restManager.uploadContent(filePart);

Now that's still firing ok with no OkHttpProfiler interceptor and fails with it getting new unexpected end of stream.
At least we have valid multipart header for now :D

New output:
https://gist.github.com/DummyCo/3e78f660567a9c2d31ebec4e77a9c521

And one more interesting observation. In case we're creating prepared RequestBody with byte array:

ByteArrayOutputStream buffer = new ByteArrayOutputStream();

int nRead;
byte[] data = new byte[16384];

while ((nRead = inputStream.read(data, 0, data.length)) != -1) {
	buffer.write(data, 0, nRead);
}

RequestBody requestBody = RequestBody.create(
		MediaType.parse("image/jpeg"),
		buffer.toByteArray()
);

MultipartBody.Part filePart = MultipartBody.Part.createFormData(
		"file",
		"file",
		requestBody
);

It fires ok with or without interceptor:
https://gist.github.com/DummyCo/aeb5409b9b167364a875b33f66e3881a

I'm comparing successful and failed request bodies side-by-side and getting small diffs present...

@DummyCo can you remove a reference from this issue square/okhttp#3585 ?
I am sure, that this is not a problem of OkHttp.
The other tested way is to send a file

val mimeType = FileUtil.getMimeType(filePath)
val mediaType = MediaType.parse(mimeType ?: filePath)
val file = File(filePath)
val requestBody: RequestBody = RequestBody.create(mediaType, file)
val data = MultipartBody.Part.createFormData("file", file.name, requestBody)

I will test 'CountingRequestBody' and define the problem

Sure, just wanted to provide author implementations, but I'm afraid reference is set automatically by GitHub and can't be undone 😥
Talking about file-based ways: we're pretty limited about this approach because we're forced to work with URIs and resolved streams, so content can be represented by not file-backed resource.