-
Notifications
You must be signed in to change notification settings - Fork 0
[Refactor] s3 이미지 업로드 방식을 Presigned URL로 수정 #288
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
Changes from all commits
02294a5
99072b2
898d69f
1f8b39d
8d55e7b
31cb3c6
9d2329b
ce079e7
1dd5af6
35bb345
3dd8bc6
b2e5d20
ba0c2b8
27ef6a6
4325330
d51471b
102db8c
63e8531
699414d
d4f542a
76081cc
37a41f6
370d7e9
bfb8c77
97291a7
2656ffd
b6935c8
3b7a375
8f5803e
cc97c23
d3a04ca
c1390ea
d991e82
248f5ee
25a9738
3577d94
e16a57b
c04be0a
88f92d7
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,57 @@ | ||
| package konkuk.thip.common.s3.service; | ||
|
|
||
| import konkuk.thip.common.annotation.application.HelperService; | ||
| import konkuk.thip.common.exception.BusinessException; | ||
| import lombok.RequiredArgsConstructor; | ||
| import org.springframework.beans.factory.annotation.Value; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| import static konkuk.thip.common.exception.code.ErrorCode.URL_INVALID_DOMAIN; | ||
| import static konkuk.thip.common.exception.code.ErrorCode.URL_USER_ID_MISMATCH; | ||
|
|
||
| @HelperService | ||
| @RequiredArgsConstructor | ||
| public class ImageUrlValidationService { | ||
|
Comment on lines
+13
to
+15
Collaborator
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. 확인했습니다 |
||
|
|
||
| @Value("${cloud.aws.s3.cloud-front-base-url}") | ||
| private String cloudFrontBaseUrl; | ||
|
|
||
| public void validateUrlDomainAndUser(List<String> imageUrls, Long currentUserId) { | ||
|
|
||
| if (imageUrls == null || imageUrls.isEmpty()) { | ||
| return; | ||
| } | ||
|
|
||
| for (String url : imageUrls) { | ||
| // 1. 도메인 시작 주소 확인 | ||
| if (!url.startsWith(cloudFrontBaseUrl)) { | ||
| throw new BusinessException(URL_INVALID_DOMAIN); | ||
| } | ||
| // 2. 도메인 이후의 key 추출 | ||
| String key = url.substring(cloudFrontBaseUrl.length()); | ||
| // 3. key를 '/'로 분리 | ||
| if (key.startsWith("/")) { | ||
| key = key.substring(1); // 맨 앞 슬래시 제거 | ||
| } | ||
| String[] parts = key.split("/"); | ||
|
|
||
| // 4. 경로 구조 확인 | ||
| // feed/{userId}/{date}/{uuid + filename} | ||
| if (parts.length != 4) { | ||
| throw new BusinessException(URL_INVALID_DOMAIN); | ||
| } | ||
|
|
||
| // 5. userId와 비교 | ||
| String userIdInUrl = parts[1]; | ||
| try { | ||
| Long userIdFromUrl = Long.parseLong(userIdInUrl); | ||
| if (!userIdFromUrl.equals(currentUserId)) { | ||
| throw new BusinessException(URL_USER_ID_MISMATCH); | ||
| } | ||
| } catch (NumberFormatException e) { | ||
| throw new BusinessException(URL_USER_ID_MISMATCH); | ||
| } | ||
| } | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -17,13 +17,13 @@ public class AwsS3ImageUrlInitializer { | |
| private final AwsS3Properties awsS3Properties; | ||
|
|
||
| @PostConstruct | ||
| void bindBucketBaseUrl() { | ||
| String baseUrl = awsS3Properties.bucketBaseUrl(); | ||
| void bindCloudFrontBaseUrl() { | ||
| String baseUrl = awsS3Properties.cloudFrontBaseUrl(); | ||
| if (baseUrl == null || baseUrl.isEmpty()) { | ||
| throw new BusinessException(AWS_BUCKET_BASE_URL_NOT_CONFIGURED); | ||
| } | ||
|
|
||
| Alias.registerBaseUrlSupplier(awsS3Properties::bucketBaseUrl); | ||
| Category.registerBaseUrlSupplier(awsS3Properties::bucketBaseUrl); | ||
| Alias.registerBaseUrlSupplier(awsS3Properties::cloudFrontBaseUrl); | ||
| Category.registerBaseUrlSupplier(awsS3Properties::cloudFrontBaseUrl); | ||
|
Comment on lines
+26
to
+27
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. 🛠️ Refactor suggestion 트레일링 슬래시를 정규화하지 않으면 '//' 이중 슬래시 URL이 생성될 수 있습니다. 등록 전 한 번 정규화해서 두 도메인 객체에 동일하게 주입하세요. - Alias.registerBaseUrlSupplier(awsS3Properties::cloudFrontBaseUrl);
- Category.registerBaseUrlSupplier(awsS3Properties::cloudFrontBaseUrl);
+ String normalizedBaseUrl = baseUrl.endsWith("/") ? baseUrl.substring(0, baseUrl.length() - 1) : baseUrl;
+ Alias.registerBaseUrlSupplier(() -> normalizedBaseUrl);
+ Category.registerBaseUrlSupplier(() -> normalizedBaseUrl);
🤖 Prompt for AI Agents
Contributor
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. 코드래빗이 말하는 것처럼 현재 cloud-front-base-url의 환경변수 마지막이 슬래쉬(/)로 끝나고, Alias에서 사진 url의 시작이 슬래쉬(/)로 시작하는데 이중 슬래쉬 이슈는 없을까요?? 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. 🧩 Analysis chain@buzz0331 좋은 지적입니다! 이중 슬래시 문제를 확인해보겠습니다. 네, 말씀하신 우려가 정확합니다! 검색 결과를 보면 // Alias 클래스에서
return baseUrlSupplier.get() + "/user/" + userId + "/alias/" + value + ".jpg";
// Category 클래스에서
return baseUrlSupplier.get() + "/room/" + value + ".jpg";만약
이렇게 이중 슬래시( CodeRabbit의 제안이 정확합니다. 다음과 같이 수정하는 것이 좋겠습니다: @PostConstruct
void bindCloudFrontBaseUrl() {
String baseUrl = awsS3Properties.cloudFrontBaseUrl();
if (baseUrl == null || baseUrl.isEmpty()) {
throw new BusinessException(AWS_BUCKET_BASE_URL_NOT_CONFIGURED);
}
// 트레일링 슬래시 정규화
String normalizedBaseUrl = baseUrl.endsWith("/") ? baseUrl.substring(0, baseUrl.length() - 1) : baseUrl;
Alias.registerBaseUrlSupplier(() -> normalizedBaseUrl);
Category.registerBaseUrlSupplier(() -> normalizedBaseUrl);
}이렇게 하면 환경변수 설정과 관계없이 항상 올바른 URL이 생성됩니다. 🏁 Script executed: Length of output: 44 🏁 Script executed: Length of output: 4903 🏁 Script executed: Length of output: 37 🏁 Script executed: Length of output: 1297 🏁 Script executed: Length of output: 1649 @buzz0331 네, 정확히 맞습니다! 코드를 확인해보니 이중 슬래시 문제가 실제로 발생할 수 있습니다. 문제 상황 분석Alias 클래스 (Line 40): public String getImageUrl() {
return baseUrlSupplier.get() + imageUrl;
}Category 클래스 (Line 49): public String getImageUrl() {
return baseUrlSupplier.get() + imageUrl;
}두 클래스 모두에서
만약 환경변수 해결 방안CodeRabbit의 제안대로 트레일링 슬래시를 정규화하는 것이 정확한 해결책입니다: @PostConstruct
void bindCloudFrontBaseUrl() {
String baseUrl = awsS3Properties.cloudFrontBaseUrl();
if (baseUrl == null || baseUrl.isEmpty()) {
throw new BusinessException(AWS_BUCKET_BASE_URL_NOT_CONFIGURED);
}
// 트레일링 슬래시 정규화 - 이중 슬래시 방지
String normalizedBaseUrl = baseUrl.endsWith("/") ? baseUrl.substring(0, baseUrl.length() - 1) : baseUrl;
Alias.registerBaseUrlSupplier(() -> normalizedBaseUrl);
Category.registerBaseUrlSupplier(() -> normalizedBaseUrl);
}이렇게 수정하면 환경변수 설정과 관계없이 항상 올바른 URL이 생성됩니다:
좋은 지적이었습니다! 🎯
Member
Author
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. 앗 아눃 환경변수 값에서 /뻇습니닿ㅎㅎ 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.
|
||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -8,8 +8,8 @@ | |
| @ConfigurationProperties(prefix = "cloud.aws.s3") | ||
| @Validated | ||
| public record AwsS3Properties( | ||
| // 스프링 컨텍스트 시작 시점에 bucket-base-url의 값 유효성 검사 실행 | ||
| @NotBlank(message = "bucket-base-url 은 비어있을 수 없습니다.") | ||
| @URL(message = "bucket-base-url 의 형식이 올바르지 않습니다.") | ||
| String bucketBaseUrl | ||
| // 스프링 컨텍스트 시작 시점에 cloud-front-base-url의 값 유효성 검사 실행 | ||
| @NotBlank(message = "cloud-front-base-url 은 비어있을 수 없습니다.") | ||
| @URL(message = "cloud-front-base-url 의 형식이 올바르지 않습니다.") | ||
| String cloudFrontBaseUrl | ||
|
Comment on lines
+11
to
+14
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. 💡 Verification agent 🧩 Analysis chain설정 키 변경에 따른 하위 호환 및 문서/환경 변수 갱신 확인 필요. 기존 bucket-base-url → cloud-front-base-url로 바뀌었으므로 application.yml/운영 환경 변수/문서에서 누락된 레거시 키가 없는지 점검해 주세요. 필요 시 마이그레이션 가이드를 PR 설명에 명시해주세요. 아래 스크립트로 레거시 용례를 빠르게 확인할 수 있습니다. 🏁 Script executed: #!/bin/bash
# 레거시 키/식별자 흔적 찾기
rg -n -C2 -S 'bucket-base-url|bucketBaseUrl|AWS_BUCKET_BASE_URL_NOT_CONFIGURED'
# 설정 파일에서 현재 키 사용 여부 확인
fd -a 'application*.yml' | xargs -I{} rg -n 'cloud\.aws\.s3\.(cloud-front-base-url|bucket-base-url)' {}Length of output: 1786 애플리케이션 설정·문서·예외코드 레거시 키 갱신 필요
🤖 Prompt for AI Agents |
||
| ) { } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| package konkuk.thip.feed.adapter.in.web.request; | ||
|
|
||
|
|
||
| import io.swagger.v3.oas.annotations.media.Schema; | ||
| import jakarta.validation.constraints.NotBlank; | ||
| import jakarta.validation.constraints.Positive; | ||
|
|
||
| @Schema(description = "피드 생성시 이미지 업로드용 presigned url발급 요청 DTO") | ||
| public record FeedUploadImagePresignedUrlRequest( | ||
|
|
||
| @Schema(description = "파일 이름 (확장자명 포함)", example = "image1.png") | ||
| @NotBlank(message = "파일 이름은 필수입니다.") | ||
| String filename, | ||
|
|
||
| @Schema(description = "파일 확장자 [허용 확장자 jpg, jpeg, png, gif]", example = "png") | ||
| @NotBlank(message = "파일 확장자는 필수입니다.") | ||
| String extension, | ||
|
|
||
| @Schema(description = "파일 크기 (바이트) [허용 파일 최대 크기 5MB]", example = "102400") | ||
| @Positive(message = "파일 크기는 양수여야 합니다.") | ||
| long size | ||
| ) { | ||
| } |
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,23 @@ | ||
| package konkuk.thip.feed.adapter.in.web.response; | ||
|
|
||
| import io.swagger.v3.oas.annotations.media.Schema; | ||
|
|
||
| import java.util.List; | ||
|
|
||
| public record FeedUploadImagePresignedUrlResponse( | ||
| List<PresignedUrlInfo> presignedUrls | ||
| ) { | ||
| public static FeedUploadImagePresignedUrlResponse of(List<PresignedUrlInfo> urls) { | ||
| return new FeedUploadImagePresignedUrlResponse(urls); | ||
| } | ||
|
|
||
| @Schema(description = "Presigned URL 및 파일 접근 URL 정보") | ||
| public record PresignedUrlInfo( | ||
|
|
||
| @Schema(description = "PUT 요청을 보내기 위한 파일 업로드용 Presigned URL") | ||
| String presignedUrl, | ||
|
Contributor
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. 👍🏻 |
||
|
|
||
| @Schema(description = "업로드 후 접근 가능한 파일 URL") | ||
| String fileUrl | ||
|
Comment on lines
+20
to
+21
Collaborator
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. p3 : 응답에 업로드 이후 접근 가능한 파일 url을 포함하신 이유가 있을까요?? FE에게 fileUrl 을 응답한 후, 다시 피드 생성 요청시에 fileUrl을 request body로 받는 것 보다는, FE에게 presignedUrl과 key(= presignedUrl을 생성할 때 사용한 key) 를 응답하고, 피드 생성 요청시에 key를 request body로 받아 내부적으로 key에 해당하는 fileUrl을 DB에 저장하는 플로우는 어떤가 싶습니다!! 현재 구현된 코드에서는 피드 생성 request 에 포함된 fileUrl 을 검증하는 부분이 누락되어 있는데, 위 방식으로 수정되면 내부에서 request 에 포함된 key에 해당하는 fileUrl 을 획득하는 과정에서 key 값의 검증과정이 포함되므로 더 안전할 것 같습니다!! 제가 생각한 피드 생성 플로우를 간략하게 추가해보았습니다!
Contributor
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. 굿굿 깔끔하게 정리해주셨네요! 성준님 말대로 접근 가능한 파일 url이 노출될 필요는 없어보이네요! 그리고 앞 리뷰에서 key를 구조화 시키면 성준님이 작성하신 것 중 5번에 해당하는 사용자 prefix(userId) 검증도 가능할 것 같습니다! |
||
| ) {} | ||
| } | ||
This file was deleted.
Uh oh!
There was an error while loading. Please reload this page.