From 2192e74d98574d3dd7f0d15b0bcb3d190d4a257c Mon Sep 17 00:00:00 2001
From: Fabian Meumertzheim <fabian@meumertzhe.im>
Date: Wed, 22 May 2024 12:28:37 -0700
Subject: [PATCH] Fail early on certain invalid labels in module files

The following validation checks were not enforced due to backwards compatibility concerns, but ended up crashing Bazel when invalid labels made it into the lockfile, which is enabled by default. We might as well enable them now:

* Fail if a label passed to `use_extension` is not valid
* Fail if a label passed to the `patches` attribute of an override is not valid or points to a repo other than the main repo

Work towards #21845

Closes #22487.

PiperOrigin-RevId: 636255834
Change-Id: I55dda374cd1716e514c4d78642479b136fd3ad43
---
 .../lib/bazel/bzlmod/ArchiveOverride.java     |  5 +-
 .../bazel/bzlmod/ArchiveRepoSpecBuilder.java  |  3 +-
 .../build/lib/bazel/bzlmod/GitOverride.java   |  5 +-
 .../lib/bazel/bzlmod/GitRepoSpecBuilder.java  |  3 +-
 .../lib/bazel/bzlmod/ModuleFileGlobals.java   | 74 +++++++++++--------
 .../bazel/bzlmod/SingleVersionOverride.java   |  5 +-
 .../bazel/bzlmod/ModuleFileFunctionTest.java  | 41 ++++++++++
 7 files changed, 96 insertions(+), 40 deletions(-)

diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveOverride.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveOverride.java
index cb0cbebc48a581..463abb946fdf15 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveOverride.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveOverride.java
@@ -18,6 +18,7 @@
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule.ResolutionReason;
+import com.google.devtools.build.lib.cmdline.Label;
 
 /** Specifies that a module should be retrieved from an archive. */
 @AutoValue
@@ -25,7 +26,7 @@ public abstract class ArchiveOverride implements NonRegistryOverride {
 
   public static ArchiveOverride create(
       ImmutableList<String> urls,
-      ImmutableList<Object> patches,
+      ImmutableList<Label> patches,
       ImmutableList<String> patchCmds,
       String integrity,
       String stripPrefix,
@@ -38,7 +39,7 @@ public static ArchiveOverride create(
   public abstract ImmutableList<String> getUrls();
 
   /** The labels of patches to apply after extracting the archive. */
-  public abstract ImmutableList<Object> getPatches();
+  public abstract ImmutableList<Label> getPatches();
 
   /** The patch commands to execute after extracting the archive. Should be a list of commands. */
   public abstract ImmutableList<String> getPatchCmds();
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java
index d6ffe2e46c577b..62074385419042 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ArchiveRepoSpecBuilder.java
@@ -18,6 +18,7 @@
 import com.google.common.base.Strings;
 import com.google.common.collect.ImmutableList;
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import net.starlark.java.eval.StarlarkInt;
 
@@ -54,7 +55,7 @@ public ArchiveRepoSpecBuilder setStripPrefix(String stripPrefix) {
   }
 
   @CanIgnoreReturnValue
-  public ArchiveRepoSpecBuilder setPatches(ImmutableList<Object> patches) {
+  public ArchiveRepoSpecBuilder setPatches(ImmutableList<Label> patches) {
     attrBuilder.put("patches", patches);
     return this;
   }
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitOverride.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitOverride.java
index 7256d2cf2ee9bb..29b98488bbfd93 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitOverride.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitOverride.java
@@ -18,6 +18,7 @@
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
 import com.google.devtools.build.lib.bazel.bzlmod.BazelModuleInspectorValue.AugmentedModule.ResolutionReason;
+import com.google.devtools.build.lib.cmdline.Label;
 
 /** Specifies that a module should be retrieved from a Git repository. */
 @AutoValue
@@ -25,7 +26,7 @@ public abstract class GitOverride implements NonRegistryOverride {
   public static GitOverride create(
       String remote,
       String commit,
-      ImmutableList<Object> patches,
+      ImmutableList<Label> patches,
       ImmutableList<String> patchCmds,
       int patchStrip,
       boolean initSubmodules,
@@ -41,7 +42,7 @@ public static GitOverride create(
   public abstract String getCommit();
 
   /** The labels of patches to apply after fetching from Git. */
-  public abstract ImmutableList<Object> getPatches();
+  public abstract ImmutableList<Label> getPatches();
 
   /** The patch commands to execute after fetching from Git. Should be a list of commands. */
   public abstract ImmutableList<String> getPatchCmds();
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitRepoSpecBuilder.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitRepoSpecBuilder.java
index ce8420176e1e95..e0cff55b789446 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitRepoSpecBuilder.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/GitRepoSpecBuilder.java
@@ -16,6 +16,7 @@
 package com.google.devtools.build.lib.bazel.bzlmod;
 
 import com.google.common.collect.ImmutableMap;
+import com.google.devtools.build.lib.cmdline.Label;
 import com.google.errorprone.annotations.CanIgnoreReturnValue;
 import java.util.List;
 
@@ -71,7 +72,7 @@ public GitRepoSpecBuilder setStripPrefix(String stripPrefix) {
   }
 
   @CanIgnoreReturnValue
-  public GitRepoSpecBuilder setPatches(List<Object> patches) {
+  public GitRepoSpecBuilder setPatches(List<Label> patches) {
     return setAttr("patches", patches);
   }
 
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java
index fcf2103bd17dd6..b4b215a18f1db3 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileGlobals.java
@@ -15,7 +15,6 @@
 
 package com.google.devtools.build.lib.bazel.bzlmod;
 
-import static com.google.common.collect.ImmutableList.toImmutableList;
 
 import com.google.common.annotations.VisibleForTesting;
 import com.google.common.collect.ImmutableCollection;
@@ -457,7 +456,8 @@ public ModuleExtensionProxy useExtension(
     return new ModuleExtensionProxy(newUsageBuilder, proxyBuilder);
   }
 
-  private String normalizeLabelString(InterimModule.Builder module, String rawExtensionBzlFile) {
+  private String normalizeLabelString(InterimModule.Builder module, String rawExtensionBzlFile)
+      throws EvalException {
     // Normalize the label by parsing and stringifying it with a repo mapping that preserves the
     // apparent repository name, except that a reference to the main repository via the empty
     // repo name is translated to using the module repo name. This is necessary as
@@ -467,30 +467,27 @@ private String normalizeLabelString(InterimModule.Builder module, String rawExte
     // ownName can't change anymore as calling module() after this results in an error.
     String ownName = module.getRepoName().orElse(module.getName());
     RepositoryName ownRepoName = RepositoryName.createUnvalidated(ownName);
+    ImmutableMap<String, RepositoryName> repoMapping = ImmutableMap.of();
+    if (module.getKey().equals(ModuleKey.ROOT)) {
+      repoMapping = ImmutableMap.of("", ownRepoName);
+    }
+    Label label;
     try {
-      ImmutableMap<String, RepositoryName> repoMapping = ImmutableMap.of();
-      if (module.getKey().equals(ModuleKey.ROOT)) {
-        repoMapping = ImmutableMap.of("", ownRepoName);
-      }
-      Label label =
+      label =
           Label.parseWithPackageContext(
               rawExtensionBzlFile,
               Label.PackageContext.of(
                   PackageIdentifier.create(ownRepoName, PathFragment.EMPTY_FRAGMENT),
                   RepositoryMapping.createAllowingFallback(repoMapping)));
-      // Skip over the leading "@" of the unambiguous form.
-      return label.getUnambiguousCanonicalForm().substring(1);
-    } catch (LabelSyntaxException ignored) {
-      // Preserve backwards compatibility by not failing eagerly, rather keep the invalid label and
-      // let the extension fail when evaluated.
-      return rawExtensionBzlFile;
+    } catch (LabelSyntaxException e) {
+      throw Starlark.errorf("invalid label \"%s\": %s", rawExtensionBzlFile, e.getMessage());
     }
+    // Skip over the leading "@" of the unambiguous form.
+    return label.getUnambiguousCanonicalForm().substring(1);
   }
 
-  /**
-   * Returns a {@link Label} when the given string is a valid label, otherwise the string itself.
-   */
-  private Object parseOverrideLabelAttribute(InterimModule.Builder module, String rawLabel) {
+  private Label convertAndValidatePatchLabel(InterimModule.Builder module, String rawLabel)
+      throws EvalException {
     RepositoryMapping repoMapping =
         RepositoryMapping.create(
             ImmutableMap.<String, RepositoryName>builder()
@@ -498,14 +495,21 @@ private Object parseOverrideLabelAttribute(InterimModule.Builder module, String
                 .put(module.getRepoName().orElse(module.getName()), RepositoryName.MAIN)
                 .buildKeepingLast(),
             RepositoryName.MAIN);
+    Label label;
     try {
-      return Label.parseWithPackageContext(
-          rawLabel, Label.PackageContext.of(PackageIdentifier.EMPTY_PACKAGE_ID, repoMapping));
+      label =
+          Label.parseWithPackageContext(
+              rawLabel, Label.PackageContext.of(PackageIdentifier.EMPTY_PACKAGE_ID, repoMapping));
     } catch (LabelSyntaxException e) {
-      // Preserve backwards compatibility by not failing eagerly, rather keep the invalid label and
-      // let the module repo fail when fetched.
-      return rawLabel;
+      throw Starlark.errorf("invalid label \"%s\" in 'patches': %s", rawLabel, e.getMessage());
     }
+    if (!label.getRepository().isVisible()) {
+      throw Starlark.errorf(
+          "invalid label in 'patches': only patches in the main repository can be applied, not from"
+              + " '@%s'",
+          label.getRepository().getName());
+    }
+    return label;
   }
 
   @StarlarkBuiltin(name = "module_extension_proxy", documented = false)
@@ -798,16 +802,18 @@ public void singleVersionOverride(
     try {
       parsedVersion = Version.parse(version);
     } catch (ParseException e) {
-      throw new EvalException("Invalid version in single_version_override()", e);
+      throw Starlark.errorf("Invalid version in single_version_override(): %s", version);
+    }
+    ImmutableList.Builder<Label> patchesBuilder = ImmutableList.builder();
+    for (String patch : Sequence.cast(patches, String.class, "patches")) {
+      patchesBuilder.add(convertAndValidatePatchLabel(context.getModuleBuilder(), patch));
     }
     context.addOverride(
         moduleName,
         SingleVersionOverride.create(
             parsedVersion,
             registry,
-            Sequence.cast(patches, String.class, "patches").stream()
-                .map(l -> parseOverrideLabelAttribute(context.getModuleBuilder(), l))
-                .collect(toImmutableList()),
+            patchesBuilder.build(),
             Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
             patchStrip.toInt("single_version_override.patch_strip")));
   }
@@ -948,13 +954,15 @@ public void archiveOverride(
         urls instanceof String
             ? ImmutableList.of((String) urls)
             : Sequence.cast(urls, String.class, "urls").getImmutableList();
+    ImmutableList.Builder<Label> patchesBuilder = ImmutableList.builder();
+    for (String patch : Sequence.cast(patches, String.class, "patches")) {
+      patchesBuilder.add(convertAndValidatePatchLabel(context.getModuleBuilder(), patch));
+    }
     context.addOverride(
         moduleName,
         ArchiveOverride.create(
             urlList,
-            Sequence.cast(patches, String.class, "patches").stream()
-                .map(l -> parseOverrideLabelAttribute(context.getModuleBuilder(), l))
-                .collect(toImmutableList()),
+            patchesBuilder.build(),
             Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
             integrity,
             stripPrefix,
@@ -1040,14 +1048,16 @@ public void gitOverride(
     ModuleThreadContext context = ModuleThreadContext.fromOrFail(thread, "git_override()");
     context.setNonModuleCalled();
     validateModuleName(moduleName);
+    ImmutableList.Builder<Label> patchesBuilder = ImmutableList.builder();
+    for (String patch : Sequence.cast(patches, String.class, "patches")) {
+      patchesBuilder.add(convertAndValidatePatchLabel(context.getModuleBuilder(), patch));
+    }
     context.addOverride(
         moduleName,
         GitOverride.create(
             remote,
             commit,
-            Sequence.cast(patches, String.class, "patches").stream()
-                .map(l -> parseOverrideLabelAttribute(context.getModuleBuilder(), l))
-                .collect(toImmutableList()),
+            patchesBuilder.build(),
             Sequence.cast(patchCmds, String.class, "patchCmds").getImmutableList(),
             patchStrip.toInt("git_override.patch_strip"),
             initSubmodules,
diff --git a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleVersionOverride.java b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleVersionOverride.java
index 1e2ceb980e3fd6..02f1caa0848704 100644
--- a/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleVersionOverride.java
+++ b/src/main/java/com/google/devtools/build/lib/bazel/bzlmod/SingleVersionOverride.java
@@ -17,6 +17,7 @@
 
 import com.google.auto.value.AutoValue;
 import com.google.common.collect.ImmutableList;
+import com.google.devtools.build.lib.cmdline.Label;
 
 /**
  * Specifies that the module should:
@@ -33,7 +34,7 @@ public abstract class SingleVersionOverride implements RegistryOverride {
   public static SingleVersionOverride create(
       Version version,
       String registry,
-      ImmutableList<Object> patches,
+      ImmutableList<Label> patches,
       ImmutableList<String> patchCmds,
       int patchStrip) {
     return new AutoValue_SingleVersionOverride(version, registry, patches, patchCmds, patchStrip);
@@ -49,7 +50,7 @@ public static SingleVersionOverride create(
   public abstract String getRegistry();
 
   /** The labels of patches to apply after retrieving per the registry. */
-  public abstract ImmutableList<Object> getPatches();
+  public abstract ImmutableList<Label> getPatches();
 
   /**
    * The patch commands to execute after retrieving per the registry. Should be a list of commands.
diff --git a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java
index 7908509f7e2e56..05cb51caa8632e 100644
--- a/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java
+++ b/src/test/java/com/google/devtools/build/lib/bazel/bzlmod/ModuleFileFunctionTest.java
@@ -1537,4 +1537,45 @@ public void isolatedUsage_notEnabled() throws Exception {
             + "and thus unavailable with the current flags. It may be enabled by setting "
             + "--experimental_isolated_extension_usages");
   }
+
+  @Test
+  public void testInvalidRepoInPatches() throws Exception {
+    scratch.overwriteFile(
+        rootDirectory.getRelative("MODULE.bazel").getPathString(),
+        "module(name='aaa')",
+        "single_version_override(",
+        "    module_name = 'bbb',",
+        "    version = '1.0',",
+        "    patches = ['@unknown_repo//:patch.bzl'],",
+        ")");
+
+    reporter.removeHandler(failFastHandler);
+    EvaluationResult<RootModuleFileValue> result =
+        evaluator.evaluate(
+            ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext);
+    assertThat(result.hasError()).isTrue();
+
+    assertContainsEvent(
+        "Error in single_version_override: invalid label in 'patches': only patches in the main"
+            + " repository can be applied, not from '@unknown_repo'");
+  }
+
+  @Test
+  public void testInvalidUseExtensionLabel() throws Exception {
+    scratch.overwriteFile(
+        rootDirectory.getRelative("MODULE.bazel").getPathString(),
+        "module(name='aaa')",
+        "use_extension('@foo/bar:extensions.bzl', 'my_ext')");
+
+    reporter.removeHandler(failFastHandler);
+    EvaluationResult<RootModuleFileValue> result =
+        evaluator.evaluate(
+            ImmutableList.of(ModuleFileValue.KEY_FOR_ROOT_MODULE), evaluationContext);
+    assertThat(result.hasError()).isTrue();
+
+    assertContainsEvent(
+        "Error in use_extension: invalid label \"@foo/bar:extensions.bzl\": invalid repository"
+            + " name 'foo/bar:extensions.bzl': repo names may contain only A-Z, a-z, 0-9, '-',"
+            + " '_', '.' and '~' and must not start with '~'");
+  }
 }