Application setHeader() does not respect $replace option

[weeblr]

The setHeader() method causes the last header set to always be output even if $replace is set to false.

Summary of Changes

Incorrect use of $replace causes the last value for a given header to always override pre-existing values, even if $replace is set to false.

Testing Instructions

In any Joomla 4 version, use setHeader() similar to:

$app->setHeader('Some-Header', 'This should be used', true);
$app->setHeader('Some-Header', 'This should not be used', false);

Currently, the "Some-Header" header is set to "This should not be used" instead of "This should be used".

Documentation Changes Required

None, bug fix.

[nibra]

That is expected behaviour.

After

$app->setHeader('Some-Header', 'This should be used', true);
$app->setHeader('Some-Header', 'This should not be used', false);

the value of the header is

Array (
    'Some-Header' => Array (
        'This should be used',
        'This should not be used'
    )
)

which is intended. If you want to set a header only if it is not yet set, you need to check it separately.

[weeblr]

@nibra Why are all values not output then?

If you want to set a header only if it is not yet set, you need to check it separately.
What I wanted is for Joomla to not override my header actually (Cache-Control) but I see now I can't do that just by setting my own.

[nibra]

@nibra Why are all values not output then?

That depends on the actual header definition. Some accept multiple values, some don't.

[weeblr]

Correct, but currently multiple headers are just not output. Joomla 3 had some code to compose multiple headers into one single comma-separated line as they should, but that code does not seem to be in Joomla 4.

Or are you saying that just because some cannot be composed, you decided to not output them to avoid the risk of an invalid one?

[Llewellynvdm]

@weeblr can you give me a link to where this code to compose multiple headers is found in Joomla 3?

Then I see that we are setting the header here with required->use Laminas\Diactoros\Response using method->withAddedHeader which in fact manages the multiple loading headers already very well. There is some header security that can prevent headers from being set.

@weeblr when looking over this, it seems all is working as it should. Can you again go over the Laminas side of this, and see if your issue is not resolved taking their behavior into account?

[weeblr]

@Llewellynvdm

Problem is still there - although the code in WebApplication::setHeader() has changed since I made the PR a year ago.

Just test it:

$app->setHeader('Some-Header', 'This should be used', true);
$app->setHeader('Some-Header', 'This should not be used', false);

Problem is not in the Response object but in setHeader(). The logic is (still) wrong.

I had a quick look and the buggy line is this one now:

if ($value && (!$keys || ($keys && ($replace || !$single))))

It adds the (second) header to the response if

• $replace is true - OK
  or
• it's not on the singleValueResponseHeaders property - not OK

Which means the issue happens for any header name that's not on that Joomla hardcoded list of single value headers.

My previous fix is still valid I guess:

...
	// Find existing headers by name
	$keys = array_keys($names, $name);
}
if(!$replace && $keys)
{
  return $this;
}

// Remove if $replace is true and there are duplicate names
if ($replace && $keys)
{
	$this->response->headers = array_diff_key($this->response->headers, array_flip($keys));
}

[Llewellynvdm]

Where do you see this hardcoded list of single value headers?
Further have you looked at why Laminas does not merge your header?
Where is this line if ($value && (!$keys || ($keys && ($replace || !$single)))) from?

I did not say that the issue is resolved, I said it could possible be because what you trying to do is being blocked at a lower level, and weakening the code here is not the best choice.

[weeblr]

Sorry, been nearly a year since opened this, wanted to reply to you quickly so I actually looked up and posted Joomla 3 stuff.

First off, the issue is not solved. Please just test it. Paste this into any com_content template on Joomla 4.2.8:

$app->setHeader('Some-Header', 'This should be used', true);
$app->setHeader('Some-Header', 'This should not be used', false);

and the Some-Header header is output with the "This should not be used value". Today. Joomla 4.2.8.

what you trying to do is being blocked at a lower level, and weakening the code here is not the best choice.

The setHeader() method does not behave according to its definition. It replaces an existing header when instructed not to do so. The second value should be appended to the first as CSV, and that does not happen.

I looked quicky at how it's done inJ4 and the problem is indeed lower down, when rendering the headers, in j4/libraries/vendor/joomla/application/src/AbstractWebApplication.php at around line 630 in method sendHeaders.

That method just iterate over the values and therefore always output the last one.

In Joomla 3, the sendHeaders() method is in j3/libraries/src/Application/WebApplication.php, around line 800 and has code to correctly concatenate multiple headers, which has been removed from the J4 version.

[Llewellynvdm]

That method just iterate over the values and therefore always output the last one.

Therefore we should fix this getHeaders method instead, so that it will correctly deal with headers that have multiple values.

