Conversation
|
Caution Review failedThe pull request is closed. Walkthrough최종 계약서 내보내기/서명/암호화/이메일 전송까지의 신규 워크플로우를 컨트롤러·서비스·매퍼 전반에 추가/대체했습니다. 다수의 DTO/VO/enum이 신설되었고, Mapper XML이 대폭 갱신되었습니다. AES-GCM 기반 파일 암호화·해시 및 Multipart 변환, 숫자 포맷 유틸이 추가되었습니다. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
participant C as Client
participant Ctrl as ContractController
participant Svc as ContractService
participant Map as ContractMapper
participant S3 as S3
participant AI as AI Server
participant R as Redis
C->>Ctrl: POST /contract/{id}/final_contract
Ctrl->>Svc: finalContractPDF(contractChatId, userId)
Svc->>Map: selectFinalContractPDF/selectFinalContract
Svc->>R: get step/payment state
Svc->>AI: generate final PDF (payload DTO)
AI-->>Svc: PDF bytes
Svc->>S3: upload PDF
S3-->>Svc: s3Key, hash
Svc->>Map: updateFinalContract(s3Key, hash)
Svc-->>Ctrl: MultipartFile (PDF)
Ctrl-->>C: 200 PDF
sequenceDiagram
autonumber
participant C as Client
participant Ctrl as ContractController
participant Svc as ContractService
participant AES as ImgAesCryptoUtil
participant S3 as S3
participant Map as ContractMapper
C->>Ctrl: POST /contract/{id}/signature (dto + image)
Ctrl->>Svc: saveSignature(id, userId, dto, img)
Svc->>AES: encrypt img, sha256
AES-->>Svc: base64Cipher, hash
Svc->>S3: upload encrypted image
S3-->>Svc: s3Key
Svc->>Map: insertSignature(id, s3Key, hash, signedType, userId)
Svc-->>Ctrl: Boolean (bothSigned?)
Ctrl-->>C: 200
sequenceDiagram
autonumber
participant C as Client
participant Ctrl as ContractController
participant Svc as ContractService
participant Map as ContractMapper
participant Mail as Mailer
C->>Ctrl: POST /contract/{id}/email (FindContractDTO)
Ctrl->>Svc: sendContractPDF(id, userId, dto)
Svc->>Map: selectFinalContract/selectMail
Svc-->>Svc: build password-protected PDF bytes
Svc->>Mail: send email with attachment
Mail-->>Svc: ok
Svc-->>Ctrl: Void
Ctrl-->>C: 200
Estimated code review effort🎯 5 (Critical) | ⏱️ ~120 minutes Assessment against linked issues
Assessment against linked issues: Out-of-scope changes
Possibly related PRs
Suggested reviewers
Poem
Tip 🔌 Remote MCP (Model Context Protocol) integration is now available!Pro plan users can now connect to remote MCP servers from the Integrations page. Connect with popular remote MCPs such as Notion and Linear to add more context to your reviews and chats. 📜 Recent review detailsConfiguration used: CodeRabbit UI 💡 Knowledge Base configuration:
You can enable these sources in your CodeRabbit configuration. 📒 Files selected for processing (23)
✨ Finishing Touches
🧪 Generate unit tests
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR/Issue comments)Type Other keywords and placeholders
CodeRabbit Configuration File (
|
![github-advanced-security[bot]](https://avatars.githubusercontent.com/in/57789?s=60&v=4){kind=small}
|
|
||
| @Override | ||
| @GetMapping("/final_contract") | ||
| public ResponseEntity<ApiResponse<MultipartFile>> finalContractPDF( |
Check failure
Code scanning / CodeQL
HTTP request type unprotected from CSRF High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix this issue, the HTTP method for the finalContractPDF endpoint should be changed from GET to POST (or another "unsafe" method like PUT or PATCH) to ensure that Spring's default CSRF protection applies. This involves changing the @GetMapping("/final_contract") annotation to @PostMapping("/final_contract") on the finalContractPDF method in ContractControllerImpl.java. No changes are needed in the service layer, as the vulnerability is in the controller's HTTP method exposure. If clients are using this endpoint, they will need to update their requests to use POST instead of GET.
Steps:
- In
ContractControllerImpl.java, locate thefinalContractPDFmethod. - Change
@GetMapping("/final_contract")to@PostMapping("/final_contract"). - No additional imports or method changes are required.
| @@ -221,7 +221,7 @@ | ||
| // } | ||
|
|
||
| @Override | ||
| @GetMapping("/final_contract") | ||
| @PostMapping("/final_contract") | ||
| public ResponseEntity<ApiResponse<MultipartFile>> finalContractPDF( | ||
| @PathVariable Long contractChatId, | ||
| @AuthenticationPrincipal CustomUserDetails userDetails) { |
| restTemplate.exchange(url, HttpMethod.POST, requestEntity, LegalityDTO.class); | ||
| LegalityDTO res = response.getBody(); | ||
| assert res != null; | ||
| log.warn("AI 응답 값 확인: {}", res.toString()); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the log injection vulnerability, we should sanitize the data before logging it, just as is done in the error path. Specifically, before logging the response object (res) in the success path, we should serialize it to a JSON string and remove any newline (\n) or carriage return (\r) characters. This ensures that even if the response contains malicious content, it cannot break the log format or inject additional log entries. The fix should be applied to line 473 in ContractServiceImpl.java. We can reuse the same approach as in the error path: use ObjectMapper to serialize the object, and then replace any newline or carriage return characters with spaces before logging.
No changes are needed in LegalityDTO.java.
We may need to ensure that ObjectMapper is available in the method scope.
| @@ -470,7 +470,16 @@ | ||
| restTemplate.exchange(url, HttpMethod.POST, requestEntity, LegalityDTO.class); | ||
| LegalityDTO res = response.getBody(); | ||
| assert res != null; | ||
| log.warn("AI 응답 값 확인: {}", res.toString()); | ||
| // Sanitize AI response before logging to prevent log injection | ||
| String sanitizedResStr; | ||
| try { | ||
| ObjectMapper objectMapper = new ObjectMapper(); | ||
| sanitizedResStr = objectMapper.writeValueAsString(res); | ||
| } catch (Exception ex) { | ||
| sanitizedResStr = String.valueOf(res); | ||
| } | ||
| sanitizedResStr = sanitizedResStr.replaceAll("[\\r\\n]", " "); | ||
| log.warn("AI 응답 값 확인: {}", sanitizedResStr); | ||
|
|
||
| log.warn("AI 응답 헤더 확인: {}", response.getStatusCode()); | ||
| if (response.getStatusCode().is2xxSuccessful() && response.getBody() != null) { |
| } | ||
| // Remove newlines and carriage returns | ||
| responseBodyStr = responseBodyStr.replaceAll("[\\r\\n]", " "); | ||
| log.error(responseBodyStr); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fully mitigate log injection, all user-controlled or external data written to logs should be sanitized to remove not only newlines and carriage returns, but also other control characters that could be used to forge log entries or disrupt log parsing. The best way to fix this is to implement a utility method that strips or escapes all non-printable characters from the string before logging. This method should be applied to responseBodyStr before it is passed to the logger. The fix should be made in the getLegality method of ContractServiceImpl.java, specifically in the block where responseBodyStr is logged. If a suitable utility method does not already exist, it should be defined in this file or imported from a well-known library. For this fix, we will define a private static method in the class to sanitize log messages, and use it before logging responseBodyStr.
| @@ -54,6 +54,16 @@ | ||
|
|
||
| @Service | ||
| @RequiredArgsConstructor | ||
|
|
||
| /** | ||
| * Sanitizes a string for safe logging by removing control characters that could be used for log injection. | ||
| * This includes newlines, carriage returns, and other non-printable characters. | ||
| */ | ||
| private static String sanitizeForLog(String input) { | ||
| if (input == null) return null; | ||
| // Remove all control characters except tab (\\t) if desired | ||
| return input.replaceAll("[\\p{Cntrl}&&[^\t]]", ""); | ||
| } | ||
| @Log4j2 | ||
| public class ContractServiceImpl implements ContractService { | ||
|
|
||
| @@ -484,8 +494,8 @@ | ||
| } catch (Exception ex) { | ||
| responseBodyStr = String.valueOf(response.getBody()); | ||
| } | ||
| // Remove newlines and carriage returns | ||
| responseBodyStr = responseBodyStr.replaceAll("[\\r\\n]", " "); | ||
| // Remove control characters to prevent log injection | ||
| responseBodyStr = sanitizeForLog(responseBodyStr); | ||
| log.error(responseBodyStr); | ||
| throw new BusinessException(ContractException.CONTRACT_AI_SERVER_ERROR); | ||
| } |
| if (response.getStatusCode().is2xxSuccessful() && response.getBody() != null) { | ||
| // 응답 바이트를 파일로 저장 | ||
| byte[] fileBytes = response.getBody(); | ||
| File tempFile = File.createTempFile("contract_", ".pdf"); |
Check warning
Code scanning / CodeQL
Local information disclosure in a temporary directory Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, the temporary file should be created with permissions that restrict access to only the owner. The best way to do this is to use Files.createTempFile from the java.nio.file package, which by default creates files with permissions -rw------- on Unix-like systems. This method returns a Path, which can be converted to a File if needed. The code on line 763 should be replaced with a call to Files.createTempFile("contract_", ".pdf"), and the rest of the code should use the resulting Path (or convert it to a File if required by downstream APIs). No change in functionality is required, only the method of file creation.
You will need to:
- Replace
File.createTempFile("contract_", ".pdf")withFiles.createTempFile("contract_", ".pdf"). - Convert the resulting
Pathto aFileif needed (e.g., forMultipartFileUtils.fromFile). - Ensure that the import for
java.nio.file.Filesis present (it already is). - No additional dependencies are required.
| @@ -760,9 +760,9 @@ | ||
| if (response.getStatusCode().is2xxSuccessful() && response.getBody() != null) { | ||
| // 응답 바이트를 파일로 저장 | ||
| byte[] fileBytes = response.getBody(); | ||
| File tempFile = File.createTempFile("contract_", ".pdf"); | ||
| Files.write(tempFile.toPath(), fileBytes); | ||
| result = MultipartFileUtils.fromFile(tempFile, "contract.pdf", "application/pdf"); | ||
| java.nio.file.Path tempFilePath = Files.createTempFile("contract_", ".pdf"); | ||
| Files.write(tempFilePath, fileBytes); | ||
| result = MultipartFileUtils.fromFile(tempFilePath.toFile(), "contract.pdf", "application/pdf"); | ||
|
|
||
| // s3에 파일 업로드 하기 | ||
| String key = s3Service.uploadFile(result); |
| } | ||
| // Remove newlines and carriage returns | ||
| responseBodyStr = responseBodyStr.replaceAll("[\\r\\n]", " "); | ||
| log.error(responseBodyStr); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fully mitigate log injection, the log entry should:
- Sanitize the response body string by removing or escaping all control characters that could affect log structure (not just newlines and carriage returns, but also tabs and other non-printable characters).
- Clearly mark the user-controlled data in the log entry, so that it cannot be confused with other log entries.
- Use a structured logging format, such as including a label or context in the log message.
The best way to fix the problem is to:
- Further sanitize
responseBodyStrby removing all non-printable/control characters (e.g., using a regex to keep only printable characters). - Log the value with a clear label, e.g.,
AI server error response: [sanitized value]. - Make this change only in the block where the log entry is written (line 785).
No new imports are needed, as the required functionality can be implemented with standard Java methods.
| @@ -781,8 +781,9 @@ | ||
| responseBodyStr = String.valueOf(response.getBody()); | ||
| } | ||
| // Remove newlines and carriage returns | ||
| responseBodyStr = responseBodyStr.replaceAll("[\\r\\n]", " "); | ||
| log.error(responseBodyStr); | ||
| // Remove all control characters (including newlines, carriage returns, tabs, etc.) | ||
| responseBodyStr = responseBodyStr.replaceAll("[\\p{Cntrl}]", " "); | ||
| log.error("AI server error response: [{}]", responseBodyStr); | ||
| throw new BusinessException(ContractException.CONTRACT_AI_SERVER_ERROR); | ||
| } | ||
|
|
| if (response.getStatusCode().is2xxSuccessful() && response.getBody() != null) { | ||
| // 응답 바이트를 파일로 저장 | ||
| byte[] fileBytes = response.getBody(); | ||
| tempFile = File.createTempFile("contract_", ".pdf"); |
Check warning
Code scanning / CodeQL
Local information disclosure in a temporary directory Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, replace the use of File.createTempFile with the secure alternative from the JDK: Files.createTempFile. This method creates a file with permissions set to be readable and writable only by the owner (-rw-------) on Unix-like systems. The returned object is a Path, but you can convert it to a File if needed.
Specifically, in src/main/java/org/scoula/domain/contract/service/ContractServiceImpl.java, replace the line:
tempFile = File.createTempFile("contract_", ".pdf");with:
tempFile = Files.createTempFile("contract_", ".pdf").toFile();No additional imports are needed since java.nio.file.Files is already imported. This change ensures the temporary file is created with secure permissions, preventing local information disclosure.
| @@ -1139,7 +1139,7 @@ | ||
| if (response.getStatusCode().is2xxSuccessful() && response.getBody() != null) { | ||
| // 응답 바이트를 파일로 저장 | ||
| byte[] fileBytes = response.getBody(); | ||
| tempFile = File.createTempFile("contract_", ".pdf"); | ||
| tempFile = Files.createTempFile("contract_", ".pdf").toFile(); | ||
| Files.write(tempFile.toPath(), fileBytes); | ||
|
|
||
| } else { |
| } | ||
| // Remove newlines and carriage returns | ||
| responseBodyStr = responseBodyStr.replaceAll("[\\r\\n]", " "); | ||
| log.error(responseBodyStr); |
Check failure
Code scanning / CodeQL
Log Injection High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fully mitigate log injection, we should sanitize the response body string before logging by removing all control characters (not just newlines and carriage returns). This can be done using a regular expression that removes all characters in the Unicode control character range (\p{Cntrl}), which includes newlines, carriage returns, tabs, and other non-printable characters. Additionally, it is good practice to clearly mark user input in log entries, e.g., by prefixing with a label. The fix should be applied in the block where responseBodyStr is logged (lines 1146–1157). No new methods are needed, but we should update the sanitization logic.
| @@ -1151,9 +1151,9 @@ | ||
| } catch (Exception ex) { | ||
| responseBodyStr = String.valueOf(response.getBody()); | ||
| } | ||
| // Remove newlines and carriage returns | ||
| responseBodyStr = responseBodyStr.replaceAll("[\\r\\n]", " "); | ||
| log.error(responseBodyStr); | ||
| // Remove all control characters (including newlines, carriage returns, tabs, etc.) | ||
| responseBodyStr = responseBodyStr.replaceAll("\\p{Cntrl}", " "); | ||
| log.error("AI server error response body: [{}]", responseBodyStr); | ||
| throw new BusinessException(ContractException.CONTRACT_AI_SERVER_ERROR); | ||
| } | ||
|
|
| public static File toTempFile(MultipartFile multipart, String prefix, String suffix) | ||
| throws IOException { | ||
| Objects.requireNonNull(multipart, "multipart must not be null"); | ||
| File temp = File.createTempFile(prefix, suffix); |
Check warning
Code scanning / CodeQL
Local information disclosure in a temporary directory Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, replace the use of File.createTempFile(prefix, suffix) with Files.createTempFile(prefix, suffix), which returns a Path with secure permissions. Then, convert the resulting Path to a File using .toFile() to preserve the method's return type. This change should be made in both toTempFile (line 30) and inputStreamToTempFile (line 49). No additional imports are needed, as java.nio.file.Files is already imported. This change will ensure that temporary files are created with restrictive permissions, mitigating the local information disclosure risk, while preserving existing functionality.
| @@ -27,7 +27,7 @@ | ||
| public static File toTempFile(MultipartFile multipart, String prefix, String suffix) | ||
| throws IOException { | ||
| Objects.requireNonNull(multipart, "multipart must not be null"); | ||
| File temp = File.createTempFile(prefix, suffix); | ||
| File temp = Files.createTempFile(prefix, suffix).toFile(); | ||
| try (InputStream in = multipart.getInputStream()) { | ||
| Files.copy(in, temp.toPath(), java.nio.file.StandardCopyOption.REPLACE_EXISTING); | ||
| } | ||
| @@ -46,7 +46,7 @@ | ||
| if (prefix == null || prefix.isBlank()) prefix = "contract_"; | ||
| if (suffix == null || suffix.isBlank()) suffix = ".pdf"; | ||
|
|
||
| File temp = File.createTempFile(prefix, suffix); | ||
| File temp = Files.createTempFile(prefix, suffix).toFile(); | ||
| try (InputStream src = in) { | ||
| Files.copy(src, temp.toPath(), java.nio.file.StandardCopyOption.REPLACE_EXISTING); | ||
| } |
| if (prefix == null || prefix.isBlank()) prefix = "contract_"; | ||
| if (suffix == null || suffix.isBlank()) suffix = ".pdf"; | ||
|
|
||
| File temp = File.createTempFile(prefix, suffix); |
Check warning
Code scanning / CodeQL
Local information disclosure in a temporary directory Medium
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, we should use the java.nio.file.Files.createTempFile method, which creates temporary files with secure permissions (-rw-------) by default on Unix-like systems. This method returns a Path, which can be converted to a File if needed. We should replace all instances of File.createTempFile(prefix, suffix) with Files.createTempFile(prefix, suffix).toFile(). This change should be made in both the toTempFile and inputStreamToTempFile methods. No additional imports are needed, as java.nio.file.Files is already imported.
| @@ -27,7 +27,7 @@ | ||
| public static File toTempFile(MultipartFile multipart, String prefix, String suffix) | ||
| throws IOException { | ||
| Objects.requireNonNull(multipart, "multipart must not be null"); | ||
| File temp = File.createTempFile(prefix, suffix); | ||
| File temp = Files.createTempFile(prefix, suffix).toFile(); | ||
| try (InputStream in = multipart.getInputStream()) { | ||
| Files.copy(in, temp.toPath(), java.nio.file.StandardCopyOption.REPLACE_EXISTING); | ||
| } | ||
| @@ -46,7 +46,7 @@ | ||
| if (prefix == null || prefix.isBlank()) prefix = "contract_"; | ||
| if (suffix == null || suffix.isBlank()) suffix = ".pdf"; | ||
|
|
||
| File temp = File.createTempFile(prefix, suffix); | ||
| File temp = Files.createTempFile(prefix, suffix).toFile(); | ||
| try (InputStream src = in) { | ||
| Files.copy(src, temp.toPath(), java.nio.file.StandardCopyOption.REPLACE_EXISTING); | ||
| } |
|
|
||
| private static String probeContentType(File file) { | ||
| try { | ||
| String ct = Files.probeContentType(file.toPath()); |
Check failure
Code scanning / CodeQL
Uncontrolled data used in path expression High
Show autofix suggestion
Hide autofix suggestion
Copilot Autofix
AI 6 months ago
To fix the problem, we need to validate the extension extracted from the user-controlled filename before using it in File.createTempFile. Specifically, we should ensure that the extension does not contain any path separators (/, \), parent directory references (..), or other potentially dangerous characters. The extension should match a safe pattern, such as starting with a dot and containing only alphanumeric characters (and possibly a limited set of other safe characters like underscores or hyphens). If the extension fails validation, we should default to a safe value (e.g., .pdf). The validation should be added in EncryptionServiceImpl.java where the extension is extracted and used.
| @@ -331,8 +331,8 @@ | ||
| } | ||
| } | ||
|
|
||
| // 확장자가 없으면 기본값 설정 | ||
| if (extension.isEmpty()) { | ||
| // 확장자가 없거나, 확장자가 안전하지 않으면 기본값 설정 | ||
| if (extension.isEmpty() || !isSafeExtension(extension)) { | ||
| extension = ".pdf"; // PDF의 기본 확장자 | ||
| } | ||
|
|
||
| @@ -353,6 +353,15 @@ | ||
| return tempFile; | ||
| } | ||
|
|
||
| /** | ||
| * 확장자가 안전한지 검사 (".pdf", ".png" 등, 점으로 시작하고 알파벳/숫자/언더스코어/하이픈만 허용) | ||
| */ | ||
| private static boolean isSafeExtension(String ext) { | ||
| if (ext == null) return false; | ||
| // Must start with a dot, and only contain [a-zA-Z0-9_-] after the dot | ||
| return ext.matches("^\\.[a-zA-Z0-9_-]+$"); | ||
| } | ||
|
|
||
| /** 2-of-3 암호화된 PDF 복호화 커스텀 파일 포맷에서 shares를 추출하여 복호화 */ | ||
| private byte[] decrypt2Of3Pdf(byte[] fileData, String password) throws Exception { | ||
| log.info("Starting 2-of-3 PDF decryption"); |
1 participant
🚀 관련 이슈
🔑 주요 변경사항
✔️ 체크 리스트
mainbranch에 실수로 PR 생성 금지)📢 To Reviewers
📸 스크린샷 or 실행영상
Summary by CodeRabbit