Skip to content

Conversation

jay-choe
Copy link
Contributor

@jay-choe jay-choe commented Sep 18, 2024

Hi.

I have looked this issue and got an idea to deal with.

In current code, convert null body to not null and it write to outputstream.

In that process, when writing empty string in outputstream, unwanted Content-Type Header is added which is ''application/x-www-form-urlencoded".

So, I rearrange null body code to avoid unwanted header.

With this, Content-Length header is not set, so I added. But, this header is restricted header so 'sun.net.http.allowRestrictedHeaders' system property should be true to add content-length header.

And this property should be set before client class loaded. I guess there is no suitable way to inject restricted header to connection.

Feel Free to comment this idea. thanks.

@jay-choe jay-choe force-pushed the remove-content-type-header-with-empty-post-body branch 2 times, most recently from 30079f9 to 21ed9eb Compare September 18, 2024 15:35
@kdavisk6
Copy link
Member

kdavisk6 commented Oct 4, 2024

This look good. Could you please add some information to the README to indicate how to take advantage of this change, helping folks understand the System Property requirement?

@kdavisk6 kdavisk6 added ready to merge Will be merged if no other member ask for changes feedback provided Feedback has been provided to the author labels Oct 4, 2024
@jay-choe jay-choe force-pushed the remove-content-type-header-with-empty-post-body branch 4 times, most recently from 5757b3d to 49d5f4d Compare October 6, 2024 08:22
@jay-choe
Copy link
Contributor Author

jay-choe commented Oct 6, 2024

@kdavisk6 sure, I added it.

@velo velo force-pushed the master branch 3 times, most recently from 8b27da5 to 0f759bf Compare October 8, 2024 10:18
…Default Client).

- add Content-Length Header with 0 value when `sun.net.http.allowRestrictedHeaders` System Property is set true

- fix not running test to run

- add README for setting zero content-length header
@velo velo force-pushed the remove-content-type-header-with-empty-post-body branch from 5be2434 to e2fbea1 Compare October 8, 2024 12:20
@velo
Copy link
Member

velo commented Oct 8, 2024

@jay-choe @kdavisk6 FWIW, I have rebased this PR after the formatting changes I did on master

@velo velo merged commit 7e7940e into OpenFeign:master Oct 8, 2024
5 checks passed
@zoomout
Copy link

zoomout commented Dec 18, 2024

@velo
Please revert this change. Setting "sun.net.http.allowRestrictedHeaders" is breaking security.
restrictedHeaders include e.g. Host, enabling CSRF attack.

private static final String[] restrictedHeaders = {
    /* Restricted by XMLHttpRequest2 */
    //"Accept-Charset",
    //"Accept-Encoding",
    "Access-Control-Request-Headers",
    "Access-Control-Request-Method",
    "Connection", /* close is allowed */
    "Content-Length",
    //"Cookie",
    //"Cookie2",
    "Content-Transfer-Encoding",
    //"Date",
    //"Expect",
    "Host",
    "Keep-Alive",
    "Origin",
    // "Referer",
    // "TE",
    "Trailer",
    "Transfer-Encoding",
    "Upgrade",
    //"User-Agent",
    "Via"
};

@velo
Copy link
Member

velo commented Dec 18, 2024

Hello, good morning, how are you today?

Raise a PR with test case.

Thank you.

@evgeniycheban
Copy link

evgeniycheban commented May 7, 2025

Hi @velo, this PR turned out into a breaking change for us, currently we have some POST requests sent with no body, for example:

@PostMapping("/action/{actionId}")
String performAction(@PathVariable Long actionId);

it fails when the request goes through our gateway proxy which forces us to either send the request with an empty body
"{}" or implement an interceptor that would add "Content-Length": 0 header for POST requests with no body. I have looked at the code and this change also does not meet the initial requirement from gh-2068:

POST requests with no body should by default add Content-Length: 0 header (as described in https://github.com/OpenFeign/feign/issues/1229), but no Content-Type header.

We cannot use sun.net.http.allowRestrictedHeaders either due to environment restrictions and I also believe that setting this property could lead to potential security risks and Feign should not rely on it in order to be able to set "Content-Length": 0, instead I think a new configuration option for Feign.Builder should be introduced that would enable an old behaviour by default to restore backward compatibility, I can also provide a PR for this and to address the initial gh-2068 issue.
What do you think?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
feedback provided Feedback has been provided to the author ready to merge Will be merged if no other member ask for changes
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants