fix: only allow select instructions in privileged mode#19
Closed
bmuddha wants to merge 1 commit into
Closed
Conversation
WalkthroughThis PR implements a conditional access-validation bypass for privileged fee payers when transactions contain allowlisted Magic program instructions. The feature adds Magic program ID and discriminant constants, implements security logic to scan instructions and validate they match Magic and an allowlist, expands writable-account permissions to include confined accounts, and validates the changes through unit and integration tests. ChangesPrivileged Magic Instruction Allowlisting
🎯 3 (Moderate) | ⏱️ ~25 minutes Possibly Related PRs
Suggested Reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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: 2
🤖 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 `@src/access_permissions.rs`:
- Around line 128-145: The test coverage for the scan-all bypass is incomplete:
the helper message() (returning SanitizedMessage::Legacy via LegacyMessage::new)
and the existing tests only exercise single-instruction messages, so the bypass
could incorrectly use any()-style logic; add new regression tests that construct
multi-instruction messages using the message(...) helper (or a similar helper)
with one allowlisted Magic instruction and one instruction that is either a
non-Magic program or a Magic program not in the allowlist, then assert the
scan-all rule does NOT bypass (i.e., it must require full scanning). Update both
places referenced (the existing single-instruction tests around the message(...)
helper and the other block noted at lines ~148-185) to include this
mixed-instruction case so the bypass only applies when every instruction is
allowlisted Magic.
- Around line 38-62: The current logic lets a privileged payer with zero
instructions bypass because `privileged` remains true; change the loop in the
block that iterates `message.instructions_iter()` to require at least one
allowlisted Magic instruction before returning early. Add a boolean (e.g.,
`found_magic`) initialized false, set it true only when you encounter an
instruction whose program equals `MAGIC_PROGRAM_ID` and whose discriminant is in
`PRIVILEGED_MAGIC_DISCRIMINANTS`; keep using the existing checks inside the
loop, but at the end only treat the transaction as privileged (and reach the
existing `return Ok(())`) if both the original `privileged` condition and
`found_magic` are true. Update references to `privileged`/early-break behavior
accordingly so an empty instruction list no longer bypasses.
🪄 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: Organization UI
Review profile: ASSERTIVE
Plan: Pro
Run ID: da597afb-343e-431f-ac28-571e4c29f9f9
📒 Files selected for processing (2)
src/access_permissions.rstests/integration_test.rs
Comment on lines
+38
to
62
| let mut privileged = payer.is_some_and(AccountSharedData::privileged); | ||
| if privileged { | ||
| for i in message.instructions_iter() { | ||
| let Some(program) = message.account_keys().get(i.program_id_index as usize) else { | ||
| privileged = false; | ||
| break; | ||
| }; | ||
| if *program != MAGIC_PROGRAM_ID { | ||
| privileged = false; | ||
| break; | ||
| } | ||
| let discriminant = i | ||
| .data | ||
| .get(0..4) | ||
| .and_then(|b| <[u8; 4]>::try_from(b).ok()) | ||
| .map(u32::from_le_bytes) | ||
| .unwrap_or(u32::MAX); | ||
| if !PRIVILEGED_MAGIC_DISCRIMINANTS.contains(&discriminant) { | ||
| privileged = false; | ||
| break; | ||
| } | ||
| } | ||
| } | ||
| if privileged { | ||
| return Ok(()); |
There was a problem hiding this comment.
Require at least one allowlisted Magic instruction before bypassing.
As written, a privileged payer with an empty instruction list keeps privileged == true and returns Ok(()), even though the transaction does not contain any allowlisted Magic instruction. That broadens the bypass beyond the documented contract.
Suggested fix
let payer = self.accounts.first().map(|(_, acc)| acc);
let mut privileged = payer.is_some_and(AccountSharedData::privileged);
+ let mut saw_instruction = false;
if privileged {
for i in message.instructions_iter() {
+ saw_instruction = true;
let Some(program) = message.account_keys().get(i.program_id_index as usize) else {
privileged = false;
break;
};
if *program != MAGIC_PROGRAM_ID {
@@
if !PRIVILEGED_MAGIC_DISCRIMINANTS.contains(&discriminant) {
privileged = false;
break;
}
}
}
- if privileged {
+ if privileged && saw_instruction {
return Ok(());
}📝 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.
Suggested change
| let mut privileged = payer.is_some_and(AccountSharedData::privileged); | |
| if privileged { | |
| for i in message.instructions_iter() { | |
| let Some(program) = message.account_keys().get(i.program_id_index as usize) else { | |
| privileged = false; | |
| break; | |
| }; | |
| if *program != MAGIC_PROGRAM_ID { | |
| privileged = false; | |
| break; | |
| } | |
| let discriminant = i | |
| .data | |
| .get(0..4) | |
| .and_then(|b| <[u8; 4]>::try_from(b).ok()) | |
| .map(u32::from_le_bytes) | |
| .unwrap_or(u32::MAX); | |
| if !PRIVILEGED_MAGIC_DISCRIMINANTS.contains(&discriminant) { | |
| privileged = false; | |
| break; | |
| } | |
| } | |
| } | |
| if privileged { | |
| return Ok(()); | |
| let mut privileged = payer.is_some_and(AccountSharedData::privileged); | |
| let mut saw_instruction = false; | |
| if privileged { | |
| for i in message.instructions_iter() { | |
| saw_instruction = true; | |
| let Some(program) = message.account_keys().get(i.program_id_index as usize) else { | |
| privileged = false; | |
| break; | |
| }; | |
| if *program != MAGIC_PROGRAM_ID { | |
| privileged = false; | |
| break; | |
| } | |
| let discriminant = i | |
| .data | |
| .get(0..4) | |
| .and_then(|b| <[u8; 4]>::try_from(b).ok()) | |
| .map(u32::from_le_bytes) | |
| .unwrap_or(u32::MAX); | |
| if !PRIVILEGED_MAGIC_DISCRIMINANTS.contains(&discriminant) { | |
| privileged = false; | |
| break; | |
| } | |
| } | |
| } | |
| if privileged && saw_instruction { | |
| return Ok(()); |
🤖 Prompt for 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.
In `@src/access_permissions.rs` around lines 38 - 62, The current logic lets a
privileged payer with zero instructions bypass because `privileged` remains
true; change the loop in the block that iterates `message.instructions_iter()`
to require at least one allowlisted Magic instruction before returning early.
Add a boolean (e.g., `found_magic`) initialized false, set it true only when you
encounter an instruction whose program equals `MAGIC_PROGRAM_ID` and whose
discriminant is in `PRIVILEGED_MAGIC_DISCRIMINANTS`; keep using the existing
checks inside the loop, but at the end only treat the transaction as privileged
(and reach the existing `return Ok(())`) if both the original `privileged`
condition and `found_magic` are true. Update references to
`privileged`/early-break behavior accordingly so an empty instruction list no
longer bypasses.
Comment on lines
+128
to
+145
| fn message(program: Pubkey, data: Vec<u8>) -> SanitizedMessage { | ||
| SanitizedMessage::Legacy(LegacyMessage::new( | ||
| Message { | ||
| account_keys: vec![Pubkey::new_unique(), Pubkey::new_unique(), program], | ||
| header: MessageHeader { | ||
| num_required_signatures: 1, | ||
| num_readonly_signed_accounts: 0, | ||
| num_readonly_unsigned_accounts: 1, | ||
| }, | ||
| instructions: vec![CompiledInstruction { | ||
| program_id_index: 2, | ||
| accounts: vec![1], | ||
| data, | ||
| }], | ||
| recent_blockhash: Hash::default(), | ||
| }, | ||
| &ReservedAccountKeys::empty_key_set(), | ||
| )) |
There was a problem hiding this comment.
🧹 Nitpick | 🔵 Trivial | ⚡ Quick win
Add a mixed-instruction regression test for the scan-all rule.
The bypass logic is only safe if every instruction is Magic + allowlisted, but the new tests only cover single-instruction messages. Please add a case with one allowlisted Magic instruction plus one non-Magic or unlisted Magic instruction so an accidental any()-style regression gets caught.
Also applies to: 148-185
🤖 Prompt for 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.
In `@src/access_permissions.rs` around lines 128 - 145, The test coverage for the
scan-all bypass is incomplete: the helper message() (returning
SanitizedMessage::Legacy via LegacyMessage::new) and the existing tests only
exercise single-instruction messages, so the bypass could incorrectly use
any()-style logic; add new regression tests that construct multi-instruction
messages using the message(...) helper (or a similar helper) with one
allowlisted Magic instruction and one instruction that is either a non-Magic
program or a Magic program not in the allowlist, then assert the scan-all rule
does NOT bypass (i.e., it must require full scanning). Update both places
referenced (the existing single-instruction tests around the message(...) helper
and the other block noted at lines ~148-185) to include this mixed-instruction
case so the bypass only applies when every instruction is allowlisted Magic.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary by CodeRabbit
Bug Fixes
Tests