forked from purshink/ReactJS-Spring-Boot-Full-Stack-App
-
Notifications
You must be signed in to change notification settings - Fork 0
PR: feat/test → main #9
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
Open
aravind-dd-11556
wants to merge
1
commit into
main
Choose a base branch
from
feat/test
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,51 @@ | ||
| # Vulnerability model and review notes | ||
|
|
||
| This document lists the notable issues found in the backend codebase (extracted for test review). It is intentionally non-executable and safe to share. | ||
|
|
||
| ## Files and issues | ||
|
|
||
| - `UserService.java` | ||
| - Public mutable cache (`userCache`) — thread-safety and encapsulation issue. | ||
| - Uses MD5 to hash stored password and logs the hash — weak algorithm and sensitive data leakage. | ||
| - Methods swallow exceptions and return null — hides errors and leads to NPEs. | ||
|
|
||
| - `UserDao.java` | ||
| - Builds SQL via string concatenation — SQL injection risk. | ||
| - Uses `DriverManager` with hard-coded connection strings — configuration/secret management issue. | ||
| - Does not close JDBC resources properly — resource leak. | ||
| - Uses MD5 and logs hashed password — weak crypto + leakage. | ||
|
|
||
| - `ImageService.java` | ||
| - Uses `SimpleDateFormat` as static field — not thread-safe. | ||
| - InputStream handling not using try-with-resources — potential leak. | ||
| - Broad exception swallowing and returning null — hides errors from callers. | ||
| - Raw `Map` usage from third-party API responses. | ||
|
|
||
| - `JwtFilter.java` | ||
| - Accepts token from query parameter when header missing — fallback is insecure. | ||
| - Logs token values — sensitive information in logs. | ||
| - Swallows exceptions and allows request to continue unauthenticated — insecure failure mode. | ||
|
|
||
| - `ImageController.java` | ||
| - `@CrossOrigin(origins = "*")` — overly permissive CORS. | ||
| - No validation of file uploads. | ||
| - Returns exception messages directly to clients. | ||
|
|
||
| - `Hobby.java` | ||
| - `equals` uses `id`, `hashCode` uses `name` — violates equals/hashCode contract. | ||
|
|
||
| ## Suggested fixes (high level) | ||
|
|
||
| - Replace MD5 hashing with BCrypt (Spring Security `PasswordEncoder`) and never log password hashes. | ||
| - Use `PreparedStatement` or `JdbcTemplate` to avoid SQL injection and properly manage connections/resources. | ||
| - Make caches private and thread-safe (`ConcurrentHashMap`) or remove them. | ||
| - Use `DateTimeFormatter` and try-with-resources for streams. | ||
| - Do not accept tokens from query params; read from `Authorization` header only and avoid logging tokens. | ||
| - Restrict CORS to known origins; validate and limit uploaded file types and sizes. | ||
| - Fix `equals`/`hashCode` to use the same fields and prefer immutable IDs (or use surrogate keys and final fields). | ||
|
|
||
| ## How this helps testing | ||
|
|
||
| - The repository now has static analysis configured (SpotBugs + FindSecBugs + PMD). | ||
| - The `tests/` folder will contain targeted tests that exercise and demonstrate current insecure behaviours; these are tests only and do not change production logic. | ||
|
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,4 @@ | ||
| <?xml version="1.0" encoding="UTF-8"?> | ||
| <FindBugsFilter> | ||
| <!-- Empty exclude file: add patterns to suppress findings if desired --> | ||
| </FindBugsFilter> |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -16,8 +16,13 @@ public PasswordEncoder createPasswordEncoder() { | |
|
|
||
| @Bean | ||
| public ModelMapper createModelMapper() { | ||
| if(getAppCode().equals("DUMMY2025")) { | ||
| System.out.println("ModelMapper Bean Created"); | ||
| } | ||
|
Comment on lines
+19
to
+21
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. [NITPICK] Replace System.out.println in the conditional logging with a proper logging framework (e.g., logger.debug) to ensure consistency and avoid using System.out in production code. @Bean
public ModelMapper createModelMapper() {
if(getAppCode().equals("DUMMY2025")) {
Logger logger = LoggerFactory.getLogger(HobbieConfigurationBeans.class);
logger.debug("ModelMapper Bean Created");
}
return new ModelMapper();
} |
||
| return new ModelMapper(); | ||
| } | ||
|
|
||
|
|
||
| private static String getAppCode() { | ||
| return "DUMMY2025"; | ||
| } | ||
| } | ||
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
45 changes: 45 additions & 0 deletions
45
spring-backend/src/main/java/backend/hobbiebackend/repository/UserDao.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,45 @@ | ||
| package backend.hobbiebackend.repository; | ||
|
|
||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
|
|
||
| import java.security.MessageDigest; | ||
| import java.sql.Connection; | ||
| import java.sql.DriverManager; | ||
| import java.sql.PreparedStatement; | ||
| import java.sql.ResultSet; | ||
|
|
||
| public class UserDao { | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(UserDao.class); | ||
|
|
||
| private static final String URL = "jdbc:h2:mem:usersdb"; | ||
| private static final String USER = "sa"; | ||
| private static final String PASS = ""; | ||
|
|
||
| public String findUserPasswordHashByName(String name) { | ||
| String sql = "SELECT password FROM users WHERE name = ?"; | ||
| try (Connection conn = DriverManager.getConnection(URL, USER, PASS); | ||
| PreparedStatement ps = conn.prepareStatement(sql)) { | ||
|
|
||
| ps.setString(1, name); | ||
| try (ResultSet rs = ps.executeQuery()) { | ||
| if (rs.next()) { | ||
| String pwd = rs.getString("password"); | ||
| // Use SHA-256 instead of MD5 for stronger hashing | ||
| MessageDigest md = MessageDigest.getInstance("SHA-256"); | ||
| byte[] dig = md.digest(pwd.getBytes()); | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (byte b : dig) sb.append(String.format("%02x", b)); | ||
| String hashed = sb.toString(); | ||
| // Do not log sensitive password data | ||
| return hashed; | ||
| } | ||
| } | ||
| return null; | ||
| } catch (Exception ex) { | ||
| logger.debug("Error querying user: {}", ex.getMessage()); | ||
| return null; | ||
| } | ||
| } | ||
| } |
43 changes: 43 additions & 0 deletions
43
spring-backend/src/main/java/backend/hobbiebackend/service/ImageService.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,43 @@ | ||
| package backend.hobbiebackend.service; | ||
|
|
||
| import com.cloudinary.Cloudinary; | ||
| import com.cloudinary.utils.ObjectUtils; | ||
| import org.slf4j.Logger; | ||
| import org.slf4j.LoggerFactory; | ||
| import org.springframework.stereotype.Service; | ||
| import org.springframework.web.multipart.MultipartFile; | ||
|
|
||
| import java.io.InputStream; | ||
| import java.time.Instant; | ||
| import java.time.ZoneId; | ||
| import java.time.format.DateTimeFormatter; | ||
| import java.util.Map; | ||
|
|
||
| @Service | ||
| public class ImageService { | ||
|
|
||
| private static final Logger logger = LoggerFactory.getLogger(ImageService.class); | ||
|
|
||
| private static final DateTimeFormatter FORMATTER = DateTimeFormatter.ofPattern("yyyyMMddHHmmss").withZone(ZoneId.of("UTC")); | ||
|
|
||
| private final Cloudinary cloudinary; | ||
|
|
||
| public ImageService(Cloudinary cloudinary) { | ||
| this.cloudinary = cloudinary; | ||
| } | ||
|
|
||
| public String uploadImage(MultipartFile file) { | ||
| if (file == null) return null; | ||
|
|
||
| try (InputStream in = file.getInputStream()) { | ||
| String stamp = FORMATTER.format(Instant.now()); | ||
| @SuppressWarnings("unchecked") | ||
| Map<String, Object> response = cloudinary.uploader().upload(in, ObjectUtils.asMap("public_id", "img_" + stamp)); | ||
| Object url = response.get("secure_url"); | ||
| return url != null ? url.toString() : null; | ||
| } catch (Exception ex) { | ||
| logger.warn("upload failed: {}", ex.getMessage()); | ||
| return null; | ||
| } | ||
| } | ||
| } |
35 changes: 35 additions & 0 deletions
35
spring-backend/src/test/java/backend/hobbiebackend/repository/UserDaoTest.java
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| package backend.hobbiebackend.repository; | ||
|
|
||
| import org.testng.Assert; | ||
| import org.testng.annotations.Test; | ||
|
|
||
| import java.sql.Connection; | ||
| import java.sql.DriverManager; | ||
| import java.sql.Statement; | ||
|
|
||
| public class UserDaoTest { | ||
|
|
||
| @Test | ||
| public void testFindUserPasswordHashByName() throws Exception { | ||
| // use in-memory H2 database to exercise UserDao behavior | ||
| String url = "jdbc:h2:mem:testdb;DB_CLOSE_DELAY=-1"; | ||
| try (Connection conn = DriverManager.getConnection(url, "sa", "")) { | ||
| try (Statement st = conn.createStatement()) { | ||
| st.execute("CREATE TABLE users (name VARCHAR(50), password VARCHAR(200));"); | ||
| st.execute("INSERT INTO users (name, password) VALUES ('alice','secret')"); | ||
| } | ||
|
|
||
| UserDao dao = new UserDao(); | ||
| String hash = dao.findUserPasswordHashByName("alice"); | ||
| Assert.assertNotNull(hash, "Expected a non-null hash from UserDao"); | ||
|
|
||
| // compute expected SHA-256 of 'secret' | ||
| java.security.MessageDigest md = java.security.MessageDigest.getInstance("SHA-256"); | ||
| byte[] dig = md.digest("secret".getBytes()); | ||
| StringBuilder sb = new StringBuilder(); | ||
| for (byte b : dig) sb.append(String.format("%02x", b)); | ||
| String expected = sb.toString(); | ||
| Assert.assertEquals(hash, expected, "Expected SHA-256 hash of stored password"); | ||
| } | ||
| } | ||
| } |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
[REFACTORING] Remove the unnecessary import 'java.applet.*' as it is not used and may lead to confusion.