feat: add strict_visibility to npm_install / yarn_install rules (#2193)

With strict_visibility enabled (currently defaults false), only dependencies within the given `package.json` file are given public visibility.

All transitive dependencies are given limited visibility, enforcing that all direct dependencies are listed in the `package.json` file.

closes #2110

Author: mattem

examples/from_source/WORKSPACE

@@ -40,5 +40,6 @@ node_repositories(
 yarn_install(
     name = "npm",
     package_json = "//:package.json",
+    strict_visibility = True,
     yarn_lock = "//:yarn.lock",
 )

internal/npm_install/BUILD.bazel

@@ -7,7 +7,7 @@ load("//packages/typescript:checked_in_ts_project.bzl", "checked_in_ts_project")
 
 # Using checked in ts library like the linker
 # To update index.js run:
-#   bazel run //internal/npm_install:compile_generate_build_file_check_compiled.accept
+#   bazel run //internal/npm_install:compile_generate_build_file_check_compiled.update
 checked_in_ts_project(
     name = "compile_generate_build_file",
     src = "generate_build_file.ts",

internal/npm_install/generate_build_file.ts

@@ -48,20 +48,26 @@ function log_verbose(...m: any[]) {
   if (!!process.env['VERBOSE_LOGS']) console.error('[generate_build_file.ts]', ...m);
 }
 
-const BUILD_FILE_HEADER = `# Generated file from yarn_install/npm_install rule.
-# See rules_nodejs/internal/npm_install/generate_build_file.ts
-
-# All rules in other repositories can use these targets
-package(default_visibility = ["//visibility:public"])
-
-`
-
 const args = process.argv.slice(2);
 const WORKSPACE = args[0];
 const RULE_TYPE = args[1];
-const LOCK_FILE_PATH = args[2];
-const INCLUDED_FILES = args[3] ? args[3].split(',') : [];
-const BAZEL_VERSION = args[4];
+const PKG_JSON_FILE_PATH = args[2];
+const LOCK_FILE_PATH = args[3];
+const STRICT_VISIBILITY = args[4]?.toLowerCase() === 'true';
+const INCLUDED_FILES = args[5] ? args[5].split(',') : [];
+const BAZEL_VERSION = args[6];
+
+const PUBLIC_VISIBILITY = '//visibility:public';
+const LIMITED_VISIBILITY = `@${WORKSPACE}//:__subpackages__`;
+
+function generateBuildFileHeader(visibility = PUBLIC_VISIBILITY): string {
+  return `# Generated file from ${RULE_TYPE} rule.
+# See rules_nodejs/internal/npm_install/generate_build_file.ts
+
+package(default_visibility = ["${visibility}"])
+
+`;
+}
 
 if (require.main === module) {
   main();
@@ -91,8 +97,11 @@ function writeFileSync(p: string, content: string) {
  * Main entrypoint.
  */
 export function main() {
+  // get a set of all the direct dependencies for visibility
+  const deps = getDirectDependencySet(PKG_JSON_FILE_PATH);
+
   // find all packages (including packages in nested node_modules)
-  const pkgs = findPackages();
+  const pkgs = findPackages('node_modules', deps);
 
   // flatten dependencies
   flattenDependencies(pkgs);
@@ -157,7 +166,7 @@ function generateRootBuildFile(pkgs: Dep[]) {
 `;
                })});
 
-  let buildFile = BUILD_FILE_HEADER + `load("@build_bazel_rules_nodejs//:index.bzl", "js_library")
+  let buildFile = generateBuildFileHeader() + `load("@build_bazel_rules_nodejs//:index.bzl", "js_library")
 
 exports_files([
 ${exportsStarlark}])
@@ -198,12 +207,18 @@ function generatePackageBuildFiles(pkg: Dep) {
     buildFilePath = 'BUILD.bazel'
   }
 
+  // if the dependency doesn't appear in the given package.json file, and the 'strict_visibility' flag is set
+  // on the npm_install / yarn_install rule, then set the visibility to be limited internally to the @repo workspace
+  // if the dependency is listed, set it as public
+  // if the flag is false, then always set public visibility
+  const visibility = !pkg._directDependency && STRICT_VISIBILITY ? LIMITED_VISIBILITY : PUBLIC_VISIBILITY;
+
   // If the package didn't ship a bin/BUILD file, generate one.
   if (!pkg._files.includes('bin/BUILD.bazel') && !pkg._files.includes('bin/BUILD')) {
     const binBuildFile = printPackageBin(pkg);
     if (binBuildFile.length) {
       writeFileSync(
-          path.posix.join(pkg._dir, 'bin', 'BUILD.bazel'), BUILD_FILE_HEADER + binBuildFile);
+          path.posix.join(pkg._dir, 'bin', 'BUILD.bazel'), generateBuildFileHeader(visibility) + binBuildFile);
     }
   }
 
@@ -241,7 +256,7 @@ exports_files(["index.bzl"])
     }
   }
 
-  writeFileSync(path.posix.join(pkg._dir, buildFilePath), BUILD_FILE_HEADER + buildFile);
+  writeFileSync(path.posix.join(pkg._dir, buildFilePath), generateBuildFileHeader(visibility) + buildFile);
 }
 
 /**
@@ -389,7 +404,7 @@ You can suppress this message by passing "suppress_warning = True" to install_ba
  * Generate build files for a scope.
  */
 function generateScopeBuildFiles(scope: string, pkgs: Dep[]) {
-  const buildFile = BUILD_FILE_HEADER + printScope(scope, pkgs);
+  const buildFile = generateBuildFileHeader() + printScope(scope, pkgs);
   writeFileSync(path.posix.join(scope, 'BUILD.bazel'), buildFile);
 }
 
@@ -407,6 +422,13 @@ function isDirectory(p: string) {
   return fs.existsSync(p) && fs.statSync(p).isDirectory();
 }
 
+/**
+ * Strips the byte order mark from a string if present
+ */
+function stripBom(s: string) {
+  return s.charCodeAt(0) === 0xFEFF ? s.slice(1) : s;
+}
+
 /**
  * Returns an array of all the files under a directory as relative
  * paths to the directory.
@@ -468,10 +490,24 @@ function hasRootBuildFile(pkg: Dep, rootPath: string) {
   return false;
 }
 
+/**
+ * Returns a set of the root package.json files direct dependencies
+ */
+export function getDirectDependencySet(pkgJsonPath: string): Set<string> {
+  const pkgJson = JSON.parse(
+    stripBom(fs.readFileSync(pkgJsonPath, {encoding: 'utf8'}))
+  );
+
+  const dependencies: string[] = Object.keys(pkgJson.dependencies || {});
+  const devDependencies: string[] = Object.keys(pkgJson.devDependencies || {});
+
+  return new Set([...dependencies, ...devDependencies]);
+}
+
 /**
  * Finds and returns an array of all packages under a given path.
  */
-function findPackages(p = 'node_modules') {
+function findPackages(p: string, dependencies: Set<string>) {
   if (!isDirectory(p)) {
     return [];
   }
@@ -490,13 +526,13 @@ function findPackages(p = 'node_modules') {
                        .filter(f => isDirectory(f));
 
   packages.forEach(f => {
-    pkgs.push(parsePackage(f), ...findPackages(path.posix.join(f, 'node_modules')));
+    pkgs.push(parsePackage(f, dependencies), ...findPackages(path.posix.join(f, 'node_modules'), dependencies));
   });
 
   const scopes = listing.filter(f => f.startsWith('@'))
                      .map(f => path.posix.join(p, f))
                      .filter(f => isDirectory(f));
-  scopes.forEach(f => pkgs.push(...findPackages(f)));
+  scopes.forEach(f => pkgs.push(...findPackages(f, dependencies)));
 
   return pkgs;
 }
@@ -525,10 +561,9 @@ function findScopes() {
  * package json and return it as an object along with
  * some additional internal attributes prefixed with '_'.
  */
-export function parsePackage(p: string): Dep {
+export function parsePackage(p: string, dependencies: Set<string> = new Set()): Dep {
   // Parse the package.json file of this package
   const packageJson = path.posix.join(p, 'package.json');
-  const stripBom = (s: string) => s.charCodeAt(0) === 0xFEFF ? s.slice(1) : s;
   const pkg = isFile(packageJson) ?
       JSON.parse(stripBom(fs.readFileSync(packageJson, {encoding: 'utf8'}))) :
       {version: '0.0.0'};
@@ -559,6 +594,10 @@ export function parsePackage(p: string): Dep {
   // which is later filled with the flattened dependency list
   pkg._dependencies = [];
 
+  // set if this is a direct dependency of the root package.json file
+  // which is later used to determine the generated rules visibility
+  pkg._directDependency = dependencies.has(pkg._moduleName);
+
   return pkg;
 }
 
@@ -1131,6 +1170,7 @@ type Dep = {
   _dependencies: Dep[],
   _files: string[],
   _runfiles: string[],
+  _directDependency: boolean,
   [k: string]: any
 }
 

internal/npm_install/index.js

@@ -1,4 +1,5 @@
 /* THIS FILE GENERATED FROM .ts; see BUILD.bazel */ /* clang-format off */'use strict';
+var _a;
 Object.defineProperty(exports, "__esModule", { value: true });
 const fs = require("fs");
 const path = require("path");
@@ -7,19 +8,24 @@ function log_verbose(...m) {
     if (!!process.env['VERBOSE_LOGS'])
         console.error('[generate_build_file.ts]', ...m);
 }
-const BUILD_FILE_HEADER = `# Generated file from yarn_install/npm_install rule.
+const args = process.argv.slice(2);
+const WORKSPACE = args[0];
+const RULE_TYPE = args[1];
+const PKG_JSON_FILE_PATH = args[2];
+const LOCK_FILE_PATH = args[3];
+const STRICT_VISIBILITY = ((_a = args[4]) === null || _a === void 0 ? void 0 : _a.toLowerCase()) === 'true';
+const INCLUDED_FILES = args[5] ? args[5].split(',') : [];
+const BAZEL_VERSION = args[6];
+const PUBLIC_VISIBILITY = '//visibility:public';
+const LIMITED_VISIBILITY = `@${WORKSPACE}//:__subpackages__`;
+function generateBuildFileHeader(visibility = PUBLIC_VISIBILITY) {
+    return `# Generated file from ${RULE_TYPE} rule.
 # See rules_nodejs/internal/npm_install/generate_build_file.ts
 
-# All rules in other repositories can use these targets
-package(default_visibility = ["//visibility:public"])
+package(default_visibility = ["${visibility}"])
 
 `;
-const args = process.argv.slice(2);
-const WORKSPACE = args[0];
-const RULE_TYPE = args[1];
-const LOCK_FILE_PATH = args[2];
-const INCLUDED_FILES = args[3] ? args[3].split(',') : [];
-const BAZEL_VERSION = args[4];
+}
 if (require.main === module) {
     main();
 }
@@ -34,7 +40,8 @@ function writeFileSync(p, content) {
     fs.writeFileSync(p, content);
 }
 function main() {
-    const pkgs = findPackages();
+    const deps = getDirectDependencySet(PKG_JSON_FILE_PATH);
+    const pkgs = findPackages('node_modules', deps);
     flattenDependencies(pkgs);
     generateBazelWorkspaces(pkgs);
     generateBuildFiles(pkgs);
@@ -79,7 +86,7 @@ function generateRootBuildFile(pkgs) {
 `;
         });
     });
-    let buildFile = BUILD_FILE_HEADER + `load("@build_bazel_rules_nodejs//:index.bzl", "js_library")
+    let buildFile = generateBuildFileHeader() + `load("@build_bazel_rules_nodejs//:index.bzl", "js_library")
 
 exports_files([
 ${exportsStarlark}])
@@ -114,10 +121,11 @@ function generatePackageBuildFiles(pkg) {
     else {
         buildFilePath = 'BUILD.bazel';
     }
+    const visibility = !pkg._directDependency && STRICT_VISIBILITY ? LIMITED_VISIBILITY : PUBLIC_VISIBILITY;
     if (!pkg._files.includes('bin/BUILD.bazel') && !pkg._files.includes('bin/BUILD')) {
         const binBuildFile = printPackageBin(pkg);
         if (binBuildFile.length) {
-            writeFileSync(path.posix.join(pkg._dir, 'bin', 'BUILD.bazel'), BUILD_FILE_HEADER + binBuildFile);
+            writeFileSync(path.posix.join(pkg._dir, 'bin', 'BUILD.bazel'), generateBuildFileHeader(visibility) + binBuildFile);
         }
     }
     if (pkg._files.includes('index.bzl')) {
@@ -146,7 +154,7 @@ exports_files(["index.bzl"])
 `;
         }
     }
-    writeFileSync(path.posix.join(pkg._dir, buildFilePath), BUILD_FILE_HEADER + buildFile);
+    writeFileSync(path.posix.join(pkg._dir, buildFilePath), generateBuildFileHeader(visibility) + buildFile);
 }
 function generateBazelWorkspaces(pkgs) {
     const workspaces = {};
@@ -247,7 +255,7 @@ You can suppress this message by passing "suppress_warning = True" to install_ba
     writeFileSync('install_bazel_dependencies.bzl', bzlFile);
 }
 function generateScopeBuildFiles(scope, pkgs) {
-    const buildFile = BUILD_FILE_HEADER + printScope(scope, pkgs);
+    const buildFile = generateBuildFileHeader() + printScope(scope, pkgs);
     writeFileSync(path.posix.join(scope, 'BUILD.bazel'), buildFile);
 }
 function isFile(p) {
@@ -256,6 +264,9 @@ function isFile(p) {
 function isDirectory(p) {
     return fs.existsSync(p) && fs.statSync(p).isDirectory();
 }
+function stripBom(s) {
+    return s.charCodeAt(0) === 0xFEFF ? s.slice(1) : s;
+}
 function listFiles(rootDir, subDir = '') {
     const dir = path.posix.join(rootDir, subDir);
     if (!isDirectory(dir)) {
@@ -294,7 +305,14 @@ function hasRootBuildFile(pkg, rootPath) {
     }
     return false;
 }
-function findPackages(p = 'node_modules') {
+function getDirectDependencySet(pkgJsonPath) {
+    const pkgJson = JSON.parse(stripBom(fs.readFileSync(pkgJsonPath, { encoding: 'utf8' })));
+    const dependencies = Object.keys(pkgJson.dependencies || {});
+    const devDependencies = Object.keys(pkgJson.devDependencies || {});
+    return new Set([...dependencies, ...devDependencies]);
+}
+exports.getDirectDependencySet = getDirectDependencySet;
+function findPackages(p, dependencies) {
     if (!isDirectory(p)) {
         return [];
     }
@@ -306,12 +324,12 @@ function findPackages(p = 'node_modules') {
         .map(f => path.posix.join(p, f))
         .filter(f => isDirectory(f));
     packages.forEach(f => {
-        pkgs.push(parsePackage(f), ...findPackages(path.posix.join(f, 'node_modules')));
+        pkgs.push(parsePackage(f, dependencies), ...findPackages(path.posix.join(f, 'node_modules'), dependencies));
     });
     const scopes = listing.filter(f => f.startsWith('@'))
         .map(f => path.posix.join(p, f))
         .filter(f => isDirectory(f));
-    scopes.forEach(f => pkgs.push(...findPackages(f)));
+    scopes.forEach(f => pkgs.push(...findPackages(f, dependencies)));
     return pkgs;
 }
 function findScopes() {
@@ -326,9 +344,8 @@ function findScopes() {
         .map(f => f.replace(/^node_modules\//, ''));
     return scopes;
 }
-function parsePackage(p) {
+function parsePackage(p, dependencies = new Set()) {
     const packageJson = path.posix.join(p, 'package.json');
-    const stripBom = (s) => s.charCodeAt(0) === 0xFEFF ? s.slice(1) : s;
     const pkg = isFile(packageJson) ?
         JSON.parse(stripBom(fs.readFileSync(packageJson, { encoding: 'utf8' }))) :
         { version: '0.0.0' };
@@ -339,6 +356,7 @@ function parsePackage(p) {
     pkg._files = listFiles(p);
     pkg._runfiles = pkg._files.filter((f) => !/[^\x21-\x7E]/.test(f));
     pkg._dependencies = [];
+    pkg._directDependency = dependencies.has(pkg._moduleName);
     return pkg;
 }
 exports.parsePackage = parsePackage;

internal/npm_install/npm_install.bzl

@@ -83,6 +83,17 @@ fine grained npm dependencies.
         default = True,
         doc = "If stdout and stderr should be printed to the terminal.",
     ),
+    "strict_visibility": attr.bool(
+        default = False,
+        doc = """Turn on stricter visibility for generated BUILD.bazel files
+
+When enabled, only dependencies within the given `package.json` file are given public visibility.
+All transitive dependencies are given limited visibility, enforcing that all direct dependencies are
+listed in the `package.json` file.
+
+Currently the default is set `False`, but will likely be flipped `True` in rules_nodejs 3.0.0
+""",
+    ),
     "symlink_node_modules": attr.bool(
         doc = """Turn symlinking of node_modules on
 
@@ -115,7 +126,9 @@ def _create_build_files(repository_ctx, rule_type, node, lock_file):
         "index.js",
         repository_ctx.attr.name,
         rule_type,
+        repository_ctx.path(repository_ctx.attr.package_json),
         repository_ctx.path(lock_file),
+        str(repository_ctx.attr.strict_visibility),
         ",".join(repository_ctx.attr.included_files),
         native.bazel_version,
     ])

internal/npm_install/test/BUILD.bazel

@@ -11,6 +11,7 @@ jasmine_node_test(
     name = "test",
     srcs = ["generate_build_file.spec.js"],
     data = [
+        "package.spec.json",
         ":check.js",
         ":goldens",
         "//internal/npm_install:compile_generate_build_file",