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

JApplicationWeb::setHeader/sendHeaders behavior #9836

Merged
merged 23 commits into from Nov 15, 2016

Conversation

Projects
None yet
6 participants
@stutteringp0et
Copy link
Contributor

stutteringp0et commented Apr 10, 2016

Pull Request for Issue #9819 .

Summary of Changes

setHeader $replace previously did nothing other than delete a value - even when false it allowed a duplicate to be created which effectively replaces the original. $replace didn't matter, you could have $replace=false and the item would still be replaced when the headers were sent.

This backwards-compatible modification repairs issues with setHeader and sendHeaders to allow for multiple values to be sent for applicable headers per RFC 7230-7, and to appropriately handle header replacements where the original created duplicates which became replacements later.

When a header can contain only a single value:
setHeader($name,$value) will insert when no previous value is set
setHeader($name,$value,true) will replace, removing the previous value

When a header can contain multiple values:
setHeader($name,$value) will insert
setHeader($name,$value,true) will replace, removing the previous value(s)

If multiple values are present for a header name when sendHeaders is called, the multiple values are imploded on ', '.

Testing Instructions

I'm starting this off with an $app variable to bring my test line lengths down to something that won't wrap in this description.

$app = JFactory::getApplication();

First test shows that setting a duplicate single value header like Expires without $replace does not replace the value as was the previous behavior.

$app->allowCache(true);  // if not set, the Expires header cannot be changed from a 2005 date
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 3600) . ' GMT',true); // 1 hour
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 600) . ' GMT'); // 10 minutes

Result: Expires 1 hour future from the Date header.
(previous behavior would have the 15 minute future header, and this is the reason I made this change - because JApplicationWeb::respond sets it to now+900 seconds without $replace=true, replacing any previous value)

Now, let's reverse the $replace value

$app->allowCache(true);  // if not set, the Expires header cannot be changed from a 2005 date
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 3600) . ' GMT'); // 1 hour
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 600) . ' GMT',true); // 10 minutes

Result: Expires 10 minutes future from the Date header.
(previous behavior would have the 15 minute future header, and this is the reason I made this change - because JApplicationWeb::respond sets it to now+900 seconds without $replace=true, replacing any previous value)

Next we repeat the last test with no $replace value

$app->allowCache(true);  // if not set, the Expires header cannot be changed from a 2005 date
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 3600) . ' GMT'); // 1 hour
$app->setHeader('Expires',gmdate('D, d M Y H:i:s', time() + 600) . ' GMT'); // 10 minutes

Result: Expires 1 hour future from the Date header.
(previous behavior would have the 15 minute future header, and this is the reason I made this change - because JApplicationWeb::respond sets it to now+900 seconds without $replace=true, replacing any previous value)

Our next tests relate to a made-up header. All headers not in the single value list are considered to allow multiple values

$app->setHeader('Test-Header','aaa');
$app->setHeader('Test-Header','bbb');
$app->setHeader('Test-Header','ccc');

Result: Test-Header value is "aaa, bbb, ccc"
(previous behavior would have a value of "ccc")

Next test issues $replace = true in the middle, losing one of the values

$app->setHeader('Test-Header','aaa');
$app->setHeader('Test-Header','bbb',true);
$app->setHeader('Test-Header','ccc');

Result: Test-Header value is "bbb, ccc"
(previous behavior would have a value of "ccc")

Next test issues $replace = true on the last value, losing the other entries

$app->setHeader('Test-Header','aaa');
$app->setHeader('Test-Header','bbb');
$app->setHeader('Test-Header','ccc',true);

Result: Test-Header value is "ccc"
(previous behavior would have a value of "ccc")

Update web.php
setHeader $replace previously did nothing other than delete a value - even when false it allowed a duplicate to be created which effectively replaces the original.

This modification searches for an existing header with the same name and only inserts it if it doesn't already exist, or if $replace is true.

@brianteeman brianteeman changed the title Update web.php JApplicationWeb::setHeader behavior Apr 10, 2016

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 10, 2016

you need to fix travis CI errors

JApplicationWeb::setHeader behavior
Fix Travis CI error regarding invalid foreach
@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 10, 2016

Reading the JApplicationWebTest - it's actually testing for the issue this PR is meant to resolve.

