Skip to content

fix(pack): harden asset archive extraction - #306

Open
kgarg2468 wants to merge 1 commit into
smol-machines:mainfrom
kgarg2468:kgarg/fix-archive-path-safety
Open

fix(pack): harden asset archive extraction#306
kgarg2468 wants to merge 1 commit into
smol-machines:mainfrom
kgarg2468:kgarg/fix-archive-path-safety

Conversation

@kgarg2468

Copy link
Copy Markdown

Summary

  • replace direct asset tar unpacking with a safe extraction helper
  • reject absolute/parent paths plus hardlinks and symlinks before extracting assets
  • add regression tests for normal extraction, traversal paths, and link entries

Validation

  • cargo fmt --check
  • cargo test -p smolvm-pack

@ghost

ghost commented May 27, 2026

Copy link
Copy Markdown

Ready to review this PR? Stage has broken it down into 3 individual chapters for you:

Title
1 Implement safe archive extraction helpers
2 Apply safe extraction to asset decompression
3 Add regression tests for asset extraction
Open in Stage

Chapters generated by Stage for commit 5c4a0e4 on May 27, 2026 9:06pm UTC.

@BinSquare BinSquare left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

looks good, requesting for slight expansion of test coverage + nits

);
}

#[test]

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

it's unclear what this specific part is doing? What is the filesystem breakout scenario?

}

#[test]
fn test_decompress_assets_extracts_regular_files() {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

a normal pack is extracting regular files so this test is unnecessary

Ok(0) // Entire file is zeros
}

fn validate_archive_path(path: &Path) -> Result<()> {

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

would want test in particular for this specific functionality

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

Status: Backlog

Development

Successfully merging this pull request may close these issues.

2 participants