Skip to content
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

Do not sanity check hash of anonymous requests #289

Merged
merged 5 commits into from Aug 31, 2016

Conversation

Projects
None yet
7 participants
@bdunogier
Copy link
Contributor

commented Mar 3, 2016

Fixes an issue described in #274

The hardcoded anonymous hash might differ from the one used in the sanity check introduced by this PR. This causes cache expirations for us since anonymous requests don't match.

This change excludes anonymous requests, using an AnonymousRequestMatcher, from this sanity check.

ezsystems/ezpublish-kernel#1601 describes the consequences and way to reproduce on ezplatform.

TODO

  • Update tests
  • Check inconsistency with what is used for the Vary header

@bdunogier bdunogier referenced this pull request Mar 3, 2016

Closed

EZP-25533: allow newer versions of http-cache-bundle #1601

0 of 2 tasks complete
@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Mar 3, 2016

Travis looks very broken, but I doubt that my changes did that... how stable is the 1.3 branch ? :-)

@ddeboer

This comment has been minimized.

Copy link
Member

commented Mar 6, 2016

The test failures have to do with your introduction of AnonymousRequestMatcher. Just to be sure, I restarted the tests on the 1.3 branch and they are still green.

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Mar 9, 2016

Okay, thank you @ddeboer.

*
* For backward compatibility reasons, true will be returned if no anonymous request matcher was provided.
*
* @param \Symfony\Component\HttpFoundation\Request $request

This comment has been minimized.

Copy link
@dbu

dbu Mar 29, 2016

Contributor

please only use Request here, use statements work for annotations too

