Conversation
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| "bulk set expiration", (ctx) -> { | ||
| Map<String, Object> modification = buildModification( | ||
| "MANUAL_DURATION_CHANGE", ctx.now, performerUsername, reason, effectiveDuration); |
There was a problem hiding this comment.
newDurationMs is zero or negative
Map.of() does not permit null values and will throw NullPointerException at runtime whenever newDurationMs <= 0, because effectiveDuration is set to null in that branch and then passed directly to Map.of("duration", effectiveDuration). The controller only guards against null, not against non-positive values, so a caller sending newDurationMs: 0 will trigger this crash.
| "bulk set expiration", (ctx) -> { | |
| Map<String, Object> modification = buildModification( | |
| "MANUAL_DURATION_CHANGE", ctx.now, performerUsername, reason, effectiveDuration); | |
| auditRepository.appendPunishmentModificationWithData( | |
| server, ctx.playerId, ctx.punishmentId, modification, | |
| effectiveDuration != null ? Map.of("duration", effectiveDuration) : Map.of()); |
|
|
||
| Object legacyEmail = request.formData().containsKey("contact_email") | ||
| ? request.formData().get("contact_email") | ||
| : request.formData().get("email"); |
There was a problem hiding this comment.
ClassCastException on non-string input
x.get("content") returns Object. If a caller sends JSON like {"content": 123}, Jackson deserialises it as an Integer inside the Map<String, Object>, and the hard cast (String) throws ClassCastException, resulting in an unhandled 500 instead of a proper validation error.
| Object legacyEmail = request.formData().containsKey("contact_email") | |
| ? request.formData().get("contact_email") | |
| : request.formData().get("email"); | |
| Object rawContent = x.get("content"); | |
| String content = rawContent instanceof String s ? s : (rawContent != null ? rawContent.toString() : null); |
| public int bulkPardonByType( | ||
| Server server, List<Integer> typeOrdinals, String reason, String performerUsername) { | ||
| return processBulkPunishmentAction(server, typeOrdinals, reason, performerUsername, | ||
| "bulk pardon", (ctx) -> { | ||
| if (AuditDocumentUtil.hasModificationType(ctx.punishmentDoc, "MANUAL_PARDON", "APPEAL_ACCEPT", "SYSTEM_PARDON")) { | ||
| return false; |
There was a problem hiding this comment.
PunishmentModificationType enum
The rest of the PR uses PunishmentModificationType.XYZ.name() (e.g. in StaffPerformanceService and AuditDocumentUtil calls), but this pardon check passes bare strings. If an enum value is ever renamed the check silently stops working.
| public int bulkPardonByType( | |
| Server server, List<Integer> typeOrdinals, String reason, String performerUsername) { | |
| return processBulkPunishmentAction(server, typeOrdinals, reason, performerUsername, | |
| "bulk pardon", (ctx) -> { | |
| if (AuditDocumentUtil.hasModificationType(ctx.punishmentDoc, "MANUAL_PARDON", "APPEAL_ACCEPT", "SYSTEM_PARDON")) { | |
| return false; | |
| if (AuditDocumentUtil.hasModificationType(ctx.punishmentDoc, | |
| PunishmentModificationType.MANUAL_PARDON.name(), | |
| PunishmentModificationType.APPEAL_ACCEPT.name(), | |
| PunishmentModificationType.SYSTEM_PARDON.name())) { |
| issuerMatch.add(Criteria.where(PunishmentFields.ISSUER_ID).is(staffId)); | ||
| } | ||
|
|
||
| Query query = Query.query(MongoQueries.where(PlayerFields.PUNISHMENTS).elemMatch( | ||
| Query query = Query.query(Criteria.where(PlayerFields.PUNISHMENTS).elemMatch( | ||
| new Criteria().orOperator(issuerMatch.toArray(new Criteria[0])) | ||
| )); | ||
| query.fields().include(PlayerFields.ID, PlayerFields.MINECRAFT_UUID, PlayerFields.PUNISHMENTS); | ||
|
|
||
| return tenantMongoAccess.forServer(server).find(query, Document.class, CollectionName.PLAYERS); |
There was a problem hiding this comment.
findPlayersForBulkAction fetches complete player documents with no field projection
For popular punishment types this query can return thousands of full player records into application memory at once. Only _id, usernames, and punishments are actually consumed. Adding a projection avoids loading full documents for every matched player.
| issuerMatch.add(Criteria.where(PunishmentFields.ISSUER_ID).is(staffId)); | |
| } | |
| Query query = Query.query(MongoQueries.where(PlayerFields.PUNISHMENTS).elemMatch( | |
| Query query = Query.query(Criteria.where(PlayerFields.PUNISHMENTS).elemMatch( | |
| new Criteria().orOperator(issuerMatch.toArray(new Criteria[0])) | |
| )); | |
| query.fields().include(PlayerFields.ID, PlayerFields.MINECRAFT_UUID, PlayerFields.PUNISHMENTS); | |
| return tenantMongoAccess.forServer(server).find(query, Document.class, CollectionName.PLAYERS); | |
| public List<Document> findPlayersForBulkAction(Server server, List<Integer> typeOrdinals) { | |
| Query query = Query.query(Criteria.where(PlayerFields.PUNISHMENTS).elemMatch( | |
| Criteria.where(PunishmentFields.TYPE_ORDINAL).in(typeOrdinals) | |
| )); | |
| query.fields() | |
| .include(PlayerFields.ID, PlayerFields.MINECRAFT_UUID, | |
| PlayerFields.USERNAMES, PlayerFields.PUNISHMENTS); | |
| return tenantMongoAccess.forServer(server).find(query, Document.class, CollectionName.PLAYERS); | |
| } |
Greptile SummaryThis large PR (v2.2.0-rc.1, ~330 files) is primarily a package reorganization moving classes to Confidence Score: 4/5Safe to merge after fixing the raw string literal issues in the two new bulk-action methods. Two P1 findings exist in the newly added bulkPardonByType and bulkSetExpirationByType methods: raw string literals are used where the PunishmentModificationType enum constants (introduced in this same PR) should be referenced. This means if enum values are renamed, the already-pardoned detection silently breaks and duplicate pardons become possible. The remaining findings are P2 (missing max constraint on typeOrdinals and potential NPE in storage aggregation). The large-scale package reorganization and service extractions are cleanly done. src/main/java/gg/modl/backend/audit/service/AuditService.java (P1 raw strings in bulk action methods); src/main/java/gg/modl/backend/audit/dto/request/BulkPunishmentActionRequest.java (missing max size); src/main/java/gg/modl/backend/database/mongo/repository/StorageFileMongoRepository.java (NPE potential) Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
A[POST /punishments/bulk-pardon\nor /bulk-set-expiration] --> B[AuditController]
B --> C{requireSuperAdmin?}
C -- No --> D[403 Forbidden]
C -- Yes --> E[AuditService.bulkPardonByType\nor bulkSetExpirationByType]
E --> F[processBulkPunishmentAction]
F --> G[findPlayersForBulkAction\nAuditMongoRepository]
G --> H[Iterate Players]
H --> I[Iterate Punishments]
I --> J{typeOrdinal\nin typeOrdinals?}
J -- No --> I
J -- Yes --> K{isPunishmentActive?}
K -- No --> I
K -- Yes --> L{BulkPunishmentAction.apply}
L -- Pardon --> M[appendPunishmentModification\nstatus=Pardoned\ncascadeLinkedBans if altBlocking]
L -- SetExpiration --> N[appendPunishmentModification\nduration=newDurationMs]
M --> O[saveAuditLog]
N --> O
O --> I
I --> P[Return count]
Greploops — Automatically fix all review issues by running Reviews (2): Last reviewed commit: "Merge branch 'main' into dev" | Re-trigger Greptile |
2 participants
No description provided.