Collect: Request headers are treated as case sensitive

[mateusmedeiros]

Software and hardware versions

Collect v1.21.2, Android v8.0.0, Asus Zenfone 3 Zoom (ZE553KL)

Problem description

The RFC2616 section 4.2 dictates that request / response headers are supposed to be case insensitive.

Each header field consists of a name followed by a colon (":") and the field value. Field names are case-insensitive.

But from what I could tell, it seems the way collect currently works is by putting the headers into a HashMap<String, String>, which seems to have the effect of making the headers case sensitive.

In my specific case, I was trying to raise the content length limit by sending an "X-OpenRosa-Accept-Content-Length" header in the HEAD response, but found it didn't work, even though I could see from the source code below that it did check for it's presence and set the contentLength accordingly:
https://github.com/opendatakit/collect/blob/8f65795c613a7759abfff0ba557c261fe3c5fa8e/collect_app/src/main/java/org/odk/collect/android/upload/InstanceServerUploader.java#L100-L107

These responseHeaders I believe came from here:
https://github.com/opendatakit/collect/blob/8f7a3e6abb9ac18ee3ce8a1eb81dfa8e78b00aa6/collect_app/src/main/java/org/odk/collect/android/http/HttpClientConnection.java#L241-L262

Which justs put the result of head.getName() in the HashMap.

Technically, it is stated in the Form Submission API docs "Successful server responses MUST include an X-OpenRosa-Accept-Content-Length header in addition to the required OpenRosa Request response headers.", so I guess that may be interpreted as the burden of sending the correct header being with the server, but since we're talking HTTP requests here, I believe it makes sense that the collect app doesn't depend on that casing.

Also, that problem exists in other places, like for example here:
https://github.com/opendatakit/collect/blob/8f65795c613a7759abfff0ba557c261fe3c5fa8e/collect_app/src/main/java/org/odk/collect/android/upload/InstanceServerUploader.java#L123

it will fail to see a header "Location" if it isn't written exactly like that, even though the Form Submission API states only "If the server is RESTful, the server MAY return an HTTP URI (using the standard HTTP Location header) where the form can be found." (I believe "standard HTTP Location header" meaning that it follows the HTTP standard and so that would be case insensitive).

Steps to reproduce the problem

1. Setup an OpenRosa compliant server except that it sends headers as all uppercase, all downcase or any other combination besides the ones specifically used in the code
2. Configure the server to send a custom content length limit through the X-OpenRosa-Accept-Content-Length header
3. Send a form with a body larger than 10mb
4. See it being sent as multiple requests to the server

Expected behavior

Collect respecting the limit on the X-OpenRosa-Accept-Content-Length header no matter in which case it is sent.

Other information

From what I could see, a seemingly common way to make a case insensitive Map is by using a TreeMap with the String.CASE_INSENSITIVE_ORDER comparator being passed as argument to the constructor.

(Kinda like this: new TreeMap<String, String>(String.CASE_INSENSITIVE_ORDER)).

But my Java is so rusty that I have no confidence in affirming that that will be an easy or good solution.

Also, I confess that I didn't check if the underlying HttpClient will keep the original formatting of headers or will normalize them in some way. If it normalizes them, then this bug will actually be about the "X-OpenRosa-Accept-Content-Length" probably being normalized to "X-Openrosa-Accept-Content-Length" (which is still missed in the check).

[lognaturel]

Thanks so much for that really thoughtful description of the problem, @mateusmedeiros! Agreed that it's worth complying with RFC2616 and not enforcing a particular case on the headers. I hope you weren't also too frustrated by the vague requirements of the Date header. May I ask what you're working on?

There has on and off been some work to modernize the networking code but unfortunately it hasn't been a priority so it has been rather slow. I expect it will pick back up soon and ideally this fix would happen as part of that. I figure a fix is not urgent because in the short term you can use the enforced case, is that right?

[mateusmedeiros]

@lognaturel Thanks!

I'm working on a Ruby on Rails implementation of an OpenRosa compliant server to be mounted as an engine in a private Rails project.

In my case, because the underlying Ruby standard class Net::HTTP, which is responsible for the response behind the scenes, will automatically do a normalization of the headers' case for some reason, Rails is always sending "X-Openrosa-Accept-Content-Length" no matter which case I use.

I tried to find a way to work around that but probably because the case is not supposed to matter, there isn't any official supported way. The easiest way I could find was monkey-patching the Net::HTTP, overriding some internal methods to custom implementations that will do some workarounds to preserve the case.

As that is not an ideal solution, I presented the situation to our client, explaining that one solution would be the most correct one in my opinion but involved a change being made in the ODK collect app, which may take at the very least some days (but probably more) to be effective, even if I go and lose some time opening a PR myself to be quicker (because we have to wait until it is merged, if it is, then to be cut into a release, published into the play store, etc, and it's FOSS, so it's not as if we can just demand people to work because we're needing it), and would probably first be deployed as a beta release, so in everyone's phones they would have to enter the beta in the play store and update, etc, and that the other solution was more instantaneous but would involve doing something not very recommended that would be easy to break with any update of Ruby's version.

Now I'm waiting for their response and that will be the difference between this being considerably urgent and not urgent at all, haha.

I believe, though, that they will not want to update all of their teams phones just for that and will opt for the monkey-patch solution. I opened this issue more because I wanted to raise awareness to this (understandably not very critical) problem and less because I needed it fixed right away or anything like that, so there's no harm in waiting to properly fix this with that modernization of the networking code you mentioned. :grin:

[breakbusyloop]

@mateusmedeiros @lognaturel
We should look into using an existing implementation for managing headers, since there are other concerns besides case sensitivity we may encounter (e.g. non ASCII characters in header, trailing whitespace in value field, etc.). It looks like we could either use okhttp3.Headers or com.google.api.client.http.HTTPHeaders. There's also a Sun implementation, but I assume there's good reason to avoid Oracle/Sun in Android apps (I'm new to Android, so I don't know how Google's fight with Oracle has played out in the Android space).

The okhttp3 implementation seems to do more field validation and preserves header case (while still supporting case-insensitive lookup). The implementation also stores key/value pairs as adjacent entries in a simple array (yikes!?), making lookup O(n) instead of O(log n). This likely isn't a problem for a small number of headers or a small number of lookup operations, but I'm still not entirely comfortable with this approach.

The Google client implementation does almost no key/value validation and looks up by lower case only (making the client store everything in lower case). It looks like a less attractive solution, but does support O(log n) lookups.

[lognaturel]

Good point. I have a slight preference for the okhttp3 implementation because it feels more consistent across the networking layer.

The implementation also stores key/value pairs as adjacent entries in a simple array (yikes!?)

This certainly doesn't sound like the best data structure but given that there are only ~40 standard header keys defined, I think O(n) lookup is totally fine. One thing you could consider doing is exploring whether an upstream change in okhttp3 would be welcome.