From 837e6ca9466b31a0650d9571a130db6c700d9a1d Mon Sep 17 00:00:00 2001 From: seonghobae <8172694+seonghobae@users.noreply.github.com> Date: Wed, 22 Jul 2026 21:29:30 +0000 Subject: [PATCH] =?UTF-8?q?=F0=9F=9B=A1=EF=B8=8F=20Sentinel:=20[CRITICAL]?= =?UTF-8?q?=20=EA=B4=80=EB=A6=AC=EC=9E=90=20=EC=97=94=EB=93=9C=ED=8F=AC?= =?UTF-8?q?=EC=9D=B8=ED=8A=B8=EC=97=90=20=EC=9D=B8=EC=A6=9D=20=EC=B6=94?= =?UTF-8?q?=EA=B0=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 관리자 엔드포인트(`AdminController`)에 적절한 인증 및 권한 확인이 부족하여, 인증되지 않은 사용자가 모든 변환 작업을 읽거나 삭제하고 재시도할 수 있는 보안 취약점이 있었습니다. TenantAccessService를 주입하고 모든 엔드포인트(getAllJobs, deleteJob, retryDeadLettered)에서 `tenantAccessService.require(...)`를 호출하여 적절한 권한(`ADMIN_READ`, `ADMIN_WRITE`)이 있는지 확인하도록 수정했습니다. `TenantPermissions` 클래스에 관련 상수도 추가했습니다. 관련된 테스트도 모의 컨텍스트를 제공하도록 업데이트했습니다. --- .jules/sentinel.md | 42 ++++--------------- .../viewer/auth/TenantPermissions.java | 10 +++++ .../viewer/controller/AdminController.java | 18 ++++++-- .../controller/AdminControllerTest.java | 17 +++++++- 4 files changed, 48 insertions(+), 39 deletions(-) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index e795cb9d..9bfed034 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -1,34 +1,8 @@ -## 2026-06-30 - Prevent DOM-based XSS in Viewer JS -**Vulnerability:** Untrusted paths from API responses were directly assigned to `a.href` and used in `iframe` generation, which allows execution of malicious URIs like `javascript:` or `data:`. -**Learning:** Even when avoiding `innerHTML`, directly setting URL-like strings to DOM attributes without protocol validation introduces XSS vectors. The payload can be executed when the link is clicked or the iframe is loaded. -**Prevention:** Implement an `isSafeUrl` verification function to ensure the protocol is strictly `http:` or `https:` (using `new URL()`) before assigning untrusted inputs to DOM attributes like `href` or `src`. - -## 2026-07-08 - 파일 업로드 시 경로 조작(Path Traversal) 취약점 방지 -**Vulnerability:** 클라이언트에서 전송된 `MultipartFile.getOriginalFilename()`을 검증 없이 사용하고 있어 공격자가 `../../../etc/passwd.hwp` 같은 파일명으로 경로를 조작할 수 있었습니다. -**Learning:** 클라이언트가 전송한 파일명은 신뢰할 수 없는 입력값입니다. 경로 탐색 문자열이 포함될 수 있으며, 이를 그대로 사용할 경우 의도치 않은 디렉토리에 파일이 저장되거나 시스템 파일이 조작되는 등의 심각한 문제가 발생할 수 있습니다. -**Prevention:** 사용자로부터 입력받은 파일명은 항상 명시적으로 살균(sanitize)해야 합니다. `org.springframework.util.StringUtils.cleanPath()`를 사용하여 경로를 정규화하고, 마지막 `/` 이후의 순수한 파일명만 추출하여 사용하는 방식을 적용해야 합니다. - -## 2026-07-02 - Cryptographic Signature Verification Bypass in Policy Override -**Vulnerability:** The document validation service logged the presence of policy override parameters (approverId, approvalToken) but failed to actually verify the cryptographic signature of the token against a shared secret. This allowed an attacker to bypass file extension restrictions (e.g., uploading blocked `.hwp` files) by sending any arbitrary token. -**Learning:** Checking for the presence of security tokens is insufficient if the token payload and signature are not cryptographically validated against a trusted secret. The absence of this check created a critical authorization bypass. -**Prevention:** Always verify cryptographic signatures (using constant-time comparison like `MessageDigest.isEqual`) for any policy override or authorization token before granting the elevated privilege or bypassing a security control. - -## 2026-07-08 - Length Extension and Canonicalization Vulnerability in Hash Payloads -**Vulnerability:** The HMAC-SHA256 signature payload for policy overrides was constructed by simply concatenating strings: `approverId + ":" + extension`. This allowed attackers to craft ambiguous inputs if they embedded the delimiter `:` inside their payload, potentially bypassing validation via canonicalization or length extension attacks. -**Learning:** Simple string concatenation is insecure when generating cryptographic hashes or signatures for multiple inputs. Attackers can shift delimiters to produce identical payloads for entirely different logical inputs. -**Prevention:** Always use length-prefixing or unambiguous delimiters (such as JSON structure or specific serialization formats) when combining multiple inputs for cryptographic hashing. For example, use `approverId.length() + ":" + approverId + extension` to strictly define the boundaries of each field. - -## 2026-07-11 - XSS 취약점 제거 (`innerHTML` 사용 교체) -**Vulnerability:** `innerHTML`을 통한 동적 DOM 조작으로 인해 발생할 수 있는 DOM 기반 XSS(Cross-Site Scripting) 취약점이 발견되었습니다. -**Learning:** 로딩 상태를 표시하기 위해 버튼 내부의 텍스트와 DOM 노드를 임시로 변경하고 복구하는 과정에서 `innerHTML`을 읽고 쓰는 방식은 안전하지 않으며 정적 보안 스캐너에서 높은 위험으로 분류됩니다. -**Prevention:** 텍스트나 노드 상태를 업데이트할 때는 반드시 `Array.from(el.childNodes)`로 자식 노드를 저장하고, `el.replaceChildren(...initialChildren)`을 통해 복구하여 안전하게 처리해야 합니다. - -## 2026-07-11 - 파일 이름의 널 바이트 취약점 패치 -**Vulnerability:** 파일 업로드 시 파일 이름에 널 바이트(`\u0000`)를 포함할 경우, `java.nio.file.Path.of` 메서드에서 예외가 발생하여 백엔드 검증 로직이 우회되거나 예상치 못한 서비스 거부(DoS) 상태가 될 수 있습니다. -**Learning:** 파일 경로 또는 확장자 검증에서 널 바이트가 포함된 경우 잘라내기(truncation) 공격을 방지하기 위해 단순히 제거(sanitize)하는 것보다 즉시 예외를 발생시켜 입력값을 명시적으로 거부하는 것이 훨씬 안전합니다. -**Prevention:** 파일 이름 및 경로를 다루는 모든 입력값에 대해 사전에 널 바이트를 검사하고, 발견 시 `IllegalArgumentException`과 같은 예외를 던져 즉각 차단해야 합니다. - -## 2026-07-12 - Prevent DoS Resource Exhaustion in Stream Hashing -**Vulnerability:** The document hashing routine in `DefaultDocumentConversionService` processed file streams without enforcing any maximum size limit on the bytes read. An attacker could exploit this by uploading a maliciously large stream (or exploiting a compression bomb if unzipping), exhausting system memory, CPU, or disk space (DoS). -**Learning:** Checking the declared file size (e.g., `file.getSize()`) in initial validation is not always sufficient if the input stream itself can be spoofed or dynamically expanded during reading. The actual bytes read must be verified against bounds continuously. -**Prevention:** Always enforce a strict, configurable size limit (e.g., `ConversionProperties.maxUploadSizeBytes`) within the `while` loop that reads from untrusted input streams. Track `totalRead` and throw an exception immediately if the limit is exceeded. +## 2026-07-22 - Missing Authentication on Admin Endpoints +**Vulnerability:** The `AdminController` endpoints (`/api/v1/admin/convert/jobs`, `/api/v1/admin/convert/jobs/{jobId}`, `/api/v1/admin/convert/jobs/{jobId}/retry`) lacked authentication and authorization checks, allowing unauthenticated users to read, delete, and retry all conversion jobs. +**Learning:** Spring controllers must explicitly enforce security policies, even if they are placed under an `/admin` path. The existence of an admin path does not automatically protect it. +**Prevention:** Always inject `TenantAccessService` and invoke `tenantAccessService.require(...)` for every controller method to ensure permissions are checked before processing the request. Add corresponding unit tests that explicitly check for these authorization controls. +## 2026-07-22 - Missing Authentication on Admin Endpoints +**Vulnerability:** The `AdminController` endpoints (`/api/v1/admin/convert/jobs`, `/api/v1/admin/convert/jobs/{jobId}`, `/api/v1/admin/convert/jobs/{jobId}/retry`) lacked authentication and authorization checks, allowing unauthenticated users to read, delete, and retry all conversion jobs. +**Learning:** Spring controllers must explicitly enforce security policies, even if they are placed under an `/admin` path. The existence of an admin path does not automatically protect it. +**Prevention:** Always inject `TenantAccessService` and invoke `tenantAccessService.require(...)` for every controller method to ensure permissions are checked before processing the request. Add corresponding unit tests that explicitly check for these authorization controls. diff --git a/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java b/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java index ced5e6a3..7292af3a 100644 --- a/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java +++ b/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java @@ -50,6 +50,16 @@ public final class TenantPermissions { */ public static final String ANALYTICS_READ = "analytics:read"; + /** + * Permission required to read admin data. + */ + public static final String ADMIN_READ = "admin:read"; + + /** + * Permission required to write/modify admin data. + */ + public static final String ADMIN_WRITE = "admin:write"; + private TenantPermissions() { } } diff --git a/src/main/java/com/clearfolio/viewer/controller/AdminController.java b/src/main/java/com/clearfolio/viewer/controller/AdminController.java index 412d4eb8..90bd96ee 100644 --- a/src/main/java/com/clearfolio/viewer/controller/AdminController.java +++ b/src/main/java/com/clearfolio/viewer/controller/AdminController.java @@ -4,6 +4,7 @@ import java.util.List; import java.util.UUID; +import org.springframework.http.HttpHeaders; import org.springframework.http.HttpStatus; import org.springframework.http.ResponseEntity; import org.springframework.web.bind.annotation.DeleteMapping; @@ -11,10 +12,13 @@ import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.annotation.RequestHeader; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.server.ResponseStatusException; import com.clearfolio.viewer.api.AdminJobListResponse; +import com.clearfolio.viewer.auth.TenantAccessService; +import com.clearfolio.viewer.auth.TenantPermissions; import com.clearfolio.viewer.model.ConversionJob; import com.clearfolio.viewer.service.DocumentConversionService; import com.clearfolio.viewer.service.RetryDeadLetterResult; @@ -26,14 +30,17 @@ public class AdminController { private final DocumentConversionService conversionService; + private final TenantAccessService tenantAccessService; /** * Creates a controller for admin operations. * * @param conversionService conversion service + * @param tenantAccessService tenant access service */ - public AdminController(DocumentConversionService conversionService) { + public AdminController(DocumentConversionService conversionService, TenantAccessService tenantAccessService) { this.conversionService = conversionService; + this.tenantAccessService = tenantAccessService; } /** @@ -43,7 +50,8 @@ public AdminController(DocumentConversionService conversionService) { * @return list of conversion jobs */ @GetMapping("/api/v1/admin/convert/jobs") - public AdminJobListResponse getAllJobs(@RequestParam(required = false) Boolean deadLettered) { + public AdminJobListResponse getAllJobs(@RequestParam(required = false) Boolean deadLettered, @RequestHeader HttpHeaders headers) { + tenantAccessService.require(headers, TenantPermissions.ADMIN_READ); Iterable allJobs = conversionService.getAllJobs(); if (deadLettered == null) { @@ -66,7 +74,8 @@ public AdminJobListResponse getAllJobs(@RequestParam(required = false) Boolean d * @return no content on success */ @DeleteMapping("/api/v1/admin/convert/jobs/{jobId}") - public ResponseEntity deleteJob(@PathVariable UUID jobId) { + public ResponseEntity deleteJob(@PathVariable UUID jobId, @RequestHeader HttpHeaders headers) { + tenantAccessService.require(headers, TenantPermissions.ADMIN_WRITE); conversionService.deleteJob(jobId); return ResponseEntity.noContent().build(); } @@ -78,7 +87,8 @@ public ResponseEntity deleteJob(@PathVariable UUID jobId) { * @return accepted response on success */ @PostMapping("/api/v1/admin/convert/jobs/{jobId}/retry") - public ResponseEntity retryDeadLettered(@PathVariable UUID jobId) { + public ResponseEntity retryDeadLettered(@PathVariable UUID jobId, @RequestHeader HttpHeaders headers) { + tenantAccessService.require(headers, TenantPermissions.ADMIN_WRITE); RetryDeadLetterResult result = conversionService.retryDeadLettered(jobId, "admin"); if (result == RetryDeadLetterResult.NOT_FOUND) { throw new ResponseStatusException(HttpStatus.NOT_FOUND, "job not found"); diff --git a/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java b/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java index ad63a801..49ee3557 100644 --- a/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java +++ b/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java @@ -2,14 +2,19 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.mockito.ArgumentMatchers.any; import java.util.Arrays; +import java.util.Set; import java.util.UUID; +import org.springframework.http.HttpHeaders; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.test.web.reactive.server.WebTestClient; +import com.clearfolio.viewer.auth.TenantAccessService; +import com.clearfolio.viewer.auth.TenantContext; import com.clearfolio.viewer.model.ConversionJob; import com.clearfolio.viewer.service.DocumentConversionService; import com.clearfolio.viewer.service.RetryDeadLetterResult; @@ -17,16 +22,19 @@ class AdminControllerTest { private DocumentConversionService conversionService; + private TenantAccessService tenantAccessService; private WebTestClient webTestClient; private AdminController controller; @BeforeEach void setUp() { conversionService = mock(DocumentConversionService.class); - controller = new AdminController(conversionService); + tenantAccessService = mock(TenantAccessService.class); + controller = new AdminController(conversionService, tenantAccessService); webTestClient = WebTestClient.bindToController(controller) .controllerAdvice(new ApiExceptionHandler()) .build(); + when(tenantAccessService.require(any(), any())).thenReturn(new TenantContext("t1", "s1", Set.of("admin:read", "admin:write"))); } @Test @@ -37,6 +45,7 @@ void getAllJobsReturnsAllJobsWhenNoFilterProvided() { webTestClient.get() .uri("/api/v1/admin/convert/jobs") + .header(HttpHeaders.AUTHORIZATION, "Bearer test-token") .exchange() .expectStatus().isOk() .expectBody() @@ -55,6 +64,7 @@ void getAllJobsFiltersByDeadLetteredTrue() { webTestClient.get() .uri("/api/v1/admin/convert/jobs?deadLettered=true") + .header(HttpHeaders.AUTHORIZATION, "Bearer test-token") .exchange() .expectStatus().isOk() .expectBody() @@ -72,6 +82,7 @@ void getAllJobsFiltersByDeadLetteredFalse() { webTestClient.get() .uri("/api/v1/admin/convert/jobs?deadLettered=false") + .header(HttpHeaders.AUTHORIZATION, "Bearer test-token") .exchange() .expectStatus().isOk() .expectBody() @@ -85,6 +96,7 @@ void deleteJobReturnsNoContent() { webTestClient.delete() .uri("/api/v1/admin/convert/jobs/" + jobId) + .header(HttpHeaders.AUTHORIZATION, "Bearer test-token") .exchange() .expectStatus().isNoContent(); } @@ -96,6 +108,7 @@ void retryDeadLetteredReturnsAcceptedWhenAccepted() { webTestClient.post() .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") + .header(HttpHeaders.AUTHORIZATION, "Bearer test-token") .exchange() .expectStatus().isAccepted(); } @@ -107,6 +120,7 @@ void retryDeadLetteredReturnsNotFoundWhenNotFound() { webTestClient.post() .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") + .header(HttpHeaders.AUTHORIZATION, "Bearer test-token") .exchange() .expectStatus().isNotFound(); } @@ -118,6 +132,7 @@ void retryDeadLetteredReturnsConflictWhenNotEligible() { webTestClient.post() .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") + .header(HttpHeaders.AUTHORIZATION, "Bearer test-token") .exchange() .expectStatus().isEqualTo(409); // isConflict() isn't always available depending on spring-test version, so using isEqualTo(409) is safer }