Please add more information to your issue. Without test instructions and/or any description we will close this issue within 4 weeks. Thanks.
This is an automated message from the J!Tracker Application.

Looks good on a quick look thanks @PhilETaylor

Drone failure unrelated to PR content :-)

So @HLeithner what's your decision on this one? Its fixing a reported and proved security issue (like the other PR you rejected) and is implementing a new feature to fix it (like the other PR you rejected)

Is this one also a waste of time?

No that's (in my eyes) a more serious issue. At the first glance I would say it's incomplete but maybe I missed it.

1. I miss the configuration for the used HTTP header
2. IP pinning (of the proxy) maybe makes sense too.

I'm also not sure about the location in the framework.php I would at least move it after the debugging/error handling.

I have had extended consultation on this, including @nikosdion and decided not to implement a trusted proxy IP whitelist or IP pinning for Joomla 3, as that WOULD be a much larger and more complex PR for a dead end of life product that no new features are allowed to be added to.

IP Pinning would be a totally new feature.

trusted proxy whitelisting would be a new feature.

Initially I was planning on implementing the whole feature, such as Symfony's implementation, but decided against it as that's a much larger new feature. Im glad I did not follow that route now.

The used headers are already a part of IpHelper. All this PR does is toggle an already badly implemented (and now changed) default in a class. This is a simple replacement for #32452 provided by the JSST.

It makes no difference to be after error handling, all this PR is doing is checking the config (which has already loaded) and reconfiguring the default in IpHelper based on the configuration.

If there is a commitment from the Joomla 4 maintainers (@wilsonge) then I will submit a Complete new PR for Joomla (instead of the current proposal) that contains the full suite of security features aka Symfony's implementation

I'm also not sure about the location in the framework.php I would at least move it after the debugging/error handling.

Done.

Ok thanks, Header selection should be done in j4 if possible in a good way later. Happy to merge this if we get tests.

The selection on which header is actually used ultimately is offloaded into the joomla-framework/utility package here:

https://github.com/joomla-framework/utilities/blob/29e39da1d16d666c1b446b343caeb6b495c57a32/src/IpHelper.php#L476

Yes I have seen this, should be configurable at least in the future because if you proxy uses HTTP_CLIENT_IP you could still fake your IP with HTTP_X_FORWARDED_FOR.

ok, but as-is, this is ready for testing. It fulfils the security issue reported to the JSST I believe.

This should be a release blocker on Joomla 3.9.26 as it fixes a reported security issue to the JSST.

I have tested this item ✅ successfully on e6c3050

The PR fixes the issue by not allowing IP override when the new option "Behind Load Balancer" is switched off (default).

When the option is switched on, IP override is still allowed (and has to be, otherwise the load balancer or similar would not work).

Tested with help of Firefox' developer tools.

Questions: Should there be a hint in release notes to switch the new option on with a clear description of when it has to be done and when not?

Questions: Should there be a hint in release notes to switch the new option on with a clear description of when it has to be done and when not?

In joomla 3 similar changes have been "documented" in post installation messages.

Questions: Should there be a hint in release notes to switch the new option on with a clear description of when it has to be done and when not?

In joomla 3 similar changes have been "documented" in post installation messages.

I could care for that, if nobody else wants. I might have to ask for advice regarding the message text, how detailed it should be. I tend to write too much, as you can see by my testing instructions in my PR's.

Remember also it’s an official security fix to a reported issue so JSST will be/should be issuing a CVE and commenting on this

Remember also it’s an official security fix to a reported issue so JSST will be/should be issuing a CVE and commenting on this

Ping @zero-24 .

@PhilETaylor I would really like to have a post installation message for this but I also want to create the RC today and richard hasn't time for it at the moment. Do you have the chance to add it? if so I wait with the RC for it.

7:36am here... I can try before lunch time

7:36am here... I can try before lunch time

8:53am here so no problem

@HLeithner @brianteeman Should the postinstall message be shown only after updates? Or shall it also be shown after a new installation?

I had a free moment and was starting to prepare a PR for @PhilETaylor .

I see we have some others which are added with an update SQL when updating but not being in joomla.sql for new installations. I don't know if that is by purpose or by mistake.

Only after update.

@HLeithner The first draft of the Post Install Message is here for you to review. Thanks to @richard67 and @brianteeman for quick review and edits.

What a pity that our automated integration tests test only new installations but not updates e.g. to the patch package for this PR, to make sure I have no SQL syntax error on one of the update scripts. But as I have copied them from a before working one and just modified the language string constants, it should be fine. Will at least to a quick test on MySQL soon to be sure.

