Possible to break PageCache when adding a RequestPolicy that unconditionally returns DENY or ALLOW

to prevent logged in pages to come up in the cache.

Please write a test proving that "logged in pages" are being stored in Page Cache. I cannot reproduce this. If you can, this is a critical security issue.

Aha! Adjusting title accordingly.

That suggests that \Drupal\Core\PageCache\ChainRequestPolicy::check() needs hardening. Hardening NoSessionOpen is pointless, because another request policy can then have the exact same "weakness". Hardening \Drupal\Core\PageCache\ChainRequestPolicy::check() would fix it in all request policies at once.

Right?

Looks like we have the more info. But

elseif (isset($result)) {
throw new \UnexpectedValueException('Return value of RequestPolicyInterface::check() must be one of RequestPolicyInterface::ALLOW, RequestPolicyInterface::DENY or NULL');
}

So NULL was part of the design.

* @return string|null
* One of static::ALLOW, static::DENY or NULL. Calling code may attempt to
* deliver a cached page if static::ALLOW is returned. Returns NULL if the
* policy is not specified for the given request.

@Wim Leers so what hardening where you thinking of?

@alex.pott: hardening to prevent the problem that @jeroen.b described from also causing problems in request policies in contrib/custom modules.

I see that the documentation around page cache request/response policies is far from complete. Sorry for that.

I try to clarify the meaning of the three possible results of a request policy rule:

ALLOW

The request satisfies the condition encoded in the policy rule and as a result it is safe to lookup/deliver a cached response

DENY

The request satisfies the condition encoded in the policy rule and as a result it is unsafe to lookup/deliver a cached response

NULL

The request does not satisfy the condition encoded in the rule

The combination of multiple rules follows common sense: The result is DENY if any rule returned DENY and it is NULL unless at least one rule returned ALLOW - then it is obviously ALLOW.

It follows that a policy rule should be designed in a way that it either returns ALLOW|NULL or DENY|NULL. A rule returning ALLOW|DENY cannot be combined in any useful way with other policy rules.

We have the following request policy rules in core:

$ git grep -l implements.*RequestPolicy
core/lib/Drupal/Core/PageCache/ChainRequestPolicy.php
core/lib/Drupal/Core/PageCache/RequestPolicy/CommandLineOrUnsafeMethod.php
core/lib/Drupal/Core/PageCache/RequestPolicy/NoSessionOpen.php
core/modules/basic_auth/src/PageCache/DisallowBasicAuthRequests.php
core/modules/toolbar/src/PageCache/AllowToolbarPath.php

The notable allow examples are NoSessionOpen and AllowToolbarPath, the relevant deny-rule is CommandLineOrUnsafeMethod. The ChainRequestPolicy let's us combine all the rules.

NoSessionOpen returns ALLOW if there is no session on the request, it returns NULL otherwise. AllowToolbarPath returns ALLOW if the path matches the toolbar/subtree route and returns NULL otherwise. The combination of those two policy rules results in an ALLOW for any request which is either session-less or directed to the toolbar subtree endpoint. By adding CommandLineOrUnsafeMethod to the mix we additionally require the request to be either HTTP GET or HEAD.

Does that make sense?