Skip to content

Conversation

@malc0mn
Copy link
Contributor

@malc0mn malc0mn commented Jul 11, 2018

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
Copy link
Collaborator

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?

@stayallive stayallive requested a review from Jean85 July 13, 2018 09:44
@Jean85
Copy link
Contributor

Jean85 commented Jul 13, 2018

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

@malc0mn
Copy link
Contributor Author

malc0mn commented Jul 16, 2018

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
Copy link
Collaborator

@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
Copy link
Contributor Author

malc0mn commented Jul 16, 2018

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

Copy link
Contributor

@Jean85 Jean85 left a comment

Choose a reason for hiding this comment

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

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

@stayallive
Copy link
Collaborator

Thanks @malc0mn!

@stayallive stayallive merged commit d574017 into getsentry:master Jul 17, 2018
@malc0mn
Copy link
Contributor Author

malc0mn commented Jul 18, 2018

You're welcome!

@Jean85 Jean85 mentioned this pull request Aug 3, 2018
11 tasks
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants