fix(builtin): runfile resolution incorrect if entry starts similarly

devversion · alexeagle · commit 3be2902c3473 · 2021-07-02T09:15:21.000-07:00
Given the following runfile manifest:

```
src/lib/README.md /c/users/paul/projects/test/src/lib/README.md
src/lib/README.md.update &lt;..&gt;/bazel-out/&lt;..&gt;
```

If a developer intends to resolve the `README.md` file using
the `@bazel/runfiles` package, or the patched module resolution,
they will end up with a wrong path (at least on Windows where
runfiles are not symlinked).

This happens because the `Runfiles.resolve` method continues
looking for entries that start similar (even if we have a full-match).
This logic helps with finding directories, but breaks resolution
with exact matches. This commit also corrects the method name
because technically this method is used for more than resolution
of directories.

BREAKING CHANGE: The `@bazel/runfiles` `lookupDirectory` method has been
removed. Use the `resolve` method instead
diff --git a/internal/linker/index.js b/internal/linker/index.js
@@ -145,7 +145,7 @@ function resolveWorkspaceNodeModules(workspace, startCwd, isExecroot, execroot,
         if (!execroot) {
             return path.resolve(`${startCwd}/../${targetManifestPath}`);
         }
-        const fromManifest = runfiles.lookupDirectory(targetManifestPath);
+        const fromManifest = runfiles.resolve(targetManifestPath);
         if (fromManifest) {
             return fromManifest;
         }
diff --git a/internal/linker/link_node_modules.ts b/internal/linker/link_node_modules.ts
@@ -189,7 +189,7 @@ async function resolveWorkspaceNodeModules(
   // If we got a runfilesManifest map, look through it for a resolution
   // This will happen if we are running a binary that had some npm packages
   // "statically linked" into its runfiles
-  const fromManifest = runfiles.lookupDirectory(targetManifestPath);
+  const fromManifest = runfiles.resolve(targetManifestPath);
   if (fromManifest) {
     return fromManifest;
   } else {
diff --git a/internal/runfiles/index.js b/internal/runfiles/index.js
@@ -75,21 +75,28 @@ class Runfiles {
             this.package = target.split(':')[0].replace(/^\/\//, '');
         }
     }
-    lookupDirectory(dir) {
+    /** Resolves the given path from the runfile manifest. */
+    _resolveFromManifest(searchPath) {
         if (!this.manifest)
             return undefined;
         let result;
         for (const [k, v] of this.manifest) {
             // Account for Bazel --legacy_external_runfiles
             // which pollutes the workspace with 'my_wksp/external/...'
-            if (k.startsWith(`${dir}/external`))
+            if (k.startsWith(`${searchPath}/external`))
                 continue;
-            // Entry looks like
-            // k: npm/node_modules/semver/LICENSE
-            // v: /path/to/external/npm/node_modules/semver/LICENSE
-            // calculate l = length(`/semver/LICENSE`)
-            if (k.startsWith(dir)) {
-                const l = k.length - dir.length;
+            // If the manifest entry fully matches, return the value path without
+            // considering other manifest entries. We already have an exact match.
+            if (k === searchPath) {
+                return v;
+            }
+            // Consider a case where `npm/node_modules` is resolved, and we have the following
+            // manifest: `npm/node_modules/semver/LICENSE /path/to/external/npm/node_modules/semver/LICENSE`
+            // To resolve the directory, we look for entries that either fully match, or refer to contents
+            // within the directory we are looking for. We can then subtract the child path to resolve the
+            // directory. e.g. in the case above we subtract `length(`/semver/LICENSE`)` from the entry value.
+            if (k.startsWith(`${searchPath}/`)) {
+                const l = k.length - searchPath.length;
                 const maybe = v.substring(0, v.length - l);
                 if (maybe.match(paths.BAZEL_OUT_REGEX)) {
                     return maybe;
@@ -166,7 +173,7 @@ class Runfiles {
     /** Helper for resolving a given module recursively in the runfiles. */
     _resolve(moduleBase, moduleTail) {
         if (this.manifest) {
-            const result = this.lookupDirectory(moduleBase);
+            const result = this._resolveFromManifest(moduleBase);
             if (result) {
                 if (moduleTail) {
                     const maybe = path__default['default'].join(result, moduleTail || '');
diff --git a/packages/runfiles/BUILD.bazel b/packages/runfiles/BUILD.bazel
@@ -14,7 +14,10 @@ ts_project(
 js_library(
     name = "bazel_runfiles",
     package_name = "@bazel/runfiles",
-    visibility = ["//internal/linker:__subpackages__"],
+    visibility = [
+        "//internal/linker:__subpackages__",
+        "//packages/runfiles/test:__pkg__",
+    ],
     deps = [":runfiles_lib"],
 )
 
diff --git a/packages/runfiles/runfiles.ts b/packages/runfiles/runfiles.ts
@@ -55,21 +55,29 @@ export class Runfiles {
     }
   }
 
-  lookupDirectory(dir: string): string|undefined {
+  /** Resolves the given path from the runfile manifest. */
+  private _resolveFromManifest(searchPath: string): string|undefined {
     if (!this.manifest) return undefined;
 
     let result: string|undefined;
     for (const [k, v] of this.manifest) {
       // Account for Bazel --legacy_external_runfiles
       // which pollutes the workspace with 'my_wksp/external/...'
-      if (k.startsWith(`${dir}/external`)) continue;
+      if (k.startsWith(`${searchPath}/external`)) continue;
 
-      // Entry looks like
-      // k: npm/node_modules/semver/LICENSE
-      // v: /path/to/external/npm/node_modules/semver/LICENSE
-      // calculate l = length(`/semver/LICENSE`)
-      if (k.startsWith(dir)) {
-        const l = k.length - dir.length;
+      // If the manifest entry fully matches, return the value path without
+      // considering other manifest entries. We already have an exact match.
+      if (k === searchPath) {
+        return v;
+      }
+
+      // Consider a case where `npm/node_modules` is resolved, and we have the following
+      // manifest: `npm/node_modules/semver/LICENSE /path/to/external/npm/node_modules/semver/LICENSE`
+      // To resolve the directory, we look for entries that either fully match, or refer to contents
+      // within the directory we are looking for. We can then subtract the child path to resolve the
+      // directory. e.g. in the case above we subtract `length(`/semver/LICENSE`)` from the entry value.
+      if (k.startsWith(`${searchPath}/`)) {
+        const l = k.length - searchPath.length;
         const maybe = v.substring(0, v.length - l);
         if (maybe.match(BAZEL_OUT_REGEX)) {
           return maybe;
@@ -156,7 +164,7 @@ export class Runfiles {
   /** Helper for resolving a given module recursively in the runfiles. */
   private _resolve(moduleBase: string, moduleTail: string|undefined): string|undefined {
     if (this.manifest) {
-      const result = this.lookupDirectory(moduleBase);
+      const result = this._resolveFromManifest(moduleBase);
       if (result) {
         if (moduleTail) {
           const maybe = path.join(result, moduleTail || '');
diff --git a/packages/runfiles/test/BUILD.bazel b/packages/runfiles/test/BUILD.bazel
@@ -0,0 +1,20 @@
+load("//packages/jasmine:index.bzl", "jasmine_node_test")
+
+jasmine_node_test(
+    name = "test",
+    srcs = ["runfile_resolution.spec.js"],
+    data = [
+        "test_fixture.md",
+        ":test_fixture.md.generated_file_suffix",
+        "//packages/runfiles:bazel_runfiles",
+    ],
+)
+
+# Path of file must start similar to `test_fixture.md` in order to regression-test a
+# scenario where the runfile resolution would accidentally resolve the path to
+# `test_fixture.md` through a runfile manifest entry that starts similarly.
+genrule(
+    name = "test_fixture.md.generated_file_suffix",
+    outs = ["test_fixture.md.generated_file_suffix"],
+    cmd = """echo "Generated" > $@""",
+)
diff --git a/packages/runfiles/test/runfile_resolution.spec.js b/packages/runfiles/test/runfile_resolution.spec.js
@@ -0,0 +1,21 @@
+const {join} = require('path')
+const {runfiles} = require('@bazel/runfiles');
+
+describe('runfile resolution', () => {
+
+  it('should properly resolve the "test_fixture.md" file', () => {
+    const testFixturePath = runfiles.resolve('build_bazel_rules_nodejs/packages/runfiles/test/test_fixture.md');
+    const expectedPath = join(__dirname, 'test_fixture.md');
+
+    expect(normalizePath(testFixturePath)).toEqual(normalizePath(expectedPath),
+            'Expected the test fixture to be resolved next to the spec source file.');
+  });
+});
+
+/**
+ * Normalizes the delimiters within the specified path. This is useful for test assertions
+ * where paths might be computed using different path delimiters.
+ */
+function normalizePath(value) {
+  return value.replace(/\\/g, '/');
+}
diff --git a/packages/runfiles/test/test_fixture.md b/packages/runfiles/test/test_fixture.md
@@ -0,0 +1 @@
+This is a file part of the sources

Original file line number	Diff line number	Diff line change
`@@ -145,7 +145,7 @@ function resolveWorkspaceNodeModules(workspace, startCwd, isExecroot, execroot,`
`145`	`145`	`if (!execroot) {`
`146`	`146`	return path.resolve(`${startCwd}/../${targetManifestPath}`);
`147`	`147`	`}`
`148`		`- const fromManifest = runfiles.lookupDirectory(targetManifestPath);`
	`148`	`+ const fromManifest = runfiles.resolve(targetManifestPath);`
`149`	`149`	`if (fromManifest) {`
`150`	`150`	`return fromManifest;`
`151`	`151`	`}`