Security Fix: Operator precedence bug in UpdateDelegate revoke_plugin_authority#253
Conversation
Due to Rust's operator precedence (&& binds tighter than ||), the condition `A || B && C && D` was evaluated as `A || (B && C && D)` instead of the intended `(A || (B && C)) && D`. This meant the `plugin.manager() == Authority::UpdateAuthority` check only applied to the additional_delegates branch, not the main resolved_authorities branch. As a result, UpdateDelegate could revoke authority on owner-managed plugins (FreezeDelegate, TransferDelegate) that it should not control. Added explicit parentheses to ensure the manager check applies to both branches.
|
@AdeshAtole is attempting to deploy a commit to the Metaplex Foundation Team on Vercel. A member of the Team first needs to authorize it. |
Summary by CodeRabbit
WalkthroughThis change is a security fix that adds explicit parentheses to correct an operator-precedence issue in the authority validation logic, ensuring the UpdateAuthority check applies correctly to both authority branches in the revoke-validation condition. Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes 🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing touches
🧪 Generate unit tests (beta)
Tip Issue Planner is now in beta. Read the docs and try it out! Share your feedback on Discord. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rs (1)
243-268:⚠️ Potential issue | 🟠 MajorNo test coverage for the security fix.
This is a security-critical change to authority validation logic, but no new tests were added to verify:
- That
validate_revoke_plugin_authoritynow rejects revocation of owner-managed plugins (e.g.,FreezeDelegate) when onlyresolved_authoritiesmatches.- That it still approves revocation of update-authority-managed plugins.
- That
additional_delegatescannot revoke authority on theUpdateDelegateplugin itself.Regression tests for security fixes are essential — without them, a future refactor could silently reintroduce the precedence bug.
Would you like me to generate unit tests covering these scenarios, or open an issue to track this?
🤖 Fix all issues with AI agents
In `@programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rs`:
- Line 155: The conditional expression in validate (the long line containing
self.additional_delegates.contains(ctx.authority_info.key) &&
PluginType::from(plugin) != PluginType::UpdateDelegate) is excessively long and
hard to read; reformat it to multiple lines consistent with other validators
(e.g., break after each boolean operator and align predicates), for example
split the OR branches and put the contains(...) and PluginType::from(...)
comparisons on separate lines so the whole condition is under 120 chars while
preserving exact logic and precedence for self.additional_delegates,
ctx.authority_info.key, PluginType::from(plugin), and
PluginType::UpdateDelegate.
- Around line 144-148: Update the security-fix comment to accurately reflect
both changes: note that parentheses were added to correct operator precedence
and that an additional self-revoke guard was introduced by checking
PluginType::from(plugin) != PluginType::UpdateDelegate to prevent UpdateDelegate
from revoking itself for additional_delegates; reference the condition change
and the PluginType::UpdateDelegate guard so readers know this is more than a
parenthesization fix in the UpdateDelegate logic.
- Around line 149-157: Add an inline comment in the authorize validation logic
(near validate_revoke_plugin_authority and the existing "SECURITY FIX" block)
explaining the intentional asymmetry: clarify that PluginType::UpdateDelegate is
globally disallowed from being approved (see validate_approve_plugin_authority
and the PluginType::UpdateDelegate guard), but the owner (resolved_authorities /
ctx.self_authority) is still allowed to revoke UpdateDelegate permissions (the
branch checking resolved_authorities and plugin.manager() ==
Authority::UpdateAuthority) to ensure the owner retains ultimate control;
mention that additional_delegates (self.additional_delegates) are intentionally
prevented from approving or revoking UpdateDelegate and reference
plugin.manager() == Authority::UpdateAuthority and the additional_delegates
check to make the rationale clear for future maintainers.
| // SECURITY FIX: Added explicit parentheses to fix operator precedence. | ||
| // Previously, `&& plugin.manager() == Authority::UpdateAuthority` only bound | ||
| // to the additional_delegates branch due to && having higher precedence than ||. | ||
| // This allowed UpdateDelegate to revoke authority on owner-managed plugins | ||
| // (FreezeDelegate, TransferDelegate) which it should not control. |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Security fix comment is slightly misleading about the scope of changes.
The comment states the fix is about operator precedence only, but a new condition (PluginType::from(plugin) != PluginType::UpdateDelegate) was also introduced on line 155 — this is not merely a parenthesization change. The comment should mention that the UpdateDelegate self-revoke guard was also added for additional delegates.
Suggested comment update
- // SECURITY FIX: Added explicit parentheses to fix operator precedence.
- // Previously, `&& plugin.manager() == Authority::UpdateAuthority` only bound
- // to the additional_delegates branch due to && having higher precedence than ||.
- // This allowed UpdateDelegate to revoke authority on owner-managed plugins
- // (FreezeDelegate, TransferDelegate) which it should not control.
+ // SECURITY FIX: (1) Added explicit parentheses to fix operator precedence so
+ // `plugin.manager() == Authority::UpdateAuthority` applies to BOTH the
+ // resolved_authorities and additional_delegates branches. Previously, && binding
+ // tighter than || meant the manager check only applied to the additional_delegates
+ // branch, allowing revocation of owner-managed plugins (FreezeDelegate,
+ // TransferDelegate). (2) Added a guard preventing additional delegates from
+ // revoking authority on the UpdateDelegate plugin itself.📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| // SECURITY FIX: Added explicit parentheses to fix operator precedence. | |
| // Previously, `&& plugin.manager() == Authority::UpdateAuthority` only bound | |
| // to the additional_delegates branch due to && having higher precedence than ||. | |
| // This allowed UpdateDelegate to revoke authority on owner-managed plugins | |
| // (FreezeDelegate, TransferDelegate) which it should not control. | |
| // SECURITY FIX: (1) Added explicit parentheses to fix operator precedence so | |
| // `plugin.manager() == Authority::UpdateAuthority` applies to BOTH the | |
| // resolved_authorities and additional_delegates branches. Previously, && binding | |
| // tighter than || meant the manager check only applied to the additional_delegates | |
| // branch, allowing revocation of owner-managed plugins (FreezeDelegate, | |
| // TransferDelegate). (2) Added a guard preventing additional delegates from | |
| // revoking authority on the UpdateDelegate plugin itself. |
🤖 Prompt for AI Agents
In `@programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rs`
around lines 144 - 148, Update the security-fix comment to accurately reflect
both changes: note that parentheses were added to correct operator precedence
and that an additional self-revoke guard was introduced by checking
PluginType::from(plugin) != PluginType::UpdateDelegate to prevent UpdateDelegate
from revoking itself for additional_delegates; reference the condition change
and the PluginType::UpdateDelegate guard so readers know this is more than a
parenthesization fix in the UpdateDelegate logic.
| if ((ctx.resolved_authorities.is_some() | ||
| && ctx | ||
| .resolved_authorities | ||
| .unwrap() | ||
| .contains(ctx.self_authority)) | ||
| // Or the authority is one of the additional delegates. | ||
| || (self.additional_delegates.contains(ctx.authority_info.key) && PluginType::from(plugin) != PluginType::UpdateDelegate) | ||
| // And it's an authority-managed plugin. | ||
| || (self.additional_delegates.contains(ctx.authority_info.key) && PluginType::from(plugin) != PluginType::UpdateDelegate)) | ||
| // And it's an authority-managed plugin (applies to BOTH branches). | ||
| && plugin.manager() == Authority::UpdateAuthority |
There was a problem hiding this comment.
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
# Compare the boolean structure of validate_approve_plugin_authority vs validate_revoke_plugin_authority
echo "=== validate_approve_plugin_authority ==="
rg -n -A 25 'fn validate_approve_plugin_authority' programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rs
echo ""
echo "=== validate_revoke_plugin_authority ==="
rg -n -A 25 'fn validate_revoke_plugin_authority' programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rsRepository: metaplex-foundation/mpl-core
Length of output: 2690
The asymmetry between validate_approve_plugin_authority and validate_revoke_plugin_authority is real, but the design intent is underdocumented.
The code confirms the asymmetry you identified: in validate_revoke_plugin_authority, the PluginType::UpdateDelegate guard is nested inside the additional_delegates branch only, allowing resolved_authorities (the owner) to revoke authority on UpdateDelegate while blocking additional_delegates from doing so. By contrast, validate_approve_plugin_authority prevents both from approving authority on UpdateDelegate.
The "SECURITY FIX" comment (lines 144–148) explains why parentheses were added—to ensure that the plugin.manager() == Authority::UpdateAuthority check applies to both branches and prevent UpdateDelegate from revoking authority on owner-managed plugins. However, this comment does not explicitly explain why the revoke logic allows the owner to revoke UpdateDelegate authority while the approve logic blocks it entirely.
The design appears intentional: the owner should retain ultimate authority to revoke UpdateDelegate permissions, while delegates should be restricted. However, add an inline comment explaining this design choice—specifically why revoke permits the owner to revoke UpdateDelegate authority when approve does not permit approval of it at all—to prevent future confusion.
🤖 Prompt for AI Agents
In `@programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rs`
around lines 149 - 157, Add an inline comment in the authorize validation logic
(near validate_revoke_plugin_authority and the existing "SECURITY FIX" block)
explaining the intentional asymmetry: clarify that PluginType::UpdateDelegate is
globally disallowed from being approved (see validate_approve_plugin_authority
and the PluginType::UpdateDelegate guard), but the owner (resolved_authorities /
ctx.self_authority) is still allowed to revoke UpdateDelegate permissions (the
branch checking resolved_authorities and plugin.manager() ==
Authority::UpdateAuthority) to ensure the owner retains ultimate control;
mention that additional_delegates (self.additional_delegates) are intentionally
prevented from approving or revoking UpdateDelegate and reference
plugin.manager() == Authority::UpdateAuthority and the additional_delegates
check to make the rationale clear for future maintainers.
| // Or the authority is one of the additional delegates. | ||
| || (self.additional_delegates.contains(ctx.authority_info.key) && PluginType::from(plugin) != PluginType::UpdateDelegate) | ||
| // And it's an authority-managed plugin. | ||
| || (self.additional_delegates.contains(ctx.authority_info.key) && PluginType::from(plugin) != PluginType::UpdateDelegate)) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial
Line is excessively long — break for readability.
Line 155 is well over 120 characters. The other validation methods in this file (e.g., validate_add_plugin, validate_approve_plugin_authority) break their conditions across multiple lines. Keeping a consistent style also reduces the chance of future precedence bugs in this security-critical expression.
Suggested formatting
- || (self.additional_delegates.contains(ctx.authority_info.key) && PluginType::from(plugin) != PluginType::UpdateDelegate))
+ || (self.additional_delegates.contains(ctx.authority_info.key)
+ && PluginType::from(plugin) != PluginType::UpdateDelegate))📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| || (self.additional_delegates.contains(ctx.authority_info.key) && PluginType::from(plugin) != PluginType::UpdateDelegate)) | |
| || (self.additional_delegates.contains(ctx.authority_info.key) | |
| && PluginType::from(plugin) != PluginType::UpdateDelegate)) |
🤖 Prompt for AI Agents
In `@programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rs`
at line 155, The conditional expression in validate (the long line containing
self.additional_delegates.contains(ctx.authority_info.key) &&
PluginType::from(plugin) != PluginType::UpdateDelegate) is excessively long and
hard to read; reformat it to multiple lines consistent with other validators
(e.g., break after each boolean operator and align predicates), for example
split the OR branches and put the contains(...) and PluginType::from(...)
comparisons on separate lines so the whole condition is under 120 chars while
preserving exact logic and precedence for self.additional_delegates,
ctx.authority_info.key, PluginType::from(plugin), and
PluginType::UpdateDelegate.
blockiosaurus
merged commit 1a68114
into
metaplex-foundation:main
|
Thank you for this excellent PR! We appreciate the clear explanation and the provided fix with comments. However, in the future please disclose all security issues through bounty@metaplex.foundation rather than publicly posting the issue. |
2 participants
Security Finding
[MEDIUM] UpdateDelegate Operator Precedence Bug in Revoke Authority
File:
programs/mpl-core/src/plugins/internal/authority_managed/update_delegate.rs:149-154Issue:
The boolean condition in
validate_revoke_plugin_authorityhas an operator precedence bug:Impact: UpdateDelegate can revoke authority on owner-managed plugins (FreezeDelegate, TransferDelegate) that it should not control. Only UpdateAuthority-managed plugins should be revocable by the UpdateDelegate.
Fix: Added explicit parentheses so
plugin.manager() == Authority::UpdateAuthorityapplies to both the resolved_authorities and additional_delegates branches.Found during a Solana security audit