Maybe we should complete the last change Only sites behind a Load Balance/Reverse Proxy ... by a ... or insane site admins ? (joking)

... or insane site admins ? (joking)

cough.. joomla.org is behind such a setting :)

@PhilETaylor I meant that the site admin is insane when he wants to enable the new setting when NOT being behind a load balancer or so.

I have tested this item ✅ successfully on f633efd

New configuration option works as well as with my previous test.
New postinstall message is shown after updating current staging to the patch package for this PT built by drone.

I like the message.

@PhilETaylor It seems there is an unwanted change from another PR: 9066e4b . Maybe my mistake?

Unit tests failing:

There was 1 error:

1) JFilesystemPatcherTest::testApply with data set "Test delete" ('Index: lao\n==================...ames.\n', '/drone/src/tests/unit/tmp/patcher', 0, array('The Way that can be told of i...ames.\n'), array(null), 1, false)
Undefined index: extension

/drone/src/tests/unit/core/helper.php:55
/drone/src/libraries/src/Filesystem/File.php:196
/drone/src/libraries/src/Filesystem/File.php:272
/drone/src/libraries/src/Filesystem/Patcher.php:187
/drone/src/tests/unit/suites/libraries/joomla/filesystem/JFilesystemPatcherTest.php:935

FAILURES!
Tests: 6599, Assertions: 10844, Errors: 1, Skipped: 138, Incomplete: 37.

See https://ci.joomla.org/joomla/joomla-cms/41722/4/8 when expanding to see the whole log.

what exactly ?

@PhilETaylor The changed file libraries/src/Filesystem/File.php with the opcache changes.

I have not tested this item.

ah I see now - doh...

I have reverted the opcache changes from this PR, no idea how I added that haha - so. many. PRs. :)

I have tested this item ✅ successfully on f633efd

@HLeithner Postinstall message is done.

If I asking for auto detection in the postinstall message it would be too much right ? ;)

If I asking for auto detection in the postinstall message it would be too much right ? ;)

@HLeithner Auto detection of what? If we are behind such a load balancer or whatever else thing where we should switch the new option on?

It should be easy to detect the reverse proxy and you could tell the user about it and give him/her a button to activate it directly from the post installation message.

It should be easy to detect the reverse proxy

This is beyond my knowledge.

and you could tell the user about it and give him/her a button to activate it directly from the post installation message.

With this I could help.

Lol you talk some crap haha

If it was "easy" to detect if you were behind a load balancer or reverse proxy then this PR is a complete was of time - we would not need a switch because we could "detect" it

Hint: we need a switch. You can't (reliably) detect it.

You only need to check if one of the 2 $_SERVER variables exists and then added the button to activate the new option. I'm on mobile and can't check how this is done

I'm on mobile too.

I guess you could theoretically check for x-forwarded-for is set to something - but don't actually use that value (for that would introduce the security issue we are trying to prevent)

BUT different proxies put the IP in different keys of _SERVER - and we would be chasing each vendors prefix like X-Real-IP etc

For example iirc cloud flare uses a proprietary key

R![Uploading 113872411...Eh

Doesn't need 100%

What would be better would be a well researched and uneaten wiki page authored by JSST on how to securely run joomla behind a proxy like cloudflare (eg blocking direct access and only allowing cloudflare network access to your site)

S/uneaten/written

If someone gives me the precise code for the check(s), returning true if the message is to be shown and false if not, I can integrate that into our postinstall message.

But I think it is some kind of overkill, doing that check.

From a security point of view we do want the admins to read docs and research so they know what they are doing, and not blindly trusting on our maybe not complete detection. So I'd prefer a Wiki page or some other kind of docs. Like we have it now is sufficient for me.

We want people to see the message BEFORE they are behind a proxy too... so I vote for a helpful wiki page.

The problem with "guessing" is that it would be incomplete... for example it could be any of the headers

// normal
X-Forwarded-For
// Used for some Google services
X-ProxyUser-Ip: 203.0.113.19
// Cloudflare
True-Client-IP
// Joomla detects
Client-IP
// or even behind a TRANSPARENT proxy it could even be in
REMOTE_ADDR
or literally any header the proxy decides to use...

One could have set the Client-IP header just for fun to the right client IP for not using any proxy or whatever.

One could have set the Client-IP header just for fun to the right client IP for not using any proxy or whatever.

And therein is the security issue. Its just a header, and can be spoofed.

I could call joomla.org right now, and the load balancer would send my real IP in X-Forwarded-For to the origin server, but I could also send request headers of the rest of them as fake values... and without any validation apps can inject crap into logs etc...

I guess for the Post Install Message, @HLeithner is right that we could detect if any of the custom headers has a value, and therefore display the message, but the question is would we really want to. If we get it wrong and people enable the new option, they have just introduced a security issue to their site :)

