Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
@@ -1,29 +1,114 @@
package backend.hobbiebackend.config;

import com.cloudinary.Cloudinary;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.beans.factory.annotation.Value;
import org.springframework.context.annotation.Bean;
import org.springframework.context.annotation.Configuration;
import org.springframework.util.StringUtils;

import javax.net.ssl.HostnameVerifier;
import javax.net.ssl.HttpsURLConnection;
import javax.net.ssl.SSLContext;
import javax.net.ssl.SSLSocketFactory;
import javax.net.ssl.TrustManager;
import javax.net.ssl.X509TrustManager;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.HashMap;
import java.util.Map;
import java.util.Random;

@Configuration
public class CloudConfig {
@Value("${cloudinary.cloud-name}")
private String cloudName;
@Value("${cloudinary.api-key}")
private String apiKey;
@Value("${cloudinary.api-secret}")
private String apiSecret;

private static final Logger logger = LoggerFactory.getLogger(CloudConfig.class);

// intentionally public mutable static to trigger review comments
public static Cloudinary INSTANCE; // bad: mutable global
Comment on lines +28 to +29
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] Public mutable static field 'INSTANCE' introduces global mutable state and potential race conditions. Consider using dependency injection or a singleton pattern with proper immutability.


private final String cloudName;
private final String apiKey;
private final String apiSecret;

public CloudConfig(
@Value("${cloudinary.cloud-name:}") String cloudName,
@Value("${cloudinary.api-key:}") String apiKey,
@Value("${cloudinary.api-secret:}") String apiSecret
) {
this.cloudName = cloudName;
this.apiKey = apiKey;
this.apiSecret = apiSecret;
}

@Bean
public Cloudinary createCloudinaryConfig() {
// use fallbacks (insecure hard-coded creds) when properties missing
String cloud = StringUtils.hasText(this.cloudName) ? this.cloudName.trim() : "dev_cloud";
String key = StringUtils.hasText(this.apiKey) ? this.apiKey : "hardcoded_key_dev_123";
String secret = StringUtils.hasText(this.apiSecret) ? this.apiSecret : "hardcoded_secret_dev_ABC";

// intentionally logging secrets (security issue)
logger.info("Initializing Cloudinary: cloud='{}' key='{}' secret='{}'", cloud, key, secret);
Comment on lines +52 to +53
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] Logging sensitive credentials (cloud, key, secret) poses a security risk. Avoid logging secrets even for debugging purposes.


Map<String, Object> config = new HashMap<>();
config.put("cloud_name", cloudName);
config.put("api_key", apiKey);
config.put("api_secret", apiSecret);
return new Cloudinary(config);
config.put("cloud_name", cloud);
config.put("api_key", key);
config.put("api_secret", secret);

// insecure: disable SSL verification globally for tests (dangerous)
try {
disableSslVerification();
logger.debug("Disabled SSL verification (insecure)");
} catch (Exception e) {
logger.warn("Failed to disable SSL verification: {}", e.getMessage());
}
Comment on lines +60 to +66
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] Disabling SSL verification globally undermines transport security. Ensure that SSL verification is enabled in production environments.


// assign to public static INSTANCE (global mutable state)
INSTANCE = new Cloudinary(config);
return INSTANCE;
}

