Skip to content

fix(activesync): refuse Draft Modify append-as-new for stale UIDs#201

Open
TDannhauer wants to merge 2 commits into
FRAMEWORK_6_0from
fix/activesync-draft-modify-stale-uid
Open

fix(activesync): refuse Draft Modify append-as-new for stale UIDs#201
TDannhauer wants to merge 2 commits into
FRAMEWORK_6_0from
fix/activesync-draft-modify-stale-uid

Conversation

@TDannhauer

Copy link
Copy Markdown
Contributor

Summary

  • Refuse EAS 16 Drafts-folder Modify when the client ServerId (IMAP UID) is already missing: do not append a new draft
  • Load the existing draft before setDraftMessage so stale UIDs fail fast and attachment carry-over order stays correct
  • Unit test covers the no-append path

Motivation

Defense in depth for horde/ActiveSync#85. The ActiveSync importer short-circuits retries when the prior apply was recorded; if that map write never landed, the previous Core behaviour (“stale UID → treat as new”) could still append unbounded copies on retry.

Changes

  • Horde_Core_ActiveSync_Driver::changeMessage Drafts branch: missing Modify UID → log notice, return false
  • DriverDraftModifyStaleUidTest

Test plan

When an EAS 16 Drafts Modify references an IMAP UID that is already gone,
do not treat it as a new draft append. Retries after a lost Sync response
would otherwise grow the Drafts folder unbounded. Defense in depth for
horde/ActiveSync#85 (idempotent importer short-circuit).
@TDannhauer
TDannhauer requested a review from ralflang July 17, 2026 16:12
Comment thread doc/changelog.yml Outdated
@@ -1,4 +1,14 @@
---
3.2.1:

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Don't to that - this is the release tool's job. You also create a mess as soon as another PR is released faster.

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good point, i'll instruct my agents.md

$driver->setProtocolVersion(Horde_ActiveSync::VERSION_SIXTEEN);

$userProp = new \ReflectionProperty(
\Horde_ActiveSync_Driver_Base::class,

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Use "use" statement here.

Drop the hand-edited changelog entry (release tool owns it) and use import statements in the unit test instead of inline FQCNs.
@TDannhauer

Copy link
Copy Markdown
Contributor Author

Addressed the review notes:

  • Reverted the hand-edited doc/changelog.yml entry (release tool owns that file).
  • Replaced inline \ReflectionProperty / \Horde_ActiveSync_Driver_Base with use imports in DriverDraftModifyStaleUidTest.

@TDannhauer
TDannhauer requested a review from ralflang July 18, 2026 09:29
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants