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

AMM- 1049 | Assam Redis Issue #31

Merged
merged 10 commits into from
Nov 4, 2024
4 changes: 2 additions & 2 deletions .github/workflows/build-on-pull-request.yml
Original file line number Diff line number Diff line change
Expand Up @@ -13,10 +13,10 @@ jobs:
ref: ${{ github.event.pull_request.head.ref }}
repository: ${{ github.event.pull_request.head.repo.full_name }}

- name: Setup JDK 8
- name: Setup JDK 17
uses: actions/setup-java@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update actions/setup-java to v3.

The current version (@v2) is outdated. GitHub Actions recommends using the latest version for improved features and security.

Apply this diff:

-        uses: actions/setup-java@v2
+        uses: actions/setup-java@v3
πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
uses: actions/setup-java@v2
uses: actions/setup-java@v3
🧰 Tools
πŸͺ› actionlint

17-17: the runner of "actions/setup-java@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

with:
java-version: 8
java-version: 17
distribution: 'adopt'
- name: Build with Maven
run: mvn clean install
6 changes: 3 additions & 3 deletions .github/workflows/package.yml
Original file line number Diff line number Diff line change
Expand Up @@ -22,10 +22,10 @@ jobs:
- name: Checkout code
uses: actions/checkout@v2

- name: Setup JDK 8
- name: Setup JDK 17
uses: actions/setup-java@v2
with:
java-version: 8
java-version: 17
distribution: 'adopt'

Comment on lines +25 to 30
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update actions/setup-java to v3

The action actions/setup-java@v2 is outdated. Version 3 is now available with improved features and security updates.

Apply this diff to upgrade:

    - name: Setup JDK 17
-     uses: actions/setup-java@v2
+     uses: actions/setup-java@v3
      with:
        java-version: 17
        distribution: 'adopt'
πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
- name: Setup JDK 17
uses: actions/setup-java@v2
with:
java-version: 8
java-version: 17
distribution: 'adopt'
- name: Setup JDK 17
uses: actions/setup-java@v3
with:
java-version: 17
distribution: 'adopt'
🧰 Tools
πŸͺ› actionlint

26-26: the runner of "actions/setup-java@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

πŸͺ› yamllint

[error] 30-30: trailing spaces

(trailing-spaces)

- name: Build with Maven
Expand All @@ -35,7 +35,7 @@ jobs:
run: mvn -B package --file pom.xml

- name: Upload WAR file as artifact
uses: actions/upload-artifact@v2
uses: actions/upload-artifact@v3
with:
name: Helpline104-API
path: target/104api-v1.0.war
4 changes: 2 additions & 2 deletions .github/workflows/sast.yml
Original file line number Diff line number Diff line change
Expand Up @@ -37,10 +37,10 @@ jobs:
- name: Initialize CodeQL
uses: github/codeql-action/init@v2

- name: Setup JDK 8
- name: Setup JDK 17
uses: actions/setup-java@v2
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Update actions/setup-java to v3.

The current version (v2) of the setup-java action is outdated. Version 3 includes security updates and performance improvements.

Apply this diff to update the action:

-    - name: Setup JDK 17
-      uses: actions/setup-java@v2
+    - name: Setup JDK 17
+      uses: actions/setup-java@v3

Committable suggestion was skipped due to low confidence.

🧰 Tools
πŸͺ› actionlint

41-41: the runner of "actions/setup-java@v2" action is too old to run on GitHub Actions. update the action's version to fix this issue

(action)

with:
java-version: 8
java-version: 17
distribution: 'adopt'

- name: Build with Maven
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -29,96 +29,86 @@
import org.springframework.web.servlet.HandlerInterceptor;
import org.springframework.web.servlet.ModelAndView;

import com.iemr.helpline104.utils.redis.RedisStorage;
import com.iemr.helpline104.utils.response.OutputResponse;
import com.iemr.helpline104.utils.sessionobject.SessionObject;

import jakarta.servlet.http.HttpServletRequest;
import jakarta.servlet.http.HttpServletResponse;
import jakarta.ws.rs.core.MediaType;
import com.iemr.helpline104.utils.validator.Validator;


