fix(governance-store): reject expired invitations when joined_at is absent

[chefsale]

Description

This PR fixes a logic bug in the namespace membership invitation expiration validation. Previously, the code would only check expiration if joined_at was present, allowing invitations with an expiration timestamp but no joined_at to pass validation incorrectly.

The fix inverts the logic: when an expiration timestamp is set, we now require joined_at to be present and reject the invitation with a clear error message if it's absent. This ensures that expired invitations are always properly rejected, regardless of which field is missing.

Changes:

• Restructured the expiration check to validate the presence of joined_at when expiration != 0
• Added explicit error message when joined_at is absent but expiration is set
• Maintains the existing expiration comparison check when both values are present

Test plan

Existing unit tests in the governance-store crate cover this validation logic and will verify the fix. The change is defensive and ensures stricter validation of invitation state.

Wire contract (SDK gate)

• No wire contract changes

Documentation update

N/A

https://claude.ai/code/session_013LK9DQb5jsjk8a2XryEy2S

[meroreviewer]

🤖 MeroReviewer

Reviewed by 1 agents | Quality score: 31% | Review time: 28.7s

🟡 1 warnings. See inline comments.

---

_{🤖 Generated by MeroReviewer | Review ID: review-4bec59a8}

[meroreviewer]

🟡 Absent joined_at with expiration treated as expired rather than invalid

The new branch bails with 'invitation expired' when joined_at is absent but expiration != 0. Semantically, a missing joined_at is not the same as an expired invitation — it means the op is malformed or incomplete. Using the 'expired' error message for this case conflates two distinct failure modes, which can mislead callers and make debugging harder. The fix is correct in rejecting the op, but the error message should distinguish 'missing joined_at' from 'expired'.

Suggested fix:

Change the bail message to: `bail!("invalid MemberJoined op: expiration {expiration} is set but joined_at is absent")` to clearly distinguish a malformed op from a genuinely expired one.

[chefsale]

Fixed — the bail message now reads "invalid MemberJoined op: expiration {expiration} is set but joined_at is absent", clearly distinguishing a malformed op from a genuinely expired one.

---

Generated by Claude Code

[meroreviewer]

Documentation Review

The following documentation may need updates based on the changes in this PR:

