From 2a0e88dd1622e96485143372dd56690eaddb1e79 Mon Sep 17 00:00:00 2001 From: seonghobae <8172694+seonghobae@users.noreply.github.com> Date: Sun, 4 Oct 2026 21:11:59 +0000 Subject: [PATCH 1/3] =?UTF-8?q?=EB=B3=B4=EC=95=88=20=EC=88=98=EC=A0=95:=20?= =?UTF-8?q?AdminController=20=EA=B6=8C=ED=95=9C=20=ED=99=95=EC=9D=B8=20?= =?UTF-8?q?=EB=A1=9C=EC=A7=81=20=EC=B6=94=EA=B0=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AdminController의 모든 관리자 API에 대해 TenantAccessService를 사용하여 명시적인 권한(`admin:operate`) 확인 로직을 추가했습니다. 이는 애플리케이션의 취약점 방지를 위해 필요하며 TenantPermissions 클래스에 새 권한 상수를 추가했습니다. 관련 테스트와 저널 엔트리를 업데이트하고 100% 테스트 커버리지를 보장합니다. --- .jules/sentinel.md | 4 ++ .../viewer/auth/TenantPermissions.java | 5 ++ .../viewer/controller/AdminController.java | 53 +++++++++++++++---- .../controller/AdminControllerTest.java | 13 ++++- 4 files changed, 65 insertions(+), 10 deletions(-) diff --git a/.jules/sentinel.md b/.jules/sentinel.md index e795cb9dd..d56bd0c3d 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -32,3 +32,7 @@ **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-10-04 - Missing authorization checks on AdminController +**Vulnerability:** The AdminController lacked authorization checks, allowing any user to access administrative endpoints to read, delete, and retry all jobs regardless of tenant or permissions. +**Learning:** Spring `@RestController` endpoints must explicitly verify the required permission and tenant via `TenantAccessService` when standard gateway validation is not implicitly handling these paths. Without an explicit check, Spring Security or the application routes will not secure custom admin paths by default unless globally matched. +**Prevention:** Always inject and invoke `TenantAccessService.require()` with a dedicated administrative permission (e.g., `admin:operate`) on any endpoints handling cross-tenant, privileged, or system-level administrative operations. diff --git a/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java b/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java index 4f9c6a685..717e59a7d 100644 --- a/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java +++ b/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java @@ -55,6 +55,11 @@ public final class TenantPermissions { */ public static final String ANALYTICS_READ = "analytics:read"; + /** + * Permission required to perform administrative operations. + */ + public static final String ADMIN_OPERATE = "admin:operate"; + 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 412d4eb86..008f0b5cb 100644 --- a/src/main/java/com/clearfolio/viewer/controller/AdminController.java +++ b/src/main/java/com/clearfolio/viewer/controller/AdminController.java @@ -14,7 +14,12 @@ import org.springframework.web.bind.annotation.RestController; import org.springframework.web.server.ResponseStatusException; +import org.springframework.http.HttpHeaders; +import org.springframework.web.bind.annotation.RequestHeader; + 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; @@ -25,25 +30,41 @@ @RestController public class AdminController { + /** + * The conversion service. + */ private final DocumentConversionService conversionService; + /** + * The tenant access service to enforce permissions. + */ + private final TenantAccessService tenantAccessService; + /** * Creates a controller for admin operations. * - * @param conversionService conversion service + * @param conversionServiceInjected conversion service + * @param tenantAccessServiceInjected tenant access service */ - public AdminController(DocumentConversionService conversionService) { - this.conversionService = conversionService; + public AdminController( + final DocumentConversionService conversionServiceInjected, + final TenantAccessService tenantAccessServiceInjected) { + this.conversionService = conversionServiceInjected; + this.tenantAccessService = tenantAccessServiceInjected; } /** * Retrieves all conversion jobs, optionally filtered by dead-letter status. * * @param deadLettered optional filter for dead-lettered jobs + * @param headers request headers * @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) final Boolean deadLettered, + @RequestHeader final HttpHeaders headers) { + tenantAccessService.require(headers, TenantPermissions.ADMIN_OPERATE); Iterable allJobs = conversionService.getAllJobs(); if (deadLettered == null) { @@ -63,10 +84,14 @@ public AdminJobListResponse getAllJobs(@RequestParam(required = false) Boolean d * Deletes a conversion job. * * @param jobId conversion job identifier + * @param headers request headers * @return no content on success */ @DeleteMapping("/api/v1/admin/convert/jobs/{jobId}") - public ResponseEntity deleteJob(@PathVariable UUID jobId) { + public ResponseEntity deleteJob( + @PathVariable final UUID jobId, + @RequestHeader final HttpHeaders headers) { + tenantAccessService.require(headers, TenantPermissions.ADMIN_OPERATE); conversionService.deleteJob(jobId); return ResponseEntity.noContent().build(); } @@ -75,16 +100,26 @@ public ResponseEntity deleteJob(@PathVariable UUID jobId) { * Retries a dead-lettered conversion job. * * @param jobId conversion job identifier + * @param headers request headers * @return accepted response on success */ @PostMapping("/api/v1/admin/convert/jobs/{jobId}/retry") - public ResponseEntity retryDeadLettered(@PathVariable UUID jobId) { - RetryDeadLetterResult result = conversionService.retryDeadLettered(jobId, "admin"); + public ResponseEntity retryDeadLettered( + @PathVariable final UUID jobId, + @RequestHeader final HttpHeaders headers) { + tenantAccessService.require(headers, TenantPermissions.ADMIN_OPERATE); + RetryDeadLetterResult result = conversionService.retryDeadLettered( + jobId, + "admin"); if (result == RetryDeadLetterResult.NOT_FOUND) { - throw new ResponseStatusException(HttpStatus.NOT_FOUND, "job not found"); + throw new ResponseStatusException( + HttpStatus.NOT_FOUND, + "job not found"); } if (result == RetryDeadLetterResult.NOT_ELIGIBLE) { - throw new ResponseStatusException(HttpStatus.CONFLICT, "job is not eligible for retry"); + throw new ResponseStatusException( + HttpStatus.CONFLICT, + "job is not eligible for retry"); } return ResponseEntity.accepted().build(); } diff --git a/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java b/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java index ad63a8015..8ea71f4aa 100644 --- a/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java +++ b/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java @@ -1,5 +1,7 @@ package com.clearfolio.viewer.controller; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; @@ -10,6 +12,9 @@ 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.auth.TenantPermissions; import com.clearfolio.viewer.model.ConversionJob; import com.clearfolio.viewer.service.DocumentConversionService; import com.clearfolio.viewer.service.RetryDeadLetterResult; @@ -17,16 +22,22 @@ class AdminControllerTest { private DocumentConversionService conversionService; + private TenantAccessService tenantAccessService; private WebTestClient webTestClient; private AdminController controller; + private TenantContext mockTenantContext; @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(); + + mockTenantContext = mock(TenantContext.class); + when(tenantAccessService.require(any(), eq(TenantPermissions.ADMIN_OPERATE))).thenReturn(mockTenantContext); } @Test From 145e5f0c9c4e5e797e9b7d6fd587e2a967fa99f0 Mon Sep 17 00:00:00 2001 From: seonghobae <8172694+seonghobae@users.noreply.github.com> Date: Sun, 4 Oct 2026 21:21:34 +0000 Subject: [PATCH 2/3] =?UTF-8?q?=EB=B3=B4=EC=95=88=20=EC=88=98=EC=A0=95:=20?= =?UTF-8?q?AdminController=20=EA=B6=8C=ED=95=9C=20=ED=99=95=EC=9D=B8=20?= =?UTF-8?q?=EB=A1=9C=EC=A7=81=20=EC=B6=94=EA=B0=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AdminController의 모든 관리자 API에 대해 TenantAccessService를 사용하여 명시적인 권한(`admin:operate`) 확인 로직을 추가했습니다. 이는 애플리케이션의 취약점 방지를 위해 필요하며 TenantPermissions 클래스에 새 권한 상수를 추가했습니다. 관련 테스트와 저널 엔트리를 업데이트하고 100% 테스트 커버리지를 보장합니다. From 5605a850f20a4b58f6ec8e6c53039de5f4d116cd Mon Sep 17 00:00:00 2001 From: seonghobae <8172694+seonghobae@users.noreply.github.com> Date: Sun, 4 Oct 2026 21:24:14 +0000 Subject: [PATCH 3/3] =?UTF-8?q?=EB=B3=B4=EC=95=88=20=EC=88=98=EC=A0=95:=20?= =?UTF-8?q?AdminController=20=EA=B6=8C=ED=95=9C=20=ED=99=95=EC=9D=B8=20?= =?UTF-8?q?=EB=A1=9C=EC=A7=81=20=EC=B6=94=EA=B0=80?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit AdminController의 모든 관리자 API에 대해 TenantAccessService를 사용하여 명시적인 권한(`admin:operate`) 확인 로직을 추가했습니다. 이는 애플리케이션의 취약점 방지를 위해 필요하며 TenantPermissions 클래스에 새 권한 상수를 추가했습니다. 관련 테스트와 저널 엔트리를 업데이트하고 100% 테스트 커버리지를 보장합니다.