IllegalArgumentException in Headers$Builder.checkNameAndValue() for content-disposition filename response header

[would-code-again]

The fix for #891 is causing IllegalArgumentException to be thrown for non ASCII response headers. Most notably, content-disposition, and the filename part. This is basically causing filenames with all the best letters from all the best countries (ü,ä,î,â etc) to crash our app.

The failure mode for us is semi-rare for a variety of reasons, but I imagine this would be far worse for others.

The RFC on the "filename" parameter allows any ISO-8859-1 character: http://tools.ietf.org/html/rfc6266#page-5 so the current behaviour definitely seems to be incorrect.

It makes sense to check request headers. I'm not convinced that it's reasonable for response headers, given that there are plenty of occasions where you just don't control the full stack. Actually, even the unit test that was added in the commit a57aa43 (responseHeaderParsingIsLenient) seems to imply that this is what was intended - must just be a bug.

It would be great if OkHttp could be a little less strict in production situations, but that's sort of unrelated.

Possible fix would be addLenient at https://github.com/square/okhttp/blob/master/okhttp/src/main/java/com/squareup/okhttp/internal/http/FramedTransport.java#L192 but I don't know the ins and outs of this code with enough confidence to make those changes.

java.lang.IllegalArgumentException: Unexpected char 0xf6 at 21 in header value: inline;filename="Andy��IsSad.png"
at com.squareup.okhttp.Headers$Builder.checkNameAndValue(Headers.java:295)
at com.squareup.okhttp.Headers$Builder.add(Headers.java:245)
at com.squareup.okhttp.internal.http.FramedTransport.readNameValueBlock(FramedTransport.java:192)
at com.squareup.okhttp.internal.http.FramedTransport.readResponseHeaders(FramedTransport.java:104)
at com.squareup.okhttp.internal.http.HttpEngine.readNetworkResponse(HttpEngine.java:906)
at com.squareup.okhttp.internal.http.HttpEngine.access$300(HttpEngine.java:92)
at com.squareup.okhttp.internal.http.HttpEngine$NetworkInterceptorChain.proceed(HttpEngine.java:891)
at com.squareup.okhttp.internal.http.HttpEngine.readResponse(HttpEngine.java:749)
at com.squareup.okhttp.Call.getResponse(Call.java:268)
at com.squareup.okhttp.Call$ApplicationInterceptorChain.proceed(Call.java:224)
at com.squareup.okhttp.Call.getResponseWithInterceptorChain(Call.java:195)
at com.squareup.okhttp.Call.execute(Call.java:79)

[elincoln]

Definitely an issue for me right now -- in my case, its other servers sending us set-cookie: headers that contain the 0x01 control character.

(EDIT: Reverted to 2.4 to avoid the issue for now.)

[HugoGresse]

any update on this?

[swankjesse]

Nothing yet. Is it just redirects and cookies?

[swankjesse]

Yeah, we definitely shouldn’t be throwing an unchecked exception here.

One problem is that the spec wants us to not interpret header fields, but since we’re exposing a Java API we really want strings.

Historically, HTTP has allowed field content with text in the
ISO-8859-1 charset [ISO-8859-1], supporting other charsets only
through use of [RFC2047] encoding. In practice, most HTTP header
field values use only a subset of the US-ASCII charset [USASCII].
Newly defined header fields SHOULD limit their field values to
US-ASCII octets. A recipient SHOULD treat other octets in field
content (obs-text) as opaque data.

Handling ISO-8859-1 feels wrong, particularly since the upside of supporting additional characters has the downside of being locked out of UTF-8. Neither stripping non-ASCII characters nor stripping non-ASCII headers feels appropriate.

[zewenwang]

Use URLEncoder encode headers before addHeader, it works well for me.

[would-code-again]

@zewenwang that only works when you're in control of the headers. If you're sending requests to some site that you don't control and receiving troublesome response headers, it's a much less effective strategy.

[rfc2822]

Same problem here. Just got this (anonymous) Google Play crash report:

java.lang.RuntimeException: An error occurred while executing doInBackground()
    at android.support.v4.content.ModernAsyncTask$3.done(ModernAsyncTask.java:142)
    at java.util.concurrent.FutureTask.finishCompletion(FutureTask.java:354)
    at java.util.concurrent.FutureTask.setException(FutureTask.java:223)
    at java.util.concurrent.FutureTask.run(FutureTask.java:242)
    at java.util.concurrent.ThreadPoolExecutor.runWorker(ThreadPoolExecutor.java:1113)
    at java.util.concurrent.ThreadPoolExecutor$Worker.run(ThreadPoolExecutor.java:588)
    at java.lang.Thread.run(Thread.java:818)
