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

Parse X-Forwarded-Port header even if X-Forwarded-Host is not present #2761

Merged
merged 3 commits into from
Apr 4, 2023

Conversation

pderop
Copy link
Contributor

@pderop pderop commented Apr 3, 2023

Always takes into account any available X-Forwarded-Port header even if the X-Forwarded-Host header is missing.

Fixes #2751

@pderop pderop added the type/bug A general bug label Apr 3, 2023
@pderop pderop added this to the 1.0.31 milestone Apr 3, 2023
@pderop pderop self-assigned this Apr 3, 2023
@pderop pderop requested a review from a team April 4, 2023 05:30
@violetagg
Copy link
Member

violetagg commented Apr 4, 2023

@pderop What do you think about this? We need to recreate the host information only if we are able to parse the port.

String hostHeader = request.headers().get(X_FORWARDED_HOST_HEADER);
if (hostHeader != null) {
	connectionInfo = connectionInfo.withHostAddress(
			AddressUtils.parseAddress(hostHeader.split(",", 2)[0].trim(),
					getDefaultHostPort(connectionInfo), DEFAULT_FORWARDED_HEADER_VALIDATION));
}

String portHeader = request.headers().get(X_FORWARDED_PORT_HEADER);
if (portHeader != null && !portHeader.isEmpty()) {
	String portStr = portHeader.split(",", 2)[0].trim();
	if (portStr.chars().allMatch(Character::isDigit)) {
		int port = Integer.parseInt(portStr);
		connectionInfo = new ConnectionInfo(
				AddressUtils.createUnresolved(connectionInfo.getHostAddress().getHostString(), port),
				connectionInfo.getHostName(), port, connectionInfo.getRemoteAddress(), connectionInfo.getScheme());
	}
	else if (DEFAULT_FORWARDED_HEADER_VALIDATION) {
		throw new IllegalArgumentException("Failed to parse a port from " + portHeader);
	}
}

clientRequestHeaders.add("X-Forwarded-Proto", "https");
},
serverRequest -> {
Assertions.assertThat(serverRequest.remoteAddress().getHostString()).isEqualTo("192.168.0.1");
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Let's check also the host address as we do in the rest of the tests

Suggested change
Assertions.assertThat(serverRequest.remoteAddress().getHostString()).isEqualTo("192.168.0.1");
Assertions.assertThat(serverRequest.remoteAddress().getHostString()).isEqualTo("192.168.0.1");
Assertions.assertThat(serverRequest.hostAddress().getHostString())
.containsPattern("^0:0:0:0:0:0:0:1(%\\w*)?|127.0.0.1$");
Assertions.assertThat(serverRequest.hostAddress().getPort()).isEqualTo(8443);

@pderop
Copy link
Contributor Author

pderop commented Apr 4, 2023

I have Applied the two feedbacks. Can you recheck please ?

Indeed, recreating the host information only if we are able to parse X-Forwarded-Port has the following benefit: if X-Forwarded-Host is present and contains a port and if X-Forwarded-Port is present but is malformed, then now we do not ignore the X-Forwarded-Host port (if it was present).

@pderop
Copy link
Contributor Author

pderop commented Apr 4, 2023

@violetagg , thanks for the review.

@pderop pderop merged commit 77621ce into reactor:1.0.x Apr 4, 2023
pderop added a commit that referenced this pull request Apr 4, 2023
@violetagg violetagg changed the title Parse X-Forwarded-Port header even if X-Forwarded-Host is not present Parse X-Forwarded-Port header even if X-Forwarded-Host is not present Apr 4, 2023
pderop added a commit that referenced this pull request Apr 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