The failure is on line 1321 of the test, which is within testSetHeader:

    public function testSetHeader()
    {
        // Fill the header body with an arbitrary value.
        TestReflection::setValue(
            $this->class,
            'response',
            (object) array(
                'cachable' => null,
                'headers' => array(
                    array('name' => 'foo', 'value' => 'bar'),
                ),
                'body' => null,
            )
        );
        $this->class->setHeader('foo', 'car');
        $this->assertEquals(
            array(
                array('name' => 'foo', 'value' => 'bar'),
                array('name' => 'foo', 'value' => 'car')
            ),
            TestReflection::getValue($this->class, 'response')->headers
        );
        $this->class->setHeader('foo', 'car', true);
        $this->assertEquals(
            array(
                array('name' => 'foo', 'value' => 'car')
            ),
            TestReflection::getValue($this->class, 'response')->headers
        );
    }

Notice, the 2nd time the header is set, a duplicate header (foo) exists. The way headers are set, by name, means that only the 2nd header will be added to the resulting output - effectively replacing the original entry without using $replace=true!

This is a bad test, it was written to support an error. The second setHeader should fail because a duplicate name exists without $replace=true.

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 10, 2016

@mbabker can you help here?

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 10, 2016

Anyway, you also have a lot of code style errors.
For performance reasons, the code style test and consequent errors only appear in php 5.6 test
See https://travis-ci.org/joomla/joomla-cms/jobs/122080816#L1323

stutteringp0et added some commits Apr 10, 2016

JApplicationWeb::setHeader behavior
Formatting changes
JApplicationWeb::setHeader behavior
Formatting issues
@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 10, 2016

OK, code style errors resolved (including one that was present in the existing code)

@andrepereiradasilva

This comment has been minimized.

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 10, 2016

That's what I said earlier...

within testSetHeader

In JApplicationWeb::header, when headers are set - they're set with $replace=true 4 out of 5 times (the default) with the exception of the status header. So it becomes even more important that $replace be enforced in setHeader, as the only other place it gets enforced isn't ever being used beyond the status code.

JApplicationWebTest needs repair before this issue can be resolved.

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 10, 2016

working on that now....Do I reference the commit here? These must be fixed in correct order.

stutteringp0et added a commit to stutteringp0et/joomla-cms that referenced this pull request Apr 10, 2016

fix testSetHeader test
in line 1314 ��"$this->class->setHeader('foo', 'car');", the method is called without the $replace argument, meaning it uses the default of "false".

The way headers are set, allowing a duplicate here means the header is replaced in JApplicationWeb::header.  If $replace isn't enforced in setHeader then duplicates mean the header gets replaced anyway.

The attempted entry is removed from the assertEquals test.

This change is in conjunction with a JApplicationWeb PR I have waiting
joomla#9836
@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 10, 2016

just directly edit the file in this link https://github.com/stutteringp0et/joomla-cms/blob/patch-1/tests/unit/suites/libraries/joomla/application/JApplicationWebTest.php#L1299

If you edit in this link it will automatically changed in this PR

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 10, 2016

no, don't do another PR for that

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 10, 2016

oops...sorry

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 10, 2016

just directly edit the file in this link https://github.com/stutteringp0et/joomla-cms/blob/patch-1/tests/unit/suites/libraries/joomla/application/JApplicationWebTest.php#L1299

If you edit in this link it will automatically changed in this PR

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 10, 2016

no problem just close the other PR

Fix testSetHeader

in line 1314 ��"$this->class->setHeader('foo', 'car');", the method is called without the $replace argument, meaning it uses the default of "false".

The way headers are set, allowing a duplicate here means the header is replaced in JApplicationWeb::header.  If $replace isn't enforced in setHeader then duplicates mean the header gets replaced anyway.

The attempted entry is removed from the assertEquals test.
@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 10, 2016

OK, fixed it

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 10, 2016

i have some questions regarding this.

  • The first is the test instructions (they could be more clear). for what i understand we have two do this:

    1. Install the plugin you attach and enable it
    2. Go to frontend and check the HTTP headers generated
    3. ...
  • The second question is more conceptual.

    Will this remove the possibility of adding several HTTP headers with same designation?
    Remember, this behavior is allowed by HTTP protocol. See http://stackoverflow.com/questions/4371328/are-duplicate-http-response-headers-acceptable

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 10, 2016

