There's a critical bug in the if-conditions of boost_exit(), which in some cases causes it to skip checks whether a page is uncacheable by status code or drupal_page_is_cacheable():

<?php
if (!isset($_boost['cache_this'])) {
    if (!isset(
$_boost['is_cacheable'])) {
      return;
    }
    elseif (!
$_boost['is_cacheable']) {
      return;
    }
   
//if both of these inner conditions were false, we do not exit here
    //but the following conditions are not evaluated any more
 
}
  elseif (
$_boost['cache_this'] == FALSE) {
    return;
  }
  elseif (!
$_boost['is_cacheable']) { //this elseif has to become an if!
   
return;
  }
  elseif (
$_boost['menu_item']['status'] != 200) {
    return;
  }
  elseif (!
drupal_page_is_cacheable()) {
   
$_boost['is_cacheable'] = FALSE;
    return;
  }
?>
Files: 
CommentFileSizeAuthor
#1 boost-1677512.patch344 bytesbgm

Comments

Status:Active» Fixed
StatusFileSize
new344 bytes

Good catch. Although I'd be curious to see in which situations this can happen, it's safer to split that 'if'.

Maybe you actually want to check the logic of the upper ifs again and see if you can't simplify them in the first place. Some of them seam to repeat further below. You can probably use empty(x) instead of !isset(x) + !x.

And since all of these statements fire a return if true, I would not use else at all.
I don't know the exact usage of the $_boost values, so this is only a suggestion:

<?php
 
if (empty($_boost['is_cacheable'])) {
      return;
  }
  if (empty(
$_boost['cache_this'])) {
      return;
  }
  if (
$_boost['menu_item']['status'] != 200) {
      return;
  }
  if (!
drupal_page_is_cacheable()) {
   
//$_boost['is_cacheable'] = FALSE; //is this ever evaluated after we return?
   
return;
  }
?>

As for the situation in which it can happen: Boost was caching 404 pages for me, although it shouldn't.

Status:Fixed» Needs review

Makes sense, but I don't have much time to test.
Can you test and let us know how it works?

I've been using my own heavily modified version of boost for a while, so I don't assume my tests would guarantee functionality for everyone.

Well, here is a version that I tested (on my boost branch). There was in fact something that didn't work with the "cache_this" value. Now it seems pretty straightforward and safe, but still, like I said, no guarantees.

<?php
 
if (isset($_boost['cache_this']) && !$_boost['cache_this']) {
      return;
  }
  if (empty(
$_boost['is_cacheable'])) {
      return;
  }
  if (
$_boost['menu_item']['status'] != 200) {
      return;
  }
  if (!
drupal_page_is_cacheable()) {
      return;
  }
?>