@@ -61,21 +62,28 @@ class UserContextSubscriber implements EventSubscriberInterface
*/
private $ttl;
/**
* @var \Symfony\Component\HttpFoundation\RequestMatcherInterface

This comment has been minimized.

Copy link
@dbu

dbu Mar 29, 2016

Contributor

please only use RequestMatcherInterface here, use statements work for annotations too

This comment has been minimized.

Copy link
@bdunogier

bdunogier Apr 29, 2016

Author Contributor

Will do.

Habit we have for those using less fancy editors.

use Symfony\Component\HttpFoundation\RequestMatcherInterface;
use Symfony\Component\OptionsResolver\OptionsResolver;
class AnonymousRequestMatcher implements RequestMatcherInterface

This comment has been minimized.

Copy link
@dbu

dbu Mar 29, 2016

Contributor

please add a class level comment to explain that this matcher matches all requests that are anonymous.

@@ -61,21 +62,28 @@ class UserContextSubscriber implements EventSubscriberInterface
*/
private $ttl;
/**

This comment has been minimized.

Copy link
@dbu

dbu Mar 29, 2016

Contributor

and please explain here that not booting the symfony kernel is the reason why we use this matcher rather than just looking at symfony and see if there is a non-anonymous user.

This comment has been minimized.

Copy link
@dbu

dbu Mar 29, 2016

Contributor

actually, its not only performance. if we would boot the kernel, we would treat the expired session as anonymous which is exactly what #274 wanted to prevent. maybe explain the thing about anonymous fake hashes in the phpdoc here so we still know why this is done like this.

This comment has been minimized.

Copy link
@bdunogier

bdunogier Apr 29, 2016

Author Contributor

Added a comment.

@@ -10,6 +10,7 @@
<parameter key="fos_http_cache.user_context.role_provider.class">FOS\HttpCacheBundle\UserContext\RoleProvider</parameter>
<parameter key="fos_http_cache.user_context.request_matcher.class">FOS\HttpCacheBundle\UserContext\RequestMatcher</parameter>
<parameter key="fos_http_cache.user_context.logout_handler.class">FOS\HttpCacheBundle\Security\Http\Logout\ContextInvalidationLogoutHandler</parameter>
<parameter key="fos_http_cache.user_context.anonymous_request_matcher.class">FOS\HttpCacheBundle\UserContext\AnonymousRequestMatcher</parameter>

This comment has been minimized.

Copy link
@dbu

dbu Mar 29, 2016

Contributor

parameters for class names is not best practice anymore. lets just put the class name into the service definition directly. we will do that for everything, if not already done with 2.0

@dbu

This comment has been minimized.

Copy link
Contributor

commented Mar 29, 2016

i agree with the solution. if i think this through correctly, things should work unless you manage to re-use a non-anonymous hash with an anonymous request - but that would be a misconfiguration of your application. #274 was about expired sessions, where the server side session has expired, but the session id is still sent in the request. in that case, we find the hash in the cache and do the request with hash and session id, and thus compare hashes (and see that they mismatch, or if the hash was not in the cache, see the "real" anonymous hash, vs the fake hash ez would create on a request without session cookie). @bastnic maybe you can give us your thoughts as well?

(btw, i still feel ezpublish would be better off simply do a hash lookup for anonymous users as well and then cache that - its a low overhead keeping many things simpler. for example, the anonymous cache will be duplicated between completely anonymous users and users that are determined by symfony to be anonymous because the session expired or something.)

the tests seem to say that the configuration for the anonymous matcher is wrong.

@bdunogier bdunogier force-pushed the bdunogier:anonymous_request_hash-1.3 branch from e204ef7 to e4dca8a Apr 29, 2016

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Apr 29, 2016

Review remarks taken care of.

*/
private function isAnonymous(Request $request)
{
return isset($this->anonymousRequestMatcher) ? $this->anonymousRequestMatcher->matches($request) : false;

This comment has been minimized.

Copy link
@dbu

dbu Apr 30, 2016

Contributor

lets remove the isset here and just say $this->anonymousRequestMatcher ? $this-... or if you don't like this null !== $this->anonymousRequestMatcher ?

@@ -39,5 +40,9 @@
<argument />
<argument />
</service>

<service id="fos_http_cache.user_context.anonymous_request_matcher" class="FOS\HttpCacheBundle\UserContext\AnonymousRequestMatcher">
<argument />

This comment has been minimized.

Copy link
@dbu

dbu Apr 30, 2016

Contributor

this leads to the travis failure. i would define the service without argument at all.

This comment has been minimized.

Copy link
@dbu

dbu Apr 30, 2016

Contributor

hm, or maybe rather use a variable for the php session id - i think tihs is already asked for in the configuration, so we should use it here too.

This comment has been minimized.

Copy link
@bdunogier

bdunogier Jun 30, 2016

Author Contributor

i think tihs is already asked for in the configuration

did you have something specific in mind ? I'm not sure what variable should actually be used there.

This comment has been minimized.

Copy link
@stof

stof Jul 4, 2016

Member

it should be <argument type="collection" /> to pass an empty array rather than an empty string

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

Hmm, does anyone know what could cause PHP Warning: Unexpected character in input: '�' (ASCII=18) state=0 in /home/travis/build/FriendsOfSymfony/FOSHttpCacheBundle/vendor/symfony/symfony/src/Symfony/Bridge/Twig/Extension/CodeExtension.php on line 140 ?

Tests pass except for this error.

@dbu

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

the lowest version test fails. i just re-ran that test for the master branch and it works. can you please try rebasing this branch on master? or check the failures, if they are actually caused by a change in this branch?

@dbu

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

oh, just noticed this branch is against the 1.3 version. we now have 1.4 and should target 1.4 instead. or indeed master as its a new feature and there will be no 1.5 but 2 hopefully soonish.

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

Has 1.3 reached end of life in between ? :)

1.3.x is affected by this issue, as far as I can say.

@dbu

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

sorry, had lib and bundle open at the same time. the bundle is still at 1.3. so rebase on 1.3 should be the right thing.

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

Note that iirc, I've done that yesterday.

