diff --git a/.jules/sentinel.md b/.jules/sentinel.md index e795cb9dd..9f18f4382 100644 --- a/.jules/sentinel.md +++ b/.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-15 - Missing Authorization on Admin Endpoints +**Vulnerability:** The admin endpoints (`/api/v1/admin/convert/jobs/*`) lacked permission checks, allowing any tenant to view or manage conversion jobs belonging to other tenants. +**Learning:** Endpoints intended for cross-tenant administration must strictly enforce dedicated administrative permissions (e.g., `admin:operate`). Missing these checks results in a critical authorization bypass. +**Prevention:** Always secure administrative API endpoints with `TenantAccessService` and verify cross-tenant administrative privileges using specific, non-default permissions rather than relying solely on path obscurity. diff --git a/CHANGELOG.md b/CHANGELOG.md index 1187deb2a..9733b2c15 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -24,6 +24,7 @@ ### Security +- 관리자용 엔드포인트(`GET /api/v1/admin/convert/jobs`, `DELETE /api/v1/admin/convert/jobs/{jobId}`, `POST /api/v1/admin/convert/jobs/{jobId}/retry`)에 교차 테넌트 접근 제어(`admin:operate` 권한 요구)를 추가하여 무단 접근 및 권한 상승 취약점을 해결했습니다. - `GET /api/v1/convert/jobs/{jobId}/download`가 리소스 조회 전에 전용 `artifact:read` 권한을 검증하고, PDF 저장소 접근 전에 작업의 tenant 소유권을 확인하도록 강화했습니다. `job:read`만으로는 문서 바이트를 읽을 수 없으며, 인증 누락·권한 누락·교차 tenant UUID 접근은 각각 fail closed 처리되고 교차 tenant 요청은 리소스 존재를 숨기는 `404`를 반환합니다. - Maven XML 테스트 보고서 검증기는 각 `testsuite`의 `tests`, `skipped`, `failures`, `errors` 속성을 모두 필수 증거로 요구합니다. 누락된 결과 수를 암묵적으로 0으로 간주하지 않고 fail closed 처리하며, 각 속성 누락 회귀 테스트를 추가했습니다. - Maven XML 테스트 보고서 검증기는 UTF-8만 허용하고 UTF-8 BOM은 수용하며, NUL 바이트·DTD·엔터티 선언을 파싱 전에 거부합니다. UTF-16 같은 대체 인코딩으로 위험 선언을 바이트 검사에서 숨기는 우회와 외부 엔터티 읽기·엔터티 확장형 서비스 거부를 회귀 테스트로 차단했습니다. diff --git a/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java b/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java index 4f9c6a685..a2273d9d2 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 for cross-tenant administration. + */ + 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..8e74d46cc 100644 --- a/src/main/java/com/clearfolio/viewer/controller/AdminController.java +++ b/src/main/java/com/clearfolio/viewer/controller/AdminController.java @@ -12,8 +12,13 @@ import org.springframework.web.bind.annotation.PostMapping; import org.springframework.web.bind.annotation.RequestParam; import org.springframework.web.bind.annotation.RestController; +import org.springframework.web.bind.annotation.RequestHeader; +import org.springframework.http.HttpHeaders; import org.springframework.web.server.ResponseStatusException; +import com.clearfolio.viewer.auth.TenantAccessService; +import com.clearfolio.viewer.auth.TenantPermissions; + import com.clearfolio.viewer.api.AdminJobListResponse; import com.clearfolio.viewer.model.ConversionJob; import com.clearfolio.viewer.service.DocumentConversionService; @@ -27,23 +32,33 @@ public class AdminController { private final DocumentConversionService conversionService; + private final TenantAccessService tenantAccessService; + /** * Creates a controller for admin operations. * * @param conversionService conversion service + * @param tenantAccessService tenant and permission guard */ - public AdminController(DocumentConversionService conversionService) { + public AdminController( + final DocumentConversionService conversionService, + final TenantAccessService tenantAccessService) { this.conversionService = conversionService; + this.tenantAccessService = tenantAccessService; } /** * 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 +78,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,10 +94,14 @@ 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) { + 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"); diff --git a/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java b/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java index ad63a8015..a82b153e2 100644 --- a/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java +++ b/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java @@ -10,6 +10,12 @@ 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 org.springframework.http.HttpHeaders; +import java.util.Set; + import com.clearfolio.viewer.model.ConversionJob; import com.clearfolio.viewer.service.DocumentConversionService; import com.clearfolio.viewer.service.RetryDeadLetterResult; @@ -20,10 +26,16 @@ class AdminControllerTest { private WebTestClient webTestClient; private AdminController controller; + private static void addAuth(HttpHeaders headers, String permissions) { + headers.add(TenantContext.TENANT_ID_HEADER, "admin-tenant"); + headers.add(TenantContext.SUBJECT_ID_HEADER, "admin-subject"); + headers.add(TenantContext.PERMISSIONS_HEADER, permissions); + } + @BeforeEach void setUp() { conversionService = mock(DocumentConversionService.class); - controller = new AdminController(conversionService); + controller = new AdminController(conversionService, new TenantAccessService()); webTestClient = WebTestClient.bindToController(controller) .controllerAdvice(new ApiExceptionHandler()) .build(); @@ -37,6 +49,7 @@ void getAllJobsReturnsAllJobsWhenNoFilterProvided() { webTestClient.get() .uri("/api/v1/admin/convert/jobs") + .headers(headers -> addAuth(headers, TenantPermissions.ADMIN_OPERATE)) .exchange() .expectStatus().isOk() .expectBody() @@ -55,6 +68,7 @@ void getAllJobsFiltersByDeadLetteredTrue() { webTestClient.get() .uri("/api/v1/admin/convert/jobs?deadLettered=true") + .headers(headers -> addAuth(headers, TenantPermissions.ADMIN_OPERATE)) .exchange() .expectStatus().isOk() .expectBody() @@ -72,6 +86,7 @@ void getAllJobsFiltersByDeadLetteredFalse() { webTestClient.get() .uri("/api/v1/admin/convert/jobs?deadLettered=false") + .headers(headers -> addAuth(headers, TenantPermissions.ADMIN_OPERATE)) .exchange() .expectStatus().isOk() .expectBody() @@ -85,6 +100,7 @@ void deleteJobReturnsNoContent() { webTestClient.delete() .uri("/api/v1/admin/convert/jobs/" + jobId) + .headers(headers -> addAuth(headers, TenantPermissions.ADMIN_OPERATE)) .exchange() .expectStatus().isNoContent(); } @@ -96,6 +112,7 @@ void retryDeadLetteredReturnsAcceptedWhenAccepted() { webTestClient.post() .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") + .headers(headers -> addAuth(headers, TenantPermissions.ADMIN_OPERATE)) .exchange() .expectStatus().isAccepted(); } @@ -107,6 +124,7 @@ void retryDeadLetteredReturnsNotFoundWhenNotFound() { webTestClient.post() .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") + .headers(headers -> addAuth(headers, TenantPermissions.ADMIN_OPERATE)) .exchange() .expectStatus().isNotFound(); } @@ -118,6 +136,7 @@ void retryDeadLetteredReturnsConflictWhenNotEligible() { webTestClient.post() .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") + .headers(headers -> addAuth(headers, TenantPermissions.ADMIN_OPERATE)) .exchange() .expectStatus().isEqualTo(409); // isConflict() isn't always available depending on spring-test version, so using isEqualTo(409) is safer }