🛡️ Sentinel: [CRITICAL] AdminController의 인증 및 권한 부여 우회 취약점 해결

.jules/sentinel.md

@@ -32,3 +32,8 @@
 **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 Authentication in Admin Controller
+**Vulnerability:** The AdminController endpoints for listing, deleting, and retrying conversion jobs were exposed without any authentication or authorization checks.
+**Learning:** Endpoints intended for administrative use are just as vulnerable to unauthorized access if they do not explicitly enforce tenant context and permission claims like other protected APIs. Relying on path naming conventions alone does not secure endpoints.
+**Prevention:** Always inject and utilize `TenantAccessService` (or equivalent authorization enforcement mechanism) on every protected endpoint, explicitly checking for required permissions (e.g., `TenantPermissions.AUDIT_READ`, `JOB_DELETE`) before processing any business logic.

src/main/java/com/clearfolio/viewer/controller/AdminController.java

@@ -18,32 +18,48 @@
 import com.clearfolio.viewer.model.ConversionJob;
 import com.clearfolio.viewer.service.DocumentConversionService;
 import com.clearfolio.viewer.service.RetryDeadLetterResult;
+import com.clearfolio.viewer.auth.TenantAccessService;
+import com.clearfolio.viewer.auth.TenantPermissions;
+import org.springframework.http.HttpHeaders;
+import org.springframework.web.bind.annotation.RequestHeader;
 
 /**
  * Controller for admin-specific endpoints.
  */
 @RestController
 public class AdminController {
 
+    /** Conversion service. */
     private final DocumentConversionService conversionService;
 
+    /** Tenant access service. */
+    private final TenantAccessService tenantAccessService;
+
     /**
      * Creates a controller for admin operations.
      *
-     * @param conversionService conversion service
+     * @param pConversionService conversion service
+     * @param pTenantAccessService tenant access service
      */
-    public AdminController(DocumentConversionService conversionService) {
-        this.conversionService = conversionService;
+    public AdminController(
+            final DocumentConversionService pConversionService,
+            final TenantAccessService pTenantAccessService) {
+        this.conversionService = pConversionService;
+        this.tenantAccessService = pTenantAccessService;
     }
 
     /**
      * Retrieves all conversion jobs, optionally filtered by dead-letter status.
      *
      * @param deadLettered optional filter for dead-lettered jobs
+     * @param headers request headers for auth
      * @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.AUDIT_READ);
         Iterable<ConversionJob> allJobs = conversionService.getAllJobs();
 
         if (deadLettered == null) {
@@ -63,10 +79,14 @@ public AdminJobListResponse getAllJobs(@RequestParam(required = false) Boolean d
      * Deletes a conversion job.
      *
      * @param jobId conversion job identifier
+     * @param headers request headers for auth
      * @return no content on success
      */
     @DeleteMapping("/api/v1/admin/convert/jobs/{jobId}")
-    public ResponseEntity<Void> deleteJob(@PathVariable UUID jobId) {
+    public ResponseEntity<Void> deleteJob(
+            @PathVariable final UUID jobId,
+            @RequestHeader final HttpHeaders headers) {
+        tenantAccessService.require(headers, TenantPermissions.JOB_DELETE);
         conversionService.deleteJob(jobId);
         return ResponseEntity.noContent().build();
     }
@@ -75,16 +95,23 @@ public ResponseEntity<Void> deleteJob(@PathVariable UUID jobId) {
      * Retries a dead-lettered conversion job.
      *
      * @param jobId conversion job identifier
+     * @param headers request headers for auth
      * @return accepted response on success
      */
     @PostMapping("/api/v1/admin/convert/jobs/{jobId}/retry")
-    public ResponseEntity<Void> retryDeadLettered(@PathVariable UUID jobId) {
-        RetryDeadLetterResult result = conversionService.retryDeadLettered(jobId, "admin");
+    public ResponseEntity<Void> retryDeadLettered(
+            @PathVariable final UUID jobId,
+            @RequestHeader final HttpHeaders headers) {
+        tenantAccessService.require(headers, TenantPermissions.JOB_RETRY);
+        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();
     }

src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java

@@ -13,17 +13,25 @@
 import com.clearfolio.viewer.model.ConversionJob;
 import com.clearfolio.viewer.service.DocumentConversionService;
 import com.clearfolio.viewer.service.RetryDeadLetterResult;
+import com.clearfolio.viewer.auth.TenantAccessService;
+import com.clearfolio.viewer.auth.TenantPermissions;
+import static org.mockito.ArgumentMatchers.any;
+import static org.mockito.ArgumentMatchers.eq;
 
 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);
+                com.clearfolio.viewer.auth.TenantContext mockContext = mock(com.clearfolio.viewer.auth.TenantContext.class);
+        when(tenantAccessService.require(any(), any())).thenReturn(mockContext);
         webTestClient = WebTestClient.bindToController(controller)
                 .controllerAdvice(new ApiExceptionHandler())
                 .build();
@@ -37,6 +45,7 @@ void getAllJobsReturnsAllJobsWhenNoFilterProvided() {
 
         webTestClient.get()
                 .uri("/api/v1/admin/convert/jobs")
+                .header("X-Dummy", "dummy")
                 .exchange()
                 .expectStatus().isOk()
                 .expectBody()
@@ -55,6 +64,7 @@ void getAllJobsFiltersByDeadLetteredTrue() {
 
         webTestClient.get()
                 .uri("/api/v1/admin/convert/jobs?deadLettered=true")
+                .header("X-Dummy", "dummy")
                 .exchange()
                 .expectStatus().isOk()
                 .expectBody()
@@ -72,6 +82,7 @@ void getAllJobsFiltersByDeadLetteredFalse() {
 
         webTestClient.get()
                 .uri("/api/v1/admin/convert/jobs?deadLettered=false")
+                .header("X-Dummy", "dummy")
                 .exchange()
                 .expectStatus().isOk()
                 .expectBody()
@@ -85,6 +96,7 @@ void deleteJobReturnsNoContent() {
 
         webTestClient.delete()
                 .uri("/api/v1/admin/convert/jobs/" + jobId)
+                .header("X-Dummy", "dummy")
                 .exchange()
                 .expectStatus().isNoContent();
     }
@@ -96,6 +108,7 @@ void retryDeadLetteredReturnsAcceptedWhenAccepted() {
 
         webTestClient.post()
                 .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry")
+                .header("X-Dummy", "dummy")
                 .exchange()
                 .expectStatus().isAccepted();
     }
@@ -107,6 +120,7 @@ void retryDeadLetteredReturnsNotFoundWhenNotFound() {
 
         webTestClient.post()
                 .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry")
+                .header("X-Dummy", "dummy")
                 .exchange()
                 .expectStatus().isNotFound();
     }
@@ -118,6 +132,7 @@ void retryDeadLetteredReturnsConflictWhenNotEligible() {
 
         webTestClient.post()
                 .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry")
+                .header("X-Dummy", "dummy")
                 .exchange()
                 .expectStatus().isEqualTo(409); // isConflict() isn't always available depending on spring-test version, so using isEqualTo(409) is safer
     }