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

HTTP header field names should be case insensitive #796

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

lu4p
Copy link

@lu4p lu4p commented Dec 30, 2024

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

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 <[email protected]>
@lu4p lu4p changed the title HTTP header field names should be case independent HTTP header field names should be case insensitive Dec 30, 2024
Copy link
Member

@stefan-niedermann stefan-niedermann left a 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:

  1. ParsedResponse#headers is now a Headers object, not a Map
  2. 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)
  3. (This is just a guess): I believe, that a custom Gson TypeAdapter must be implemented in order to be able to use the new Headers class with Retrofit.

Feedback to the PR itself:

  • Please annotate @NonNull / @Nullable hints or implement a guard to ensure null safety, especially when calling .toLowerCase()
  • The Headers class could / should be a record (or implement equals, 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 :)

@tobiasKaminsky
Copy link
Member

I would like to avoid a breaking change, as for any 3rd party app the functionality stays the same.
I think the same can be achieved without changing away from Map.

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants