Skip to content

Add tests for PR 253: UpdateDelegate revoke authority operator precedence bug#254

Merged
danenbm merged 5 commits intomainfrom
claude/slack-add-mpl-core-tests-bzwxP
Feb 12, 2026
Merged

Add tests for PR 253: UpdateDelegate revoke authority operator precedence bug#254
danenbm merged 5 commits intomainfrom
claude/slack-add-mpl-core-tests-bzwxP

Conversation

@blockiosaurus
Copy link
Contributor

@blockiosaurus blockiosaurus commented Feb 11, 2026

These tests validate the security vulnerability where UpdateDelegate's
validate_revoke_plugin_authority function has incorrect operator precedence,
allowing UpdateDelegate to revoke authority on owner-managed plugins
(FreezeDelegate, TransferDelegate) when it should only be able to revoke
authority on UpdateAuthority-managed plugins.

The tests are designed to:

  • FAIL with the current buggy code (revoke succeeds incorrectly)
  • PASS after PR 253 fix is applied (revoke correctly throws NoApprovals)

Slack thread: https://metaplexfoundation.slack.com/archives/C08DQ50FBC2/p1770843548886539?thread_ts=1770840905.712979&cid=C08DQ50FBC2

https://claude.ai/code/session_01QVjAFPaMwv5T4NK3Y3DJYW


Note

Low Risk
Test-only changes that add coverage for a known security bug without modifying runtime logic.

Overview
Adds a new AVA test suite updateDelegateRevokeBug.test.ts that reproduces the UpdateDelegate revoke-authority operator-precedence vulnerability and asserts the corrected behavior.

The tests ensure revoking plugin authority via UpdateDelegate fails with NoApprovals for owner-managed plugins (e.g. FreezeDelegate, TransferDelegate) for both update-authority and delegated update-delegate signers, while confirming revocation still succeeds for UpdateAuthority-managed plugins (e.g. Edition).

Written by Cursor Bugbot for commit b01c22c. This will update automatically on new commits. Configure here.

…ence bug

These tests validate the security vulnerability where UpdateDelegate's
validate_revoke_plugin_authority function has incorrect operator precedence,
allowing UpdateDelegate to revoke authority on owner-managed plugins
(FreezeDelegate, TransferDelegate) when it should only be able to revoke
authority on UpdateAuthority-managed plugins.

The tests are designed to:
- FAIL with the current buggy code (revoke succeeds incorrectly)
- PASS after PR 253 fix is applied (revoke correctly throws NoApprovals)

Slack thread: https://metaplexfoundation.slack.com/archives/C08DQ50FBC2/p1770843548886539?thread_ts=1770840905.712979&cid=C08DQ50FBC2

https://claude.ai/code/session_01QVjAFPaMwv5T4NK3Y3DJYW
@vercel
Copy link

vercel bot commented Feb 11, 2026

The latest updates on your projects. Learn more about Vercel for GitHub.

Project Deployment Actions Updated (UTC)
mpl-core-js-docs Ready Ready Preview, Comment Feb 12, 2026 0:17am

Request Review

@coderabbitai
Copy link

coderabbitai bot commented Feb 11, 2026

Summary by CodeRabbit

  • Tests
    • Added comprehensive test suite validating UpdateDelegate revoke authority behavior across different plugin and authority combinations, including both successful owner revocations and unauthorized revocation scenarios.

Walkthrough

Adds a new test suite clients/js/test/plugins/asset/updateDelegateRevokeBug.test.ts that reproduces and validates an operator-precedence bug in UpdateDelegate revoke authority logic across owner-managed and UpdateAuthority-managed plugins.

Changes

Cohort / File(s) Summary
New test suite
clients/js/test/plugins/asset/updateDelegateRevokeBug.test.ts
Adds a ~329-line test file that creates assets with combinations of UpdateDelegate and various plugins, exercises approval and revoke flows (including delegated UpdateDelegate cases), and verifies NoApprovals/permission behaviors for owner-managed vs UpdateAuthority-managed plugins.

Estimated code review effort

🎯 2 (Simple) | ⏱️ ~10 minutes

Possibly related PRs

🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and specifically summarizes the main change: adding tests for a known security bug (PR 253) related to UpdateDelegate operator precedence.
Description check ✅ Passed The description is directly related to the changeset, explaining the security vulnerability being tested and the expected behavior of the tests.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing touches
  • 📝 Generate docstrings
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch claude/slack-add-mpl-core-tests-bzwxP

