xmtp_mls: EXTERNAL_COMMIT_POLICY component + enable/revoke API with XIP-82 lifecycle invariants#3666
Conversation
|
Claude finished @tylerhawkes's task —— View job Code Review CompleteReview Date: 2026-06-09 23:54 UTC This is a well-architected implementation of XIP-82's external commit policy with strong invariant enforcement. The code demonstrates careful attention to security and correctness. Below are observations beyond what Macroscope has already identified: Security & Correctness1. Missing DM conversation check
2. Potential error swallowing in registry lookup (external_commit_policy.rs:117)
3. Post-state validation correctly handles atomic commits ✅
Code Quality4. Strong cryptographic hygiene ✅
5. Test gap: validator behavior not actually tested (already flagged by Macroscope)
6. Potential panic in unreachable branch (sender_intents.rs:88)
7. Excellent documentation ✅
Performance8. Efficient in-batch accumulation ✅
Test CoverageAccording to Codecov: 45.6% patch coverage with 81 missing lines. Key gaps:
The PR description mentions receive-side coverage will come with the L-7 validator PR, which is reasonable for a stacked PR approach. RecommendationsMust fix before merge:
Should fix: Nice to have: |
![macroscopeapp[bot]](https://avatars.githubusercontent.com/in/900172?s=60&v=4){kind=small}
| /// group to be migrated to AppData. The component's | ||
| /// `permissions.update_policy` (super-admin-only by default) gates | ||
| /// who can flip the bits. | ||
| pub async fn set_external_commit_policy( |
There was a problem hiding this comment.
🔴 Critical groups/mod.rs:2015
set_external_commit_policy allows external commit policies to be set on DM conversations, bypassing the restriction that other metadata update methods enforce. This could enable third parties to join a two-person DM via external commits. Consider adding the DM check that other methods use: if self.metadata().await?.conversation_type == ConversationType::Dm { return Err(MetadataPermissionsError::DmGroupMetadataForbidden.into()); }
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/xmtp_mls/src/groups/mod.rs around line 2015:
`set_external_commit_policy` allows external commit policies to be set on DM conversations, bypassing the restriction that other metadata update methods enforce. This could enable third parties to join a two-person DM via external commits. Consider adding the DM check that other methods use: `if self.metadata().await?.conversation_type == ConversationType::Dm { return Err(MetadataPermissionsError::DmGroupMetadataForbidden.into()); }`
Evidence trail:
crates/xmtp_mls/src/groups/mod.rs lines 2015-2046 (set_external_commit_policy missing DM check), lines 1987-2004 (update_commit_log_signer with DM check at 1993-1995), lines 2051-2060 (set_allow_external_commit wrapper). git_grep for DmGroupMetadataForbidden shows the check at lines 1901, 1927, 1994, 2086, 2144, 2184, 2482 — all other metadata update paths.
ApprovabilityVerdict: Needs human review 6 blocking correctness issues found. Diff is too large for automated approval analysis. A human reviewer should evaluate this PR. You can customize Macroscope's approvability policy. Learn more. |
tylerhawkes
force-pushed
the
tyler/qr-invite-l6-external-commit-policy
branch
from
60eeee5 to
9d4e150
Compare
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## tyler/qr-invite-l5-invite-encryption #3666 +/- ##
========================================================================
- Coverage 84.54% 84.25% -0.29%
========================================================================
Files 409 410 +1
Lines 60248 60747 +499
========================================================================
+ Hits 50938 51184 +246
- Misses 9310 9563 +253 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
tylerhawkes
force-pushed
the
tyler/qr-invite-l6-external-commit-policy
branch
3 times, most recently
from
c6765a6 to
2cdc15e
Compare
tylerhawkes
changed the base branch from
main
to
tyler/qr-invite-l5-invite-encryption
tylerhawkes
force-pushed
the
tyler/qr-invite-l6-external-commit-policy
branch
from
2cdc15e to
67cd34e
Compare
| component_id: ComponentId, | ||
| ) -> Result<Option<ComponentPermissions>, ComponentSourceError> { | ||
| let registry = load_component_registry(mls_group)?; | ||
| let Some(meta) = registry.get(&component_id).ok().flatten() else { |
There was a problem hiding this comment.
🟡 Medium groups/external_commit_policy.rs:117
registry.get(&component_id).ok().flatten() silently discards decode errors by converting them to None, causing the function to return Ok(None) instead of Err(_) when component metadata fails to decode. This violates the documented contract on lines 104-108 where decode failures should surface as errors. Consider using ? to propagate the error instead of ok().flatten().
- let Some(meta) = registry.get(&component_id).ok().flatten() else {
+ let Some(meta) = registry.get(&component_id)? else {🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file crates/xmtp_mls/src/groups/external_commit_policy.rs around line 117:
`registry.get(&component_id).ok().flatten()` silently discards decode errors by converting them to `None`, causing the function to return `Ok(None)` instead of `Err(_)` when component metadata fails to decode. This violates the documented contract on lines 104-108 where decode failures should surface as errors. Consider using `?` to propagate the error instead of `ok().flatten()`.
Evidence trail:
crates/xmtp_mls/src/groups/external_commit_policy.rs lines 96-121 (REVIEWED_COMMIT) — documented contract at line 108 says `Err(_)` for registry decode failure, but line 117 uses `.ok().flatten()` which converts errors to `None`.
crates/xmtp_mls_common/src/app_data/component_registry.rs lines 103-119 (REVIEWED_COMMIT) — `ComponentRegistry::get()` returns `Result<Option<ComponentMetadata>, ComponentRegistryError>`, returning `Err(DecodeError{..})` on protobuf decode failure.
No `From<ComponentRegistryError> for ComponentSourceError` impl exists (verified via git_grep).
03ad9fe to
54721d0
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l5-invite-encryption
branch
3 times, most recently
from
004ddc2 to
d58d4c3
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l6-external-commit-policy
branch
from
67cd34e to
ba504d7
Compare
tylerhawkes
changed the title
tylerhawkes
force-pushed
the
tyler/qr-invite-l6-external-commit-policy
branch
from
ba504d7 to
ccb136f
Compare
| openmls_group, | ||
| )?; | ||
|
|
||
| if let openmls::messages::proposals::AppDataUpdateOperation::Update(payload) = |
There was a problem hiding this comment.
🟢 Low groups/validated_commit.rs:1619
policy_write and registry_write are only updated on AppDataUpdateOperation::Update operations, so a later Remove for the same component leaves the stale Update bytes in the tracker. validate_external_commit_policy_post_state then validates against the wrong (non-final) state, rejecting valid commits where Remove should win. Consider clearing the tracker on Remove so the last operation's effect is always reflected.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/xmtp_mls/src/groups/validated_commit.rs around line 1619:
`policy_write` and `registry_write` are only updated on `AppDataUpdateOperation::Update` operations, so a later `Remove` for the same component leaves the stale `Update` bytes in the tracker. `validate_external_commit_policy_post_state` then validates against the wrong (non-final) state, rejecting valid commits where `Remove` should win. Consider clearing the tracker on `Remove` so the last operation's effect is always reflected.
| // variants fail closed at decode time. | ||
|
|
||
| impl From<AppDataUpdateIntentData> for Vec<u8> { | ||
| fn from(intent: AppDataUpdateIntentData) -> Self { |
There was a problem hiding this comment.
🟡 Medium groups/intents.rs:1079
Vec::<u8>::from(AppDataUpdateIntentData) writes additional_updates into the intent blob, but older binaries decoding that blob silently drop unknown protobuf fields. If a queued enable_external_commits intent is processed by an older version, the coupled COMPONENT_REGISTRY/GROUP_MEMBERSHIP grant update is lost, breaking the same-commit atomicity that XIP-82 guarantees. Consider using a new intent type or a wire-format change that older versions reject rather than silently truncate.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/xmtp_mls/src/groups/intents.rs around line 1079:
`Vec::<u8>::from(AppDataUpdateIntentData)` writes `additional_updates` into the intent blob, but older binaries decoding that blob silently drop unknown protobuf fields. If a queued `enable_external_commits` intent is processed by an older version, the coupled `COMPONENT_REGISTRY`/`GROUP_MEMBERSHIP` grant update is lost, breaking the same-commit atomicity that XIP-82 guarantees. Consider using a new intent type or a wire-format change that older versions reject rather than silently truncate.
| let current = self | ||
| .load_mls_group_with_lock(self.context.mls_storage(), |mls_group| { | ||
| external_commit_policy::load_external_commit_policy(&mls_group) | ||
| .map_err(GroupError::from) | ||
| })? | ||
| .unwrap_or_default(); |
There was a problem hiding this comment.
🟡 Medium groups/mod.rs:2274
revoke_external_commits calls load_external_commit_policy(...).unwrap_or_default() and copies expire_in_ns and max_uses from the result. When an older client loads a policy written by a newer client with an unknown ExternalCommitPolicyEntry variant, load_external_commit_policy returns Ok(None), causing unwrap_or_default() to return a zeroed/default struct. The durable settings are then silently overwritten with zeros instead of being preserved, losing the original policy configuration across mixed-version clients.
- let current = self
- .load_mls_group_with_lock(self.context.mls_storage(), |mls_group| {
- external_commit_policy::load_external_commit_policy(&mls_group)
- .map_err(GroupError::from)
- })?
- .unwrap_or_default();Also found in 1 other location(s)
crates/xmtp_mls/src/groups/external_commit_policy.rs:331
external_committer_permissions_forsuppresses decode failures fromComponentRegistry::getby calling.ok().flatten()on the result. If the registry contains a corruptComponentMetadataentry for a component, this helper returnsOk(None)and the caller treats it as an ordinary "all-Deny" absence instead of surfacing the malformed registry state. That silently changes behavior and hides data corruption from the validator.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/xmtp_mls/src/groups/mod.rs around lines 2274-2279:
`revoke_external_commits` calls `load_external_commit_policy(...).unwrap_or_default()` and copies `expire_in_ns` and `max_uses` from the result. When an older client loads a policy written by a newer client with an unknown `ExternalCommitPolicyEntry` variant, `load_external_commit_policy` returns `Ok(None)`, causing `unwrap_or_default()` to return a zeroed/default struct. The durable settings are then silently overwritten with zeros instead of being preserved, losing the original policy configuration across mixed-version clients.
Also found in 1 other location(s):
- crates/xmtp_mls/src/groups/external_commit_policy.rs:331 -- `external_committer_permissions_for` suppresses decode failures from `ComponentRegistry::get` by calling `.ok().flatten()` on the result. If the registry contains a corrupt `ComponentMetadata` entry for a component, this helper returns `Ok(None)` and the caller treats it as an ordinary "all-Deny" absence instead of surfacing the malformed registry state. That silently changes behavior and hides data corruption from the validator.
| /// Pure function of the value; the cross-component grant check is | ||
| /// separate (see [`grant_admits_joiner_insert`]) because it needs the | ||
| /// post-state registry. | ||
| pub(crate) fn validate_policy_v1( |
There was a problem hiding this comment.
🟡 Medium groups/external_commit_policy.rs:136
validate_policy_v1 does not check grant_admits_joiner_insert even when allow_external_commit is true, so MissingMembershipGrant is never returned by this function. Callers can queue an enabled policy that will later fail validation during commit because the group lacks the GROUP_MEMBERSHIP external-committer insert grant. Consider adding the grant check to the validation function, or ensuring all callers invoke it separately before accepting the policy.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/xmtp_mls/src/groups/external_commit_policy.rs around line 136:
`validate_policy_v1` does not check `grant_admits_joiner_insert` even when `allow_external_commit` is true, so `MissingMembershipGrant` is never returned by this function. Callers can queue an enabled policy that will later fail validation during commit because the group lacks the `GROUP_MEMBERSHIP` external-committer insert grant. Consider adding the grant check to the validation function, or ensuring all callers invoke it separately before accepting the policy.
| /// definition neither admin nor super-admin at validation time, so any | ||
| /// stricter base policy denies it; the validator's atomic-shape checks | ||
| /// — own entry only — bound what "Allow" can do.) | ||
| pub(crate) fn grant_admits_joiner_insert(perms: Option<&ComponentPermissions>) -> bool { |
There was a problem hiding this comment.
🟢 Low groups/external_commit_policy.rs:193
grant_admits_joiner_insert returns false for MetadataPolicy values like AnyCondition([Base(Allow), Base(AllowIfAdmin)]) even though this policy semantically allows the external committer to insert. Because the validator calls this helper before accepting an enabled policy, valid configurations will have external commits incorrectly denied.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/xmtp_mls/src/groups/external_commit_policy.rs around line 193:
`grant_admits_joiner_insert` returns `false` for `MetadataPolicy` values like `AnyCondition([Base(Allow), Base(AllowIfAdmin)])` even though this policy semantically allows the external committer to insert. Because the validator calls this helper before accepting an enabled policy, valid configurations will have external commits incorrectly denied.
| /// leaves every per-invite field absent). Not retryable — the caller | ||
| /// supplied a malformed policy. | ||
| #[error("external commit policy error: {0}")] | ||
| ExternalCommitPolicy(#[from] super::external_commit_policy::ExternalCommitPolicyError), |
There was a problem hiding this comment.
🟢 Low groups/error.rs:289
GroupError::ExternalCommitPolicy wraps ExternalCommitPolicyError without #[error_code(inherit)], and ExternalCommitPolicyError doesn't implement ErrorCode. When the Node/WASM/mobile bindings call error_code(), every variant of ExternalCommitPolicyError is flattened to the single code GroupError::ExternalCommitPolicy. SDK consumers cannot distinguish MissingSymmetricKey, InvalidExternalGroupIdLength, MissingMembershipGrant, etc. without parsing the free-form message string. Consider adding #[error_code(inherit)] to the variant (if the inner type gains ErrorCode) or implementing ErrorCode directly on ExternalCommitPolicyError.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/xmtp_mls/src/groups/error.rs around line 289:
`GroupError::ExternalCommitPolicy` wraps `ExternalCommitPolicyError` without `#[error_code(inherit)]`, and `ExternalCommitPolicyError` doesn't implement `ErrorCode`. When the Node/WASM/mobile bindings call `error_code()`, every variant of `ExternalCommitPolicyError` is flattened to the single code `GroupError::ExternalCommitPolicy`. SDK consumers cannot distinguish `MissingSymmetricKey`, `InvalidExternalGroupIdLength`, `MissingMembershipGrant`, etc. without parsing the free-form message string. Consider adding `#[error_code(inherit)]` to the variant (if the inner type gains `ErrorCode`) or implementing `ErrorCode` directly on `ExternalCommitPolicyError`.
…ves) + adapt invite::payload Regenerates xmtp_proto from xmtp/proto#336 (typed SymmetricKey / GroupStateHash / ServicePointer newtypes, ExternalCommitPolicyV1 max_uses + refresh_pointers, EncryptedGroupInfoBlobV1 AAD + effective-expiry semantics, GroupMembershipEntry.V1 admitted_via_external_group_id). Consuming changes kept to what main already contains: - invite::payload rewritten for the typed shape: symmetric_key is a SymmetricKey submessage (MissingSymmetricKey + material-length checks), service_pointer is an optional ServicePointer (absent = application-resolved; present-but-empty oneof fails closed; https_url parsed and scheme-checked via url). New https_service_pointer / opaque_service_pointer / validate_service_pointer helpers. - GroupMembershipEntry.V1 construction sites gain the new admitted_via_external_group_id field, empty everywhere: the migrator's synthesized entries are all Welcome/legacy members (absent is permanently correct), and the membership-update rewrite path documents that tag preservation MUST land together with the validator's write-once enforcement in the external-commit stack — nothing on main can set the tag yet, so empty is correct today. The rest of the QR-invite stack (#3671 -> #3666 -> #3667 -> #3668 -> #3673 -> #3674) rebases onto this and picks up the new fields layer by layer.
…ves) + adapt invite::payload Regenerates xmtp_proto from xmtp/proto#336 (typed SymmetricKey / GroupStateHash / ServicePointer newtypes, ExternalCommitPolicyV1 max_uses + refresh_pointers, EncryptedGroupInfoBlobV1 AAD + effective-expiry semantics, GroupMembershipEntry.V1 admitted_via_external_group_id). Consuming changes kept to what main already contains: - invite::payload rewritten for the typed shape: symmetric_key is a SymmetricKey submessage (MissingSymmetricKey + material-length checks), service_pointer is an optional ServicePointer (absent = application-resolved; present-but-empty oneof fails closed; https_url parsed and scheme-checked via url). New https_service_pointer / opaque_service_pointer / validate_service_pointer helpers. - GroupMembershipEntry.V1 construction sites gain the new admitted_via_external_group_id field, empty everywhere: the migrator's synthesized entries are all Welcome/legacy members (absent is permanently correct), and the membership-update rewrite path documents that tag preservation MUST land together with the validator's write-once enforcement in the external-commit stack — nothing on main can set the tag yet, so empty is correct today. The rest of the QR-invite stack (#3671 -> #3666 -> #3667 -> #3668 -> #3673 -> #3674) rebases onto this and picks up the new fields layer by layer.
…th XIP-82 lifecycle invariants enable_external_commits mints fresh CSPRNG coordinates and applies the caller settings in a single-component AppDataUpdate commit — under the revised XIP-82, allow_external_commit itself authorizes the joiner self-entry insert, so no companion registry grant exists. revoke_external_commits preserves durable settings (expire_in_ns, max_uses) and leaves every per-invite field absent. Receive side: validate_external_commit_policy_post_state enforces the field-coupling shape (validate_policy_v1) on every commit carrying a policy write.
tylerhawkes
force-pushed
the
tyler/qr-invite-l6-external-commit-policy
branch
from
ccb136f to
5b4bf1f
Compare
tylerhawkes
force-pushed
the
tyler/qr-invite-l5-invite-encryption
branch
from
d58d4c3 to
ea93854
Compare
| @@ -2136,6 +2138,144 @@ where | |||
| Ok(()) | |||
There was a problem hiding this comment.
🟠 High groups/mod.rs:2138
queue_external_commit_policy writes the EXTERNAL_COMMIT_POLICY component via AppDataUpdate, but validate_one_app_data_update requires every component to have a COMPONENT_REGISTRY entry seeded during migration. The migration code never creates this registry entry for EXTERNAL_COMMIT_POLICY, so the first call to set_external_commit_policy, enable_external_commits, or revoke_external_commits on a migrated group fails with a permissions error. Consider seeding the EXTERNAL_COMMIT_POLICY registry entry during the bootstrap migration in enable_proposals_inner.
🚀 Reply "fix it for me" or copy this AI Prompt for your agent:
In file @crates/xmtp_mls/src/groups/mod.rs around line 2138:
`queue_external_commit_policy` writes the `EXTERNAL_COMMIT_POLICY` component via `AppDataUpdate`, but `validate_one_app_data_update` requires every component to have a `COMPONENT_REGISTRY` entry seeded during migration. The migration code never creates this registry entry for `EXTERNAL_COMMIT_POLICY`, so the first call to `set_external_commit_policy`, `enable_external_commits`, or `revoke_external_commits` on a migrated group fails with a permissions error. Consider seeding the `EXTERNAL_COMMIT_POLICY` registry entry during the bootstrap migration in `enable_proposals_inner`.
1 participant
Summary
The
EXTERNAL_COMMIT_POLICYwell-known component (0x800C) and its full lifecycle, per the revised XIP-82:SymmetricKeysubmessage (absence is the only cleared encoding; an empty submessage is the forbidden second representable state), plus the newmax_uses(concurrent per-invite cap) andrefresh_pointers(member-driven blob-refresh locations).enable_external_commits(settings) → ExternalInviteCoordinates— mints a fresh CSPRNG key + slot id (uniform 256-bit randomness replaces key-history tracking for no-key-revival), applies the caller's settings, and establishes theGROUP_MEMBERSHIPexternal-committer insert grant in the same commit when absent. Without the grant the switch would be on but every join would dead-end at validation (the atomic external-commit shape requires the joiner's membership-entry write).revoke_external_commits()— leaves every per-invite field absent (key, slot id, campaign expiry, refresh pointers) while preserving the durable settings (expire_in_ns,max_uses). A revoked policy serializes byte-identical to one that never had an invite.AppDataUpdateIntentData.additional_updates+stage_app_data_propose_many_and_commit: N standaloneAppDataUpdateproposals swept into one commit (the existing lazy-batching design makes this a small generalization). This is what makes enable atomicity real on the send side.validate_external_commit_policy_post_stateruns on every commit carrying a policy write: field-coupling invariants (validate_policy_v1, shared with the setter) plus the grant coupling evaluated against the post-state registry (aCOMPONENT_REGISTRYwrite in the same commit counts). A proposal that plants a key while the switch is off, or enables without the grant, is rejected by every member identically — both checks are pure functions of the commit's own payloads + the shared pre-commit registry, so the verdict converges.Consumed by the L-7 validator (#3667) for the external-commit-time checks (master switch, envelope-timestamp time windows,
max_usescounting).Stack
Base:
tyler/qr-invite-l5-invite-encryption(#3671) ←tyler/qr-invite-l4b-proto-rebump(#3751) ← main.Test plan
additional_updatesoverflow handling.cargo check --locked,just lint-rustclean.ValidatedCommit.🤖 Generated with Claude Code
Note
Add
EXTERNAL_COMMIT_POLICYcomponent with enable/revoke API and XIP-82 lifecycle validationComponentId::EXTERNAL_COMMIT_POLICY(0x800C) and theExternalCommitPolicyV1proto type to store per-group external commit settings in app data.MlsGroup::enable_external_commits(high-level) andset_external_commit_policy(low-level) methods that mint a fresh symmetric key andexternal_group_id, persist the policy via anAppDataUpdateintent, and returnExternalInviteCoordinatesfor QR payloads.validate_app_data_update_proposals_in_commit.wrap_payload_symmetric/unwrap_payload_symmetricto bon builder style and adds an optionalaadparameter so AEAD encryption can bind cleartext envelope fields.encrypted_group_infohelpers (AAD construction, error types) andServicePointerbuilders (https_service_pointer,opaque_service_pointer) to invite/payload.rs.EXTERNAL_COMMIT_POLICYvalue now fail validation and are non-retryable; existing callers ofwrap_payload_symmetricmust migrate to the builder API.Macroscope summarized 5b4bf1f.