I think one of the places where some information has to be added to docs is this one here: https://docs.joomla.org/Security_Checklist/Joomla!_Setup .

Maybe somewhere near section "Running on a non-vhost environment".

@PhilETaylor What do you think?

that page is so out of touch and out of date now :) but yes, something like that. Especially if I ever get time to do the full Symfony type Trusted Proxy set up, it will certainly need its own page.

that page is so out of touch and out of date now :)

I wouldn't say it's out of date. It has been recently updated by the section "Running on a non-vhost environment".

The only thing which appears old-fashioned to me is the "Enable SSL on your server", because that appears to me to be quite standard in our days, but as there might be still site owners who live below a stone and check their site every 5 years or so, it might still be necessary.

The "Avoid shared servers if possible" I never understood ... I am happy with a safe shared hosting provider for my homepage. Or does "shared server" mean something else?

We have a nice shiny post installation condition that works, without a security issue.

function admin_postinstall_behindproxy_condition()
{
	$headers = array(
		// Most common.
		'x-forwarded-for',
		// Some Google services.
		'x-proxyuser-ip',
		// Cloudflare.
		'true-client-ip',
		// Joomla detects this as well.
		'client-ip',
	);

	foreach ($_SERVER as $k => $v)
	{
		// Headers are case-insensitive so ensure comparing like for like, while still not trusting the user provided value
		if (in_array(strtolower($k), $headers) && !empty($_SERVER[$k]))
		{
			return true;
		}
	}

	return false;
}

for testing the condition just add

$_SERVER['X-ProxyUser-Ip'] = 'my hackers injection'; 

into your /administrator/index.php after the opening <?php

I have tested this item ✅ successfully on 918267a

@HLeithner Please check again.

The reason I want a button is that the admin like not try to spoof/hack his own site. So detection on the 2 headers we accept (it's only 2 right?) at the moment is fairly simple.

I checked the IpHelper and we only support X-FORWARDED-FOR and CLIENT_IP so the additional 2 headers could be a problem if we only support them here in the post installation message.

If we have a value in any of the 2 values we display another text that explain that it seems that the website is behind a reverse proxy and the user should activate the new option with on click. But of course should make sure that he/she should check with the provider if he/she needs this.

For example my customers are behind a loadbalancer but they don't need to know this because my application server transparently set the REMOTE_ADDR to the real client ip because on the loadbalancer information so in such case the user is behind a reverse proxy but doesn't need to activate the flag.

On many webhoster you don't have this service but also don't have the knowledge that you are behind a loadbalancer or accelerator proxy or ssl terminator or what ever construct you want to create. So it's better to give the user a hint that we detected a proxy instead of running into troubles without noticing it for month. At this point in time joomla does this auto detect and it worked fine as far as I know.

If it were not so late in the day, we could have improved IpHelper to also handle the other common headers - CloudFlare are HUGE now, and we should at least support their headers.

IpHelper was never designed FOR PROXY DETECTION,, its method is so called "workarounds"...

actually my code is wrong... one moment....

@PhilETaylor Harald wrote something about a button which he wants in the postinstall message to activate the new option directly from there.

If it were not so late in the day, we could have improved IpHelper to also handle the other common headers - CloudFlare are HUGE now, and we should at least support their headers.

IpHelper was never designed FOR PROXY DETECTION,, its method is so called "workarounds"...

Doesn't matter it's used in the cms for that purpose, I also think it makes sense to support the 2 additional headers... sadly they are in the framework package. Not sure if I want to write a PR and pull package in j3 now...

Harald wrote something about a button which he wants in the postinstall message to activate the new option directly from there.

He loves to dream big :-)

Not sure if I want to write a PR and pull package in j3 now...

Im already working on IPHelper... but I think its too late for Joomla 3.

That's not about a big dream it's about usability. If we have this postmessage now and don't support cloudflare and google on the other side cloudflare also supports x-forwarded-for so it should detect cloudflare too. Don't know how it's with google?

This PR already makes Joomla "more secure" and that was its initial purpose - to address the issue reported to the JSST.

The rest was just nice to play with and get right, but with Joomla 3 so close to end of life its too close to get it perfect.

My personal aim is to add trusted proxies and trusted headers (aka Symfony) in Joomla 4.

Do you have a quick 3 line bit for code for

• read JConfig
• Set new value
• save JConfig

?

Do you have a quick 3 line bit for code for

