From 215ab11b09b9886ba44dc4abd6fc4d3a7a69c1b6 Mon Sep 17 00:00:00 2001 From: FenjuFu Date: Sun, 30 Aug 2026 18:39:48 +0800 Subject: [PATCH] fix(audit): render audit detail JSON with Jackson instead of concatenation audit_log.detail_json is a JSONB column, so the value has to be valid JSON or the insert fails. It was built by string concatenation at every call site, with three inconsistent levels of escaping: none at all in ClawHubCompatAppService, DeviceAuthWebController, LabelAdminAppService and SkillLabelAppService; quotes only in ReviewPortalAppService, SkillLifecycleAppService, NamespaceGovernanceService and SkillGovernanceService; quotes and backslashes in PromotionPortalAppService.escapeJson. None of the three escapes control characters, which JSON forbids raw inside a string. A reviewer pressing Enter in a review comment therefore produced a payload PostgreSQL rejects, and because the audit write happens after the domain mutation, the review was already approved when the request returned 500. Add AuditDetail, which renders the payload through Jackson, and route all 29 construction sites through it. 17 of those interpolate a string value and are the actual defect surface; the numeric and constant ones are converted too so there is one way to build audit detail and no hand-rolled example left to copy. SkillHardDeleteService.toAuditPayload already did this correctly with a LinkedHashMap and an ObjectMapper; AuditDetail is that shape extracted. The service itself is left alone rather than changing its constructor signature for no behavior gain. Output is byte-identical for values that were already escaped correctly, so the existing exact-string assertions in AdminSearchControllerTest and PromotionPortalAppServiceTest are unchanged. null still means "no detail": the builder returns null rather than {} when no field is set. Addresses the JSON half of #615. The transaction half -- the domain mutation and the audit write not sharing one transaction -- is a separate design decision about whether an audit failure should roll back a review, and is not bundled here. Signed-off-by: FenjuFu --- .../compat/ClawHubCompatAppService.java | 5 +- .../controller/DeviceAuthWebController.java | 3 +- .../admin/AdminSearchController.java | 3 +- .../service/LabelAdminAppService.java | 9 +- .../service/PromotionPortalAppService.java | 26 +--- .../service/ReviewPortalAppService.java | 7 +- .../service/SkillLabelAppService.java | 5 +- .../service/SkillLifecycleAppService.java | 10 +- .../PromotionPortalAppServiceTest.java | 31 ++++ .../skillhub/domain/audit/AuditDetail.java | 86 ++++++++++ .../namespace/NamespaceGovernanceService.java | 3 +- .../domain/report/SkillReportService.java | 7 +- .../domain/review/PromotionService.java | 5 +- .../skillhub/domain/review/ReviewService.java | 5 +- .../skill/service/SkillGovernanceService.java | 5 +- .../domain/audit/AuditDetailTest.java | 147 ++++++++++++++++++ 16 files changed, 309 insertions(+), 48 deletions(-) create mode 100644 server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/audit/AuditDetail.java create mode 100644 server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/audit/AuditDetailTest.java diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubCompatAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubCompatAppService.java index 801b2ed2..4c8c9491 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubCompatAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/compat/ClawHubCompatAppService.java @@ -12,6 +12,7 @@ import com.iflytek.skillhub.compat.dto.ClawHubUnstarResponse; import com.iflytek.skillhub.compat.dto.ClawHubWhoamiResponse; import com.iflytek.skillhub.controller.support.MultipartPackageExtractor; import com.iflytek.skillhub.controller.support.ZipPackageExtractor; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.audit.AuditLogService; import com.iflytek.skillhub.domain.namespace.NamespaceRole; import com.iflytek.skillhub.domain.shared.exception.DomainNotFoundException; @@ -345,7 +346,7 @@ public class ClawHubCompatAppService { confirmWarnings ); recordCompatPublishAudit(principal.userId(), result.version().getId(), clientIp, userAgent, - "{\"namespace\":\"" + namespace + "\",\"slug\":\"" + extracted.payload().slug() + "\"}"); + AuditDetail.of("namespace", namespace, "slug", extracted.payload().slug())); return new ClawHubPublishResponse(result.skillId().toString(), result.version().getId().toString()); } @@ -364,7 +365,7 @@ public class ClawHubCompatAppService { confirmWarnings ); recordCompatPublishAudit(principal.userId(), result.version().getId(), clientIp, userAgent, - "{\"namespace\":\"" + namespace + "\"}"); + AuditDetail.of("namespace", namespace)); return new ClawHubPublishResponse(result.skillId().toString(), result.version().getId().toString()); } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/DeviceAuthWebController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/DeviceAuthWebController.java index 71f27a19..44dd64d2 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/DeviceAuthWebController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/DeviceAuthWebController.java @@ -2,6 +2,7 @@ package com.iflytek.skillhub.controller; import com.iflytek.skillhub.auth.device.DeviceAuthService; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.audit.AuditLogService; import com.iflytek.skillhub.dto.ApiResponse; import com.iflytek.skillhub.dto.ApiResponseFactory; @@ -51,7 +52,7 @@ public class DeviceAuthWebController extends BaseApiController { requestIdAccessor.current(), httpRequest.getRemoteAddr(), httpRequest.getHeader("User-Agent"), - "{\"userCode\":\"" + request.userCode() + "\"}" + AuditDetail.of("userCode", request.userCode()) ); return ok("response.success.updated", new MessageResponse("Device authorized successfully")); } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AdminSearchController.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AdminSearchController.java index 87770644..899d80ad 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AdminSearchController.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/controller/admin/AdminSearchController.java @@ -2,6 +2,7 @@ package com.iflytek.skillhub.controller.admin; import com.iflytek.skillhub.auth.rbac.PlatformPrincipal; import com.iflytek.skillhub.controller.BaseApiController; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.dto.ApiResponse; import com.iflytek.skillhub.dto.ApiResponseFactory; import com.iflytek.skillhub.domain.audit.AuditLogService; @@ -48,7 +49,7 @@ public class AdminSearchController extends BaseApiController { requestIdAccessor.current(), httpRequest.getRemoteAddr(), httpRequest.getHeader("User-Agent"), - "{\"scope\":\"ALL\"}" + AuditDetail.of("scope", "ALL") ); return ok("response.success.updated", null); } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/LabelAdminAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/LabelAdminAppService.java index 88316c50..8b69b1fe 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/LabelAdminAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/LabelAdminAppService.java @@ -1,6 +1,7 @@ package com.iflytek.skillhub.service; import com.iflytek.skillhub.auth.rbac.RbacService; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.audit.AuditLogService; import com.iflytek.skillhub.domain.label.LabelDefinition; import com.iflytek.skillhub.domain.label.LabelDefinitionService; @@ -62,7 +63,7 @@ public class LabelAdminAppService { userId, platformRoles(userId) ); - recordAudit("LABEL_CREATE", userId, labelDefinition.getId(), auditContext, "{\"slug\":\"" + labelDefinition.getSlug() + "\"}"); + recordAudit("LABEL_CREATE", userId, labelDefinition.getId(), auditContext, AuditDetail.of("slug", labelDefinition.getSlug())); return toResponse(labelDefinition); } @@ -87,7 +88,7 @@ public class LabelAdminAppService { if (!affectedSkillIds.isEmpty()) { afterCommit(() -> labelSearchSyncService.rebuildSkills(affectedSkillIds)); } - recordAudit("LABEL_UPDATE", userId, updated.getId(), auditContext, "{\"slug\":\"" + updated.getSlug() + "\"}"); + recordAudit("LABEL_UPDATE", userId, updated.getId(), auditContext, AuditDetail.of("slug", updated.getSlug())); return toResponse(updated); } @@ -102,7 +103,7 @@ public class LabelAdminAppService { if (!affectedSkillIds.isEmpty()) { afterCommit(() -> labelSearchSyncService.rebuildSkills(affectedSkillIds)); } - recordAudit("LABEL_DELETE", userId, existing.getId(), auditContext, "{\"slug\":\"" + slug + "\"}"); + recordAudit("LABEL_DELETE", userId, existing.getId(), auditContext, AuditDetail.of("slug", slug)); } @Transactional @@ -118,7 +119,7 @@ public class LabelAdminAppService { List responses = labelDefinitionService.updateSortOrders(updates, platformRoles(userId)).stream() .map(this::toResponse) .toList(); - recordAudit("LABEL_SORT_ORDER_UPDATE", userId, null, auditContext, "{\"count\":" + request.items().size() + "}"); + recordAudit("LABEL_SORT_ORDER_UPDATE", userId, null, auditContext, AuditDetail.of("count", request.items().size())); return responses; } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/PromotionPortalAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/PromotionPortalAppService.java index 1c7f6b44..be6fbb47 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/PromotionPortalAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/PromotionPortalAppService.java @@ -1,6 +1,7 @@ package com.iflytek.skillhub.service; import com.iflytek.skillhub.auth.rbac.RbacService; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.audit.AuditLogService; import com.iflytek.skillhub.domain.namespace.NamespaceRole; import com.iflytek.skillhub.domain.review.PromotionRequest; @@ -67,7 +68,7 @@ public class PromotionPortalAppService { userId, promotion.getId(), auditContext, - "{\"sourceSkillId\":" + sourceSkillId + ",\"sourceVersionId\":" + sourceVersionId + "}" + AuditDetail.of("sourceSkillId", sourceSkillId, "sourceVersionId", sourceVersionId) ); return governanceQueryRepository.getPromotionResponse(promotion); } @@ -247,24 +248,9 @@ public class PromotionPortalAppService { private String detailWithComment(String comment, boolean selfReview) { boolean hasComment = comment != null && !comment.isBlank(); - if (!hasComment && !selfReview) { - return null; - } - StringBuilder detail = new StringBuilder("{"); - if (hasComment) { - detail.append("\"comment\":\"").append(escapeJson(comment)).append("\""); - } - if (selfReview) { - if (hasComment) { - detail.append(","); - } - detail.append("\"selfReview\":true"); - } - detail.append("}"); - return detail.toString(); - } - - private String escapeJson(String value) { - return value.replace("\\", "\\\\").replace("\"", "\\\""); + return AuditDetail.builder() + .put("comment", hasComment ? comment : null) + .put("selfReview", selfReview ? Boolean.TRUE : null) + .build(); } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/ReviewPortalAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/ReviewPortalAppService.java index 96663c9e..b05b9b19 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/ReviewPortalAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/ReviewPortalAppService.java @@ -1,6 +1,7 @@ package com.iflytek.skillhub.service; import com.iflytek.skillhub.auth.rbac.RbacService; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.audit.AuditLogService; import com.iflytek.skillhub.domain.namespace.Namespace; import com.iflytek.skillhub.domain.namespace.NamespaceRepository; @@ -62,7 +63,7 @@ public class ReviewPortalAppService { normalizeRoles(userNsRoles), platformRoles(userId) ); - recordAudit("REVIEW_SUBMIT", userId, task.getId(), auditContext, "{\"skillVersionId\":" + skillVersionId + "}"); + recordAudit("REVIEW_SUBMIT", userId, task.getId(), auditContext, AuditDetail.of("skillVersionId", skillVersionId)); return governanceQueryRepository.getReviewTaskResponse(task); } @@ -109,7 +110,7 @@ public class ReviewPortalAppService { userId, reviewTaskId, auditContext, - "{\"skillVersionId\":" + task.getSkillVersionId() + "}" + AuditDetail.of("skillVersionId", task.getSkillVersionId()) ); } @@ -270,6 +271,6 @@ public class ReviewPortalAppService { if (comment == null || comment.isBlank()) { return null; } - return "{\"comment\":\"" + comment.replace("\"", "\\\"") + "\"}"; + return AuditDetail.of("comment", comment); } } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillLabelAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillLabelAppService.java index 5947934e..86a16948 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillLabelAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillLabelAppService.java @@ -1,6 +1,7 @@ package com.iflytek.skillhub.service; import com.iflytek.skillhub.auth.rbac.RbacService; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.audit.AuditLogService; import com.iflytek.skillhub.domain.label.LabelDefinition; import com.iflytek.skillhub.domain.label.LabelDefinitionService; @@ -96,7 +97,7 @@ public class SkillLabelAppService { platformRoles(userId) ); afterCommit(() -> labelSearchSyncService.rebuildSkill(skill.getId())); - recordAudit("SKILL_LABEL_ATTACH", userId, skill.getId(), auditContext, "{\"labelSlug\":\"" + labelSlug + "\"}"); + recordAudit("SKILL_LABEL_ATTACH", userId, skill.getId(), auditContext, AuditDetail.of("labelSlug", labelSlug)); return toDtos(List.of(attached)).getFirst(); } @@ -116,7 +117,7 @@ public class SkillLabelAppService { platformRoles(userId) ); afterCommit(() -> labelSearchSyncService.rebuildSkill(skill.getId())); - recordAudit("SKILL_LABEL_DETACH", userId, skill.getId(), auditContext, "{\"labelSlug\":\"" + labelSlug + "\"}"); + recordAudit("SKILL_LABEL_DETACH", userId, skill.getId(), auditContext, AuditDetail.of("labelSlug", labelSlug)); return new MessageResponse("Label detached"); } diff --git a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillLifecycleAppService.java b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillLifecycleAppService.java index 3e2119d9..4aa57b8d 100644 --- a/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillLifecycleAppService.java +++ b/server/skillhub-app/src/main/java/com/iflytek/skillhub/service/SkillLifecycleAppService.java @@ -1,5 +1,6 @@ package com.iflytek.skillhub.service; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.audit.AuditLogService; import com.iflytek.skillhub.domain.namespace.Namespace; import com.iflytek.skillhub.domain.namespace.NamespaceRepository; @@ -128,7 +129,7 @@ public class SkillLifecycleAppService { null, auditContext.clientIp(), auditContext.userAgent(), - "{\"version\":\"" + version.replace("\"", "\\\"") + "\"}" + AuditDetail.of("version", version) ); return new SkillLifecycleMutationResponse( skill.getId(), @@ -165,8 +166,7 @@ public class SkillLifecycleAppService { null, auditContext.clientIp(), auditContext.userAgent(), - "{\"sourceVersion\":\"" + version.replace("\"", "\\\"") - + "\",\"targetVersion\":\"" + targetVersion.replace("\"", "\\\"") + "\"}" + AuditDetail.of("sourceVersion", version, "targetVersion", targetVersion) ); return new SkillLifecycleMutationResponse( result.skillId(), @@ -201,7 +201,7 @@ public class SkillLifecycleAppService { null, auditContext.clientIp(), auditContext.userAgent(), - "{\"version\":\"" + version.replace("\"", "\\\"") + "\",\"targetVisibility\":\"" + targetVisibility + "\"}" + AuditDetail.of("version", version, "targetVisibility", targetVisibility) ); return new SkillLifecycleMutationResponse( skill.getId(), @@ -234,7 +234,7 @@ public class SkillLifecycleAppService { null, auditContext.clientIp(), auditContext.userAgent(), - "{\"version\":\"" + version.replace("\"", "\\\"") + "\"}" + AuditDetail.of("version", version) ); return new SkillLifecycleMutationResponse( skill.getId(), diff --git a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/PromotionPortalAppServiceTest.java b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/PromotionPortalAppServiceTest.java index 9d8a5acc..b19ce879 100644 --- a/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/PromotionPortalAppServiceTest.java +++ b/server/skillhub-app/src/test/java/com/iflytek/skillhub/service/PromotionPortalAppServiceTest.java @@ -134,6 +134,37 @@ class PromotionPortalAppServiceTest { ); } + @Test + void approvePromotion_escapesMultiLineReviewCommentIntoValidJson() { + // Regression: the detail column is JSONB. A reviewer pressing Enter used + // to produce a raw newline inside the JSON string, so the audit insert + // failed after the promotion had already been approved. + String comment = "looks good\nbut rename it \"foo\"\tfirst\\done"; + PromotionRequest promotion = promotionRequest(PROMOTION_ID, SUBMITTER_ID); + when(rbacService.getUserRoleCodes(REVIEWER_ID)).thenReturn(Set.of("SKILL_ADMIN")); + when(promotionService.approvePromotion(PROMOTION_ID, REVIEWER_ID, comment, Set.of("SKILL_ADMIN"))) + .thenReturn(promotion); + when(governanceQueryRepository.getPromotionResponse(promotion)).thenReturn(response(promotion)); + + service.approvePromotion( + PROMOTION_ID, + comment, + REVIEWER_ID, + new AuditRequestContext("127.0.0.1", "JUnit") + ); + + verify(auditLogService).record( + eq(REVIEWER_ID), + eq("PROMOTION_APPROVE"), + eq("PROMOTION_REQUEST"), + eq(PROMOTION_ID), + eq(null), + eq("127.0.0.1"), + eq("JUnit"), + eq("{\"comment\":\"looks good\\nbut rename it \\\"foo\\\"\\tfirst\\\\done\"}") + ); + } + private PromotionResponseDto response(PromotionRequest request) { return new PromotionResponseDto( request.getId(), diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/audit/AuditDetail.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/audit/AuditDetail.java new file mode 100644 index 00000000..69c6dac1 --- /dev/null +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/audit/AuditDetail.java @@ -0,0 +1,86 @@ +package com.iflytek.skillhub.domain.audit; + +import com.fasterxml.jackson.core.JsonProcessingException; +import com.fasterxml.jackson.databind.ObjectMapper; + +import java.util.LinkedHashMap; +import java.util.Map; + +/** + * Builds the JSON stored in {@code audit_log.detail_json}. + * + *

