fix(dav): handle Thunderbird accept-before-sync UID collisions#61207
fix(dav): handle Thunderbird accept-before-sync UID collisions#61207ndo84bw wants to merge 2 commits into
Conversation
Signed-off-by: Richard Steinmetz <richard@steinmetz.cloud> Signed-off-by: Nico Donath <ndo84bw@gmx.de>
ndo84bw
requested review from
Altahrim,
come-nc,
provokateurin and
salmart-dev
and removed request for
a team
ndo84bw
changed the title
joshtrichards
added
bug
3. to review
Assisted-by: ClaudeCode:claude-opus-4-8 Signed-off-by: Nico Donath <ndo84bw@gmx.de>
ndo84bw
force-pushed
the
fix/54471-thunderbird-put-uid-collision
branch
from
f3a6854 to
20045d6
Compare
ChristophWurst
requested review from
SebastianKrupinski
and removed request for
Altahrim,
provokateurin and
salmart-dev
|
Thanks for the contribution. We will have a look soon |
SebastianKrupinski
left a comment
SebastianKrupinski
left a comment
There was a problem hiding this comment.
Hi https://github.com/ndo84bw
Thank you for the PR and the continued contribution.
I have reviewed your changes and made some comments.
BUT... In general, I am against making quirky fixes like this... You are essentially trying to fix, client logic flaws, with server code. The desktop client should make sure it has the latest version of the calendar before creating a new event... not the other way around, the server should not try to fix the clients bad logic, by guessing what the client is trying to do.
| // Before the ACL plugin (priority 20) so its PUT check sees the rewritten object. | ||
| $server->on('beforeMethod:PUT', $this->beforePut(...), 19); |
There was a problem hiding this comment.
The ACL plugin should also be first, bypassing it like this is not a good idea.
| // Same calendar as the request, real calendar objects only, nothing trashed. | ||
| $qb = $this->db->getQueryBuilder(); | ||
| $qb->select('co.uri', 'co.calendardata') | ||
| ->from('calendarobjects', 'co') | ||
| ->join('co', 'calendars', 'c', $qb->expr()->eq('co.calendarid', 'c.id')) | ||
| ->where( | ||
| $qb->expr()->eq( | ||
| 'c.principaluri', | ||
| $qb->createNamedParameter($currentUserPrincipal, IQueryBuilder::PARAM_STR), | ||
| IQueryBuilder::PARAM_STR, | ||
| ), | ||
| $qb->expr()->eq( | ||
| 'c.uri', | ||
| $qb->createNamedParameter($calendarUri, IQueryBuilder::PARAM_STR), | ||
| IQueryBuilder::PARAM_STR, | ||
| ), | ||
| $qb->expr()->eq( | ||
| 'co.uid', | ||
| $qb->createNamedParameter($uid, IQueryBuilder::PARAM_STR), | ||
| IQueryBuilder::PARAM_STR, | ||
| ), | ||
| $qb->expr()->eq( | ||
| 'co.calendartype', | ||
| $qb->createNamedParameter(CalDavBackend::CALENDAR_TYPE_CALENDAR, IQueryBuilder::PARAM_INT), | ||
| IQueryBuilder::PARAM_INT, | ||
| ), | ||
| $qb->expr()->isNull('co.deleted_at'), | ||
| $qb->expr()->isNull('c.deleted_at'), | ||
| ); | ||
| $result = $qb->executeQuery(); | ||
| $rows = $result->fetchAll(); | ||
| $result->closeCursor(); | ||
|
|
||
| if (count($rows) !== 1) { | ||
| // Either no collision or too many collisions | ||
| return; | ||
| } | ||
|
|
There was a problem hiding this comment.
A few issues with this:
a) we should not be building database requests inside the plugin, we have a backend for this reason.
b) this will essentially only work with internal caldav calendars, this will fail, with calendars provided by other apps.
| return; | ||
| } | ||
|
|
||
| // Restore SCHEDULE-AGENT from the stored organizer so server-side scheduling still runs. |
There was a problem hiding this comment.
We should not be merging two different data sets like this, this will cause very unpredictable behavior. if the client purposely removes the " SCHEDULE-AGENT" from the properties, this would force it back, essentially you would never be able to remove them.
|
Thanks for taking the time to dig into the code. The issue dates back to 2019, @st3iny PR for it is from 2025, and I tried to rescue that code - and now we're left with a bit of a mess, only to realize that this is really a client-side problem that we don't actually want to solve in code. I'm going to take a step back and think it over, and I'll set this PR to draft. |
|
Hello there, We hope that the review process is going smooth and is helpful for you. We want to ensure your pull request is reviewed to your satisfaction. If you have a moment, our community management team would very much appreciate your feedback on your experience with this PR review process. Your feedback is valuable to us as we continuously strive to improve our community developer experience. Please take a moment to complete our short survey by clicking on the following link: https://cloud.nextcloud.com/apps/forms/s/i9Ago4EQRZ7TWxjfmeEpPkf6 Thank you for contributing to Nextcloud and we hope to hear from you soon! (If you believe you should not receive this message, you can add yourself to the blocklist.) |
|
After Sebastian's review I agree this does not belong in the server: deciding that a PUT with a colliding UID is really an update and rewriting the request means guessing client intent, and that is client logic. Thunderbird should sync before letting you accept (Thunderbird Bug 1717401) What the server can do correctly is answer that collision per spec, so a client has a clean, machine-readable basis to self-correct. That is the RFC 4791 / 6352 no-uid-conflict precondition (409 with a DAV:href to the existing object), which is being handled in #61659 (originating from my #61640). That is the part worth keeping. So this PR is superseded and I am closing it. Thanks @SebastianKrupinski and @ChristophWurst for the review and the clear reasoning. |
7 participants
Processing message failed. Status: 80004005.#17915Summary
This takes over and completes #54471 with agreement (#54471 (comment)).
The problem
When someone is invited to an event, the server's scheduling code immediately writes a copy of that event into the invitee's personal calendar (under a server-generated object name). If the invitee opens the invitation email in Thunderbird and clicks Accept/Decline before their calendar has synced, Thunderbird does not yet know that object name. It therefore sends the PUT to a name it guesses itself. Because the event UID already exists in the calendar, the server rejects the PUT and Thunderbird shows error
80004005; the acceptance is lost.What this PR does
Commit 1 (original work by @st3iny, rebased onto master, fixup commits squashed): a Sabre plugin that, for a Thunderbird PUT of a calendar object whose UID already exists in the same target calendar, rewrites the request URI to the existing object so the PUT updates it instead of failing on the collision.
Commit 2 (this take-over): completes and hardens that plugin.
Deliver the organizer's reply. Thunderbird's accept-before-sync body stamps the organizer with
SCHEDULE-AGENT=CLIENT. That parameter tells the server not to perform scheduling, so no iTip REPLY is generated and the organizer never learns of the acceptance (they stayNEEDS-ACTION, no email). An attendee is not allowed to change the ORGANIZER property (RFC 5546), so the plugin restoresSCHEDULE-AGENTto the value of the stored event. This is a no-op when the organizer was genuinely client-scheduled (the stored copy already carriesCLIENT) and re-enables the reply in the common server-scheduled case.Let the rewritten PUT through. Thunderbird sends
If-None-Match: *because it believes it is creating a new object. After the URI has been rewritten to an existing object that precondition would fail with 412, so the header is dropped on the rewrite path.Only act on a genuine accept-before-sync. The plugin now returns early when Thunderbird already targets the object's real URI (i.e. the calendar was already synced). This avoids touching the body on a normal update - in particular it never strips a
SCHEDULE-AGENTthat the organizer set on their own copy.Authorization for the rewrite target. The plugin now runs before the ACL plugin (event priority 19 vs. the ACL plugin's 20; the current user principal is provided by the auth plugin at priority 10 and is available either way). The ACL plugin only checks PUT privileges when the target node exists; the original (guessed) URI never exists, so without this change the privilege check was skipped and not repeated after the rewrite. Running first means the ACL plugin evaluates
{DAV:}write-contentagainst the rewritten, existing object.Scope the collision lookup. The lookup is restricted to the calendar the PUT targets (
calendars/{principal}/{calendar}/...) and excludes subscription/federated cache rows (calendartype) and trashed objects/calendars (deleted_at). This prevents rewriting onto an object in a different calendar of the same user, onto a read-only subscription cache, or onto a deleted object.Testing
Manually verified on a live Nextcloud 33.0.3 instance (MariaDB, Apache/mod_php) with this patch applied, using Thunderbird as the invitee/organizer client:
80004005), the PUT succeedsUnit tests (
apps/dav/tests/unit/Connector/Sabre/ThunderbirdPutInvitationQuirkPluginTest.php) cover the URI rewrite, theIf-None-Matchremoval, the early return when the object URI already matches, and theSCHEDULE-AGENTrestore (restored from the stored organizer, left untouched when already equal, dropped when the stored organizer has none, and skipped when the stored data is unparsable). 17 tests pass locally with the flags CI uses (--fail-on-warning --fail-on-risky).The authorization change (item 4) is not reproduced as a live exploit here; it is established from the Sabre ACL plugin source (the privilege check is bound to
beforeMethodand skipped for a non-existent node, with no second check on the update path) and covered by the unit test asserting the registration priority.Checklist
3. to review, feature component)stable32)AI (if applicable)