[Llewellynvdm]

Wondering why they did not just use this function \Laminas\Diactoros\Response->getHeaderLine in the Response class, since it already deals with this issue, or so it seems.

[Llewellynvdm]

This could be the fix:

/**
 * Method to get the array of response headers to be sent when the response is sent to the client.
 *
 * @return  array
 *
 * @since   1.0.0
 */
public function getHeaders()
{
	$return = [];

	$headers = array_unique(array_keys($this->getResponse()->getHeaders()));

	foreach ($headers as $name) {
	    $return[] = ['name' => $name, 'value' => $this->getResponse()->getHeaderLine($name)];
	}

	return $return;
}

@nibra or @HLeithner what do you think?

[Llewellynvdm]

How will we deal with this? https://github.com/laminas/laminas-diactoros/blob/2.2.2/src/MessageTrait.php#L164

[Llewellynvdm]

I see the suggestive way of displaying this seems better:

/**
* Method to get the array of response headers to be sent when the response is sent to the client.
*
* @return  array
*
* @since   1.0.0
*/
public function getHeaders()
{
	$return = [];

	foreach ($this->getResponse()->getHeaders() as $name => $values) {
	    // how will we check for headers that need special separators?
	    $return[] = ['name' => $name, 'value' => implode(", ", $values)];
	}

	return $return;
}

[Llewellynvdm]

Yes against all good practice Laminas uses a Trait to add these methods.

[weeblr]

How will we deal with this? https://github.com/laminas/laminas-diactoros/blob/2.2.2/src/MessageTrait.php#L164

This is what the "hardcoded list of single value headers" in Joomla 3 was doing.

[weeblr]

$singleValueResponseHeaders in /libraries/src/Application/WebApplication.php

[Llewellynvdm]

Simple question, why not set your header this way?

private function setSomeHeader(string $value)
{
	// you control the separator
	$separator = ';';

	// get either previously set headers or empty array
	$headers = $this->app->getResponse()->getHeader('Some-Header');

	// add the new value
	$headers[] = $value;

	// update the header
	$this->app->setHeader('Some-Header', implode($separator, $headers));
}

Then you can just do:

$this->setSomeHeader('This should be used');
$this->setSomeHeader('This should be the second value');

This seems to be how it should be done now, without any changes.

Looking at the PSR7 documentation and the Laminas Diactoros Response class, the responsibility of setting the headers correctly does not belong to the application, but in fact belongs to the developer setting the header in the first place. Therefore, we should not hardcode $singleValueResponseHeaders and just let multiple headers load if they were set to be multiple, giving the responsibility back to the one setting the headers to not break the header conventions set forth in the rfc7231.

Having said that, I assume that we have an intentional or none intentional limitation since the current AbstractWebApplication.php getHeaders method does not allow for multiple headers to be set the way your trying to set them. That is why I suggested the above workaround that actually gives you full control.

[weeblr]

Simple question, why not set your header this way?

Never assume you're in control. A website usually has many extensions, which can (will) often try to do the same thing.

[Llewellynvdm]

I hope they adopt the same approach as it is currently the correct way to set multiple values on a single header. Although I called it a workaround, it is essentially the only way to achieve this. The current application avoids the question of what should be single or multiple and instead forces all headers to be single unless explicitly set as multiple by the extension developer. This can be considered as a safeguard, as all headers that can be multiple can also be single, but not the other way around. Explicitly setting it as double/multiple indicates that the developer knows what they are doing, and the application does not need to assist them.

[weeblr]

That's not what I mean. One extension can set "Some-header" and another extension can also set "Some-header", unaware of what happened elsewhere.

[Llewellynvdm]

I will ask some of the other Maintainer to check my perspective, as I still feel the false flag does indicate adding multiple values as part of the contract, and if this is not the case we need to change the contract. So I am not ignoring the obvious issue here... just stating why the current behavior makes sense.

[weeblr]

I think there's some confusion here:

as I still feel the false flag does indicate adding multiple values as part of the contract,

Correct. Desired. The bug is that this is not what Joomla 4 is doing. This works in Joomla 3 but not in Joomla 4.

$this->app->setHeader('Some-Header', 'Value 1');
$this->app->setHeader('Some-Header', 'Value 2', false);

should result in

Some-Header: Value 1,Value 2

Currently, the result is:

Some-Header: Value 2

That's just a flat out bug.

[Llewellynvdm]

...another extension can also set "Some-header", unaware of what happened elsewhere...

Yes, you are correct. But this is always true, even if the application worked the way you wanted it to, and I understand that. When I said "you're in control," I meant to show that you have the ability to set the separator and not rely on the hard-coded , separator of the class. I understand the limitations in this area and the obvious possible collusion.

