Handle ip behind proxies #914

michield · 2022-12-13T20:56:20Z

Description

handle proxies better

Related Issue

Screenshots (if appropriate):

…nues to work behind a proxy

michield · 2022-12-13T21:27:27Z

I think most of these were already done before. Just the one commit let.

bramley · 2022-12-18T09:27:52Z

The function hostname() seems to be used in only a few places when constructing a URL. Elsewhere the global variable $website which is taken from getConfig('website') is used for that.
Will this change use X_FORWARDED_FOR everywhere it is required?

michield · 2022-12-19T19:34:47Z

You're right. We should the hostname() here, so that it initialises the website and domain config settings correctly

https://github.com/phpList/phplist3/blob/main/public_html/lists/admin/connect.php#L21

michield · 2022-12-21T21:46:47Z

Not only that, I'm using the wrong value. I should use HTTP_X_FORWARDED_HOST or HTTP_X_FORWARDED_SERVER

HTTP_X_FORWARDED_FOR is the IP address of the user.

…detection and hard code the admin and frontend locations.

michield · 2023-07-27T09:35:27Z

Ok. I've decided that "auto detection" and config is too complicated and open to massive errors. Instead I've now introduced "ADMIN_WWWROOT" and "USER_WWWROOT" so that the URLs can be hard coded.

This is particularly relevant in eg Docker environments, where the Proxy container may forward to the phpList container and phplist runs locally from eg "http://phplist/lists" but the frontend URL is something like https://website.com/newsletter/

michield · 2023-07-27T09:52:01Z

The CI is failing, I'll find out why

…able yet

michield · 2023-07-30T11:59:56Z

ok, good we have CI. There were some errors, but that's fixed now.

I may add some tests with these values set in the config file as well.

bramley · 2023-08-10T07:26:51Z

~~I'm not sure that I understand the problem this is solving. Is it that admin access is on a different host/path than public access?~~

~~From your earlier comment~~

~~public access, i.e. subscribes, link clicks, etc is on https://website.com/newsletter/~~
~~but admin access is on http://phplist/lists/admin~~

~~What is stopping admin access using https://website.com/newsletter/admin ?~~

Ignore the above as I have now seen the explanation in config_extended.php

The example given has the same path (newsletter) for both. Is that just the example, or are you intending to allow the paths to differ?

#define('ADMIN_WWWROOT','https://admin.mydomain.com:8080/newsletter/admin');
#define('USER_WWWROOT','https://mydomain.com/newsletter');

In an earlier comment there is an example where one uses newsletter and the other uses lists

michield · 2023-08-12T08:59:33Z

In an earlier comment there is an example where one uses newsletter and the other uses lists

Yes, the idea is that in a proxy setup (eg Docker), you can have /newsletter/ as the public location, but it maps to /lists/ inside the phpList container. Additionally for security, you could map the admin pages via a different proxy, which is not public. So, it's useful to allow a mix of locations.

bramley · 2023-08-14T07:33:29Z

I have no experience of what you are describing, but if it is required then isn't there an external way to achieve it, using apache rewriting or some other way of mapping one to the other? What do other packages, such as Wordpress or Drupal, do to achieve this?

The code changes, although individually simple, are very widespread. I'd be inclined to not include this in the next release but continue with more testing by installing in one of your own environments that need the new feature.

bramley

There are a number of global variables or settings that now become ambiguous such as
$pageroot, $adminpages, [WEBSITE] and $website, [DOMAIN] and $domain,

I think you need to try to remove use of all those by setting new variables that are independent of whether the new defines are being used.

bramley · 2023-08-10T13:34:27Z

public_html/lists/admin/actions/import2.php

Suggest create a global variable $adminBaseUrl similar to $publicBaseUrl so that this and several others can be simplified to

$history_entry = $adminBaseUrl.'/?page=user&id='.$userid."\n\n";

bramley · 2023-08-10T13:55:57Z

public_html/lists/admin/actions/processqueue.php

$progressUrl = $GLOBALS['admin_scheme'].'://'.hostName().$GLOBALS['adminpages'].'/?page=messages&tab=active';

Not sure why hostName() is used here but it can get replaced by using a new variable $adminBaseUrl

