Stop accepting a code once the address changes (3.3) - #217
Open
nursoda wants to merge 2 commits into
Open
Conversation
nursoda
added
security
Codecov Report✅ All modified and coverable lines are covered by tests. 📢 Thoughts on this report? Let us know! |
A code stays valid for minutes, and it was mailed to the address in force when it was issued. Nothing dropped it when that address changed, so the code stayed acceptable while the mailbox holding it had already moved on — correcting a typo is enough for it to belong to someone else, and a group subadmin or a directory sync can make that change. The window is the remaining validity of the code, and whoever holds the old mailbox needs the password as well, so this is narrow rather than urgent. A listener on the event EMailDeleted already uses now drops the stored code whenever the address for delivery changes, clearing included. The user gets a new code at the new address on the next attempt. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olav Seyfarth <olav@seyfarth.de>
Every login challenge asks CodeStorage for the current code, and a user who has none reads a timestamp of 0 — which always counts as expired, so readCode() called deleteCode(), which issued two DELETE statements against oc_preferences for rows that do not exist. Nextcloud's deleteUserConfig() always sends the statement; it does not check its own cache first. The new listener makes this weigh more: a directory sync that touches many addresses now runs it per account, including accounts that never used this app. The deletes are skipped when neither key holds anything. A timestamp without a code still has to go, because deleteExpired() finds users by their timestamp alone and would otherwise count a removal that never happened. One existing test claimed to cover an expired code but described a user who never had one: a timestamp of 0 with no code stored. It now sets a real stored code with an hour-old timestamp, which is what its name says. Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com> Signed-off-by: Olav Seyfarth <olav@seyfarth.de>
nursoda
force-pushed
the
security/drop-the-code-when-the-address-changes-3.3
branch
from
August 26, 2026 18:27
815fc18 to
300cbcd
Compare
This was referenced Aug 27, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The 3.3 twin of #214. Same change, same reasoning; this text
repeats what matters so the pull request stands on its own.
A one-time code is mailed to whatever address the account carries at that moment.
If the address then changes, the mailbox holding that code may belong to someone
else — and until now the code stayed valid, so whoever held it could finish the
login. Changing or clearing the address now drops any pending code.
The app listens for Nextcloud's
UserChangedEvent, which every documented way ofwriting the address emits. An unchanged value is ignored so that rewriting the same
address cannot break a login in progress; that comparison casts both sides to
string, because for an address that was never set the emitter compares against
null, and clearing an already empty address would otherwise drop a code that isstill deliverable through the account's primary address.
Carried along is a database write that was never needed: every login challenge asks
for the current code, and a user who has none reads a timestamp of
0, which alwayscounts as expired — so two DELETE statements went out for rows that do not exist.
They are skipped now, while a timestamp left behind without a code is still removed,
because the cleanup command finds such users by their timestamp alone.
What is deliberately missing here: the smoke test. This line has no
tests/smoke/, so the end-to-end proof exists only on the main line.Checked
php-legacy, because Psalm 6.8.2 refuses torun on PHP 8.5
🤖 Generated with Claude Code, verified, tweaked and approved by @nursoda.