Skip to content
This repository has been archived by the owner on Jan 30, 2020. It is now read-only.

Passing an invalid uri to the referrer header should no longer throw #151

Conversation

roelvanduijnhoven
Copy link
Contributor

Due to the change in #147 we removed the try/catch control structures from our code. However: exceptions are still generated. I think that this is not intended.

The only valid way to check for a referrer is now the following. Which is kind of cumbersome:

function isRefererFromGitHub($headers) {
    try {
        $referer = $headers->get('Referer');
        if (!$referer instanceof \Zend\Http\Header\GenericHeader) {
            return false;
        }
        
        return $referer->getUri()->getHost() === 'github.com';
    } catch (\Zend\Http\Header\Exception\InvalidArgumentException $e) {
        return false;
    }
}

This PR fixes this by wrapping the exception generated.


I am however a bit unsure if this is correct. The release notes state:

Now, Header::has() will return false for such headers

However; in my case the has function does yield true. Therefore I omitted that assertion from my test code.

Any thoughts @webimpress @weierophinney?

@weierophinney
Copy link
Member

This is a case where has() SHOULD return true, but get() MUST raise an exception. As such.

We have a similar situation in zend-servicemanager: an entry can be both registered, allowing has() to return true, but, due to missing dependencies or other problems, get() will raise an exception. This is normal; has() is simply indicating that the container knows about the entry. As such, your changes make sense.

@weierophinney weierophinney merged commit 9820cbb into zendframework:master Aug 13, 2018
weierophinney added a commit that referenced this pull request Aug 13, 2018
…d-uri

Passing an invalid uri to the referrer header should no longer throw
weierophinney added a commit that referenced this pull request Aug 13, 2018
weierophinney added a commit that referenced this pull request Aug 13, 2018
weierophinney added a commit that referenced this pull request Aug 13, 2018
@weierophinney
Copy link
Member

Thanks, @roelvanduijnhoven!

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants