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

#1490 Add support for X-Forwarded-Port header. #1491

Merged
merged 3 commits into from May 18, 2016

Conversation

Projects
None yet
3 participants
@Mihail-K
Contributor

Mihail-K commented May 12, 2016

Honours the X-Forwarded-Port header, when present.

Show outdated Hide outdated source/vibe/http/server.d
url.schema = "https";
if (m_port != 443) url.port = m_port;
// Set URL port segment.
Nullable!ushort port;

This comment has been minimized.

@s-ludwig

s-ludwig May 18, 2016

Member

Could simply use ushort port = 0; here, because 0 isn't a valid port anyway.

@s-ludwig

s-ludwig May 18, 2016

Member

Could simply use ushort port = 0; here, because 0 isn't a valid port anyway.

Show outdated Hide outdated source/vibe/http/server.d
try {
port = xfp.to!ushort;
} catch (ConvException) {
// Value was not ushort.

This comment has been minimized.

@s-ludwig

s-ludwig May 18, 2016

Member

Another possibility would be to respond with a 400 error here. May be the better option in case of e.g. proxy configuration errors. But I'd say that there should be a diagnostic log message here at least.

@s-ludwig

s-ludwig May 18, 2016

Member

Another possibility would be to respond with a 400 error here. May be the better option in case of e.g. proxy configuration errors. But I'd say that there should be a diagnostic log message here at least.

This comment has been minimized.

@Mihail-K

Mihail-K May 18, 2016

Contributor

Alright. I've added a logger-warning in case of a bad port number. I'm hesitant to serve a 400 error from here though, considering this is in HTTPServerRequest. Should it be sending responses?

@Mihail-K

Mihail-K May 18, 2016

Contributor

Alright. I've added a logger-warning in case of a bad port number. I'm hesitant to serve a 400 error from here though, considering this is in HTTPServerRequest. Should it be sending responses?

This comment has been minimized.

@s-ludwig

s-ludwig May 18, 2016

Member

You are right, I forgot that this happens after the general request parsing, within the user's request callback.

@s-ludwig

s-ludwig May 18, 2016

Member

You are right, I forgot that this happens after the general request parsing, within the user's request callback.

@s-ludwig

This comment has been minimized.

Show comment
Hide comment
@s-ludwig

s-ludwig May 18, 2016

Member

Okay, thanks! Merging in.

Member

s-ludwig commented May 18, 2016

Okay, thanks! Merging in.

@s-ludwig s-ludwig merged commit 4c2d72a into vibe-d:master May 18, 2016

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
if (m_port != 80) url.port = m_port;
// Set URL schema segment.
if (auto xfp = this.headers.get("X-Forwarded-Proto")) {
url.schema = xfp;

This comment has been minimized.

@MartinNowak

MartinNowak Jan 1, 2018

Contributor

I'd argue this should set the port to the default port for that schema for cases where X-Forwarded-Port isn't set.

@MartinNowak

MartinNowak Jan 1, 2018

Contributor

I'd argue this should set the port to the default port for that schema for cases where X-Forwarded-Port isn't set.

This comment has been minimized.

@s-ludwig

s-ludwig Jan 2, 2018

Member

Setting no port (defaut of 0) is considered to be implicitly equal to setting the default port. Are you thinking of something in particular that is broken?

@s-ludwig

s-ludwig Jan 2, 2018

Member

Setting no port (defaut of 0) is considered to be implicitly equal to setting the default port. Are you thinking of something in particular that is broken?

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment