fix(bulk-edit): backend hardening — audit, caps, dedupe, CRLF, WRITE_ALL on /ids
Addresses Markus B1+B2, Nora C1+C4+C5, Tobias #1, Sara B1+B2+C2, Elicit S2+C4 from the cycle 1 review on PR #331. Audit / version trail applyBulkEditToDocument now takes actorId, calls documentVersionService.recordVersion(saved), and emits an AuditKind.METADATA_UPDATED event tagged source=BULK_EDIT — restoring parity with the single-doc updateDocument path. Caps /api/documents/batch-metadata: 500-ID cap (matches PATCH cap) /api/documents/ids: 5000 result cap with BULK_EDIT_TOO_MANY_IDS on overflow Permission tightening /api/documents/ids re-gated WRITE_ALL — its only consumer is the bulk-edit fast path (least-privilege per Elicit S2 + Nora's defence-in-depth). Audit log /ids and /batch-metadata now emit one log.info per call, mirroring the quickUpload + bulkEdit format. Robustness Duplicates in PATCH documentIds are de-duplicated via LinkedHashSet so a double-clicked "Alle X editieren" cannot inflate the updated count. log.warn lines that interpolate Throwable.getMessage() now run through a CRLF-strip helper (CWE-117). Tests added applyBulkEditToDocument_recordsVersion_andLogsAuditEvent_taggedSourceBulkEdit patchBulk_acceptsExactly500Ids_atTheCap (off-by-one fence) patchBulk_dedupesDuplicateDocumentIds_doesNotInflateUpdatedCount getDocumentIds_returns403_forUserWithoutWriteAll getDocumentIds_returns400_whenResultExceedsFilterCap batchMetadata_returns403_forUserWithoutReadAll batchMetadata_returns400_whenIdsExceedsCap All 231 backend tests green. Refs #225, PR #331 Co-Authored-By: Claude Sonnet 4.6 <noreply@anthropic.com>
This commit is contained in:
Marcel
@@ -245,6 +245,11 @@ public class DocumentController {
|
||||
// --- BULK EDIT ---
|
||||
|
||||
private static final int BULK_EDIT_MAX_IDS = 500;
|
||||
/** Hard cap for {@code GET /api/documents/ids}: prevents an unfiltered
|
||||
* call from materialising the entire {@code documents} table into JSON.
|
||||
* Generous enough for real-world "Alle X editieren" against the family
|
||||
* archive's bounded scale (~1500 docs today, expected growth to ~5k). */
|
||||
private static final int BULK_EDIT_FILTER_MAX_IDS = 5000;
|
||||
|
||||
@PatchMapping("/bulk")
|
||||
@RequirePermission(Permission.WRITE_ALL)
|
||||
@@ -263,26 +268,37 @@ public class DocumentController {
|
||||
int updated = 0;
|
||||
List<BulkEditError> errors = new ArrayList<>();
|
||||
|
||||
for (UUID id : dto.getDocumentIds()) {
|
||||
// Dedupe duplicate document IDs while preserving submission order. A
|
||||
// double-click on "Alle X editieren" would otherwise hit each document
|
||||
// twice and inflate the `updated` count returned to the user.
|
||||
java.util.LinkedHashSet<UUID> uniqueIds = new java.util.LinkedHashSet<>(dto.getDocumentIds());
|
||||
|
||||
for (UUID id : uniqueIds) {
|
||||
try {
|
||||
documentService.applyBulkEditToDocument(id, dto);
|
||||
documentService.applyBulkEditToDocument(id, dto, actorId);
|
||||
updated++;
|
||||
} catch (DomainException e) {
|
||||
errors.add(new BulkEditError(id, e.getMessage()));
|
||||
errors.add(new BulkEditError(id, sanitizeForLog(e.getMessage())));
|
||||
} catch (Exception e) {
|
||||
errors.add(new BulkEditError(id, "Internal error"));
|
||||
log.warn("Bulk edit failed for document {}: {}", id, e.getMessage());
|
||||
log.warn("Bulk edit failed for document {}: {}", id, sanitizeForLog(e.getMessage()));
|
||||
}
|
||||
}
|
||||
|
||||
log.info("bulkEdit actor={} documentIds={} updated={} errors={}",
|
||||
actorId, dto.getDocumentIds().size(), updated, errors.size());
|
||||
log.info("bulkEdit actor={} documentIds={} unique={} updated={} errors={}",
|
||||
actorId, dto.getDocumentIds().size(), uniqueIds.size(), updated, errors.size());
|
||||
|
||||
return new BulkEditResult(updated, errors);
|
||||
}
|
||||
|
||||
/** CRLF strip for any log line interpolating a free-form string (e.g.
|
||||
* {@link Throwable#getMessage()}). Defends against CWE-117 log injection. */
|
||||
private static String sanitizeForLog(String s) {
|
||||
return s == null ? null : s.replaceAll("[\\r\\n]", "_");
|
||||
}
|
||||
|
||||
@GetMapping("/ids")
|
||||
@RequirePermission(Permission.READ_ALL)
|
||||
@RequirePermission(Permission.WRITE_ALL)
|
||||
public List<UUID> getDocumentIds(
|
||||
@RequestParam(required = false) String q,
|
||||
@RequestParam(required = false) LocalDate from,
|
||||
@@ -292,17 +308,31 @@ public class DocumentController {
|
||||
@RequestParam(required = false, name = "tag") List<String> tags,
|
||||
@RequestParam(required = false) String tagQ,
|
||||
@RequestParam(required = false) DocumentStatus status,
|
||||
@RequestParam(required = false) String tagOp) {
|
||||
@RequestParam(required = false) String tagOp,
|
||||
Authentication authentication) {
|
||||
TagOperator operator = "OR".equalsIgnoreCase(tagOp) ? TagOperator.OR : TagOperator.AND;
|
||||
return documentService.findIdsForFilter(q, from, to, senderId, receiverId, tags, tagQ, status, operator);
|
||||
List<UUID> ids = documentService.findIdsForFilter(q, from, to, senderId, receiverId, tags, tagQ, status, operator);
|
||||
if (ids.size() > BULK_EDIT_FILTER_MAX_IDS) {
|
||||
throw DomainException.badRequest(ErrorCode.BULK_EDIT_TOO_MANY_IDS,
|
||||
"Filter matches " + ids.size() + " documents — refine filter (max " + BULK_EDIT_FILTER_MAX_IDS + ")");
|
||||
}
|
||||
UUID actorId = requireUserId(authentication);
|
||||
log.info("documentIds actor={} matched={}", actorId, ids.size());
|
||||
return ids;
|
||||
}
|
||||
|
||||
@PostMapping(value = "/batch-metadata", consumes = MediaType.APPLICATION_JSON_VALUE)
|
||||
@RequirePermission(Permission.READ_ALL)
|
||||
public List<DocumentBatchSummary> batchMetadata(@RequestBody BatchMetadataRequest request) {
|
||||
public List<DocumentBatchSummary> batchMetadata(@RequestBody BatchMetadataRequest request, Authentication authentication) {
|
||||
if (request == null || request.ids() == null || request.ids().isEmpty()) {
|
||||
throw new ResponseStatusException(HttpStatus.BAD_REQUEST, "ids is required");
|
||||
}
|
||||
if (request.ids().size() > BULK_EDIT_MAX_IDS) {
|
||||
throw DomainException.badRequest(ErrorCode.BULK_EDIT_TOO_MANY_IDS,
|
||||
"Maximum " + BULK_EDIT_MAX_IDS + " ids per request, got: " + request.ids().size());
|
||||
}
|
||||
UUID actorId = requireUserId(authentication);
|
||||
log.info("batchMetadata actor={} ids={}", actorId, request.ids().size());
|
||||
return documentService.batchMetadata(request.ids());
|
||||
}
|
||||
|
||||
|
||||
@@ -400,10 +400,20 @@ public class DocumentService {
|
||||
* Tags and receivers are additive (merged into existing sets); sender and the
|
||||
* three location fields are replace-on-non-blank (null/blank means "no change").
|
||||
* Wrapped in its own transaction so a failure on one document never partially
|
||||
* mutates another in the batch loop.
|
||||
* mutates another in the controller's batch loop.
|
||||
*
|
||||
* Each successful update emits a {@link AuditKind#METADATA_UPDATED} audit
|
||||
* event tagged {@code source=BULK_EDIT} and writes a row to
|
||||
* {@code document_versions} so the family archive's "who changed what"
|
||||
* trail stays complete across both single- and bulk-doc edit paths.
|
||||
*
|
||||
* NOTE on N+1: tag and person resolution happens per-document. With 500
|
||||
* documents × 10 tags this fans out to ~5000 tag-resolve queries per
|
||||
* request. Acceptable today because the family archive is bounded at
|
||||
* ~1500 documents total. Tracked as a perf follow-up.
|
||||
*/
|
||||
@Transactional
|
||||
public Document applyBulkEditToDocument(UUID id, org.raddatz.familienarchiv.dto.DocumentBulkEditDTO dto) {
|
||||
public Document applyBulkEditToDocument(UUID id, org.raddatz.familienarchiv.dto.DocumentBulkEditDTO dto, UUID actorId) {
|
||||
Document doc = documentRepository.findById(id)
|
||||
.orElseThrow(() -> DomainException.notFound(ErrorCode.DOCUMENT_NOT_FOUND, "Document not found: " + id));
|
||||
|
||||
@@ -438,7 +448,11 @@ public class DocumentService {
|
||||
doc.setArchiveFolder(dto.getArchiveFolder());
|
||||
}
|
||||
|
||||
return documentRepository.save(doc);
|
||||
Document saved = documentRepository.save(doc);
|
||||
documentVersionService.recordVersion(saved);
|
||||
auditService.logAfterCommit(AuditKind.METADATA_UPDATED, actorId, saved.getId(),
|
||||
Map.of("source", "BULK_EDIT"));
|
||||
return saved;
|
||||
}
|
||||
|
||||
/**
|
||||
|
||||
Reference in New Issue
Block a user