Skip to content

Moving ME checks to the plugin. - #273

Open
blockiosaurus wants to merge 35 commits into
opt/move-me-checksfrom
claude/resolve-pr-222-conflicts-ijaDA
Open

Moving ME checks to the plugin.#273
blockiosaurus wants to merge 35 commits into
opt/move-me-checksfrom
claude/resolve-pr-222-conflicts-ijaDA

Conversation

@blockiosaurus

@blockiosaurus blockiosaurus commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

Summary by CodeRabbit

  • Bug Fixes

    • Implemented stricter validation to prevent MasterEdition plugins from being added to assets in restricted contexts.
  • Error Handling

    • Improved error reporting with more specific and descriptive messages for invalid plugin operations.

danenbm and others added 30 commits May 13, 2025 07:02
* Add a BubblegumV2 permanent plugin which is used to indicate to the Bubblegum V2 program that this Core collection is suitable for storing cNFTs.

* The BubblegumV2 plugin limits what other plugins and external plugin adapters can be added to the collection, both at creation and via add instructions.

* Also added the instruction UpdateCollectionInfoV1, which allows a specific PDA signer from the Bubblegum program to directly change the collection num_minted and current_size fields.

* This is used for bookkeeping when cNFTs are minted, burned, or otherwise added to and removed from the collection.
* feat: implement freeze execute

* feat: add JS test for freeze execute

* feat: pr comments on type cast

* chore: code rabbit nit

* chore: remove unneeded plugin types

* chore: fix ci

* chore: fix js client ci

* chore: fix js client ci
* Bumping umi version

* Addressing comments

* Formatting nit
* feat: mid upgrade still doesn't compile

* chore: bump all solana versions to 2.0 and bump mpl-utils version

* chore: bump versions

* chore: force version 3 in lock file

* chore: frozen lockfile pnpm

* chore: updating github env to try to fix ci

* chore: updating github env to try to fix ci

* chore: updating github env to try to fix ci

* chore: match workflow files to githubenv

* chore: match workflow files to githubenv

* chore: final rustv change

* chore: add protobuf compiler to github workflows

* chore: upgrade anchor version in rust client

* chore: revert lock back

* chore: anchor ci error

* chore: fix js client ci

* chore: lockfile issue

* chore: lockfile issue

* chore: trying to fix all of the ci stuff

* chore: further bumping versions

* chore: bump down lock v

* chore: update gh actions solana v

* chore: fix version to crate published version

* chore: fix lockfile v again

* chore: update github env file

* chore: rustc to v1.83.0

* chore: lockfile

* chore: update package manager v

* chore: match package manager

* chore: downgrade package manager

* chore: run pnpm lockfile again with other v
* Bumping anchor version and adding discrim for compatibility

* Making const static to reflect trait
* Adding permanent freeze execute plugin

* Fixing based on feedback

* Fix rust client and change to wrong file

* Remove unnecessary steps

* Adding some extra tests