bramley · 2023-08-12T06:46:39Z

public_html/lists/admin/sendemaillib.php

$clicktrack_root = sprintf('%s://%s/lt.php', $GLOBALS['public_scheme'], $website.$GLOBALS['pageroot']);

This needs to change to use $publicBaseUrl .

bramley · 2023-08-12T06:56:13Z

public_html/lists/admin/lib.php

@@ -403,7 +403,11 @@ function sendAdminPasswordToken($adminId)
    $emailBody = $GLOBALS['I18N']->get('Hello').' '.$adminName."\n\n";
    $emailBody .= $GLOBALS['I18N']->get('You have requested a new password for phpList.')."\n\n";
    $emailBody .= $GLOBALS['I18N']->get('To enter a new one, please visit the following link:')."\n\n";
-    $emailBody .= sprintf('%s://%s/?page=login&token=%s', $GLOBALS['admin_scheme'], $urlroot, $key)."\n\n";
+    if (defined('ADMIN_WWWROOT')) {


This is the wrong way around, but should be replaced by using a single variable.

bramley · 2023-08-12T06:56:50Z

public_html/lists/admin/processbounces.php

@@ -580,7 +580,11 @@ function processMessages($link, $max = 3000)
                if ($row['user']) {
                    $userdata = Sql_Fetch_Array_Query("select * from {$tables['user']} where id = ".$row['user']);
                }
-                $report_linkroot = $GLOBALS['admin_scheme'].'://'.$GLOBALS['website'].$GLOBALS['adminpages'];
+                if (defined('ADMIN_WWWROOT')) {


This one too is the wrong way around.

bramley · 2023-08-12T13:46:07Z

public_html/lists/admin/defaultconfig.php

+if (defined('USER_WWWROOT')) {
+  $domainParts = parse_url(USER_WWWROOT);
+  $D_website = $domainParts['host'];
+  if ($domainParts['port'] != 80 && $domainParts['port'] != 443) {


$domainParts['port'] will be set only if a port was in the URL. But it seems simpler just to include whatever is in the URL, even if is one of the standard port numbers

if (isset($domainParts['port')) { $D_website .= ":".$domainParts['port']; }

bramley · 2023-08-12T13:47:11Z

public_html/lists/admin/index.php

@@ -708,7 +700,7 @@ function mb_strtolower($string)
 }

 if (WARN_ABOUT_PHP_SETTINGS && !$GLOBALS['commandline']) {
-    if (strpos(getenv('REQUEST_URI'), $pageroot.'/admin') !== 0) {
+    if (!defined('USER_WWWROOT') && strpos(getenv('REQUEST_URI'), $pageroot.'/admin') !== 0) {


if (!defined('USER_WWWROOT') && strpos(getenv('REQUEST_URI'), $pageroot.'/admin') !== 0) {

This test is of the admin URL not the public URL, but should it still be a valid test when ADMIN_WWWROOT is used?

When using the new USER_WWWROOT and ADMIN_WWWROOT Which value should $pageroot in config.php be set to?

bramley · 2023-08-12T13:48:23Z

public_html/lists/admin/init.php

    $pageroot = '/lists';
+  }
+  $publicBaseUrl = "http://[WEBSITE]$pageroot";


This doesn't work for creating link tracking URLs because the variable doesn't have placeholders replaced

$publicBaseUrl = "http://[WEBSITE]$pageroot";

It does work for creating the user tracking image URL because the img element is added to the message before placeholders are replaced.

I suggest moving the handling of USER_WWWROOT to a point where getConfig() can be called to get the WEBSITE. Maybe at the start of defaultconfig.php, where a similar $adminBaseUrl can be set.

It should use the public scheme which is set earlier $public_scheme

bramley · 2023-08-12T13:49:04Z

public_html/lists/admin/init.php

-    if ($pageroot == '/') {
-        $pageroot = '';
-    }
+if (defined('USER_WWWROOT')) {


if (defined('USER_WWWROOT')) { $pageroot = USER_WWWROOT; $publicBaseUrl = USER_WWWROOT; } else {

$pageroot is only the path, not the whole URL. But I am not sure that is should be used, as there is ambiguity as to which path is meant - the public or admin. Hiding it with the $publicBaseUrl and $adminBaseUrl variables should mean that it is no longer used.

bramley · 2023-08-13T18:11:02Z

public_html/lists/admin/init.php

 }
+
 // as the "admin" in adminpages is hardcoded, don't put it in the config file
 $adminpages = $GLOBALS['pageroot'].'/admin';


This is using $pageroot taken from the public URL but it is used to create URLs for admin pages. Related to the previous comment.

michield · 2023-08-15T07:01:21Z

I have no experience of what you are describing, but if it is required then isn't there an external way to achieve it, using apache rewriting or some other way of mapping one to the other? What do other packages, such as Wordpress or Drupal, do to achieve this?

The code changes, although individually simple, are very widespread. I'd be inclined to not include this in the next release but continue with more testing by installing in one of your own environments that need the new feature.

Ok. thanks for the review. I will go through the suggestions and check in the next few days.

I don't know Wordpress or Drupal, but I do know Moodle, which has a global CFG variable where it hardcodes the URL of the system, and it doesn't work without that set.

It's just very tricky to "detect" the URL environment, as there are many options. phpList currently does that, but it causes a lot of issues when in a proxied environment. Maybe we also need to remove the "website" and "domain" config variables, and force them to be set in the config file, but that may have a major impact on existing systems.

michield · 2023-08-15T20:15:44Z

Thanks, I've reviewed your suggestions and I think the idea of an adminBaseUrl is good.

The main complication is this:

publicBaseUrl is set and then used to pre-populate the default config. So, users can then change it and the final value lives in the DB. It defaults as something like "https://[WEBSITE]/lists/?p=subscribe" etc, and then [WEBSITE] is replaced on the fly.
If someone changes that in the config then the DB config is taken instead.

I think we need to remove this. There's no point in having it editable, as it's application specific. The only one that could be editable is the "subscribe page" so that people can point it at their own website for example.

What do you think. The result would be:

publicBaseUrl: set with current "detection" of the location
adminBaseUrl: $publicBaseUrl + /admin
use the new constants to deviate from it
allow "subscribe URL" to be editable, but remove the [WEBSITE] stuff
remove all other "URL configs" and hard-code their location based on the publicBaseUrl

bramley · 2023-08-17T08:05:13Z

I don't know. The changes just seem to be getting larger. I'm away for a few days so won't be able to look at anything now.

bramley · 2023-08-21T12:58:08Z

publicBaseUrl: set with current "detection" of the location

adminBaseUrl: $publicBaseUrl + /admin

use the new constants to deviate from it

I think this is fine. Can the website and subscribe page changes wait until another release? The new defines can be explained that they override all other settings such as the [WEBSITE] setting and $pageroot in config.php.

What will happen to globals such as $website, $systemroot, $public_scheme etc? I think that these need to remain because they are used by some plugins, such as CKEditor, so will need to be derived when the new defines are used.

michield · 2023-08-21T19:53:18Z

I think this is fine. Can the website and subscribe page changes wait until another release? The new defines can be explained that they override all other settings such as the [WEBSITE] setting and $pageroot in config.php.

Well, the issue is the two-step approach we use at the moment, which needs to go:

set URL configs to default
allow editing them in the UI
use the DB values

With the new constants step 3 would have to change to (a) from DB or (b) from constant, so that's more code changes than changing it in step 1.

But I'll give it a try.

What will happen to globals such as $website, $systemroot, $public_scheme etc? I think that these need to remain because they are used by some plugins, such as CKEditor, so will need to be derived when the new defines are used.

Yes, we can easily parse those out of the new constants with url_parse.

I'll add that to my PR.

…tched for application URLs

michield · 2023-08-28T20:31:36Z

Ok, I think I have addressed all the issues, but we may need to do an extensive test if this is in an RC.

publicBaseUrl
publicConfigBaseUrl -> to initialise the config, but leave it at that
adminBaseUrl
set public_schema and website when the constants are in use
systemroot is not affected here

michield · 2023-08-28T20:41:06Z

Hmm the CI fails, but I think that's not the code, but Github playing up

michield · 2023-08-30T21:46:42Z

Hmm the CI fails, but I think that's not the code, but Github playing up

I re-started the actions and they pass now

bramley · 2023-08-31T10:17:51Z

A couple of my previous comments were missed, and a few more.

In init.php

if (defined('USER_WWWROOT')) {
  $pageroot = USER_WWWROOT;
  $publicConfigBaseUrl = USER_WWWROOT;

$pageroot needs to be the path such as /lists

In sendemaillib.php there are a few lines that should be changed to use the $publicBaseUrl variable

$clicktrack_root = sprintf('%s://%s/lt.php', $GLOBALS['public_scheme'], $website.$GLOBALS['pageroot']);

$masked .= '&hm='.hash_hmac(HASH_ALGO, sprintf('%s://%s/lt.php?tid=%s', $GLOBALS['public_scheme'], $website.$GLOBALS['pageroot'], $masked), HMACKEY);

$viewurl = $GLOBALS['public_scheme'].'://'.$website.$GLOBALS['pageroot'].'/dl.php?id='.$att['id'];

If you grep for pageroot there are still many remaining uses that need to be reviewed to see whether they need changing or removing.
There is a use of $adminpages in function sendAdminPasswordToken() in file lib.php that now doesn't look correct.
```
 $urlroot = getConfig('website').$GLOBALS['adminpages'];
```

The last commit of a change to index.php omitted a '/' character

  $html .= '<p><a href="'.$GLOBALS['adminBaseUrl'].'?page=spage" class="button">'.s('Go back to admin area').'</a></p>';

Some of the code changes have been indented two characters instead of 4.
For a new installation the website config entry is set to the host of the admin URL which will be ADMIN_WWWROOT when that is being used, shouldn't it be the host of USER_WWWROOT instead?

I suggest that you test this on its own before trying to apply the changes. Can you test creating a campaign using ckeditor, as I don't know whether it will use the correct domain for image URLs.
Update - I have checked this and the plugin constructs the URL for an image like this, which I think is correct

            $uploadUrl = sprintf('%s://%s/%s', $public_scheme, $website, ltrim(UPLOADIMAGES_DIR, '/'));

…nded

michield · 2023-09-04T19:19:39Z

Just a quick explanation on why this is needed. In Docker, or other "proxied" setups, you have one container (eg swag) at the front sending traffic to backend containers. The backend one has eg "phplist" as the Apache Host and additionally running on HTTP, because the proxy does the SSL. So, you end up with all links being http://phplist/lists/admin/ etc, instead of what the actual site runs on being https://mysite.com/lists/admin/

michield · 2023-09-04T19:24:55Z

Pageroot needs to match the value on the backend server, so I've updated that in the explanation of the value.
I've fixed the init.php and sendemaillib.php
the urlroot line in sendAdminPasswordToken is obsolete.
the missing / added

But yes, CKeditor file upload stopped working. I will investigate why.

Correction, the images folder wasn't readable.

michield · 2023-09-04T20:09:50Z

Hmm, but it's true, CKeditor doesn't work. It throws this error https://github.com/bramley/phplist-plugin-ckeditor/blob/master/plugins/CKEditorPlugin/kcfinder/core/class/uploader.php#L251

michield · 2023-09-04T20:19:07Z

Ok, that's something local. It also happens with the "main" branch or the phplist-3.6.13 branch.

Considering this change, I think we should number the next one 3.7.0 and tell people to wait with upgrading.

I think we postpone this one, and get all the other PRs out, including #986 which is a security issue.

michield added 7 commits July 10, 2021 16:45

move IP detection function to more global location

fc33876

replace (almost) all REMOTE_ADDR with getClientIP(), so that it conti…

98ad57f

…nues to work behind a proxy

Merge branch 'master' into handle_IP_behind_proxies

66bf671

cache the ip address found

8676f77

tidy up

02faef6

detect proxy in hostname

dcff53c

Merge branch 'main' into handle_IP_behind_proxies

bb458e6

michield marked this pull request as ready for review December 13, 2022 21:27

michield added the work in progress label Dec 21, 2022

Allow config to set ADMIN_WWWROOT and USER_WWWROOT to bypass the URL …

27c00fb

…detection and hard code the admin and frontend locations.

michield removed the work in progress label Jul 27, 2023

michield added 2 commits July 27, 2023 10:36

Merge branch 'main' into handle_IP_behind_proxies

3771d43

add section to config_extended about the new values

a26eccf

michield added the work in progress label Jul 27, 2023

michield added 2 commits July 30, 2023 12:11

fix publicBaseUrl initialisation, as functions and vars are not avail…

0b472cd

…able yet

default to http:// to make CI pass

e5a17ad

michield removed the work in progress label Jul 30, 2023

michield requested a review from bramley August 9, 2023 19:41

bramley reviewed Aug 14, 2023

View reviewed changes

michield added 5 commits August 28, 2023 21:25

define new global vars for user and admin base urls

b7e3265

keep the URL config method as before, but override the config when fe…

d853540

…tched for application URLs

use new global adminBaseUrl

da74e1a

use intermediate config var for initialisation of the config URLs

66a1a78

use new global adminBaseUrl

7a36207

michield added 2 commits September 4, 2023 20:13

update pageroot only when not set, and add explanation in config_exte…

6cbfdec

…nded

fix some more links with the new global var

a1e9bc3

michield added 2 commits September 4, 2023 20:22

remove obsolete line

0b36b40

fix typo

0dadc39

michield added the 3.7.x PRs for 3.7 label Sep 9, 2023

michield mentioned this pull request Sep 17, 2023

phpList 3.7 #989

Open

michield mentioned this pull request Jan 20, 2024

start with alpine to reduce the image size phpList/phplist-docker#11

Open

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Handle ip behind proxies #914

Handle ip behind proxies #914

michield commented Dec 13, 2022

michield commented Dec 13, 2022

bramley commented Dec 18, 2022

michield commented Dec 19, 2022

michield commented Dec 21, 2022

michield commented Jul 27, 2023

michield commented Jul 27, 2023

michield commented Jul 30, 2023

bramley commented Aug 10, 2023 •

edited

michield commented Aug 12, 2023

bramley commented Aug 14, 2023 •

edited

bramley left a comment

bramley Aug 10, 2023

bramley Aug 10, 2023

bramley Aug 12, 2023

bramley Aug 12, 2023

bramley Aug 12, 2023

bramley Aug 12, 2023

bramley Aug 12, 2023

bramley Aug 12, 2023

bramley Aug 12, 2023

bramley Aug 13, 2023

michield commented Aug 15, 2023

michield commented Aug 15, 2023

bramley commented Aug 17, 2023

bramley commented Aug 21, 2023

michield commented Aug 21, 2023

michield commented Aug 28, 2023

michield commented Aug 28, 2023

michield commented Aug 30, 2023

bramley commented Aug 31, 2023 •

edited

michield commented Sep 4, 2023

michield commented Sep 4, 2023 •

edited

michield commented Sep 4, 2023

michield commented Sep 4, 2023

Handle ip behind proxies #914

Are you sure you want to change the base?

Handle ip behind proxies #914

Conversation

michield commented Dec 13, 2022

Description

Related Issue

Screenshots (if appropriate):

michield commented Dec 13, 2022

bramley commented Dec 18, 2022

michield commented Dec 19, 2022

michield commented Dec 21, 2022

michield commented Jul 27, 2023

michield commented Jul 27, 2023

michield commented Jul 30, 2023

bramley commented Aug 10, 2023 • edited

michield commented Aug 12, 2023

bramley commented Aug 14, 2023 • edited

bramley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

michield commented Aug 15, 2023

michield commented Aug 15, 2023

bramley commented Aug 17, 2023

bramley commented Aug 21, 2023

michield commented Aug 21, 2023

michield commented Aug 28, 2023

michield commented Aug 28, 2023

michield commented Aug 30, 2023

bramley commented Aug 31, 2023 • edited

michield commented Sep 4, 2023

michield commented Sep 4, 2023 • edited

michield commented Sep 4, 2023

michield commented Sep 4, 2023

bramley commented Aug 10, 2023 •

edited

bramley commented Aug 14, 2023 •

edited

bramley commented Aug 31, 2023 •

edited

michield commented Sep 4, 2023 •

edited