Skip to content

feat: prevent cranking privileged instructions#1223

Open
Dodecahedr0x wants to merge 3 commits into
masterfrom
dode/check-privilege
Open

feat: prevent cranking privileged instructions#1223
Dodecahedr0x wants to merge 3 commits into
masterfrom
dode/check-privilege

Conversation

@Dodecahedr0x
Copy link
Copy Markdown
Contributor

@Dodecahedr0x Dodecahedr0x commented May 19, 2026

Closes #1222

Summary

Introduce a new signing authority for cranks

Breaking Changes

  • None

Summary by CodeRabbit

  • Bug Fixes
    • Enhanced instruction validation to prevent execution of privileged program-management operations, improving security by blocking unauthorized instruction variants.

Review Change Stack

@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented May 19, 2026

Warning

Rate limit exceeded

@Dodecahedr0x has exceeded the limit for the number of commits that can be reviewed per hour. Please wait 38 minutes and 22 seconds before requesting another review.

You’ve run out of usage credits. Purchase more in the billing tab.

⌛ How to resolve this issue?

After the wait time has elapsed, a review can be triggered using the @coderabbitai review command as a PR comment. Alternatively, push new commits to this PR.

We recommend that you space out your commits to avoid hitting the rate limit.

🚦 How do rate limits work?

CodeRabbit enforces hourly rate limits for each developer per organization.

Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout.

Please see our FAQ for further information.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 9d8f1902-2f49-434d-aa71-7ac289ec6dff

📥 Commits

Reviewing files that changed from the base of the PR and between a85f5d5 and 4acb34a.

📒 Files selected for processing (1)
  • programs/magicblock/src/schedule_task/mod.rs
📝 Walkthrough

Walkthrough

This PR adds security validation to the crank instruction processing pipeline. It imports MagicBlockInstruction type, adds a clarifying comment about the privileged instruction constraint in the validation function, and extends validate_cranks_instructions to attempt bincode-decoding each instruction's data as a MagicBlockInstruction. When a privileged program-management variant is detected, the validation logs an error and rejects the instruction with InstructionError::InvalidInstructionData. Non-MagicBlock instructions and non-privileged variants are skipped without error.

Assessment against linked issues

