From 18c6e80089c228226e6da127ab6c3c77b6aaf7b6 Mon Sep 17 00:00:00 2001 From: Matt Mackay Date: Thu, 17 Sep 2020 11:06:07 -0400 Subject: [PATCH] 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 --- examples/from_source/WORKSPACE | 1 + internal/npm_install/BUILD.bazel | 2 +- internal/npm_install/generate_build_file.ts | 82 ++++++++++++++----- internal/npm_install/index.js | 56 ++++++++----- internal/npm_install/npm_install.bzl | 13 +++ internal/npm_install/test/BUILD.bazel | 1 + .../test/generate_build_file.spec.js | 17 +++- internal/npm_install/test/package.spec.json | 10 +++ 8 files changed, 139 insertions(+), 43 deletions(-) create mode 100644 internal/npm_install/test/package.spec.json diff --git a/examples/from_source/WORKSPACE b/examples/from_source/WORKSPACE index 3b4501d7ac..4479b84d72 100644 --- a/examples/from_source/WORKSPACE +++ b/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", ) diff --git a/internal/npm_install/BUILD.bazel b/internal/npm_install/BUILD.bazel index 75e04f01b5..f138dc7c74 100644 --- a/internal/npm_install/BUILD.bazel +++ b/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", diff --git a/internal/npm_install/generate_build_file.ts b/internal/npm_install/generate_build_file.ts index 18a0234c85..8dd0e5080f 100644 --- a/internal/npm_install/generate_build_file.ts +++ b/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 { + 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) { 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 = 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 } diff --git a/internal/npm_install/index.js b/internal/npm_install/index.js index b68c5df0c1..40c0bd121f 100644 --- a/internal/npm_install/index.js +++ b/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; diff --git a/internal/npm_install/npm_install.bzl b/internal/npm_install/npm_install.bzl index 7bbe12972c..b2ae33529e 100644 --- a/internal/npm_install/npm_install.bzl +++ b/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, ]) diff --git a/internal/npm_install/test/BUILD.bazel b/internal/npm_install/test/BUILD.bazel index 32b5c34e68..286295f9f9 100644 --- a/internal/npm_install/test/BUILD.bazel +++ b/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", diff --git a/internal/npm_install/test/generate_build_file.spec.js b/internal/npm_install/test/generate_build_file.spec.js index 67cb6d329d..f67fa9823f 100644 --- a/internal/npm_install/test/generate_build_file.spec.js +++ b/internal/npm_install/test/generate_build_file.spec.js @@ -1,7 +1,7 @@ const fs = require('fs'); const path = require('path'); const {check, files} = require('./check'); -const {parsePackage, printPackageBin, printIndexBzl} = require('../generate_build_file'); +const {parsePackage, printPackageBin, printIndexBzl, getDirectDependencySet} = require('../generate_build_file'); describe('build file generator', () => { describe('integration test', () => { @@ -21,7 +21,7 @@ describe('build file generator', () => { }); describe('should exclude nodejs_binary rules when', () => { - const pkg = {_name: 'some_name', _dir: 'some_dir', _dependencies: [], _files: []}; + const pkg = {_name: 'some_name', _dir: 'some_dir', _dependencies: [], _files: [], _directDependency: true}; it('no bin entry is provided', () => { expect(printPackageBin({...pkg, _files: []})).not.toContain('nodejs_binary('); @@ -94,4 +94,17 @@ describe('build file generator', () => { expect(bzl).toContain('def http_server('); }); }); + + describe('getDirectDependencySet', () => { + it('returns a set of all dependencies in a package.json file', () => { + const runfiles = require(process.env.BAZEL_NODE_RUNFILES_HELPER); + const relPath = runfiles.resolvePackageRelative('package.spec.json'); + const deps = getDirectDependencySet(relPath); + + expect(deps.has('@angular/core')).toBeTruthy(); + expect(deps.has('@angular/common')).toBeTruthy(); + expect(deps.has('zone.js')).toBeTruthy(); + expect(deps.size).toBe(3); + }); + }); }); diff --git a/internal/npm_install/test/package.spec.json b/internal/npm_install/test/package.spec.json new file mode 100644 index 0000000000..3486b4ea4e --- /dev/null +++ b/internal/npm_install/test/package.spec.json @@ -0,0 +1,10 @@ +{ + "name": "test_package", + "dependencies": { + "@angular/core": "9.1.0" + }, + "devDependencies": { + "@angular/common": "9.1.0", + "zone.js": "0.8.29" + } +}