diff --git a/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultStorage.java b/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultStorage.java index d27cf8fc..dccd7ec3 100644 --- a/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultStorage.java +++ b/mateclaw-server/src/main/java/vip/mate/agent/graph/executor/ToolResultStorage.java @@ -5,6 +5,7 @@ import lombok.extern.slf4j.Slf4j; import org.springframework.ai.chat.messages.ToolResponseMessage; import vip.mate.agent.context.StructuredTruncator; import vip.mate.tool.guard.WorkspacePathGuard; +import vip.mate.workspace.core.service.ChatUploadLocationResolver; import org.springframework.boot.context.properties.EnableConfigurationProperties; import org.springframework.context.annotation.Configuration; import org.springframework.stereotype.Component; @@ -478,10 +479,14 @@ public class ToolResultStorage { return deleted; } - /** Strip path separators and reserved characters so user-supplied IDs cannot escape the directory. */ + /** + * Strip path separators and reserved characters so user-supplied IDs cannot + * escape the directory. Delegates to the canonical conversation-id sanitizer + * so every id → path-segment mapping across the codebase stays byte-for-byte + * identical (see issue #507). + */ private static String sanitize(String s) { - if (s == null) return ""; - return s.replaceAll("[^A-Za-z0-9_.-]", "_"); + return ChatUploadLocationResolver.sanitizeSegment(s); } /** Test/admin helper: lexicographic ordering by length, descending. Not used at runtime. */ diff --git a/mateclaw-server/src/main/java/vip/mate/channel/ChannelMessageRouter.java b/mateclaw-server/src/main/java/vip/mate/channel/ChannelMessageRouter.java index d0790db7..397f6766 100644 --- a/mateclaw-server/src/main/java/vip/mate/channel/ChannelMessageRouter.java +++ b/mateclaw-server/src/main/java/vip/mate/channel/ChannelMessageRouter.java @@ -21,6 +21,7 @@ import vip.mate.memory.event.ConversationCompletionPublisher; import vip.mate.tts.TtsService; import vip.mate.workspace.conversation.ConversationService; import vip.mate.workspace.conversation.model.MessageContentPart; +import vip.mate.workspace.core.service.ChatUploadLocationResolver; import vip.mate.workspace.conversation.model.MessageEntity; import com.fasterxml.jackson.core.type.TypeReference; @@ -1539,16 +1540,18 @@ public class ChannelMessageRouter { */ private Path resolveVoiceReplyAudio(String conversationId, String fileName) { if (chatUploadLocationResolver != null) { - for (Path root : chatUploadLocationResolver.resolveCandidateUploadRoots(conversationId)) { - Path candidate = root.resolve(conversationId).resolve(fileName); + for (Path dir : chatUploadLocationResolver.resolveCandidateConversationDirs(conversationId)) { + Path candidate = dir.resolve(fileName); if (Files.exists(candidate)) { return candidate; } } } // Fallback to the legacy default dir when the resolver is absent - // (e.g. direct-construction unit tests). - Path legacy = Paths.get("data", "chat-uploads", conversationId, fileName); + // (e.g. direct-construction unit tests). Sanitize the id for the path + // segment so it matches the write side. + Path legacy = Paths.get("data", "chat-uploads", + ChatUploadLocationResolver.sanitizeSegment(conversationId), fileName); return Files.exists(legacy) ? legacy : null; } diff --git a/mateclaw-server/src/main/java/vip/mate/channel/feishu/FeishuChannelAdapter.java b/mateclaw-server/src/main/java/vip/mate/channel/feishu/FeishuChannelAdapter.java index 61f6d262..b79340ac 100644 --- a/mateclaw-server/src/main/java/vip/mate/channel/feishu/FeishuChannelAdapter.java +++ b/mateclaw-server/src/main/java/vip/mate/channel/feishu/FeishuChannelAdapter.java @@ -9,6 +9,7 @@ import reactor.core.publisher.Flux; import vip.mate.agent.AgentService.StreamDelta; import vip.mate.channel.AbstractChannelAdapter; import vip.mate.channel.ChannelMessage; +import vip.mate.workspace.core.service.ChatUploadLocationResolver; import vip.mate.channel.ChannelMessageRouter; import vip.mate.channel.ExponentialBackoff; import vip.mate.channel.StreamingChannelAdapter; @@ -271,6 +272,29 @@ public class FeishuChannelAdapter extends AbstractChannelAdapter implements Stre return roots; } + /** + * Candidate conversation attachment directories: the sanitized segment under + * each candidate root first, then — for backward compatibility with pre-fix + * Linux uploads that used the raw id verbatim — the raw-id dir when legal on + * this filesystem. Mirrors {@code ChatUploadLocationResolver + * .resolveCandidateConversationDirs} for the resolver-less fallback path. + */ + private java.util.List candidateChatUploadDirs(String conversationId) { + String safe = ChatUploadLocationResolver.sanitizeSegment(conversationId); + java.util.Set dirs = new java.util.LinkedHashSet<>(); + for (java.nio.file.Path root : candidateChatUploadRoots(conversationId)) { + dirs.add(root.resolve(safe)); + if (!safe.equals(conversationId)) { + try { + dirs.add(root.resolve(conversationId)); + } catch (java.nio.file.InvalidPathException ignore) { + // Raw id illegal on this filesystem (e.g. ':' on Windows). + } + } + } + return new java.util.ArrayList<>(dirs); + } + public FeishuChannelAdapter(ChannelEntity channelEntity, ChannelMessageRouter messageRouter, ObjectMapper objectMapper) { @@ -1775,8 +1799,11 @@ public class FeishuChannelAdapter extends AbstractChannelAdapter implements Stre : maybeDownloadResource(messageId, fileKey, type, fileName); if (dl == null) return null; - // Save under the workspace/agent-aware upload root ({convId}/ subdir) - Path uploadDir = chatUploadRootFor(conversationId).resolve(conversationId); + // Save under the workspace/agent-aware upload root ({convId}/ subdir). + // Sanitize the id for the path segment — IM ids like "feishu:xxx" + // carry a ':' that is illegal in a Windows filename. + Path uploadDir = chatUploadRootFor(conversationId) + .resolve(ChatUploadLocationResolver.sanitizeSegment(conversationId)); Files.createDirectories(uploadDir); String rawName = (dl.fileName() != null && !dl.fileName().isBlank()) ? dl.fileName() : fileKey; @@ -1869,8 +1896,8 @@ public class FeishuChannelAdapter extends AbstractChannelAdapter implements Stre private List loadRecentFilesFromDisk(String conversationId) { long cutoff = System.currentTimeMillis() - RECENT_FILE_TTL_MINUTES * 60_000L; List merged = new java.util.ArrayList<>(); - for (Path root : candidateChatUploadRoots(conversationId)) { - merged.addAll(loadRecentFilesFromDisk(root.resolve(conversationId), cutoff)); + for (Path dir : candidateChatUploadDirs(conversationId)) { + merged.addAll(loadRecentFilesFromDisk(dir, cutoff)); } return merged; } diff --git a/mateclaw-server/src/main/java/vip/mate/channel/web/ChatController.java b/mateclaw-server/src/main/java/vip/mate/channel/web/ChatController.java index 1492785d..e83e95bd 100644 --- a/mateclaw-server/src/main/java/vip/mate/channel/web/ChatController.java +++ b/mateclaw-server/src/main/java/vip/mate/channel/web/ChatController.java @@ -15,6 +15,7 @@ import org.springframework.web.bind.annotation.*; import org.springframework.web.multipart.MultipartFile; import org.springframework.web.servlet.mvc.method.annotation.SseEmitter; import vip.mate.common.result.R; +import vip.mate.workspace.core.service.ChatUploadLocationResolver; import vip.mate.agent.AgentService; import vip.mate.agent.model.AgentEntity; import vip.mate.approval.ApprovalWorkflowService; @@ -1106,7 +1107,10 @@ public class ChatController { String safeFilename = Path.of(originalFilename).getFileName().toString().replaceAll("[^a-zA-Z0-9._-]", "_"); String storedName = System.currentTimeMillis() + "_" + safeFilename; Path uploadRoot = uploadLocationResolver.resolveUploadRoot(conversationId); - Path conversationDir = uploadRoot.resolve(conversationId); + // Sanitize the id before using it as a path segment — IM-channel ids like + // "wecom:XXXX" carry a ':' that is illegal in a Windows filename and would + // throw InvalidPathException here. Reads use the same sanitization. + Path conversationDir = uploadRoot.resolve(ChatUploadLocationResolver.sanitizeSegment(conversationId)); Files.createDirectories(conversationDir); Path target = conversationDir.resolve(storedName); file.transferTo(target); @@ -1143,10 +1147,12 @@ public class ChatController { // current workspace-scoped ones, are both servable. Each candidate keeps // its own startsWith traversal guard. Path filePath = null; - for (Path root : uploadLocationResolver.resolveCandidateUploadRoots(conversationId)) { - Path conversationDir = root.resolve(conversationId).normalize(); - Path candidate = conversationDir.resolve(storedName).normalize(); - if (Files.exists(candidate) && candidate.startsWith(conversationDir)) { + // Sanitized-then-raw candidate dirs so both new writes (sanitized) and + // legacy Linux uploads (raw ':' dir) resolve. + for (Path conversationDir : uploadLocationResolver.resolveCandidateConversationDirs(conversationId)) { + Path normDir = conversationDir.normalize(); + Path candidate = normDir.resolve(storedName).normalize(); + if (Files.exists(candidate) && candidate.startsWith(normDir)) { filePath = candidate; break; } @@ -1556,7 +1562,7 @@ public class ChatController { * separators are normalized to {@code /} so the value is stable across OSes. */ static String toRelativeUploadPath(Path uploadRoot, String conversationId, String storedName) { - Path target = uploadRoot.resolve(conversationId).resolve(storedName); + Path target = uploadRoot.resolve(ChatUploadLocationResolver.sanitizeSegment(conversationId)).resolve(storedName); Path base = uploadRoot.getParent(); Path relative = base != null ? base.relativize(target) : target; return relative.toString().replace('\\', '/'); diff --git a/mateclaw-server/src/main/java/vip/mate/channel/webchat/WebChatFileService.java b/mateclaw-server/src/main/java/vip/mate/channel/webchat/WebChatFileService.java index 87832cb2..edbb0b64 100644 --- a/mateclaw-server/src/main/java/vip/mate/channel/webchat/WebChatFileService.java +++ b/mateclaw-server/src/main/java/vip/mate/channel/webchat/WebChatFileService.java @@ -120,7 +120,9 @@ public class WebChatFileService { String storedName = UUID.randomUUID() + "_" + safeName; Path uploadRoot = uploadLocationResolver.resolveUploadRoot(conversationId).normalize(); - Path dir = uploadRoot.resolve(conversationId).normalize(); + // Sanitize the id for the path segment (IM ids like "wecom:XXXX" carry a + // ':' illegal on Windows); reads use the same sanitization. + Path dir = uploadRoot.resolve(ChatUploadLocationResolver.sanitizeSegment(conversationId)).normalize(); if (!dir.startsWith(uploadRoot)) { // conversationId is server-derived, so this should never happen; fail closed if it does. throw new UploadRejectedException("Invalid conversation"); @@ -172,10 +174,10 @@ public class WebChatFileService { } // Check every candidate root (workspace-scoped dir + legacy default dir) // so files written before the workspace-aware relocation still resolve. - for (Path root : uploadLocationResolver.resolveCandidateUploadRoots(conversationId)) { - Path base = root.resolve(conversationId).normalize(); - Path file = base.resolve(storedName).normalize(); - if (file.startsWith(base) && Files.exists(file) && Files.isRegularFile(file)) { + for (Path base : uploadLocationResolver.resolveCandidateConversationDirs(conversationId)) { + Path normBase = base.normalize(); + Path file = normBase.resolve(storedName).normalize(); + if (file.startsWith(normBase) && Files.exists(file) && Files.isRegularFile(file)) { return Optional.of(file); } } diff --git a/mateclaw-server/src/main/java/vip/mate/channel/wecom/WeComChannelAdapter.java b/mateclaw-server/src/main/java/vip/mate/channel/wecom/WeComChannelAdapter.java index 284fae60..5dbb483e 100644 --- a/mateclaw-server/src/main/java/vip/mate/channel/wecom/WeComChannelAdapter.java +++ b/mateclaw-server/src/main/java/vip/mate/channel/wecom/WeComChannelAdapter.java @@ -5,6 +5,7 @@ import lombok.extern.slf4j.Slf4j; import vip.mate.channel.AbstractChannelAdapter; import vip.mate.channel.ChannelMessage; import vip.mate.channel.ChannelMessageRouter; +import vip.mate.workspace.core.service.ChatUploadLocationResolver; import vip.mate.channel.ExponentialBackoff; import vip.mate.channel.media.InboundMediaDownloader; import vip.mate.channel.model.ChannelEntity; @@ -2970,8 +2971,8 @@ public class WeComChannelAdapter extends AbstractChannelAdapter { // dedup-named write; the WeCom-specific AES-256-CBC decrypt stays here // inside the byte source so a fetch + decrypt is retried as one unit. Path uploadDir = (chatUploadLocationResolver != null) - ? chatUploadLocationResolver.resolveUploadRoot(conversationId).resolve(conversationId) - : Path.of("data", "chat-uploads", conversationId); + ? chatUploadLocationResolver.resolveConversationDir(conversationId) + : Path.of("data", "chat-uploads", ChatUploadLocationResolver.sanitizeSegment(conversationId)); String hint = (fileNameHint == null || fileNameHint.isBlank()) ? null : fileNameHint; return InboundMediaDownloader.download( () -> { diff --git a/mateclaw-server/src/main/java/vip/mate/channel/weixin/WeixinChannelAdapter.java b/mateclaw-server/src/main/java/vip/mate/channel/weixin/WeixinChannelAdapter.java index 125d3c31..d7050524 100644 --- a/mateclaw-server/src/main/java/vip/mate/channel/weixin/WeixinChannelAdapter.java +++ b/mateclaw-server/src/main/java/vip/mate/channel/weixin/WeixinChannelAdapter.java @@ -10,6 +10,7 @@ import vip.mate.channel.media.InboundMediaDownloader; import vip.mate.channel.model.ChannelEntity; import vip.mate.channel.weixin.error.TokenExpiredException; import vip.mate.common.security.SecretEquals; +import vip.mate.workspace.core.service.ChatUploadLocationResolver; import vip.mate.workspace.conversation.model.MessageContentPart; import java.io.IOException; @@ -687,8 +688,8 @@ public class WeixinChannelAdapter extends AbstractChannelAdapter { } Path uploadDir = (chatUploadLocationResolver != null) - ? chatUploadLocationResolver.resolveUploadRoot(conversationId).resolve(conversationId) - : Path.of("data", "chat-uploads", conversationId); + ? chatUploadLocationResolver.resolveConversationDir(conversationId) + : Path.of("data", "chat-uploads", ChatUploadLocationResolver.sanitizeSegment(conversationId)); return InboundMediaDownloader.download( () -> client.downloadMedia("", aesKey, encryptQueryParam), filenameHint, diff --git a/mateclaw-server/src/main/java/vip/mate/tool/builtin/ChatUploadResolver.java b/mateclaw-server/src/main/java/vip/mate/tool/builtin/ChatUploadResolver.java index cb713d39..1007cec0 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/builtin/ChatUploadResolver.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/builtin/ChatUploadResolver.java @@ -5,6 +5,7 @@ import vip.mate.workspace.core.service.ChatUploadLocationResolver; import java.io.IOException; import java.nio.file.Files; +import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.nio.file.Paths; import java.util.ArrayList; @@ -97,13 +98,32 @@ public final class ChatUploadResolver { Set dirs = new LinkedHashSet<>(); String basePath = ToolExecutionContext.workspaceBasePath(); if (basePath != null && !basePath.isBlank()) { - dirs.add(Paths.get(basePath).toAbsolutePath().normalize() - .resolve(UPLOAD_SUBDIR).resolve(conversationId)); + Path scopedRoot = Paths.get(basePath).toAbsolutePath().normalize().resolve(UPLOAD_SUBDIR); + addConversationDirs(dirs, scopedRoot, conversationId); } - dirs.add(defaultRoot.resolve(conversationId).toAbsolutePath().normalize()); + addConversationDirs(dirs, defaultRoot, conversationId); return new ArrayList<>(dirs); } + /** + * Add a conversation's attachment dir under {@code root} to {@code dirs}: + * the sanitized segment first (matching the write path), then — for + * backward compatibility with pre-fix Linux uploads that used the raw id + * verbatim — the raw-id dir when it differs and is a legal path on this OS. + */ + private static void addConversationDirs(Set dirs, Path root, String conversationId) { + String safe = ChatUploadLocationResolver.sanitizeSegment(conversationId); + dirs.add(root.resolve(safe).toAbsolutePath().normalize()); + if (!safe.equals(conversationId)) { + try { + dirs.add(root.resolve(conversationId).toAbsolutePath().normalize()); + } catch (InvalidPathException ignore) { + // Raw id illegal on this filesystem (e.g. ':' on Windows) — no + // legacy attachments could exist there. + } + } + } + private static Path resolveIn(String rawPath, Path uploadDir) { if (!Files.isDirectory(uploadDir)) { return null; diff --git a/mateclaw-server/src/main/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardian.java b/mateclaw-server/src/main/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardian.java index 5c0d333b..5ee19d99 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardian.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/guard/guardian/WorkspaceBoundaryGuardian.java @@ -9,6 +9,7 @@ import vip.mate.tool.guard.WorkspacePathGuard; import vip.mate.tool.guard.model.*; import vip.mate.workspace.core.service.ChatUploadLocationResolver; +import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.nio.file.Paths; import java.util.List; @@ -193,11 +194,26 @@ public class WorkspaceBoundaryGuardian implements ToolGuardGuardian { } catch (Exception e) { return false; } + String safeSegment = ChatUploadLocationResolver.sanitizeSegment(conversationId); for (Path root : candidateRoots) { - Path uploadDir = root.resolve(conversationId).toAbsolutePath().normalize(); - if (normalized.startsWith(uploadDir)) { + // Accept the sanitized dir (where writes land) — always a legal + // segment — and, for legacy Linux uploads, the raw-id dir when it is + // a legal path on this OS. Write and read must agree here or a valid + // attachment read would be flagged as a boundary escape. + Path sanitizedDir = root.resolve(safeSegment).toAbsolutePath().normalize(); + if (normalized.startsWith(sanitizedDir)) { return true; } + if (!safeSegment.equals(conversationId)) { + try { + Path rawDir = root.resolve(conversationId).toAbsolutePath().normalize(); + if (normalized.startsWith(rawDir)) { + return true; + } + } catch (InvalidPathException ignore) { + // Raw id illegal on this filesystem (e.g. ':' on Windows). + } + } } return false; } diff --git a/mateclaw-server/src/main/java/vip/mate/tool/image/ImageFileDownloader.java b/mateclaw-server/src/main/java/vip/mate/tool/image/ImageFileDownloader.java index 5cace0df..45cf1558 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/image/ImageFileDownloader.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/image/ImageFileDownloader.java @@ -40,7 +40,7 @@ public class ImageFileDownloader { if (imageUrl == null) { throw new IOException("imageUrl is null"); } - Path dir = uploadLocationResolver.resolveUploadRoot(conversationId).resolve(conversationId); + Path dir = uploadLocationResolver.resolveConversationDir(conversationId); Files.createDirectories(dir); if (imageUrl.startsWith("data:")) { @@ -112,7 +112,7 @@ public class ImageFileDownloader { * 将 Base64 编码的图片保存到本地 */ public Path saveBase64(String base64Data, String conversationId, String taskId, int index) throws IOException { - Path dir = uploadLocationResolver.resolveUploadRoot(conversationId).resolve(conversationId); + Path dir = uploadLocationResolver.resolveConversationDir(conversationId); Files.createDirectories(dir); String fileName = "image_" + taskId + "_" + index + ".png"; diff --git a/mateclaw-server/src/main/java/vip/mate/tool/model3d/Model3dFileDownloader.java b/mateclaw-server/src/main/java/vip/mate/tool/model3d/Model3dFileDownloader.java index 326a9f14..56c3a9b3 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/model3d/Model3dFileDownloader.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/model3d/Model3dFileDownloader.java @@ -24,7 +24,7 @@ public class Model3dFileDownloader { public Path download(String modelUrl, String conversationId, String taskId, String preferredExtension) throws IOException { - Path dir = uploadLocationResolver.resolveUploadRoot(conversationId).resolve(conversationId); + Path dir = uploadLocationResolver.resolveConversationDir(conversationId); Files.createDirectories(dir); String ext = guessExtension(modelUrl, preferredExtension); diff --git a/mateclaw-server/src/main/java/vip/mate/tool/music/MusicGenerationService.java b/mateclaw-server/src/main/java/vip/mate/tool/music/MusicGenerationService.java index 0a2ddef8..d228db79 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/music/MusicGenerationService.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/music/MusicGenerationService.java @@ -191,7 +191,7 @@ public class MusicGenerationService { private PersistedAudio persistAudio(String conversationId, String taskId, MusicGenerationResult result) throws IOException { - Path dir = uploadLocationResolver.resolveUploadRoot(conversationId).resolve(conversationId); + Path dir = uploadLocationResolver.resolveConversationDir(conversationId); Files.createDirectories(dir); String fileName = "music_" + taskId + "." + result.getFormat(); Path filePath = dir.resolve(fileName); diff --git a/mateclaw-server/src/main/java/vip/mate/tool/video/VideoFileDownloader.java b/mateclaw-server/src/main/java/vip/mate/tool/video/VideoFileDownloader.java index 63e00632..856c6921 100644 --- a/mateclaw-server/src/main/java/vip/mate/tool/video/VideoFileDownloader.java +++ b/mateclaw-server/src/main/java/vip/mate/tool/video/VideoFileDownloader.java @@ -31,7 +31,7 @@ public class VideoFileDownloader { * @return 本地文件路径 */ public Path download(String videoUrl, String conversationId, String taskId) throws IOException { - Path dir = uploadLocationResolver.resolveUploadRoot(conversationId).resolve(conversationId); + Path dir = uploadLocationResolver.resolveConversationDir(conversationId); Files.createDirectories(dir); String extension = guessExtension(videoUrl); diff --git a/mateclaw-server/src/main/java/vip/mate/tts/TtsService.java b/mateclaw-server/src/main/java/vip/mate/tts/TtsService.java index ee1a8fd4..f25fc16d 100644 --- a/mateclaw-server/src/main/java/vip/mate/tts/TtsService.java +++ b/mateclaw-server/src/main/java/vip/mate/tts/TtsService.java @@ -204,7 +204,7 @@ public class TtsService { private Path saveAudioFile(String conversationId, String fileId, byte[] data, String format) throws IOException { - Path dir = uploadLocationResolver.resolveUploadRoot(conversationId).resolve(conversationId); + Path dir = uploadLocationResolver.resolveConversationDir(conversationId); Files.createDirectories(dir); String fileName = "tts_" + fileId + "." + format; Path filePath = dir.resolve(fileName); diff --git a/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java b/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java index 3ad92676..99df5997 100644 --- a/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java +++ b/mateclaw-server/src/main/java/vip/mate/workspace/conversation/ConversationService.java @@ -36,7 +36,6 @@ import vip.mate.workspace.core.service.WorkspaceService; import java.io.IOException; import java.nio.file.Files; -import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.time.LocalDateTime; import java.util.ArrayList; @@ -1692,18 +1691,10 @@ public class ConversationService { return; } boolean cleanedAny = false; - for (Path root : chatUploadLocationResolver.resolveCandidateUploadRoots(conversationId)) { - Path dir; - try { - dir = root.resolve(conversationId); - } catch (InvalidPathException e) { - // Conversation id contains characters illegal on this filesystem - // (e.g. ':' in cron: on Windows). No attachments could - // ever have been written under such an id on this OS, so there - // is nothing to clean. - log.debug("Skipping attachment cleanup for non-path-safe conversation id: {}", conversationId); - return; - } + // resolveCandidateConversationDirs sanitizes the id for the path segment + // (so ids like "wecom:XXXX" clean correctly on Windows) and also probes + // the raw-id dir for pre-fix Linux uploads. + for (Path dir : chatUploadLocationResolver.resolveCandidateConversationDirs(conversationId)) { if (!Files.exists(dir)) { continue; } diff --git a/mateclaw-server/src/main/java/vip/mate/workspace/core/service/ChatUploadLocationResolver.java b/mateclaw-server/src/main/java/vip/mate/workspace/core/service/ChatUploadLocationResolver.java index 034f0455..f6f3c9e9 100644 --- a/mateclaw-server/src/main/java/vip/mate/workspace/core/service/ChatUploadLocationResolver.java +++ b/mateclaw-server/src/main/java/vip/mate/workspace/core/service/ChatUploadLocationResolver.java @@ -17,6 +17,7 @@ import vip.mate.workspace.core.model.WorkspaceEntity; import vip.mate.workspace.conversation.model.ConversationEntity; import vip.mate.workspace.conversation.repository.ConversationMapper; +import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.nio.file.Paths; import java.time.Duration; @@ -62,6 +63,65 @@ public class ChatUploadLocationResolver { /** Sub-directory appended under a configured base path. */ public static final String UPLOAD_SUBDIR = "chat-uploads"; + /** + * Turn a business conversation id into a filesystem-safe path segment. + *

+ * IM-channel conversation ids carry a {@code channelType:identifier} shape + * (e.g. {@code wecom:XuZhanFu}); the {@code :} is illegal in a Windows + * filename (reserved for drive / alternate-data-stream syntax), so using the + * raw id as a directory name throws {@link InvalidPathException} on Windows + * and breaks every attachment / media write for IM channels there. Every + * conversationId → directory-segment mapping MUST route through this method + * so writes and reads agree on the on-disk layout. + *

+ * The replacement set matches {@code ToolResultStorage.sanitize} exactly and + * is a no-op for ids that are already {@code [A-Za-z0-9_.-]}-only (web / + * webchat / numeric), so their existing on-disk layout is unchanged. + * + * @param conversationId raw business id ({@code null} yields empty string) + * @return a segment safe to use as a single path component on any OS + */ + public static String sanitizeSegment(String conversationId) { + if (conversationId == null) return ""; + return conversationId.replaceAll("[^A-Za-z0-9_.-]", "_"); + } + + /** + * The single conversation attachment directory (write target): + * {@code {uploadRoot}/{sanitizeSegment(conversationId)}/}. + */ + public Path resolveConversationDir(String conversationId) { + return resolveUploadRoot(conversationId).resolve(sanitizeSegment(conversationId)); + } + + /** + * Every conversation attachment directory a read / cleanup path should probe, + * ordered: the sanitized dir under each candidate root first, then — for + * backward compatibility with pre-fix Linux uploads that used the raw id + * verbatim — the raw-id dir (only when it differs from the sanitized form + * and is a legal path on this filesystem). + *

+ * On Windows a raw id containing {@code :} throws {@link InvalidPathException} + * from {@link Path#resolve(String)}; such an id never produced a directory on + * Windows, so the raw candidate is simply skipped. + */ + public List resolveCandidateConversationDirs(String conversationId) { + String safe = sanitizeSegment(conversationId); + Set dirs = new LinkedHashSet<>(); + for (Path root : resolveCandidateUploadRoots(conversationId)) { + dirs.add(root.resolve(safe)); + if (!safe.equals(conversationId)) { + try { + dirs.add(root.resolve(conversationId)); + } catch (InvalidPathException ignore) { + // Raw id is not a legal path on this OS (e.g. ':' on Windows); + // no legacy attachments could exist there, so skip it. + } + } + } + return new ArrayList<>(dirs); + } + private final ConversationMapper conversationMapper; private final WorkspaceService workspaceService; private final ChatUploadProperties properties; diff --git a/mateclaw-server/src/test/java/vip/mate/channel/webchat/WebChatAttachmentE2ETest.java b/mateclaw-server/src/test/java/vip/mate/channel/webchat/WebChatAttachmentE2ETest.java index a41140de..1d6136c2 100644 --- a/mateclaw-server/src/test/java/vip/mate/channel/webchat/WebChatAttachmentE2ETest.java +++ b/mateclaw-server/src/test/java/vip/mate/channel/webchat/WebChatAttachmentE2ETest.java @@ -13,6 +13,7 @@ import reactor.core.publisher.Flux; import vip.mate.MateClawApplication; import vip.mate.agent.AgentService; import vip.mate.agent.model.AgentEntity; +import vip.mate.workspace.core.service.ChatUploadLocationResolver; import java.io.ByteArrayOutputStream; import java.io.IOException; @@ -282,9 +283,11 @@ class WebChatAttachmentE2ETest { assertThat(parts).contains("\"fileName\":\"note.txt\""); assertThat(parts).contains("\"contentType\":\"text/plain\""); assertThat(parts).contains("\"path\":\""); - // Path points into the conversation's upload dir on disk. + // Path points into the conversation's upload dir on disk. The dir uses + // the sanitized conversation id (cid carries ':' which is path-illegal + // on Windows), so assert against the sanitized segment. String path = extractStringField(parts, "path"); - assertThat(path).contains(cid); + assertThat(path).contains(ChatUploadLocationResolver.sanitizeSegment(cid)); assertThat(Files.isRegularFile(Path.of(path))).isTrue(); // The bytes on disk match what we uploaded. assertThat(Files.readString(Path.of(path))).isEqualTo(fileBody); diff --git a/mateclaw-server/src/test/java/vip/mate/channel/webchat/WebChatFileServiceTest.java b/mateclaw-server/src/test/java/vip/mate/channel/webchat/WebChatFileServiceTest.java index 03223a72..9e8c17b3 100644 --- a/mateclaw-server/src/test/java/vip/mate/channel/webchat/WebChatFileServiceTest.java +++ b/mateclaw-server/src/test/java/vip/mate/channel/webchat/WebChatFileServiceTest.java @@ -4,6 +4,7 @@ import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.DisplayName; import org.junit.jupiter.api.Test; import org.springframework.mock.web.MockMultipartFile; +import vip.mate.workspace.core.service.ChatUploadLocationResolver; import vip.mate.workspace.core.service.ChatUploadLocationResolverTestSupport; import java.io.IOException; @@ -39,7 +40,9 @@ class WebChatFileServiceTest { @AfterEach void cleanup() throws IOException { - Path dir = Paths.get("data", "chat-uploads", CONV); + // Attachments land under the sanitized segment (CONV carries ':'), so + // clean that dir — not the raw-id one. + Path dir = Paths.get("data", "chat-uploads", ChatUploadLocationResolver.sanitizeSegment(CONV)); if (Files.exists(dir)) { try (Stream walk = Files.walk(dir)) { walk.sorted(Comparator.reverseOrder()).forEach(p -> { diff --git a/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceCleanAttachmentFilesTest.java b/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceCleanAttachmentFilesTest.java index 992a115f..852a42f3 100644 --- a/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceCleanAttachmentFilesTest.java +++ b/mateclaw-server/src/test/java/vip/mate/workspace/conversation/ConversationServiceCleanAttachmentFilesTest.java @@ -17,7 +17,6 @@ import vip.mate.workspace.core.service.ChatUploadLocationResolver; import java.io.IOException; import java.nio.file.Files; -import java.nio.file.InvalidPathException; import java.nio.file.Path; import java.nio.file.Paths; import java.util.Comparator; @@ -27,27 +26,20 @@ import java.util.stream.Stream; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatCode; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.when; /** - * Regression coverage for issue #36: deleting a CRON-task conversation throws - * {@code InvalidPathException} on Windows because the conversation id - * ("cron:<jobId>") contains a colon, which is illegal in Windows path - * segments. The exception bubbles out of the {@code @Transactional} - * {@code deleteConversation}, rolling back the row deletes and leaving the - * user unable to remove the entry. + * Regression coverage for issues #36 / #507: a conversation id like + * {@code cron:} or {@code wecom:} carries a colon, which is illegal + * in a Windows path segment. The old behaviour caught the resulting + * {@code InvalidPathException} and silently skipped cleanup; the fix instead + * sanitizes the id to a filesystem-safe segment so the attachment directory is + * both written and cleaned consistently on every OS. */ @ExtendWith(MockitoExtension.class) class ConversationServiceCleanAttachmentFilesTest { - /** NUL byte: rejected by Paths.get on every OS, so it portably triggers - * the same InvalidPathException branch the colon hits on Windows. Built - * via String.valueOf((char) 0) so the source text contains no embedded - * NUL (which would be invisible in diff tools). */ - private static final String UNREPRESENTABLE_ID = "bad" + (char) 0 + "id"; - @Mock private ConversationMapper conversationMapper; @Mock private MessageMapper messageMapper; @Mock private AgentMapper agentMapper; @@ -60,11 +52,11 @@ class ConversationServiceCleanAttachmentFilesTest { @BeforeEach void stubResolver() { - // cleanAttachmentFiles now resolves the upload root via the resolver. - // Point its candidate roots at the legacy default dir so both the - // happy-path and unrepresentable-id cases exercise the real filesystem. - when(chatUploadLocationResolver.resolveCandidateUploadRoots(any())) - .thenReturn(List.of(Paths.get("data", "chat-uploads"))); + // cleanAttachmentFiles now walks sanitized conversation dirs. Mirror the + // real resolver: {legacy-default-root}/{sanitizeSegment(id)}. + when(chatUploadLocationResolver.resolveCandidateConversationDirs(any())) + .thenAnswer(inv -> List.of(Paths.get("data", "chat-uploads") + .resolve(ChatUploadLocationResolver.sanitizeSegment(inv.getArgument(0))))); } @AfterEach @@ -79,28 +71,25 @@ class ConversationServiceCleanAttachmentFilesTest { } @Test - @DisplayName("conversation id that yields an unrepresentable path is skipped, not thrown") - void unrepresentablePathIdIsSkipped() { - // Precondition: confirm the id really does break Paths.resolve on - // this JDK / OS. If a future JDK ever accepts the NUL byte, the - // service-level assertion below would silently pass without - // exercising the catch branch we are guarding — fail loudly here - // instead. - assertThatThrownBy(() -> Paths.get("data", "chat-uploads").resolve(UNREPRESENTABLE_ID)) - .isInstanceOf(InvalidPathException.class); + @DisplayName("colon-bearing id (cron:jobId / wecom:xxx) is sanitized and its dir is cleaned, not skipped") + void colonIdIsSanitizedAndCleaned() throws IOException { + // The ':' would throw InvalidPathException as a raw Windows path segment; + // the service must sanitize it and clean the sanitized dir — never throw, + // never silently skip (the pre-fix bug from issue #36). + String convId = "cron:job-" + UUID.randomUUID(); + Path uploadRoot = Paths.get("data", "chat-uploads"); + createdDir = uploadRoot.resolve(ChatUploadLocationResolver.sanitizeSegment(convId)); + Files.createDirectories(createdDir); + Files.writeString(createdDir.resolve("a.txt"), "hello"); - // Without the catch in cleanAttachmentFiles, this would propagate - // InvalidPathException out of the @Transactional deleteConversation, - // rolling back the row deletes — the user-visible bug from issue #36. - assertThatCode(() -> service.cleanAttachmentFiles(UNREPRESENTABLE_ID)) - .doesNotThrowAnyException(); + assertThatCode(() -> service.cleanAttachmentFiles(convId)).doesNotThrowAnyException(); + assertThat(Files.exists(createdDir)).isFalse(); } @Test @DisplayName("legal id with a real attachment dir is still cleaned (happy path)") void legalIdHappyPathStillCleans() throws IOException { - // UUID-shaped id matches what the web channel actually uses, so the - // resolve() succeeds on every OS and the walk/delete loop runs. + // UUID-shaped id matches what the web channel uses; sanitize is a no-op. String convId = "test-" + UUID.randomUUID(); Path uploadRoot = Paths.get("data", "chat-uploads"); createdDir = uploadRoot.resolve(convId); diff --git a/mateclaw-server/src/test/java/vip/mate/workspace/core/service/ChatUploadLocationResolverTest.java b/mateclaw-server/src/test/java/vip/mate/workspace/core/service/ChatUploadLocationResolverTest.java index 900f7078..214fb224 100644 --- a/mateclaw-server/src/test/java/vip/mate/workspace/core/service/ChatUploadLocationResolverTest.java +++ b/mateclaw-server/src/test/java/vip/mate/workspace/core/service/ChatUploadLocationResolverTest.java @@ -200,4 +200,68 @@ class ChatUploadLocationResolverTest { assertThat(root).isEqualTo(tempDir.toAbsolutePath().normalize()); } + + // ==================== conversationId path safety (issue #507) ==================== + + @Test + @DisplayName("sanitizeSegment: colon (and other unsafe chars) → underscore; safe ids unchanged") + void sanitizeSegmentReplacesUnsafeChars() { + // IM-channel id with the ':' that breaks Windows paths. + assertThat(ChatUploadLocationResolver.sanitizeSegment("wecom:XuZhanFu")) + .isEqualTo("wecom_XuZhanFu"); + // Separator-laden id is fully flattened to a single safe segment. + assertThat(ChatUploadLocationResolver.sanitizeSegment("a/b\\c:d*e?")) + .isEqualTo("a_b_c_d_e_"); + // Already-safe ids (web / webchat / numeric) are a no-op. + assertThat(ChatUploadLocationResolver.sanitizeSegment("2055137662148763649")) + .isEqualTo("2055137662148763649"); + assertThat(ChatUploadLocationResolver.sanitizeSegment("conv-abc_1.2")) + .isEqualTo("conv-abc_1.2"); + assertThat(ChatUploadLocationResolver.sanitizeSegment(null)).isEmpty(); + } + + @Test + @DisplayName("resolveConversationDir: colon id lands under the sanitized segment (no InvalidPathException)") + void resolveConversationDirUsesSanitizedSegment() { + stubConversation("wecom:XuZhanFu", null, null); + when(workspaceService.getById(anyLong())).thenReturn(workspace(1L, null)); + + ChatUploadLocationResolver r = resolver(tempDir); + Path dir = r.resolveConversationDir("wecom:XuZhanFu"); + + assertThat(dir).isEqualTo(tempDir.toAbsolutePath().normalize().resolve("wecom_XuZhanFu")); + } + + @Test + @DisplayName("candidate conversation dirs: sanitized first, then raw id for backward compat") + void candidateConversationDirsIncludeSanitizedAndRaw() { + stubConversation("wecom:XuZhanFu", null, null); + when(workspaceService.getById(anyLong())).thenReturn(workspace(1L, null)); + + ChatUploadLocationResolver r = resolver(tempDir); + List dirs = r.resolveCandidateConversationDirs("wecom:XuZhanFu"); + Path base = tempDir.toAbsolutePath().normalize(); + + int sanitizedIdx = dirs.indexOf(base.resolve("wecom_XuZhanFu")); + assertThat(sanitizedIdx).isGreaterThanOrEqualTo(0); + // On a POSIX filesystem the raw ':' dir is a legal (legacy) candidate, + // ordered after the sanitized one. + boolean posix = !System.getProperty("os.name").toLowerCase().contains("win"); + if (posix) { + int rawIdx = dirs.indexOf(base.resolve("wecom:XuZhanFu")); + assertThat(rawIdx).isGreaterThan(sanitizedIdx); + } + } + + @Test + @DisplayName("candidate conversation dirs: safe id yields a single dir (no duplicate raw)") + void candidateConversationDirsNoDuplicateForSafeId() { + stubConversation("plainconv", null, null); + when(workspaceService.getById(anyLong())).thenReturn(workspace(1L, null)); + + ChatUploadLocationResolver r = resolver(tempDir); + List dirs = r.resolveCandidateConversationDirs("plainconv"); + + assertThat(dirs).containsExactly(tempDir.toAbsolutePath().normalize().resolve("plainconv")); + } }