Skip to content

Commit 8dfbce6

Browse files
committed
fix(xmtp_db,xmtp_mls): tolerate unknown intent kinds so future downgrades don't wedge outbound
1 parent 59f5c02 commit 8dfbce6

5 files changed

Lines changed: 221 additions & 12 deletions

File tree

crates/xmtp_db/src/encrypted_store/group_intent.rs

Lines changed: 112 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -68,6 +68,36 @@ pub enum IntentKind {
6868
AppDataUpdate = 12,
6969
}
7070

71+
impl IntentKind {
72+
/// Every kind this build knows how to deserialize. Production
73+
/// queries pass this as the `allowed_kinds` filter so rows written
74+
/// by a NEWER build (which may use discriminants this build has no
75+
/// variant for) are excluded in SQL instead of poisoning the whole
76+
/// `load()` — `FromSql` errors on unknown discriminants, and one
77+
/// such row would otherwise wedge every intent query for the group
78+
/// after an app downgrade. Unknown-kind rows stay untouched in the
79+
/// table and resume processing when the app is upgraded again.
80+
///
81+
/// Keep in sync with the enum — the `intent_kind_all_is_exhaustive`
82+
/// test enforces it.
83+
pub fn all() -> Vec<IntentKind> {
84+
vec![
85+
IntentKind::SendMessage,
86+
IntentKind::KeyUpdate,
87+
IntentKind::MetadataUpdate,
88+
IntentKind::UpdateGroupMembership,
89+
IntentKind::UpdateAdminList,
90+
IntentKind::UpdatePermission,
91+
IntentKind::ReaddInstallations,
92+
IntentKind::ProposeMemberUpdate,
93+
IntentKind::ProposeGroupContextExtensions,
94+
IntentKind::CommitPendingProposals,
95+
IntentKind::BootstrapMigration,
96+
IntentKind::AppDataUpdate,
97+
]
98+
}
99+
}
100+
71101
impl std::fmt::Display for IntentKind {
72102
fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result {
73103
let description = match self {
@@ -746,6 +776,88 @@ pub(crate) mod tests {
746776
.unwrap()
747777
}
748778

779+
/// `IntentKind::all()` is the downgrade-tolerance allowlist — if a
780+
/// variant is added to the enum but not the list, production
781+
/// queries silently stop seeing intents of that kind. Iterating
782+
/// discriminants 1..=max pins the exhaustiveness.
783+
#[xmtp_common::test]
784+
fn intent_kind_all_is_exhaustive() {
785+
let all = IntentKind::all();
786+
let max = all.iter().map(|k| *k as i32).max().unwrap();
787+
assert_eq!(
788+
all.len() as i32,
789+
max,
790+
"IntentKind::all() must contain every discriminant 1..={max} exactly once"
791+
);
792+
for expected in 1..=max {
793+
assert!(
794+
all.iter().any(|k| *k as i32 == expected),
795+
"IntentKind::all() is missing discriminant {expected}"
796+
);
797+
}
798+
}
799+
800+
/// Downgrade simulation: a row whose `kind` discriminant this build
801+
/// doesn't know (written by a future version) must not poison
802+
/// kind-filtered queries. Unfiltered queries still error — pinned
803+
/// here so a future change to that behavior is a conscious one.
804+
#[xmtp_common::test]
805+
fn unknown_kind_row_is_excluded_by_kind_filter() {
806+
let group_id = GroupId::generate();
807+
808+
with_connection(|conn| {
809+
insert_group(conn, group_id);
810+
811+
// A known-kind intent this build must keep seeing.
812+
NewGroupIntent::new_test(
813+
IntentKind::SendMessage,
814+
group_id,
815+
rand_vec::<24>(),
816+
IntentState::ToPublish,
817+
)
818+
.store(conn)
819+
.unwrap();
820+
821+
// A future-kind row (discriminant beyond every known
822+
// variant), inserted raw — exactly what a newer build
823+
// leaves behind before an app downgrade.
824+
let future_kind = IntentKind::all().len() as i32 + 1;
825+
conn.raw_query(|raw_conn| {
826+
diesel::insert_into(dsl::group_intents)
827+
.values((
828+
dsl::kind.eq(future_kind),
829+
dsl::group_id.eq(group_id),
830+
dsl::data.eq(rand_vec::<24>()),
831+
dsl::state.eq(IntentState::ToPublish),
832+
dsl::publish_attempts.eq(0),
833+
dsl::should_push.eq(false),
834+
))
835+
.execute(raw_conn)
836+
})
837+
.unwrap();
838+
839+
// Kind-filtered (the production shape): unknown row is
840+
// excluded in SQL, the known intent still comes back.
841+
let intents = conn
842+
.find_group_intents(
843+
group_id,
844+
Some(vec![IntentState::ToPublish]),
845+
Some(IntentKind::all()),
846+
)
847+
.unwrap();
848+
assert_eq!(intents.len(), 1);
849+
assert_eq!(intents[0].kind, IntentKind::SendMessage);
850+
851+
// Unfiltered: the unknown discriminant fails row
852+
// deserialization and poisons the whole query.
853+
assert!(
854+
conn.find_group_intents(group_id, Some(vec![IntentState::ToPublish]), None)
855+
.is_err(),
856+
"unfiltered query should surface the FromSql error for unknown kinds"
857+
);
858+
})
859+
}
860+
749861
#[xmtp_common::test]
750862
fn test_store_and_fetch() {
751863
let group_id = GroupId::generate();

crates/xmtp_mls/src/groups/app_data/mod.rs

Lines changed: 4 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -457,10 +457,10 @@ pub(crate) fn is_migrated_group(mls_group: &OpenMlsGroup) -> bool {
457457
/// [`TEST_REGISTRY_OVERRIDE`] scope the group is treated as migrated
458458
/// regardless of what the dict contains. This bridges the gap for
459459
/// tests that exercise post-bootstrap reader semantics without
460-
/// actually running the bootstrap commit (which writes the real
461-
/// `COMPONENT_REGISTRY` entry — that work lives in a follow-up
462-
/// branch). Production paths never hit this branch because the
463-
/// task-local is only initialized inside test scopes.
460+
/// actually running the bootstrap commit (`enable_proposals()` end to
461+
/// end, which writes the real `COMPONENT_REGISTRY` entry). Production
462+
/// paths never hit this branch because the task-local is only
463+
/// initialized inside test scopes.
464464
pub(crate) fn is_migrated_extensions(
465465
extensions: &openmls::extensions::Extensions<openmls::group::GroupContext>,
466466
) -> bool {

crates/xmtp_mls/src/groups/mls_sync.rs

Lines changed: 16 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -548,10 +548,13 @@ where
548548
tracing::instrument(level = "trace", skip_all)
549549
)]
550550
pub(crate) async fn sync_until_last_intent_resolved(&self) -> Result<SyncSummary, GroupError> {
551+
// Filter to kinds this build understands: after a downgrade,
552+
// rows written by a newer build would otherwise fail `FromSql`
553+
// and poison the whole query (see `IntentKind::all`).
551554
let intents = self.context.db().find_group_intents(
552555
self.group_id,
553556
Some(vec![IntentState::ToPublish, IntentState::Published]),
554-
None,
557+
Some(IntentKind::all()),
555558
)?;
556559

557560
let Some(intent) = intents.last() else {
@@ -2749,10 +2752,11 @@ where
27492752
pub(super) async fn publish_intents(&self) -> Result<(), GroupError> {
27502753
let db = self.context.db();
27512754
self.load_mls_group_with_lock_async(async |mut mls_group| {
2755+
// Kind-filtered for downgrade tolerance — see `IntentKind::all`.
27522756
let intents = db.find_group_intents(
27532757
self.group_id,
27542758
Some(vec![IntentState::ToPublish]),
2755-
None,
2759+
Some(IntentKind::all()),
27562760
)?;
27572761

27582762
for intent in intents {
@@ -3461,11 +3465,10 @@ where
34613465
// similar extension shape can't accidentally trigger
34623466
// the bootstrap path.
34633467
//
3464-
// NOTE: honest receivers reject the bootstrap commit
3465-
// until the receiver-side validator that understands
3466-
// `COMPONENT_REGISTRY` / `GROUP_MEMBERSHIP` writes is
3467-
// wired. Emitting this intent is only useful when the
3468-
// validator lands in the same release.
3468+
// Receive-side validation lives in
3469+
// `validated_commit.rs` (`is_bootstrap_commit` routes
3470+
// into `validate_bootstrap_and_build`, which drives
3471+
// `bootstrap_validator::validate_bootstrap_commit`).
34693472
let intent_data =
34703473
ProposeGroupContextExtensionsIntentData::try_from(intent.data.as_slice())?;
34713474

@@ -3876,7 +3879,12 @@ where
38763879
pub(crate) async fn post_commit(&self) -> Result<(), GroupError> {
38773880
let db = self.context.db();
38783881
let intents =
3879-
db.find_group_intents(self.group_id, Some(vec![IntentState::Committed]), None)?;
3882+
// Kind-filtered for downgrade tolerance — see `IntentKind::all`.
3883+
db.find_group_intents(
3884+
self.group_id,
3885+
Some(vec![IntentState::Committed]),
3886+
Some(IntentKind::all()),
3887+
)?;
38803888

38813889
for intent in intents {
38823890
if let Some(post_commit_data) = intent.post_commit_data {

crates/xmtp_mls_common/src/app_data/registry_table.rs

Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,23 @@ use crate::app_data::{
8989
/// pre-commit registry snapshot, so a same-commit registration
9090
/// followed by a same-commit write fails to dispatch.
9191
///
92+
/// **Floor-bump convention (pause, don't fork).** Any release that
93+
/// introduces something old receivers cannot interpret — a new
94+
/// [`ComponentType`], a new set/map delta mutation tag, a new registry
95+
/// entry format, a reserved-range (`0xFF00+`) allocation, or a change
96+
/// to the bootstrap synthesis encoding — MUST raise
97+
/// `PROPOSALS_MIN_PROTOCOL_VERSION` in the same release AND land each
98+
/// group's `MIN_SUPPORTED_PROTOCOL_VERSION` floor bump in a commit
99+
/// **strictly earlier** than the first commit using the new construct
100+
/// (the floor-bump commit itself must contain nothing format-novel).
101+
/// Receivers below a committed floor pause the group
102+
/// (defer-and-reprocess after upgrade) via the pause-before-parse
103+
/// guards in `xmtp_mls::groups::app_data` and
104+
/// `ValidatedCommit::from_staged_commit`; a same-commit floor bump is
105+
/// NOT protected — its proposal hasn't passed the super-admin policy
106+
/// check when the guards run, and pausing on unvalidated input would
107+
/// let any member freeze a group.
108+
///
92109
/// Two ergonomic patterns for shipping a new component without
93110
/// editing `WELL_KNOWN`:
94111
///
Lines changed: 72 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,72 @@
1+
# App-data validator remediation runbook
2+
3+
What to do when a **fielded release ships a receive-side validation bug** on the
4+
app-data (post-migration) path — a validator that wrongly rejects commits other
5+
versions accept, or wrongly accepts commits it shouldn't. The goal is always the
6+
same: convert divergence into **pause** (recoverable by upgrading) and never into
7+
**fork** (permanent group split). No wire-format migration is required for any
8+
scenario below.
9+
10+
## The lever: per-group protocol-version floor bumps
11+
12+
`MlsGroup::update_group_min_version` (and `update_group_min_version_to_match_self`)
13+
writes the group's `MIN_SUPPORTED_PROTOCOL_VERSION` floor — the legacy
14+
`GroupMutableMetadata` attribute pre-migration, the `0x800A` AppData component
15+
post-migration. Receive-side monotonicity means a floor can only rise.
16+
17+
Any member processing a commit on a group whose **committed** floor exceeds its
18+
own version pauses the group (`paused_for_version`): the cursor holds, all
19+
subsequent commits are deferred, and after the app upgrades the sweep unpauses
20+
the group and reprocesses everything with fixed code. Pausing never accepts or
21+
rejects anything, so a wrongly-paused client always converges once upgraded.
22+
23+
Writing the floor is super-admin-gated on groups (both members on DMs).
24+
25+
## Scenario: version X validator wrongly REJECTS valid commits
26+
27+
Fielded X clients fork off groups whenever the triggering commit shape appears.
28+
29+
1. Fix the validator in version Y.
30+
2. Bump `PROPOSALS_MIN_PROTOCOL_VERSION` to Y so newly-migrating groups get the
31+
Y floor from bootstrap.
32+
3. For existing migrated groups: have (super-admin) clients on Y bump each
33+
group's floor to Y — e.g. wire `update_group_min_version_to_match_self` into
34+
a recovery flow. X members pause **at the floor-bump commit** (its cursor
35+
holds there), so they never process the bug-triggering commits mid-stream;
36+
after upgrading to Y they reprocess everything with the fixed validator.
37+
4. X members who already rejected a commit before the floor bump landed are
38+
forked; recovery for them is the normal fork-recovery path (re-add /
39+
re-welcome), not this runbook.
40+
41+
## Scenario: version X validator wrongly ACCEPTS bad state
42+
43+
E.g. the pre-#3863 registry-poison bug: state that wedges or corrupts readers.
44+
45+
1. Fix acceptance in Y, plus a read-side tolerance for the already-fielded bad
46+
state where possible (the #3863 pattern: preserved-but-invisible registry
47+
entries — bad state degrades to "that component is unavailable", not "no
48+
commit validates").
49+
2. Floor-bump affected groups to Y as above so X members stop extending the bad
50+
state and pause until they can run the tolerant readers.
51+
52+
## Why the floor is trustworthy
53+
54+
The pause guards read only **committed** floor state (the pre-commit dict or
55+
legacy GMM), never same-commit proposals — a floor value only exists after its
56+
super-admin policy check passed on every honest receiver. See the floor-bump
57+
convention in `xmtp_mls_common::app_data::registry_table` for the send-side
58+
rules protocol evolution must follow.
59+
60+
## Sharp edges
61+
62+
- **Monotonic means permanent.** A floor of `999.0.0` written by a rogue or
63+
buggy super admin pauses the group for everyone forever (send-side clamp
64+
`min_version <= own pkg_version` makes accidents unlikely; a malicious super
65+
admin can equally destroy a group in more direct ways).
66+
- **The registry must stay loadable** for the post-migration floor to be
67+
readable — the entry-validation + tolerant-loader pair (#3863) is what keeps
68+
this lever alive on groups carrying unexpected registry state.
69+
- **Same-device downgrades** below a group's committed floor pause at the next
70+
commit (pause-before-parse guards, #3864); unknown-`IntentKind` rows written
71+
by the newer build are excluded from outbound queries rather than wedging
72+
them. Both recover on re-upgrade.

0 commit comments

Comments
 (0)