SessionManagerFactory + session validators via config makes validation always fail

[Martin-P]

When using Zend\Session\Service\SessionManagerFactory validators can be added via config:

'session_manager' => array(
    'validators' => array(
        'Zend\Session\Validator\HttpUserAgent',
        'Zend\Session\Validator\RemoteAddr',
    ),
),

Before security fix ddbf43a validators always returned true. After the security fix validators always fail. This PR fixes that issue with Zend\Session\Service\SessionManagerFactory (not to be confused with standalone usage of Zend\Session\SessionManager).

It is the same fix as in #7094, but it got ignored because of the security issue.

[jaapio]

why was this comment removed?

[Martin-P]

The old code attached validators at that point ($chain->attach(...)). That code has been removed and there are no validators attached at that point anymore, which makes the comment obsolete.

[YDyachenko]

Hi guys.
This fix have one problem. He attach one validator two times (in SessionManager::initializeValidatorChain() and ValidatorChain::__construct())

Also i think what we should setup validators via ValidatorChain rather than SessionManager constructor.

So i created another patch for this issue: YDyachenko@feecb0a

[Martin-P]

He attach one validator two times (in SessionManager::initializeValidatorChain() and ValidatorChain::__construct())

Can you specify where that happens? In my code nothing is attached at ValidatorChain::__construct().
See line 353 of Zend\Session\SessionManager:

$this->setValidatorChain(new ValidatorChain($this->getStorage()));

Perhaps you can post a failing unittest for that? That way it is easier to check if that actually happens.

[YDyachenko]

Can you specify where that happens? In my code nothing is attached at ValidatorChain::__construct().

If session contains validators, they will be attached:
Line 36 of Zend\Session\ValidatorChain.

Test:

    /**
     * @runInSeparateProcess
     */
    public function testFactoryDoesNotAttachValidatorTwoTimes()
    {
        $storage = new ArrayStorage();
        $storage->setMetadata('_VALID', array(
            'Zend\Session\Validator\RemoteAddr' => '127.0.0.1',
        ));
        $this->services->setService('Zend\Session\Storage\StorageInterface', $storage);

        $config = array(
            'session_manager' => array(
                'validators' => array(
                    'Zend\Session\Validator\RemoteAddr',
                ),
            ),
        );
        $this->services->setService('Config', $config);

        $manager = $this->services->get('Zend\Session\ManagerInterface');
        try {
            $manager->start();
        } catch (RuntimeException $e) {
            // pass
        }

        $this->assertEquals(1, $manager->getValidatorChain()->getListeners('session.validate')->count()); 
    }

1. ZendTest\Session\Service\SessionManagerFactoryTest::testFactoryDoesNotAttachValidatorTwoTimes
   Failed asserting that 2 matches expected 1.

[Martin-P]

Did a rebase because I encountered some merge conflicts.

@YDyachenko Thanks! I added your test to this PR and fixed the issue for that specific test.

[weierophinney]

This changes the context for the test! The original test omitted it for a specific reason, based on the security issue we addressed.

[Martin-P]

Please see the previous discussion: #7094 (comment)

That is why I said it is a BC break, because the validators are now attached after the session has started. This makes sense, because before the session has not started there is no session data to compare with and thus the validators always fail.

In my opening post #7128 (comment) I explained the tests always passed before the security fix and always fail after the security fix. The tests added to detect the security fix still pass, which shows this PR does not compromise the security fix in any way.

This test is not related to the security issue, because:

1. If it was here to detect the security issue, it should have failed before the security issue was even addressed. It did not and it always passed the test.
2. The commit which fixed the security issue changed nothing about this test: ddbf43a

[weierophinney]

Merged to develop for release with 2.4.