[TEAMMATES#9152] Add more SpotBugs rules (TEAMMATES#11580)

build.gradle

@@ -9,7 +9,7 @@ apply plugin: "cz.habarta.typescript-generator"
 
 def checkstyleVersion = "8.33"
 def pmdVersion = "6.24.0"
-def spotbugsVersion = "4.0.6"
+def spotbugsVersion = "4.5.3"
 def jacocoVersion = "0.8.5"
 
 buildscript {
@@ -21,7 +21,7 @@ buildscript {
     }
     dependencies {
         classpath "com.google.cloud.tools:appengine-gradle-plugin:2.3.0"
-        classpath "gradle.plugin.com.github.spotbugs.snom:spotbugs-gradle-plugin:4.4.3"
+        classpath "com.github.spotbugs.snom:spotbugs-gradle-plugin:5.0.6"
         classpath("cz.habarta.typescript-generator:typescript-generator-gradle-plugin:2.24.612") {
             exclude group: "org.gradle"
         }
@@ -57,6 +57,7 @@ dependencies {
     implementation("com.google.guava:guava:30.1.1-jre")
     implementation(objectify)
     implementation("com.googlecode.owasp-java-html-sanitizer:owasp-java-html-sanitizer:20211018.2")
+    implementation("com.helger:ph-commons:9.5.1") // necessary to add SpotBugs suppression
     implementation("com.mailjet:mailjet-client:4.5.0")
     implementation("com.sendgrid:sendgrid-java:4.6.0")
     implementation("com.sun.jersey:jersey-client:1.19.4")

src/client/java/teammates/client/scripts/DataBundleRegenerator.java

@@ -28,6 +28,9 @@ private DataBundleRegenerator() {
 
     private static void regenerateDataBundleJson(File folder) throws IOException {
         File[] listOfFiles = folder.listFiles();
+        if (listOfFiles == null) {
+            listOfFiles = new File[] {};
+        }
         for (File file : listOfFiles) {
             if (!file.getName().endsWith(".json") || NON_DATA_BUNDLE_JSON.contains(file.getName())) {
                 continue;
@@ -69,6 +72,9 @@ private static void saveFile(String filePath, String content) throws IOException
 
     private static void regenerateWebsiteDataJson() throws IOException {
         File[] listOfFiles = new File("./src/main/webapp/data").listFiles();
+        if (listOfFiles == null) {
+            listOfFiles = new File[] {};
+        }
         for (File file : listOfFiles) {
             if (!file.getName().endsWith(".json")) {
                 continue;

src/client/java/teammates/client/scripts/DataMigrationEntitiesBaseScript.java

@@ -19,6 +19,7 @@
 import com.googlecode.objectify.cmd.Query;
 
 import teammates.client.connector.DatastoreClient;
+import teammates.common.util.Const;
 import teammates.storage.entity.BaseEntity;
 import teammates.test.FileHelper;
 
@@ -266,7 +267,7 @@ protected void log(String logLine) {
         Path logPath = Paths.get(BASE_LOG_URI + this.getClass().getSimpleName() + ".log");
         try (OutputStream logFile = Files.newOutputStream(logPath,
                 StandardOpenOption.CREATE, StandardOpenOption.WRITE, StandardOpenOption.APPEND)) {
-            logFile.write((logLine + System.lineSeparator()).getBytes());
+            logFile.write((logLine + System.lineSeparator()).getBytes(Const.ENCODING));
         } catch (Exception e) {
             System.err.println("Error writing log line: " + logLine);
             System.err.println(e.getMessage());

src/client/java/teammates/client/scripts/statistics/FileStore.java

@@ -31,6 +31,7 @@
 import com.google.gson.stream.JsonWriter;
 
 import teammates.common.util.Config;
+import teammates.common.util.Const;
 import teammates.common.util.StringHelper;
 import teammates.test.FileHelper;
 
@@ -138,7 +139,7 @@ private static <T> void saveEncryptedJsonToFile(String fileName, T object, Type
 
         try (OutputStream os = Files.newOutputStream(Paths.get(fileName))) {
             CipherOutputStream out = new CipherOutputStream(os, cipher);
-            JsonWriter writer = new JsonWriter(new OutputStreamWriter(out));
+            JsonWriter writer = new JsonWriter(new OutputStreamWriter(out, Const.ENCODING));
             getSerializer().toJson(object, typeOfObject, writer);
             writer.close();
             out.close();
@@ -152,7 +153,7 @@ private static <T> T parseEncryptedJsonFile(String fileName, CheckedFunction<Jso
 
         try (InputStream is = Files.newInputStream(Paths.get(fileName))) {
             CipherInputStream in = new CipherInputStream(is, cipher);
-            JsonReader reader = new JsonReader(new InputStreamReader(in));
+            JsonReader reader = new JsonReader(new InputStreamReader(in, Const.ENCODING));
             T result = parser.apply(reader);
             reader.close();
             in.close();

src/e2e/java/teammates/e2e/pageobjects/AdminSearchPage.java

@@ -408,10 +408,10 @@ public void verifyAccountRequestRowContent(AccountRequestAttributes accountReque
         assertEquals(accountRequest.getEmail(), actualEmail);
         assertEquals(accountRequest.getInstitute(), actualInstitute);
         assertFalse(actualCreatedAt.isBlank());
-        if (actualRegisteredAt == null) {
+        if (accountRequest.getRegisteredAt() == null) {
             assertEquals("Not Registered Yet", actualRegisteredAt);
         } else {
-            assertFalse(actualCreatedAt.isBlank());
+            assertFalse(actualRegisteredAt.isBlank());
         }
     }
 

src/e2e/java/teammates/e2e/util/GmailServiceMaker.java

@@ -24,6 +24,8 @@
 import com.google.api.services.gmail.Gmail;
 import com.google.api.services.gmail.GmailScopes;
 
+import teammates.common.util.Const;
+
 /**
  * Class that builds a Gmail service for use in Gmail API.
  */
@@ -87,7 +89,7 @@ private Credential authorizeAndCreateCredentials() throws IOException {
 
     private GoogleClientSecrets loadClientSecretFromJson() throws IOException {
         try (InputStream in = Files.newInputStream(Paths.get(TestProperties.TEST_GMAIL_API_FOLDER, "client_secret.json"))) {
-            return GoogleClientSecrets.load(JSON_FACTORY, new InputStreamReader(in));
+            return GoogleClientSecrets.load(JSON_FACTORY, new InputStreamReader(in, Const.ENCODING));
         } catch (FileNotFoundException e) {
             throw new RuntimeException("You need to set up your Gmail API credentials." + System.lineSeparator()
                     + "See docs/development.md section \"Deploying to a staging server\".", e);

src/e2e/java/teammates/e2e/util/TestDataValidityTest.java

@@ -20,6 +20,8 @@
 import teammates.test.BaseTestCase;
 import teammates.test.FileHelper;
 
+import edu.umd.cs.findbugs.annotations.SuppressFBWarnings;
+
 /**
  * Checks for test data validity.
  *
@@ -40,6 +42,8 @@
 public class TestDataValidityTest extends BaseTestCase {
 
     @Test
+    @SuppressFBWarnings("RCN_REDUNDANT_NULLCHECK_WOULD_HAVE_BEEN_A_NPE")
+    // SpotBugs false positive: https://github.com/spotbugs/spotbugs/issues/1694
     public void checkTestDataValidity() throws IOException {
         Map<String, List<String>> errors = new HashMap<>();
         try (Stream<Path> paths = Files.walk(Paths.get(TestProperties.TEST_DATA_FOLDER))) {
@@ -52,6 +56,9 @@ public void checkTestDataValidity() throws IOException {
                     errors.put(pathString, Collections.singletonList("Error reading file: " + e.getMessage()));
                     return;
                 }
+                if (path.getFileName() == null) {
+                    return;
+                }
 
                 String testPage = path.getFileName().toString().replace("E2ETest.json", "");
                 DataBundle dataBundle = JsonUtils.fromJson(jsonString, DataBundle.class);

src/lnp/java/teammates/lnp/cases/BaseLNPTestCase.java

@@ -321,6 +321,9 @@ protected void cleanupResults() throws IOException {
                 .listFiles((d, s) -> {
                     return s.contains(this.getClass().getSimpleName());
                 });
+        if (fileList == null) {
+            fileList = new File[] {};
+        }
         Arrays.sort(fileList, (a, b) -> {
             return b.getName().compareTo(a.getName());
         });

src/lnp/java/teammates/lnp/cases/InstructorStudentCascadingUpdateLNPTest.java

@@ -61,8 +61,8 @@ public class InstructorStudentCascadingUpdateLNPTest extends BaseLNPTestCase {
     private static final double MEAN_RESP_TIME_LIMIT = 60;
 
     // To generate multiple csv files for multiple sections
-    private static int csvTestDataIndex;
-    private static LNPTestData testData;
+    private int csvTestDataIndex;
+    private LNPTestData testData;
 
     @Override
     protected LNPTestData getTestData() {

src/main/java/teammates/common/datatransfer/questions/FeedbackContributionQuestionDetails.java

@@ -373,5 +373,21 @@ public ContributionStatisticsEntry(int claimed, int perceived, Map<String, Integ
             this.claimedOthers = claimedOthers;
             this.perceivedOthers = perceivedOthers;
         }
+
+        public int getClaimed() {
+            return claimed;
+        }
+
+        public int getPerceived() {
+            return perceived;
+        }
+
+        public Map<String, Integer> getClaimedOthers() {
+            return claimedOthers;
+        }
+
+        public int[] getPerceivedOthers() {
+            return perceivedOthers;
+        }
     }
 }

src/main/java/teammates/common/datatransfer/questions/FeedbackRubricQuestionDetails.java

@@ -157,7 +157,8 @@ public List<String> validateResponsesDetails(List<FeedbackResponseDetails> respo
             }
 
             if (details.getAnswer().stream().anyMatch(choice ->
-                    choice != RUBRIC_ANSWER_NOT_CHOSEN
+                    choice == null
+                            || choice != RUBRIC_ANSWER_NOT_CHOSEN
                             && (choice < 0 || choice >= rubricChoices.size()))) {
                 errors.add(RUBRIC_INVALID_ANSWER);
             }

src/main/java/teammates/common/util/Config.java

@@ -3,6 +3,7 @@
 import java.io.IOException;
 import java.io.InputStream;
 import java.util.Arrays;
+import java.util.Collections;
 import java.util.List;
 import java.util.Properties;
 
@@ -124,8 +125,10 @@ public final class Config {
         OAUTH2_CLIENT_SECRET = properties.getProperty("app.oauth2.client.secret");
         ENABLE_DEVSERVER_LOGIN = Boolean.parseBoolean(properties.getProperty("app.enable.devserver.login", "true"));
         CAPTCHA_SECRET_KEY = properties.getProperty("app.captcha.secretkey");
-        APP_ADMINS = Arrays.asList(properties.getProperty("app.admins", "").split(","));
-        APP_MAINTAINERS = Arrays.asList(properties.getProperty("app.maintainers", "").split(","));
+        APP_ADMINS = Collections.unmodifiableList(
+                Arrays.asList(properties.getProperty("app.admins", "").split(",")));
+        APP_MAINTAINERS = Collections.unmodifiableList(
+                Arrays.asList(properties.getProperty("app.maintainers", "").split(",")));
         SUPPORT_EMAIL = properties.getProperty("app.crashreport.email");
         EMAIL_SENDEREMAIL = properties.getProperty("app.email.senderemail");
         EMAIL_SENDERNAME = properties.getProperty("app.email.sendername");

src/main/java/teammates/common/util/Const.java

@@ -1,5 +1,7 @@
 package teammates.common.util;
 
+import java.nio.charset.Charset;
+import java.nio.charset.StandardCharsets;
 import java.time.Duration;
 import java.time.Instant;
 
@@ -27,7 +29,7 @@ public final class Const {
     public static final String UNKNOWN_INSTITUTION = "Unknown Institution";
 
     public static final String DEFAULT_TIME_ZONE = "UTC";
-    public static final String ENCODING = "UTF8";
+    public static final Charset ENCODING = StandardCharsets.UTF_8;
 
     public static final Duration FEEDBACK_SESSIONS_SEARCH_WINDOW = Duration.ofDays(30);
     public static final Duration LOGS_RETENTION_PERIOD = Duration.ofDays(30);

src/main/java/teammates/common/util/SanitizationHelper.java

@@ -1,6 +1,5 @@
 package teammates.common.util;
 
-import java.io.UnsupportedEncodingException;
 import java.net.URLEncoder;
 
 import org.owasp.html.HtmlPolicyBuilder;
@@ -34,7 +33,6 @@ public final class SanitizationHelper {
                 .allowElements("quote", "ecode")
                 .allowStyling()
                 .toFactory();
-    private static final Logger log = Logger.getLogger();
 
     private SanitizationHelper() {
         // utility class
@@ -128,13 +126,7 @@ public static String sanitizeForHtml(String unsanitizedString) {
      * Converts a string to be put in URL (replaces some characters).
      */
     public static String sanitizeForUri(String uri) {
-        try {
-            return URLEncoder.encode(uri, Const.ENCODING).replaceAll("\\+", "%20");
-        } catch (UnsupportedEncodingException wontHappen) {
-            log.warning("Unexpected UnsupportedEncodingException in "
-                        + "SanitizationHelper.sanitizeForUri(" + uri + ", " + Const.ENCODING + ")");
-            return uri;
-        }
+        return URLEncoder.encode(uri, Const.ENCODING).replaceAll("\\+", "%20");
     }
 
 }

src/main/java/teammates/common/util/StringHelper.java

@@ -89,7 +89,7 @@ public static String generateSignature(String data) {
                     new SecretKeySpec(hexStringToByteArray(Config.ENCRYPTION_KEY), "HmacSHA1");
             Mac mac = Mac.getInstance("HmacSHA1");
             mac.init(signingKey);
-            byte[] value = mac.doFinal(data.getBytes());
+            byte[] value = mac.doFinal(data.getBytes(Const.ENCODING));
             return byteArrayToHexString(value);
         } catch (Exception e) {
             assert false;
@@ -123,7 +123,7 @@ public static String encrypt(String value) {
             SecretKeySpec sks = new SecretKeySpec(hexStringToByteArray(Config.ENCRYPTION_KEY), "AES");
             Cipher cipher = Cipher.getInstance("AES/ECB/PKCS5Padding");
             cipher.init(Cipher.ENCRYPT_MODE, sks, cipher.getParameters());
-            byte[] encrypted = cipher.doFinal(value.getBytes());
+            byte[] encrypted = cipher.doFinal(value.getBytes(Const.ENCODING));
             return byteArrayToHexString(encrypted);
         } catch (Exception e) {
             assert false;
@@ -145,7 +145,7 @@ public static String decrypt(String message) throws InvalidParametersException {
             Cipher cipher = Cipher.getInstance("AES/ECB/PKCS5Padding");
             cipher.init(Cipher.DECRYPT_MODE, sks);
             byte[] decrypted = cipher.doFinal(hexStringToByteArray(message));
-            return new String(decrypted);
+            return new String(decrypted, Const.ENCODING);
         } catch (NumberFormatException | IllegalBlockSizeException | BadPaddingException e) {
             log.warning("Attempted to decrypt invalid ciphertext: " + message);
             throw new InvalidParametersException(e);

src/main/java/teammates/logic/core/GoogleCloudTasksService.java

@@ -1,7 +1,6 @@
 package teammates.logic.core;
 
 import java.io.IOException;
-import java.nio.charset.Charset;
 import java.time.Instant;
 
 import com.google.cloud.tasks.v2.AppEngineHttpRequest;
@@ -49,7 +48,7 @@ public void addDeferredTask(TaskWrapper task, long countdownTime) {
                 String requestBody = JsonUtils.toCompactJson(task.getRequestBody());
                 requestBuilder.putHeaders("Content-Type", "application/json; charset=UTF-8")
                         .setRelativeUri(task.getWorkerUrl())
-                        .setBody(ByteString.copyFrom(requestBody, Charset.forName(Const.ENCODING)));
+                        .setBody(ByteString.copyFrom(requestBody, Const.ENCODING));
             }
 
             Task.Builder taskBuilder = Task.newBuilder().setAppEngineHttpRequest(requestBuilder.build());

src/main/java/teammates/logic/core/LocalTaskQueueService.java

@@ -3,7 +3,6 @@
 import java.io.IOException;
 import java.net.URI;
 import java.net.URISyntaxException;
-import java.nio.charset.Charset;
 import java.util.ArrayList;
 import java.util.List;
 import java.util.Map;
@@ -42,7 +41,7 @@ public void addDeferredTask(TaskWrapper task, long countdownTime) {
 
         if (task.getRequestBody() != null) {
             StringEntity entity = new StringEntity(
-                    JsonUtils.toCompactJson(task.getRequestBody()), Charset.forName(Const.ENCODING));
+                    JsonUtils.toCompactJson(task.getRequestBody()), Const.ENCODING);
             post.setEntity(entity);
         }
 

src/main/java/teammates/ui/servlets/DevServerLoginServlet.java

@@ -26,6 +26,8 @@ public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOExc
         if (nextUrl == null) {
             nextUrl = "/";
         }
+        // Prevent HTTP response splitting
+        nextUrl = resp.encodeRedirectURL(nextUrl.replace("\r\n", ""));
         if (!Config.isDevServerLoginEnabled()) {
             resp.setStatus(HttpServletResponse.SC_MOVED_PERMANENTLY);
             resp.setHeader("Location", Const.WebPageURIs.LOGIN + "?nextUrl=" + nextUrl.replace("&", "%26"));
@@ -66,6 +68,8 @@ public void doPost(HttpServletRequest req, HttpServletResponse resp) throws IOEx
         if (nextUrl == null) {
             nextUrl = "/";
         }
+        // Prevent HTTP response splitting
+        nextUrl = resp.encodeRedirectURL(nextUrl.replace("\r\n", ""));
         resp.sendRedirect(nextUrl);
     }
 

src/main/java/teammates/ui/servlets/LoginServlet.java

@@ -30,6 +30,8 @@ public void doGet(HttpServletRequest req, HttpServletResponse resp) throws IOExc
         if (nextUrl == null) {
             nextUrl = "/";
         }
+        // Prevent HTTP response splitting
+        nextUrl = resp.encodeRedirectURL(nextUrl.replace("\r\n", ""));
         if (Config.isDevServerLoginEnabled()) {
             resp.setStatus(HttpStatus.SC_MOVED_PERMANENTLY);
             resp.setHeader("Location", "/devServerLogin?nextUrl=" + nextUrl.replace("&", "%26"));

src/main/java/teammates/ui/servlets/OAuth2CallbackServlet.java

@@ -80,8 +80,8 @@ protected void doGet(HttpServletRequest req, HttpServletResponse resp) throws IO
 
             Map<String, Object> parsedResponse =
                     JsonUtils.fromJson(userInfoResponse, new TypeToken<Map<String, Object>>(){}.getType());
-            String email = String.valueOf(parsedResponse.get("email"));
-            if (email != null) {
+            if (parsedResponse.containsKey("email")) {
+                String email = String.valueOf(parsedResponse.get("email"));
                 googleId = email.replaceFirst("@gmail\\.com$", "");
             }
         } catch (URISyntaxException | IOException | JsonSyntaxException e) {

src/main/java/teammates/ui/webapi/DatastoreBackupAction.java

@@ -3,7 +3,6 @@
 import java.io.BufferedReader;
 import java.io.IOException;
 import java.io.InputStreamReader;
-import java.nio.charset.Charset;
 import java.time.Instant;
 import java.util.HashMap;
 import java.util.Map;
@@ -64,12 +63,13 @@ public JsonResult execute() {
         // Documentation is wrong; the param name is output_url_prefix instead of outputUrlPrefix
         body.put("output_url_prefix", "gs://" + Config.BACKUP_GCS_BUCKETNAME + "/datastore-backups/" + timestamp);
 
-        StringEntity entity = new StringEntity(JsonUtils.toCompactJson(body), Charset.forName(Const.ENCODING));
+        StringEntity entity = new StringEntity(JsonUtils.toCompactJson(body), Const.ENCODING);
         post.setEntity(entity);
 
         try (CloseableHttpClient client = HttpClients.createDefault();
                 CloseableHttpResponse resp = client.execute(post);
-                BufferedReader br = new BufferedReader(new InputStreamReader(resp.getEntity().getContent()))) {
+                BufferedReader br = new BufferedReader(
+                        new InputStreamReader(resp.getEntity().getContent(), Const.ENCODING))) {
             String output = br.lines().collect(Collectors.joining(System.lineSeparator()));
             if (resp.getStatusLine().getStatusCode() == HttpStatus.SC_OK) {
                 log.info("Backup request successful:" + System.lineSeparator() + output);