Skip to content

Conversation

@mxcl
Copy link
Member

@mxcl mxcl commented Mar 11, 2025

No description provided.

@mxcl mxcl requested a review from jhheider March 11, 2025 20:26
while ((dir = dirs.pop()) != undefined) {
for await (const [path, { name, isDirectory, isSymlink }] of dir.ls()) {
if (!isDirectory || isSymlink) continue;
if (/^v\d+\./.test(name)) {
Copy link
Contributor

Choose a reason for hiding this comment

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

should we test for ${basePath}/pkgs as well, to be careful?

Copy link
Member Author

Choose a reason for hiding this comment

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

basePath not set in this function, isn’t it just one of the 2 on line 436?

Copy link
Contributor

Choose a reason for hiding this comment

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

shorthand. what i meant was, since anything could be here, testing to ensure the link looks like basePath/pkgs/whatever/vFOO/ is safer.

but that's probably a v1-grade safety feature too.

if (isDirectory) {
dirs.push(resolved_path);
} else {
files.push(resolved_path);
Copy link
Contributor

Choose a reason for hiding this comment

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

i wonder if we should test the inodes here to ensure nothing hinky:

$ pkgm i git
$ ln -f /usr/bin/git /usr/local/bin/git
$ pkgm u git
# problem? maybe not...

Copy link
Member Author

Choose a reason for hiding this comment

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

we should be more careful, but that's a v1 feature.

@mxcl mxcl merged commit b89485e into main Mar 12, 2025
3 checks passed
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.

3 participants