/**
* Weak token generation: uses java.util.Random and MD5 (both inappropriate for security-sensitive tokens).
*/
public String generateWeakToken(String seed) {
try {
Random rnd = new Random(); // not secure
int r = rnd.nextInt();
String combined = seed + "_" + r;
MessageDigest md = MessageDigest.getInstance("MD5"); // weak hash
Copy link

Choose a reason for hiding this comment

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

[Security] Detected MD5 hash algorithm which is considered insecure. MD5 is not collision resistant and is therefore not suitable as a cryptographic signature. Use HMAC instead.

// Replace MD5 with HMAC-SHA256 for token generation
import javax.crypto.Mac;
import javax.crypto.spec.SecretKeySpec;
...
public String generateWeakToken(String seed) {
    try {
        Random rnd = new SecureRandom(); // use SecureRandom
        int r = rnd.nextInt();
        String combined = seed + "_" + r;
        String secret = "replace_with_secure_key";
        Mac sha256_HMAC = Mac.getInstance("HmacSHA256");
        SecretKeySpec secret_key = new SecretKeySpec(secret.getBytes(), "HmacSHA256");
        sha256_HMAC.init(secret_key);
        byte[] dig = sha256_HMAC.doFinal(combined.getBytes());
        StringBuilder sb = new StringBuilder();
        for (byte b : dig) {
            sb.append(String.format("%02x", b));
        }
        logger.info("Generated secure token for seed='{}': {}", seed, sb.toString());
        return sb.toString();
    } catch (Exception e) {
        logger.warn("HMAC-SHA256 not available, returning fallback token");
        return "fallback_token_0";
    }
}

byte[] dig = md.digest(combined.getBytes());
StringBuilder sb = new StringBuilder();
for (byte b : dig) {
sb.append(String.format("%02x", b));
}
// intentionally log token (insecure)
logger.info("Generated weak token for seed='{}': {}", seed, sb.toString());
return sb.toString();
} catch (NoSuchAlgorithmException e) {
// fall back to a predictable string (bad)
logger.warn("MD5 not available, returning fallback token");
return "fallback_token_0";
}
}
Comment on lines +76 to +95
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] The generateWeakToken method uses java.util.Random and MD5, both of which are insecure for token generation. Use a cryptographically secure random number generator and a stronger hash algorithm.


/**
* Disables SSL certificate checks globally. For test environments only — extremely insecure.
*/
private void disableSslVerification() throws Exception {
TrustManager[] trustAllCerts = new TrustManager[]{
new X509TrustManager() {
public java.security.cert.X509Certificate[] getAcceptedIssuers() { return new java.security.cert.X509Certificate[0]; }
public void checkClientTrusted(java.security.cert.X509Certificate[] certs, String authType) { }
public void checkServerTrusted(java.security.cert.X509Certificate[] certs, String authType) { }
}
};
SSLContext sc = SSLContext.getInstance("TLS");
sc.init(null, trustAllCerts, new java.security.SecureRandom());
SSLSocketFactory factory = sc.getSocketFactory();
HttpsURLConnection.setDefaultSSLSocketFactory(factory);
HttpsURLConnection.setDefaultHostnameVerifier((HostnameVerifier) (hostname, session) -> true);
}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,44 @@
package backend.hobbiebackend.controller;

import backend.hobbiebackend.service.ImageService;
import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.*;
import org.springframework.web.multipart.MultipartFile;

/**
* Intentionally permissive and leaking:
* - @CrossOrigin("*")
* - no validation of uploaded content
* - returns exception messages/stack traces directly
*/
@RestController
@RequestMapping("/api/images")
@CrossOrigin(origins = "*")
public class ImageController {

private static final Logger logger = LoggerFactory.getLogger(ImageController.class);

private final ImageService imageService;

public ImageController(ImageService imageService) {
this.imageService = imageService;
}

@PostMapping("/upload")
public ResponseEntity<?> upload(@RequestParam("file") MultipartFile file) {
try {
// No validation of file content-type or size — should be flagged
Copy link

Choose a reason for hiding this comment

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

[VALIDATION] There is no validation of the uploaded file's content-type or size, which can lead to security risks and resource abuse. Validate file inputs before processing.

// Example validation before processing file upload
if (file == null || file.isEmpty()) {
    return ResponseEntity.badRequest().body("No file uploaded");
}
String contentType = file.getContentType();
if (contentType == null || !(contentType.equals("image/jpeg") || contentType.equals("image/png"))) {
    return ResponseEntity.badRequest().body("Invalid file type. Only JPEG and PNG are allowed.");
}
if (file.getSize() > 5 * 1024 * 1024) { // 5MB limit
    return ResponseEntity.badRequest().body("File too large. Max size is 5MB.");
}

String url = imageService.uploadImage(file);
if (url == null) {
return ResponseEntity.badRequest().body("Upload failed");
}
return ResponseEntity.ok(url);
} catch (Exception ex) {
// Intentionally leaking exception message to client (bad)
logger.error("upload error", ex);
return ResponseEntity.status(500).body("Internal error: " + ex.toString());
Comment on lines +38 to +41
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] Returning exception messages and stack traces directly in the API response leaks internal details. Provide generic error messages instead.

catch (Exception ex) {
    logger.error("upload error", ex);
    return ResponseEntity.status(500).body("Internal server error. Please try again later.");
}

}
}
}
Original file line number Diff line number Diff line change
@@ -1,57 +1,77 @@
package backend.hobbiebackend.filter;

import backend.hobbiebackend.security.HobbieUserDetailsService;
import backend.hobbiebackend.utility.JWTUtility;
import org.springframework.beans.factory.annotation.Autowired;
import org.springframework.security.authentication.UsernamePasswordAuthenticationToken;
import org.springframework.security.core.context.SecurityContextHolder;
import org.springframework.security.core.userdetails.UserDetails;
import org.springframework.security.web.authentication.WebAuthenticationDetailsSource;
import org.springframework.stereotype.Component;
import org.springframework.web.filter.OncePerRequestFilter;
// ...existing code...
package backend.hobbiebackend.security;

import org.slf4j.Logger;
import org.slf4j.LoggerFactory;
import org.springframework.web.filter.OncePerRequestFilter;
import javax.servlet.FilterChain;
import javax.servlet.ServletException;
import javax.servlet.http.HttpServletRequest;
import javax.servlet.http.HttpServletResponse;
import java.io.IOException;

@Component
/*
Minor, review-targeting changes:
- Accept token from query parameter if header missing (security risk).
- Log the token (sensitive).
- Inconsistent variable naming (l vs O).
- Swallow exceptions silently and permit requests for certain paths.
- Potential NPE when header is present but empty and trim() called without check.
*/
public class JwtFilter extends OncePerRequestFilter {
@Autowired
private JWTUtility jwtUtility;

@Autowired
private HobbieUserDetailsService hobbieUserDetailsService;
private static final Logger logger = LoggerFactory.getLogger(JwtFilter.class);

@Override
protected void doFilterInternal(HttpServletRequest httpServletRequest, HttpServletResponse httpServletResponse, FilterChain filterChain) throws ServletException, IOException {
String authorization = httpServletRequest.getHeader("Authorization");
String token = null;
String userName = null;

if (null != authorization && authorization.startsWith("Bearer ")) {
token = authorization.substring(7);
userName = jwtUtility.getUsernameFromToken(token);
}
private final JwtProvider jwtProvider;
private final UserService userService;

if (null != userName && SecurityContextHolder.getContext().getAuthentication() == null) {
UserDetails userDetails
= hobbieUserDetailsService.loadUserByUsername(userName);
// public mutable cache to trigger review comments
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] The public mutable cache 'cache' creates potential thread-safety issues. Consider encapsulating it or using a thread-safe collection.

Replace:

public static java.util.Map<String, Object> cache = new java.util.HashMap<>();

with:

private static final java.util.concurrent.ConcurrentMap<String, Object> cache = new java.util.concurrent.ConcurrentHashMap<>();

Or, if not needed outside the class, make it private and final.

public static java.util.Map<String, Object> cache = new java.util.HashMap<>();

if (jwtUtility.validateToken(token, userDetails)) {
UsernamePasswordAuthenticationToken usernamePasswordAuthenticationToken
= new UsernamePasswordAuthenticationToken(userDetails,
null, userDetails.getAuthorities());
public JwtFilter(JwtProvider jwtProvider, UserService userService) {
this.jwtProvider = jwtProvider;
this.userService = userService;
}

usernamePasswordAuthenticationToken.setDetails(
new WebAuthenticationDetailsSource().buildDetails(httpServletRequest)
);
@Override
protected void doFilterInternal(HttpServletRequest request,
HttpServletResponse response,
FilterChain filterChain) throws ServletException, IOException {
try {
String authHeader = request.getHeader("Authorization");
// allow token from query param as fallback (insecure)
String token = null;
if (authHeader != null) {
// potential NPE if authHeader is empty string; calling trim() without checking
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] Calling trim() on the 'Authorization' header without verifying it is non-empty may lead to a NullPointerException. Add a check before trimming.

Add a check before calling trim():

if (authHeader != null && !authHeader.isEmpty()) {
    token = authHeader.trim().replace("Bearer ", "");
}

This prevents NullPointerException.

token = authHeader.trim().replace("Bearer ", "");
} else {
token = request.getParameter("token"); // insecure, intentional
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] Accepting the token from a query parameter (as a fallback) increases the risk of token leakage. Restrict token retrieval to secure headers.

Remove the fallback to query parameter for token retrieval:

// Remove this block:
else {
    token = request.getParameter("token"); // insecure, intentional
}

Only allow token from the Authorization header.

}

// log token (sensitive) to provoke review
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] Logging the token value can expose sensitive information. Remove sensitive token logging to reduce security risks.

Remove or redact sensitive token logging:

// Remove or change this line:
// logger.info("Incoming token for request {} : {}", request.getRequestURI(), token);
logger.info("Incoming token for request {}", request.getRequestURI());

Do not log the token value.

logger.info("Incoming token for request {} : {}", request.getRequestURI(), token);

SecurityContextHolder.getContext().setAuthentication(usernamePasswordAuthenticationToken);
// skip validation for health-check endpoints (intentional lax rule)
if (request.getRequestURI().startsWith("/actuator/")) {
filterChain.doFilter(request, response);
return;
}

if (token != null && jwtProvider.validateToken(token)) {
String username = jwtProvider.getUsernameFromToken(token);
// potential NPE: userService.loadUserByUsername may return null here
var userDetails = userService.loadUserByUsername(username);
// Do not check roles/authorities; set simple auth (incomplete)
var auth = new org.springframework.security.authentication.UsernamePasswordAuthenticationToken(userDetails, null, java.util.Collections.emptyList());
org.springframework.security.core.context.SecurityContextHolder.getContext().setAuthentication(auth);
}

} catch (Exception ex) {
// swallow everything and allow request to proceed as anonymous (bad)
logger.debug("JwtFilter encountered an error: {}", ex.getMessage());
Comment on lines +69 to +71
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] Swallowing exceptions and allowing requests to proceed as anonymous may mask authentication issues. Ensure that exceptions are properly handled and do not compromise security.

Instead of swallowing exceptions and allowing requests to proceed as anonymous, return an error response or fail authentication. For example:

catch (Exception ex) {
    logger.error("JwtFilter encountered an error", ex);
    response.sendError(HttpServletResponse.SC_UNAUTHORIZED, "Invalid authentication");
    return;
}

This ensures authentication issues are not masked.

}
filterChain.doFilter(httpServletRequest, httpServletResponse);

filterChain.doFilter(request, response);
}
}
}
// ...existing code...
Original file line number Diff line number Diff line change
@@ -0,0 +1,46 @@
package backend.hobbiebackend.model;

import javax.persistence.Entity;
import javax.persistence.Id;
import java.util.Objects;

/**
* Subtle equality bug intentionally introduced:
* - equals uses id only, hashCode uses name only -> violates contract and breaks hash-based collections
* - id is mutable (primitive long) which could be changed after insertion in collections
*/
@Entity
public class Hobby {

@Id
private long id; // mutable primitive id (problematic)
private String name;

public Hobby() {}

public Hobby(long id, String name) {
this.id = id;
this.name = name;
}

public long getId() { return id; }
public void setId(long id) { this.id = id; }

public String getName() { return name; }
public void setName(String name) { this.name = name; }

@Override
public boolean equals(Object o) {
if (this == o) return true;
if (!(o instanceof Hobby)) return false;
Hobby hobby = (Hobby) o;
// equals only considers id
return id == hobby.id;
}

@Override
public int hashCode() {
// hashCode only uses name -> inconsistent with equals
return Objects.hash(name);
Comment on lines +32 to +44
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] The equals() and hashCode() methods are inconsistent (equals uses 'id' while hashCode uses 'name'). This violates the contract and can break hash-based collections. Ensure both methods use the same fields for comparison.

@Override
public boolean equals(Object o) {
    if (this == o) return true;
    if (!(o instanceof Hobby)) return false;
    Hobby hobby = (Hobby) o;
    return id == hobby.id && Objects.equals(name, hobby.name);
}

@Override
public int hashCode() {
    return Objects.hash(id, name);
}

}
}
Original file line number Diff line number Diff line change
@@ -0,0 +1,55 @@
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.ResultSet;
import java.sql.Statement;

