Skip to content

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

Merged
pderop merged 3 commits intoreactor:1.0.xfrom
pderop:1.0.x-issue-2751
Apr 4, 2023
Merged

Parse X-Forwarded-Port header even if X-Forwarded-Host is not present#2761
pderop merged 3 commits intoreactor:1.0.xfrom
pderop:1.0.x-issue-2751

Conversation

@pderop
Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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