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 all 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());
}
}
59 changes: 41 additions & 18 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,23 @@ 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
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 Expand Up @@ -431,19 +438,35 @@ public Integer call() throws IOException {
FileCache fileCache = FileCache.createDefault(maxFileCacheSize);

for (CodemodIdPair codemod : codemods) {
CodemodExecutor codemodExecutor =
new DefaultCodemodExecutor(
projectPath,
includesExcludes,
codemod,
projectProviders,
codeTFProviders,
fileCache,
javaParserFacade,
encodingDetector,
maxFileSize,
maxFiles,
maxWorkers);
CodemodExecutor codemodExecutor;
if (useGlobalIncludesExcludes) {
codemodExecutor =
new DefaultCodemodExecutor(
projectPath,
includesExcludes,
codemod,
projectProviders,
codeTFProviders,
fileCache,
javaParserFacade,
encodingDetector,
maxFileSize,
maxFiles,
maxWorkers);
} else {
codemodExecutor =
new DefaultCodemodExecutor(
projectPath,
codemod,
projectProviders,
codeTFProviders,
fileCache,
javaParserFacade,
encodingDetector,
maxFileSize,
maxFiles,
maxWorkers);
}

log.info("running codemod: {}", codemod.getId());
CodeTFResult result = codemodExecutor.execute(filePaths);
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);
}
}
Loading
Loading