also looking at this
fix(install): discover executable nested npm programs without leaking candidates
#270
1 file
+117
-19
| @@ -648,19 +648,21 @@ fn discoverNpmBinaries( | ||
| 648 | 648 | ); |
| 649 | 649 | errdefer allocator.free(full_path); |
| 650 | 650 | |
| 651 | // Check if executable using io_helper | |
| 652 | const stat = io_helper.statFile(full_path) catch { | |
| 651 | // statFile reports mode 0 wherever the compiler's stat type carries | |
| 652 | // no mode field, so permission bits cannot decide this. isExecutable | |
| 653 | // owns executable detection across the supported platforms. | |
| 654 | if (!io_helper.isExecutable(full_path)) { | |
| 653 | 655 | allocator.free(full_path); |
| 654 | 656 | continue; |
| 655 | }; | |
| 656 | const is_executable = (stat.mode & 0o111) != 0; | |
| 657 | ||
| 658 | if (is_executable) { | |
| 659 | try binaries.append(allocator, .{ | |
| 660 | .name = try allocator.dupe(u8, bin_entry.name), | |
| 661 | .path = full_path, | |
| 662 | }); | |
| 663 | 657 | } |
| 658 | ||
| 659 | const name = try allocator.dupe(u8, bin_entry.name); | |
| 660 | errdefer allocator.free(name); | |
| 661 | ||
| 662 | try binaries.append(allocator, .{ | |
| 663 | .name = name, | |
| 664 | .path = full_path, | |
| 665 | }); | |
| 664 | 666 | } |
| 665 | 667 | } |
| 666 | 668 | } |
| @@ -669,21 +671,14 @@ fn discoverNpmBinaries( | ||
| 669 | 671 | } |
| 670 | 672 | |
| 671 | 673 | /// Discover binaries in a package directory |
| 672 | /// 1. Checks for npm package structure (lib/node_modules/*/bin/*) | |
| 673 | /// 2. Scans standard bin/ and sbin/ directories | |
| 674 | /// 1. Scans standard bin/ and sbin/ directories | |
| 675 | /// 2. Falls back to npm package structure (lib/node_modules/*/bin/*) | |
| 674 | 676 | /// 3. Falls back to scanning the package root for native executables |
| 675 | 677 | /// (handles packages like zig that ship the binary at root level) |
| 676 | 678 | pub fn discoverBinaries( |
| 677 | 679 | allocator: std.mem.Allocator, |
| 678 | 680 | package_dir: []const u8, |
| 679 | 681 | ) ![]BinaryInfo { |
| 680 | // First, try to find npm binaries (these take precedence) | |
| 681 | const npm_bins = try discoverNpmBinaries(allocator, package_dir); | |
| 682 | if (npm_bins.len > 0) { | |
| 683 | return npm_bins; | |
| 684 | } | |
| 685 | defer allocator.free(npm_bins); | |
| 686 | ||
| 687 | 682 | var binaries = try std.ArrayList(BinaryInfo).initCapacity(allocator, 8); |
| 688 | 683 | errdefer { |
| 689 | 684 | for (binaries.items) |*bin| { |
| @@ -729,6 +724,19 @@ pub fn discoverBinaries( | ||
| 729 | 724 | } |
| 730 | 725 | } |
| 731 | 726 | |
| 727 | // Fallback: a package that ships no bin/ or sbin/ programs of its own may still | |
| 728 | // carry programs in an npm layout. This is consulted second, not first: a recipe's | |
| 729 | // own wrappers in bin/ (npmjs.com replaces bin/npx with a shim) must keep owning | |
| 730 | // their names, and the nested directory also holds non-command files next to them. | |
| 731 | if (binaries.items.len == 0) { | |
| 732 | const npm_bins = try discoverNpmBinaries(allocator, package_dir); | |
| 733 | if (npm_bins.len > 0) { | |
| 734 | binaries.deinit(allocator); | |
| 735 | return npm_bins; | |
| 736 | } | |
| 737 | allocator.free(npm_bins); | |
| 738 | } | |
| 739 | ||
| 732 | 740 | // Fallback: if no binaries found in bin/sbin, scan the package root for native executables. |
| 733 | 741 | // This handles packages (e.g. zig) that ship the binary at the top level instead of bin/. |
| 734 | 742 | // Only pick up files that are executable and look like native binaries (no extension), |
| @@ -1127,6 +1135,96 @@ test "discoverBinaries" { | ||
| 1127 | 1135 | try std.testing.expectEqualStrings("testbin", binaries[0].name); |
| 1128 | 1136 | } |
| 1129 | 1137 | |
| 1138 | test "discoverBinaries activates executable nested npm programs and leaks no rejected candidate" { | |
| 1139 | const allocator = std.testing.allocator; | |
| 1140 | ||
| 1141 | const test_dir = "test_pkg_npm_bin"; | |
| 1142 | io_helper.makePath(test_dir) catch {}; | |
| 1143 | defer io_helper.deleteTree(test_dir) catch {}; | |
| 1144 | ||
| 1145 | const npm_bin_dir = try std.fmt.allocPrint(allocator, "{s}/lib/node_modules/example-cli/bin", .{test_dir}); | |
| 1146 | defer allocator.free(npm_bin_dir); | |
| 1147 | try io_helper.makePath(npm_bin_dir); | |
| 1148 | ||
| 1149 | // An executable program: this is the command that must be activated. | |
| 1150 | { | |
| 1151 | const program = try std.fmt.allocPrint(allocator, "{s}/example", .{npm_bin_dir}); | |
| 1152 | defer allocator.free(program); | |
| 1153 | const file = try io_helper.cwd().createFile(io_helper.io, program, .{ .permissions = .executable_file }); | |
| 1154 | file.close(io_helper.io); | |
| 1155 | } | |
| 1156 | ||
| 1157 | // A non-executable sibling: it must be rejected, and its candidate path freed. | |
| 1158 | { | |
| 1159 | const resource = try std.fmt.allocPrint(allocator, "{s}/example-manifest.json", .{npm_bin_dir}); | |
| 1160 | defer allocator.free(resource); | |
| 1161 | const file = try io_helper.cwd().createFile(io_helper.io, resource, .{}); | |
| 1162 | file.close(io_helper.io); | |
| 1163 | } | |
| 1164 | ||
| 1165 | const binaries = try discoverBinaries(allocator, test_dir); | |
| 1166 | defer { | |
| 1167 | for (binaries) |*bin| { | |
| 1168 | var b = bin.*; | |
| 1169 | b.deinit(allocator); | |
| 1170 | } | |
| 1171 | allocator.free(binaries); | |
| 1172 | } | |
| 1173 | ||
| 1174 | try std.testing.expectEqual(@as(usize, 1), binaries.len); | |
| 1175 | try std.testing.expectEqualStrings("example", binaries[0].name); | |
| 1176 | try std.testing.expect(std.mem.endsWith(u8, binaries[0].path, "lib/node_modules/example-cli/bin/example")); | |
| 1177 | } | |
| 1178 | ||
| 1179 | test "discoverBinaries keeps a package's own bin programs over its nested npm copies" { | |
| 1180 | const allocator = std.testing.allocator; | |
| 1181 | ||
| 1182 | // Mirrors the npmjs.com layout: the recipe's own bin/ wrappers (one of which is a | |
| 1183 | // deliberate shim) alongside the upstream package's nested bin directory, which | |
| 1184 | // also carries non-command files such as npm-cli.js. | |
| 1185 | const test_dir = "test_pkg_npm_shadow"; | |
| 1186 | io_helper.makePath(test_dir) catch {}; | |
| 1187 | defer io_helper.deleteTree(test_dir) catch {}; | |
| 1188 | ||
| 1189 | const own_bin = try std.fmt.allocPrint(allocator, "{s}/bin", .{test_dir}); | |
| 1190 | defer allocator.free(own_bin); | |
| 1191 | try io_helper.makePath(own_bin); | |
| 1192 | ||
| 1193 | const nested_bin = try std.fmt.allocPrint(allocator, "{s}/lib/node_modules/npm/bin", .{test_dir}); | |
| 1194 | defer allocator.free(nested_bin); | |
| 1195 | try io_helper.makePath(nested_bin); | |
| 1196 | ||
| 1197 | const own_programs = [_][]const u8{ "npm", "npx" }; | |
| 1198 | for (own_programs) |name| { | |
| 1199 | const path = try std.fmt.allocPrint(allocator, "{s}/{s}", .{ own_bin, name }); | |
| 1200 | defer allocator.free(path); | |
| 1201 | const file = try io_helper.cwd().createFile(io_helper.io, path, .{ .permissions = .executable_file }); | |
| 1202 | file.close(io_helper.io); | |
| 1203 | } | |
| 1204 | ||
| 1205 | const nested_programs = [_][]const u8{ "npm", "npx", "npm-cli.js" }; | |
| 1206 | for (nested_programs) |name| { | |
| 1207 | const path = try std.fmt.allocPrint(allocator, "{s}/{s}", .{ nested_bin, name }); | |
| 1208 | defer allocator.free(path); | |
| 1209 | const file = try io_helper.cwd().createFile(io_helper.io, path, .{ .permissions = .executable_file }); | |
| 1210 | file.close(io_helper.io); | |
| 1211 | } | |
| 1212 | ||
| 1213 | const binaries = try discoverBinaries(allocator, test_dir); | |
| 1214 | defer { | |
| 1215 | for (binaries) |*bin| { | |
| 1216 | var b = bin.*; | |
| 1217 | b.deinit(allocator); | |
| 1218 | } | |
| 1219 | allocator.free(binaries); | |
| 1220 | } | |
| 1221 | ||
| 1222 | try std.testing.expectEqual(@as(usize, own_programs.len), binaries.len); | |
| 1223 | for (binaries) |bin| { | |
| 1224 | try std.testing.expect(std.mem.indexOf(u8, bin.path, "node_modules") == null); | |
| 1225 | } | |
| 1226 | } | |
| 1227 | ||
| 1130 | 1228 | test "createShimsFromBinConfig normalizes scoped bin object keys" { |
| 1131 | 1229 | const allocator = std.testing.allocator; |
| 1132 | 1230 | const test_dir = "test_scoped_npm_shim"; |