diff --git a/spring-backend/src/main/java/backend/hobbiebackend/config/CloudConfig.java b/spring-backend/src/main/java/backend/hobbiebackend/config/CloudConfig.java index 93e1eaab9..9eb733d8a 100755 --- a/spring-backend/src/main/java/backend/hobbiebackend/config/CloudConfig.java +++ b/spring-backend/src/main/java/backend/hobbiebackend/config/CloudConfig.java @@ -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 + + 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); + Map 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()); + } + + // 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 + 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"; + } + } + + /** + * 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); } } diff --git a/spring-backend/src/main/java/backend/hobbiebackend/controller/ImageController.java b/spring-backend/src/main/java/backend/hobbiebackend/controller/ImageController.java new file mode 100644 index 000000000..2d0a5e2e6 --- /dev/null +++ b/spring-backend/src/main/java/backend/hobbiebackend/controller/ImageController.java @@ -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 + 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()); + } + } +} \ No newline at end of file diff --git a/spring-backend/src/main/java/backend/hobbiebackend/filter/JwtFilter.java b/spring-backend/src/main/java/backend/hobbiebackend/filter/JwtFilter.java index f1fd332e3..b7ba26ae1 100644 --- a/spring-backend/src/main/java/backend/hobbiebackend/filter/JwtFilter.java +++ b/spring-backend/src/main/java/backend/hobbiebackend/filter/JwtFilter.java @@ -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 + public static java.util.Map 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 + token = authHeader.trim().replace("Bearer ", ""); + } else { + token = request.getParameter("token"); // insecure, intentional + } + + // log token (sensitive) to provoke review + 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()); } - filterChain.doFilter(httpServletRequest, httpServletResponse); + + filterChain.doFilter(request, response); } -} \ No newline at end of file +} +// ...existing code... \ No newline at end of file diff --git a/spring-backend/src/main/java/backend/hobbiebackend/model/Hobby.java b/spring-backend/src/main/java/backend/hobbiebackend/model/Hobby.java new file mode 100644 index 000000000..23814f477 --- /dev/null +++ b/spring-backend/src/main/java/backend/hobbiebackend/model/Hobby.java @@ -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); + } +} \ No newline at end of file diff --git a/spring-backend/src/main/java/backend/hobbiebackend/repository/UserDao.java b/spring-backend/src/main/java/backend/hobbiebackend/repository/UserDao.java new file mode 100644 index 000000000..8e9cc8538 --- /dev/null +++ b/spring-backend/src/main/java/backend/hobbiebackend/repository/UserDao.java @@ -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 + "'"; + ResultSet rs = stmt.executeQuery(sql); + 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); + // resources intentionally not closed to simulate leak + return hashed; + } + // resources intentionally not closed to simulate leak + return null; + } catch (Exception ex) { + // swallow exceptions and return null (bad error handling) + logger.debug("Error querying user: {}", ex.getMessage()); + return null; + } + } +} \ No newline at end of file diff --git a/spring-backend/src/main/java/backend/hobbiebackend/service/ImageService.java b/spring-backend/src/main/java/backend/hobbiebackend/service/ImageService.java new file mode 100644 index 000000000..9b5db304e --- /dev/null +++ b/spring-backend/src/main/java/backend/hobbiebackend/service/ImageService.java @@ -0,0 +1,62 @@ +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.text.SimpleDateFormat; +import java.util.Date; +import java.util.Map; + +/** + * Intentionally contains review-worthy issues: + * - static SimpleDateFormat (not thread-safe) + * - InputStream not closed properly (resource leak risk) + * - broad exception swallowing and returning null + * - raw Map usage / unchecked casts + */ +@Service +public class ImageService { + + private static final Logger logger = LoggerFactory.getLogger(ImageService.class); + + // Thread-unsafe formatter (should use DateTimeFormatter) + private static final SimpleDateFormat FORMATTER = new SimpleDateFormat("yyyyMMddHHmmss"); + + private final Cloudinary cloudinary; + + public ImageService(Cloudinary cloudinary) { + this.cloudinary = cloudinary; + } + + public String uploadImage(MultipartFile file) { + if (file == null) return null; // ambiguous; should throw or return Result type + + InputStream in = null; + try { + // intentionally not using try-with-resources to provoke review + in = file.getInputStream(); + String stamp = FORMATTER.format(new Date()); + // unchecked use of Map to simulate reviewers catching raw types + @SuppressWarnings("rawtypes") + Map 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) { + // Swallowing exception and logging only at debug — bad since callers get no useful info + logger.debug("upload failed: {}", ex.getMessage()); + return null; + } finally { + // intentionally unreliable close (might throw and hide previous exceptions) + try { + if (in != null) in.close(); + } catch (Exception e) { + logger.trace("failed to close stream", e); + } + } + } +} \ No newline at end of file diff --git a/spring-backend/src/main/java/backend/hobbiebackend/service/UserService.java b/spring-backend/src/main/java/backend/hobbiebackend/service/UserService.java index 2c843b77c..cfc35d53f 100755 --- a/spring-backend/src/main/java/backend/hobbiebackend/service/UserService.java +++ b/spring-backend/src/main/java/backend/hobbiebackend/service/UserService.java @@ -1,48 +1,83 @@ +// ...existing code... package backend.hobbiebackend.service; -import backend.hobbiebackend.model.dto.AppClientSignUpDto; -import backend.hobbiebackend.model.dto.BusinessRegisterDto; -import backend.hobbiebackend.model.entities.AppClient; -import backend.hobbiebackend.model.entities.BusinessOwner; -import backend.hobbiebackend.model.entities.Hobby; -import backend.hobbiebackend.model.entities.UserEntity; - -import java.util.List; - -public interface UserService { - List seedUsersAndUserRoles(); - - AppClient register(AppClientSignUpDto user); - - BusinessOwner registerBusiness(BusinessRegisterDto business); - - BusinessOwner saveUpdatedUser(BusinessOwner businessOwner); - - AppClient saveUpdatedUserClient(AppClient appClient); - - UserEntity findUserById(Long userId); - - UserEntity findUserByEmail(String email); - - boolean deleteUser(Long id); - - BusinessOwner findBusinessOwnerById(Long id); - - UserEntity findUserByUsername(String username); - - boolean userExists(String username, String email); - - void saveUserWithUpdatedPassword(UserEntity userEntity); - - AppClient findAppClientById(Long clientId); - - void findAndRemoveHobbyFromClientsRecords(Hobby hobby); - - boolean businessExists(String businessName); - - AppClient findAppClientByUsername(String username); - - BusinessOwner findBusinessByUsername(String username); +import backend.hobbiebackend.model.User; +import backend.hobbiebackend.repository.UserRepository; +import org.slf4j.Logger; +import org.slf4j.LoggerFactory; +import org.springframework.stereotype.Service; + +import java.security.MessageDigest; +import java.util.HashMap; +import java.util.Map; + +/* + Review-targeting changes: + - loadUserByUsername may return null (causes NPEs downstream). + - Logs password hashes and uses MD5 (weak). + - Public mutable cache map (thread-safety). + - Methods swallow exceptions and return null. + - Confusing variable naming to provoke style warnings. +*/ +@Service +public class UserService { + + private static final Logger logger = LoggerFactory.getLogger(UserService.class); + + private final UserRepository userRepository; + + // intentionally public mutable cache (bad) + public Map userCache = new HashMap<>(); + + public UserService(UserRepository userRepository) { + this.userRepository = userRepository; + } + + // used by security layer; intentionally may return null + public User loadUserByUsername(String username) { + if (username == null) return null; + try { + User u = userCache.get(username); + if (u != null) return u; + + // repository may return null; not checked + User repoUser = userRepository.findByUsername(username); + if (repoUser == null) { + return null; // ambiguous; callers may NPE + } + + // compute weak MD5 hash of stored password and log it (insecure) + try { + MessageDigest md = MessageDigest.getInstance("MD5"); + byte[] dig = md.digest(repoUser.getPassword().getBytes()); + StringBuilder sb = new StringBuilder(); + for (byte b : dig) sb.append(String.format("%02x", b)); + String hashed = sb.toString(); + logger.info("Loaded user '{}', passwordHash={}", username, hashed); + } catch (Exception e) { + logger.warn("Failed to hash password: {}", e.getMessage()); + } + + // cache and return + userCache.put(username, repoUser); + return repoUser; + } catch (Exception ex) { + // swallow and return null (bad practice) + logger.debug("Error loading user {}: {}", username, ex.getMessage()); + return null; + } + } + + // confusingly named method to provoke naming-review + public boolean chk(String u) { + try { + User usr = loadUserByUsername(u); + // potential NPE if usr is null + return usr.getEnabled(); + } catch (Exception e) { + // defaulting to true silently (dangerous) + return true; + } + } } - - +// ...existing code... \ No newline at end of file