Line 955 of JApplicationWeb defines the header() method, which defaults to $replace=true. The only time the header method is used where $replace is altered (set to null - line 691) is for the status header, but header() only responds to FALSE, so null doesn't do anything on line 691. So any duplicated header gets replaced anyway.

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 10, 2016

Sorry, yes you're right about the test. Install and run the plugin and check the Expires header. Alter JApplicationWeb and check the header again.

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 10, 2016

So the test can be just add to protostar index.php ...

JFactory::getApplication()->setHeader('Test', 'aaa');
JFactory::getApplication()->setHeader('Test', 'bbb');
JFactory::getApplication()->setHeader('Test 2', 'ccc');
JFactory::getApplication()->setHeader('Test 2', 'ddd', true);

Result:

Before patch
Test: bbb
Test 2: ddd
Affter patch
Test: aaa
Test 2: ddd
For what i understand, should be
Test: aaa, bbb
Test 2: ddd

I'm not 100% sure it should be like this tought.

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 13, 2016

@andrepereiradasilva yes, I updated the instructions in the initial post.

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 14, 2016

@stutteringp0et one thing how do i unset an header that has already been setted before. Is there a way?

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 14, 2016

for instance something like this doesn't work, right?

$app->setHeader('Test-Header', null, true); 
@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 14, 2016

To more clear this is waht i'm trying to do to make tests easier

$app = JFactory::getApplication();
$app->allowCache(true);  // if not set, the Expires header cannot be changed from a 2005 date
$testResults = array();
$datePlusOneHour = gmdate('D, d M Y H:i:s', time() + 3600) . ' GMT';
$datePlusTenMinutes = gmdate('D, d M Y H:i:s', time() + 600) . ' GMT';

// Do tests
$app->setHeader('Expires', $datePlusOneHour, true); // 1 hour
$app->setHeader('Expires', $datePlusTenMinutes); // 10 minutes
$testResults['test1'] = $app->getHeaders();
$app->setHeader('Expires', null, true); // reset

$app->setHeader('Expires', $datePlusOneHour); // 1 hour
$app->setHeader('Expires', $datePlusTenMinutes, true); // 10 minutes
$testResults['test2'] = $app->getHeaders();
$app->setHeader('Expires', null, true); // reset

$app->setHeader('Expires', $datePlusOneHour); // 1 hour
$app->setHeader('Expires', $datePlusTenMinutes); // 10 minutes
$testResults['test3'] = $app->getHeaders();
$app->setHeader('Expires', null, true); // reset

$app->setHeader('Test-Header', 'aaa');
$app->setHeader('Test-Header', 'bbb');
$app->setHeader('Test-Header', 'ccc');
$testResults['test4'] = $app->getHeaders();
$app->setHeader('Test-Header', null, true); // reset

$app->setHeader('Test-Header', 'aaa');
$app->setHeader('Test-Header', 'bbb', true);
$app->setHeader('Test-Header', 'ccc');
$testResults['test5'] = $app->getHeaders();
$app->setHeader('Test-Header', null, true); // reset

$app->setHeader('Test-Header', 'aaa');
$app->setHeader('Test-Header', 'bbb');
$app->setHeader('Test-Header', 'ccc', true);
$testResults['test6'] = $app->getHeaders();
$app->setHeader('Test-Header', null, true); // reset

// Show test results
print_r($testResults);
@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 14, 2016

I don't believe there ever was a facility to remove a header once set - they could only be added or replaced. It's not hard to accomplish, and I'm not opposed to adding it if you think it's necessary - but it isn't something that's being done now.

Edit: just tested pre-patch and setHeader('name',null,true) just creates an empty header (header with no value), so the method never removed on a null value.

I can make that happen if you think it's necessary.

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 14, 2016

Remove on null was really easy to accomplish, and I've got that ready to add if you think it's necessary.

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 14, 2016

actually i think it would be a good feature allowing to remove an already setted header.

If you could do it, it would be great.

JApplicationWeb -setHeader and sendHeader upgrade
Adding ability to unset (remove) an existing header using $replace=true and $value=null

setHeader('name', null, true) now removes the header entirely, where previous behavior would have sent the header with an empty value.
@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 14, 2016

Done, waiting on Travis

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 14, 2016

DARN YOU FORMATTING ERRORS - DARN YOU ALL TO HECK!

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 14, 2016

OK, it's done now, passed tests and your test now works as expected on my test server

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 14, 2016

ok will test.

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 14, 2016

