Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Adds per codemod Includes/Excludes #438

Merged
merged 8 commits into from
Aug 23, 2024
Merged
Show file tree
Hide file tree
Changes from 7 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -26,6 +26,29 @@ final class WebGoat822Test extends GitRepositoryTest {
super("https://github.com/WebGoat/WebGoat", "main", "e75cfbeb110e3d3a2ca3c8fee2754992d89c419d");
}

private static final String testPathIncludes =
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we expose the actual values so both the tests and the CLI use the same values?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

CLI doesn't use these default includes anymore.

Do you still want these to be the default values for find-and-fix codemods? Right now every javaparser based one will only have **.java as its pattern.

"**.java,"
+ "**/*.java,"
+ "pom.xml,"
+ "**/pom.xml,"
+ "**.jsp,"
+ "**/*.jsp,"
+ "web.xml,"
+ "**/web.xml,"
+ ".github/workflows/*.yml,"
+ ".github/workflows/*.yaml";

private static final String testPathExcludes =
"**/test/**,"
+ "**/testFixtures/**,"
+ "**/*Test.java,"
+ "**/intTest/**,"
+ "**/tests/**,"
+ "**/target/**,"
+ "**/build/**,"
+ "**/.mvn/**,"
+ ".mvn/**";

@Test
void it_injects_dependency_even_when_no_poms_included() throws Exception {

Expand Down Expand Up @@ -83,7 +106,13 @@ void it_injects_dependency_even_when_no_poms_included() throws Exception {
void it_transforms_webgoat_normally() throws Exception {
DefaultCodemods.main(
new String[] {
"--output", outputFile.getPath(), "--verbose", "--dont-exit", repoDir.getPath()
"--output",
outputFile.getPath(),
"--verbose",
"--dont-exit",
repoDir.getPath(),
"--path-include=" + testPathIncludes,
"--path-exclude=" + testPathExcludes,
});

ObjectMapper objectMapper = new ObjectMapper();
Expand Down Expand Up @@ -121,6 +150,8 @@ void it_transforms_webgoat_with_codeql() throws Exception {
"--sarif",
"src/test/resources/webgoat_v8.2.2_codeql.sarif",
"--dont-exit",
"--path-include=" + testPathIncludes,
"--path-exclude=" + testPathExcludes,
repoDir.getPath()
});

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -53,16 +53,18 @@ private List<Language> putInPreferredOrder(final List<Language> languages) {
return orderedLanguages;
}

@Override
public boolean supports(final Path file) {
return getPropertyFilePrefix(file.getFileName().toString()).isPresent();
}

@Override
public CodemodFileScanningResult visitFile(final CodemodInvocationContext context)
throws IOException {
Path path = context.path();
String fileName = path.getFileName().toString();
// The supports check will guarantee that this won't be empty
Optional<String> prefix = getPropertyFilePrefix(fileName);
if (prefix.isEmpty()) {
// doesn't look like a i18n properties file
return CodemodFileScanningResult.none();
}
return doVisitFile(context, path, prefix.get());
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -57,6 +57,7 @@ public static List<Class<? extends CodeChanger>> asList() {
SimplifyRestControllerAnnotationsCodemod.class,
SubstituteReplaceAllCodemod.class,
SonarXXECodemod.class,
andrecsilva marked this conversation as resolved.
Show resolved Hide resolved
SonarSQLInjectionCodemod.class,
SQLParameterizerCodemod.class,
SSRFCodemod.class,
StackTraceExposureCodemod.class,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,7 +1,9 @@
package io.codemodder.codemods;

import io.codemodder.*;
import java.nio.file.Path;
import java.util.List;
import java.util.Set;
import java.util.regex.Pattern;

/**
Expand All @@ -19,12 +21,12 @@
reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW)
public final class JSPScriptletXSSCodemod extends RegexFileChanger {

private IncludesExcludesPattern includesExcludesPattern;

public JSPScriptletXSSCodemod() {
super(
path -> path.getFileName().toString().toLowerCase().endsWith(".jsp"),
scriptlet,
true,
List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER));
super(scriptlet, true, List.of(DependencyGAV.OWASP_XSS_JAVA_ENCODER));
this.includesExcludesPattern =
new IncludesExcludesPattern.Default(Set.of("**.[jJ][sS][pP]"), Set.of());
}

@Override
Expand All @@ -34,8 +36,18 @@ public String getReplacementFor(final String matchingSnippet) {
return "<%=org.owasp.encoder.Encode.forHtml(" + codeWithinScriptlet + ")%>";
}

@Override
public boolean supports(final Path file) {
return file.getFileName().toString().toLowerCase().endsWith(".jsp");
}

private static final Pattern scriptlet =
Pattern.compile(
"<%(\\s*)=(\\s*)request(\\s*).(\\s*)get((Header|Parameter)(\\s*)\\((\\s*)\".*\"(\\s*)\\)|QueryString\\((\\s*)\\))(\\s*)%>",
Pattern.MULTILINE);

@Override
public IncludesExcludesPattern getIncludesExcludesPattern() {
return includesExcludesPattern;
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -126,7 +126,7 @@ private CodemodFileScanningResult processXml(final Path file, final List<Result>
}

/*
* Change contents of the {@code url} tag if it it uses an insecure protocol.
* Change contents of the {@code url} tag if it uses an insecure protocol.
*/
private static void handle(
final XMLEventReader xmlEventReader,
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -46,13 +46,16 @@ public SpringAbsoluteCookieTimeoutCodemod(
this.safeDuration = parseExistingValueFromLine(count, units);
}

@Override
public boolean supports(final Path file) {
return "application.properties".equalsIgnoreCase(file.getFileName().toString());
}

@Override
public CodemodFileScanningResult visitFile(final CodemodInvocationContext context)
throws IOException {
Path path = context.path();
if (!"application.properties".equalsIgnoreCase(path.getFileName().toString())) {
return CodemodFileScanningResult.none();
} else if (!inExpectedDir(context.codeDirectory().asPath().relativize(path))) {
if (!inExpectedDir(context.codeDirectory().asPath().relativize(path))) {
return CodemodFileScanningResult.none();
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -36,9 +36,6 @@ public VerbTamperingCodemod(final XPathStreamProcessor processor) {
public CodemodFileScanningResult visitFile(final CodemodInvocationContext context)
throws IOException {
Path file = context.path();
if (!"web.xml".equalsIgnoreCase(file.getFileName().toString())) {
return CodemodFileScanningResult.none();
}
try {
List<CodemodChange> changes = processWebXml(context, file);
return CodemodFileScanningResult.withOnlyChanges(changes);
Expand Down Expand Up @@ -107,4 +104,14 @@ public String getIndividualChangeDescription(final Path filePath, final CodemodC
public List<CodeTFReference> getReferences() {
return reporter.getReferences().stream().map(u -> new CodeTFReference(u, u)).toList();
}

@Override
public IncludesExcludesPattern getIncludesExcludesPattern() {
return new IncludesExcludesPattern.Default(Set.of("{,**/}[wW][eE][bB].[xX][mM][lL]"), Set.of());
}

@Override
public boolean supports(final Path file) {
return "web.xml".equalsIgnoreCase(file.getFileName().toString());
}
}
18 changes: 13 additions & 5 deletions framework/codemodder-base/src/main/java/io/codemodder/CLI.java
Original file line number Diff line number Diff line change
Expand Up @@ -344,16 +344,24 @@ public Integer call() throws IOException {

// get path includes/excludes
List<String> pathIncludes = this.pathIncludes;
List<String> pathExcludes = this.pathExcludes;

// If no path includes/excludes were specified, relegate filtering to each codemod
// This is indicated by the MatchesEverything type of IncludesExcludes
boolean useGlobalIncludesExcludes = pathIncludes != null || pathExcludes != null;
IncludesExcludes includesExcludes = IncludesExcludes.any();
if (pathIncludes == null) {
pathIncludes = defaultPathIncludes;
pathIncludes = List.of("**");
}

List<String> pathExcludes = this.pathExcludes;
if (pathExcludes == null) {
pathExcludes = defaultPathExcludes;
pathExcludes = List.of();
}

if (useGlobalIncludesExcludes) {
includesExcludes =
IncludesExcludes.withSettings(projectDirectory, pathIncludes, pathExcludes);
}
IncludesExcludes includesExcludes =
IncludesExcludes.withSettings(projectDirectory, pathIncludes, pathExcludes);

log.debug("including paths: {}", pathIncludes);
log.debug("excluding paths: {}", pathExcludes);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,21 @@ public interface CodeChanger {
/** A description of an individual change made by this codemod. */
String getIndividualChangeDescription(final Path filePath, final CodemodChange change);

/**
* A list of paths patterns requested or rejected by the codemod. Those patterns are treated as
* relative to the repository root. These patterns should follow the {@link
* java.nio.file.PathMatcher} specification. These patterns can be overridden by global patterns.
*/
default IncludesExcludesPattern getIncludesExcludesPattern() {
return IncludesExcludesPattern.getAnyMatcher();
}

/**
* A predicate which dictates if the file should be inspected by the codemod. This cannot be
* overridden and should always pass before executing the codemod.
*/
boolean supports(final Path file);

/**
* A lifecycle event that is called before any files are processed. This is a good place to short
* circuit if you don't have the necessary resources (e.g., SARIF).
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -3,6 +3,7 @@
import com.github.javaparser.ast.CompilationUnit;
import io.codemodder.codetf.UnfixedFinding;
import io.codemodder.javaparser.JavaParserChanger;
import java.nio.file.Path;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.List;
Expand All @@ -29,6 +30,17 @@ protected CompositeJavaParserChanger(final JavaParserChanger... changers) {
this.changers = Arrays.asList(Objects.requireNonNull(changers));
}

@Override
public IncludesExcludesPattern getIncludesExcludesPattern() {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This doesn't seem right, but not sure I have another proposal right now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ideally this would be an intersection of all the patterns. But I'm not sure we can calculate this with just the patterns themselves.

// The first changer will dictate which files this composition accepts
return changers.get(0).getIncludesExcludesPattern();
}

@Override
public boolean supports(final Path file) {
return changers.stream().anyMatch(c -> c.supports(file));
}

@Override
public CodemodFileScanningResult visit(
final CodemodInvocationContext context, final CompilationUnit cu) {
Expand All @@ -44,9 +56,4 @@ public CodemodFileScanningResult visit(

return CodemodFileScanningResult.from(changes, unfixedFindings);
}

@Override
public boolean shouldRun() {
return changers.stream().anyMatch(CodeChanger::shouldRun);
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -82,9 +82,14 @@ public CodeTFResult execute(final List<Path> filePaths) {
if (codeChanger instanceof JavaParserChanger) {
codemodRunner =
new JavaParserCodemodRunner(
javaParserFacade, (JavaParserChanger) codeChanger, encodingDetector);
javaParserFacade,
(JavaParserChanger) codeChanger,
projectDir,
includesExcludes,
encodingDetector);
} else if (codeChanger instanceof RawFileChanger) {
codemodRunner = new RawFileCodemodRunner((RawFileChanger) codeChanger);
codemodRunner =
new RawFileCodemodRunner((RawFileChanger) codeChanger, projectDir, includesExcludes);
} else {
throw new UnsupportedOperationException(
"unsupported codeChanger type: " + codeChanger.getClass().getName());
Expand Down Expand Up @@ -366,13 +371,12 @@ private CodemodPackageUpdateResult addPackages(
dependencies.stream()
.filter(d -> !skippedDependencies.contains(d))
.forEach(
dep -> {
pkgActions.add(
new CodeTFPackageAction(
CodeTFPackageAction.CodeTFPackageActionType.ADD,
CodeTFPackageAction.CodeTFPackageActionResult.FAILED,
toPackageUrl(dep)));
});
dep ->
pkgActions.add(
new CodeTFPackageAction(
CodeTFPackageAction.CodeTFPackageActionType.ADD,
CodeTFPackageAction.CodeTFPackageActionResult.FAILED,
toPackageUrl(dep))));
return CodemodPackageUpdateResult.from(pkgActions, pkgChanges, unscannableFiles);
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -166,6 +166,7 @@ private static boolean noPatternsSpecified(
return includePatterns.isEmpty() && excludePatterns.isEmpty();
}

/** Matches every file, indicates that the codemods should use their own filtering. */
class MatchesEverything implements IncludesExcludes {
@Override
public boolean shouldInspect(final File file) {
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,77 @@
package io.codemodder;

import java.nio.file.Path;
import java.util.*;

/** Holds includes and excludes patterns for files */
public interface IncludesExcludesPattern {

/**
* Returns an IncludesExcludes file matcher that matches files following the patterns specified
* patterns. The patterns must follow the java's {@link java.nio.file.PathMatcher} specification
* and are treated as relative paths with regard to the repository root.
*/
IncludesExcludes getRootedMatcher(final Path root);

class Default implements IncludesExcludesPattern {

private final Set<String> pathIncludes;
private final Set<String> pathExcludes;

public Default(final Set<String> pathIncludes, final Set<String> pathExcludes) {
andrecsilva marked this conversation as resolved.
Show resolved Hide resolved
this.pathIncludes = Objects.requireNonNull(pathIncludes);
this.pathExcludes = Objects.requireNonNull(pathExcludes);
}

@Override
public String toString() {
return "Includes: " + pathIncludes + "\nExcludes: " + pathExcludes;
}

@Override
public IncludesExcludes getRootedMatcher(final Path root) {
return IncludesExcludes.withSettings(
root.toFile(), pathIncludes.stream().toList(), pathExcludes.stream().toList());
}
}

/** An includes/excludes pattern that matches java files. */
final class JavaMatcherSingleton {
private static IncludesExcludesPattern singleton;

private JavaMatcherSingleton() {}

static IncludesExcludesPattern getInstance() {
if (singleton == null) {
singleton =
new IncludesExcludesPattern.Default(
Set.of("**.[jJ][aA][vV][Aa]"), Collections.emptySet());
}
return singleton;
}
}

/** An includes/excludes pattern that matches any files. */
final class AnySingleton {
private static IncludesExcludesPattern singleton;

private AnySingleton() {}

static IncludesExcludesPattern getInstance() {
if (singleton == null) {
singleton = new IncludesExcludesPattern.Default(Set.of("**"), Collections.emptySet());
}
return singleton;
}
}

/** Returns an includes/excludes pattern that matches any java files. */
static IncludesExcludesPattern getJavaMatcher() {
return JavaMatcherSingleton.getInstance();
}

/** Returns an includes/excludes pattern that matches any files. */
static IncludesExcludesPattern getAnyMatcher() {
return AnySingleton.getInstance();
}
}
Loading
Loading