Skip to content

[2.1] Ensure we set the cookie when presenting the login form#9167

Open
jdarwood007 wants to merge 10 commits intoSimpleMachines:release-2.1from
jdarwood007:fix9158
Open

[2.1] Ensure we set the cookie when presenting the login form#9167
jdarwood007 wants to merge 10 commits intoSimpleMachines:release-2.1from
jdarwood007:fix9158

Conversation

@jdarwood007
Copy link
Copy Markdown
Member

Fixes #9158

@sbulen, given my reproducibility instructions, are you able to confirm this works?

@jdarwood007
Copy link
Copy Markdown
Member Author

I should note that if bots hit the login page, they will generate cookies. This has to occur in order for session data to be able to carry over. There is no workaround for this. We can only limit the cookie generation to specific pages

@sbulen
Copy link
Copy Markdown
Contributor

sbulen commented Mar 22, 2026

I'll test later.

Pretty sure this won't work though. In the scenario discussed, sa=login is never invoked. The non-modal form directly invokes login2.

@jdarwood007
Copy link
Copy Markdown
Member Author

Well, that can be fixed by updating the KickGuest() logic.

However, this will essentially negate the fixes for bot improvements on sessions for forums with guest access disabled.

There is no simple workaround for this.

We would have to change the kick guest to not issue the login template, but rather provide a simple direct link to the login page.

This also affects maintenance mode.

@jdarwood007
Copy link
Copy Markdown
Member Author

So this fix actually is tackling 3 parts

First part is the login modal, in which I just ensure that when we hit action=login, we generate a cookie always. This can increase the abuse by bots, but there is no avoiding this. We need cookies to process a login. To avoid abuse of action=login, there is options in apache to do rate limiting that would be better than what we can in SMF.

The second fix is for the KickGuest action that occurs for guests who visit the forum, and guest access is disabled. If a cookie has not been set, it presents a link to the action=login. The action=login will set the cookie and allow them to proceed.

The last part is for InMaintenance, which is set when the forum is in Maintenance mode. This is similar to KickGuest, but with a side effect that even in maintenance mode, we don't truly run the login action. So if a cookie is still not present, they will just get the same page. I don't think its worth it in maintenance mode to run logic to figure out that out and provide any help.

@sbulen
Copy link
Copy Markdown
Contributor

sbulen commented Mar 22, 2026

A couple notes -

  • To the best of my knowledge, the issue has nothing to do with cookie creation. Extra cookies aren't the problem. Physical DB writes to the session cause issues, not cookies. In fact, the problem is almost the opposite: a couple of forums are not getting a new PHP cookie after the session is destroyed upon logout. The lack of the new PHP cookie with a fresh session ID for some folks is the issue.
  • PHP creates that cookie, not SMF.

Ideally, we'd find why those folks aren't getting new PHP session cookies upon logout. What is different about their config that is causing that.

@jdarwood007
Copy link
Copy Markdown
Member Author

After more thinking, I should revert to the process to just create the cookie on every page that would be affected. It seems that the cookie is generated anyway. Even if you hit this page, you're sent the cookie. The difference is similar to as @sbulen mentioned and PHP is generating it. The difference is that since SMF doesn't call for it, when it exits, the session data is lost. So by calling it in SMF, we do get the cookie information to store the session properly.

@jdarwood007
Copy link
Copy Markdown
Member Author

Found another case of an invalid session.
Interesting way to trigger this.

  1. Visit the forum with guests disabled
  2. Delete the cookies, do not refresh
  3. Attempt to login once, get a cookie error
  4. Attempt to login again, get a session validation error

This is because in Login2 when we find out that $_COOKIE doesn't exist, we exit and present the login form. It needs a cookie. SMF has to generate the cookie in order for the session data to succeed. We can't rely on PHP as it seems that the session data is simply just lost.

@sbulen
Copy link
Copy Markdown
Contributor

sbulen commented Mar 28, 2026

Looks like you need to add $modSettings to the list of globals on line 30.

https://www.simplemachines.org/community/index.php?msg=4200739

@sbulen
Copy link
Copy Markdown
Contributor

sbulen commented Apr 2, 2026

This PR passes all my tests (with the minor tweak noted above).

I have been able to successfully reproduce the original reported error in a vanilla 2.1.7 environment. To reproduce the EXACT issue the users reported, the same steps should work in 2.1.6 and fail in 2.1.7. I have always gotten similar session verification messages by deleting the PHP cookie, but, I'd get the same behavior in 2.1.6, so I don't think it faithfully reproduced the reported problem. Also, in 2.1.7 I would get a "check your cookie settings" message, but that was not reported by the users, so I don't think we were reproducing the same scenario.

OTOH... The following steps will reproduce it exactly. It'll work fine in 2.1.6, and fail in 2.1.7 with the "session verification" error. With no additional cookie or token errors in either version.

Steps:

  • Guest browsing must be disabled.
  • Log out of all apps & close all tabs. (Any apps that leave cookies around, e.g., phpmyadmin & adminer, will interfere with the test...)
  • Close the browser.
  • Open the browser.
  • Go straight to SMF & login.

No need to manually delete cookies or sessions. Also, running 2.1.7 with the empty cookie check removed works OK, which is what the users are seeing as well.

I'm not sure if this info would change the approach at all, but I feel better that I can finally reproduce the same symptoms.

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

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants