-
Notifications
You must be signed in to change notification settings - Fork 34
HTTP header field names should be case insensitive #796
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
Conversation
See https://www.rfc-editor.org/rfc/rfc2616#section-4.2 In the nextcloud notes android app relying on cased headers lead to the app always downloading every note ever written if a reverse proxy automatically converted all proxied headers to lowercase (cloudflare for example does this). See nextcloud/notes-android#2531 Signed-off-by: Paul Scheduikat <lu4p@pm.me>
stefan-niedermann
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ringing in @tobiasKaminsky and @AndyScherzinger as maintainers and Nextcloud GmbH employees.
Background knowledge:
HTTP Headers are currently passed through directly. According to the corresponding RFC:
Field names are case-insensitive.
This PR aims to lower case all response header names before forwarding the headers to the 3rd party app, so 3rd party apps don't have to think about case sensitivity.
This PR introduces two / three breaking changes:
ParsedResponse#headersis now aHeadersobject, not aMap- This makes all header name fields lowercase (by intention), which is RFC compliant, but still a breaking change for the SSO library (note, that other 3rd party apps may rely on case sensitivity even if this is not RFC compliant)
- (This is just a guess): I believe, that a custom
GsonTypeAdaptermust be implemented in order to be able to use the newHeadersclass withRetrofit.
Feedback to the PR itself:
- Please annotate
@NonNull/@Nullablehints or implement a guard to ensurenullsafety, especially when calling.toLowerCase() - The
Headersclasscould / should be a record (or implementequals,hashCode,toString)
Personal opinion:
I am considering the SSO as a transparent library that does modify the network traffic as little as possible.
Therefore I generally would leave it up to the 3rd party app to extract headers RFC compliant and would recommend to implement this logic in the 3rd party apps themself (for example with a static util method in this lib).
@tobiasKaminsky @AndyScherzinger would like to hear your opinion
PS.: Einen guten Rutsch euch allen :)
|
I would like to avoid a breaking change, as for any 3rd party app the functionality stays the same. Regarding changing case sensitivity: Even though the lib should be as transparent as possible, I think this is a good idea, with less/no risk of breaking it. |
|
I will close this PR for now, but please feel free to update and re-open it. |
|
Notes isn't the only app that's affected by this issue, from my testing. In the official Nextcloud app, Instant Uploads are delayed by quite a while when the headers are not normalized. I normalized (lowercased) the headers with my CDN and new images are detected and uploaded in a couple minutes hands-off instead of 10 or more. There might be other instances of inefficiencies due to header mismatch. |
|
Simply changing To Should be fine but I've not managed to get a test version built. It'll still be a If anyone depends on it being case sensitive, it'll break but they never should have been doing that to start with. HTTP spec states header names are case insensitive, and HTTP/2 requires lowercase headers and considers uppercase to be invalid. |
See https://www.rfc-editor.org/rfc/rfc2616#section-4.2
In the nextcloud notes android app relying on cased headers lead to the app always downloading every note ever written if a reverse proxy automatically converted all proxied headers to lowercase (cloudflare for example does this). See nextcloud/notes-android#2531