Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: isUnpackedDir matches non-child directories that have same folder name prefix #333

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

mmaietta
Copy link

Fixes: #331 and updates test case for minimatch patterns. (Happy to write additional or separate tests if preferred)

Test project file structure:

/project# tree "/tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-jp88Jv/asar-app-1"
/tmp/et-db411cd21f3bbd882f3a5a30c58db6ff/t-jp88Jv/asar-app-1
├── app
│   ├── package.json
│   │   └── readme.md
│   └── readme.md
├── index.html
├── index.js
├── node_modules
│   ├── @electron-builder
│   │   ├── test-smart-unpack
│   │   │   ├── foo.dll
│   │   │   └── package.json
│   │   └── test-smart-unpack-empty
│   │       └── package.json

The pattern passed to unpackDir minimatch glob.

pattern = "{node_modules/@electron-builder/test-smart-unpack,node_modules/edge-cs}"

asar/src/asar.ts

Lines 22 to 31 in 464e834

function isUnpackedDir(dirPath: string, pattern: string, unpackDirs: string[]) {
if (dirPath.startsWith(pattern) || minimatch(dirPath, pattern)) {
if (!unpackDirs.includes(dirPath)) {
unpackDirs.push(dirPath);
}
return true;
} else {
return unpackDirs.some((unpackDir) => dirPath.startsWith(unpackDir));
}
}

The issue I'm running into is that test-smart-unpack-empty is not supposed to be getting unpacked as it fails the minimatch, but the else logic in isUnpackedDir is causing it to do a startsWith match on the folder path and then returns true.
Image

This PR introduces a check to make sure that the previous folders being unpacked do not include folders that happen to have the same prefix but are not a child directory of a previously flagged unpacked directory.

Additional note, this does not change logic for non-minimatch patterns (e.g. --unpack-dir dir2/subdir) as that logic flow is left unchanged.

if (dirPath.startsWith(pattern) || minimatch(dirPath, pattern)) {

@mmaietta mmaietta requested a review from a team as a code owner October 15, 2024 19:44
@mmaietta mmaietta marked this pull request as draft October 15, 2024 20:35
@mmaietta
Copy link
Author

So I'm not sure why the unit tests are timing out with Error: Timeout of 2000ms exceeded. The timeout seems to occur on random tests between runs, not on the test case that I updated.
First timeout was on UT:

1) should create archive from directory (without hidden files)

Second timeout was on UT:

1) should create archive from directory with unpacked dirs and files

@mmaietta mmaietta marked this pull request as ready for review October 21, 2024 15:43
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.

isUnpackedDir unintentionally unpacks other directories with same dir name prefix
1 participant