Repository navigation
fix(settings): Don't fail when a stored domain is added again - #13976
Open
DerDreschner wants to merge 3 commits into
Open
DerDreschner wants to merge 3 commits into
DerDreschner wants to merge 3 commits into
Conversation
Adding a trusted sender or an internal address only skipped the insert when the value was already covered by a stored entry. For domain entries that check compares the host of the given address, but a domain itself has no "@", so Horde finds no host and a stored domain was never found. The insert then failed on the unique index: the API answered with HTTP 500 and the user migration import aborted. Two parallel requests for the same new value failed the same way. Both services now treat a unique constraint violation on insert as "already stored", as the user migration already does for tags. exists() stays unchanged, because it also decides whether a sender is trusted when a message is shown. insertIgnoreConflict() isn't used, because on MySQL it runs INSERT IGNORE, which also hides truncated values. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: David Dreschner <david.dreschner@nextcloud.com>
DerDreschner
added
bug
3. to review
AI assisted
DerDreschner
requested review from
ChristophWurst,
GretaD and
kesselb
as code owners
October 30, 2026 10:13
DerDreschner
enabled auto-merge
October 30, 2026 10:13
Member
Author
|
/backport to stable5.13 |
1 task done
DerDreschner
added
the
skill:backend
Trusted senders and internal addresses accepted any text and any type, through the API as well as the user migration import, so entries that can never match a sender were stored. A new entry now has to be an address by RFC 5322 and RFC 6532 for the type "individual", parsed with Horde's strict mode, or a host name (RFC 1123, internationalized by IDNA2008 with the UTS #46 mapping) or an address literal (RFC 5321) for the type "domain". Horde's strict mode accepts a trailing dot, over-long labels, any address literal, invalid UTF-8 and control characters, so the encoding and the domain part are checked separately. Control characters, invisible format characters and line or paragraph separators are rejected, although RFC 5322 and RFC 6532 allow some of them, because they can break the UI or disguise an address. The zero-width joiner and non-joiner stay allowed, because names in Persian or Indian languages such as Hindi need them. Entries are stored in the form Horde produces for a sender's address (quotes and brackets removed, the domain lowercased and IDN-decoded), because that's the form senders are compared with. Domains are mapped by UTS #46 first, so other spellings of the same domain match as well. The stored form is limited to the 254 octets of an RFC 5321 path, since IDN decoding can lengthen it and Oracle's columns count bytes; very long internationalized domains are rejected because of that. Invalid input is answered with HTTP 422, and the import skips such entries with its existing notice. Senders whose local part needs its quotes, or with an IPv6 literal, can't be trusted individually anymore, because their compared form isn't a valid address. Trusting their domain still works. Removing an entry isn't validated, so existing invalid entries can still be deleted; they aren't cleaned up automatically. Assisted-by: ClaudeCode:claude-opus-5-5 Signed-off-by: David Dreschner <david.dreschner@nextcloud.com>
1 task done
…usted-addresses fix(settings): Reject invalid addresses and domains for internal addresses and trusted senders
kesselb
reviewed
Oct 31, 2026
| $normalized = match ($type) { | ||
| self::TYPE_INDIVIDUAL => $this->normalizeAddress($value), | ||
| self::TYPE_DOMAIN => $this->normalizeDomain($value), | ||
| default => throw new InvalidAddressException('Invalid type'), |
Contributor
There was a problem hiding this comment.
throws InvalidAddressException('Invalid type'), which reports a programming mistake as bad user input. An enum, or reusing the constants the mappers already have, would make that case impossible.
| use Horde_Mail_Rfc822_Address; | ||
| use OCA\Mail\Exception\InvalidAddressException; | ||
|
|
||
| class AddressOrDomainNormalizer { |
Contributor
There was a problem hiding this comment.
Rename it after what it produces, not what it accepts, such as TrustedEntryNormalizer, or something that matches the individual/domain type values.
This branch has not been deployed
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
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 database entries for any trusted domain or internal sender (both individual and domain) must be unique. If someone tries to add a trusted domain or internal sender that's already there, the request fails with an HTTP 500 response and a "Could not add internal address {address}" error message. This is pretty confusing, as what the user wants to achieve in the end - a domain or sender should be trusted / marked as internal - is done, so, why an error message?
To handle that case better, this PR catches the database exception and returns
nullwhere needed (internal addresses). That allows the controller to send out proper 200 / 201 responses.References to: #5139
🤖 AI (if applicable)