From 5fa7ac458ec225cf58396d015ebb9aa6a538062d Mon Sep 17 00:00:00 2001 From: Jonathan Pryor Date: Fri, 27 Jan 2023 15:06:47 -0500 Subject: [PATCH] [java-source-utils] Fix lgtm java/path-injection-local (#1079) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Context: e11d0242441210ac006d86ccf21c4d4dc2db3a28 Commit e11d0242 attempted to fix LGTM-reported [`java/path-injection-local`][0] warnings by using the comment `// lgtm [java/path-injection-local]`. Unfortunately, this is insufficient: the comment *also* needs to provide a 25+ character justification for why the offending statement can be ignored. This justification was not provided. Update the `// lgtm [java/path-injection-local]` comments to provide a justification, as required by tooling. Copying the longer justification from e11d0242: > LGTM is complaining that `tools/java-source-utils` (69e1b80a) accepts > user-controlled data. These warnings will be *ignored* because the > app is *unusable* without "user-controlled data" > … > These are all user-controlled, and they are necessary to allow > `java-source-utils` to *work*. > … > LGTM complains that `--output-javadoc FILE` accepts a user-controlled > path which may [contain] directory separator chars, and > *this is intentional*; using it would be annoying if that weren't true! See also [`JavaSourceUtils.cs`][1], which passes [a value][2] located within `$(IntermediateOutputPath)` to `java-source-utils.jar --output-javadoc`. Allowing `--output-javadoc` to contain directory separator chars is what makes this possible! [0]: https://github.com/github/codeql/blob/f192191e8c4c14d70a86342de47c8882516c7c25/java/ql/src/Security/CWE/CWE-022/TaintedPath.qhelp [1]: https://github.com/xamarin/xamarin-android/blob/b00185c485287c2c5f0350a067ebc178aec2382c/src/Xamarin.Android.Build.Tasks/Tasks/JavaSourceUtils.cs#L134-L135 [2]: https://github.com/xamarin/xamarin-android/blob/b00185c485287c2c5f0350a067ebc178aec2382c/src/Xamarin.Android.Build.Tasks/MSBuild/Xamarin/Android/Xamarin.Android.Bindings.ClassParse.targets#L69 --- .../com/microsoft/android/JavaSourceUtilsOptions.java | 8 ++++---- .../java/com/microsoft/android/JavadocXmlGenerator.java | 2 +- 2 files changed, 5 insertions(+), 5 deletions(-) diff --git a/tools/java-source-utils/src/main/java/com/microsoft/android/JavaSourceUtilsOptions.java b/tools/java-source-utils/src/main/java/com/microsoft/android/JavaSourceUtilsOptions.java index 6a453d669..f110c170d 100644 --- a/tools/java-source-utils/src/main/java/com/microsoft/android/JavaSourceUtilsOptions.java +++ b/tools/java-source-utils/src/main/java/com/microsoft/android/JavaSourceUtilsOptions.java @@ -167,7 +167,7 @@ private final JavaSourceUtilsOptions parse(Iterator args) throws IOExcep final String bootClassPath = getNextOptionValue(args, arg); final ArrayList files = new ArrayList(); for (final String cp : bootClassPath.split(File.pathSeparator)) { - final File file = new File(cp); // lgtm [java/path-injection-local] + final File file = new File(cp); // lgtm [java/path-injection-local] java-source-utils.jar is a command-line app, and is useless if it doesn't support command-line args. if (!file.exists()) { System.err.println(App.APP_NAME + ": warning: invalid file path for option `-bootclasspath`: " + cp); continue; @@ -253,7 +253,7 @@ private final JavaSourceUtilsOptions parse(Iterator args) throws IOExcep if (arg.startsWith("@")) { // response file? final String responseFileName = arg.substring(1); - final File responseFile = new File(responseFileName); // lgtm [java/path-injection-local] + final File responseFile = new File(responseFileName); // lgtm [java/path-injection-local] java-source-utils.jar is a command-line app, and is useless if it doesn't support command-line args. if (responseFile.exists()) { final Iterator lines = Files.readAllLines(responseFile.toPath()) @@ -267,7 +267,7 @@ private final JavaSourceUtilsOptions parse(Iterator args) throws IOExcep break; } } - final File file = new File(arg); // lgtm [java/path-injection-local] + final File file = new File(arg); // lgtm [java/path-injection-local] java-source-utils.jar is a command-line app, and is useless if it doesn't support command-line args. if (!file.exists()) { System.err.println(App.APP_NAME + ": warning: invalid file path for option `FILES`: " + arg); break; @@ -347,7 +347,7 @@ static File getNextOptionFile(final Iterator args, final String option) throw new IllegalArgumentException( "Expected required value for option `" + option + "`."); final String fileName = args.next(); - final File file = new File(fileName); // lgtm [java/path-injection-local] + final File file = new File(fileName); // lgtm [java/path-injection-local] java-source-utils.jar is a command-line app, and is useless if it doesn't support command-line args. if (!file.exists()) { System.err.println(App.APP_NAME + ": warning: invalid file path for option `" + option + "`: " + fileName); return null; diff --git a/tools/java-source-utils/src/main/java/com/microsoft/android/JavadocXmlGenerator.java b/tools/java-source-utils/src/main/java/com/microsoft/android/JavadocXmlGenerator.java index 21f3f6251..ba1246819 100644 --- a/tools/java-source-utils/src/main/java/com/microsoft/android/JavadocXmlGenerator.java +++ b/tools/java-source-utils/src/main/java/com/microsoft/android/JavadocXmlGenerator.java @@ -39,7 +39,7 @@ public JavadocXmlGenerator(final String output) throws FileNotFoundException, Pa if (output == null) this.output = System.out; else { - final File file = new File(output); // lgtm [java/path-injection-local] + final File file = new File(output); // lgtm [java/path-injection-local] java-source-utils.jar is a command-line app, and is useless if it doesn't support command-line args. final File parent = file.getParentFile(); if (parent != null) { parent.mkdirs();