$ git l origin/1.3..
79bbdb454b96da7ae46e557baa8834863b2d2034 fixup! Do not generate hash for anonymous requests
e4dca8a55c90ba62af278e5c149ffa59258e3b8a fixup! Do not generate hash for anonymous requests
a2a675baafbd4e189a1df293e25da8ac4b848978 fixup! Do not generate hash for anonymous requests
53764a2a146b68c4fe577862b676f464c1d39014 Do not generate hash for anonymous requests

$ git l ..origin/1.3
@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

@dbu is there anything I should do in particular to run the 1.3 tests ?

On both by branch and 1.3, after composer update --prefer-lowest:

$ SYMFONY_DEPRECATIONS_HELPER=weak ./vendor/bin/phpunit
... 
26) FOS\HttpCacheBundle\Tests\Functional\Test\ProxyTestCaseTest::testGetHttpClient
Class __PHP_Incomplete_Class has no unserializer

/Users/bdunogier/ezplatform/vendor/friendsofsymfony/http-cache-bundle/vendor/symfony/symfony/src/Symfony/Component/Config/ConfigCache.php:76
/Users/bdunogier/ezplatform/vendor/friendsofsymfony/http-cache-bundle/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:559
/Users/bdunogier/ezplatform/vendor/friendsofsymfony/http-cache-bundle/vendor/symfony/symfony/src/Symfony/Component/HttpKernel/Kernel.php:132
/Users/bdunogier/ezplatform/vendor/friendsofsymfony/http-cache-bundle/vendor/symfony/symfony/src/Symfony/Bundle/FrameworkBundle/Test/WebTestCase.php:43
/Users/bdunogier/ezplatform/vendor/friendsofsymfony/http-cache-bundle/Test/ProxyTestCase.php:139
/Users/bdunogier/ezplatform/vendor/friendsofsymfony/http-cache-bundle/Tests/Functional/Test/ProxyTestCaseTest.php:20

As far as I can tell, it can't find find \FOS\HttpCache\Test\Proxy\VarnishProxy, even though it is there. I'll keep digging.

@dbu

This comment has been minimized.

Copy link
Contributor

commented Jul 1, 2016

maybe you need to remove the cache files our little test symfony app generates?

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Jul 1, 2016

maybe you need to remove the cache files our little test symfony app generates?

HAH, cache, I knew it !

@bdunogier bdunogier force-pushed the bdunogier:anonymous_request_hash-1.3 branch 3 times, most recently from 3720c45 to 5157bde Jul 4, 2016

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Jul 5, 2016

One error left:

PHP Warning:  Unexpected character in input:  '�' (ASCII=18) state=0 in /home/travis/build/FriendsOfSymfony/FOSHttpCacheBundle/vendor/symfony/symfony/src/Symfony/Bridge/Twig/Extension/CodeExtension.php on line 140

It seems to be fixed in Sf 2.3.4, but the failing test uses 2.3.0 (--prefer-lowest). My main doubt is that it apparently doesn't happen without this PR, but I hardly see how it can cause this.

Hints, anybody ? @stof maybe ?

@lolautruche

This comment has been minimized.

Copy link
Contributor

commented Jul 5, 2016

+1 with the solution

Shouldn't there be some semantic config in order to be able to configure session prefix?

