Skip to content
Closed
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions .jules/sentinel.md
Original file line number Diff line number Diff line change
Expand Up @@ -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-07-30 - Admin endpoint 인증 누락 해결
**Vulnerability:** `/api/v1/admin/*` 경로의 API 엔드포인트들에 인증 및 권한 확인(`TenantAccessService.require`)이 누락되어 있어, 인가되지 않은 사용자가 관리자 기능을 사용할 수 있는 심각한 보안 취약점이 있었습니다.
**Learning:** 새로운 API 컨트롤러(`AdminController`)를 추가할 때 전역적인 보안 필터가 아닌 컨트롤러 수준에서 권한 검증을 강제하고 있었으므로, 새 엔드포인트에 실수로 권한 검증 코드를 누락하기 쉬운 구조였습니다.
**Prevention:** 모든 새로운 엔드포인트 추가 시 반드시 `TenantAccessService`를 주입받아 적절한 `TenantPermissions` 권한을 검증하는지 확인해야 하며, 이를 자동화된 테스트나 리뷰 프로세스에서 필수적으로 점검해야 합니다.
Original file line number Diff line number Diff line change
Expand Up @@ -50,6 +50,11 @@ public final class TenantPermissions {
*/
public static final String ANALYTICS_READ = "analytics:read";

/**
* Permission required to manage admin operations.
*/
public static final String ADMIN_MANAGE = "admin:manage";

private TenantPermissions() {
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,16 @@
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.http.HttpHeaders;
import org.springframework.web.bind.annotation.PostMapping;
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;
Expand All @@ -25,25 +29,41 @@
@RestController
public class AdminController {

/**
* Conversion service for document processing.
*/
private final DocumentConversionService conversionService;

/**
* Service for tenant and permission enforcement.
*/
private final TenantAccessService tenantAccessService;

/**
* Creates a controller for admin operations.
*
* @param conversionService conversion service
* @param conversionSvc conversion service
* @param tenantAccessSvc access service
*/
public AdminController(DocumentConversionService conversionService) {
this.conversionService = conversionService;
public AdminController(
final DocumentConversionService conversionSvc,
final TenantAccessService tenantAccessSvc) {
this.conversionService = conversionSvc;
this.tenantAccessService = tenantAccessSvc;
}

/**
* 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_MANAGE);
Iterable<ConversionJob> allJobs = conversionService.getAllJobs();

if (deadLettered == null) {
Expand All @@ -63,10 +83,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<Void> deleteJob(@PathVariable UUID jobId) {
public ResponseEntity<Void> deleteJob(
@PathVariable final UUID jobId,
@RequestHeader final HttpHeaders headers) {
tenantAccessService.require(headers, TenantPermissions.ADMIN_MANAGE);
conversionService.deleteJob(jobId);
return ResponseEntity.noContent().build();
}
Expand All @@ -75,16 +99,23 @@ public ResponseEntity<Void> 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<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.ADMIN_MANAGE);
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();
}
Expand Down
Original file line number Diff line number Diff line change
@@ -1,29 +1,41 @@
package com.clearfolio.viewer.controller;

import static org.mockito.Mockito.mock;
import static org.mockito.ArgumentMatchers.any;
import static org.mockito.ArgumentMatchers.eq;
import static org.mockito.Mockito.when;

import java.util.Arrays;
import java.util.Set;
import java.util.UUID;

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.auth.TenantPermissions;
import com.clearfolio.viewer.model.ConversionJob;
import com.clearfolio.viewer.service.DocumentConversionService;
import com.clearfolio.viewer.service.RetryDeadLetterResult;

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);

TenantContext dummyContext = new TenantContext("tenant", "subject", Set.of(TenantPermissions.ADMIN_MANAGE));
when(tenantAccessService.require(any(), eq(TenantPermissions.ADMIN_MANAGE))).thenReturn(dummyContext);

controller = new AdminController(conversionService, tenantAccessService);
webTestClient = WebTestClient.bindToController(controller)
.controllerAdvice(new ApiExceptionHandler())
.build();
Expand Down
Loading