Add option to ignore SERVER_PORT getting added to url.

[malc0mn]

Nowadays, running a site behind a reverse proxy is extremely common. When you are behind a reverse proxy, you cannot be sure that whatever is serving PHP is on a standard port (80 or 443, see #572). It can easily be 8080 or whatever is available/suitable for the hoster.

Blindly appending the port to the url in that case is annoying for developers when going through the logs.

An option to ignore the server port would help out immensly, so this pull request proposes to add ignore_server_port as an option defaulting to false to keep the current behavior.

[stayallive]

I am not to sure about this since nginx/haproxy/php-fpm/whatever can (and probably should) be configured so the correct server port and IP trickles down to the application.

See for example: https://serverfault.com/a/256205

However I am not against it, it's just adding another option 😄

What do you thing @Jean85?

[Jean85]

The X-Forwarded headers should be used for this exact purposes, but I'm not an expert on those matters...

[malc0mn]

True indeed! I guess the X-Forwarded-Port header should have been used in #572 from the start ;-)

So: also taking into account the X-Forwarded-Port header when appending the port to the URL.

I still think the ignore_server_port should be kept in. Speaking from experience, my current job is in hosting, we see lots of funky setups pass by that you can very easily "support" in Sentry when it's possible to ignore the server port: if the X-Forwarded-Port header is not set when behind a proxy, and let's be honest: a simple PHP setup does not have much use for it anyhow, it'll all crumble down and you end up with "wrong" URLs in Sentry...

[stayallive]

@malc0mn, no it should not since those headers can be set by anyone, including your end-users. This is why we're also not using the IP that comes in the X-Forwarded-IP header for example (#594). These should be validated correctly and that is (way) out of scope for the Sentry library.

I think I'm fine adding the option, however I still believe the best way to resolve this is on the webserver/proxy configuration. But that's another discussion 😄

So if you could remove the X_Forward commit I can merge the original PR 😉

[malc0mn]

Okay, removed the last commit.
Edit: not sure if there are docs yet to be updated to describe this new option?

[Jean85]

I'm fine with this since we scrapped the headers part

[stayallive]

Thanks @malc0mn!

[malc0mn]

You're welcome!