• read JConfig
• Set new value
• save JConfig

?

Joomla is not designed to make configuration changes easy ;-)

https://github.com/joomla/joomla-cms/blob/staging/administrator/components/com_admin/postinstall/eaccelerator.php#L44-L81

I think we skip this for now

On the other side I think the code is ready for copy paste

This PR already makes Joomla "more secure" and that was its initial purpose - to address the issue reported to the JSST.

The rest was just nice to play with and get right, but with Joomla 3 so close to end of life its too close to get it perfect.

My personal aim is to add trusted proxies and trusted headers (aka Symfony) in Joomla 4.

That's true, I just don't want to bother people with the same problem/postinstallation message twice

What ever happen I will go outside with the dog for an hour and will then test and merge what ever we have ;-)

@PhilETaylor if (in_array('HTTP_CLIENT_IP', $_SERVER) && !empty($_SERVER['HTTP_CLIENT_IP'])). With in_array you check for the value, and then you use it as key, that seems weird to me.

if you try and access a key without it being there you get a php warning https://3v4l.org/2McgZ

hmmm but empty doesn't give a warning - strange https://3v4l.org/JNkiX

Then you should use array_key_exists

lol - been one of those days :)

@PhilETaylor Another things is now the check is cace-sensitive for the array key. Before it was case-insensitive. What is right?

PHP forces them to upper case and prefixes them with HTTP_

And we need to add the check to the display condition if the option has been enabled, and if so, not show the message.

Or we show the current value of that new option in the message instead of hiding it when switched on.

Otherwise there is no visual feedback after the new action button has been clicked.

yes yes :) im getting there e:)

The problem I have at the moment is when you press the button

it calls a GET on http://127.0.0.1:4444/administrator/index.php?option=com_postinstall&view=messages&task=action&id=11&7b7ce145a56b9dde1ed833e4418bb3cc=1

which updates configuration.php - but doesn't update $app or JConfig loaded in the stack... so the message shows again until you refresh the page...

Would be easier to have a link to the server settings tab of global config so the user can switch it on or off there, instead of a button.

haha - but Harold/Harald wanted a button :) and the whole post-installmessages component is built to have an actionable button...

Ok it works! Please test :)

I have tested this item ✅ successfully on e37c15a

Tested that update to the patched update package for this PR works on MySQL 8 and on PostgreSQL.

Tested message condition and action button.

had to put an opcache_invalidate in there too :) because else when you redirect back to the list of messages, opcache has not yet hit the default 2 seconds of opcache.revalidate_freq meaning it sees the old JConfig values... Im just tidying that up now I have proved it.

actually screw opcache... this is Joomla 3 :)

ok I'll do no more - :)

could just put a sleep(2); in there so opcache reloads the configuration.php for sure before redirecting after the action? So hacky

@PhilETaylor Leave it as is? Or do an opcache_reset?

I have tested this item ✅ successfully on 70f6208

to do an opcache_invalidate (not opcache_reset) would require the whole lot of checking ini settings and function exists and basically the whole of the Joomla 3 version of the PR you and I worked on for Joomla 4, and Im not convinced we want to go down there tonight if there is a chance that Joomla 3.9.26 RC could be released tonight.

@PhilETaylor Agree we shouldn't go the opcache_invalidate way here.

But maybe for J3 an opcache_reset would be ok?

One more time more or less we use that sledge hammer in J3, does that make a difference?

@HLeithner What do you think?

Worse case is that they have opcache enabled with a default opcache.revalidate_freq of 2 seconds. They click the action button, the config is written to the hard disk but PHP still has the cached version and redirects and the user sees the button again ... and clicks it again... this time "it appeared to work" because it had already worked, but the 2 seconds had not elapsed between changing the hard disk and redirecting back to the post install messages.

The more I read about opcache_reset the more Im appalled that people even blogged about it to start with.

@PhilETaylor Well if you think an ugly hack with sleep for 2 seconds is ok and if Harald can live with that, le't do that, or let it like it is now.

Also the "correct" calling of ANY opcache_* function should be wrapped in the ini_get and function exists etc....

I'll let @HLeithner decide :)

If we are not checking for Cloudflare headers specifically maybe we should remove their name and Sucuri from here:?

If we are not checking for Cloudflare headers specifically maybe we should remove their name and Sucuri from here:?

Agree.

I have tested this item ✅ successfully on 7042e23

Thanks for all the work.

/me runs for the door... and hides in the woods for 3 weeks....

/me runs for the door... and hides in the woods for 3 weeks....

@PhilETaylor Hide from what? Sharks? ;-)

RC is ready #33059