Recent Posts does not properly adhere to unapproved topics

[Arantor]

2.0.2 - happens on this very board. In fact I swear I reported this before, though I seem to recall at the time it was concluded to be caching - it definitely isn't.

If a topic is unapproved but a moderator replies, the moderator's post is automatically considered approved, even if the topic isn't, and so Recent Posts shows that moderator's reply (as m.approved = 1, as apparently no check for t.approved = 1 or user has can_approve)

(For those playing along at home, I'm 99% certain Wedge actually patched this when Nao added topic privacy, which is probably why I never thought too much about it until I noticed it here again just now)

[mashby]

I love this part!

For those playing along at home

I cannot say you're wrong regarding this bug report. I just don't see this board as being subject to approval posts. Do you mean another board like Site Showcase? Can we try to replicate it there or am I just being Jane, the ignorant slut?

[Arantor]

By 'this board' I mean this site.

You won't be able to see it, because you have approval rights and thus aren't subject to the same lack of permissions I am.

Basically what happens is that when Old Fossil replied to a topic in Showcase saying that the site shouldn't be using a default theme, I was able to see the reply even though I wasn't able to see the topic. The topic is not yet approved, but *his post* is, so his post shows up in Recent Posts (action=recent) even though the topic isn't approved, just because the test is only applied on the post itself, not the topic as a whole.

While this wouldn't necessarily be a big deal on most forums, it could be.

Consider how often the suggestion is made to implement user/staff privacy, and to use post moderation and 'just not approve posts' - except that staff replies will be visible through action=recent. I'd possibly even go so far as to suggest this is a privacy issue.

[mashby]

Your specifics are extremely enlightening. I do see what you are referring to. I took a look at the (!) thing that appears on Site Showcase and it sure does reflect your findings. I complete agree with your findings regarding privacy/security. Yes, it might be minor in impact, but the philosophy of it certainly isn't being adhered to in the site code. I, for one, have no idea how to fix it, but maybe it will be addressed in 2.1.1?

[Arantor]

It's relatively low on this site, but knowing how others may choose to use post moderation, I can't rule out that it has a potentially nasty privacy consequence.

FWIW, I'd suggest it should be patched in the next 2.0.x branch, not a 2.1 fix.

That said, let me elaborate a bit more now I've had time to examine the code. The offender, this beast in Recent.php:

Code Select Expand

// Find the 10 most recent messages they can *view*.
// !!!SLOW This query is really slow still, probably?
$request = $smcFunc['db_query']('', '
SELECT m.id_msg
FROM {db_prefix}messages AS m
INNER JOIN {db_prefix}boards AS b ON (b.id_board = m.id_board)
WHERE ' . $query_this_board . '
AND m.approved = {int:is_approved}
ORDER BY m.id_msg DESC
LIMIT {int:offset}, {int:limit}',
array_merge($query_parameters, array(
'is_approved' => 1,
'offset' => $_REQUEST['start'],
'limit' => 10,
))
);

It confirms one supposition and disproves another: it is based solely on m.approved, but I suspected it would elect not to carry out that test if the user was a moderator - it does not, it simply filters solely on approval.

Now, the reason for that is pure performance: there's no join to the topics table, thus the state of the topic being approved or not approved is not known at that point, and doing that does potentially slow it down significantly.

The simplest rewrite to include this would merely be:

Code Select Expand

// Find the 10 most recent messages they can *view*.
// !!!SLOW This query is really slow still, probably?
$request = $smcFunc['db_query']('', '
SELECT m.id_msg
FROM {db_prefix}messages AS m
INNER JOIN {db_prefix}boards AS b ON (b.id_board = m.id_board)
INNER JOIN {db_prefix}topics AS t ON (t.id_topic = m.id_topic)
WHERE ' . $query_this_board . '
AND m.approved = {int:is_approved}
AND t.approved = {int:is_approved}
ORDER BY m.id_msg DESC
LIMIT {int:offset}, {int:limit}',
array_merge($query_parameters, array(
'is_approved' => 1,
'offset' => $_REQUEST['start'],
'limit' => 10,
))
);

This would confine it to approved posts only (and still in the boards the user can see) which would close the loophole, but would also remove the ability of moderators (who can approve) from seeing unapproved topics entirely.

Personally, I don't think that's a huge deal (considering there are other places where this is already an issue, e.g. in the unread posts where unapproved topics do not turn up IIRC even if you would otherwise have permission to see them)

[mashby]

Well, I certainly cannot speak for 2.0.x or 2.x releases. It will be ready when it's ready (even if "it" isn't defined), but this part is interesting:

This would confine it to approved posts only (and still in the boards the user can see) which would close the loophole, but would also remove the ability of moderators (who can approve) from seeing unapproved topics entirely.

You're just referring to the action=recent part? The fix you provided wouldn't affect the (!) that I see on the Showcase board. I'm pretty sure that's true. It would just impact the action=recent.

[Arantor]

Yup, the only issue I'm seeing is confined to action=recent. The suggested fix is also confined to action=recent. (It would just mean that only approved topics would ever show in action=recent, as opposed to unapproved topics with approved replies, the current state of play)

I can't see the (!) on the Showcase board, nor can I see any unapproved posts or topics in it by accessing the board directly. All I can see are approved posts via action=recent.

[mashby]

Your attention to detail is very admirable. How much of an impact do you see on this, referring to this point:

Now, the reason for that is pure performance: there's no join to the topics table, thus the state of the topic being approved or not approved is not known at that point, and doing that does potentially slow it down significantly.

How significant is it? Sure seems like the right thing to do (correct the code) though.

[Arantor]

It's already a slow query. On a big forum it's going to be painful. Any extra join makes it slower. I haven't benchmarked it on this site (though I would imagine a dev could do some decent benchmarks given this size of database)

Exactly what the effect will be, I don't know.

[emanuele]

Thanks Arantor for the report, that's indeed a problem (maybe worth a patch?...that's Norv's problem ).

I don't use much the post moderation, so I don't have a good overview, but doing a quick test (on the 2.1 code base, I don't remember changes in that area, but worth mention) it seems that when the first post of a topic is approved all the posts in that topic are approved as well.
And apparently this query is known to be "not so fast".
So, instead of touching this query wouldn't be the same to post as unapproved all the messages (irrespective of the author) when a topic is not approved?
That would avoid the need to join the topics table, downside the topic would become completely pink...

[Arantor]

it seems that when the first post of a topic is approved all the posts in that topic are approved as well.

I'm not sure that's the best behaviour, but I'm not sure the alternative is any better either.

So, instead of touching this query wouldn't be the same to post as unapproved all the messages (irrespective of the author) when a topic is not approved?

That would also work.

That would avoid the need to join the topics table, downside the topic would become completely pink...

But it requires altering the way moderation is handled, and more importantly would be more confusing to users - imagine getting 'Why is my post moderated? I'm the admin!' support requests.

(Of course, I've set myself up for that with Wedge but the UI is set up so that even admins can be subject to moderation under specific circumstances.)

[emanuele]

it seems that when the first post of a topic is approved all the posts in that topic are approved as well.

I'm not sure that's the best behaviour, but I'm not sure the alternative is any better either.

Yep, I don't like that behaviour so much...but it sort of makes sense...sort of...

That would avoid the need to join the topics table, downside the topic would become completely pink...

But it requires altering the way moderation is handled, and more importantly would be more confusing to users - imagine getting 'Why is my post moderated? I'm the admin!' support requests.

Users are always confused!
BTW it could be a nice "reminder" that the topic is still visible only to moderators. Don't know if useful though.

[Arantor]

BTW it could be a nice "reminder" that the topic is still visible only to moderators. Don't know if useful though.

It's visible to anyone with the approve permission and the originating user, so even then you'd have to tailor the note depending on it being the topic starter or a moderator.

[Arantor]

Moved to Github where it is centrally tracked: https://github.com/SimpleMachines/SMF2.1/issues/1292