Objective Addressed Explanation
Prevent validator from signing on user-provided privileged instructions [#1222] The validation now decodes instructions and rejects privileged MagicBlock variants before they can be executed in the crank context.

Suggested reviewers

  • taco-paco
  • GabrielePicco
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Commit unit tests in branch dode/check-privilege

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.

❤️ Share

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

Copy link
Copy Markdown
Contributor

@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: 4

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@magicblock-api/src/fund_account.rs`:
- Around line 48-50: Change init_execution_authority to return Result<(),
ApiError> and remove the panic-prone unwrap: replace the call to
accountsdb.get_account(execution_authority_id).unwrap() with error-aware code
that returns a structured ApiError when the account is missing or retrieval
fails (e.g., use Option/Result combinators or map_err/ok_or to produce
ApiError::StorageError or another appropriate ApiError variant). After setting
authority.set_delegated(true), propagate any error from
accountsdb.insert_account(execution_authority_id, &authority) by mapping its
failure into an ApiError instead of ignoring it; ensure callers of
init_execution_authority are updated to handle the Result. Reference symbols:
init_execution_authority, accountsdb.get_account, authority.set_delegated,
accountsdb.insert_account, and ApiError.

In `@magicblock-api/src/magic_validator.rs`:
- Around line 286-294: The code currently creates a fresh execution authority
each startup via Keypair::new() (execution_authority_keypair) and calls
init_execution_authority(&accountsdb, &execution_authority_keypair.pubkey()),
which rotates crank authority and breaks replay; instead persist and load the
execution keypair similar to how validator identity is handled by
init_validator_identity/validator_pubkey. Modify startup to attempt to load the
saved execution keypair from storage (or generate-and-save if missing), then
pass its pubkey to init_execution_authority; ensure the same persistent
storage/location and serialization format used for validator identity is reused
so the execution authority survives restarts.

In `@programs/magicblock/src/execution_authority.rs`:
- Around line 18-24: The current init_execution_authority and
set_execution_authority_override discard the Result from OnceLock::set, silently
ignoring double-initialization; change both functions (init_execution_authority
and set_execution_authority_override) to check the Result returned by
EXECUTION_AUTHORITY.set(...) and EXECUTION_AUTHORITY_OVERRIDE.set(...), and fail
fast on Err (e.g., panic or return a Result) with a clear message including the
attempted pubkey/keypair to surface misconfiguration rather than silently
proceeding. Ensure the error branch is reachable so re-init attempts do not get
ignored.

In `@programs/magicblock/src/utils/instruction_utils.rs`:
- Around line 268-269: The AccountMeta list was changed to require
execution_authority_id() as a signer but execute_task(...) still signs with
validator_authority(), causing missing signatures; update the execute_task
implementation to sign the transaction with the execution authority keypair
(replace usage of validator_authority()/validator signer with the execution
authority keypair or call that returns the execution authority signer) so the
signer matches AccountMeta::new_readonly(execution_authority_id(), true) and the
transaction includes the required execution-authority signature.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 61919831-6458-46e1-bb0b-cf6098663eac

📥 Commits

Reviewing files that changed from the base of the PR and between 21b6564 and ab3c87c.

📒 Files selected for processing (7)
  • magicblock-api/src/fund_account.rs
  • magicblock-api/src/magic_validator.rs
  • magicblock-task-scheduler/src/service.rs
  • programs/magicblock/src/execution_authority.rs
  • programs/magicblock/src/lib.rs
  • programs/magicblock/src/schedule_task/process_execute_task.rs
  • programs/magicblock/src/utils/instruction_utils.rs

Comment thread magicblock-api/src/fund_account.rs Outdated
Comment thread magicblock-api/src/magic_validator.rs Outdated
Comment thread programs/magicblock/src/execution_authority.rs Outdated
Comment thread programs/magicblock/src/utils/instruction_utils.rs Outdated
@Dodecahedr0x Dodecahedr0x force-pushed the dode/check-privilege branch from ab3c87c to a85f5d5 Compare May 19, 2026 12:59
@Dodecahedr0x Dodecahedr0x marked this pull request as ready for review May 19, 2026 13:32
@Dodecahedr0x Dodecahedr0x changed the title feat: use executor authority instead of validator feat: prevent cranking privileged instrutions May 19, 2026
@Dodecahedr0x Dodecahedr0x changed the title feat: prevent cranking privileged instrutions feat: prevent cranking privileged instructions May 19, 2026
Copy link
Copy Markdown
Contributor

@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

🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

Inline comments:
In `@programs/magicblock/src/schedule_task/mod.rs`:
- Around line 48-73: The code currently attempts to bincode::deserialize every
instruction into MagicBlockInstruction (decoded_instruction) without verifying
the instruction is for the MagicBlock program; change this to first check
instruction.program_id matches the MagicBlock program id (e.g., the MagicBlock
program's public key constant or id() function) and only then call
bincode::deserialize::<MagicBlockInstruction>(&instruction.data); if the
program_id does not match, skip/continue instead of attempting to decode. Update
the logic around decoded_instruction in schedule_task/mod.rs so the program id
guard happens before deserialization and the privileged-instruction rejection
remains only for true MagicBlock instructions.
🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

  • Push a commit to this branch (recommended)
  • Create a new PR with the fixes

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: ASSERTIVE

Plan: Pro

Run ID: 38faae46-3a48-4e4d-b1e4-1c1862ee790b

📥 Commits

Reviewing files that changed from the base of the PR and between ab3c87c and a85f5d5.

⛔ Files ignored due to path filters (2)
  • Cargo.lock is excluded by !**/*.lock
  • test-integration/Cargo.lock is excluded by !**/*.lock
📒 Files selected for processing (1)
  • programs/magicblock/src/schedule_task/mod.rs

Comment thread programs/magicblock/src/schedule_task/mod.rs
Copy link
Copy Markdown
Collaborator

@GabrielePicco GabrielePicco left a comment

Choose a reason for hiding this comment

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

LGTM!

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.

bug: use a new authority for cranks and callbacks

2 participants