{
if ($request->server->has('AUTHORIZATION') ||
$request->server->has('HTTP_AUTHORIZATION') ||
$request->server->has('PHP_AUTH_USER')

This comment has been minimized.

Copy link
@dbu

dbu Jul 21, 2016

Contributor

what if people use their custom authentication scheme?

This comment has been minimized.

Copy link
@dbu

dbu Jul 21, 2016

Contributor

i noticed that we have a configuration user_identifier_headers. maybe we should instead of looking for a session cookie or these headers just inject that list of headers and check for those headers.

the idea with cookies is that as we Vary anyways, the caching proxy has to clean up the cookie to not have random js cookie things in it. so we would not need to really go into the individual cookies.

This comment has been minimized.

Copy link
@dbu

dbu Jul 21, 2016

Contributor

does symfony itself look at HTTP_AUTHORIZATION and PHP_AUTH_USER? if so, we should change the default value of user_identifier_header in the config.

This comment has been minimized.

Copy link
@bdunogier

bdunogier Jul 22, 2016

Author Contributor

the idea with cookies is that as we Vary anyways, the caching proxy has to clean up the cookie to not have random js cookie things in it. so we would not need to really go into the individual cookies.

Meaning that a request is anonymous only if it doesn't have any of the headers from user_identifier_headers. The part that checks for custom sessions names should then be removed completely, right ?

This comment has been minimized.

Copy link
@bdunogier

bdunogier Jul 22, 2016

Author Contributor

Note that HTTP_AUTHORIZATION and PHP_AUTH_USER are server variables, not cookies. The cookie would, afaik, be Authorization in both cases.

This comment has been minimized.

Copy link
@dbu

dbu Jul 22, 2016

Contributor

The part that checks for custom sessions names should then be removed completely, right ?

yes, exactly. because we also Vary on those headers, if you send a cookie header with google analytics data in it, you break the cache anyways. this has to be handled on the caching proxy.

it looks like header based authentication is always put into the AUTHORIZATION header: https://github.com/symfony/symfony/blob/master/src/Symfony/Component/HttpFoundation/ServerBag.php#L94-L98 so cookie and authorization should be enough

This comment has been minimized.

Copy link
@andrerom

andrerom Aug 23, 2016

Contributor

@dbu so to double check here: in default vcl we strip every other cookie other then session. And from what you say this is done somewhere with the Sf proxy as well, so essentially here if there is any cookie, it is session cookie meaning we don't need to loop true them here. Correct?

This comment has been minimized.

Copy link
@dbu

dbu Aug 24, 2016

Contributor

that is what i think. i just double-checked and for HttpCache, this is in https://github.com/FriendsOfSymfony/FOSHttpCache/blob/master/src/SymfonyCache/UserContextSubscriber.php#L124-L137

for varnish, this is documented here: http://foshttpcache.readthedocs.io/en/latest/varnish-configuration.html#cleaning-the-cookie-header

caching the user context hash without cleaning the cookie header can not work.

@dbu

This comment has been minimized.

Copy link
Contributor

commented Jul 21, 2016

sorry that i missed this. had a look again, and i think we are almost there. looking over the code one more time, i noticed that we have a inconsistency with what we use for the Vary header. can you look at that please?

i was thinking whether we need any documentation, but its probably too low level.

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Jul 21, 2016

i was thinking whether we need any documentation, but its probably too low level.

Honestly glad you say that :-)
But if you think it is worth it, I can cut-my-own-throat and write something down.

i noticed that we have a inconsistency with what we use for the Vary header. can you look at that please?

Sure, I'll have a look, thank you.

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Jul 22, 2016

i noticed that we have a inconsistency with what we use for the Vary header. can you look at that please?

@dbu that's about the user_identifier_headers right ? Or did you have something else in mind ?

@dbu

This comment has been minimized.

Copy link
Contributor

commented Jul 22, 2016

yes, that was about the comment on the code we discuss. just wanted to summarize what remains open to do.

@bdunogier bdunogier force-pushed the bdunogier:anonymous_request_hash-1.3 branch 2 times, most recently from 17087b9 to ef71732 Aug 24, 2016

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2016

Changed the AnonymousRequestMatcher:

  • receives the user_identifier_headers
  • checks for the given headers (the part specific to cookies should maybe be changed, please let me know)
{
private $userIdentifierHeaders;
public function __construct($userIdentifierHeaders = ['Cookie', 'Authorization'])

This comment has been minimized.

Copy link
@dbu

dbu Aug 24, 2016

Contributor

as this is in the symfony bundle and always created from the service container, i don't think we need to default this. the Configuration.php already defines defaults for user_identifier_headers.

and please typehint array for $userIdentifierHeaders and add a docblock that says that its a list of the header names of headers that make a request not be anonymous.

This comment has been minimized.

Copy link
@bdunogier

bdunogier Aug 24, 2016

Author Contributor

Ack. Note that UserContextSubscribed does set a default ctor value for them.

This comment has been minimized.

Copy link
@bdunogier

bdunogier Aug 24, 2016

Author Contributor

Removed the default altogether, and updated the unit test.
Rebased on top of 1.3, and cleaned the commits up.

@dbu

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2016

apart from #289 (comment) this looks ready to merge. some build fail because composer has too many options and runs out of memory. not related to this PR

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Aug 24, 2016

Changed. Let me know if it's okay, and I'll cleanup the commits a bit.

/**
* @param array $userIdentifierHeaders List of request headers that authenticate a non-anonymous request
*/
public function __construct(array $userIdentifierHeaders = [])

This comment has been minimized.

Copy link
@dbu

dbu Aug 24, 2016

Contributor

my idea was to not specify a default at all, so that calling code is forced to pass an argument and not null. as it is now its is rather dangerous, if anything would go wrong we would simply consider everything anonymous.

@dbu

This comment has been minimized.

Copy link
Contributor

commented Aug 24, 2016

apart from that 1 comment i am happy with this. please squash the commits as you see fit, and rebase on 1.3 to get the fixed testing setup i just merged.

bdunogier added some commits Mar 3, 2016

Require symfony/symfony:2.3.4
Fixes the "Unexpected character in input" notice that happens on Travis, when running tests using the phar version.

@bdunogier bdunogier force-pushed the bdunogier:anonymous_request_hash-1.3 branch from 2c97a57 to 6492160 Aug 24, 2016

if (strtolower($header) === 'cookie' && $request->cookies->count()) {
return false;
}
if ($request->headers->has($header)) {

This comment has been minimized.

Copy link
@dbu

dbu Aug 25, 2016

Contributor

sorry, one more question: what does symfony request do with the cookie header? is that removed from the header bag? what happens if we have a request with only an empty cookie header? won't we then get in here and see the cookie header, making the previous if on the cookies count kind of pointless? should we do

if ($request->headers->has($header)) {
    if (strtolower($header) === 'cookie' && 0 === $request->cookies->count()) {
        // ignore empty cookie header
        continue;
    }

    return false;
}

This comment has been minimized.

Copy link
@bdunogier

bdunogier Aug 25, 2016

Author Contributor

Done, with test, @dbu.

This comment has been minimized.

Copy link
@dbu

dbu Aug 26, 2016

Contributor

the first if with cookie header and nonzero number of cookies is now redundant and should be removed.

This comment has been minimized.

Copy link
@bdunogier

bdunogier Aug 29, 2016

Author Contributor

Removed the if, adjusted the existing test, and added a new case.

@bdunogier

This comment has been minimized.

Copy link
Contributor Author

commented Aug 25, 2016

By the way, once this is merged, do you think we'll be able to tag a new release shortly, @dbu ?

@dbu dbu changed the title Do not generate hash for anonymous requests Do not sanity check hash of anonymous requests Aug 31, 2016

@dbu dbu merged commit 5ffcbf5 into FriendsOfSymfony:1.3 Aug 31, 2016

2 checks passed

continuous-integration/styleci/pr The StyleCI analysis has passed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dbu

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2016

yay, thanks bertrand

@dbu

This comment has been minimized.

Copy link
Contributor

commented Aug 31, 2016

ported to master as well, and tagged 1.3.8

@blankse

This comment has been minimized.

Copy link
Contributor

commented Jan 10, 2018

I have a problem with this.

I use eZ Platform and a anonymous with cookies (e. g. from piwik) but without the session cookie doesn't use the http cache.

Why would only look after cookie count and not after the specific session cookie?

@blankse

This comment has been minimized.

Copy link
Contributor

commented Jan 11, 2018

I created a PR for my issue: #421

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.