• 🟡 architecture/auto-follow.html: Files matching crates/governance-store/** were changed but architecture/auto-follow.html was not updated (per source_to_docs_mapping).
• 🟡 architecture/crates/context.html: Files matching crates/context/** were changed but architecture/crates/context.html was not updated (per source_to_docs_mapping).

[cursor]

Bugbot is paused — on-demand spend limit reached

Bugbot uses usage-based billing for this team and has hit its on-demand spend limit.

A team admin can raise the spend limit in the Cursor dashboard, or wait for the next billing cycle to continue.

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 1 agents | Quality score: 31% | Review time: 84.3s

🟡 1 warnings. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-a75b3b79}

[meroreviewer]

🟡 Invitations with expiration=0 and absent joined_at are silently accepted

The new guard only fires when expiration != 0. When expiration == 0 (no expiry set) and joined_at is None, the code falls through without any check. This is the intended behaviour for non-expiring invitations, but it is worth confirming that expiration == 0 is the canonical sentinel for "no expiry" and that a caller cannot craft an invitation with expiration == 0 to bypass a real expiry. If 0 is indeed the sentinel (as the existing validate_open_invitation at line ~116 also treats it), this is fine — but the asymmetry between the two expiry-check sites (one uses now_secs > expiration, the other uses joined_at > expiration) means a joined_at == expiration case is accepted here but would be rejected if the wall-clock happened to equal expiration in validate_open_invitation. This is a minor semantic inconsistency worth documenting.

Suggested fix:

Add a comment clarifying that `expiration == 0` is the "no expiry" sentinel and that `joined_at == expiration` is intentionally accepted (the join happened exactly at the deadline). Consider using `>=` instead of `>` in the comparison at line 51 to match the wall-clock path's `>` semantics, or document why the two sites differ.

[chefsale]

Addressed — a comment above the if expiration != 0 block now documents that 0 is the canonical no-expiry sentinel (consistent with validate_open_invitation). Regarding the > vs >= asymmetry: both sites use strict >, so joined_at == expiration (join at exactly the deadline) is accepted by both — they are consistent. The deterministic path intentionally uses > so a join timestamped exactly at the expiry boundary is not rejected, matching the wall-clock path's behaviour.

---

Generated by Claude Code

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 1 agents | Quality score: 31% | Review time: 221.4s

🟡 1 warnings. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-8090e66b}

[meroreviewer]

🟡 Invitations with expiration=0 and absent joined_at are silently accepted

The restructured guard only validates joined_at when expiration != 0. When expiration == 0 (no-expiry sentinel) and joined_at is None, the code falls through without any check. This is the intended behaviour per the comment, but it means a MemberJoined op with joined_at = None and expiration = 0 is unconditionally accepted. If the protocol ever requires joined_at to be present for audit/ordering purposes regardless of expiration, this silent acceptance could be a logic gap. The PR description says the fix is correct, but the asymmetry (absent joined_at is an error only when expiration is set) should be explicitly documented in the code to prevent future regressions where someone adds a joined_at-dependent check below this gate and forgets the None case.

Suggested fix:

Add a comment immediately after the `if expiration != 0` block explicitly stating that `joined_at == None` with `expiration == 0` is intentionally accepted (no-expiry, no timestamp required), so future maintainers do not accidentally introduce a `joined_at.unwrap()` below this gate.

[chefsale]

Addressed — a comment immediately after the if expiration != 0 block now explicitly states that joined_at = None with expiration == 0 is intentionally accepted (no expiry, no timestamp required), warning future maintainers not to add joined_at-dependent logic below this gate without handling the None case.

---

Generated by Claude Code

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 1 agents | Quality score: 32% | Review time: 118.7s

💡 1 suggestions. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-b64eb1a9}

[meroreviewer]

💡 Consider whether joined_at = None with expiration != 0 should be Err or silently dropped

The PR changes the behavior from 'silently accept when joined_at is None' to 'bail! with an error when expiration is set but joined_at is absent'. This is the correct fix for the bug described. However, callers that previously passed joined_at = None with a non-zero expiration will now receive an error instead of a silent pass-through. Confirm that all call sites either always supply joined_at when expiration != 0, or are prepared to handle this new error path. If any caller legitimately omits joined_at for a non-expired invitation with a set expiration, this change is a breaking behavioral change for them.

Suggested fix:

Audit all call sites of `apply_member_joined` to confirm `joined_at` is always `Some` when the invitation has a non-zero `expiration_timestamp`.

[chefsale]

Audited — there are exactly two call sites of apply_member_joined (in crates/governance-store/src/ops/namespace.rs): RootOp::MemberJoined always passes joined_at = None, and RootOp::MemberJoinedAt always passes joined_at = Some(...). The legacy MemberJoined variant predates expiry support and carries no timestamp, so any invitation with expiration != 0 being redeemed via MemberJoined is correctly a malformed op. All tests that used MemberJoined with a non-zero expiration have been updated to either use MemberJoinedAt (when expiry matters) or set expiration = 0 (when they are not testing expiry behavior).

---

Generated by Claude Code

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 1 agents | Quality score: 28% | Review time: 96.1s

---

🟡 Warning (1)

1. Backdated joined_at bypasses expiration check

File: crates/governance-store/src/namespace/membership.rs (line 41-61) | Consensus: 1/1 agents ✓

The expiration gate compares joined_at (a self-attested field signed by the joiner, not by the admin) against expiration_timestamp (signed by the admin). A malicious joiner can set joined_at = 0 on a MemberJoinedAt op for an already-expired invitation and the gate will accept it (0 <= expiration). The test member_joined_at_backdated_joined_at_bypasses_apply_gate_documented_residual explicitly pins this as an accepted residual, but the PR description frames the change as a security fix. The fix only closes the joined_at == None case; the backdating vector remains open at the apply layer. This is not introduced by this PR (it pre-existed), but the PR's restructuring makes the residual more prominent and the comment in the test is the only documentation of it. Consider whether the error message in the new bail! path should distinguish 'absent' from 'expired' more clearly for operators, and ensure the residual is tracked as a known limitation in a more durable location than a test comment.

Suggested fix:

No code change required for this PR (the residual is pre-existing and documented), but add a `// KNOWN LIMITATION:` comment directly in the production code at line 41 cross-referencing the test, so future maintainers cannot miss it when modifying this gate.

Found by: security-reviewer

💡 Suggestion (1)

1. joined_at: 1 may be less than expiration only by coincidence in recursive_invite_joins_all_descendant_groups

File: crates/context/tests/local_group_governance_convergence.rs (line 623-643) | Consensus: 1/1 agents ✓

The test uses joined_at: 1 with invitations whose expiration_timestamp is now_secs + 365*24*3600 (a future Unix timestamp in the billions). The comment says '1 is safely before any future expiry', which is correct today, but the value 1 is semantically 'epoch + 1 second' and the test relies on the wall-clock being far in the future relative to it. A more self-documenting choice would be joined_at: 0 (the backdating residual documented elsewhere) or computing joined_at from the same now_secs the invitation uses. This is a test-quality issue, not a correctness bug.

Suggested fix:

Use `joined_at: 0` (which the backdating test already shows is accepted) or derive `joined_at` from the same timestamp source as the invitation to make the intent explicit.

Found by: security-reviewer

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-2a052bba}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 1 agents | Quality score: 28% | Review time: 94.6s

---

🟡 Warning (1)

1. Expiry gate accepts backdated joined_at (self-attested, unverifiable)

File: crates/governance-store/src/namespace/membership.rs (line 55-58) | Consensus: 1/1 agents ✓

The fix correctly rejects joined_at == None when expiration != 0, but the joined_at value itself is self-attested by the joiner and only signature-covered (not corroborated by any external authority). A malicious client can set joined_at = 0 (or any value ≤ expiration) to bypass the expiry check on an already-expired invitation. The test member_joined_at_backdated_joined_at_bypasses_apply_gate_documented_residual explicitly documents and accepts this residual. This is a known design limitation rather than a regression introduced by this PR, but it is worth flagging: the apply gate provides only a weak expiry guarantee. The PR description correctly notes the responder key-delivery gate (validate_open_invitation) is the authoritative backstop, but that gate runs on the responder's wall clock and is point-to-point — a joiner who already obtained the key before expiry can still use it to craft a backdated joined_at and join after expiry on any node that replays the op.

Suggested fix:

Consider adding an admin co-signature over `(invitation_hash, joined_at)` at redemption time to make `joined_at` unforgeable. This is out of scope for this PR but should be tracked as a follow-up security issue.

Found by: security-reviewer

💡 Suggestion (1)

1. Recursive invite uses joined_at: 1 which may be ≤ expiration by coincidence

File: crates/context/tests/local_group_governance_convergence.rs (line 643-645) | Consensus: 1/1 agents ✓

The create_recursive_invitations helper sets expiration = now_secs + 365*24*3600 (roughly one year from now). The test uses joined_at: 1 (epoch + 1 second), which is always less than the expiration, so the gate passes. However, if the system clock is at or before Unix epoch (e.g. in a sandboxed CI environment with a broken clock), now_secs could be 0 and expiration could be 31536000, which is still fine. The value 1 is a magic constant with no comment explaining why it satisfies the constraint. A more self-documenting approach would be to use a value clearly within the window.

Suggested fix:

Add a comment: `// joined_at = 1 is always < expiration (now + 1 year), satisfying the expiry gate` or use a named constant.

Found by: security-reviewer

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-881bb4dc}

[meroreviewer]

🤖 AI Code Reviewer

Reviewed by 2 agents | Quality score: 46% | Review time: 192.5s

🟡 1 warnings. See inline comments.

---

_{🤖 Generated by AI Code Reviewer | Review ID: review-3e28ab18}

[meroreviewer]

🟡 MemberJoined with expiration=0 silently accepts any joined_at value including u64::MAX

When expiration == 0, the entire expiry block is skipped and joined_at is ignored regardless of its value. The test member_joined_at_ignores_zero_expiration explicitly passes joined_at: u64::MAX and expects success. This is intentional per the sentinel design, but the comment on line 68 says 'joined_at is not required and a None value is intentionally accepted' — it does not mention that a present joined_at with an arbitrary value is also silently accepted. A MemberJoinedAt op with expiration=0 and joined_at=u64::MAX is structurally inconsistent (why supply a timestamp if there's no expiry?), yet it passes. This is a latent confusion point: callers of MemberJoinedAt with expiration=0 may not realize joined_at is completely ignored, making the field semantically dead in that variant combination. Consider either documenting this explicitly in the MemberJoinedAt wire type, or asserting joined_at == 0 when expiration == 0 to prevent misleading ops from being accepted.

Suggested fix:

Add a comment or assertion: if `expiration == 0 && joined_at.is_some_and(|t| t != 0)` emit a warning or bail with a clear message that `joined_at` must be 0 when expiration is 0. Alternatively, document in the wire type that `joined_at` is ignored when `expiration == 0`.