Inclusion of parameter exhausts site memory with exposed filter block

smustgrave → credited greggles → .

smustgrave → credited jpieper → .

smustgrave → credited larowlan → .

smustgrave → credited poker10 → .

smustgrave → credited rlhawk → .

smustgrave → credited xjm → .

Took the patch from the security issue and crediting those who helped

Going to get this out.

I'm reopening this issue because this fix broke pagination on one of my clients site running Drupal 10.3.2. Comparing the 7.0.X branch (which is now the recommended one) with the 6.0.x branch and looking specifically at setCurrentPage calls, I saw this issue number mentioned in the commit.

I haven't dug that deep into the issue, but it appears that $this->view->getCurrentPage() returns null at least once before being set by a view pager plugin. When null, the added !is_numeric($current_page) || $current_page < 0 check added from this issue is true and the setCurrentPage method is then called with 0 (not null), which in turn appears to break the pagination somehow.

Again, I haven't dug deep enough to figure out why this is happening on the Views side, but I'm happy to do so if needed. For the time being, simply adding a not-null check to the condition does the trick for me as per the attached patch. I wasn't able to reproduce the fatal error that started this issue without the provided fix added, so I can't say for sure that it will work after adding mine, but I also don't see how this patch could break the fix given that the $current_page is already null anyway.

Same problem, pager is broken after this fix. Rolling back to 7.0.0-beta1 also solves the problem.

Can you test the patch in #13?

I find this fix useless because I can't reproduce this issue on 7.0.0-beta1 or 6.0.6 following the steps in the issue. The patch looks like it disables this and makes this code dead, so yes it does solve the problem, but not very well. It's very strange that there is no test case here and the fix is ​​already merged and released.

Because it was reported as a security issue at first and was reproducible

Applying the patch re-introduces the problem this security fix was solving.