That column is PostgreSQL {@code JSONB}, so the value has to be valid JSON or the + * insert fails. Hand-concatenating it does not survive real input: a value containing a + * newline, tab, backslash, or any other control character produces a string PostgreSQL + * rejects, and the audit write then fails after the domain mutation has already + * committed. Escaping only {@code "} — or only {@code "} and {@code \} — is not enough. + * + *

Every audit detail payload should be built here so there is exactly one place where + * that escaping is decided. + * + *

The mapper is a private static instance rather than the injected application bean on + * purpose: audit records are a stored format, and they should not change shape because + * someone reconfigures Jackson elsewhere in the application. + */ +public final class AuditDetail { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + + private AuditDetail() { + } + + /** + * Renders a single-field detail payload, e.g. {@code {"slug":"my-skill"}}. + */ + public static String of(String key, Object value) { + return builder().put(key, value).build(); + } + + /** + * Renders a two-field detail payload, preserving argument order. + */ + public static String of(String key1, Object value1, String key2, Object value2) { + return builder().put(key1, value1).put(key2, value2).build(); + } + + public static Builder builder() { + return new Builder(); + } + + /** + * Accumulates fields in insertion order. Use it when a field is conditional. + */ + public static final class Builder { + + private final Map fields = new LinkedHashMap<>(); + + private Builder() { + } + + /** + * Adds a field. A {@code null} value is skipped, so an optional field can be + * offered unconditionally. + */ + public Builder put(String key, Object value) { + if (value != null) { + fields.put(key, value); + } + return this; + } + + /** + * Returns the rendered JSON, or {@code null} when no field was set — the audit + * log stores {@code null} rather than an empty object for "no detail". + */ + public String build() { + if (fields.isEmpty()) { + return null; + } + try { + return MAPPER.writeValueAsString(fields); + } catch (JsonProcessingException e) { + throw new IllegalStateException("Failed to serialize audit detail JSON", e); + } + } + } +} diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/namespace/NamespaceGovernanceService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/namespace/NamespaceGovernanceService.java index 4809a264..984fe4fc 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/namespace/NamespaceGovernanceService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/namespace/NamespaceGovernanceService.java @@ -1,5 +1,6 @@ package com.iflytek.skillhub.domain.namespace; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.audit.AuditLogService; import com.iflytek.skillhub.domain.shared.exception.DomainBadRequestException; import com.iflytek.skillhub.domain.shared.exception.DomainForbiddenException; @@ -206,7 +207,7 @@ public class NamespaceGovernanceService { requestId, clientIp, userAgent, - reason == null || reason.isBlank() ? null : "{\"reason\":\"" + reason.replace("\"", "\\\"") + "\"}" + reason == null || reason.isBlank() ? null : AuditDetail.of("reason", reason) ); } } diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/report/SkillReportService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/report/SkillReportService.java index 3a9b82b1..a94d43c7 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/report/SkillReportService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/report/SkillReportService.java @@ -1,5 +1,6 @@ package com.iflytek.skillhub.domain.report; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.audit.AuditLogService; import com.iflytek.skillhub.domain.event.ReportResolvedEvent; import com.iflytek.skillhub.domain.event.ReportSubmittedEvent; @@ -78,7 +79,7 @@ public class SkillReportService { normalize(details) )); auditLogService.record(reporterId, "REPORT_SKILL", "SKILL", skillId, null, clientIp, userAgent, - "{\"reportId\":" + saved.getId() + "}"); + AuditDetail.of("reportId", saved.getId())); eventPublisher.publishEvent(new ReportSubmittedEvent( saved.getId(), saved.getSkillId(), saved.getReporterId())); return saved; @@ -120,7 +121,7 @@ public class SkillReportService { "SKILL_REPORT", reportId, "Report handled", - "{\"status\":\"RESOLVED\"}" + AuditDetail.of("status", "RESOLVED") ); return saved; } @@ -146,7 +147,7 @@ public class SkillReportService { "SKILL_REPORT", reportId, "Report dismissed", - "{\"status\":\"DISMISSED\"}" + AuditDetail.of("status", "DISMISSED") ); return saved; } diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/PromotionService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/PromotionService.java index 1a1429d0..97751186 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/PromotionService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/PromotionService.java @@ -1,5 +1,6 @@ package com.iflytek.skillhub.domain.review; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.event.PromotionApprovedEvent; import com.iflytek.skillhub.domain.event.PromotionRejectedEvent; import com.iflytek.skillhub.domain.event.PromotionSubmittedEvent; @@ -266,7 +267,7 @@ public class PromotionService { "PROMOTION_REQUEST", promotionId, "Promotion approved", - "{\"status\":\"APPROVED\"}" + AuditDetail.of("status", "APPROVED") ); return savedRequest; @@ -323,7 +324,7 @@ public class PromotionService { "PROMOTION_REQUEST", promotionId, "Promotion rejected", - "{\"status\":\"REJECTED\"}" + AuditDetail.of("status", "REJECTED") ); return request; diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java index 56ea7884..18275f3e 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/review/ReviewService.java @@ -1,6 +1,7 @@ package com.iflytek.skillhub.domain.review; import com.fasterxml.jackson.databind.ObjectMapper; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.namespace.Namespace; import com.iflytek.skillhub.domain.namespace.NamespaceRepository; import com.iflytek.skillhub.domain.namespace.NamespaceRole; @@ -241,7 +242,7 @@ public class ReviewService { "REVIEW_TASK", reviewTaskId, "Review approved", - "{\"status\":\"APPROVED\"}" + AuditDetail.of("status", "APPROVED") ); return task; @@ -294,7 +295,7 @@ public class ReviewService { "REVIEW_TASK", reviewTaskId, "Review rejected", - "{\"status\":\"REJECTED\"}" + AuditDetail.of("status", "REJECTED") ); return task; diff --git a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceService.java b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceService.java index 552d9e1f..e4bfc81c 100644 --- a/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceService.java +++ b/server/skillhub-domain/src/main/java/com/iflytek/skillhub/domain/skill/service/SkillGovernanceService.java @@ -1,5 +1,6 @@ package com.iflytek.skillhub.domain.skill.service; +import com.iflytek.skillhub.domain.audit.AuditDetail; import com.iflytek.skillhub.domain.audit.AuditLogService; import com.iflytek.skillhub.domain.event.SkillStatusChangedEvent; import com.iflytek.skillhub.domain.namespace.NamespaceRole; @@ -205,7 +206,7 @@ public class SkillGovernanceService { null, clientIp, userAgent, - "{\"version\":\"" + version.getVersion().replace("\"", "\\\"") + "\"}" + AuditDetail.of("version", version.getVersion()) ); } @@ -311,7 +312,7 @@ public class SkillGovernanceService { if (reason == null || reason.isBlank()) { return null; } - return "{\"reason\":\"" + reason.replace("\"", "\\\"") + "\"}"; + return AuditDetail.of("reason", reason); } private Instant currentInstant() { diff --git a/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/audit/AuditDetailTest.java b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/audit/AuditDetailTest.java new file mode 100644 index 00000000..950de0c6 --- /dev/null +++ b/server/skillhub-domain/src/test/java/com/iflytek/skillhub/domain/audit/AuditDetailTest.java @@ -0,0 +1,147 @@ +package com.iflytek.skillhub.domain.audit; + +import com.fasterxml.jackson.databind.JsonNode; +import com.fasterxml.jackson.databind.ObjectMapper; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.ValueSource; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatCode; + +class AuditDetailTest { + + private static final ObjectMapper MAPPER = new ObjectMapper(); + + // ------------------------------------------------------------------ + // Shape — the rendered JSON must match what callers wrote by hand + // ------------------------------------------------------------------ + + @Test + void of_rendersSingleField() { + assertThat(AuditDetail.of("slug", "my-skill")).isEqualTo("{\"slug\":\"my-skill\"}"); + } + + @Test + void of_preservesArgumentOrderForTwoFields() { + assertThat(AuditDetail.of("version", "1.0.0", "targetVisibility", "PUBLIC")) + .isEqualTo("{\"version\":\"1.0.0\",\"targetVisibility\":\"PUBLIC\"}"); + } + + @Test + void of_keepsNumbersAndBooleansUnquoted() { + assertThat(AuditDetail.of("count", 3)).isEqualTo("{\"count\":3}"); + assertThat(AuditDetail.of("reportId", 42L)).isEqualTo("{\"reportId\":42}"); + assertThat(AuditDetail.of("selfReview", Boolean.TRUE)).isEqualTo("{\"selfReview\":true}"); + } + + @Test + void builder_preservesInsertionOrder() { + assertThat(AuditDetail.builder() + .put("comment", "ship") + .put("selfReview", Boolean.TRUE) + .build()) + .isEqualTo("{\"comment\":\"ship\",\"selfReview\":true}"); + } + + // ------------------------------------------------------------------ + // Escaping — the regression this class exists for + // ------------------------------------------------------------------ + + /** + * Every one of these breaks a hand-rolled builder. The audit column is JSONB, + * so an unescaped control character makes the insert fail after the domain + * mutation has already committed. + */ + @ParameterizedTest + @ValueSource(strings = { + "line one\nline two", + "carriage\rreturn", + "tab\tseparated", + "back\\slash", + "quote \" inside", + "both \\ and \"", + "form\ffeed", + "backspace\bhere", + "中文评审意见\n第二行", + "emoji 🚀 and \"quotes\"", + }) + void escapedValuesRoundTripAsValidJson(String raw) throws Exception { + String json = AuditDetail.of("comment", raw); + + JsonNode parsed = MAPPER.readTree(json); + assertThat(parsed.get("comment").asText()).isEqualTo(raw); + } + + /** + * Built at runtime rather than in the annotation: a raw control character in + * a source literal is fragile, and these are exactly the bytes JSON forbids + * unescaped inside a string. + */ + @Test + void lowControlCharactersRoundTripAsValidJson() throws Exception { + for (char c = 0; c < 0x20; c++) { + String raw = "before" + c + "after"; + String json = AuditDetail.of("comment", raw); + + JsonNode parsed = MAPPER.readTree(json); + assertThat(parsed.get("comment").asText()) + .as("control character U+%04X", (int) c) + .isEqualTo(raw); + } + } + + @Test + void newlineIsEscapedRatherThanEmbeddedRaw() { + // The literal two-character sequence backslash-n, not a raw 0x0A. + assertThat(AuditDetail.of("comment", "a\nb")).isEqualTo("{\"comment\":\"a\\nb\"}"); + } + + @Test + void multiFieldPayloadWithControlCharactersStaysParseable() throws Exception { + String json = AuditDetail.of("sourceVersion", "1.0.0\n", "targetVersion", "2.0.0\t\"x\""); + + JsonNode parsed = MAPPER.readTree(json); + assertThat(parsed.get("sourceVersion").asText()).isEqualTo("1.0.0\n"); + assertThat(parsed.get("targetVersion").asText()).isEqualTo("2.0.0\t\"x\""); + } + + @Test + void everyAwkwardCharacterAtOnceStillSerializes() throws Exception { + // Assembled from char casts: a \\uXXXX escape is expanded by the Java + // lexer before parsing, which would put the raw byte back in the literal. + String awkward = "" + (char) 0x00 + (char) 0x1F + "\\\"" + "\n\r\t" + "中文"; + + assertThatCode(() -> AuditDetail.of("reason", awkward)).doesNotThrowAnyException(); + assertThat(MAPPER.readTree(AuditDetail.of("reason", awkward)).get("reason").asText()) + .isEqualTo(awkward); + } + + // ------------------------------------------------------------------ + // Null handling — "no detail" must stay null, not become "{}" + // ------------------------------------------------------------------ + + @Test + void nullValueIsSkipped() { + assertThat(AuditDetail.of("comment", null, "selfReview", Boolean.TRUE)) + .isEqualTo("{\"selfReview\":true}"); + } + + @Test + void allNullValuesProduceNull() { + assertThat(AuditDetail.of("comment", null)).isNull(); + assertThat(AuditDetail.of("comment", null, "selfReview", null)).isNull(); + } + + @Test + void emptyBuilderProducesNull() { + assertThat(AuditDetail.builder().build()).isNull(); + } + + @Test + void emptyStringIsStillARecordedValue() { + // Only null is treated as absent; callers that want to drop blanks keep + // their own isBlank() guard. + assertThat(AuditDetail.of("comment", "")).isEqualTo("{\"comment\":\"\"}"); + } +}