No actionable comments were generated in the recent review. 🎉


Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@clients/js/test/plugins/asset/updateDelegateRevokeBug.test.ts`:
- Line 198: The test contains a redundant t.pass() call after assertions made by
assertAsset (lines around the test block in updateDelegateRevokeBug.test.ts);
remove the explicit t.pass() invocation to rely on AVA's automatic pass behavior
when all assertions in the test (including the assertAsset checks)
succeed—locate and delete the t.pass() statement inside the failing test
function.

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

Bugbot Autofix is OFF. To automatically fix reported issues with Cloud Agents, enable Autofix in the Cursor dashboard.

This is the final PR Bugbot will review for you during this billing cycle

Your free Bugbot reviews will reset on March 25

Details

You are on the Bugbot Free tier. On this plan, Bugbot will review limited PRs each billing cycle.

To receive Bugbot reviews on all of your PRs, visit the Cursor dashboard to activate Pro and start your 14-day free trial.

Copy link

@github-actions github-actions bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Benchmark

Details
Benchmark suite Current: 08723d4 Previous: 1a68114 Ratio
CU: create a new, empty asset 7628 Compute Units 7628 Compute Units 1
Space: create a new, empty asset 91 Bytes 91 Bytes 1
CU: create a new, empty asset with empty collection 15706 Compute Units 15706 Compute Units 1
Space: create a new, empty asset with empty collection 91 Bytes 91 Bytes 1
CU: create a new asset with plugins 25922 Compute Units 25922 Compute Units 1
Space: create a new asset with plugins 194 Bytes 194 Bytes 1
CU: create a new asset with plugins and empty collection 30830 Compute Units 30830 Compute Units 1
Space: create a new asset with plugins and empty collection 194 Bytes 194 Bytes 1
CU: list an asset 19019 Compute Units 19019 Compute Units 1
CU: sell an asset 24206 Compute Units 24206 Compute Units 1
CU: list an asset with empty collection 23516 Compute Units 23516 Compute Units 1
CU: sell an asset with empty collection 31593 Compute Units 31593 Compute Units 1
CU: list an asset with collection royalties 22906 Compute Units 22906 Compute Units 1
CU: sell an asset with collection royalties 34644 Compute Units 34644 Compute Units 1
CU: transfer an empty asset 3611 Compute Units 3611 Compute Units 1
CU: transfer an empty asset with empty collection 5171 Compute Units 5171 Compute Units 1
CU: transfer an asset with plugins 7048 Compute Units 7048 Compute Units 1
CU: transfer an asset with plugins and empty collection 8608 Compute Units 8608 Compute Units 1

This comment was automatically generated by workflow using github-action-benchmark.

… bug

The previous tests used umi.identity as both owner and update authority.
This caused the revoke to succeed via owner permissions rather than testing
the UpdateDelegate bug path.

Now each test uses a separate owner signer distinct from the update authority,
ensuring the signer acts ONLY as update authority when attempting to revoke.

https://claude.ai/code/session_01QVjAFPaMwv5T4NK3Y3DJYW
Copy link

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Actionable comments posted: 1

🤖 Fix all issues with AI agents
In `@clients/js/test/plugins/asset/updateDelegateRevokeBug.test.ts`:
- Around line 1-347: Add a positive test that verifies the owner can revoke
authority on owner-managed plugins: create a new test using createUmi and
generateSigner to make a distinct owner and a new delegate key, create an asset
with a FreezeDelegate via createAsset (owner set to the owner.publicKey), call
approvePluginAuthority to set the delegate (signed by owner), then call
revokePluginAuthority with authority set to the owner to revoke it, and finally
assert the plugin reverted to owner-managed using assertAsset and DEFAULT_ASSET;
use the same helper symbols (createUmi, generateSigner, createAsset,
approvePluginAuthority, revokePluginAuthority, assertAsset, DEFAULT_ASSET) and
mirror the structure of the existing tests.

danenbm
danenbm previously approved these changes Feb 12, 2026
@danenbm danenbm merged commit e021ca4 into main Feb 12, 2026
17 checks passed
@danenbm danenbm deleted the claude/slack-add-mpl-core-tests-bzwxP branch February 12, 2026 00:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants