Skip to content

Commit e1f2346

Browse files
Suraj-kumar00Suraj-kumar00
committed
fix: address CodeRabbit security and code quality feedback
- Simplify /health to return minimal response {status: UP/DOWN} - Make Maven properties path configurable via @value annotation - Remove exception message exposure in JWT filter error response - Remove unused methods and imports
1 parent 860e4ce commit e1f2346

3 files changed

Lines changed: 33 additions & 99 deletions

File tree

‎src/main/java/com/iemr/admin/service/health/HealthService.java‎

Lines changed: 28 additions & 97 deletions
Original file line numberDiff line numberDiff line change
@@ -24,7 +24,6 @@
2424
import java.sql.Connection;
2525
import java.sql.PreparedStatement;
2626
import java.sql.ResultSet;
27-
import java.time.Instant;
2827
import java.util.HashMap;
2928
import java.util.Map;
3029

@@ -33,7 +32,6 @@
3332
import org.slf4j.Logger;
3433
import org.slf4j.LoggerFactory;
3534
import org.springframework.beans.factory.annotation.Autowired;
36-
import org.springframework.beans.factory.annotation.Value;
3735
import org.springframework.data.redis.core.RedisCallback;
3836
import org.springframework.data.redis.core.RedisTemplate;
3937
import org.springframework.stereotype.Service;
@@ -47,142 +45,75 @@ public class HealthService {
4745
@Autowired
4846
private DataSource dataSource;
4947

50-
@Value("${app.version:unknown}")
51-
private String appVersion;
52-
5348
@Autowired(required = false)
5449
private RedisTemplate<String, Object> redisTemplate;
5550

5651
public Map<String, Object> checkHealth() {
5752
Map<String, Object> healthStatus = new HashMap<>();
58-
Map<String, Object> services = new HashMap<>();
5953
boolean overallHealth = true;
6054

61-
// Check database connectivity
62-
Map<String, Object> dbStatus = checkDatabaseHealth();
63-
services.put("database", dbStatus);
64-
if (!"UP".equals(dbStatus.get("status"))) {
55+
// Check database connectivity (details logged internally, not exposed)
56+
boolean dbHealthy = checkDatabaseHealthInternal();
57+
if (!dbHealthy) {
6558
overallHealth = false;
6659
}
6760

68-
// Check Redis connectivity if configured
61+
// Check Redis connectivity if configured (details logged internally)
6962
if (redisTemplate != null) {
70-
Map<String, Object> redisStatus = checkRedisHealth();
71-
services.put("redis", redisStatus);
72-
if (!"UP".equals(redisStatus.get("status"))) {
63+
boolean redisHealthy = checkRedisHealthInternal();
64+
if (!redisHealthy) {
7365
overallHealth = false;
7466
}
75-
} else {
76-
Map<String, Object> redisStatus = new HashMap<>();
77-
redisStatus.put("status", "NOT_CONFIGURED");
78-
redisStatus.put("message", "Redis not configured for this environment");
79-
services.put("redis", redisStatus);
8067
}
8168

8269
healthStatus.put("status", overallHealth ? "UP" : "DOWN");
83-
healthStatus.put("services", services);
84-
healthStatus.put("timestamp", Instant.now().toString());
85-
healthStatus.put("application", "admin-api");
86-
healthStatus.put("version", appVersion);
8770

8871
logger.info("Health check completed - Overall status: {}", overallHealth ? "UP" : "DOWN");
8972
return healthStatus;
9073
}
9174

92-
private Map<String, Object> checkDatabaseHealth() {
93-
Map<String, Object> dbStatus = new HashMap<>();
75+
private boolean checkDatabaseHealthInternal() {
9476
long startTime = System.currentTimeMillis();
9577

9678
try (Connection connection = dataSource.getConnection()) {
97-
// Test connection validity
98-
boolean isConnectionValid = connection.isValid(5); // 5 second timeout
79+
boolean isConnectionValid = connection.isValid(2); // 2 second timeout per best practices
9980

10081
if (isConnectionValid) {
101-
// Execute a simple query to ensure database is responsive
102-
try (PreparedStatement stmt = connection.prepareStatement(DB_HEALTH_CHECK_QUERY);
103-
ResultSet rs = stmt.executeQuery()) {
104-
105-
if (rs.next() && rs.getInt(1) == 1) {
106-
long responseTime = System.currentTimeMillis() - startTime;
107-
108-
dbStatus.put("status", "UP");
109-
dbStatus.put("responseTime", responseTime + "ms");
110-
dbStatus.put("message", "Database connection successful");
111-
112-
logger.debug("Database health check: UP ({}ms)", responseTime);
113-
} else {
114-
dbStatus.put("status", "DOWN");
115-
dbStatus.put("message", "Database query returned unexpected result");
116-
logger.warn("Database health check: Query returned unexpected result");
82+
try (PreparedStatement stmt = connection.prepareStatement(DB_HEALTH_CHECK_QUERY)) {
83+
stmt.setQueryTimeout(3); // 3 second query timeout
84+
try (ResultSet rs = stmt.executeQuery()) {
85+
if (rs.next() && rs.getInt(1) == 1) {
86+
long responseTime = System.currentTimeMillis() - startTime;
87+
logger.debug("Database health check: UP ({}ms)", responseTime);
88+
return true;
89+
}
11790
}
11891
}
119-
} else {
120-
dbStatus.put("status", "DOWN");
121-
dbStatus.put("message", "Database connection is not valid");
122-
logger.warn("Database health check: Connection is not valid");
12392
}
124-
93+
logger.warn("Database health check: Connection not valid");
94+
return false;
12595
} catch (Exception e) {
126-
long responseTime = System.currentTimeMillis() - startTime;
127-
dbStatus.put("status", "DOWN");
128-
dbStatus.put("responseTime", responseTime + "ms");
129-
dbStatus.put("error", e.getClass().getSimpleName());
130-
131-
logger.error("Database health check failed: {}", e.getMessage(), e);
96+
logger.error("Database health check failed: {}", e.getMessage());
97+
return false;
13298
}
133-
134-
return dbStatus;
13599
}
136100

137-
private Map<String, Object> checkRedisHealth() {
138-
Map<String, Object> redisStatus = new HashMap<>();
101+
private boolean checkRedisHealthInternal() {
139102
long startTime = System.currentTimeMillis();
140103

141104
try {
142-
// Test Redis connection with ping using explicit RedisCallback
143-
String pong = redisTemplate.execute((RedisCallback<String>) connection -> {
144-
return connection.ping();
145-
});
105+
String pong = redisTemplate.execute((RedisCallback<String>) connection -> connection.ping());
146106

147107
if ("PONG".equals(pong)) {
148108
long responseTime = System.currentTimeMillis() - startTime;
149-
150-
// Additional test: set and get a test key
151-
String testKey = "health:check:" + System.currentTimeMillis();
152-
String testValue = "test-value";
153-
154-
redisTemplate.opsForValue().set(testKey, testValue);
155-
Object retrievedValue = redisTemplate.opsForValue().get(testKey);
156-
redisTemplate.delete(testKey); // Clean up test key
157-
158-
if (testValue.equals(retrievedValue)) {
159-
redisStatus.put("status", "UP");
160-
redisStatus.put("responseTime", responseTime + "ms");
161-
redisStatus.put("message", "Redis connection and operations successful");
162-
redisStatus.put("ping", "PONG");
163-
164-
logger.debug("Redis health check: UP ({}ms)", responseTime);
165-
} else {
166-
redisStatus.put("status", "DOWN");
167-
redisStatus.put("message", "Redis set/get operation failed");
168-
logger.warn("Redis health check: Set/Get operation failed");
169-
}
170-
} else {
171-
redisStatus.put("status", "DOWN");
172-
redisStatus.put("message", "Redis ping returned: " + pong);
173-
logger.warn("Redis health check: Ping returned unexpected response: {}", pong);
109+
logger.debug("Redis health check: UP ({}ms)", responseTime);
110+
return true;
174111
}
175-
112+
logger.warn("Redis health check: Ping returned unexpected response");
113+
return false;
176114
} catch (Exception e) {
177-
long responseTime = System.currentTimeMillis() - startTime;
178-
redisStatus.put("status", "DOWN");
179-
redisStatus.put("message", "Redis connection failed");
180-
redisStatus.put("responseTime", responseTime + "ms");
181-
redisStatus.put("error", e.getClass().getSimpleName());
182-
183-
logger.error("Redis health check failed", e);
115+
logger.error("Redis health check failed: {}", e.getMessage());
116+
return false;
184117
}
185-
186-
return redisStatus;
187118
}
188119
}

‎src/main/java/com/iemr/admin/service/version/VersionService.java‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,9 @@ public class VersionService {
4545
@Value("${app.version:unknown}")
4646
private String appVersion;
4747

48+
@Value("${maven.properties.path:META-INF/maven/com.iemr.admin/admin-api/pom.properties}")
49+
private String mavenPropertiesPath;
50+
4851
private final ObjectMapper objectMapper = new ObjectMapper();
4952

5053
/**
@@ -117,7 +120,7 @@ private void addBuildInformation(Map<String, String> versionInfo) {
117120
versionInfo.put("build.time", time);
118121
} else {
119122
logger.info("build-info.properties not found, trying Maven properties");
120-
Properties mavenProps = loadPropertiesFile("META-INF/maven/com.iemr.admin/admin-api/pom.properties");
123+
Properties mavenProps = loadPropertiesFile(mavenPropertiesPath);
121124
if (mavenProps != null) {
122125
String version = mavenProps.getProperty("version", appVersion);
123126
versionInfo.put("build.version", version);

‎src/main/java/com/iemr/admin/utils/JwtUserIdValidationFilter.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,7 @@ public void doFilter(ServletRequest servletRequest, ServletResponse servletRespo
131131

132132
} catch (Exception e) {
133133
logger.error("Authorization error: ", e);
134-
response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Authorization error: " + e.getMessage());
134+
response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Unauthorized: Invalid or missing token");
135135
}
136136
}
137137

0 commit comments

Comments
 (0)