Skip to content

Commit 753abba

Browse files
pstreeftimtebeek
andauthored
Prevent cross-scope source files from being parsed as PlainText (#1123)
* Prevent cross-scope source files from being parsed as PlainText processMainSources and processTestSources each iterate resource directories via getResources()/getTestResources(). When a resource directory points at ${project.basedir}, OmniParser walks the entire project tree. Source files belonging to the other scope are not yet in alreadyParsed, so they get claimed by PlainTextParser. The real parse in the other scope then produces a duplicate entry, and the PlainText version shadows the properly typed one — making type information and annotations invisible to recipes. Fix: before each scope's resource loop, pre-populate alreadyParsed with the other scope's source file paths (Java, Kotlin, Groovy). * Centralize cross-scope source exclusion in listSourceFiles Move the alreadyParsed pre-population from each scope method into listSourceFiles(), eliminating coupling between processMainSources and processTestSources. Both scopes' source paths are now computed once upfront before either scope method runs. * Rename alreadyParsed to parsedPaths and remove redundant addAll calls Rename the Set<Path> variable/parameter from alreadyParsed to parsedPaths since it is now pre-populated before parsing begins. Remove the redundant addAll calls in processMainSources and processTestSources that re-added paths already present from the upfront pre-population. * Add integration test for basedir resource PlainText leak Verify that source files in both main and test scopes are parsed as Java (not PlainText) when resource directories point at project.basedir. AutoFormat only operates on Java ASTs, so it will not modify files that were incorrectly claimed by PlainTextParser. --------- Co-authored-by: Tim te Beek <tim@moderne.io>
1 parent 86478fe commit 753abba

File tree

5 files changed

+108
-31
lines changed

5 files changed

+108
-31
lines changed

src/main/java/org/openrewrite/maven/MavenMojoProjectParser.java

Lines changed: 34 additions & 31 deletions
Original file line numberDiff line numberDiff line change
@@ -196,11 +196,11 @@ public Stream<SourceFile> listSourceFiles(MavenProject mavenProject, Xml.@Nullab
196196
public Stream<SourceFile> listSourceFiles(MavenProject mavenProject, Xml.@Nullable Document maven, List<Marker> projectProvenance, List<MavenScope> scopes,
197197
List<NamedStyles> styles, ExecutionContext ctx) throws DependencyResolutionRequiredException, MojoExecutionException {
198198
Stream<SourceFile> sourceFiles = Stream.empty();
199-
Set<Path> alreadyParsed = new HashSet<>();
199+
Set<Path> parsedPaths = new HashSet<>();
200200

201201
if (maven != null) {
202202
sourceFiles = Stream.of(maven);
203-
alreadyParsed.add(baseDir.resolve(maven.getSourcePath()));
203+
parsedPaths.add(baseDir.resolve(maven.getSourcePath()));
204204
}
205205

206206
JavaParser.Builder<? extends JavaParser, ?> javaParserBuilder = JavaParser.fromJavaVersion()
@@ -211,11 +211,20 @@ public Stream<SourceFile> listSourceFiles(MavenProject mavenProject, Xml.@Nullab
211211
KotlinParser.Builder kotlinParserBuilder = KotlinParser.builder();
212212
GroovyParser.Builder groovyParserBuilder = GroovyParser.builder();
213213

214+
// Pre-populate parsedPaths with all source paths from both scopes
215+
// to prevent resource parsers from claiming cross-scope sources as PlainText
216+
parsedPaths.addAll(listJavaSources(mavenProject, mavenProject.getExecutionProject().getCompileSourceRoots()));
217+
parsedPaths.addAll(listKotlinSources(mavenProject, "compile", mavenProject.getBuild().getSourceDirectory()));
218+
parsedPaths.addAll(listGroovySources(mavenProject, mavenProject.getExecutionProject().getCompileSourceRoots()));
219+
parsedPaths.addAll(listJavaSources(mavenProject, mavenProject.getExecutionProject().getTestCompileSourceRoots()));
220+
parsedPaths.addAll(listKotlinSources(mavenProject, "test-compile", mavenProject.getBuild().getTestSourceDirectory()));
221+
parsedPaths.addAll(listGroovySources(mavenProject, mavenProject.getExecutionProject().getTestCompileSourceRoots()));
222+
214223
if (scopes.contains(MAIN)) {
215-
sourceFiles = Stream.concat(sourceFiles, processMainSources(mavenProject, javaParserBuilder.clone(), kotlinParserBuilder.clone(), groovyParserBuilder.clone(), alreadyParsed, ctx));
224+
sourceFiles = Stream.concat(sourceFiles, processMainSources(mavenProject, javaParserBuilder.clone(), kotlinParserBuilder.clone(), groovyParserBuilder.clone(), parsedPaths, ctx));
216225
}
217226
if (scopes.contains(TEST)) {
218-
sourceFiles = Stream.concat(sourceFiles, processTestSources(mavenProject, javaParserBuilder.clone(), kotlinParserBuilder.clone(), groovyParserBuilder.clone(), alreadyParsed, ctx));
227+
sourceFiles = Stream.concat(sourceFiles, processTestSources(mavenProject, javaParserBuilder.clone(), kotlinParserBuilder.clone(), groovyParserBuilder.clone(), parsedPaths, ctx));
219228
}
220229
Collection<PathMatcher> exclusionMatchers = exclusions.stream()
221230
.map(pattern -> baseDir.getFileSystem().getPathMatcher("glob:" + pattern))
@@ -227,10 +236,10 @@ public Stream<SourceFile> listSourceFiles(MavenProject mavenProject, Xml.@Nullab
227236
return sourceFile;
228237
}).filter(Objects::nonNull);
229238

230-
Stream<SourceFile> mavenWrapperFiles = parseMavenWrapperFiles(mavenProject, exclusionMatchers, alreadyParsed, ctx);
239+
Stream<SourceFile> mavenWrapperFiles = parseMavenWrapperFiles(mavenProject, exclusionMatchers, parsedPaths, ctx);
231240
sourceFiles = Stream.concat(sourceFiles, mavenWrapperFiles);
232241

233-
Stream<SourceFile> nonProjectResources = parseNonProjectResources(mavenProject, alreadyParsed, ctx);
242+
Stream<SourceFile> nonProjectResources = parseNonProjectResources(mavenProject, parsedPaths, ctx);
234243
sourceFiles = Stream.concat(sourceFiles, nonProjectResources);
235244

236245
return sourceFiles.map(addProvenance(projectProvenance))
@@ -474,22 +483,19 @@ private Stream<SourceFile> processMainSources(
474483
JavaParser.Builder<? extends JavaParser, ?> javaParserBuilder,
475484
KotlinParser.Builder kotlinParserBuilder,
476485
GroovyParser.Builder groovyParserBuilder,
477-
Set<Path> alreadyParsed,
486+
Set<Path> parsedPaths,
478487
ExecutionContext ctx) throws DependencyResolutionRequiredException, MojoExecutionException {
479488

480489
Stream<SourceFile> sourceFiles = Stream.of();
481490

482491
// scan Java files
483492
Collection<Path> mainJavaSources = listJavaSources(mavenProject, mavenProject.getExecutionProject().getCompileSourceRoots());
484-
alreadyParsed.addAll(mainJavaSources);
485493

486494
// scan Kotlin files
487495
List<Path> mainKotlinSources = listKotlinSources(mavenProject, "compile", mavenProject.getBuild().getSourceDirectory());
488-
alreadyParsed.addAll(mainKotlinSources);
489496

490497
// scan Groovy files
491498
List<Path> mainGroovySources = listGroovySources(mavenProject, mavenProject.getExecutionProject().getCompileSourceRoots());
492-
alreadyParsed.addAll(mainGroovySources);
493499

494500
logInfo(mavenProject, "Parsing source files");
495501
List<Path> dependencies = mavenProject.getCompileClasspathElements().stream()
@@ -531,25 +537,25 @@ private Stream<SourceFile> processMainSources(
531537
logDebug(mavenProject, "Scanned " + mainGroovySources.size() + " groovy source files in main scope.");
532538
}
533539

534-
OmniParser omniParser = omniParser(alreadyParsed, mavenProject);
540+
OmniParser omniParser = omniParser(parsedPaths, mavenProject);
535541
for (Resource resource : mavenProject.getResources()) {
536542
Path resourcePath = mavenProject.getBasedir().toPath().resolve(resource.getDirectory());
537-
if (Files.exists(resourcePath) && !alreadyParsed.contains(resourcePath)) {
543+
if (Files.exists(resourcePath) && !parsedPaths.contains(resourcePath)) {
538544
List<Path> accepted = omniParser.acceptedPaths(baseDir, resourcePath);
539-
alreadyParsed.add(resourcePath);
545+
parsedPaths.add(resourcePath);
540546
sourceFiles = Stream.concat(sourceFiles, omniParser.parse(accepted, baseDir, ctx));
541-
alreadyParsed.addAll(accepted);
547+
parsedPaths.addAll(accepted);
542548
}
543549
}
544550

545551
// Also parse webapp resources (e.g., web.xml) for WAR projects
546552
if ("war".equals(mavenProject.getPackaging())) {
547553
Path webappPath = mavenProject.getBasedir().toPath().resolve("src/main/webapp");
548-
if (Files.exists(webappPath) && !alreadyParsed.contains(webappPath)) {
554+
if (Files.exists(webappPath) && !parsedPaths.contains(webappPath)) {
549555
List<Path> accepted = omniParser.acceptedPaths(baseDir, webappPath);
550-
alreadyParsed.add(webappPath);
556+
parsedPaths.add(webappPath);
551557
sourceFiles = Stream.concat(sourceFiles, omniParser.parse(accepted, baseDir, ctx));
552-
alreadyParsed.addAll(accepted);
558+
parsedPaths.addAll(accepted);
553559
}
554560
}
555561

@@ -570,22 +576,19 @@ private Stream<SourceFile> processTestSources(
570576
JavaParser.Builder<? extends JavaParser, ?> javaParserBuilder,
571577
KotlinParser.Builder kotlinParserBuilder,
572578
GroovyParser.Builder groovyParserBuilder,
573-
Set<Path> alreadyParsed,
579+
Set<Path> parsedPaths,
574580
ExecutionContext ctx) throws DependencyResolutionRequiredException, MojoExecutionException {
575581

576582
Stream<SourceFile> sourceFiles = Stream.of();
577583

578584
// scan Java files
579585
Collection<Path> testJavaSources = listJavaSources(mavenProject, mavenProject.getExecutionProject().getTestCompileSourceRoots());
580-
alreadyParsed.addAll(testJavaSources);
581586

582587
// scan Kotlin files
583588
List<Path> testKotlinSources = listKotlinSources(mavenProject, "test-compile", mavenProject.getBuild().getTestSourceDirectory());
584-
alreadyParsed.addAll(testKotlinSources);
585589

586590
// scan Groovy files
587591
List<Path> testGroovySources = listGroovySources(mavenProject, mavenProject.getExecutionProject().getTestCompileSourceRoots());
588-
alreadyParsed.addAll(testGroovySources);
589592

590593
List<Path> testDependencies = mavenProject.getTestClasspathElements().stream()
591594
.distinct()
@@ -626,14 +629,14 @@ private Stream<SourceFile> processTestSources(
626629
logDebug(mavenProject, "Scanned " + testGroovySources.size() + " groovy source files in test scope.");
627630
}
628631

629-
OmniParser omniParser = omniParser(alreadyParsed, mavenProject);
632+
OmniParser omniParser = omniParser(parsedPaths, mavenProject);
630633
for (Resource resource : mavenProject.getTestResources()) {
631634
Path resourcePath = mavenProject.getBasedir().toPath().resolve(resource.getDirectory());
632-
if (Files.exists(resourcePath) && !alreadyParsed.contains(resourcePath)) {
635+
if (Files.exists(resourcePath) && !parsedPaths.contains(resourcePath)) {
633636
List<Path> accepted = omniParser.acceptedPaths(baseDir, resourcePath);
634-
alreadyParsed.add(resourcePath);
637+
parsedPaths.add(resourcePath);
635638
sourceFiles = Stream.concat(sourceFiles, omniParser.parse(accepted, baseDir, ctx));
636-
alreadyParsed.addAll(accepted);
639+
parsedPaths.addAll(accepted);
637640
}
638641
}
639642

@@ -1002,10 +1005,10 @@ public FileVisitResult visitFile(Path file, BasicFileAttributes attrs) {
10021005
}
10031006
}
10041007

1005-
private Stream<SourceFile> parseMavenWrapperFiles(MavenProject mavenProject, Collection<PathMatcher> exclusions, Set<Path> alreadyParsed, ExecutionContext ctx) {
1008+
private Stream<SourceFile> parseMavenWrapperFiles(MavenProject mavenProject, Collection<PathMatcher> exclusions, Set<Path> parsedPaths, ExecutionContext ctx) {
10061009
Stream<SourceFile> sourceFiles = Stream.empty();
10071010
if (mavenProject.getParent() == null) {
1008-
OmniParser omniParser = omniParser(alreadyParsed, mavenProject);
1011+
OmniParser omniParser = omniParser(parsedPaths, mavenProject);
10091012
List<Path> mavenWrapperFiles = Stream.of(
10101013
Paths.get(MVN_JVM_CONFIG),
10111014
Paths.get(MVN_MAVEN_CONFIG),
@@ -1023,17 +1026,17 @@ private Stream<SourceFile> parseMavenWrapperFiles(MavenProject mavenProject, Col
10231026
return sourceFiles;
10241027
}
10251028

1026-
protected Stream<SourceFile> parseNonProjectResources(MavenProject mavenProject, Set<Path> alreadyParsed, ExecutionContext ctx) {
1029+
protected Stream<SourceFile> parseNonProjectResources(MavenProject mavenProject, Set<Path> parsedPaths, ExecutionContext ctx) {
10271030
if (!parseAdditionalResources) {
10281031
return Stream.empty();
10291032
}
10301033
//Collect any additional yaml/properties/xml files that are NOT already in a source set.
1031-
OmniParser omniParser = omniParser(alreadyParsed, mavenProject);
1034+
OmniParser omniParser = omniParser(parsedPaths, mavenProject);
10321035
List<Path> accepted = omniParser.acceptedPaths(baseDir, mavenProject.getBasedir().toPath());
10331036
return omniParser.parse(accepted, baseDir, ctx);
10341037
}
10351038

1036-
private OmniParser omniParser(Set<Path> alreadyParsed, MavenProject mavenProject) {
1039+
private OmniParser omniParser(Set<Path> parsedPaths, MavenProject mavenProject) {
10371040
return OmniParser.builder(
10381041
OmniParser.defaultResourceParsers(),
10391042
PlainTextParser.builder()
@@ -1042,7 +1045,7 @@ private OmniParser omniParser(Set<Path> alreadyParsed, MavenProject mavenProject
10421045
QuarkParser.builder().build()
10431046
)
10441047
.exclusionMatchers(pathMatchers(baseDir, mergeExclusions(mavenProject)))
1045-
.exclusions(alreadyParsed)
1048+
.exclusions(parsedPaths)
10461049
.sizeThresholdMb(sizeThresholdMb)
10471050
.build();
10481051
}

src/test/java/org/openrewrite/maven/RewriteRunIT.java

Lines changed: 12 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -89,6 +89,18 @@ void multi_main_source_sets_project(MavenExecutionResult result) {
8989
.contains("Changes have been made to project/src/additional-main/java/sample/AdditionalMainClass.java by:");
9090
}
9191

92+
@MavenTest
93+
void basedir_resource_no_plaintext_leak(MavenExecutionResult result) {
94+
assertThat(result)
95+
.isSuccessful()
96+
.out()
97+
.warn()
98+
.contains(
99+
"Changes have been made to %s by:".formatted(separatorsToSystem("project/src/main/java/sample/Main.java")),
100+
"Changes have been made to %s by:".formatted(separatorsToSystem("project/src/test/java/sample/MainTest.java"))
101+
);
102+
}
103+
92104
@MavenTest
93105
void single_project(MavenExecutionResult result) {
94106
assertThat(result)
Lines changed: 48 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,48 @@
1+
<project xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance"
2+
xmlns="http://maven.apache.org/POM/4.0.0"
3+
xsi:schemaLocation="http://maven.apache.org/POM/4.0.0 http://maven.apache.org/xsd/maven-4.0.0.xsd">
4+
<modelVersion>4.0.0</modelVersion>
5+
6+
<groupId>org.openrewrite.maven</groupId>
7+
<artifactId>basedir_resource_no_plaintext_leak</artifactId>
8+
<version>1.0</version>
9+
<packaging>jar</packaging>
10+
<name>RewriteRunIT#basedir_resource_no_plaintext_leak</name>
11+
12+
<properties>
13+
<maven.compiler.source>1.8</maven.compiler.source>
14+
<maven.compiler.target>1.8</maven.compiler.target>
15+
<project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
16+
</properties>
17+
18+
<build>
19+
<resources>
20+
<resource>
21+
<directory>${project.basedir}</directory>
22+
<excludes>
23+
<exclude>**/*</exclude>
24+
</excludes>
25+
</resource>
26+
</resources>
27+
<testResources>
28+
<testResource>
29+
<directory>${project.basedir}</directory>
30+
<excludes>
31+
<exclude>**/*</exclude>
32+
</excludes>
33+
</testResource>
34+
</testResources>
35+
<plugins>
36+
<plugin>
37+
<groupId>@project.groupId@</groupId>
38+
<artifactId>@project.artifactId@</artifactId>
39+
<version>@project.version@</version>
40+
<configuration>
41+
<activeRecipes>
42+
<recipe>org.openrewrite.java.format.AutoFormat</recipe>
43+
</activeRecipes>
44+
</configuration>
45+
</plugin>
46+
</plugins>
47+
</build>
48+
</project>
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
package sample;
2+
3+
public class Main {
4+
public void hello() {
5+
if(true) {}
6+
}
7+
}
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,7 @@
1+
package sample;
2+
3+
public class MainTest {
4+
public void test() {
5+
if(true) {}
6+
}
7+
}

0 commit comments

Comments
 (0)