diff --git a/its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json b/its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json index b93a181759d..dc94d5d1622 100644 --- a/its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json +++ b/its/autoscan/src/test/resources/autoscan/autoscan-diff-by-rules.json @@ -2950,5 +2950,11 @@ "hasTruePositives": false, "falseNegatives": 5, "falsePositives": 0 + }, + { + "ruleKey": "S6863", + "hasTruePositives": false, + "falseNegatives": 6, + "falsePositives": 0 } ] diff --git a/its/autoscan/src/test/resources/autoscan/diffs/diff_S6863.json b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6863.json new file mode 100644 index 00000000000..6ca5c239569 --- /dev/null +++ b/its/autoscan/src/test/resources/autoscan/diffs/diff_S6863.json @@ -0,0 +1,6 @@ +{ + "ruleKey": "S6863", + "hasTruePositives": false, + "falseNegatives": 6, + "falsePositives": 0 +} diff --git a/java-checks-test-sources/default/src/main/java/checks/spring/StatusCodesOnResponseCheckSample.java b/java-checks-test-sources/default/src/main/java/checks/spring/StatusCodesOnResponseCheckSample.java new file mode 100644 index 00000000000..157152d1e7c --- /dev/null +++ b/java-checks-test-sources/default/src/main/java/checks/spring/StatusCodesOnResponseCheckSample.java @@ -0,0 +1,89 @@ +package checks.spring; + +import javax.ws.rs.NotFoundException; +import org.springframework.http.HttpStatus; +import org.springframework.http.ResponseEntity; +import org.springframework.stereotype.Controller; + +import static org.springframework.http.HttpStatus.INTERNAL_SERVER_ERROR; +import static org.springframework.http.HttpStatus.OK; + +public class StatusCodesOnResponseCheckSample { + + class User { + } + + @Controller + class UserController { + + public ResponseEntity getOkUser() { + return ResponseEntity.ok(new User()); // Compliant + } + + public ResponseEntity getBadReqUser() { + return ResponseEntity.badRequest().build(); // Compliant + } + + public ResponseEntity getUserNoncompliant() { + + try { + return ResponseEntity.status(INTERNAL_SERVER_ERROR).body(new User()); // Noncompliant [[sc=16;ec=60]] {{Set a HttpStatus code reflective of the operation.}} + } catch (NotFoundException e) { + return ResponseEntity.status(OK).build(); // Noncompliant [[sc=16;ec=41]] {{Set a HttpStatus code reflective of the operation.}} + } catch (Exception e) { + return ResponseEntity.status(HttpStatus.NOT_FOUND).build(); // Compliant + } + } + + public ResponseEntity getUserNoncompliant2() { + try { + return ResponseEntity.badRequest().build(); // Noncompliant [[sc=16;ec=43]] {{Set a HttpStatus code reflective of the operation.}} + } catch (NotFoundException e) { + return ResponseEntity.ok().build(); // Noncompliant + } catch (Exception e) { + return ResponseEntity.status(getHttpStatus()).build(); // Compliant + } + } + + public ResponseEntity getUserNoncompliant3() { + try { + return ResponseEntity.notFound().build(); // Noncompliant + } catch (NotFoundException e) { + return ResponseEntity.ok().build(); // Noncompliant + } catch (Exception e) { + return ResponseEntity.notFound().build(); // Compliant + } + } + + public ResponseEntity getUser() { + return ResponseEntity.status(OK).build(); // Compliant + } + + public ResponseEntity getUser2() { + return ResponseEntity.status(INTERNAL_SERVER_ERROR).build(); // Compliant + } + + public ResponseEntity getUserCompliant() { + try { + return ResponseEntity.ok(new User()); // Compliant + } catch (NotFoundException e) { + return ResponseEntity.status(HttpStatus.NOT_FOUND).build(); // Compliant + } catch (Exception e) { + return ResponseEntity.status(INTERNAL_SERVER_ERROR).build(); // Compliant + } + } + + public ResponseEntity getUserCompliant2() { + try { + return ResponseEntity.status(OK).build(); // Compliant + } catch (NotFoundException e) { + return ResponseEntity.status(HttpStatus.NOT_FOUND).build(); // Compliant + } + } + } + + private HttpStatus getHttpStatus() { + return HttpStatus.NOT_FOUND; + } + +} diff --git a/java-checks/src/main/java/org/sonar/java/checks/spring/StatusCodesOnResponseCheck.java b/java-checks/src/main/java/org/sonar/java/checks/spring/StatusCodesOnResponseCheck.java new file mode 100644 index 00000000000..21f28de607e --- /dev/null +++ b/java-checks/src/main/java/org/sonar/java/checks/spring/StatusCodesOnResponseCheck.java @@ -0,0 +1,233 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2023 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks.spring; + +import java.util.List; +import java.util.stream.Stream; +import org.sonar.check.Rule; +import org.sonar.java.model.ExpressionUtils; +import org.sonar.plugins.java.api.IssuableSubscriptionVisitor; +import org.sonar.plugins.java.api.semantic.MethodMatchers; +import org.sonar.plugins.java.api.tree.BaseTreeVisitor; +import org.sonar.plugins.java.api.tree.ClassTree; +import org.sonar.plugins.java.api.tree.ExpressionTree; +import org.sonar.plugins.java.api.tree.IdentifierTree; +import org.sonar.plugins.java.api.tree.MemberSelectExpressionTree; +import org.sonar.plugins.java.api.tree.MethodInvocationTree; +import org.sonar.plugins.java.api.tree.Tree; + +@Rule(key = "S6863") +public class StatusCodesOnResponseCheck extends IssuableSubscriptionVisitor { + + public static final String RESPONSE_ENTITY = "org.springframework.http.ResponseEntity"; + public static final String ISSUE_MESSAGE = "Set a HttpStatus code reflective of the operation."; + + /* + * Values for the okCodes list are extracted from: + * https://docs.spring.io/spring-framework/docs/5.0.6.RELEASE/javadoc-api/index.html?org/springframework/http/HttpStatus.html + * by taking all that return 1xx, 2xx, 3xx code. + */ + private static final List OK_CODES = List.of("ACCEPTED", + "ALREADY_REPORTED", + "CHECKPOINT", + "CONTINUE", + "CREATED", + "FOUND", + "IM_USED", + "MOVED_PERMANENTLY", + "MULTIPLE_CHOICES", + "MULTI_STATUS", + "NON_AUTHORITATIVE_INFORMATION", + "NOT_MODIFIED", + "NO_CONTENT", + "OK", + "PARTIAL_CONTENT", + "PERMANENT_REDIRECT", + "PROCESSING", + "RESET_CONTENT", + "SEE_OTHER", + "SWITCHING_PROTOCOLS", + "TEMPORARY_REDIRECT"); + + /* + * Values for the errorCodes list are extracted from: + * https://docs.spring.io/spring-framework/docs/5.0.6.RELEASE/javadoc-api/index.html?org/springframework/http/HttpStatus.html + * by taking all that return 4xx, 5xx code. + */ + private static final List ERROR_CODES = List.of("BAD_GATEWAY", + "BAD_REQUEST", + "BANDWIDTH_LIMIT_EXCEEDED", + "CONFLICT", + "EXPECTATION_FAILED", + "FAILED_DEPENDENCY", + "FORBIDDEN", + "GATEWAY_TIMEOUT", + "GONE", + "HTTP_VERSION_NOT_SUPPORTED", + "INSUFFICIENT_STORAGE", + "INTERNAL_SERVER_ERROR", + "I_AM_A_TEAPOT", + "LENGTH_REQUIRED", + "LOCKED", + "LOOP_DETECTED", + "METHOD_NOT_ALLOWED", + "NETWORK_AUTHENTICATION_REQUIRED", + "NOT_ACCEPTABLE", + "NOT_EXTENDED", + "NOT_FOUND", + "NOT_IMPLEMENTED", + "PAYLOAD_TOO_LARGE", + "PAYMENT_REQUIRED", + "PRECONDITION_FAILED", + "PRECONDITION_REQUIRED", + "PROXY_AUTHENTICATION_REQUIRED", + "REQUESTED_RANGE_NOT_SATISFIABLE", + "REQUEST_HEADER_FIELDS_TOO_LARGE", + "REQUEST_TIMEOUT", + "SERVICE_UNAVAILABLE", + "TOO_MANY_REQUESTS", + "UNAUTHORIZED", + "UNAVAILABLE_FOR_LEGAL_REASONS", + "UNPROCESSABLE_ENTITY", + "UNSUPPORTED_MEDIA_TYPE", + "UPGRADE_REQUIRED", + "URI_TOO_LONG", + "VARIANT_ALSO_NEGOTIATES"); + + private static final MethodMatchers STATUS_METHOD_MATCHERS = MethodMatchers.create() + .ofTypes(RESPONSE_ENTITY) + .names("status") + .addParametersMatcher("org.springframework.http.HttpStatus") + .build(); + + private static final MethodMatchers OK_METHOD_MATCHERS = MethodMatchers.create() + .ofTypes(RESPONSE_ENTITY) + .names("ok", "created", "accepted", "noContent") + .withAnyParameters() + .build(); + + private static final MethodMatchers ERROR_METHODS_MATCHER = MethodMatchers.create() + .ofTypes(RESPONSE_ENTITY) + .names("badRequest", "notFound", "unprocessableEntity") + .addWithoutParametersMatcher() + .build(); + + @Override + public List nodesToVisit() { + return List.of(Tree.Kind.CLASS); + } + + @Override + public void visitNode(Tree tree) { + ClassTree classTree = (ClassTree) tree; + if (!isClassController(classTree)) { + return; + } + + MethodInvocationVisitor methodInvocationVisitor = new MethodInvocationVisitor(); + classTree.accept(methodInvocationVisitor); + } + + private class MethodInvocationVisitor extends BaseTreeVisitor { + + @Override + public void visitMethodInvocation(MethodInvocationTree methodInvocationTree) { + + if (STATUS_METHOD_MATCHERS.matches(methodInvocationTree)) { + checkTryCatch(methodInvocationTree); + return; + } + + if (OK_METHOD_MATCHERS.matches(methodInvocationTree)) { + Tree catchParent = checkCatch(methodInvocationTree, false); + if (catchParent == null) { + checkTry(methodInvocationTree, true); + } + return; + } + + if (ERROR_METHODS_MATCHER.matches(methodInvocationTree)) { + Tree catchParent = checkCatch(methodInvocationTree, true); + if (catchParent == null) { + checkTry(methodInvocationTree, false); + } + return; + } + + super.visitMethodInvocation(methodInvocationTree); + } + + private void checkTryCatch(MethodInvocationTree methodInvocationTree) { + Tree catchParent = ExpressionUtils.getParentOfType(methodInvocationTree, Tree.Kind.CATCH); + boolean isError = isCodeInList(methodInvocationTree, ERROR_CODES); + + if (catchParent != null && !isError) { + reportIssue(methodInvocationTree, ISSUE_MESSAGE); + return; + } + + if (catchParent == null) { + Tree tryParent = ExpressionUtils.getParentOfType(methodInvocationTree, Tree.Kind.TRY_STATEMENT); + boolean isOk = isCodeInList(methodInvocationTree, OK_CODES); + + if (tryParent != null && !isOk) { + reportIssue(methodInvocationTree, ISSUE_MESSAGE); + } + } + } + + private boolean isCodeInList(MethodInvocationTree methodInvocationTree, List codes) { + ExpressionTree arg = methodInvocationTree.arguments().get(0); + if (arg.is(Tree.Kind.MEMBER_SELECT)) { + MemberSelectExpressionTree memberSelectExpressionTree = (MemberSelectExpressionTree) arg; + return codes.contains(memberSelectExpressionTree.identifier().name()); + } else if (arg.is(Tree.Kind.IDENTIFIER)) { + IdentifierTree identifierTree = (IdentifierTree) arg; + return codes.contains(identifierTree.name()); + } + return true; + } + + private Tree checkCatch(MethodInvocationTree methodInvocationTree, boolean isError) { + Tree catchParent = ExpressionUtils.getParentOfType(methodInvocationTree, Tree.Kind.CATCH); + + if (catchParent != null && !isError) { + reportIssue(methodInvocationTree, ISSUE_MESSAGE); + } + return catchParent; + } + + private Tree checkTry(MethodInvocationTree methodInvocationTree, boolean isOk) { + Tree tryParent = ExpressionUtils.getParentOfType(methodInvocationTree, Tree.Kind.TRY_STATEMENT); + + if (tryParent != null && !isOk) { + reportIssue(methodInvocationTree, ISSUE_MESSAGE); + } + return tryParent; + } + + } + + private static boolean isClassController(ClassTree classTree) { + return Stream.of("org.springframework.stereotype.Controller", "org.springframework.web.bind.annotation.RestController") + .anyMatch(annotation -> classTree.symbol().metadata().isAnnotatedWith(annotation)); + } + +} diff --git a/java-checks/src/test/java/org/sonar/java/checks/spring/StatusCodesOnResponseCheckTest.java b/java-checks/src/test/java/org/sonar/java/checks/spring/StatusCodesOnResponseCheckTest.java new file mode 100644 index 00000000000..8acfe8e4df4 --- /dev/null +++ b/java-checks/src/test/java/org/sonar/java/checks/spring/StatusCodesOnResponseCheckTest.java @@ -0,0 +1,36 @@ +/* + * SonarQube Java + * Copyright (C) 2012-2023 SonarSource SA + * mailto:info AT sonarsource DOT com + * + * This program is free software; you can redistribute it and/or + * modify it under the terms of the GNU Lesser General Public + * License as published by the Free Software Foundation; either + * version 3 of the License, or (at your option) any later version. + * + * This program is distributed in the hope that it will be useful, + * but WITHOUT ANY WARRANTY; without even the implied warranty of + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU + * Lesser General Public License for more details. + * + * You should have received a copy of the GNU Lesser General Public License + * along with this program; if not, write to the Free Software Foundation, + * Inc., 51 Franklin Street, Fifth Floor, Boston, MA 02110-1301, USA. + */ +package org.sonar.java.checks.spring; + +import org.junit.jupiter.api.Test; +import org.sonar.java.checks.verifier.CheckVerifier; +import org.sonar.java.checks.verifier.TestUtils; + +class StatusCodesOnResponseCheckTest { + + @Test + void test() { + CheckVerifier.newVerifier() + .onFile(TestUtils.mainCodeSourcesPath("checks/spring/StatusCodesOnResponseCheckSample.java")) + .withCheck(new StatusCodesOnResponseCheck()) + .verifyIssues(); + } + +} diff --git a/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java b/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java index dd3707be2bd..88f392128f9 100644 --- a/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java +++ b/sonar-java-plugin/src/main/java/org/sonar/plugins/java/CheckList.java @@ -587,6 +587,7 @@ import org.sonar.java.checks.spring.SpringScanDefaultPackageCheck; import org.sonar.java.checks.spring.SpringSecurityDisableCSRFCheck; import org.sonar.java.checks.spring.SpringSessionFixationCheck; +import org.sonar.java.checks.spring.StatusCodesOnResponseCheck; import org.sonar.java.checks.spring.SuperfluousResponseBodyAnnotationCheck; import org.sonar.java.checks.spring.TransactionalMethodVisibilityCheck; import org.sonar.java.checks.spring.ValueAnnotationShouldInjectPropertyOrSpELCheck; @@ -1039,6 +1040,7 @@ public final class CheckList { ServletInstanceFieldCheck.class, ServletMethodsExceptionsThrownCheck.class, ShiftOnIntOrLongCheck.class, + StatusCodesOnResponseCheck.class, UnnecessaryBitOperationCheck.class, SillyEqualsCheck.class, SillyStringOperationsCheck.class, diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6863.html b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6863.html new file mode 100644 index 00000000000..f4920daa71c --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6863.html @@ -0,0 +1,56 @@ +

Why is this an issue?

+

The request handler function in a Controller should set the appropriate HTTP status code based on the operation’s success or failure. +This is done by returning a Response object with the appropriate status code.

+

If an exception is thrown during the execution of the handler, the status code should be in the range of 4xx or 5xx. Examples of such codes are +BAD_REQUEST, UNAUTHORIZED, FORBIDDEN, NOT_FOUND, INTERNAL_SERVER_ERROR, +BAD_GATEWAY, SERVICE_UNAVAILABLE, etc.

+

The status code should be 1xx, 2xx, or 3xx if no exception is thrown and the operation is considered successful. Such codes include +OK, CREATED, MOVED_PERMANENTLY, FOUND, etc.

+

How to fix it

+

Code examples

+

Noncompliant code example

+
+@Controller
+public class UserController {
+    public ResponseEntity<User> getUserById(Long userId) {
+        try {
+            User user = userService.getUserById(userId);
+            return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).body(user); // Noncompliant: Setting 500 for a successful operation
+        } catch (Exception e) {
+            return ResponseEntity.status(HttpStatus.OK).build(); // Noncompliant: Setting 200 for exception
+        }
+    }
+}
+
+

Compliant solution

+
+@Controller
+public class UserController {
+    public ResponseEntity<User> getUserById(Long userId) {
+        try {
+            User user = userService.getUserById(userId);
+            return ResponseEntity.ok(user); // Compliant: Setting 200 for a successful operation
+        } catch (Exception e) {
+            return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR).build(); // Compliant: Setting 500 for exception
+        }
+    }
+}
+
+

Resources

+

Documentation

+ +

Standards

+ + diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6863.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6863.json new file mode 100644 index 00000000000..35cc9df7640 --- /dev/null +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/S6863.json @@ -0,0 +1,24 @@ +{ + "title": "Set appropriate Status Codes on HTTP responses", + "type": "BUG", + "status": "ready", + "remediation": { + "func": "Constant\/Issue", + "constantCost": "5min" + }, + "tags": [ + "spring", + "best-practice" + ], + "defaultSeverity": "Major", + "ruleSpecification": "RSPEC-6863", + "sqKey": "S6863", + "scope": "Main", + "quickfix": "infeasible", + "code": { + "impacts": { + "RELIABILITY": "LOW" + }, + "attribute": "DISTINCT" + } +} diff --git a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json index a3dd5ba01a7..9fe54c3a5f3 100644 --- a/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json +++ b/sonar-java-plugin/src/main/resources/org/sonar/l10n/java/rules/java/Sonar_way_profile.json @@ -498,6 +498,7 @@ "S6838", "S6856", "S6857", - "S6862" + "S6862", + "S6863" ] }