Feat: Add timer to login screen when cooldown period is active#2264
Conversation
WalkthroughThe login failure handling was revised to always throw an exception with an embedded HTML countdown span when failures exceed the maximum allowed. Server-side logging was enhanced to include cooldown messages. The login form’s error display was updated for dynamic DOM manipulation, and a JavaScript countdown timer was added to visually indicate the cooldown period on the client side. Changes
Poem
📜 Recent review detailsConfiguration used: CodeRabbit UI 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
✨ Finishing Touches
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (1)
emhttp/plugins/dynamix/include/.login.php (1)
198-206: Race-condition / double-write risk
appendToFile($failFile, $time."\n");immediately followed by a secondcleanupFails()call re-opens the same file only a few µs later.
On a busy box with many failed attempts this tight write-then-read loop can:
- Produce inter-leaved timestamps from concurrent PHP processes because the second
flock()incleanupFails()briefly unlocks the file after the firstappendToFile().- Inflate
$failCountif two writers land in the gap.A cheaper, safer pattern is:
// append + clean-up inside one critical section flock($fp, LOCK_EX); fwrite($fp, $time."\n"); rewind($fp); $lines = array_filter(explode("\n", fread($fp, filesize($failFile))), 'strlen'); ...or, at minimum, skip the immediate clean-up and rely on the next request to purge stale entries.
zackspear
left a comment
zackspear
left a comment
There was a problem hiding this comment.
Looking forward to trying this out
| $coolReset = ""; | ||
| if ($failCount > $maxFails) { | ||
| if ($failCount == ($maxFails - 1)) { | ||
| $error .= '<br>'.sprintf(_('Logins prevented for %s'),'<span id="countdown"></span>'); |
There was a problem hiding this comment.
Ideally we don't use <br> tags to create spacing.
The proper way would be to use a new <p> tag and the $error var in PHP would be an array called $errors. That way each one would be output in a <p> or <li>.
Granted I realize that would mean further refactors and IMO there's bigger fish to fry. So I'm not asking for any changes.
Just know in the future this might be something I push back on. :)
4 participants
Summary by CodeRabbit
New Features
Bug Fixes
Style