Skip to content

Commit 356ca98

Browse files
committed
fix(gdpr): address additional PR review feedback
Apply fixes from second round of code review: Security/Compliance: - Sanitize custom consent type names in logs to prevent PII exposure (log "CUSTOM" instead of actual custom type name) Performance/Stability: - Add configurable maxQueryResults limit to AuditConfig (default 10000) to prevent unbounded memory usage when querying large audit logs - Apply limit in FileAuditLogQueryService.findByUser() stream Code Quality: - Inject Spring-configured ObjectMapper via constructor in GdprExportService and ConsentAuditService instead of creating new instances (respects application's Jackson configuration) - Update tests with @SPY ObjectMapper for proper injection Configuration: - Add user.audit.maxQueryResults property to dsspringuserconfig.properties
1 parent 69ad61b commit 356ca98

7 files changed

Lines changed: 44 additions & 11 deletions

File tree

src/main/java/com/digitalsanctuary/spring/user/audit/AuditConfig.java

Lines changed: 8 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -47,4 +47,12 @@ public class AuditConfig {
4747
*/
4848
private int flushRate;
4949

50+
/**
51+
* Maximum number of audit events to return from a single query.
52+
* This prevents unbounded memory usage when querying large audit logs.
53+
* Set to 0 or negative to disable the limit (not recommended for production).
54+
* Default is 10000.
55+
*/
56+
private int maxQueryResults = 10000;
57+
5058
}

src/main/java/com/digitalsanctuary/spring/user/audit/FileAuditLogQueryService.java

Lines changed: 11 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -104,8 +104,10 @@ private List<AuditEventDTO> findByUser(User user, Instant since, String action)
104104
String userEmail = user.getEmail();
105105
String userId = user.getId() != null ? user.getId().toString() : null;
106106

107+
int maxResults = auditConfig.getMaxQueryResults();
108+
107109
try (Stream<String> lines = Files.lines(logPath)) {
108-
return lines
110+
Stream<AuditEventDTO> stream = lines
109111
.skip(1) // Skip header line
110112
.map(this::parseLine)
111113
.filter(Objects::nonNull)
@@ -114,8 +116,14 @@ private List<AuditEventDTO> findByUser(User user, Instant since, String action)
114116
!event.getTimestamp().isBefore(since))
115117
.filter(event -> action == null || action.equals(event.getAction()))
116118
.sorted(Comparator.comparing(AuditEventDTO::getTimestamp,
117-
Comparator.nullsLast(Comparator.reverseOrder())))
118-
.collect(Collectors.toList());
119+
Comparator.nullsLast(Comparator.reverseOrder())));
120+
121+
// Apply limit if configured to prevent unbounded memory usage
122+
if (maxResults > 0) {
123+
stream = stream.limit(maxResults);
124+
}
125+
126+
return stream.collect(Collectors.toList());
119127
} catch (IOException e) {
120128
log.error("FileAuditLogQueryService.findByUser: Error reading audit log file", e);
121129
return Collections.emptyList();

src/main/java/com/digitalsanctuary/spring/user/gdpr/ConsentAuditService.java

Lines changed: 7 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -55,9 +55,7 @@ public class ConsentAuditService {
5555
private final GdprConfig gdprConfig;
5656
private final ApplicationEventPublisher eventPublisher;
5757
private final AuditLogQueryService auditLogQueryService;
58-
59-
/** ObjectMapper for JSON serialization/deserialization of consent extra data. */
60-
private final ObjectMapper objectMapper = new ObjectMapper();
58+
private final ObjectMapper objectMapper;
6159

6260
/**
6361
* Records that a user has granted consent.
@@ -134,8 +132,10 @@ public ConsentRecord recordConsentGranted(User user, ConsentType consentType, St
134132
eventPublisher.publishEvent(new ConsentChangedEvent(this, user, record,
135133
ConsentChangedEvent.ChangeType.GRANTED));
136134

135+
// Log consent type safely - avoid exposing custom type names which could contain PII
136+
String logTypeName = record.getType() == ConsentType.CUSTOM ? "CUSTOM" : record.getEffectiveTypeName();
137137
log.info("ConsentAuditService.recordConsentGranted: Recorded consent grant for user {} - type {}",
138-
user.getId(), record.getEffectiveTypeName());
138+
user.getId(), logTypeName);
139139

140140
return record;
141141
}
@@ -212,8 +212,10 @@ public ConsentRecord recordConsentWithdrawn(User user, ConsentType consentType,
212212
eventPublisher.publishEvent(new ConsentChangedEvent(this, user, record,
213213
ConsentChangedEvent.ChangeType.WITHDRAWN));
214214

215+
// Log consent type safely - avoid exposing custom type names which could contain PII
216+
String logTypeName = record.getType() == ConsentType.CUSTOM ? "CUSTOM" : record.getEffectiveTypeName();
215217
log.info("ConsentAuditService.recordConsentWithdrawn: Recorded consent withdrawal for user {} - type {}",
216-
user.getId(), record.getEffectiveTypeName());
218+
user.getId(), logTypeName);
217219

218220
return record;
219221
}

src/main/java/com/digitalsanctuary/spring/user/gdpr/GdprExportService.java

Lines changed: 1 addition & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -57,9 +57,7 @@ public class GdprExportService {
5757
private final PasswordResetTokenRepository passwordResetTokenRepository;
5858
private final List<GdprDataContributor> dataContributors;
5959
private final ApplicationEventPublisher eventPublisher;
60-
61-
/** ObjectMapper for JSON deserialization of consent extra data. */
62-
private final ObjectMapper objectMapper = new ObjectMapper();
60+
private final ObjectMapper objectMapper;
6361

6462
/**
6563
* Exports all GDPR-relevant data for a user.

src/main/resources/config/dsspringuserconfig.properties

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -27,6 +27,11 @@ user.audit.flushRate=30000
2727
# If true, all events will be logged.
2828
user.audit.logEvents=true
2929

30+
# Maximum number of audit events to return from a single query.
31+
# Prevents unbounded memory usage when querying large audit logs.
32+
# Set to 0 or negative to disable the limit (not recommended for production).
33+
user.audit.maxQueryResults=10000
34+
3035

3136
# If true, users can delete their own accounts. If false, accounts are disabled instead of deleted.
3237
user.actuallyDeleteAccount=false

src/test/java/com/digitalsanctuary/spring/user/gdpr/ConsentAuditServiceTest.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -15,7 +15,10 @@
1515
import org.mockito.ArgumentCaptor;
1616
import org.mockito.InjectMocks;
1717
import org.mockito.Mock;
18+
import org.mockito.Spy;
1819
import org.springframework.context.ApplicationEventPublisher;
20+
import tools.jackson.databind.ObjectMapper;
21+
import tools.jackson.databind.json.JsonMapper;
1922
import com.digitalsanctuary.spring.user.audit.AuditEvent;
2023
import com.digitalsanctuary.spring.user.audit.AuditLogQueryService;
2124
import com.digitalsanctuary.spring.user.event.ConsentChangedEvent;
@@ -38,6 +41,9 @@ class ConsentAuditServiceTest {
3841
@Mock
3942
private AuditLogQueryService auditLogQueryService;
4043

44+
@Spy
45+
private ObjectMapper objectMapper = JsonMapper.builder().build();
46+
4147
@Mock
4248
private HttpServletRequest request;
4349

src/test/java/com/digitalsanctuary/spring/user/gdpr/GdprExportServiceTest.java

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,10 @@
1616
import org.mockito.ArgumentCaptor;
1717
import org.mockito.InjectMocks;
1818
import org.mockito.Mock;
19+
import org.mockito.Spy;
1920
import org.springframework.context.ApplicationEventPublisher;
21+
import tools.jackson.databind.ObjectMapper;
22+
import tools.jackson.databind.json.JsonMapper;
2023
import com.digitalsanctuary.spring.user.audit.AuditEventDTO;
2124
import com.digitalsanctuary.spring.user.audit.AuditLogQueryService;
2225
import com.digitalsanctuary.spring.user.dto.GdprExportDTO;
@@ -48,6 +51,9 @@ class GdprExportServiceTest {
4851
@Mock
4952
private ApplicationEventPublisher eventPublisher;
5053

54+
@Spy
55+
private ObjectMapper objectMapper = JsonMapper.builder().build();
56+
5157
@InjectMocks
5258
private GdprExportService gdprExportService;
5359

0 commit comments

Comments
 (0)