@Component
public class HTTPRequestInterceptor implements HandlerInterceptor {
Logger logger = LoggerFactory.getLogger(this.getClass().getName());
@Autowired
private RedisStorage redisStorage;
private final Validator validator;

Logger logger = LoggerFactory.getLogger(this.getClass().getSimpleName());

@Autowired
public HTTPRequestInterceptor(Validator validator) {
this.validator = validator;
}

private SessionObject sessionObject;

@Autowired
public void setSessionObject(SessionObject sessionObject) {
this.sessionObject = sessionObject;
}
Comment on lines +54 to +57
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ› οΈ Refactor suggestion

Consider Using Constructor Injection for SessionObject Dependency

Similarly, applying constructor injection for the SessionObject dependency improves the clarity of mandatory dependencies and enhances the class's immutability.

Modify the class constructor to include SessionObject:

 public class HTTPRequestInterceptor implements HandlerInterceptor {
 	private final Validator validator;
+	private final SessionObject sessionObject;

 	@Autowired
-	public HTTPRequestInterceptor(Validator validator) {
+	public HTTPRequestInterceptor(Validator validator, SessionObject sessionObject) {
 		this.validator = validator;
+		this.sessionObject = sessionObject;
 	}

-	@Autowired
-	public void setSessionObject(SessionObject sessionObject) {
-		this.sessionObject = sessionObject;
-	}
πŸ“ Committable suggestion

‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.

Suggested change
@Autowired
public void setSessionObject(SessionObject sessionObject) {
this.sessionObject = sessionObject;
}
@Autowired
public HTTPRequestInterceptor(Validator validator, SessionObject sessionObject) {
this.validator = validator;
this.sessionObject = sessionObject;
}


@Override
public boolean preHandle(HttpServletRequest request, HttpServletResponse response, Object object) throws Exception {
logger.info("http interceptor - pre Handle");
boolean status = true;

if (request.getRequestURI().toLowerCase().contains("swagger-ui"))
return status;

String authorization = null;
String preAuth = request.getHeader("Authorization");
if(null != preAuth && preAuth.contains("Bearer "))
authorization=preAuth.replace("Bearer ", "");
else
authorization = preAuth;
logger.debug("In preHandle we are Intercepting the Request");
String authorization = request.getHeader("Authorization");
logger.debug("RequestURI::" + request.getRequestURI() + " || method :: " + request.getMethod());
if (!request.getMethod().equalsIgnoreCase("OPTIONS")) {
try {
String[] requestURIParts = request.getRequestURI().split("/");
String requestAPI = requestURIParts[requestURIParts.length - 1];
switch (requestAPI) {
case "userAuthenticate":
case "userAuthenticateNew":
case "userAuthenticateV1":
case "forgetPassword":
case "setForgetPassword":
case "changePassword":
case "saveUserSecurityQuesAns":
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ› οΈ Refactor suggestion

Consider using an enum or configuration for authentication endpoints.

The growing switch statement with hardcoded authentication-related endpoints could become difficult to maintain. Consider moving these endpoints to an enum or configuration file.

Example implementation:

public enum AuthExemptEndpoint {
    USER_AUTHENTICATE("userAuthenticate"),
    USER_AUTHENTICATE_NEW("userAuthenticateNew"),
    USER_AUTHENTICATE_V1("userAuthenticateV1"),
    FORGET_PASSWORD("forgetPassword"),
    SET_FORGET_PASSWORD("setForgetPassword"),
    CHANGE_PASSWORD("changePassword"),
    SAVE_SECURITY_QUESTIONS("saveUserSecurityQuesAns");

    private final String path;

    AuthExemptEndpoint(String path) {
        this.path = path;
    }

    public static boolean isExempt(String path) {
        return Arrays.stream(values())
            .anyMatch(endpoint -> endpoint.path.equals(path));
    }
}

case "swagger-ui.html":
break;
case "index.html":
break;
case "swagger-initializer.js":
break;
case "swagger-config":
break;
case "ui":
break;
case "swagger-resources":
break;
case "api-docs":
break;

break;
case "error":
status = false;
break;
default:
logger.debug("RequestURI::" + request.getRequestURI() + " || Authorization ::" + authorization);
if (authorization == null)
throw new Exception(
"Authorization key is NULL, please pass valid session key to proceed further. ");
String userRespFromRedis = sessionObject.getSessionObject(authorization);
if (userRespFromRedis == null)
throw new Exception("invalid Authorization key, please pass a valid key to proceed further. ");
String remoteAddress = request.getHeader("X-FORWARDED-FOR");
if (remoteAddress == null || remoteAddress.trim().length() == 0) {
remoteAddress = request.getRemoteAddr();
}

validator.checkKeyExists(authorization, remoteAddress);
Comment on lines +79 to +84
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Validate 'X-FORWARDED-FOR' Header to Prevent Spoofing

The X-FORWARDED-FOR header can be manipulated by clients to spoof their IP addresses. Relying on it without validation may introduce security vulnerabilities.

Consider validating the IP address or using a trusted proxy for fetching the client's real IP. Alternatively, rely on request.getRemoteAddr() if the server is properly configured.

break;
}
} catch (Exception e) {
logger.error(e.getLocalizedMessage());

OutputResponse output = new OutputResponse();
output.setError(e);
response.getOutputStream().print(output.toString());
response.setContentType(MediaType.APPLICATION_JSON);
response.setContentLength(output.toString().length());
response.setHeader("Access-Control-Allow-Origin", "*");
status = false;
Copy link
Contributor

Choose a reason for hiding this comment

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

⚠️ Potential issue

Handle Exceptions Securely Without Exposing Internal Details

Directly returning exception details to the client can expose sensitive information. Instead, log the exception internally and return a generic error message with an appropriate HTTP status code.

Refactor the exception handling code:

} catch (Exception e) {
+	logger.error("Error in preHandle method", e); // Log the exception internally
 	OutputResponse output = new OutputResponse();
-	output.setError(e);
+	output.setError("An unexpected error occurred.");
+	response.setStatus(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); // Set HTTP 500 status code
 	response.getOutputStream().print(output.toString());
 	response.setContentType(MediaType.APPLICATION_JSON);
 	response.setContentLength(output.toString().length());
 	status = false;
}

Committable suggestion was skipped due to low confidence.

}
Copy link
Contributor

Choose a reason for hiding this comment

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

πŸ› οΈ Refactor suggestion

Enhance error handling with structured logging and response codes

While the basic error handling is in place, consider these additional improvements:

  1. Add structured logging with error codes
  2. Include correlation IDs for request tracing
  3. Use specific HTTP status codes based on error type
 } catch (Exception e) {
+    String correlationId = generateCorrelationId(request);
+    logger.error("Request processing failed. correlationId={}, uri={}, error={}", 
+        correlationId, request.getRequestURI(), e.getMessage(), e);
     OutputResponse output = new OutputResponse();
-    output.setError(e);
+    output.setError("Request failed. Reference ID: " + correlationId);
+    int statusCode = determineHttpStatus(e);
+    response.setStatus(statusCode);
     response.getOutputStream().print(output.toString());
     response.setContentType(MediaType.APPLICATION_JSON);
     response.setContentLength(output.toString().length());
     status = false;
 }

Committable suggestion skipped: line range outside the PR's diff.

}

return status;
}

@Override
public void postHandle(HttpServletRequest request, HttpServletResponse response, Object object, ModelAndView model)
throws Exception {
try {
logger.debug("In postHandle we are Intercepting the Request");
String authorization = null;
String postAuth = request.getHeader("Authorization");
if(null != postAuth && postAuth.contains("Bearer "))
authorization=postAuth.replace("Bearer ", "");
else
authorization = postAuth;
String authorization = request.getHeader("Authorization");
logger.debug("RequestURI::" + request.getRequestURI() + " || Authorization ::" + authorization);
if (authorization != null) {
sessionObject.updateSessionObject(authorization, sessionObject.getSessionObject(authorization));
Expand All @@ -131,7 +121,9 @@ public void postHandle(HttpServletRequest request, HttpServletResponse response,
@Override
public void afterCompletion(HttpServletRequest request, HttpServletResponse response, Object object, Exception arg3)
throws Exception {
logger.info("http interceptor - after completion");

logger.debug("In afterCompletion Request Completed");
}



}
Loading