if (!is_null($current_page) && (!is_numeric($current_page) || $current_page < 0)) {

Can you try this instead? Still to address the problem here and if it fixes your issue will deploy it as a hot fix.

I was finally able to reproduce the memory exhaustion issue that the fix was created for. I overlooked the "Expose filter in a block" part when first trying to debug this error. After exposing the Views form with Views exposed filter blocks, I also get a 500 error.

Digging deep into the call stack, its seems the error appears after calling the findPage method on a PagerParameters service with an array like code parameter.

I found an issue for the Views module in core 🐛 Protect pager against [] for the page Needs work that tries to remedy this by adding some verifications on the page parameters. After getting the MR changes locally, the memory exhaustion issue goes away and pagination works as expected.

I'm not really sure what the best course of action is to resolve this issue. We could certainly implement the same verifications on BEF side when setting the current_page and completely override this page parameter to current_page logic, but that seems beyond the expected scope of the module. Maybe we could disable the setCurrentPage calls altogether given some specific conditions to determine.

Again, I'll be happy to help if anyone has any pointers on a proper fix implementation.

Yea the original fix was pulled from a core fix I believe.

Does #19 work though?

Didn't get to the bottom of this yet, but I ran into this today and deployed #19 as a hotfix. For now it works for me (i.e. the pagers of views configured to use bef work again), but I didn't try to reproduce the original issue and didn't do any thorough testing yet. I'll keep you advised.

I've also experienced the same mentioned in #13. As a temporary solution I've downgraded to 6.x. Given that this is a critical issue that breaks the intended functionality, should 6.x perhaps be set as the current recommended build until this is resolved?

No we should fix the issue.

6.0.x branch isn't compatible with D11, missing a couple of fixes, and re-introduces the dependencies back for jquery modules.

Update: Patch #19 still fixes the pager issues I had, no further problems reported so far where we deployed this as a hot fix. Bad news: I was able to consistently reproduce the original problem mentioned in the IS with #19 applied (tested on 10.3.2). Are you sure #19 works as a fix for the original issue?

Ugh you're right it re-introduces that. Will play around some more.

This feel gross and if things were typehinted shouldn't be allowed but quick testing seems to work for both.

Ah yes, I think this is what core also does to initialize the pager, if it is not yet initialized? I'll give this a test.

I also tested with 🐛 Protect pager against [] for the page Needs work and removed the change here so think once that lands we can remove this code. I'm addressing feedback on that ticket now.

Tested the MR on a dev system previously affected by the pager bug and the pagers still work. Unfortunately, the MR still does not fix the original issue for me on 10.3.2. Put the core patch on my list for review, if no one else beats me to it.

I found a solution for both the pagination and memory exhaustion issue. Instead of getting $pager with $this->view->getPager();, I use $this->view->display_handler->getPlugin('pager'); which doesn't trigger the setCurrentPage and findPage calls I talked about in #20.

With the proper typings added, we still have access to the usesExposed getter which is the only method called on the $pager instance, so everything works as expected. We can then safely remove the checks introduced in this issue and let the core Views module handle the page query parameter error for us.

I've tested this fix with and without exposed pagination in an exposed filters block and everything works as expected (i.e. Drupal throwing a client error which happens anyway whether we use this module or not and should be fixed when 🐛 Protect pager against [] for the page Needs work gets merged).

This change has the added benefit of being more in line with how the surrounding sort and filter handlers are handled in the same file.

Thanks @jverneaut, the patch looks very promising.

I tested the patch on the dev system of one of the systems affected by the regression and the pager works as expected. On my 10.3.2 dev system I was no longer able to reproduce the original issue and the pager works as well. I think immediately getting a client error instead of an OOM is the expected behavior for now and fixes the DDoS potential, which I guess is why this was reported as a private issue previously. So we can cross those off the list. I also tested the patch on two systems using Search API Solr, which use pagers from contrib/custom. Everything still worked as far as I could see.

Looking at the difference of the original approach vs. the new approach, ViewsExecutable initializes the pager, if needed, before returning it, which means assigning the pager object from the display handler to the views executable and then also resetting the items to display and the offset on the pager object. So the difference boils down to resetting the items to display and the offset. Checking all pager plugins that come with core, this shouldn't make any difference for us since the stuff we're checking doesn't depend on any of these values.

I can't rule out that this might make a difference for implementations in contrib or custom, but I think the problem with the old approach was that we were initializing the pager too early in some cases, because our handler seems to be called multiple times per request anyway and with the invalid pager parameter then ends up in an endless loop that finally triggers the OOM. So if this works correctly, even if something in the pager plugin would depend on those additional values set by ViewsExecutable, our handler should be called again after the pager has been properly initialized, so even then the end result should be correct.

I think the next step to get this resolved would be adding the patch to the MR so that we can have a look at the pipeline results. Hopefully it should be green. I don't think we have good test coverage of the pager stuff, otherwise we should have caught the regression before it went in, so once the green pipeline confirms that nothing broke with this fix, testing the new approach with an exposed pager manually would probably also be a good idea, I didn't do that yet.

@jverneaut Would you be able to add your patch to MR 97, so that we can check the pipeline? That would be great. If needed, we have good documentation → of the MR workflow. Setting back to Needs work for that. Thanks.

Thank you for this comprehensive issue summary.

I'm of the same opinion as you regarding the potential impact of this change on contrib. I actually think this fix reduces the risk of this module causing undefined behavior because we are not no longer interfering with normal pager initialization.

I just added the changes to #97 and everything is green.

Thanks. Looks good. I tested various exposed pager stuff with the MR and as far as I can see everything still works as before. I noticed an issue with the links plugin: If the exposed form is shown in a block and the block is not placed on the same page as the view, the links point to the current page instead of to the path of the view display. Luckily, this also happens without the MR, so nothing new. I'll go ahead and RTBC this.

Woo Drupal teamwork! Not in front of my computer but will get a release out in a few hours ASAP.

Of course now that we have a fix the core issue will get fixed, isn't that how it works

Automatically closed - issue fixed for 2 weeks with no activity.