this is my result

Before patch

Server date: 17:34:30 GMT

Array
(
    [test1] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 18:34:30 GMT
                )
            [1] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 17:44:30 GMT
                )
        )
    [test2] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 17:44:30 GMT
                )
        )
    [test3] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => 
                )
            [1] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 18:34:30 GMT
                )
            [2] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 17:44:30 GMT
                )
        )
    [test4] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => 
                )
            [1] => Array
                (
                    [name] => Test-Header
                    [value] => aaa
                )
            [2] => Array
                (
                    [name] => Test-Header
                    [value] => bbb
                )
            [3] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
    [test5] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => 
                )
            [1] => Array
                (
                    [name] => Test-Header
                    [value] => bbb
                )
            [2] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
    [test6] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => 
                )
            [1] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
)

After patch

Server date: 17:37:40 GMT


Array
(
    [test1] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 18:37:40 GMT
                )
        )
    [test2] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 17:47:40 GMT
                )
        )
    [test3] => Array
        (
            [0] => Array
                (
                    [name] => Expires
                    [value] => Thu, 14 Apr 2016 18:37:40 GMT
                )
        )
    [test4] => Array
        (
            [0] => Array
                (
                    [name] => Test-Header
                    [value] => aaa
                )
            [1] => Array
                (
                    [name] => Test-Header
                    [value] => bbb
                )
            [2] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
    [test5] => Array
        (
            [0] => Array
                (
                    [name] => Test-Header
                    [value] => bbb
                )
            [1] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
    [test6] => Array
        (
            [0] => Array
                (
                    [name] => Test-Header
                    [value] => ccc
                )
        )
)
@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Apr 14, 2016

That looks right to me

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented Apr 14, 2016

I have tested this item successfully on 3ee123a

besides the tests above also checked each individual test generated http header.


This comment was created with the J!Tracker Application at issues.joomla.org/joomla-cms/9836.

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented May 26, 2016

This needs another human test to be completed, correct?

@andrepereiradasilva

This comment has been minimized.

Copy link
Contributor

andrepereiradasilva commented May 26, 2016

yes

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Sep 20, 2016

What can I do to get someone else to test this?

@roland-d

This comment has been minimized.

Copy link
Contributor

roland-d commented Nov 13, 2016

@stutteringp0et I am going to see if I can find some time to test this.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/9836.

@stutteringp0et

This comment has been minimized.

Copy link
Contributor Author

stutteringp0et commented Nov 13, 2016

Thank you @roland-d

@roland-d

This comment has been minimized.

Copy link
Contributor

roland-d commented Nov 13, 2016

I have tested this item successfully on 3ee123a

> First test shows that setting a duplicate single value header like Expires without $replace does not replace the value as was the previous behavior.
This works as expected, the expire header of 1 hour is active

Second test was to reverse the $replace value in case multiple times the header is set
This works as expected, the expire header of 10 minutes is active

Third test is not to use the $replace value
This works as expected because the first set value of 1 hour is active

Multiple values for a header value that allows it
This works as expected because I get the value aaa,bbb,ccc as expected

Multiple values for a header value that allows it but setting the $replace value on the second value
This works as expected because the first value is removed and only the 2nd and 3rd values are used as bbb,ccc

Multiple values for a header value that allows it but setting the $replace value on the third value
This works as expected because the first value is removed and only the 3rd value is used as ccc by replacing the first two values

All tests are successful.


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/9836.

@roland-d

This comment has been minimized.

Copy link
Contributor

roland-d commented Nov 13, 2016

Set to RTC as we have 2 successful tests


This comment was created with the J!Tracker Application at issues.joomla.org/tracker/joomla-cms/9836.

@roland-d roland-d changed the title JApplicationWeb::setHeader/sendHeaders behavior JApplicationWeb::setHeader/sendHeaders behavior Nov 13, 2016

@joomla-cms-bot joomla-cms-bot added the RTC label Nov 13, 2016

@roland-d roland-d added this to the Joomla 3.7.0 milestone Nov 13, 2016

@rdeutz rdeutz merged commit 40ac7f0 into joomla:staging Nov 15, 2016

2 checks passed

JTracker/HumanTestResults Human Test Results: 2 Successful 0 Failed.
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details

@joomla-cms-bot joomla-cms-bot removed the RTC label Nov 15, 2016

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