Skip to content

fix: Handle malformed array type definitions in IDL parser#4029

Open
AvhiMaz wants to merge 3 commits intosolana-foundation:masterfrom
AvhiMaz:fix/idl-array-parser-panic
Open

fix: Handle malformed array type definitions in IDL parser#4029
AvhiMaz wants to merge 3 commits intosolana-foundation:masterfrom
AvhiMaz:fix/idl-array-parser-panic

Conversation

@AvhiMaz
Copy link
Copy Markdown

@AvhiMaz AvhiMaz commented Oct 26, 2025

Fixes #4016

Replace unsafe .unwrap() calls in array_from_str with proper error handling
to prevent panics on malformed IDL array syntax.

Changes:

  • Convert array_from_str return type from IdlType to Result<IdlType, Error>
  • Replace rsplit_once(';').unwrap() with .ok_or_else() for missing semicolons
  • Replace IdlType::from_str().unwrap() with .map_err() for invalid types
  • Add input validation for empty strings and size limits
  • Add validation for array element types and lengths
  • Add validation for generic parameter names
  • Include comprehensive test coverage for all error cases

  Replace unsafe .unwrap() calls in array_from_str with proper error handling
  to prevent panics on malformed IDL array syntax.

  Changes:
  - Convert array_from_str return type from IdlType to Result<IdlType, Error>
  - Replace rsplit_once(';').unwrap() with .ok_or_else() for missing semicolons
  - Replace IdlType::from_str().unwrap() with .map_err() for invalid types
  - Add input validation for empty strings and size limits
  - Add validation for array element types and lengths
  - Add validation for generic parameter names
  - Include comprehensive test coverage for all error cases

Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
@vercel
Copy link
Copy Markdown

vercel bot commented Oct 26, 2025

@AvhiMaz is attempting to deploy a commit to the Solana Foundation Team on Vercel.

A member of the Team first needs to authorize it.

@AvhiMaz AvhiMaz marked this pull request as ready for review October 27, 2025 10:30
type Err = anyhow::Error;

fn from_str(s: &str) -> Result<Self, Self::Err> {
// Input validation
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please remove this and comments like it - this just adds noise to the code

Comment on lines +322 to +324
if s.len() > 1000 {
return Err(anyhow!("Type string too long (max 1000 chars)"));
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

We don't need to add arbitrary length bounds

Comment on lines +407 to +415
// Validate reasonable array size
if len == 0 {
return Err(anyhow!("Array length cannot be zero"));
}
if len > 1_000_000 {
return Err(anyhow!(
"Array length too large (max 1,000,000)"
));
}
Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Please remove these checks, we shouldn't place arbitrary limits

)
}

// Tests for the vulnerability fix - missing semicolon
Copy link
Copy Markdown
Collaborator

@jamie-osec jamie-osec Oct 27, 2025

Choose a reason for hiding this comment

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

These are not vulnerabilities, and these comments are not necessary and add noise.

@AvhiMaz AvhiMaz requested a review from jamie-osec October 28, 2025 01:51
Signed-off-by: AvhiMaz <avhimazumder5@outlook.com>
@AvhiMaz AvhiMaz force-pushed the fix/idl-array-parser-panic branch from e6c38b4 to 5a2a398 Compare October 28, 2025 01:58
@jamie-osec jamie-osec added idl related to the IDL, either program or client side fix Bug fix PR labels Oct 28, 2025
@nutafrost nutafrost moved this to Security Review Required in Anchor 1.0 Oct 28, 2025
Copy link
Copy Markdown
Collaborator

@jamie-osec jamie-osec left a comment

Choose a reason for hiding this comment

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

LGTM if tests pass

@AvhiMaz
Copy link
Copy Markdown
Author

AvhiMaz commented Oct 28, 2025

@jamie-osec all test cases have passed, could you please merge it?

@jamie-osec jamie-osec moved this from Security Review Required to Security Review Done in Anchor 1.0 Oct 28, 2025
@jamie-osec jamie-osec removed this from Anchor 1.0 Oct 28, 2025
@jamie-osec jamie-osec removed their assignment Nov 18, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

fix Bug fix PR idl related to the IDL, either program or client side Merge

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Unhandled Panic in Anchor IDL Array Type Parser

5 participants