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-08-11 - ๊ด€๋ฆฌ์ž ์—”๋“œํฌ์ธํŠธ ์ธ์ฆ ๋ˆ„๋ฝ
**Vulnerability:** `AdminController`์˜ ์—”๋“œํฌ์ธํŠธ(`/api/v1/admin/convert/jobs` ๋“ฑ)์— ์–ด๋– ํ•œ ์ธ๊ฐ€(authorization)๋‚˜ ํ…Œ๋„ŒํŠธ ์ ‘๊ทผ ์ œ์–ด(tenant access checks)๋„ ๋ˆ„๋ฝ๋˜์–ด ์žˆ์—ˆ์Šต๋‹ˆ๋‹ค.
**Learning:** ๋ช…์‹œ์ ์ธ ์—”๋“œํฌ์ธํŠธ ์ˆ˜์ค€์˜ ๋ณด์•ˆ ๊ฒ€์‚ฌ๊ฐ€ ์—†๋Š” Spring ์ปดํฌ๋„ŒํŠธ๋Š” ๊ธ€๋กœ๋ฒŒ ์„ค์ •์— ์˜์กดํ•˜๊ฒŒ ๋˜๋Š”๋ฐ, ์ด ์ปจํŠธ๋กค๋Ÿฌ์— ๋Œ€ํ•ด์„œ๋Š” ํ•ด๋‹น ์„ค์ •์ด ๋ˆ„๋ฝ๋˜์–ด ์žˆ์—ˆ์Šต๋‹ˆ๋‹ค.
**Prevention:** ๋ฏผ๊ฐํ•œ ์—”๋“œํฌ์ธํŠธ๋ฅผ ์ƒˆ๋กœ ์ถ”๊ฐ€ํ•  ๋•Œ๋Š” ๋ฐ˜๋“œ์‹œ `TenantAccessService`๋ฅผ ์ฃผ์ž…ํ•˜๊ณ  ์ ์ ˆํ•œ ๊ถŒํ•œ(์˜ˆ: `admin:access`)์„ ์š”๊ตฌํ•˜๋Š”์ง€ ํ™•์ธํ•ด์•ผ ํ•ฉ๋‹ˆ๋‹ค.
Original file line number Diff line number Diff line change
Expand Up @@ -55,6 +55,11 @@ public final class TenantPermissions {
*/
public static final String ANALYTICS_READ = "analytics:read";

/**
* Permission required to perform administrative actions.
*/
public static final String ADMIN_ACCESS = "admin:access";

private TenantPermissions() {
}
}
49 changes: 38 additions & 11 deletions src/main/java/com/clearfolio/viewer/controller/AdminController.java
Original file line number Diff line number Diff line change
Expand Up @@ -4,17 +4,21 @@
import java.util.List;
import java.util.UUID;

import org.springframework.http.HttpHeaders;
import org.springframework.http.HttpStatus;
import org.springframework.http.ResponseEntity;
import org.springframework.web.bind.annotation.DeleteMapping;
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
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,26 +29,38 @@
@RestController
public class AdminController {

private final DocumentConversionService conversionService;
/** Document conversion service. */
private final DocumentConversionService conversionSvc;

/** Tenant access service. */
private final TenantAccessService tenantAccessSvc;

/**
* Creates a controller for admin operations.
*
* @param conversionService conversion service
* @param tenantAccessService tenant access service
*/
public AdminController(DocumentConversionService conversionService) {
this.conversionService = conversionService;
public AdminController(
final DocumentConversionService conversionService,
final TenantAccessService tenantAccessService) {
this.conversionSvc = conversionService;
this.tenantAccessSvc = 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) {
Iterable<ConversionJob> allJobs = conversionService.getAllJobs();
public AdminJobListResponse getAllJobs(
@RequestParam(required = false) final Boolean deadLettered,
@RequestHeader final HttpHeaders headers) {
tenantAccessSvc.require(headers, TenantPermissions.ADMIN_ACCESS);
Iterable<ConversionJob> allJobs = conversionSvc.getAllJobs();

if (deadLettered == null) {
return AdminJobListResponse.from(allJobs);
Expand All @@ -63,28 +79,39 @@ 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) {
conversionService.deleteJob(jobId);
public ResponseEntity<Void> deleteJob(
@PathVariable final UUID jobId,
@RequestHeader final HttpHeaders headers) {
tenantAccessSvc.require(headers, TenantPermissions.ADMIN_ACCESS);
conversionSvc.deleteJob(jobId);
return ResponseEntity.noContent().build();
}

/**
* 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) {
tenantAccessSvc.require(headers, TenantPermissions.ADMIN_ACCESS);
RetryDeadLetterResult result = conversionSvc.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,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;

Expand All @@ -8,22 +10,27 @@

import org.junit.jupiter.api.BeforeEach;
import org.junit.jupiter.api.Test;
import org.springframework.http.HttpHeaders;
import org.springframework.test.web.reactive.server.WebTestClient;

import com.clearfolio.viewer.auth.TenantAccessService;
import com.clearfolio.viewer.auth.TenantContext;
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);
controller = new AdminController(conversionService, tenantAccessService);
webTestClient = WebTestClient.bindToController(controller)
.controllerAdvice(new ApiExceptionHandler())
.build();
Expand All @@ -34,9 +41,11 @@ void getAllJobsReturnsAllJobsWhenNoFilterProvided() {
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));
when(tenantAccessService.require(any(), eq("admin:access"))).thenReturn(mock(TenantContext.class));

webTestClient.get()
.uri("/api/v1/admin/convert/jobs")
.header(HttpHeaders.AUTHORIZATION, "Bearer dummy")
.exchange()
.expectStatus().isOk()
.expectBody()
Expand All @@ -52,9 +61,11 @@ void getAllJobsFiltersByDeadLetteredTrue() {
ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L);

when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2));
when(tenantAccessService.require(any(), eq("admin:access"))).thenReturn(mock(TenantContext.class));

webTestClient.get()
.uri("/api/v1/admin/convert/jobs?deadLettered=true")
.header(HttpHeaders.AUTHORIZATION, "Bearer dummy")
.exchange()
.expectStatus().isOk()
.expectBody()
Expand All @@ -69,9 +80,11 @@ void getAllJobsFiltersByDeadLetteredFalse() {
ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L);

when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2));
when(tenantAccessService.require(any(), eq("admin:access"))).thenReturn(mock(TenantContext.class));

webTestClient.get()
.uri("/api/v1/admin/convert/jobs?deadLettered=false")
.header(HttpHeaders.AUTHORIZATION, "Bearer dummy")
.exchange()
.expectStatus().isOk()
.expectBody()
Expand All @@ -82,9 +95,11 @@ void getAllJobsFiltersByDeadLetteredFalse() {
@Test
void deleteJobReturnsNoContent() {
UUID jobId = UUID.randomUUID();
when(tenantAccessService.require(any(), eq("admin:access"))).thenReturn(mock(TenantContext.class));

webTestClient.delete()
.uri("/api/v1/admin/convert/jobs/" + jobId)
.header(HttpHeaders.AUTHORIZATION, "Bearer dummy")
.exchange()
.expectStatus().isNoContent();
}
Expand All @@ -93,9 +108,11 @@ void deleteJobReturnsNoContent() {
void retryDeadLetteredReturnsAcceptedWhenAccepted() {
UUID jobId = UUID.randomUUID();
when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.ACCEPTED);
when(tenantAccessService.require(any(), eq("admin:access"))).thenReturn(mock(TenantContext.class));

webTestClient.post()
.uri("/api/v1/admin/convert/jobs/" + jobId + "/retry")
.header(HttpHeaders.AUTHORIZATION, "Bearer dummy")
.exchange()
.expectStatus().isAccepted();
}
Expand All @@ -104,9 +121,11 @@ void retryDeadLetteredReturnsAcceptedWhenAccepted() {
void retryDeadLetteredReturnsNotFoundWhenNotFound() {
UUID jobId = UUID.randomUUID();
when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_FOUND);
when(tenantAccessService.require(any(), eq("admin:access"))).thenReturn(mock(TenantContext.class));

webTestClient.post()
.uri("/api/v1/admin/convert/jobs/" + jobId + "/retry")
.header(HttpHeaders.AUTHORIZATION, "Bearer dummy")
.exchange()
.expectStatus().isNotFound();
}
Expand All @@ -115,9 +134,11 @@ void retryDeadLetteredReturnsNotFoundWhenNotFound() {
void retryDeadLetteredReturnsConflictWhenNotEligible() {
UUID jobId = UUID.randomUUID();
when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_ELIGIBLE);
when(tenantAccessService.require(any(), eq("admin:access"))).thenReturn(mock(TenantContext.class));

webTestClient.post()
.uri("/api/v1/admin/convert/jobs/" + jobId + "/retry")
.header(HttpHeaders.AUTHORIZATION, "Bearer dummy")
.exchange()
.expectStatus().isEqualTo(409); // isConflict() isn't always available depending on spring-test version, so using isEqualTo(409) is safer
}
Expand Down
Loading