Caused by: java.lang.IllegalArgumentException: Unexpected char 0x201c at 0 in strict-transport-security value: “max-age=7200″
    at okhttp3.Headers$Builder.checkNameAndValue(Headers.java:320)
    at okhttp3.Headers$Builder.add(Headers.java:270)
    at okhttp3.internal.http.Http2xStream.readHttp2HeadersList(Http2xStream.java:263)
    at okhttp3.internal.http.Http2xStream.readResponseHeaders(Http2xStream.java:149)
    at okhttp3.internal.http.HttpEngine.readNetworkResponse(HttpEngine.java:775)
    at okhttp3.internal.http.HttpEngine.access$200(HttpEngine.java:86)
    at okhttp3.internal.http.HttpEngine$NetworkInterceptorChain.proceed(HttpEngine.java:760)
    at at.bitfire.davdroid.HttpClient$PreemptiveAuthenticationInterceptor.intercept(HttpClient.java:153)
    at okhttp3.internal.http.HttpEngine$NetworkInterceptorChain.proceed(HttpEngine.java:733)
    at at.bitfire.davdroid.HttpClient$UserAgentInterceptor.intercept(HttpClient.java:139)
    at okhttp3.internal.http.HttpEngine$NetworkInterceptorChain.proceed(HttpEngine.java:733)
    at okhttp3.internal.http.HttpEngine.readResponse(HttpEngine.java:613)
    at okhttp3.RealCall.getResponse(RealCall.java:244)
    at okhttp3.RealCall$ApplicationInterceptorChain.proceed(RealCall.java:201)
    at okhttp3.logging.HttpLoggingInterceptor.intercept(HttpLoggingInterceptor.java:212)
    at okhttp3.RealCall$ApplicationInterceptorChain.proceed(RealCall.java:190)
    at okhttp3.RealCall.getResponseWithInterceptorChain(RealCall.java:163)
    at okhttp3.RealCall.execute(RealCall.java:57)
    at at.bitfire.dav4android.DavResource.propfind(DavResource.java:268)
    at at.bitfire.davdroid.ui.setup.DavResourceFinder.getCurrentUserPrincipal(DavResourceFinder.java:329)
    at at.bitfire.davdroid.ui.setup.DavResourceFinder.findInitialConfiguration(DavResourceFinder.java:120)
    at at.bitfire.davdroid.ui.setup.DavResourceFinder.findInitialConfiguration(DavResourceFinder.java:87)
    at at.bitfire.davdroid.ui.setup.DetectConfigurationFragment$ServerConfigurationLoader.loadInBackground(DetectConfigurationFragment.java:142)
    at at.bitfire.davdroid.ui.setup.DetectConfigurationFragment$ServerConfigurationLoader.loadInBackground(DetectConfigurationFragment.java:125)
    at android.support.v4.content.AsyncTaskLoader.onLoadInBackground(AsyncTaskLoader.java:296)
    at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:54)
    at android.support.v4.content.AsyncTaskLoader$LoadTask.doInBackground(AsyncTaskLoader.java:42)
    at android.support.v4.content.ModernAsyncTask$2.call(ModernAsyncTask.java:128)
    at java.util.concurrent.FutureTask.run(FutureTask.java:237)
    ... 3 more

So, an HTTP request execute call has thrown an IllegalArgumentException which I could really not expect, causing DAVdroid to crash.

I suggest to use checked exceptions instead – maybe subclasses of IOException? For instance, CharacterCodingException is used for a similar case and it's an IOException.

[brsengar]

Don't know why but I am still getting the error for some Japanese characters in the release 3.3.1. Had the fix released in 3.3.1?

[rowi1de]

@JakeWharton The validation can easily bypassed:

public LoginRequestBuilder addUmlauts(final String umlauts) {
        Map<String, List<String>> currentHeaders = builder.build().headers().toMultimap();
        Map<String, String> newHeaders = new HashMap<String, String>();
        for (Map.Entry<String, List<String>> entrySet : currentHeaders.entrySet()) {
            final String headerName = entrySet.getKey();
            builder.removeHeader(headerName);
            newHeaders.put(headerName, entrySet.getValue().get(0));
        }
        newHeaders.put("Umlauts", umlauts);
        builder.headers(Headers.of(newHeaders));
        return this;
    }

[beefeather]

I understand the current approach is to accept malformed header (with non-ascii characters), but to never allow generating them.
Could this be just too strict? I find it hardly usable for cookies. One is supposed to accept cookies and send them back. So once you got a (malformed) utf-8 cookie value, you have a problem with sending them back.
Do you think allowing this could become an option? A dirty method with an unpleasant name that swallows bad characters maybe? :)

[blu3-b1rd]

Updated to the latest version of the lib and still getting same error with the following header:

User-Agent: okhttp/3.3.0 (X325; X325–Locked to Life Wireless; X325; 4.4.2; 3.1.5; 216.48.89.45; alive)

Throwing this exception:

Unexpected char 0x2013 at 24 in User-Agent value: okhttp/3.3.0 (X325; X325–Locked to Life Wireless; X325; 4.4.2; 3.1.5; 216.48.89.45; alive)

[manjunath143]

I am also facing same error. I have updated to latest version still issue persist. @swankjesse do we have any workaround or any fix for this ?. Thank you for help advance.

[swankjesse]

@manjunath143 please open a pull request with an executable test case that demonstrates the failure.