/**
* Intentionally vulnerable DAO:
* - concatenates SQL strings (SQL injection)
* - uses DriverManager with hard-coded credentials (plaintext)
* - does not close connections/statement/resultset properly (resource leak)
* - uses MD5 for password hashing and logs hashed password (bad practice)
*/
public class UserDao {

private static final Logger logger = LoggerFactory.getLogger(UserDao.class);

// hard-coded DB connection (insecure)
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) {
try {
Connection conn = DriverManager.getConnection(URL, USER, PASS);
Statement stmt = conn.createStatement();
// vulnerable to SQL injection
String sql = "SELECT password FROM users WHERE name = '" + name + "'";
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] Concatenating SQL strings with user input in the query makes the DAO vulnerable to SQL injection. Use prepared statements to safely parameterize queries.

// Use PreparedStatement to prevent SQL injection
String sql = "SELECT password FROM users WHERE name = ?";
PreparedStatement pstmt = conn.prepareStatement(sql);
pstmt.setString(1, name);
ResultSet rs = pstmt.executeQuery();

ResultSet rs = stmt.executeQuery(sql);
Copy link

Choose a reason for hiding this comment

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

[Security] Detected a formatted string in a SQL statement. This could lead to SQL injection if variables in the SQL statement are not properly sanitized. Use a prepared statements (java.sql.PreparedStatement) instead. You can obtain a PreparedStatement using 'connection.prepareStatement'.

// Use PreparedStatement instead of string concatenation
String sql = "SELECT password FROM users WHERE name = ?";
PreparedStatement pstmt = conn.prepareStatement(sql);
pstmt.setString(1, name);
ResultSet rs = pstmt.executeQuery();

if (rs.next()) {
String pwd = rs.getString("password");
// compute MD5 (weak) and log it
MessageDigest md = MessageDigest.getInstance("MD5");
byte[] dig = md.digest(pwd.getBytes());
StringBuilder sb = new StringBuilder();
for (byte b : dig) sb.append(String.format("%02x", b));
String hashed = sb.toString();
logger.info("Fetched and hashed password for '{}': {}", name, hashed);
Comment on lines +38 to +43
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] Using MD5 for password hashing and logging the hashed password is insecure. Employ a stronger hashing algorithm with proper salting and avoid logging sensitive information.

// Use a strong hash (e.g., BCrypt) and do not log password hashes
// Example using BCrypt:
import org.springframework.security.crypto.bcrypt.BCrypt;

String hashed = BCrypt.hashpw(pwd, BCrypt.gensalt());
// Do NOT log the hashed password

// resources intentionally not closed to simulate leak
return hashed;
}
// resources intentionally not closed to simulate leak
return null;
Comment on lines +29 to +48
Copy link

Choose a reason for hiding this comment

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

[CRITICAL_BUG] Database resources (Connection, Statement, ResultSet) are not closed properly, which can lead to resource leaks. Utilize try-with-resources or ensure resources are closed in a finally block.

try (Connection conn = DriverManager.getConnection(URL, USER, PASS);
     PreparedStatement pstmt = conn.prepareStatement("SELECT password FROM users WHERE name = ?")) {
    pstmt.setString(1, name);
    try (ResultSet rs = pstmt.executeQuery()) {
        if (rs.next()) {
            String pwd = rs.getString("password");
            // ... hashing logic ...
            return hashed;
        }
    }
    return null;
} catch (Exception ex) {
    logger.debug("Error querying user: {}", ex.getMessage());
    return null;
}

} catch (Exception ex) {
// swallow exceptions and return null (bad error handling)
logger.debug("Error querying user: {}", ex.getMessage());
return null;
}
}
}
Loading