Skip to content

Commit b243584

Browse files
illicitonioncopybara-github
illicitonion
authored and
copybara-github
committed
Report errors parsing rewriter config file
Before this change, an invalid config file results in a silent server crash. Closes #12989. PiperOrigin-RevId: 359251417
1 parent 17afbe4 commit b243584

8 files changed

Lines changed: 110 additions & 45 deletions

File tree

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

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -36,6 +36,7 @@
3636
import com.google.devtools.build.lib.bazel.repository.downloader.DownloadManager;
3737
import com.google.devtools.build.lib.bazel.repository.downloader.HttpDownloader;
3838
import com.google.devtools.build.lib.bazel.repository.downloader.UrlRewriter;
39+
import com.google.devtools.build.lib.bazel.repository.downloader.UrlRewriterParseException;
3940
import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryFunction;
4041
import com.google.devtools.build.lib.bazel.repository.starlark.StarlarkRepositoryModule;
4142
import com.google.devtools.build.lib.bazel.rules.android.AndroidNdkRepositoryFunction;
@@ -225,7 +226,7 @@ public void initializeRuleClasses(ConfiguredRuleClassProvider.Builder builder) {
225226
}
226227

227228
@Override
228-
public void beforeCommand(CommandEnvironment env) {
229+
public void beforeCommand(CommandEnvironment env) throws AbruptExitException {
229230
clientEnvironmentSupplier.set(env.getRepoEnv());
230231
PackageOptions pkgOptions = env.getOptions().getOptions(PackageOptions.class);
231232
isFetch.set(pkgOptions != null && pkgOptions.fetch);
@@ -284,10 +285,20 @@ public void beforeCommand(CommandEnvironment env) {
284285
}
285286
}
286287

287-
UrlRewriter rewriter =
288-
UrlRewriter.getDownloaderUrlRewriter(
289-
repoOptions == null ? null : repoOptions.downloaderConfig, env.getReporter());
290-
downloadManager.setUrlRewriter(rewriter);
288+
try {
289+
UrlRewriter rewriter =
290+
UrlRewriter.getDownloaderUrlRewriter(
291+
repoOptions == null ? null : repoOptions.downloaderConfig, env.getReporter());
292+
downloadManager.setUrlRewriter(rewriter);
293+
} catch (UrlRewriterParseException e) {
294+
// It's important that the build stops ASAP, because this config file may be required for
295+
// security purposes, and the build must not proceed ignoring it.
296+
throw new AbruptExitException(
297+
detailedExitCode(
298+
String.format(
299+
"Failed to parse downloader config at %s: %s", e.getLocation(), e.getMessage()),
300+
Code.BAD_DOWNLOADER_CONFIG));
301+
}
291302

292303
if (repoOptions.experimentalDistdir != null) {
293304
downloadManager.setDistdir(

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -22,6 +22,7 @@ java_library(
2222
"//src/main/java/com/google/devtools/build/lib/util",
2323
"//src/main/java/com/google/devtools/build/lib/vfs",
2424
"//src/main/java/com/google/devtools/build/lib/vfs:pathfragment",
25+
"//src/main/java/net/starlark/java/syntax",
2526
"//third_party:flogger",
2627
"//third_party:guava",
2728
"//third_party:jsr305",

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

Lines changed: 10 additions & 8 deletions
Original file line numberDiff line numberDiff line change
@@ -57,29 +57,31 @@ public class UrlRewriter {
5757
private final Consumer<String> log;
5858

5959
@VisibleForTesting
60-
UrlRewriter(Consumer<String> log, Reader reader) {
60+
UrlRewriter(Consumer<String> log, String filePathForErrorReporting, Reader reader)
61+
throws UrlRewriterParseException {
6162
this.log = Preconditions.checkNotNull(log);
6263
Preconditions.checkNotNull(reader, "UrlRewriterConfig source must be set");
63-
this.config = new UrlRewriterConfig(reader);
64+
this.config = new UrlRewriterConfig(filePathForErrorReporting, reader);
6465

6566
this.rewriter = this::rewrite;
6667
}
6768

6869
/**
6970
* Obtain a new {@code UrlRewriter} configured with the specified config file.
7071
*
71-
* @param downloaderConfig Path to the config file to use. May be null.
72+
* @param configPath Path to the config file to use. May be null.
7273
* @param reporter Used for logging when URLs are rewritten.
7374
*/
74-
public static UrlRewriter getDownloaderUrlRewriter(String downloaderConfig, Reporter reporter) {
75+
public static UrlRewriter getDownloaderUrlRewriter(String configPath, Reporter reporter)
76+
throws UrlRewriterParseException {
7577
Consumer<String> log = str -> reporter.handle(Event.info(str));
7678

77-
if (Strings.isNullOrEmpty(downloaderConfig)) {
78-
return new UrlRewriter(log, new StringReader(""));
79+
if (Strings.isNullOrEmpty(configPath)) {
80+
return new UrlRewriter(log, "", new StringReader(""));
7981
}
8082

81-
try (BufferedReader reader = Files.newBufferedReader(Paths.get(downloaderConfig))) {
82-
return new UrlRewriter(log, reader);
83+
try (BufferedReader reader = Files.newBufferedReader(Paths.get(configPath))) {
84+
return new UrlRewriter(log, configPath, reader);
8385
} catch (IOException e) {
8486
throw new UncheckedIOException(e);
8587
}

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

Lines changed: 16 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,7 @@
2525
import java.util.Map;
2626
import java.util.Set;
2727
import java.util.regex.Pattern;
28+
import net.starlark.java.syntax.Location;
2829

2930
/**
3031
* Models the downloader config file. This file has a line-based format, with each line starting
@@ -72,15 +73,18 @@ class UrlRewriterConfig {
7273
/**
7374
* Constructor to use. The {@code config} will be read to completion.
7475
*
76+
* @throws UrlRewriterParseException If the file contents was invalid.
7577
* @throws UncheckedIOException If any processing problems occur.
7678
*/
77-
public UrlRewriterConfig(Reader config) {
79+
public UrlRewriterConfig(String filePathForErrorReporting, Reader config)
80+
throws UrlRewriterParseException {
7881
ImmutableSet.Builder<String> allowList = ImmutableSet.builder();
7982
ImmutableSet.Builder<String> blockList = ImmutableSet.builder();
8083
ImmutableMultimap.Builder<Pattern, String> rewrites = ImmutableMultimap.builder();
8184

8285
try (BufferedReader reader = new BufferedReader(config)) {
83-
for (String line = reader.readLine(); line != null; line = reader.readLine()) {
86+
int lineNumber = 1;
87+
for (String line = reader.readLine(); line != null; line = reader.readLine(), lineNumber++) {
8488
// Find the first word
8589
List<String> parts = SPLITTER.splitToList(line);
8690
if (parts.isEmpty()) {
@@ -92,34 +96,37 @@ public UrlRewriterConfig(Reader config) {
9296
continue;
9397
}
9498

99+
Location location = Location.fromFileLineColumn(filePathForErrorReporting, lineNumber, 0);
100+
95101
switch (parts.get(0)) {
96102
case "allow":
97103
if (parts.size() != 2) {
98-
throw new IllegalStateException(
99-
"Only the host name is allowed after `allow`: " + line);
104+
throw new UrlRewriterParseException(
105+
"Only the host name is allowed after `allow`: " + line, location);
100106
}
101107
allowList.add(parts.get(1));
102108
break;
103109

104110
case "block":
105111
if (parts.size() != 2) {
106-
throw new IllegalStateException(
107-
"Only the host name is allowed after `block`: " + line);
112+
throw new UrlRewriterParseException(
113+
"Only the host name is allowed after `block`: " + line, location);
108114
}
109115
blockList.add(parts.get(1));
110116
break;
111117

112118
case "rewrite":
113119
if (parts.size() != 3) {
114-
throw new IllegalStateException(
120+
throw new UrlRewriterParseException(
115121
"Only the matching pattern and rewrite pattern is allowed after `rewrite`: "
116-
+ line);
122+
+ line,
123+
location);
117124
}
118125
rewrites.put(Pattern.compile(parts.get(1)), parts.get(2));
119126
break;
120127

121128
default:
122-
throw new IllegalStateException("Unable to parse: " + line);
129+
throw new UrlRewriterParseException("Unable to parse: " + line, location);
123130
}
124131
}
125132
} catch (IOException e) {

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

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,30 @@
1+
// Copyright 2021 The Bazel Authors. All rights reserved.
2+
//
3+
// Licensed under the Apache License, Version 2.0 (the "License");
4+
// you may not use this file except in compliance with the License.
5+
// You may obtain a copy of the License at
6+
//
7+
// http://www.apache.org/licenses/LICENSE-2.0
8+
//
9+
// Unless required by applicable law or agreed to in writing, software
10+
// distributed under the License is distributed on an "AS IS" BASIS,
11+
// WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
12+
// See the License for the specific language governing permissions and
13+
// limitations under the License.
14+
package com.google.devtools.build.lib.bazel.repository.downloader;
15+
16+
import net.starlark.java.syntax.Location;
17+
18+
/** An {@link Exception} thrown when failed to parse {@link UrlRewriterConfig}. */
19+
public class UrlRewriterParseException extends Exception {
20+
private final Location location;
21+
22+
public UrlRewriterParseException(String message, Location location) {
23+
super(message);
24+
this.location = location;
25+
}
26+
27+
public Location getLocation() {
28+
return location;
29+
}
30+
}

‎src/main/protobuf/failure_details.proto‎

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -240,6 +240,7 @@ message ExternalRepository {
240240
enum Code {
241241
EXTERNAL_REPOSITORY_UNKNOWN = 0 [(metadata) = { exit_code: 37 }];
242242
OVERRIDE_DISALLOWED_MANAGED_DIRECTORIES = 1 [(metadata) = { exit_code: 2 }];
243+
BAD_DOWNLOADER_CONFIG = 2 [(metadata) = { exit_code: 2 }];
243244
}
244245
Code code = 1;
245246
// Additional data could include external repository names.

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

Lines changed: 1 addition & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -21,6 +21,7 @@ java_library(
2121
"//src/main/java/com/google/devtools/build/lib/events",
2222
"//src/main/java/com/google/devtools/build/lib/util",
2323
"//src/main/java/com/google/devtools/build/lib/vfs",
24+
"//src/main/java/net/starlark/java/syntax",
2425
"//src/test/java/com/google/devtools/build/lib/testutil",
2526
"//third_party:guava",
2627
"//third_party:jsr305",

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

Lines changed: 35 additions & 23 deletions
Original file line numberDiff line numberDiff line change
@@ -14,12 +14,13 @@
1414
package com.google.devtools.build.lib.bazel.repository.downloader;
1515

1616
import static com.google.common.truth.Truth.assertThat;
17+
import static org.junit.Assert.fail;
1718

1819
import com.google.common.collect.ImmutableList;
1920
import java.io.StringReader;
20-
import java.net.MalformedURLException;
2121
import java.net.URL;
2222
import java.util.List;
23+
import net.starlark.java.syntax.Location;
2324
import org.junit.Test;
2425
import org.junit.runner.RunWith;
2526
import org.junit.runners.JUnit4;
@@ -29,8 +30,8 @@
2930
public class UrlRewriterTest {
3031

3132
@Test
32-
public void byDefaultTheUrlRewriterDoesNothing() throws MalformedURLException {
33-
UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(""));
33+
public void byDefaultTheUrlRewriterDoesNothing() throws Exception {
34+
UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(""));
3435

3536
List<URL> urls = ImmutableList.of(new URL("http://example.com"));
3637
List<URL> amended = munger.amend(urls);
@@ -39,9 +40,9 @@ public void byDefaultTheUrlRewriterDoesNothing() throws MalformedURLException {
3940
}
4041

4142
@Test
42-
public void shouldBeAbleToBlockParticularHostsRegardlessOfScheme() throws MalformedURLException {
43+
public void shouldBeAbleToBlockParticularHostsRegardlessOfScheme() throws Exception {
4344
String config = "block example.com";
44-
UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
45+
UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
4546

4647
List<URL> urls =
4748
ImmutableList.of(
@@ -54,9 +55,9 @@ public void shouldBeAbleToBlockParticularHostsRegardlessOfScheme() throws Malfor
5455
}
5556

5657
@Test
57-
public void shouldAllowAUrlToBeRewritten() throws MalformedURLException {
58+
public void shouldAllowAUrlToBeRewritten() throws Exception {
5859
String config = "rewrite example.com/foo/(.*) mycorp.com/$1/foo";
59-
UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
60+
UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
6061

6162
List<URL> urls = ImmutableList.of(new URL("https://example.com/foo/bar"));
6263
List<URL> amended = munger.amend(urls);
@@ -65,11 +66,11 @@ public void shouldAllowAUrlToBeRewritten() throws MalformedURLException {
6566
}
6667

6768
@Test
68-
public void rewritesCanExpandToMoreThanOneUrl() throws MalformedURLException {
69+
public void rewritesCanExpandToMoreThanOneUrl() throws Exception {
6970
String config =
7071
"rewrite example.com/foo/(.*) mycorp.com/$1/somewhere\n"
7172
+ "rewrite example.com/foo/(.*) mycorp.com/$1/elsewhere";
72-
UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
73+
UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
7374

7475
List<URL> urls = ImmutableList.of(new URL("https://example.com/foo/bar"));
7576
List<URL> amended = munger.amend(urls);
@@ -80,10 +81,10 @@ public void rewritesCanExpandToMoreThanOneUrl() throws MalformedURLException {
8081
}
8182

8283
@Test
83-
public void shouldBlockAllUrlsOtherThanSpecificOnes() throws MalformedURLException {
84+
public void shouldBlockAllUrlsOtherThanSpecificOnes() throws Exception {
8485
String config = "" + "block *\n" + "allow example.com";
8586

86-
UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
87+
UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
8788

8889
List<URL> urls =
8990
ImmutableList.of(
@@ -98,15 +99,15 @@ public void shouldBlockAllUrlsOtherThanSpecificOnes() throws MalformedURLExcepti
9899
}
99100

100101
@Test
101-
public void commentsArePrecededByTheHashCharacter() throws MalformedURLException {
102+
public void commentsArePrecededByTheHashCharacter() throws Exception {
102103
String config =
103104
""
104105
+ "# Block everything\n"
105106
+ "block *\n"
106107
+ "# But allow example.com\n"
107108
+ "allow example.com";
108109

109-
UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
110+
UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
110111

111112
List<URL> urls = ImmutableList.of(new URL("https://foo.com"), new URL("https://example.com"));
112113
List<URL> amended = munger.amend(urls);
@@ -115,43 +116,43 @@ public void commentsArePrecededByTheHashCharacter() throws MalformedURLException
115116
}
116117

117118
@Test
118-
public void allowListAppliesToSubdomainsToo() throws MalformedURLException {
119+
public void allowListAppliesToSubdomainsToo() throws Exception {
119120
String config = "" + "block *\n" + "allow example.com";
120121

121-
UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
122+
UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
122123

123124
List<URL> amended = munger.amend(ImmutableList.of(new URL("https://subdomain.example.com")));
124125

125126
assertThat(amended).containsExactly(new URL("https://subdomain.example.com"));
126127
}
127128

128129
@Test
129-
public void blockListAppliesToSubdomainsToo() throws MalformedURLException {
130+
public void blockListAppliesToSubdomainsToo() throws Exception {
130131
String config = "block example.com";
131132

132-
UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
133+
UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
133134

134135
List<URL> amended = munger.amend(ImmutableList.of(new URL("https://subdomain.example.com")));
135136

136137
assertThat(amended).isEmpty();
137138
}
138139

139140
@Test
140-
public void emptyLinesAreFine() throws MalformedURLException {
141+
public void emptyLinesAreFine() throws Exception {
141142
String config = "" + "\n" + " \n" + "block *\n" + "\t \n" + "allow example.com";
142143

143-
UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
144+
UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
144145

145146
List<URL> amended = munger.amend(ImmutableList.of(new URL("https://subdomain.example.com")));
146147

147148
assertThat(amended).containsExactly(new URL("https://subdomain.example.com"));
148149
}
149150

150151
@Test
151-
public void rewritingUrlsIsAppliedBeforeBlocking() throws MalformedURLException {
152+
public void rewritingUrlsIsAppliedBeforeBlocking() throws Exception {
152153
String config = "" + "block bad.com\n" + "rewrite bad.com/foo/(.*) mycorp.com/$1";
153154

154-
UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
155+
UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
155156

156157
List<URL> amended =
157158
munger.amend(
@@ -161,16 +162,27 @@ public void rewritingUrlsIsAppliedBeforeBlocking() throws MalformedURLException
161162
}
162163

163164
@Test
164-
public void rewritingUrlsIsAppliedBeforeAllowing() throws MalformedURLException {
165+
public void rewritingUrlsIsAppliedBeforeAllowing() throws Exception {
165166
String config =
166167
"" + "block *\n" + "allow mycorp.com\n" + "rewrite bad.com/foo/(.*) mycorp.com/$1";
167168

168-
UrlRewriter munger = new UrlRewriter(str -> {}, new StringReader(config));
169+
UrlRewriter munger = new UrlRewriter(str -> {}, "/dev/null", new StringReader(config));
169170

170171
List<URL> amended =
171172
munger.amend(
172173
ImmutableList.of(new URL("https://www.bad.com"), new URL("https://bad.com/foo/bar")));
173174

174175
assertThat(amended).containsExactly(new URL("https://mycorp.com/bar"));
175176
}
177+
178+
@Test
179+
public void parseError() throws Exception {
180+
String config = "#comment\nhello";
181+
try {
182+
new UrlRewriterConfig("/some/file", new StringReader(config));
183+
fail();
184+
} catch (UrlRewriterParseException e) {
185+
assertThat(e.getLocation()).isEqualTo(Location.fromFileLineColumn("/some/file", 2, 0));
186+
}
187+
}
176188
}

0 commit comments

Comments
 (0)