August 13, 2007Page Permissions Bug Found
Comments (5) While working on something completely unrelated I have discovered a bug in my favorite Page Permissions MOD. It’s not a critical error or anything related to security, so if you are using this MOD you don’t have to worry about that. No, it’s a subtle little thing. One that I have absolutely no idea how to fix at this time. 
One of the options in the MOD is to count page views by members or guests. I think that this is extremely valuable information, and having it available was one of the incentives for me to write the MOD in the first place. If you have installed it then you know that there are very few edits to the core phpBB code since everything is managed via sessions. Well, that means this bug is going to be difficult to fix.
What’s the bug? When you click “reply” to an existing topic, the page counter for posting.php (if you are tracking views for that page) will go up by 2 instead of 1. You see, there is the posting form that is presented (that’s one) and then below that is a topic review (that’s two). To get the topic review posting.php calls itself with a mode of “topicreview” and that generates the second page hit on posting.php.
When you post a new topic the page view is incremented correctly since there is no topic review during that process. And if you click the “preview” button then the page counter is correctly incremented. It should be an incremental view, since you refreshed the page. But when you are clicking reply by itself it should increment the counter only by one, and it’s wrong. 
As I am typing this out and explaining it to everyone (both of my loyal readers, that is 
) I think I may have come up with a fix. Since I know the page, and I get the url arguments for checking permissions, I bet I can simply throw away any counting when the page is “posting.php” and the $mode is ‘topicreview’ and that would take care of it. That seems to be a simple fix… which probably means it won’t work. 
I will see. But not right now, as I have to finish this other project first. Stay tuned for details.
Comments (5)
Not perfect, but pretty good:
if (basename($_SERVER['SCRIPT_NAME'] == “posting.$phpEx” && $HTTP_GET_VARS['mode'] == ‘postingreview’)
{
break;
}
Comment by eviL3 — August 14, 2007 @ 7:01 am
I can do something like that, yes, as I already capture the sending page and URL in order to be able to set permissions. It’s a bit of a hack but I figure I will do something very similar to what you posted and wrap it around the counter increment logic. If page counting is disabled there’s no need to check anyway. If page counting is enabled, then before I increment I will have to do one more check.
What I have done as a temporary fix on my board is to set a variable called $page_count which is a boolean TRUE/FALSE flag. In the topic preview page I set $page_count to FALSE. But that requires an edit to each page that might have this same situation, which is contrary to the way I tried to encapsulate all of the permissions stuff into a single include.
I think what I will do as a more permanent fix is to create an array of page / session variables that are exceptions with hard-coded entries.
I am working on a banner manager, and in trying to track down why certain pages were incrementing the page counter by two instead of one I discovered this bug. The other big challenge that I am working through is figuring out how to detect when a page view is a “meta refresh” page (such as the “thanks for posting” messages) and when it is a page with a longer lifespan. I don’t want to charge a banner advertiser for the “meta” pages, but I do want to count them. I think I have that figured out as well. It was a bit tricky since message_die() is called from posting.php, which then redirects to viewtopic.php. That means that some views for posting.php are counted as billable page views (but not the topic review ones
) while others are not. Making sure I know which are which is going to be important.
Comment by dave.rathbun — August 14, 2007 @ 11:58 am
The idea with the variables is good, i like it. But there’s one thing i must say: constants. Seriousley, it’s just not worth having to global them, it’s much better to just use constants. This would mean maybe 2 edits. One for the posting review function, one for the message_die function. Just add a:
if (!defined('PAGE_PERMISSIONS_NO_INCREMENT')){
define('PAGE_PERMISSIONS_NO_INCREMENT', true);
}
And then for the check:
if (!defined('PAGE_PERMISSIONS_NO_INCREMENT'))
{
// increment code
}
That’s a lot better. Just a suggestion
Comment by eviL3 — August 18, 2007 @ 5:19 am
Thanks, evil, I do appreciate the input. But I don’t think I need to do things that way. One of the goals was to do minimal or no edits to the individual files for phpBB. If I simply check for the page and the mode variable within the existing code (which I am already doing) and react accordingly, I can keep that approach.
Put another way… when I said to create variables, I didn’t mean variables in the posting code, but, well, essentially some standard checks that are hard-coded within the page permissions module. So far there’s only one that I know of, so it’s simple to check for page = posting.php and mode=topicreview and set a local (not global) variable to skip the increment. That way I don’t have to edit posting.php.
I think that’s going to be my approach as it works within what I am already doing. And it means zero edits to existing phpBB pages, at least so far.
Comment by Dave Rathbun — August 19, 2007 @ 10:31 pm
Ah, i guess i misunderstood you then. I thought you were adding a variable to posting.php, in which case it would have been more logical to use constants. But if you’re using something like the first idea i posted, that’s also fine
Comment by eviL3 — August 20, 2007 @ 6:54 am