[weeblr]

Have you seen my previous message? your last reply is totally NOT what I am talking about and what this discussion is about.

Again, this is a simple and straightforward bug. The setHeader() contract is fine and should not be changed.

Simply Joomla 4 does not fulfill this contract.

Passing false results in only the last header value being used, instead of all values being concatenated.

[Llewellynvdm]

... I think there's some confusion here

Nope no confusion here :)

Firstly the solution you gave will not be excepted as it does not fix the issue in the correct way. I already pointed you the function and possible fix in this comment. Have you tested that?

Secondly I also explained why the current behavior though unintentional is proving to be a safe guard, that we may possible not change, but we may in fact update the contract, and remove the option of setting multiple values with the setHeader method. I personally don't like this option, but I will have to check with the other maintainers in the CMS maintenance team, why things was coded this way in the first place.

Please remember I am sitting on the other side of the world, and as you reply I am taking time to think of what you said and then reply with a smile. (sorry if it sounds off topic, I am just trying to explain myself best I can) I am like you here to help fix and improve the framework.

[weeblr]

Firstly the solution you gave will not be excepted as it does not fix the issue in the correct way. I already pointed you the function and possible fix in this comment. Have you tested that?

I did not propose any solution, not sure what you are talking about. The initial code in that PR was for another context and since then abandonned.

I had to dig through to find the real cause of the issue, described in this comment.

The rest is all your doing. Again, I did not propose any solution, I am just asking that this function behaves per its current contract, as described by @nibra at the start. My current problem is that it does not.

Secondly I also explained why the current behavior though unintentional is proving to be a safe guard,

So now you want to change the contract? introducing another BC break from Joomla 3?

Looking at the PSR7 documentation and the Laminas Diactoros Response class, the responsibility of setting the headers correctly does not belong to the application, but in fact belongs to the developer setting the header in the first place. Therefore, we should not hardcode $singleValueResponseHeaders and just let multiple headers load if they were set to be multiple, giving the responsibility back to the one setting the headers to not break the header conventions set forth in the rfc7231.

I agree with that. Let's just have this method properly implements the behavior of this last boolean parameter.

[Llewellynvdm]

When I refer to "your proposed solution," I mean this current PR and the code contained within it (although I am aware that it is old). Today was the first time I read the PR and responded to it, with the aim of bringing it to a conclusion.

I attempted to explain the underlying issue, which we can both agree is with the getHeaders method, and how it could possibly be resolved.

Next, I tried to clarify why the getHeaders method appears to flatten the headers as a safeguard. While I can see the reasoning behind it, I also noted that I do not like the fact that it obviously breaks the contract and am unsure as to why this was done. However, I will obtain feedback from those who implemented it to ensure that we move forward without breaking any more things.

The reality is that this has been broken for a while now, so the contract has already been breached. Today, after much research, I understood that there is indeed a bug, and your initial PR did try to fix it. But what I also found though, is that this bug appears to be intentional. For this reason, I am cautious about how we proceed with fixing it.

Can you please test if this code:

/**
 * Method to get the array of response headers to be sent when the response is sent to the client.
 *
 * @return  array
 *
 * @since   1.0.0
 */
public function getHeaders()
{
	$return = [];

	foreach ($this->getResponse()->getHeaders() as $name => $values) {
	    $return[] = ['name' => $name, 'value' => implode(", ", $values)];
	}

	return $return;
}

Does indeed fix the issue?

[weeblr]

Next, I tried to clarify why the getHeaders method appears to flatten the headers as a safeguard.

I don't see that, rather I don't see how it would be some sort of safeguard. What's the benefit? what's the problem solved? The only thing that does is making it impossible to output valid headers, where there are multiple values, without using the rather convoluted way of first reading the headers, then modifying them, then setting them.

That - again - just looks like an oversight.

While I can see the reasoning behind it,

I don't see any reasoning. What would that be? what problem is this solving? *

But what I also found though, is that this bug appears to be intentional. For this reason, I am cautious about how we proceed with fixing it.

I see no sign of intention here, just a bug.

Think about it:

• I call setHeader('Some-header", "some value", FALSE);

My intention is that this call will NOT replace any previously set value for this header, right? or else, I would not specificy the FALSE parameter.

But currently, the effect is that sending TRUE or FALSE has the same result: any previously set value for this header is discarded.

Surely that cannot be "intended".

This is just something that was never implemented.

[Llewellynvdm]

Please update the PR with the fix I provided, also add a test for this issue, to ensure that we now have this issue resolved.

@wilsonge have you had time to look at this?