* Adding check
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.
…ence bug (#254)

* Add tests for PR 253: UpdateDelegate revoke authority operator precedence 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

* Fix tests: separate owner from update authority to correctly test the 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

* Remove redundant t.pass() call

https://claude.ai/code/session_01QVjAFPaMwv5T4NK3Y3DJYW

* Add positive test

---------

Co-authored-by: Claude <noreply@anthropic.com>
Co-authored-by: Michael Danenberg <56533526+danenbm@users.noreply.github.com>
* Adding agent identity external plugin

* Fix AI feedback

* Fixig wrong flag and adding more tests

* Fixing init collection with plugin

* Fixing coderabbit comments
…heck

Adding account owner check to be thorough
* Adding agent identity external plugin

* Fix AI feedback

* Fixig wrong flag and adding more tests

* Fixing init collection with plugin

* Fixing coderabbit comments

* Identity plugin should only be addable by ID program

* Switching to mollusk tests to pass

* Bump versions

* Feedback
* Adding agent identity external plugin

* Fix AI feedback

* Fixig wrong flag and adding more tests

* Fixing init collection with plugin

* Fixing coderabbit comments

* Identity plugin should only be addable by ID program

* Switching to mollusk tests to pass

* Bump versions

* Feedback

* Adding execute delegation capabilities

* Pointing to crates

---------

Co-authored-by: Sarah Strange <16522636+stranzhay@users.noreply.github.com>
* Removing double validation.

* Add some plugin validation override tests

---------

Co-authored-by: Michael Danenberg <56533526+danenbm@users.noreply.github.com>
* Adding ability for execute signer to pay fees

* Update programs/mpl-core/src/processor/execute.rs

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>

* Fix formatting

---------

Co-authored-by: coderabbitai[bot] <136622811+coderabbitai[bot]@users.noreply.github.com>
* Adding ability for execute signer to pay fees

* Adding exec delegate helper to TS function
* Fixing execute helper dropping signers

* Addressing CR feedback

* Adding tests, replacing instanceof with duck-type checks, fixing errors

* Fixing signer filter

* Simplifying type check
github-actions Bot and others added 5 commits April 2, 2026 12:21
Co-authored-by: blockiosaurus <90809591+blockiosaurus@users.noreply.github.com>
* feat: more consise pass

* feat: pr review 1

* feat: pr review 2

* feat: js client, rust client, tests

* chore: downgrade lock

* feat: pr review changes

* feat: adding more tests for groups

* chore: Release mpl-core version 0.10.1-alpha.1

* update mpl-core rust client to register groups plugin as known

* revert changes to cargo.lock, enfore rust toolchain version instead

* chore: Release mpl-core version 0.10.1-alpha.2

* update toolchain to proper version

* stranzhay/audit report (#255)

* refactor: [I-04] Duplicated code snippet

* fix: [I-03] Mark update_authority a signer in CreateGroupV1 metadata

* fix: [I-02] Reject non-child assets/collections/groups removal from parent groups

* fix: [I-01] Missing self-reference checks lets groups link to themselves

* fix: [L-05] Incorrect account resize on plugin authority approval

* fix: [L-04] Unvalidated incoming adapters in external plugin updates

* fix: [L-03] Memory violations in WriteGroupExternalPluginAdapterDataV1

* fix: [L-02] Accounts loaded from raw slice are inconsistent with schema

* fix: [L-01] Wrong memmove length when adding parent groups to assets/collections

* fix: [M-03] Block Groups plugins when creating assets/collections

* fix: [M-02] UpdateGroupPluginV1 arbitrarily alters installed plugins

* fix: [M-01] CloseGroupV1 closes group without checking child assets

* fix: [H-05] External adapter update may corrupt plugin metadata

* fix: [H-04] Child asset/collection plugin corruption from group removal

* fix: [H-03] CreateGroupV1 may corrupt parent/child group account

* fix: [H-02] Multiple IXs incorrectly truncate plugin data in GroupV1 accounts

* fix: [H-01] UpdateGroupPluginV1 corrupts group account plugin data

* fix: fixing tests

* fix: fixing tests

* feat: adding group to indexable asset

* stranzhay/audit report slim (#259)

* refactor: remove plugins

* feat: add in rest of group blocking

* chore: linting

* feat: further audit

* refactor: distribute audit coverage tests into their respective test files

* consolidate Groups plugin helpers into shared groups_plugin_utils module

* refactor: deduplicate plugin resize logic into save_updated_groups_plugin

* feat: pr review/more audit finalizing

* feat: final audit changes

* Removing double validation (#220)

* Removing double validation.

* Add some plugin validation override tests

---------

Co-authored-by: Michael Danenberg <56533526+danenbm@users.noreply.github.com>

* Fix lockfile and errors

* Removing duplicate check

---------

Co-authored-by: stranzhay <stranzhay@users.noreply.github.com>
Co-authored-by: blockiosaurus <90809591+blockiosaurus@users.noreply.github.com>
Co-authored-by: Michael Danenberg <56533526+danenbm@users.noreply.github.com>
Co-authored-by: Blockiosaurus <blockiosaurus@gmail.com>

* chore: code rabbit

* chore: ci

---------

Co-authored-by: pileks <pileks@users.noreply.github.com>
Co-authored-by: Pileks <jure@amber-it.co>
Co-authored-by: stranzhay <stranzhay@users.noreply.github.com>
Co-authored-by: blockiosaurus <90809591+blockiosaurus@users.noreply.github.com>
Co-authored-by: Michael Danenberg <56533526+danenbm@users.noreply.github.com>
Co-authored-by: Blockiosaurus <blockiosaurus@gmail.com>
* chore: bump rust client version to solana 3.0

* chore: update lock

* chore: ci still failing

* chore: bump cargo build-sbp in ci

* chore: fmt

* chore: temp workaround for ci

* chore: oml the last ci fix for this pr

* chore: ok last one for real

* chore: apply rustfmt output in rust test setup

* chore: format only generated rust files in kinobi

* chore: fix kinobi

* chore: undo lockfile changes

* Relax Rust client Solana pins to 3.0.0
…-conflicts-ijaDA

# Conflicts:
#	clients/js/src/generated/errors/mplCore.ts
#	clients/rust/src/generated/errors/mpl_core.rs
#	idls/mpl_core.json
#	programs/mpl-core/src/error.rs
#	programs/mpl-core/src/plugins/lifecycle.rs
#	programs/mpl-core/src/processor/add_plugin.rs
#	programs/mpl-core/src/processor/create.rs
@vercel

vercel Bot commented Apr 19, 2026

Copy link
Copy Markdown

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 Apr 19, 2026 1:33pm

Request Review

@coderabbitai

coderabbitai Bot commented Apr 19, 2026

Copy link
Copy Markdown
Contributor

Caution

Review failed

Failed to post review comments

Walkthrough

A new error variant PluginNotAllowedOnAsset is introduced across the IDL and error definitions, and MasterEdition plugin validation is refactored from blanket processor-level rejection to conditional validation logic within the plugin itself. Processors and lifecycle mappings are updated to accommodate this change.

Changes

Cohort / File(s) Summary
Configuration
.coderabbit.yaml
Added CodeRabbit configuration file with review automation settings, including profile, summaries, poem generation, and path-based review instructions.
Error Definitions
idls/mpl_core.json, programs/mpl-core/src/error.rs
Introduced new PluginNotAllowedOnAsset error variant (code 57) across both IDL and Rust error module with message "Plugin is not allowed to be added to an Asset".
Plugin Validation
programs/mpl-core/src/plugins/internal/authority_managed/master_edition.rs
Expanded MasterEdition PluginValidation implementation with explicit validate_create and validate_add_plugin methods that conditionally reject when MasterEdition targets an asset.
Lifecycle Permissions
programs/mpl-core/src/plugins/lifecycle.rs
Added PluginType::MasterEdition handling to return CheckResult::CanReject in both check_add_plugin and check_create match arms.
Processors
programs/mpl-core/src/processor/add_plugin.rs, programs/mpl-core/src/processor/create.rs
Removed blanket MasterEdition rejection from add_plugin; updated create to pass target_plugin context to validation and remove MasterEdition from early-return filter.
Tests
clients/js/test/plugins/collection/masterEdition.test.ts
Updated test expectations to assert PluginNotAllowedOnAsset error instead of InvalidPlugin for two error cases.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Poem

🐰 A fluffy validation dance,
MasterEdition takes its stance,
From blanket no to context-wise,
The plugin learns to scrutinize,
With error codes and checks so keen,
The cleanest code we've ever seen! ✨

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 50.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Description Check ✅ Passed Check skipped - CodeRabbit’s high-level summary is enabled.
Title check ✅ Passed The title 'Moving ME checks to the plugin' accurately reflects the main change: refactoring MasterEdition validation from processor-level checks to plugin-level validation logic.

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

✨ Finishing Touches
📝 Generate docstrings
  • Create stacked PR
  • Commit on current branch
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch claude/resolve-pr-222-conflicts-ijaDA

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

@blockiosaurus
blockiosaurus changed the base branch from main to opt/move-me-checks April 19, 2026 13:34

@github-actions github-actions Bot left a comment

Copy link
Copy Markdown

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: 4fa97e2 Previous: 6f2f577 Ratio
CU: create a new, empty asset 8023 Compute Units 8022 Compute Units 1.00
Space: create a new, empty asset 91 Bytes 91 Bytes 1
CU: create a new, empty asset with empty collection 16840 Compute Units 16839 Compute Units 1.00
Space: create a new, empty asset with empty collection 91 Bytes 91 Bytes 1
CU: create a new asset with plugins 26610 Compute Units 26608 Compute Units 1.00
Space: create a new asset with plugins 194 Bytes 194 Bytes 1
CU: create a new asset with plugins and empty collection 31938 Compute Units 31936 Compute Units 1.00
Space: create a new asset with plugins and empty collection 194 Bytes 194 Bytes 1
CU: list an asset 20041 Compute Units 20043 Compute Units 1.00
CU: sell an asset 25233 Compute Units 25233 Compute Units 1
CU: list an asset with empty collection 24991 Compute Units 24993 Compute Units 1.00
CU: sell an asset with empty collection 33304 Compute Units 33304 Compute Units 1
CU: list an asset with collection royalties 24274 Compute Units 24276 Compute Units 1.00
CU: sell an asset with collection royalties 36089 Compute Units 36089 Compute Units 1
CU: transfer an empty asset 3785 Compute Units 3785 Compute Units 1
CU: transfer an empty asset with empty collection 5498 Compute Units 5498 Compute Units 1
CU: transfer an asset with plugins 7213 Compute Units 7213 Compute Units 1
CU: transfer an asset with plugins and empty collection 8926 Compute Units 8926 Compute Units 1

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

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.

5 participants