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

Fix redirect on login for instances behind reverse proxies #6371

Merged
merged 4 commits into from Apr 6, 2017

Conversation

Projects
None yet
6 participants
@putnam
Contributor

putnam commented Apr 6, 2017

On instances where base_url has been set for use behind a reverse proxy, logins are incorrectly redirected.

This happens because REQUEST_URI is set by the proxy:

  1. librenms has base_url set to http://site.com/nms/
  2. Browser requests http://site.com/nms/
  3. nginx reverse proxies /nms/ to librenms at http://somehost:1234/
  4. librenms sees REQUEST_URI as "/"
  5. librenms logs the user in, but sends "Location: /" to the browser. This redirects to the wrong location. Similar behavior occurs if REQUEST_URI points to some resource within librenms.

To resolve, concatenate REQUEST_URI (which is relative) to base_url. As base_url is slash-terminated, crop the trailing slash. This should have no effect on users with default settings and will correctly redirect instances behind reverse proxies.

DO NOT DELETE THIS TEXT

Please note

Please read this information carefully. You can run ./scripts/pre-commit.php to check your code before submitting.

  • Have you signed the Contributors agreement - please do NOT submit a pull request unless you have (signing the agreement in the same pull request is fine). Your commit message for signing the agreement must appear as per the docs.
  • Have you followed our code guidelines?

Testers

If you would like to test this pull request then please run: ./scripts/github-apply <pr_id>, i.e ./scripts/github-apply 5926

putnam added some commits Apr 6, 2017

Fix redirect on login for instances
On instances where base_url has been set for use behind a reverse proxy, logins are incorrectly redirected.

This happens because REQUEST_URI is set by the proxy:
  1. librenms has base_url set to http://site.com/nms/
  2. Browser requests http://site.com/nms/
  3. nginx reverse proxies /nms/ to librenms at http://somehost:1234/
  4. librenms sees REQUEST_URI as "/"
  5. librenms logs the user in, but sends "Location: /" to the browser. This redirects to the wrong location.

To resolve, concatenate REQUEST_URI (which is relative) to base_url. As base_url is slash-terminated, crop the trailing slash. This should have no effect on users with default settings and will correctly redirect instances behind reverse proxies.
@mention-bot

This comment has been minimized.

Show comment
Hide comment
@mention-bot

mention-bot Apr 6, 2017

Thank you for submitting a PR @putnam! We have found the following @laf, @murrant and @ekoyle based on the history of these files to review this PR.

Thank you for submitting a PR @putnam! We have found the following @laf, @murrant and @ekoyle based on the history of these files to review this PR.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
Show outdated Hide outdated html/includes/authenticate.inc.php
@@ -96,7 +96,8 @@
$permissions = permissions_cache($_SESSION['user_id']);
if (isset($_POST['username'])) {
header('Location: '.$_SERVER['REQUEST_URI'] ?: $config['base_url'], true, 303);
// Trim the trailing slash off of base_url and concatenate the (relative) REQUEST_URI
header('Location: '.substr($config['base_url'], 0, -1).$_SERVER['REQUEST_URI'], true, 303);

This comment has been minimized.

@f0o

f0o Apr 6, 2017

Member

This variable does not always have a slash at the end.
Please add a sanity check before cutting off a character from the base_url; worst case you kill the tld.

@f0o

f0o Apr 6, 2017

Member

This variable does not always have a slash at the end.
Please add a sanity check before cutting off a character from the base_url; worst case you kill the tld.

@laf

This comment has been minimized.

Show comment
Hide comment
@laf

laf Apr 6, 2017

Member

Probably easier to just do something like rtrim($val, '/');

Member

laf commented Apr 6, 2017

Probably easier to just do something like rtrim($val, '/');

@f0o

f0o approved these changes Apr 6, 2017

@putnam

This comment has been minimized.

Show comment
Hide comment
@putnam

putnam Apr 6, 2017

Contributor

Good catch. I didn't realize rtrim had that character mask param. Looks like that is both cleaner and solves the use case where there's no trailing slash.

Contributor

putnam commented Apr 6, 2017

Good catch. I didn't realize rtrim had that character mask param. Looks like that is both cleaner and solves the use case where there's no trailing slash.

@LibreNMS-CI

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

This comment has been minimized.

Show comment
Hide comment
@scrutinizer-notifier

scrutinizer-notifier Apr 6, 2017

The inspection completed: No new issues

The inspection completed: No new issues

@f0o f0o merged commit 0201706 into librenms:master Apr 6, 2017

2 checks passed

Auto-Deploy Build finished.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

murrant added a commit to murrant/librenms that referenced this pull request Apr 7, 2017

Fix redirect on login for instances behind reverse proxies (#6371)
* Fix redirect on login for instances

On instances where base_url has been set for use behind a reverse proxy, logins are incorrectly redirected.

This happens because REQUEST_URI is set by the proxy:
  1. librenms has base_url set to http://site.com/nms/
  2. Browser requests http://site.com/nms/
  3. nginx reverse proxies /nms/ to librenms at http://somehost:1234/
  4. librenms sees REQUEST_URI as "/"
  5. librenms logs the user in, but sends "Location: /" to the browser. This redirects to the wrong location.

To resolve, concatenate REQUEST_URI (which is relative) to base_url. As base_url is slash-terminated, crop the trailing slash. This should have no effect on users with default settings and will correctly redirect instances behind reverse proxies.

* I agree to the conditions of the Contributor Agreement contained in doc/General/Contributing.md.

* Adding comment explaining redirect logic on login

* Use rtrim instead of substr

@lock lock bot locked as resolved and limited conversation to collaborators May 19, 2018

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