diff --git a/.jules/sentinel.md b/.jules/sentinel.md index e795cb9dd..4a758552c 100644 --- a/.jules/sentinel.md +++ b/.jules/sentinel.md @@ -1,4 +1,4 @@ -## 2026-06-30 - Prevent DOM-based XSS in Viewer JS +# 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`. @@ -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-01 - Add Authentication to Admin Endpoints +**Vulnerability:** Admin endpoints (`AdminController.java`) are completely open without any authentication or authorization checks. +**Learning:** Controller classes meant for internal administration were not integrated with the `TenantAccessService` used across other protected controllers (like `ConversionController`), leading to insecure direct access to jobs. +**Prevention:** Always verify that every controller endpoint is protected by appropriate authorization checks, like `TenantAccessService.require(headers, permission)`. diff --git a/CHANGELOG.md b/CHANGELOG.md index 1187deb2a..25d532536 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,8 @@ ### Added +- **관리자 엔드포인트 보안 강화**: 관리자 전용 API(`AdminController`)에 `TenantAccessService`를 통한 `ADMIN_OPERATE` 권한 인증 및 인가 검증을 추가하여 보안 취약점을 해결했습니다. + - **UI UX 개선**: 'Details' 버튼 클릭 시, 작업 상세 정보 로드 중에 사용자가 명시적인 로딩 상태를 확인할 수 있도록 'Loading...' 텍스트와 비활성화 상태를 표시하도록 추가했습니다. - **관리자용 단건 작업 삭제 및 재시도 API 추가** - 특정 변환 작업을 삭제할 수 있는 `DELETE /api/v1/admin/convert/jobs/{jobId}` 엔드포인트를 추가했습니다. diff --git a/docs/product-technical-gap-baseline.md b/docs/product-technical-gap-baseline.md new file mode 100644 index 000000000..6379e81f0 --- /dev/null +++ b/docs/product-technical-gap-baseline.md @@ -0,0 +1,52 @@ +# Product and Technical Gap Baseline + +Status: **Proposed** + +Evidence cutoff: 2026-10-01 UTC + +Evidence source head: `b20bd4ead4600208960b534b08173ee946a84c7d` on +[clearfolio#659](https://github.com/ContextualWisdomLab/clearfolio/pull/659). + +## Goal and bounded context + +Clearfolio owns the document-viewer bounded context: authenticated document conversion status, +artifact preview, and administrator recovery operations. Product-domain truth remains in Clearfolio. +Identity claims enter through the tenant-access anti-corruption layer; document extraction and +organization control-plane responsibilities remain external contracts. + +## Authoritative artifacts + +| Concern | Current evidence | Status | +| --- | --- | --- | +| PRD | `docs/prd-integrated-document-viewer-platform.md` | Current | +| TRD | `docs/trd-integrated-document-viewer-platform.md` | Current | +| Architecture and Context Map | `ARCHITECTURE.md`, `docs/architecture.md` | Current | +| UML and interaction flows | `docs/diagrams/README.md` and bounded flow diagrams | Current | +| Authentication model | `docs/security/2026-07-02-auth-tenant-model.md` | Current | +| ERD | No canonical ERD is published in this repository | Gap | +| Change history | `CHANGELOG.md` | Current | + +## Context Map + +| Relationship | Contract boundary | Direction | +| --- | --- | --- | +| Identity provider to Clearfolio | Tenant claims and explicit permissions | Upstream to ACL | +| Conversion storage to Clearfolio | Repository interfaces and artifact identifiers | Upstream to ACL | +| Clearfolio to browser | Versioned HTTP responses and signed artifact links | Product API | +| Organization CI to Clearfolio | Reusable security and review workflows | Conformance only | + +## Gap and action register + +| Gap | Exact evidence | Action | Status | +| --- | --- | --- | --- | +| Administrator endpoints lacked an explicit operation permission | PR #659 source and tests | Require `ADMIN_OPERATE` through `TenantAccessService` | Implemented; exact-head acceptance pending | +| Jackson 2.22.1 is affected by five September 2026 advisories | Security run `36792153106`, job `110147365860` | Pin Jackson BOM and databind to 2.22.3 and guard the POM version | Contract restored at `b20bd4ea…`; exact-head Security Scan pending | +| Canonical ERD is absent | Repository documentation inventory at the evidence head | Publish the persisted conversion-job and tenant ownership model without inventing storage not present in code | Proposed | +| Current successor invalidated predecessor-head CodeQL evidence | PR head advanced after restoring deleted contracts | Require fresh exact-head CodeQL plus independent review | Pending | + +## Acceptance rule + +A row becomes complete only when its implementation, regression contract, documentation, and +exact-head hosted checks are green. Draft-gated, queued, skipped, stale-head, or predecessor-head +results are not acceptance evidence. This baseline must be updated whenever the PRD, TRD, +Context Map, persistence model, or a listed Gap changes. diff --git a/docs/security/2026-07-02-auth-tenant-model.md b/docs/security/2026-07-02-auth-tenant-model.md index d6babd804..a561c6364 100644 --- a/docs/security/2026-07-02-auth-tenant-model.md +++ b/docs/security/2026-07-02-auth-tenant-model.md @@ -103,7 +103,7 @@ to the identity provider or gateway, not the viewer service. | `viewer_user` | `job:create`, `job:read`, `viewer:read`, `artifact-link:create`, `artifact:read` | | `workflow_client` | `job:create`, `job:read`, `viewer:read` | | `operator` | `job:read`, `job:retry`, `artifact-link:revoke`, `audit:read` | -| `tenant_admin` | `job:read`, `artifact-link:revoke`, `audit:read`, `tenant:configure` | +| `tenant_admin` | `job:read`, `artifact-link:revoke`, `audit:read`, `tenant:configure`, `admin:operate` | | `buyer_reviewer` | `job:read`, `viewer:read`, `analytics:read`, `audit:read` in a demo or diligence tenant | Server-side authorization must check both permission and tenant ownership. A @@ -147,6 +147,9 @@ unauthorized action, depending on route semantics. | `GET /viewer/{docId}` | none for HTML shell | Shell does not inspect job existence; protected JSON APIs decide state. | | `POST /api/v1/viewer/{docId}/artifact-links` | `artifact-link:create` | Same tenant and succeeded job. | | `GET /artifacts/{docId}.pdf` | valid signed artifact token | Signed token scope/document/tenant/current checksum/issuance/revocation must match; zero or one Range; record read audit. | +| `GET /api/v1/admin/convert/jobs` | `admin:operate` | Gateway role mapping must grant this permission to `tenant_admin`; the service validates the signed permission claim before listing global jobs. | +| `DELETE /api/v1/admin/convert/jobs/{jobId}` | `admin:operate` | Same grant contract; deny before deletion when absent. | +| `POST /api/v1/admin/convert/jobs/{jobId}/retry` | `admin:operate` | Same grant contract; deny before retry when absent. | | `GET /api/v1/analytics/kpi-snapshot` | `analytics:read` | Tenant-scoped aggregate by default. | ## Current Branch Implementation Status diff --git a/pom.xml b/pom.xml index af3ec7ad2..0675feb64 100644 --- a/pom.xml +++ b/pom.xml @@ -38,12 +38,10 @@ which contains the July 2026 HTTP, HTTP/2, MQTT, compression, and parser-boundary hardening release. --> 4.1.136.Final - - 2.22.1 + + 2.22.3 1.5.35 @@ -59,7 +57,7 @@ --> - + com.fasterxml.jackson jackson-bom diff --git a/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java b/src/main/java/com/clearfolio/viewer/auth/TenantPermissions.java index 4f9c6a685..578c6ca96 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 admin 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..488d93b1e 100644 --- a/src/main/java/com/clearfolio/viewer/controller/AdminController.java +++ b/src/main/java/com/clearfolio/viewer/controller/AdminController.java @@ -10,11 +10,15 @@ import org.springframework.web.bind.annotation.GetMapping; import org.springframework.web.bind.annotation.PathVariable; import org.springframework.web.bind.annotation.PostMapping; +import org.springframework.http.HttpHeaders; +import org.springframework.web.bind.annotation.RequestHeader; import org.springframework.web.bind.annotation.RequestParam; 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; @@ -27,23 +31,34 @@ public class AdminController { private final DocumentConversionService conversionService; + /** + * Validates required authentication headers and properties. + */ + 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(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/auth/TenantPermissionContractTest.java b/src/test/java/com/clearfolio/viewer/auth/TenantPermissionContractTest.java new file mode 100644 index 000000000..6bb623056 --- /dev/null +++ b/src/test/java/com/clearfolio/viewer/auth/TenantPermissionContractTest.java @@ -0,0 +1,26 @@ +package com.clearfolio.viewer.auth; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +import java.io.IOException; +import java.nio.file.Files; +import java.nio.file.Path; + +import org.junit.jupiter.api.Test; + +class TenantPermissionContractTest { + + @Test + void tenantAdminRoleCarriesAdminOperatePermission() throws IOException { + String contract = Files.readString( + Path.of("docs/security/2026-07-02-auth-tenant-model.md")); + String tenantAdminRow = contract.lines() + .filter(line -> line.startsWith("| `tenant_admin` |")) + .findFirst() + .orElseThrow(); + + assertTrue( + tenantAdminRow.contains("`" + TenantPermissions.ADMIN_OPERATE + "`"), + "tenant_admin must receive the permission enforced by AdminController"); + } +} diff --git a/src/test/java/com/clearfolio/viewer/config/DependencyPolicyTest.java b/src/test/java/com/clearfolio/viewer/config/DependencyPolicyTest.java index 2c3f0f2b5..f4e60ab4e 100644 --- a/src/test/java/com/clearfolio/viewer/config/DependencyPolicyTest.java +++ b/src/test/java/com/clearfolio/viewer/config/DependencyPolicyTest.java @@ -53,6 +53,19 @@ void pomPinsPatchedNettyLineForReactiveHttpServing() throws Exception { ); } + @Test + void pomPinsJacksonLinePastSeptember2026DatabindAdvisories() throws Exception { + Document document = parsedPom(); + Element properties = (Element) document.getElementsByTagName("properties").item(0); + + assertEquals( + "2.22.3", + directChildTextOf(properties, "jackson-bom.version"), + "Jackson 2.22.3 is the first 2.22.x release that fixes CVE-2026-68497, " + + "CVE-2026-91776, CVE-2026-91777, CVE-2026-19032, and CVE-2026-83557" + ); + } + @Test void mavenVerifyGeneratesWarningFreePublicApiJavadocs() throws Exception { Document document = parsedPom(); diff --git a/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java b/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java index ad63a8015..8976d6a4d 100644 --- a/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java +++ b/src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java @@ -2,28 +2,39 @@ import static org.mockito.Mockito.mock; import static org.mockito.Mockito.when; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.ArgumentMatchers.eq; import java.util.Arrays; import java.util.UUID; +import java.util.Set; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.test.web.reactive.server.WebTestClient; +import org.springframework.http.HttpHeaders; +import org.springframework.web.server.ResponseStatusException; +import org.springframework.http.HttpStatus; 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 com.clearfolio.viewer.auth.TenantContext; 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(); @@ -31,6 +42,8 @@ void setUp() { @Test void getAllJobsReturnsAllJobsWhenNoFilterProvided() { + when(tenantAccessService.require(any(HttpHeaders.class), eq(TenantPermissions.ADMIN_OPERATE))) + .thenReturn(new TenantContext("t1", "s1", Set.of(TenantPermissions.ADMIN_OPERATE))); ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "a.pdf", "application/pdf", "hash-a", 100L); ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L); when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2)); @@ -47,6 +60,8 @@ void getAllJobsReturnsAllJobsWhenNoFilterProvided() { @Test void getAllJobsFiltersByDeadLetteredTrue() { + when(tenantAccessService.require(any(HttpHeaders.class), eq(TenantPermissions.ADMIN_OPERATE))) + .thenReturn(new TenantContext("t1", "s1", Set.of(TenantPermissions.ADMIN_OPERATE))); ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "a.pdf", "application/pdf", "hash-a", 100L); job1.markDeadLettered("failed"); ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L); @@ -64,6 +79,8 @@ void getAllJobsFiltersByDeadLetteredTrue() { @Test void getAllJobsFiltersByDeadLetteredFalse() { + when(tenantAccessService.require(any(HttpHeaders.class), eq(TenantPermissions.ADMIN_OPERATE))) + .thenReturn(new TenantContext("t1", "s1", Set.of(TenantPermissions.ADMIN_OPERATE))); ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "a.pdf", "application/pdf", "hash-a", 100L); job1.markDeadLettered("failed"); ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L); @@ -81,6 +98,8 @@ void getAllJobsFiltersByDeadLetteredFalse() { @Test void deleteJobReturnsNoContent() { + when(tenantAccessService.require(any(HttpHeaders.class), eq(TenantPermissions.ADMIN_OPERATE))) + .thenReturn(new TenantContext("t1", "s1", Set.of(TenantPermissions.ADMIN_OPERATE))); UUID jobId = UUID.randomUUID(); webTestClient.delete() @@ -91,6 +110,8 @@ void deleteJobReturnsNoContent() { @Test void retryDeadLetteredReturnsAcceptedWhenAccepted() { + when(tenantAccessService.require(any(HttpHeaders.class), eq(TenantPermissions.ADMIN_OPERATE))) + .thenReturn(new TenantContext("t1", "s1", Set.of(TenantPermissions.ADMIN_OPERATE))); UUID jobId = UUID.randomUUID(); when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.ACCEPTED); @@ -102,6 +123,8 @@ void retryDeadLetteredReturnsAcceptedWhenAccepted() { @Test void retryDeadLetteredReturnsNotFoundWhenNotFound() { + when(tenantAccessService.require(any(HttpHeaders.class), eq(TenantPermissions.ADMIN_OPERATE))) + .thenReturn(new TenantContext("t1", "s1", Set.of(TenantPermissions.ADMIN_OPERATE))); UUID jobId = UUID.randomUUID(); when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_FOUND); @@ -113,6 +136,8 @@ void retryDeadLetteredReturnsNotFoundWhenNotFound() { @Test void retryDeadLetteredReturnsConflictWhenNotEligible() { + when(tenantAccessService.require(any(HttpHeaders.class), eq(TenantPermissions.ADMIN_OPERATE))) + .thenReturn(new TenantContext("t1", "s1", Set.of(TenantPermissions.ADMIN_OPERATE))); UUID jobId = UUID.randomUUID(); when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_ELIGIBLE); @@ -121,4 +146,39 @@ void retryDeadLetteredReturnsConflictWhenNotEligible() { .exchange() .expectStatus().isEqualTo(409); // isConflict() isn't always available depending on spring-test version, so using isEqualTo(409) is safer } -} + + @Test + void getAllJobsRequiresAuthorization() { + when(tenantAccessService.require(any(HttpHeaders.class), eq(TenantPermissions.ADMIN_OPERATE))) + .thenThrow(new ResponseStatusException(HttpStatus.FORBIDDEN)); + + webTestClient.get() + .uri("/api/v1/admin/convert/jobs") + .exchange() + .expectStatus().isForbidden(); + } + + @Test + void deleteJobRequiresAuthorization() { + UUID jobId = UUID.randomUUID(); + when(tenantAccessService.require(any(HttpHeaders.class), eq(TenantPermissions.ADMIN_OPERATE))) + .thenThrow(new ResponseStatusException(HttpStatus.FORBIDDEN)); + + webTestClient.delete() + .uri("/api/v1/admin/convert/jobs/" + jobId) + .exchange() + .expectStatus().isForbidden(); + } + + @Test + void retryDeadLetteredRequiresAuthorization() { + UUID jobId = UUID.randomUUID(); + when(tenantAccessService.require(any(HttpHeaders.class), eq(TenantPermissions.ADMIN_OPERATE))) + .thenThrow(new ResponseStatusException(HttpStatus.FORBIDDEN)); + + webTestClient.post() + .uri("/api/v1/admin/convert/jobs/" + jobId + "/retry") + .exchange() + .expectStatus().isForbidden(); + } +} \ No newline at end of file