From 17c2044eb517e61f73ef8d7610b5796ec24efc22 Mon Sep 17 00:00:00 2001 From: SanriaArgos Date: Mon, 10 Aug 2026 00:55:19 +0300 Subject: [PATCH 1/8] fix(api): map API errors without exposing internals --- src/main/java/goodroad/api/ApiErrors.java | 93 ++++++++++++++++++++--- 1 file changed, 84 insertions(+), 9 deletions(-) diff --git a/src/main/java/goodroad/api/ApiErrors.java b/src/main/java/goodroad/api/ApiErrors.java index 96670b0..c72cda2 100644 --- a/src/main/java/goodroad/api/ApiErrors.java +++ b/src/main/java/goodroad/api/ApiErrors.java @@ -1,10 +1,23 @@ package goodroad.api; +import jakarta.validation.ConstraintViolationException; +import lombok.extern.slf4j.Slf4j; +import org.springframework.dao.DataIntegrityViolationException; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; +import org.springframework.http.converter.HttpMessageNotReadableException; +import org.springframework.security.access.AccessDeniedException; +import org.springframework.web.HttpMediaTypeNotSupportedException; +import org.springframework.web.bind.MethodArgumentNotValidException; +import org.springframework.web.bind.MissingServletRequestParameterException; import org.springframework.web.bind.annotation.ExceptionHandler; import org.springframework.web.bind.annotation.RestControllerAdvice; -import lombok.extern.slf4j.Slf4j; +import org.springframework.web.method.annotation.HandlerMethodValidationException; +import org.springframework.web.method.annotation.MethodArgumentTypeMismatchException; +import org.springframework.web.multipart.MaxUploadSizeExceededException; +import org.springframework.web.multipart.support.MissingServletRequestPartException; +import org.springframework.web.servlet.resource.NoResourceFoundException; + import java.time.Instant; @Slf4j @@ -23,8 +36,8 @@ public static class ApiException extends RuntimeException { private final HttpStatus status; private final String code; - public ApiException(HttpStatus status, String code, String msg) { - super(msg); + public ApiException(HttpStatus status, String code, String message) { + super(message); this.status = status; this.code = code; } @@ -42,16 +55,78 @@ public String code() { public static class GlobalHandler { @ExceptionHandler(ApiException.class) - public ResponseEntity handleApiException(ApiException e) { - log.error("API exception: code={}, msg={}", e.code(), e.getMessage(), e); - return ResponseEntity.status(e.status()).body(ApiError.of(e.code(), e.getMessage())); + public ResponseEntity handleApiException(ApiException exception) { + if (exception.status().is5xxServerError()) { + log.error("API exception: code={}, msg={}", exception.code(), exception.getMessage(), exception); + } + return ResponseEntity.status(exception.status()) + .body(ApiError.of(exception.code(), exception.getMessage())); + } + + @ExceptionHandler(MethodArgumentNotValidException.class) + public ResponseEntity handleValidation(MethodArgumentNotValidException exception) { + return badRequest("REQUEST_VALIDATION_FAILED", "Request fields are invalid"); + } + + @ExceptionHandler(HttpMessageNotReadableException.class) + public ResponseEntity handleUnreadableBody(HttpMessageNotReadableException exception) { + return badRequest("REQUEST_BODY_INVALID", "Request body is missing or contains invalid JSON"); + } + + @ExceptionHandler({ + MethodArgumentTypeMismatchException.class, + MissingServletRequestParameterException.class, + ConstraintViolationException.class, + HandlerMethodValidationException.class + }) + public ResponseEntity handleInvalidParameters(Exception exception) { + return badRequest("REQUEST_VALIDATION_FAILED", "Request parameters are invalid"); + } + + @ExceptionHandler(MissingServletRequestPartException.class) + public ResponseEntity handleMissingPart(MissingServletRequestPartException exception) { + return badRequest("REQUEST_PART_MISSING", "Required request part is missing"); + } + + @ExceptionHandler(MaxUploadSizeExceededException.class) + public ResponseEntity handleUploadTooLarge(MaxUploadSizeExceededException exception) { + return ResponseEntity.status(HttpStatus.PAYLOAD_TOO_LARGE) + .body(ApiError.of("FILE_TOO_LARGE", "Uploaded file is too large")); + } + + @ExceptionHandler(HttpMediaTypeNotSupportedException.class) + public ResponseEntity handleUnsupportedMediaType(HttpMediaTypeNotSupportedException exception) { + return ResponseEntity.status(HttpStatus.UNSUPPORTED_MEDIA_TYPE) + .body(ApiError.of("CONTENT_TYPE_UNSUPPORTED", "Content type is not supported")); + } + + @ExceptionHandler(DataIntegrityViolationException.class) + public ResponseEntity handleConflict(DataIntegrityViolationException exception) { + return ResponseEntity.status(HttpStatus.CONFLICT) + .body(ApiError.of("DATA_CONFLICT", "Operation conflicts with current data")); + } + + @ExceptionHandler(AccessDeniedException.class) + public ResponseEntity handleAccessDenied(AccessDeniedException exception) { + return ResponseEntity.status(HttpStatus.FORBIDDEN) + .body(ApiError.of("ACCESS_DENIED", "Access denied")); + } + + @ExceptionHandler(NoResourceFoundException.class) + public ResponseEntity handleNotFound(NoResourceFoundException exception) { + return ResponseEntity.status(HttpStatus.NOT_FOUND) + .body(ApiError.of("ENDPOINT_NOT_FOUND", "Endpoint not found")); } @ExceptionHandler(Exception.class) - public ResponseEntity handleServerError(Exception e) { - log.error("Unexpected exception", e); + public ResponseEntity handleServerError(Exception exception) { + log.error("Unexpected exception", exception); return ResponseEntity.status(HttpStatus.INTERNAL_SERVER_ERROR) .body(ApiError.of("SERVER_INTERNAL_ERROR", "Server internal error")); } + + private ResponseEntity badRequest(String code, String message) { + return ResponseEntity.badRequest().body(ApiError.of(code, message)); + } } -} \ No newline at end of file +} From abb41977954b6351f02baf8be6f9851cbfd6a42b Mon Sep 17 00:00:00 2001 From: SanriaArgos Date: Mon, 10 Aug 2026 00:56:26 +0300 Subject: [PATCH 2/8] fix(validation): bound shared text inputs --- src/main/java/goodroad/validation/InputRules.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/main/java/goodroad/validation/InputRules.java b/src/main/java/goodroad/validation/InputRules.java index 0f357dd..5745e6c 100644 --- a/src/main/java/goodroad/validation/InputRules.java +++ b/src/main/java/goodroad/validation/InputRules.java @@ -16,7 +16,7 @@ private InputRules() { public static String requireCyrillicText(String value, String code, String fieldName) { String normalized = trimToNull(value); - if (normalized == null || !CYRILLIC_TEXT.matcher(normalized).matches()) { + if (normalized == null || normalized.length() > 80 || !CYRILLIC_TEXT.matcher(normalized).matches()) { throw new ApiException(HttpStatus.BAD_REQUEST, code, fieldName + " is invalid"); } return normalized; From f983f9c69db8c4aacfeb10b5fa662d26197b26e8 Mon Sep 17 00:00:00 2001 From: SanriaArgos Date: Mon, 10 Aug 2026 00:57:05 +0300 Subject: [PATCH 3/8] fix(validation): validate coordinates and distance calculations --- .../java/goodroad/validation/GeoUtils.java | 63 +++++++++++++++++++ 1 file changed, 63 insertions(+) create mode 100644 src/main/java/goodroad/validation/GeoUtils.java diff --git a/src/main/java/goodroad/validation/GeoUtils.java b/src/main/java/goodroad/validation/GeoUtils.java new file mode 100644 index 0000000..310ab78 --- /dev/null +++ b/src/main/java/goodroad/validation/GeoUtils.java @@ -0,0 +1,63 @@ +package goodroad.validation; + +import goodroad.api.ApiErrors.ApiException; +import org.springframework.http.HttpStatus; + +public final class GeoUtils { + private static final double EARTH_RADIUS_KM = 6371.0; + + private GeoUtils() { + } + + public static Coordinates requireCoordinates(Double latitude, Double longitude, String code) { + if (latitude == null || longitude == null + || !Double.isFinite(latitude) || !Double.isFinite(longitude) + || latitude < -90 || latitude > 90 + || longitude < -180 || longitude > 180) { + throw new ApiException(HttpStatus.BAD_REQUEST, code, "Координаты должны содержать допустимые широту и долготу"); + } + return new Coordinates(latitude, longitude); + } + + public static Coordinates parseLatLon(String value, String fieldName) { + if (value == null) { + throw invalidPoint(fieldName); + } + String[] parts = value.split(",", -1); + if (parts.length != 2) { + throw invalidPoint(fieldName); + } + try { + return requireCoordinates( + Double.parseDouble(parts[0].trim()), + Double.parseDouble(parts[1].trim()), + "ROUTE_POINT_INVALID" + ); + } catch (NumberFormatException e) { + throw invalidPoint(fieldName); + } + } + + public static double distanceKm(double firstLat, double firstLon, double secondLat, double secondLon) { + double latitudeDelta = Math.toRadians(secondLat - firstLat); + double longitudeDelta = Math.toRadians(secondLon - firstLon); + double a = Math.sin(latitudeDelta / 2) * Math.sin(latitudeDelta / 2) + + Math.cos(Math.toRadians(firstLat)) * Math.cos(Math.toRadians(secondLat)) + * Math.sin(longitudeDelta / 2) * Math.sin(longitudeDelta / 2); + return EARTH_RADIUS_KM * 2 * Math.atan2(Math.sqrt(a), Math.sqrt(1 - a)); + } + + private static ApiException invalidPoint(String fieldName) { + return new ApiException( + HttpStatus.BAD_REQUEST, + "ROUTE_POINT_INVALID", + "Поле " + fieldName + " должно иметь формат latitude,longitude" + ); + } + + public record Coordinates(double latitude, double longitude) { + public String asLatLon() { + return latitude + "," + longitude; + } + } +} From f75ae5121532caecdfdd8342a4c1d8624faa82ae Mon Sep 17 00:00:00 2001 From: SanriaArgos Date: Mon, 10 Aug 2026 00:57:56 +0300 Subject: [PATCH 4/8] fix(data): add row locks for user state changes --- src/main/java/goodroad/users/repository/UserRepo.java | 11 ++++++++++- 1 file changed, 10 insertions(+), 1 deletion(-) diff --git a/src/main/java/goodroad/users/repository/UserRepo.java b/src/main/java/goodroad/users/repository/UserRepo.java index 47fe3cb..1277548 100644 --- a/src/main/java/goodroad/users/repository/UserRepo.java +++ b/src/main/java/goodroad/users/repository/UserRepo.java @@ -2,6 +2,7 @@ import org.springframework.data.jpa.repository.*; import org.springframework.data.repository.query.Param; +import jakarta.persistence.LockModeType; import java.time.Instant; import java.util.List; import java.util.Optional; @@ -10,6 +11,14 @@ public interface UserRepo extends JpaRepository { Optional findByPhoneHash(String phoneHash); + @Lock(LockModeType.PESSIMISTIC_WRITE) + @Query("select user from UserEntity user where user.phoneHash = :phoneHash") + Optional findByPhoneHashForUpdate(@Param("phoneHash") String phoneHash); + + @Lock(LockModeType.PESSIMISTIC_WRITE) + @Query("select user from UserEntity user where user.id = :id") + Optional findByIdForUpdate(@Param("id") Long id); + List findByRoleIn(List roles); @Modifying @@ -20,4 +29,4 @@ public interface UserRepo extends JpaRepository { and user.lastActiveAt < :cutoff """) int deleteInactiveBefore(@Param("cutoff") Instant cutoff); -} \ No newline at end of file +} From 417ed59bbdfc96b115fdcc16af1cb2d5033b66f0 Mon Sep 17 00:00:00 2001 From: SanriaArgos Date: Mon, 10 Aug 2026 02:05:06 +0300 Subject: [PATCH 5/8] fix(validation): restrict external and owned storage URLs --- .../validation/TrustedUrlService.java | 71 +++++++++++++++++++ .../validation/TrustedUrlServiceTest.java | 50 +++++++++++++ 2 files changed, 121 insertions(+) create mode 100644 src/main/java/goodroad/validation/TrustedUrlService.java create mode 100644 src/test/java/goodroad/validation/TrustedUrlServiceTest.java diff --git a/src/main/java/goodroad/validation/TrustedUrlService.java b/src/main/java/goodroad/validation/TrustedUrlService.java new file mode 100644 index 0000000..2ed6d52 --- /dev/null +++ b/src/main/java/goodroad/validation/TrustedUrlService.java @@ -0,0 +1,71 @@ +package goodroad.validation; + +import goodroad.api.ApiErrors.ApiException; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.http.HttpStatus; +import org.springframework.stereotype.Service; + +import java.net.URI; +import java.net.URISyntaxException; +import java.util.Locale; +import java.util.Set; + +@Service +public class TrustedUrlService { + private static final String STORAGE_HOST = "storage.yandexcloud.net"; + private static final Set DOBRO_HOSTS = Set.of("dobro.ru", "www.dobro.ru"); + + private final String storageBucket; + + public TrustedUrlService(@Value("${yandex.storage.bucket}") String storageBucket) { + this.storageBucket = storageBucket; + } + + public String requireDobroProfileUrl(String rawUrl) { + URI uri = parseHttpsUrl(rawUrl, "DOBRO_URL_INVALID", "Укажите корректную HTTPS-ссылку на профиль dobro.ru"); + String host = uri.getHost().toLowerCase(Locale.ROOT); + if (!DOBRO_HOSTS.contains(host) || uri.getPort() != -1 || uri.getUserInfo() != null) { + throw invalid("DOBRO_URL_INVALID", "Ссылка должна вести на домен dobro.ru"); + } + return uri.normalize().toString(); + } + + public String requireOwnedStorageUrl(String rawUrl, String directory, Long userId, String code) { + URI uri = parseHttpsUrl(rawUrl, code, "Ссылка на файл имеет неверный формат"); + String expectedPrefix = "/" + storageBucket + "/" + directory + "/" + userId + "/"; + String rawPath = uri.getRawPath() == null ? "" : uri.getRawPath().toLowerCase(Locale.ROOT); + boolean containsEncodedSeparator = rawPath.contains("%2f") || rawPath.contains("%5c") || rawPath.contains("%2e"); + if (!STORAGE_HOST.equalsIgnoreCase(uri.getHost()) + || uri.getPort() != -1 + || uri.getUserInfo() != null + || uri.getQuery() != null + || uri.getFragment() != null + || containsEncodedSeparator + || uri.normalize().getPath() == null + || !uri.normalize().getPath().startsWith(expectedPrefix) + || uri.normalize().getPath().equals(expectedPrefix)) { + throw invalid(code, "Разрешены только файлы, загруженные текущим пользователем через GoodRoad"); + } + return uri.normalize().toString(); + } + + private URI parseHttpsUrl(String rawUrl, String code, String message) { + String value = InputRules.trimToNull(rawUrl); + if (value == null || value.length() > 512) { + throw invalid(code, message); + } + try { + URI uri = new URI(value); + if (!"https".equalsIgnoreCase(uri.getScheme()) || uri.getHost() == null) { + throw invalid(code, message); + } + return uri; + } catch (URISyntaxException e) { + throw invalid(code, message); + } + } + + private ApiException invalid(String code, String message) { + return new ApiException(HttpStatus.BAD_REQUEST, code, message); + } +} diff --git a/src/test/java/goodroad/validation/TrustedUrlServiceTest.java b/src/test/java/goodroad/validation/TrustedUrlServiceTest.java new file mode 100644 index 0000000..9c71af6 --- /dev/null +++ b/src/test/java/goodroad/validation/TrustedUrlServiceTest.java @@ -0,0 +1,50 @@ +package goodroad.validation; + +import goodroad.api.ApiErrors.ApiException; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +class TrustedUrlServiceTest { + private final TrustedUrlService service = new TrustedUrlService("goodroad-bucket"); + + @Test + void acceptsDobroHttpsProfile() { + assertEquals( + "https://dobro.ru/volunteer/123", + service.requireDobroProfileUrl("https://dobro.ru/volunteer/123") + ); + } + + @Test + void rejectsLookalikeDobroDomain() { + assertThrows(ApiException.class, () -> service.requireDobroProfileUrl( + "https://dobro.ru.evil.example/payload.exe" + )); + assertThrows(ApiException.class, () -> service.requireDobroProfileUrl( + "https://attacker@dobro.ru/volunteer/123" + )); + } + + @Test + void acceptsOnlyCurrentUsersUploadedCertificate() { + String expected = "https://storage.yandexcloud.net/goodroad-bucket/volunteer-certificates/10/cert.jpg"; + assertEquals(expected, service.requireOwnedStorageUrl( + expected, "volunteer-certificates", 10L, "CERTIFICATE_URL_INVALID" + )); + + assertThrows(ApiException.class, () -> service.requireOwnedStorageUrl( + "https://storage.yandexcloud.net/goodroad-bucket/volunteer-certificates/11/malware.jpg", + "volunteer-certificates", + 10L, + "CERTIFICATE_URL_INVALID" + )); + assertThrows(ApiException.class, () -> service.requireOwnedStorageUrl( + "https://storage.yandexcloud.net/goodroad-bucket/volunteer-certificates/10/%2e%2e/reviews/file.jpg", + "volunteer-certificates", + 10L, + "CERTIFICATE_URL_INVALID" + )); + } +} From b4ee0a54bfb60a33ada1aabf2fb5717b057d7da6 Mon Sep 17 00:00:00 2001 From: SanriaArgos Date: Mon, 10 Aug 2026 02:11:01 +0300 Subject: [PATCH 6/8] fix(storage): validate image uploads before storage --- .../java/goodroad/storage/StorageService.java | 57 +++++----- .../validation/UploadValidationService.java | 106 ++++++++++++++++++ .../UploadValidationServiceTest.java | 51 +++++++++ 3 files changed, 187 insertions(+), 27 deletions(-) create mode 100644 src/main/java/goodroad/validation/UploadValidationService.java create mode 100644 src/test/java/goodroad/validation/UploadValidationServiceTest.java diff --git a/src/main/java/goodroad/storage/StorageService.java b/src/main/java/goodroad/storage/StorageService.java index e7b630b..fada9e7 100644 --- a/src/main/java/goodroad/storage/StorageService.java +++ b/src/main/java/goodroad/storage/StorageService.java @@ -1,7 +1,12 @@ package goodroad.storage; +import goodroad.api.ApiErrors.ApiException; +import goodroad.validation.UploadValidationService; +import goodroad.validation.UploadValidationService.UploadPurpose; +import goodroad.validation.UploadValidationService.VerifiedUpload; import lombok.RequiredArgsConstructor; import org.springframework.beans.factory.annotation.Value; +import org.springframework.http.HttpStatus; import org.springframework.stereotype.Service; import org.springframework.web.multipart.MultipartFile; import software.amazon.awssdk.core.sync.RequestBody; @@ -15,55 +20,58 @@ public class StorageService { private final S3Client s3Client; + private final UploadValidationService uploadValidator; @Value("${yandex.storage.bucket}") private String bucket; public String uploadAvatar(MultipartFile file, String userId) { - try { + VerifiedUpload verified = uploadValidator.validate(file, UploadPurpose.AVATAR); - String ext = getExt(file.getOriginalFilename()); + try { - String key = "avatars/" + userId + "/" + UUID.randomUUID() + ext; + String key = "avatars/" + userId + "/" + UUID.randomUUID() + verified.extension(); s3Client.putObject( PutObjectRequest.builder() .bucket(bucket) .key(key) - .contentType(file.getContentType()) + .contentType(verified.contentType()) + .contentLength((long) verified.bytes().length) .build(), - RequestBody.fromBytes(file.getBytes()) + RequestBody.fromBytes(verified.bytes()) ); return "https://storage.yandexcloud.net/" + bucket + "/" + key; - } catch (Exception e) { - throw new RuntimeException("Upload failed", e); + } catch (RuntimeException e) { + throw new ApiException(HttpStatus.BAD_GATEWAY, "STORAGE_UNAVAILABLE", "File storage is unavailable"); } } public String uploadReviewPhoto(MultipartFile file, String userId) { - try { + VerifiedUpload verified = uploadValidator.validate(file, UploadPurpose.REVIEW_PHOTO); - String ext = getExt(file.getOriginalFilename()); + try { String key = "reviews/" + userId + "/" + UUID.randomUUID() - + ext; + + verified.extension(); s3Client.putObject( PutObjectRequest.builder() .bucket(bucket) .key(key) - .contentType(file.getContentType()) + .contentType(verified.contentType()) + .contentLength((long) verified.bytes().length) .build(), - RequestBody.fromBytes(file.getBytes()) + RequestBody.fromBytes(verified.bytes()) ); return "https://storage.yandexcloud.net/" @@ -71,30 +79,31 @@ public String uploadReviewPhoto(MultipartFile file, String userId) { + "/" + key; - } catch (Exception e) { - throw new RuntimeException("Upload failed", e); + } catch (RuntimeException e) { + throw new ApiException(HttpStatus.BAD_GATEWAY, "STORAGE_UNAVAILABLE", "File storage is unavailable"); } } public String uploadVolunteerCertificate(MultipartFile file, String userId) { - try { + VerifiedUpload verified = uploadValidator.validate(file, UploadPurpose.VOLUNTEER_CERTIFICATE); - String ext = getExt(file.getOriginalFilename()); + try { String key = "volunteer-certificates/" + userId + "/" + UUID.randomUUID() - + ext; + + verified.extension(); s3Client.putObject( PutObjectRequest.builder() .bucket(bucket) .key(key) - .contentType(file.getContentType()) + .contentType(verified.contentType()) + .contentLength((long) verified.bytes().length) .build(), - RequestBody.fromBytes(file.getBytes()) + RequestBody.fromBytes(verified.bytes()) ); return "https://storage.yandexcloud.net/" @@ -102,14 +111,8 @@ public String uploadVolunteerCertificate(MultipartFile file, String userId) { + "/" + key; - } catch (Exception e) { - throw new RuntimeException("Upload failed", e); + } catch (RuntimeException e) { + throw new ApiException(HttpStatus.BAD_GATEWAY, "STORAGE_UNAVAILABLE", "File storage is unavailable"); } } - - private String getExt(String name) { - if (name == null) return ""; - int i = name.lastIndexOf("."); - return i == -1 ? "" : name.substring(i); - } } \ No newline at end of file diff --git a/src/main/java/goodroad/validation/UploadValidationService.java b/src/main/java/goodroad/validation/UploadValidationService.java new file mode 100644 index 0000000..634e4d6 --- /dev/null +++ b/src/main/java/goodroad/validation/UploadValidationService.java @@ -0,0 +1,106 @@ +package goodroad.validation; + +import goodroad.api.ApiErrors.ApiException; +import org.springframework.http.HttpStatus; +import org.springframework.stereotype.Service; +import org.springframework.web.multipart.MultipartFile; + +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.util.Set; + +@Service +public class UploadValidationService { + private static final long MAX_FILE_SIZE = 10L * 1024 * 1024; + + public VerifiedUpload validate(MultipartFile file, UploadPurpose purpose) { + if (file == null || file.isEmpty()) { + throw new ApiException(HttpStatus.BAD_REQUEST, "FILE_EMPTY", "Выберите непустой файл для загрузки"); + } + + if (file.getSize() > MAX_FILE_SIZE) { + throw new ApiException(HttpStatus.PAYLOAD_TOO_LARGE, "FILE_TOO_LARGE", "Размер файла не должен превышать 10 МБ"); + } + + byte[] bytes; + try { + bytes = file.getBytes(); + } catch (IOException e) { + throw new ApiException(HttpStatus.UNPROCESSABLE_ENTITY, "FILE_READ_FAILED", "Не удалось прочитать загруженный файл"); + } + + DetectedType detectedType = detectType(bytes); + if (!purpose.allowedTypes.contains(detectedType)) { + throw new ApiException( + HttpStatus.UNSUPPORTED_MEDIA_TYPE, + "FILE_CONTENT_TYPE_INVALID", + purpose == UploadPurpose.VOLUNTEER_CERTIFICATE + ? "Сертификат должен быть файлом JPEG или PNG" + : "Изображение должно быть файлом JPEG, PNG или WEBP" + ); + } + + return new VerifiedUpload(bytes, detectedType.contentType, detectedType.extension); + } + + private DetectedType detectType(byte[] bytes) { + if (startsWith(bytes, new int[] {0xFF, 0xD8, 0xFF})) { + return DetectedType.JPEG; + } + if (startsWith(bytes, new int[] {0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A})) { + return DetectedType.PNG; + } + if (bytes.length >= 12 + && ascii(bytes, 0, 4).equals("RIFF") + && ascii(bytes, 8, 4).equals("WEBP")) { + return DetectedType.WEBP; + } + return DetectedType.UNKNOWN; + } + + private boolean startsWith(byte[] bytes, int[] signature) { + if (bytes.length < signature.length) { + return false; + } + for (int index = 0; index < signature.length; index++) { + if ((bytes[index] & 0xFF) != signature[index]) { + return false; + } + } + return true; + } + + private String ascii(byte[] bytes, int offset, int length) { + return new String(bytes, offset, length, StandardCharsets.US_ASCII); + } + + public enum UploadPurpose { + AVATAR(Set.of(DetectedType.JPEG, DetectedType.PNG, DetectedType.WEBP)), + REVIEW_PHOTO(Set.of(DetectedType.JPEG, DetectedType.PNG, DetectedType.WEBP)), + VOLUNTEER_CERTIFICATE(Set.of(DetectedType.JPEG, DetectedType.PNG)); + + private final Set allowedTypes; + + UploadPurpose(Set allowedTypes) { + this.allowedTypes = allowedTypes; + } + } + + private enum DetectedType { + JPEG("image/jpeg", ".jpg"), + PNG("image/png", ".png"), + WEBP("image/webp", ".webp"), + UNKNOWN("application/octet-stream", ""); + + private final String contentType; + private final String extension; + + DetectedType(String contentType, String extension) { + this.contentType = contentType; + this.extension = extension; + } + } + + public record VerifiedUpload(byte[] bytes, String contentType, String extension) { + } +} diff --git a/src/test/java/goodroad/validation/UploadValidationServiceTest.java b/src/test/java/goodroad/validation/UploadValidationServiceTest.java new file mode 100644 index 0000000..514784f --- /dev/null +++ b/src/test/java/goodroad/validation/UploadValidationServiceTest.java @@ -0,0 +1,51 @@ +package goodroad.validation; + +import goodroad.api.ApiErrors.ApiException; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockMultipartFile; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertThrows; + +class UploadValidationServiceTest { + private final UploadValidationService service = new UploadValidationService(); + + @Test + void acceptsPngBySignatureEvenWhenClientContentTypeIsWrong() { + byte[] png = new byte[] {(byte) 0x89, 0x50, 0x4E, 0x47, 0x0D, 0x0A, 0x1A, 0x0A, 1, 2}; + MockMultipartFile file = new MockMultipartFile("file", "image.bin", "application/octet-stream", png); + + UploadValidationService.VerifiedUpload result = service.validate( + file, + UploadValidationService.UploadPurpose.REVIEW_PHOTO + ); + + assertEquals("image/png", result.contentType()); + assertEquals(".png", result.extension()); + } + + @Test + void rejectsExecutableRenamedToJpeg() { + byte[] executable = new byte[] {'M', 'Z', 1, 2, 3, 4}; + MockMultipartFile file = new MockMultipartFile("file", "certificate.jpg", "image/jpeg", executable); + + ApiException exception = assertThrows(ApiException.class, () -> service.validate( + file, + UploadValidationService.UploadPurpose.VOLUNTEER_CERTIFICATE + )); + + assertEquals("FILE_CONTENT_TYPE_INVALID", exception.code()); + } + + @Test + void rejectsPdfAsVolunteerCertificateToAvoidActiveDocumentContent() { + MockMultipartFile file = new MockMultipartFile( + "file", "certificate.pdf", "application/pdf", "%PDF-1.7".getBytes() + ); + + assertThrows(ApiException.class, () -> service.validate( + file, + UploadValidationService.UploadPurpose.VOLUNTEER_CERTIFICATE + )); + } +} From aa49351443a68b81cbcf69e356b61013d6f5c2bb Mon Sep 17 00:00:00 2001 From: SanriaArgos Date: Mon, 10 Aug 2026 02:24:07 +0300 Subject: [PATCH 7/8] fix(reviews): validate inputs and preserve review consistency --- .../reviews/ReviewValidationService.java | 64 +++++++++++-------- .../goodroad/reviews/UserReviewService.java | 20 +++++- .../reviews/UserReviewServiceTest.java | 8 ++- 3 files changed, 62 insertions(+), 30 deletions(-) diff --git a/src/main/java/goodroad/reviews/ReviewValidationService.java b/src/main/java/goodroad/reviews/ReviewValidationService.java index 5813306..e45bc5d 100644 --- a/src/main/java/goodroad/reviews/ReviewValidationService.java +++ b/src/main/java/goodroad/reviews/ReviewValidationService.java @@ -2,7 +2,9 @@ import goodroad.api.ApiErrors.ApiException; import goodroad.model.ObstacleType; +import goodroad.validation.GeoUtils; import goodroad.validation.InputRules; +import goodroad.validation.TrustedUrlService; import org.springframework.http.HttpStatus; import org.springframework.stereotype.Service; @@ -10,9 +12,15 @@ @Service public class ReviewValidationService { + private final TrustedUrlService trustedUrls; + + public ReviewValidationService(TrustedUrlService trustedUrls) { + this.trustedUrls = trustedUrls; + } public ValidatedReviewInput validate( - UserReviewService.UpsertReviewReq req + UserReviewService.UpsertReviewReq req, + Long userId ) { if (req == null) { @@ -31,15 +39,7 @@ public ValidatedReviewInput validate( ); } - if (Double.isNaN(req.latitude()) - || Double.isNaN(req.longitude())) { - - throw new ApiException( - HttpStatus.BAD_REQUEST, - "REVIEW_COORDS_INVALID", - "Coordinates are invalid" - ); - } + GeoUtils.requireCoordinates(req.latitude(), req.longitude(), "REVIEW_COORDS_INVALID"); UserReviewService.AddressReq address = validateAddress(req.address()); @@ -50,13 +50,14 @@ public ValidatedReviewInput validate( List photoUrls = normalizePhotoUrls( - req.photoUrls() + req.photoUrls(), + userId ); - String comment = - blankToNull( - req.comment() - ); + String comment = blankToNull(req.comment()); + if (comment != null && comment.length() > 1000) { + throw new ApiException(HttpStatus.BAD_REQUEST, "REVIEW_COMMENT_TOO_LONG", "Comment is too long"); + } String primaryObstacleType = choosePrimaryObstacleType( @@ -90,42 +91,42 @@ public ValidatedReviewInput validate( } String country = - InputRules.requireAddressText( + InputRules.requireCyrillicText( raw.country(), "ADDRESS_COUNTRY_INVALID", "Country" ); String region = - InputRules.requireAddressText( + InputRules.requireCyrillicText( raw.region(), "ADDRESS_REGION_INVALID", "Region" ); String localityType = - InputRules.requireAddressText( + InputRules.requireCyrillicText( raw.localityType(), "ADDRESS_LOCALITY_TYPE_INVALID", "Locality type" ); String city = - InputRules.requireAddressText( + InputRules.requireCyrillicText( raw.city(), "ADDRESS_CITY_INVALID", "City" ); String street = - InputRules.requireAddressText( + InputRules.requireCyrillicText( raw.street(), "ADDRESS_STREET_INVALID", "Street" ); String house = - InputRules.requireAddressText( + InputRules.requireDigits( raw.house(), "ADDRESS_HOUSE_INVALID", "House" @@ -135,6 +136,9 @@ public ValidatedReviewInput validate( blankToNull( raw.placeName() ); + if (placeName != null && placeName.length() > 180) { + throw new ApiException(HttpStatus.BAD_REQUEST, "ADDRESS_PLACE_NAME_INVALID", "Place name is too long"); + } return new UserReviewService.AddressReq( country, @@ -243,14 +247,17 @@ public ValidatedReviewInput validate( } private List normalizePhotoUrls( - Collection rawUrls + Collection rawUrls, + Long userId ) { - List out = - new ArrayList<>(); + List out = new ArrayList<>(); if (rawUrls == null) { - return out; + return List.of(); + } + if (rawUrls.size() > 10) { + throw new ApiException(HttpStatus.BAD_REQUEST, "REVIEW_PHOTO_LIMIT_EXCEEDED", "Too many review photos"); } for (String raw : rawUrls) { @@ -259,7 +266,12 @@ private List normalizePhotoUrls( blankToNull(raw); if (value != null) { - out.add(value); + out.add(trustedUrls.requireOwnedStorageUrl( + value, + "reviews", + userId, + "REVIEW_PHOTO_URL_INVALID" + )); } } diff --git a/src/main/java/goodroad/reviews/UserReviewService.java b/src/main/java/goodroad/reviews/UserReviewService.java index 2ee4a19..2bb684e 100644 --- a/src/main/java/goodroad/reviews/UserReviewService.java +++ b/src/main/java/goodroad/reviews/UserReviewService.java @@ -139,7 +139,7 @@ public ReviewCardResp createReview( UserEntity user = findCurrent(phoneFromAuth); ReviewValidationService.ValidatedReviewInput input = - validator.validate(req); + validator.validate(req, user.getId()); ObstacleFeatureEntity feature = featureService.resolveOrCreateFeature( @@ -217,16 +217,21 @@ public ReviewCardResp updateOwnReview( Long oldFeatureId = review.getFeatureId(); ReviewValidationService.ValidatedReviewInput input = - validator.validate(req); + validator.validate(req, user.getId()); ObstacleFeatureEntity feature = featureService.resolveOrCreateFeature(input); + reviews.findByFeatureIdAndAuthorId(feature.getId(), user.getId()) + .filter(existing -> !existing.getId().equals(review.getId())) + .ifPresent(existing -> { + throw new ApiException(HttpStatus.CONFLICT, "REVIEW_ALREADY_EXISTS", "Review already exists"); + }); + review.setFeatureId(feature.getId()); review.setSeverity(input.rating()); review.setText(input.comment()); review.setStatus(STATUS_PENDING); - review.setAwardedPoints(0); review.setModeratorComment(null); reviews.save(review); @@ -234,6 +239,10 @@ public ReviewCardResp updateOwnReview( mapper.saveReviewObstacles(review.getId(), input.obstacles()); mapper.savePhotos(review.getId(), input.photoUrls()); + if (STATUS_APPROVED.equals(oldStatus)) { + reviewSupport.recomputeFeatureAggregate(oldFeatureId); + } + user.setLastActiveAt(Instant.now()); users.save(user); @@ -260,7 +269,12 @@ public void deleteOwnReview( ) ); + Long featureId = review.getFeatureId(); + boolean wasApproved = STATUS_APPROVED.equals(review.getStatus()); reviews.delete(review); + if (wasApproved) { + reviewSupport.recomputeFeatureAggregate(featureId); + } user.setLastActiveAt(Instant.now()); users.save(user); diff --git a/src/test/java/goodroad/reviews/UserReviewServiceTest.java b/src/test/java/goodroad/reviews/UserReviewServiceTest.java index 6a07c0a..e06b525 100644 --- a/src/test/java/goodroad/reviews/UserReviewServiceTest.java +++ b/src/test/java/goodroad/reviews/UserReviewServiceTest.java @@ -8,6 +8,7 @@ import goodroad.storage.StorageService; import goodroad.users.repository.UserEntity; import goodroad.users.repository.UserRepo; +import goodroad.validation.TrustedUrlService; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; @@ -27,6 +28,9 @@ class UserReviewServiceTest { @Mock private UserRepo users; + @Mock + private TrustedUrlService trustedUrls; + @Mock private ObstacleFeatureRepo features; @@ -49,7 +53,9 @@ class UserReviewServiceTest { @BeforeEach void setUp() { - ReviewValidationService validator = new ReviewValidationService(); + lenient().when(trustedUrls.requireOwnedStorageUrl(anyString(), eq("reviews"), anyLong(), eq("REVIEW_PHOTO_URL_INVALID"))) + .thenAnswer(invocation -> invocation.getArgument(0)); + ReviewValidationService validator = new ReviewValidationService(trustedUrls); ReviewFeatureService featureService = new ReviewFeatureService(features); ReviewMapper mapper = new ReviewMapper(reviewSupport, photos, reviewObstacles); From e8b6858e638f39f1937c40b05504a52a679580a1 Mon Sep 17 00:00:00 2001 From: SanriaArgos Date: Mon, 10 Aug 2026 02:25:11 +0300 Subject: [PATCH 8/8] fix(reviews): prevent duplicate moderation rewards --- .../reviews/ReviewModerationService.java | 1 - .../reviews/ReviewModerationServiceTest.java | 17 +++++++++++++++++ 2 files changed, 17 insertions(+), 1 deletion(-) diff --git a/src/main/java/goodroad/reviews/ReviewModerationService.java b/src/main/java/goodroad/reviews/ReviewModerationService.java index 1c0e681..30c07d6 100644 --- a/src/main/java/goodroad/reviews/ReviewModerationService.java +++ b/src/main/java/goodroad/reviews/ReviewModerationService.java @@ -181,7 +181,6 @@ public void reject(String phoneFromAuth, String reviewId, String reason) { } review.setStatus(STATUS_REJECTED); - review.setAwardedPoints(0); review.setTakenByModeratorId(null); review.setTakenAt(null); review.setModeratedBy(moderator.getId()); diff --git a/src/test/java/goodroad/reviews/ReviewModerationServiceTest.java b/src/test/java/goodroad/reviews/ReviewModerationServiceTest.java index a999d89..42eb4aa 100644 --- a/src/test/java/goodroad/reviews/ReviewModerationServiceTest.java +++ b/src/test/java/goodroad/reviews/ReviewModerationServiceTest.java @@ -72,6 +72,23 @@ void shouldApproveTakenReviewAndAddPoints() { verify(reviewSupport).recomputeFeatureAggregate(100L); } + @Test + void shouldNotAwardSameReviewTwiceAfterResubmission() { + UserEntity moderator = user(2L, Role.MODERATOR.name()); + ObstacleReviewEntity review = review(); + review.setTakenByModeratorId(2L); + review.setAwardedPoints(20); + when(users.findByPhoneHash(anyString())).thenReturn(Optional.of(moderator)); + when(reviews.findByIdForUpdate(10L)).thenReturn(Optional.of(review)); + when(photos.existsByReviewId(10L)).thenReturn(true); + + service.approve("+79990000003", "10"); + + assertEquals(20, review.getAwardedPoints()); + verify(users, never()).save(any(UserEntity.class)); + verify(reviewSupport).recomputeFeatureAggregate(100L); + } + @Test void shouldRejectTakenReview() { UserEntity moderator = user(2L, Role.MODERATOR.name());