Skip to content

Default forwarded header handler should only using ConnectionInfo public API#2789

Merged
pderop merged 7 commits intoreactor:1.0.xfrom
pderop:1.0.x-defaultforwardedheaderhandler-must-not-use-private-api
May 2, 2023
Merged

Default forwarded header handler should only using ConnectionInfo public API#2789
pderop merged 7 commits intoreactor:1.0.xfrom
pderop:1.0.x-defaultforwardedheaderhandler-must-not-use-private-api

Conversation

@pderop
Copy link
Copy Markdown
Contributor

@pderop pderop commented Apr 27, 2023

Motivations:

reactor.netty.http.server.DefaultHttpForwardedHeaderHandler should use only the reactor.netty.http.server.ConnectionInfo public API if we want the custom parsers to have the same information as us. Now, in #2761 PR , the fix done in the DefaultHttpForwardedHeaderHandler method is using some private constructors from the ConnectionInfo when parsing a X-Forwarded-Port header. This breaks the contract because people cannot then do the same using their own custom handlers.

Related to #2751

Modifications:

  • Introduced a new withHostAddress(InetSocketAddress hostAddress, String hostName, int hostPort) method in ConnectionInfo, and the DefaultHttpForwardedHeaderHandler is now using it.
  • DefaultHttpForwardedHeaderHandler is still using ConnectionInfo.getHostName() , which is not public but people can implement their own logic in order to obtain the host name.
  • a new CustomXForwardedHeadersHandler test class with a package that is different than reactor.netty.http.server has been introduced and is meant to check that people can implement the same logic for parsinx X-Forwarded headers.
  • All xForwarded* tests are now run with both DefaultHttpForwardedHeaderHandler as well as with new CustomXForwardedHeadersHandler.

@pderop pderop added the type/bug A general bug label Apr 27, 2023
@pderop pderop added this to the 1.0.32 milestone Apr 27, 2023
@pderop pderop self-assigned this Apr 27, 2023
@pderop pderop requested a review from violetagg April 27, 2023 16:13
pderop added 2 commits April 28, 2023 15:29
…(InetSocketAddress hostAddress, String hostName, int hostPort); fixed DefaultHttpForwardedHeaderHandler using new withHostAddress method; all xForwarded tests are run with default header handler, as well aswith new CustomXForwardedHeadersHandler class.
@pderop
Copy link
Copy Markdown
Contributor Author

pderop commented May 2, 2023

@violetagg ,

can you retake a look ?

So, have added public visibility to ConnectionInfo getHostName/getHostPort/getDefaultHostPort: it will simplify life of people if they want to implement their own forwarded handler logic.

@pderop pderop merged commit 295d961 into reactor:1.0.x May 2, 2023
@pderop pderop deleted the 1.0.x-defaultforwardedheaderhandler-must-not-use-private-api branch May 2, 2023 12:20
@pderop
Copy link
Copy Markdown
Contributor Author

pderop commented May 2, 2023

@violetagg , thanks for the review !

pderop added a commit that referenced this pull request May 2, 2023
pderop added a commit that referenced this pull request May 2, 2023
@violetagg violetagg changed the title Default forwarded header handler should only using ConnectionInfo public API Default forwarded header handler should only using ConnectionInfo public API May 4, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

type/bug A general bug

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants