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-10 - κ΄€λ¦¬μž API의 Broken Access Control 및 ꡐ차 ν…Œλ„ŒνŠΈ 데이터 유좜 λ°©μ§€ (Enforce Authentication and Tenant Isolation in Admin API)
**Vulnerability:** `AdminController`의 μ—”λ“œν¬μΈνŠΈλ“€μ— 인증 및 인가 검증이 λˆ„λ½λ˜μ–΄ μžˆμ—ˆμŠ΅λ‹ˆλ‹€. κ³΅κ²©μžκ°€ μ–΄λ– ν•œ 자격 증λͺ…μ΄λ‚˜ μ μ ˆν•œ κΆŒν•œ 없이도 λͺ¨λ“  ν…Œλ„ŒνŠΈμ˜ λ³€ν™˜ μž‘μ—…(Conversion Job)을 쑰회, μ‚­μ œ, μž¬μ‹œλ„ν•  수 μžˆμ–΄ Broken Access Control (BAC) 및 ꡐ차 ν…Œλ„ŒνŠΈ 데이터 유좜이 λ°œμƒν•  수 μžˆμ—ˆμŠ΅λ‹ˆλ‹€.
**Learning:** κ΄€λ¦¬μž μ»¨νŠΈλ‘€λŸ¬λŠ” 일반 μ»¨νŠΈλ‘€λŸ¬μ™€ λ™μΌν•˜κ²Œ κ°•λ ₯ν•œ 인증 및 ν…Œλ„ŒνŠΈ 격리 λ©”μ»€λ‹ˆμ¦˜μ„ μ μš©ν•΄μ•Ό ν•©λ‹ˆλ‹€. κ²½λ‘œμ— "admin"이 ν¬ν•¨λ˜μ–΄ μžˆλ‹€κ³  ν•΄μ„œ μ—”λ“œν¬μΈνŠΈκ°€ 본질적으둜 λ³΄ν˜Έλœλ‹€κ³  κ°€μ •ν•΄μ„œλŠ” μ•ˆ 되며, λ³΄μ•ˆ ν•„ν„°λ‚˜ μ•‘μ„ΈμŠ€ μ„œλΉ„μŠ€(예: `TenantAccessService`)와 λͺ…μ‹œμ μœΌλ‘œ 톡합해야 ν•©λ‹ˆλ‹€. λ˜ν•œ, μž‘μ—… 흐름(예: μž¬μ‹œλ„ μ‹œ `operatorId`)에 기둝될 λ•Œ ν”„λΌμ΄λ²„μ‹œλ₯Ό λ³΄ν˜Έν•˜κΈ° μœ„ν•΄ 원본 μ‚¬μš©μž IDλŠ” κ°€λͺ…ν™”(pseudonymize)λ˜μ–΄μ•Ό ν•©λ‹ˆλ‹€.
**Prevention:** κ΄€λ¦¬μž μ—”λ“œν¬μΈνŠΈμ—λŠ” 항상 `TenantAccessService`(λ˜λŠ” λ™λ“±ν•œ μ»¨ν…μŠ€νŠΈ 검증기)λ₯Ό μ£Όμž…ν•˜κ³ , ν˜ΈμΆœμžκ°€ λͺ…μ‹œμ μΈ κ΄€λ¦¬μž κΆŒν•œ(예: `JOB_DELETE`)을 κ°€μ§€κ³  μžˆλŠ”μ§€ ν™•μΈν•˜λ©°, λ¦¬μ†ŒμŠ€μ— μ ‘κ·Όν•˜κΈ° 전에 μ†Œμœ κΆŒ(`job.belongsToTenant(...)`)을 ν™•μΈν•˜μ—¬ ν…Œλ„ŒνŠΈ 격리λ₯Ό κ°•μ œν•΄μ•Ό ν•©λ‹ˆλ‹€.
99 changes: 84 additions & 15 deletions src/main/java/com/clearfolio/viewer/controller/AdminController.java
Original file line number Diff line number Diff line change
Expand Up @@ -10,11 +10,22 @@
import org.springframework.web.bind.annotation.GetMapping;
import org.springframework.web.bind.annotation.PathVariable;
import org.springframework.web.bind.annotation.PostMapping;
import java.nio.charset.StandardCharsets;
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;
import java.util.HexFormat;
import java.util.Optional;

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.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;
Expand All @@ -25,35 +36,51 @@
@RestController
public class AdminController {

/** Hex formatter for pseudonyms. */
private static final HexFormat HEX_FORMAT = HexFormat.of();

/** Service to perform document conversion jobs. */
private final DocumentConversionService conversionService;

/** Service to authorize tenants and track permissions. */
private final TenantAccessService tenantAccessService;

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

/**
* Retrieves all conversion jobs, optionally filtered by dead-letter status.
*
* @param headers request headers carrying tenant claims
* @param deadLettered optional filter for dead-lettered jobs
* @return list of conversion jobs
*/
Comment on lines 61 to 67

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ“ Maintainability & Code Quality | 🟑 Minor | ⚑ Quick win

ν…Œλ„ŒνŠΈ λ²”μœ„λ₯Ό JavaDoc에 λ°˜μ˜ν•˜μ‹­μ‹œμ˜€.

Line 62의 β€œall conversion jobs” μ„€λͺ…은 κ΅¬ν˜„κ³Ό λ‹€λ¦…λ‹ˆλ‹€. 이 λ©”μ„œλ“œλŠ” context.tenantId()에 μ†ν•œ μž‘μ—…λ§Œ λ°˜ν™˜ν•©λ‹ˆλ‹€. @return μ„€λͺ…도 ν˜„μž¬ ν…Œλ„ŒνŠΈμ˜ μž‘μ—… λͺ©λ‘μœΌλ‘œ λ³€κ²½ν•˜μ‹­μ‹œμ˜€.

πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/main/java/com/clearfolio/viewer/controller/AdminController.java` around
lines 61 - 67, Update the JavaDoc for the conversion-job retrieval method near
the shown documentation to state that it returns only jobs belonging to
context.tenantId(), not all conversion jobs. Also revise the `@return` description
to specify the list of jobs for the current tenant, while leaving the
implementation unchanged.

@GetMapping("/api/v1/admin/convert/jobs")
public AdminJobListResponse getAllJobs(@RequestParam(required = false) Boolean deadLettered) {
public AdminJobListResponse getAllJobs(
@RequestHeader final HttpHeaders headers,
@RequestParam(required = false) final Boolean deadLettered
) {
TenantContext context = tenantAccessService
.require(headers, TenantPermissions.JOB_READ);
Iterable<ConversionJob> allJobs = conversionService.getAllJobs();

if (deadLettered == null) {
return AdminJobListResponse.from(allJobs);
}

List<ConversionJob> filtered = new ArrayList<>();
for (ConversionJob job : allJobs) {
if (job.isDeadLettered() == deadLettered) {
filtered.add(job);
if (job.belongsToTenant(context.tenantId())) {
if (deadLettered == null
|| job.isDeadLettered() == deadLettered) {
filtered.add(job);
}
}
}
return AdminJobListResponse.from(filtered);
Expand All @@ -62,30 +89,72 @@ public AdminJobListResponse getAllJobs(@RequestParam(required = false) Boolean d
/**
* Deletes a conversion job.
*
* @param headers request headers carrying tenant claims
* @param jobId conversion job identifier
* @return no content on success
*/
@DeleteMapping("/api/v1/admin/convert/jobs/{jobId}")
public ResponseEntity<Void> deleteJob(@PathVariable UUID jobId) {
public ResponseEntity<Void> deleteJob(
@RequestHeader final HttpHeaders headers,
@PathVariable final UUID jobId) {
TenantContext context = tenantAccessService
.require(headers, TenantPermissions.JOB_DELETE);
Optional<ConversionJob> job = conversionService.getJob(jobId);
if (job.isEmpty()
|| !job.get().belongsToTenant(context.tenantId())) {
throw new ResponseStatusException(HttpStatus.NOT_FOUND,
"job not found");
}

conversionService.deleteJob(jobId);
return ResponseEntity.noContent().build();
}

/**
* Retries a dead-lettered conversion job.
*
* @param headers request headers carrying tenant claims
* @param jobId conversion job identifier
* @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(
@RequestHeader final HttpHeaders headers,
@PathVariable final UUID jobId) {
TenantContext context = tenantAccessService
.require(headers, TenantPermissions.JOB_RETRY);
Optional<ConversionJob> job = conversionService.getJob(jobId);
if (job.isEmpty()
|| !job.get().belongsToTenant(context.tenantId())) {
throw new ResponseStatusException(HttpStatus.NOT_FOUND,
"job not found");
}

String pseudonymizedOperatorId = hashSubjectId(context.subjectId());
RetryDeadLetterResult result = conversionService
.retryDeadLettered(jobId, pseudonymizedOperatorId);
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();
}

private String hashSubjectId(final String subjectId) {
if (subjectId == null) {
return "absent";
}
try {
MessageDigest digest = MessageDigest.getInstance("SHA-256");
byte[] hash = digest.digest(
subjectId.getBytes(StandardCharsets.UTF_8));
return HEX_FORMAT.formatHex(hash);
} catch (NoSuchAlgorithmException e) {
throw new IllegalStateException("SHA-256 not available", e);
}
}
}
Original file line number Diff line number Diff line change
@@ -1,42 +1,66 @@
package com.clearfolio.viewer.controller;

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

import java.util.Arrays;
import java.util.Optional;
import java.util.UUID;

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.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);
controller = new AdminController(conversionService, tenantAccessService);
webTestClient = WebTestClient.bindToController(controller)
.controllerAdvice(new ApiExceptionHandler())
.build();
}

private void mockTenantContext(String permission) {
TenantContext context = new TenantContext("tenant-a", "user-1", null);
when(tenantAccessService.require(any(HttpHeaders.class), eq(permission)))
.thenReturn(context);
}

@Test
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));
mockTenantContext(TenantPermissions.JOB_READ);
ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1",
"a.pdf", "application/pdf", "hash-a", 100L, 3);
ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1",
"b.pdf", "application/pdf", "hash-b", 100L, 3);
ConversionJob job3 = new ConversionJob(UUID.randomUUID(), "tenant-b", "user-1",
"c.pdf", "application/pdf", "hash-c", 100L, 3);
when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2, job3));

webTestClient.get()
.uri("/api/v1/admin/convert/jobs")
.header(TenantContext.TENANT_ID_HEADER, "tenant-a")
.header(TenantContext.SUBJECT_ID_HEADER, "user-1")
.header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_READ)
.exchange()
.expectStatus().isOk()
.expectBody()
Expand All @@ -47,14 +71,20 @@ void getAllJobsReturnsAllJobsWhenNoFilterProvided() {

@Test
void getAllJobsFiltersByDeadLetteredTrue() {
ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "a.pdf", "application/pdf", "hash-a", 100L);
mockTenantContext(TenantPermissions.JOB_READ);
ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1",
"a.pdf", "application/pdf", "hash-a", 100L, 3);
job1.markDeadLettered("failed");
ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L);
ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1",
"b.pdf", "application/pdf", "hash-b", 100L, 3);

when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2));

webTestClient.get()
.uri("/api/v1/admin/convert/jobs?deadLettered=true")
.header(TenantContext.TENANT_ID_HEADER, "tenant-a")
.header(TenantContext.SUBJECT_ID_HEADER, "user-1")
.header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_READ)
.exchange()
.expectStatus().isOk()
.expectBody()
Expand All @@ -64,14 +94,20 @@ void getAllJobsFiltersByDeadLetteredTrue() {

@Test
void getAllJobsFiltersByDeadLetteredFalse() {
ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "a.pdf", "application/pdf", "hash-a", 100L);
mockTenantContext(TenantPermissions.JOB_READ);
ConversionJob job1 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1",
"a.pdf", "application/pdf", "hash-a", 100L, 3);
job1.markDeadLettered("failed");
ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "b.pdf", "application/pdf", "hash-b", 100L);
ConversionJob job2 = new ConversionJob(UUID.randomUUID(), "tenant-a", "user-1",
"b.pdf", "application/pdf", "hash-b", 100L, 3);

when(conversionService.getAllJobs()).thenReturn(Arrays.asList(job1, job2));

webTestClient.get()
.uri("/api/v1/admin/convert/jobs?deadLettered=false")
.header(TenantContext.TENANT_ID_HEADER, "tenant-a")
.header(TenantContext.SUBJECT_ID_HEADER, "user-1")
.header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_READ)
.exchange()
.expectStatus().isOk()
.expectBody()
Expand All @@ -81,44 +117,75 @@ void getAllJobsFiltersByDeadLetteredFalse() {

@Test
void deleteJobReturnsNoContent() {
mockTenantContext(TenantPermissions.JOB_DELETE);
UUID jobId = UUID.randomUUID();
ConversionJob job = new ConversionJob(jobId, "tenant-a", "user-1",
"a.pdf", "application/pdf", "hash", 100L, 3);
when(conversionService.getJob(jobId)).thenReturn(Optional.of(job));

webTestClient.delete()
.uri("/api/v1/admin/convert/jobs/" + jobId)
.header(TenantContext.TENANT_ID_HEADER, "tenant-a")
.header(TenantContext.SUBJECT_ID_HEADER, "user-1")
.header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_DELETE)
.exchange()
.expectStatus().isNoContent();
}

@Test
void retryDeadLetteredReturnsAcceptedWhenAccepted() {
mockTenantContext(TenantPermissions.JOB_RETRY);
UUID jobId = UUID.randomUUID();
when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.ACCEPTED);
ConversionJob job = new ConversionJob(jobId, "tenant-a", "user-1",
"a.pdf", "application/pdf", "hash", 100L, 3);
when(conversionService.getJob(jobId)).thenReturn(Optional.of(job));
when(conversionService.retryDeadLettered(eq(jobId), anyString()))
.thenReturn(RetryDeadLetterResult.ACCEPTED);
Comment on lines 135 to +143

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

πŸ”’ Security & Privacy | 🟠 Major | ⚑ Quick win

μž¬μ‹œλ„ 주체 κ°€λͺ…ν™” 값을 μ •ν™•νžˆ κ²€μ¦ν•˜μ‹­μ‹œμ˜€.

Line 142의 anyString()은 원본 "user-1" λ˜λŠ” μž„μ˜μ˜ λ¬Έμžμ—΄λ„ ν—ˆμš©ν•©λ‹ˆλ‹€. hashSubjectIdκ°€ μ œκ±°λ˜κ±°λ‚˜ 잘λͺ»λœ 값을 λ°˜ν™˜ν•΄λ„ 이 ν…ŒμŠ€νŠΈλŠ” ν†΅κ³Όν•©λ‹ˆλ‹€. retryDeadLettered 호좜이 "user-1"의 μ •ν™•ν•œ SHA-256 hexadecimal κ°’λ§Œ λ°›λŠ”μ§€ κ²€μ¦ν•˜μ‹­μ‹œμ˜€.

As per coding guidelines, com.clearfolio.viewer.*의 생산 변경은 100% JaCoCo line and branch coverageλ₯Ό μœ μ§€ν•΄μ•Ό ν•©λ‹ˆλ‹€.

πŸ€– Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.

In `@src/test/java/com/clearfolio/viewer/controller/AdminControllerTest.java`
around lines 135 - 143, Update retryDeadLetteredReturnsAcceptedWhenAccepted so
the conversionService.retryDeadLettered verification matches the exact
precomputed SHA-256 hexadecimal digest of "user-1" instead of anyString(). Keep
the job setup and accepted-result behavior unchanged, and ensure the assertion
proves hashSubjectId supplies that exact value.

Source: Coding guidelines


webTestClient.post()
.uri("/api/v1/admin/convert/jobs/" + jobId + "/retry")
.header(TenantContext.TENANT_ID_HEADER, "tenant-a")
.header(TenantContext.SUBJECT_ID_HEADER, "user-1")
.header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_RETRY)
.exchange()
.expectStatus().isAccepted();
}

@Test
void retryDeadLetteredReturnsNotFoundWhenNotFound() {
mockTenantContext(TenantPermissions.JOB_RETRY);
UUID jobId = UUID.randomUUID();
when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_FOUND);
ConversionJob job = new ConversionJob(jobId, "tenant-a", "user-1",
"a.pdf", "application/pdf", "hash", 100L, 3);
when(conversionService.getJob(jobId)).thenReturn(Optional.of(job));
when(conversionService.retryDeadLettered(eq(jobId), anyString()))
.thenReturn(RetryDeadLetterResult.NOT_FOUND);

webTestClient.post()
.uri("/api/v1/admin/convert/jobs/" + jobId + "/retry")
.header(TenantContext.TENANT_ID_HEADER, "tenant-a")
.header(TenantContext.SUBJECT_ID_HEADER, "user-1")
.header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_RETRY)
.exchange()
.expectStatus().isNotFound();
}

@Test
void retryDeadLetteredReturnsConflictWhenNotEligible() {
mockTenantContext(TenantPermissions.JOB_RETRY);
UUID jobId = UUID.randomUUID();
when(conversionService.retryDeadLettered(jobId, "admin")).thenReturn(RetryDeadLetterResult.NOT_ELIGIBLE);
ConversionJob job = new ConversionJob(jobId, "tenant-a", "user-1",
"a.pdf", "application/pdf", "hash", 100L, 3);
when(conversionService.getJob(jobId)).thenReturn(Optional.of(job));
when(conversionService.retryDeadLettered(eq(jobId), anyString()))
.thenReturn(RetryDeadLetterResult.NOT_ELIGIBLE);

webTestClient.post()
.uri("/api/v1/admin/convert/jobs/" + jobId + "/retry")
.header(TenantContext.TENANT_ID_HEADER, "tenant-a")
.header(TenantContext.SUBJECT_ID_HEADER, "user-1")
.header(TenantContext.PERMISSIONS_HEADER, TenantPermissions.JOB_RETRY)
.exchange()
.expectStatus().isEqualTo(409); // isConflict() isn't always available depending on spring-test version, so using isEqualTo(409) is safer
.expectStatus().isEqualTo(409);
}
}
Loading