Revert "fix(governance): replace unvalidated u8 discriminants with typed enums and bounds checks"#3032
Conversation
…ped enum…" This reverts commit 504cdb2.
Bugbot is paused — on-demand spend limit reachedBugbot 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. |
chefsale
deleted the
revert-2965-claude/u8-discriminants-validation-h83wtt
branch
|
Your PR title does not adhere to the Conventional Commits convention: Common errors to avoid:
|
![meroreviewer[bot]](https://avatars.githubusercontent.com/in/2793509?s=60&v=4){kind=small}
There was a problem hiding this comment.
🤖 MeroReviewer
Reviewed by 1 agents | Quality score: 24% | Review time: 58.1s
🟡 2 warnings. See inline comments.
🤖 Generated by MeroReviewer | Review ID: review-f86e0211
| let signer = ctx.signer(); | ||
| ctx.settings().set_subgroup_visibility(signer, *mode)?; | ||
|
|
||
| let visibility = match *mode { |
There was a problem hiding this comment.
🟡 Unvalidated u8 discriminant for VisibilityMode uses catch-all default
The match 0 => Open, _ => Restricted means any non-zero byte received over the wire (e.g., 2, 255) is silently treated as Restricted. This is a silent data corruption risk: a malformed or future-extended op will be misinterpreted rather than rejected. The original typed enum with Borsh deserialization would have rejected unknown discriminants at the boundary. The same pattern appears in store_subgroup_visibility.rs.
Suggested fix:
Either restore the typed VisibilityMode enum with BorshDeserialize (which rejects unknown discriminants), or add an explicit error branch: `0 => Open, 1 => Restricted, other => return Err(eyre::eyre!("unknown visibility discriminant: {}", other))`.
| ) -> Self::Result { | ||
| let result = | ||
| CapabilitiesRepository::new(&self.datastore).set_subgroup_visibility(&group_id, mode); | ||
| let visibility = match mode { |
There was a problem hiding this comment.
🟡 Unvalidated u8 discriminant for VisibilityMode with silent fallback
The match 0 => Open, _ => Restricted silently maps any unrecognized byte to Restricted. This is the same issue as in subgroup_visibility_set.rs — a future discriminant value or a corrupted byte will be silently misinterpreted rather than rejected with an error.
Suggested fix:
Add an explicit error arm for unrecognized discriminants rather than using a catch-all `_` pattern.
Documentation ReviewThe following documentation may need updates based on the changes in this PR:
|
There was a problem hiding this comment.
🤖 AI Code Reviewer
Reviewed by 2 agents | Quality score: 34% | Review time: 174.4s
🟡 2 warnings. See inline comments.
🤖 Generated by AI Code Reviewer | Review ID: review-ae193f40
| .context_member_capability(group_id, context_id, member)? | ||
| .unwrap_or(0); | ||
| caps.set_context_member(group_id, context_id, member, current | capability.get())?; | ||
| caps.set_context_member(group_id, context_id, member, current | capability)?; |
There was a problem hiding this comment.
🟡 Zero capability bitmask accepted without validation
With ContextCapabilityBits removed, the capability parameter is now a plain &u8. Nothing prevents a ContextCapabilityGranted op from being constructed or deserialized with capability == 0. Granting zero bits is a no-op that silently succeeds, but it also means a malformed or malicious op can be replayed without error. The original ContextCapabilityBits type rejected zero at Borsh deserialization time, providing a wire-level guard. Without it, a zero-capability grant op will be applied and stored, potentially masking bugs or enabling replay of semantically invalid ops.
Suggested fix:
Add an explicit guard at the top of the `apply` function: `if *capability == 0 { return Err(eyre::eyre!("capability bitmask must not be zero")); }`. This restores the invariant that was previously enforced by the `ContextCapabilityBits` newtype.
| let signer = ctx.signer(); | ||
| ctx.settings().set_subgroup_visibility(signer, *mode)?; | ||
|
|
||
| let visibility = match *mode { |
There was a problem hiding this comment.
🟡 Unvalidated u8 discriminant silently maps unknown values to Restricted
The match *mode { 0 => Open, _ => Restricted } pattern accepts any byte value other than 0 as Restricted. A malformed or future-extended op with an unknown discriminant (e.g. 2, 255) will be silently coerced to Restricted rather than being rejected. The original typed-enum approach caught this at Borsh deserialization time. The same silent-coercion pattern also appears in store_subgroup_visibility.rs.
Suggested fix:
Add an explicit arm for the only two valid values and return an error for anything else: `match *mode { 0 => Ok(VisibilityMode::Open), 1 => Ok(VisibilityMode::Restricted), other => eyre::bail!("unknown visibility discriminant: {other}") }`.
1 participant
Reverts #2965