Skip to content

[2.1] Don't log guests if disallowed#9215

Open
sbulen wants to merge 5 commits intoSimpleMachines:release-2.1from
sbulen:21_log_online_fix
Open

[2.1] Don't log guests if disallowed#9215
sbulen wants to merge 5 commits intoSimpleMachines:release-2.1from
sbulen:21_log_online_fix

Conversation

@sbulen
Copy link
Copy Markdown
Contributor

@sbulen sbulen commented May 6, 2026

Partial for #9136
Fixes #7453

Do any guest logging AFTER checking if guests should be kicked...

This will relieve I/O during bot attacks - no need to log folks who aren't doing anything.

Also, the logging was confusing - it made it look like they were doing something in Who's Online.

sbulen added 5 commits May 6, 2026 12:44
Signed-off-by: Shawn Bulen <bulens@pacbell.net>
Signed-off-by: Shawn Bulen <bulens@pacbell.net>
Signed-off-by: Shawn Bulen <bulens@pacbell.net>
Signed-off-by: Shawn Bulen <bulens@pacbell.net>
Signed-off-by: Shawn Bulen <bulens@pacbell.net>
@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented May 7, 2026

Note that the 'User has been taken to the login page' functionality didn't work in 2.1... Worked OK in 3.0.

It may have been less confusing to users if it were displayed properly in 2.1... Maybe not... In 3.0 it said, "User is viewing board 1.0" (or whatever action) with an icon, which, when the icon was clicked, provided a popup that said "User has been taken to the login page". Contradictory info, and the real state was dependent on folks thinking of clicking on that tiny icon.

Comment thread index.php
Comment on lines -243 to -265
// Do some logging, unless this is an attachment, avatar, toggle of editor buttons, theme option, XML feed, popup, etc.
$no_stat_actions = array(
'about:unknown' => true,
'clock' => true,
'dlattach' => true,
'findmember' => true,
'helpadmin' => true,
'jsoption' => true,
'likes' => true,
'modifycat' => true,
'pm' => array('sa' => array('popup')),
'profile' => array('area' => array('popup', 'alerts_popup', 'download', 'dlattach')),
'requestmembers' => true,
'smstats' => true,
'suggest' => true,
'uploadAttach' => true,
'verificationcode' => true,
'viewquery' => true,
'viewsmfile' => true,
'xmlhttp' => true,
'.xml' => true,
);
call_integration_hook('integrate_pre_log_stats', array(&$no_stat_actions));
Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We should probably leave this array here just in case some site admin decided to modify it, unlikely as it is.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Extremely unlikely that's been changed, IMO.

And to me it makes sense to keep the logging logic together, it's more readable.

In fact, it could (should?) be a single function call. An argument could be made to put that logic in Logging.php...

@live627
Copy link
Copy Markdown
Contributor

live627 commented May 7, 2026

This looks like it might stop logging for valid topic and board requests.

@sbulen
Copy link
Copy Markdown
Contributor Author

sbulen commented May 7, 2026

This looks like it might stop logging for valid topic and board requests.

All this did was move the logging to AFTER the kickGuest test. What it does today is log invalid requests. This is very confusing, comes up in the support boards regularly. Folks deny guest access, and during a bot attack, Who's Online shows hundreds of guests/bots browsing the forum & doing various actions. When they aren't...

So it's the other way around - today it logs invalid requests, and this change stops that. The log will make more sense.

@live627
Copy link
Copy Markdown
Contributor

live627 commented May 7, 2026

That function has a very weird design where it returns a function name. The problem is that the complex branching logic includes topics.

3.0 thankfully unravels it somewhat...

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants