Report errors parsing rewriter config file

Before this change, an invalid config file results in a silent server
crash.

Closes #12989.

PiperOrigin-RevId: 359251417

Author: illicitonion

src/main/java/com/google/devtools/build/lib/bazel/BazelRepositoryModule.java

@@ -36,6 +36,7 @@
 import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
 import com.google.devtools.build.lib.bazel.repository.downloader.HttpDownloader;
 import com.google.devtools.build.lib.bazel.repository.downloader.UrlRewriter;
+import com.google.devtools.build.lib.bazel.repository.downloader.UrlRewriterParseException;
 import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryFunction;
 import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule;
 import com.google.devtools.build.lib.bazel.rules.android.AndroidNdkRepositoryFunction;
@@ -225,7 +226,7 @@ public void initializeRuleClasses(ConfiguredRuleClassProvider.Builder builder) {
   }
 
   @Override
-  public void beforeCommand(CommandEnvironment env) {
+  public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
     clientEnvironmentSupplier.set(env.getRepoEnv());
     PackageOptions pkgOptions = env.getOptions().getOptions(PackageOptions.class);
     isFetch.set(pkgOptions != null && pkgOptions.fetch);
@@ -284,10 +285,20 @@ public void beforeCommand(CommandEnvironment env) {
         }
       }
 
-      UrlRewriter rewriter =
-          UrlRewriter.getDownloaderUrlRewriter(
-              repoOptions == null ? null : repoOptions.downloaderConfig, env.getReporter());
-      downloadManager.setUrlRewriter(rewriter);
+      try {
+        UrlRewriter rewriter =
+            UrlRewriter.getDownloaderUrlRewriter(
+                repoOptions == null ? null : repoOptions.downloaderConfig, env.getReporter());
+        downloadManager.setUrlRewriter(rewriter);
+      } catch (UrlRewriterParseException e) {
+        // It's important that the build stops ASAP, because this config file may be required for
+        // security purposes, and the build must not proceed ignoring it.
+        throw new AbruptExitException(
+            detailedExitCode(
+                String.format(
+                    "Failed to parse downloader config at %s: %s", e.getLocation(), e.getMessage()),
+                Code.BAD_DOWNLOADER_CONFIG));
+      }
 
       if (repoOptions.experimentalDistdir != null) {
         downloadManager.setDistdir(

src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/BUILD

@@ -22,6 +22,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/vfs",
         "//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
+        "//src/main/java/net/starlark/java/syntax",
         "//third_party:flogger",
         "//third_party:guava",
         "//third_party:jsr305",

src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriter.java

@@ -57,29 +57,31 @@ public class UrlRewriter {
   private final Consumer<String> log;
 
   @VisibleForTesting
-  UrlRewriter(Consumer<String> log, Reader reader) {
+  UrlRewriter(Consumer<String> log, String filePathForErrorReporting, Reader reader)
+      throws UrlRewriterParseException {
     this.log = Preconditions.checkNotNull(log);
     Preconditions.checkNotNull(reader, "UrlRewriterConfig source must be set");
-    this.config = new UrlRewriterConfig(reader);
+    this.config = new UrlRewriterConfig(filePathForErrorReporting, reader);
 
     this.rewriter = this::rewrite;
   }
 
   /**
    * Obtain a new {@code UrlRewriter} configured with the specified config file.
    *
-   * @param downloaderConfig Path to the config file to use. May be null.
+   * @param configPath Path to the config file to use. May be null.
    * @param reporter Used for logging when URLs are rewritten.
    */
-  public static UrlRewriter getDownloaderUrlRewriter(String downloaderConfig, Reporter reporter) {
+  public static UrlRewriter getDownloaderUrlRewriter(String configPath, Reporter reporter)
+      throws UrlRewriterParseException {
     Consumer<String> log = str -> reporter.handle(Event.info(str));
 
-    if (Strings.isNullOrEmpty(downloaderConfig)) {
-      return new UrlRewriter(log, new StringReader(""));
+    if (Strings.isNullOrEmpty(configPath)) {
+      return new UrlRewriter(log, "", new StringReader(""));
     }
 
-    try (BufferedReader reader = Files.newBufferedReader(Paths.get(downloaderConfig))) {
-      return new UrlRewriter(log, reader);
+    try (BufferedReader reader = Files.newBufferedReader(Paths.get(configPath))) {
+      return new UrlRewriter(log, configPath, reader);
     } catch (IOException e) {
       throw new UncheckedIOException(e);
     }

src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriterConfig.java

@@ -25,6 +25,7 @@
 import java.util.Map;
 import java.util.Set;
 import java.util.regex.Pattern;
+import net.starlark.java.syntax.Location;
 
 /**
  * Models the downloader config file. This file has a line-based format, with each line starting
@@ -72,15 +73,18 @@ class UrlRewriterConfig {
   /**
    * Constructor to use. The {@code config} will be read to completion.
    *
+   * @throws UrlRewriterParseException If the file contents was invalid.
    * @throws UncheckedIOException If any processing problems occur.
    */
-  public UrlRewriterConfig(Reader config) {
+  public UrlRewriterConfig(String filePathForErrorReporting, Reader config)
+      throws UrlRewriterParseException {
     ImmutableSet.Builder<String> allowList = ImmutableSet.builder();
     ImmutableSet.Builder<String> blockList = ImmutableSet.builder();
     ImmutableMultimap.Builder<Pattern, String> rewrites = ImmutableMultimap.builder();
 
     try (BufferedReader reader = new BufferedReader(config)) {
-      for (String line = reader.readLine(); line != null; line = reader.readLine()) {
+      int lineNumber = 1;
+      for (String line = reader.readLine(); line != null; line = reader.readLine(), lineNumber++) {
         // Find the first word
         List<String> parts = SPLITTER.splitToList(line);
         if (parts.isEmpty()) {
@@ -92,34 +96,37 @@ public UrlRewriterConfig(Reader config) {
           continue;
         }
 
+        Location location = Location.fromFileLineColumn(filePathForErrorReporting, lineNumber, 0);
+
         switch (parts.get(0)) {
           case "allow":
             if (parts.size() != 2) {
-              throw new IllegalStateException(
-                  "Only the host name is allowed after `allow`: " + line);
+              throw new UrlRewriterParseException(
+                  "Only the host name is allowed after `allow`: " + line, location);
             }
             allowList.add(parts.get(1));
             break;
 
           case "block":
             if (parts.size() != 2) {
-              throw new IllegalStateException(
-                  "Only the host name is allowed after `block`: " + line);
+              throw new UrlRewriterParseException(
+                  "Only the host name is allowed after `block`: " + line, location);
             }
             blockList.add(parts.get(1));
             break;
 
           case "rewrite":
             if (parts.size() != 3) {
-              throw new IllegalStateException(
+              throw new UrlRewriterParseException(
                   "Only the matching pattern and rewrite pattern is allowed after `rewrite`: "
-                      + line);
+                      + line,
+                  location);
             }
             rewrites.put(Pattern.compile(parts.get(1)), parts.get(2));
             break;
 
           default:
-            throw new IllegalStateException("Unable to parse: " + line);
+            throw new UrlRewriterParseException("Unable to parse: " + line, location);
         }
       }
     } catch (IOException e) {

src/main/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriterParseException.java

@@ -0,0 +1,30 @@
+// Copyright 2021 The Bazel Authors. All rights reserved.
+//
+// Licensed under the Apache License, Version 2.0 (the "License");
+// you may not use this file except in compliance with the License.
+// You may obtain a copy of the License at
+//
+//    http://www.apache.org/licenses/LICENSE-2.0
+//
+// Unless required by applicable law or agreed to in writing, software
+// distributed under the License is distributed on an "AS IS" BASIS,
+// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+// See the License for the specific language governing permissions and
+// limitations under the License.
+package com.google.devtools.build.lib.bazel.repository.downloader;
+
+import net.starlark.java.syntax.Location;
+
+/** An {@link Exception} thrown when failed to parse {@link UrlRewriterConfig}. */
+public class UrlRewriterParseException extends Exception {
+  private final Location location;
+
+  public UrlRewriterParseException(String message, Location location) {
+    super(message);
+    this.location = location;
+  }
+
+  public Location getLocation() {
+    return location;
+  }
+}

src/main/protobuf/failure_details.proto

@@ -240,6 +240,7 @@ message ExternalRepository {
   enum Code {
     EXTERNAL_REPOSITORY_UNKNOWN = 0 [(metadata) = { exit_code: 37 }];
     OVERRIDE_DISALLOWED_MANAGED_DIRECTORIES = 1 [(metadata) = { exit_code: 2 }];
+    BAD_DOWNLOADER_CONFIG = 2 [(metadata) = { exit_code: 2 }];
   }
   Code code = 1;
   // Additional data could include external repository names.

src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/BUILD

@@ -21,6 +21,7 @@ java_library(
         "//src/main/java/com/google/devtools/build/lib/events",
         "//src/main/java/com/google/devtools/build/lib/util",
         "//src/main/java/com/google/devtools/build/lib/vfs",
+        "//src/main/java/net/starlark/java/syntax",
         "//src/test/java/com/google/devtools/build/lib/testutil",
         "//third_party:guava",
         "//third_party:jsr305",

src/test/java/com/google/devtools/build/lib/bazel/repository/downloader/UrlRewriterTest.java

@@ -14,12 +14,13 @@
 package com.google.devtools.build.lib.bazel.repository.downloader;
 
 import static com.google.common.truth.Truth.assertThat;
+import static org.junit.Assert.fail;
 
 import com.google.common.collect.ImmutableList;
 import java.io.StringReader;
-import java.net.MalformedURLException;
 import java.net.URL;
 import java.util.List;
+import net.starlark.java.syntax.Location;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.JUnit4;
@@ -29,8 +30,8 @@
 public class UrlRewriterTest {
 
   @Test
-  public void byDefaultTheUrlRewriterDoesNothing() throws MalformedURLException {
-    UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(""));
+  public void byDefaultTheUrlRewriterDoesNothing() throws Exception {
+    UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(""));
 
     List<URL> urls = ImmutableList.of(new URL("http://example.com"));
     List<URL> amended = munger.amend(urls);
@@ -39,9 +40,9 @@ public void byDefaultTheUrlRewriterDoesNothing() throws MalformedURLException {
   }
 
   @Test
-  public void shouldBeAbleToBlockParticularHostsRegardlessOfScheme() throws MalformedURLException {
+  public void shouldBeAbleToBlockParticularHostsRegardlessOfScheme() throws Exception {
     String config = "block example.com";
-    UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
+    UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
 
     List<URL> urls =
         ImmutableList.of(
@@ -54,9 +55,9 @@ public void shouldBeAbleToBlockParticularHostsRegardlessOfScheme() throws Malfor
   }
 
   @Test
-  public void shouldAllowAUrlToBeRewritten() throws MalformedURLException {
+  public void shouldAllowAUrlToBeRewritten() throws Exception {
     String config = "rewrite example.com/foo/(.*) mycorp.com/$1/foo";
-    UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
+    UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
 
     List<URL> urls = ImmutableList.of(new URL("https://example.com/foo/bar"));
     List<URL> amended = munger.amend(urls);
@@ -65,11 +66,11 @@ public void shouldAllowAUrlToBeRewritten() throws MalformedURLException {
   }
 
   @Test
-  public void rewritesCanExpandToMoreThanOneUrl() throws MalformedURLException {
+  public void rewritesCanExpandToMoreThanOneUrl() throws Exception {
     String config =
         "rewrite example.com/foo/(.*) mycorp.com/$1/somewhere\n"
             + "rewrite example.com/foo/(.*) mycorp.com/$1/elsewhere";
-    UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
+    UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
 
     List<URL> urls = ImmutableList.of(new URL("https://example.com/foo/bar"));
     List<URL> amended = munger.amend(urls);
@@ -80,10 +81,10 @@ public void rewritesCanExpandToMoreThanOneUrl() throws MalformedURLException {
   }
 
   @Test
-  public void shouldBlockAllUrlsOtherThanSpecificOnes() throws MalformedURLException {
+  public void shouldBlockAllUrlsOtherThanSpecificOnes() throws Exception {
     String config = "" + "block *\n" + "allow example.com";
 
-    UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
+    UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
 
     List<URL> urls =
         ImmutableList.of(
@@ -98,15 +99,15 @@ public void shouldBlockAllUrlsOtherThanSpecificOnes() throws MalformedURLExcepti
   }
 
   @Test
-  public void commentsArePrecededByTheHashCharacter() throws MalformedURLException {
+  public void commentsArePrecededByTheHashCharacter() throws Exception {
     String config =
         ""
             + "# Block everything\n"
             + "block *\n"
             + "# But allow example.com\n"
             + "allow example.com";
 
-    UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
+    UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
 
     List<URL> urls = ImmutableList.of(new URL("https://foo.com"), new URL("https://example.com"));
     List<URL> amended = munger.amend(urls);
@@ -115,43 +116,43 @@ public void commentsArePrecededByTheHashCharacter() throws MalformedURLException
   }
 
   @Test
-  public void allowListAppliesToSubdomainsToo() throws MalformedURLException {
+  public void allowListAppliesToSubdomainsToo() throws Exception {
     String config = "" + "block *\n" + "allow example.com";
 
-    UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
+    UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
 
     List<URL> amended = munger.amend(ImmutableList.of(new URL("https://subdomain.example.com")));
 
     assertThat(amended).containsExactly(new URL("https://subdomain.example.com"));
   }
 
   @Test
-  public void blockListAppliesToSubdomainsToo() throws MalformedURLException {
+  public void blockListAppliesToSubdomainsToo() throws Exception {
     String config = "block example.com";
 
-    UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
+    UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
 
     List<URL> amended = munger.amend(ImmutableList.of(new URL("https://subdomain.example.com")));
 
     assertThat(amended).isEmpty();
   }
 
   @Test
-  public void emptyLinesAreFine() throws MalformedURLException {
+  public void emptyLinesAreFine() throws Exception {
     String config = "" + "\n" + "   \n" + "block *\n" + "\t  \n" + "allow example.com";
 
-    UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
+    UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
 
     List<URL> amended = munger.amend(ImmutableList.of(new URL("https://subdomain.example.com")));
 
     assertThat(amended).containsExactly(new URL("https://subdomain.example.com"));
   }
 
   @Test
-  public void rewritingUrlsIsAppliedBeforeBlocking() throws MalformedURLException {
+  public void rewritingUrlsIsAppliedBeforeBlocking() throws Exception {
     String config = "" + "block bad.com\n" + "rewrite bad.com/foo/(.*) mycorp.com/$1";
 
-    UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
+    UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
 
     List<URL> amended =
         munger.amend(
@@ -161,16 +162,27 @@ public void rewritingUrlsIsAppliedBeforeBlocking() throws MalformedURLException
   }
 
   @Test
-  public void rewritingUrlsIsAppliedBeforeAllowing() throws MalformedURLException {
+  public void rewritingUrlsIsAppliedBeforeAllowing() throws Exception {
     String config =
         "" + "block *\n" + "allow mycorp.com\n" + "rewrite bad.com/foo/(.*) mycorp.com/$1";
 
-    UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
+    UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
 
     List<URL> amended =
         munger.amend(
             ImmutableList.of(new URL("https://www.bad.com"), new URL("https://bad.com/foo/bar")));
 
     assertThat(amended).containsExactly(new URL("https://mycorp.com/bar"));
   }
+
+  @Test
+  public void parseError() throws Exception {
+    String config = "#comment\nhello";
+    try {
+      new UrlRewriterConfig("/some/file", new StringReader(config));
+      fail();
+    } catch (UrlRewriterParseException e) {
+      assertThat(e.getLocation()).isEqualTo(Location.fromFileLineColumn("/some/file", 2, 0));
+    }
+  }
 }