Skip to content

Fix unescaped data path extraction issue - #128

Merged
JustinGrote merged 8 commits into
mainfrom
fix/unescaped-data-path-extraction
Oct 23, 2026
Merged

JustinGrote merged 8 commits into
mainfrom
fix/unescaped-data-path-extraction

Conversation

@JustinGrote

Copy link
Copy Markdown
Owner

Address an issue where data paths were not properly escaped during extraction, leading to potential security vulnerabilities. This update ensures that all data paths are correctly handled to prevent any unintended exposure.

Fixes #115

Address an issue where data paths were not properly escaped during extraction, leading to potential security vulnerabilities. This update ensures that all data paths are correctly handled to prevent any unintended exposure.

Fixes #115

Copilot AI 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.

🟡 Changes recommended

The replacement extractor introduces permission, Windows directory-handling, and cancellation regressions.

3 open issues
What changed in this PR

This PR addresses #115 by decoding URL-encoded package paths during module installation and validating that decoded paths remain inside the extraction directory.

Changes:

  • Replaces whole-archive extraction with a custom asynchronous extraction loop.
  • Adds destination containment checks after URL decoding.
  • Adds a regression test for encoded spaces in file and directory names.
File Description
Source/​Core/​ModuleFastInstaller.cs Decodes archive paths, validates destinations, and extracts entries asynchronously.
ModuleFast.tests.ps1 Tests extraction of URL-encoded file and directory names.

Review effort: Balanced

Comment thread Source/Core/ModuleFastInstaller.cs
Comment thread Source/Core/ModuleFastInstaller.cs Outdated
Comment thread Source/Core/ModuleFastInstaller.cs Outdated
JustinGrote and others added 3 commits October 23, 2026 09:54
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: Copilot Autofix powered by AI <175728472+Copilot@users.noreply.github.com>
Co-authored-by: JustinGrote <15258962+JustinGrote@users.noreply.github.com>

Copilot AI 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.

🟡 Changes recommended

Undeclared stream variables prevent compilation, and missing parent-directory creation breaks extraction of valid packages without explicit directory entries.

2 open issues
3 resolved since last review

Review effort: Balanced

Comment thread Source/Core/ModuleFastInstaller.cs Outdated
Comment thread Source/Core/ModuleFastInstaller.cs
Co-authored-by: JustinGrote <15258962+JustinGrote@users.noreply.github.com>
Co-authored-by: JustinGrote <15258962+JustinGrote@users.noreply.github.com>

Copilot AI 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.

🟡 Changes recommended

Windows filename sanitization remains unresolved, and the replacement path-containment protection lacks negative regression tests.

1 open issue
1 previously missed issue

In code that hasn't changed since last review

Medium severity Add extraction tests for decoded path traversal protection

Source/​Core/​ModuleFastInstaller.cs:53

The new extraction tests cover valid decoded names and cancellation, but do not exercise this replacement for the framework's path-traversal protection. They would still pass if the containment check were removed or moved before decoding. Add negative cases for %2e%2e/outside.txt, an absolute path outside the destination, and traversal into a sibling directory whose name shares the destination prefix. On Windows, also cover encoded backslashes such as %2e%2e%5coutside.txt. Assert that extraction throws and creates no file or directory outside the destination.

2 resolved since last review

Review effort: Balanced

{
ct.ThrowIfCancellationRequested();

string entryName = Uri.UnescapeDataString(entry.FullName);

Copy link
Copy Markdown
Owner Author

Choose a reason for hiding this comment

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

@copilot revise to use Path.GetInvalidFileNameChars() in a cross platform way to replace invalid characters with _

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.

Applied Path.GetInvalidFileNameChars() sanitization after URL decoding, replacing invalid characters with _ while preserving separators. Build and extraction regression tests pass; commit 5907ea6.

Co-authored-by: JustinGrote <15258962+JustinGrote@users.noreply.github.com>
@JustinGrote
JustinGrote merged commit e0fda85 into main Oct 23, 2026
4 checks passed
@JustinGrote
JustinGrote deleted the fix/unescaped-data-path-extraction branch October 23, 2026 18:57
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.

File paths need to be Url decoded when extracting

3 participants