reset message wrapper and fix path error

[iceljc]

PR Type

Bug fix, Enhancement

---

Description

• Reset message wrapper to prevent data leakage

• Add null/empty parameter validation across file services

• Improve code formatting and error handling

• Fix path error handling in file operations

---

Diagram Walkthrough

flowchart LR
  A["Input Parameters"] --> B["Validation Check"]
  B --> C["File Operations"]
  C --> D["Message Processing"]
  D --> E["Reset Wrapper"]
  E --> F["Clean Output"]

Loading

File Walkthrough

	Relevant files
Bug fix	LocalFileStorageService.Audio.cs Add parameter validation to audio file service                      src/Infrastructure/BotSharp.Core/Files/Services/Storage/LocalFileStorageService.Audio.cs Add null/empty validation for conversationId and fileName parameters Add whitespace formatting for better readability +6/-0      LocalFileStorageService.Conversation.cs Add parameter validation and improve formatting                    src/Infrastructure/BotSharp.Core/Files/Services/Storage/LocalFileStorageService.Conversation.cs Add comprehensive null/empty parameter validation across all methods Improve code formatting with proper brace placement Add validation for messageId in loops to prevent processing empty values Enhance error handling consistency +79/-13  RoutingService.InvokeAgent.cs Reset message wrapper to prevent data leakage                        src/Infrastructure/BotSharp.Core/Routing/RoutingService.InvokeAgent.cs Reset AdditionalMessageWrapper to null to prevent data leakage Clear wrapper in both function invocation and assistant response paths +2/-0
Enhancement	PlotChartFn.cs Simplify RoleDialogModel constructor usage                              src/Plugins/BotSharp.Plugin.ChartHandler/Functions/PlotChartFn.cs Simplify RoleDialogModel constructor usage Remove redundant property assignments by using constructor parameters +1/-3

---

[qodo-merge-pro]

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪

🧪 No relevant tests

🔒 No security concerns identified

⚡ Recommended focus areas for review Potential Behavior Change Early returns and additional guards now skip processing when inputs (e.g., messageIds elements) are null/whitespace or directories/files are missing. Verify callers rely on this silent skipping and that no required files are skipped due to trimming/whitespace or case sensitivity in file names. public IEnumerable<MessageFileModel> GetMessageFiles(string conversationId, IEnumerable<string> messageIds, string source, IEnumerable<string>? contentTypes = null) { var files = new List<MessageFileModel>(); if (string.IsNullOrWhiteSpace(conversationId) || messageIds.IsNullOrEmpty()) { return files; } foreach (var messageId in messageIds) { if (string.IsNullOrWhiteSpace(messageId)) { continue; } var dir = Path.Combine(_baseDir, CONVERSATION_FOLDER, conversationId, FILE_FOLDER, messageId, source); if (!ExistDirectory(dir)) { continue; } foreach (var subDir in Directory.GetDirectories(dir)) { var index = subDir.Split(Path.DirectorySeparatorChar).Last(); foreach (var file in Directory.GetFiles(subDir)) { var contentType = FileUtility.GetFileContentType(file); if (!contentTypes.IsNullOrEmpty() && !contentTypes.Contains(contentType)) { continue; } var fileName = Path.GetFileNameWithoutExtension(file); var fileExtension = Path.GetExtension(file).Substring(1); var model = new MessageFileModel() { MessageId = messageId, FileUrl = $"/conversation/{conversationId}/message/{messageId}/{source}/file/{index}/{fileName}", FileDownloadUrl = $"/conversation/{conversationId}/message/{messageId}/{source}/file/{index}/{fileName}/download", FileStorageUrl = file, FileName = fileName, FileExtension = fileExtension, ContentType = contentType, FileSource = source }; files.Add(model); } } } return files; } public string GetMessageFile(string conversationId, string messageId, string source, string index, string fileName) { if (string.IsNullOrWhiteSpace(conversationId) || string.IsNullOrWhiteSpace(messageId) || string.IsNullOrWhiteSpace(source) || string.IsNullOrWhiteSpace(index)) { return string.Empty; } var dir = Path.Combine(_baseDir, CONVERSATION_FOLDER, conversationId, FILE_FOLDER, messageId, source, index); if (!ExistDirectory(dir)) { return string.Empty; } var found = Directory.GetFiles(dir).FirstOrDefault(f => Path.GetFileNameWithoutExtension(f).IsEqualTo(fileName)); return found; } public IEnumerable<MessageFileModel> GetMessagesWithFile(string conversationId, IEnumerable<string> messageIds) { var foundMsgs = new List<MessageFileModel>(); if (string.IsNullOrWhiteSpace(conversationId) || messageIds.IsNullOrEmpty()) { return foundMsgs; } foreach (var messageId in messageIds) { if (string.IsNullOrWhiteSpace(messageId)) { continue; } var prefix = Path.Combine(_baseDir, CONVERSATION_FOLDER, conversationId, FILE_FOLDER, messageId); var userDir = Path.Combine(prefix, FileSourceType.User); if (ExistDirectory(userDir)) { foundMsgs.Add(new MessageFileModel { MessageId = messageId, FileSource = FileSourceType.User }); } var botDir = Path.Combine(prefix, FileSourceType.Bot); if (ExistDirectory(botDir)) { foundMsgs.Add(new MessageFileModel { MessageId = messageId, FileSource = FileSourceType.Bot }); } } return foundMsgs; } public bool SaveMessageFiles(string conversationId, string messageId, string source, List<FileDataModel> files) { if (string.IsNullOrWhiteSpace(conversationId) || string.IsNullOrWhiteSpace(messageId) || string.IsNullOrWhiteSpace(source) || files.IsNullOrEmpty()) { return false; } var dir = GetConversationFileDirectory(conversationId, messageId, createNewDir: true); if (!ExistDirectory(dir)) { return false; } for (int i = 0; i < files.Count; i++) State Reset Clearing message.AdditionalMessageWrapper is intended to prevent leakage; confirm no downstream logic expects this wrapper after invocation or assistant response, and that partial streaming paths also reset it consistently. response.FunctionName = response.FunctionName.Split("/").Last(); } message.ToolCallId = response.ToolCallId; message.FunctionName = response.FunctionName; message.FunctionArgs = response.FunctionArgs; message.Indication = response.Indication; message.CurrentAgentId = agent.Id; message.IsStreaming = response.IsStreaming; message.AdditionalMessageWrapper = null; await InvokeFunction(message, dialogs, options); } else { // Handle output routing exception. if (agent.Type == AgentType.Routing) { // Forgot about what situation needs to handle in this way response.Content = "Apologies, I'm not quite sure I understand. Could you please provide additional clarification or context?"; } message = RoleDialogModel.From(message, role: AgentRole.Assistant, content: response.Content); message.CurrentAgentId = agent.Id; message.IsStreaming = response.IsStreaming; message.AdditionalMessageWrapper = null; dialogs.Add(message); Context.SetDialogs(dialogs); Message Construction The new RoleDialogModel constructor call sets role/content directly. Ensure other required fields (e.g., locale, metadata) are not implicitly initialized before and now omitted, and that MessageId reuse is intentional. if (!string.IsNullOrEmpty(obj?.ReportSummary)) { message.AdditionalMessageWrapper = new() { SendingInterval = 1500, SaveToDb = true, Messages = new List<RoleDialogModel> { new(AgentRole.Assistant, obj.ReportSummary) { MessageId = message.MessageId, CurrentAgentId = message.CurrentAgentId, Indication = "Summarizing", FunctionName = message.FunctionName, FunctionArgs = message.FunctionArgs, CreatedAt = DateTime.UtcNow

[qodo-merge-pro]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
High-level	Harden path handling and sandboxing Many file operations directly combine user-controlled identifiers (conversationId, messageId, source, index) into paths without normalization or sandbox checks, enabling potential path traversal and unintended deletions/reads. Introduce a centralized path builder that validates inputs (rejects invalid chars, absolute paths, and ".."), normalizes with Path.GetFullPath, and enforces containment under the configured base directory before any IO. Apply it consistently across all Save/Get/Delete methods (local and cloud providers) to prevent cross-tenant leakage and destructive deletes. Examples: src/Infrastructure/BotSharp.Core/Files/Services/Storage/LocalFileStorageService.Conversation.cs [108-126] public string GetMessageFile(string conversationId, string messageId, string source, string index, string fileName) { if (string.IsNullOrWhiteSpace(conversationId) || string.IsNullOrWhiteSpace(messageId) || string.IsNullOrWhiteSpace(source) || string.IsNullOrWhiteSpace(index)) { return string.Empty; } ... (clipped 9 lines) src/Infrastructure/BotSharp.Core/Files/Services/Storage/LocalFileStorageService.Conversation.cs [257-275] public bool DeleteConversationFiles(IEnumerable<string> conversationIds) { if (conversationIds.IsNullOrEmpty()) { return false; } foreach (var conversationId in conversationIds) { var convDir = GetConversationDirectory(conversationId); ... (clipped 9 lines) Solution Walkthrough: Before: public string GetMessageFile(string conversationId, string messageId, string source, string index, string fileName) { // User-controlled inputs are directly used to build a path. // A malicious 'conversationId' like "../sensitive_data" could escape the intended directory. var dir = Path.Combine(_baseDir, CONVERSATION_FOLDER, conversationId, FILE_FOLDER, messageId, source, index); if (!ExistDirectory(dir)) { return string.Empty; } var found = Directory.GetFiles(dir).FirstOrDefault(...); return found; } public bool DeleteConversationFiles(IEnumerable<string> conversationIds) { foreach (var conversationId in conversationIds) { // A malicious 'conversationId' could lead to deleting unintended directories. var convDir = Path.Combine(_baseDir, CONVERSATION_FOLDER, conversationId); DeleteDirectory(convDir); } return true; } After: // In a new centralized path utility public static string BuildSanitizedPath(string baseDir, params string[] pathSegments) { foreach (var segment in pathSegments) { if (segment.Contains("..") || Path.IsPathRooted(segment)) { throw new ArgumentException("Invalid path segment detected."); } } var fullPath = Path.GetFullPath(Path.Combine(baseDir, Path.Combine(pathSegments))); if (!fullPath.StartsWith(Path.GetFullPath(baseDir))) { throw new SecurityException("Path traversal attempt detected."); } return fullPath; } // In LocalFileStorageService public string GetMessageFile(string conversationId, string messageId, ...) { // All path construction now goes through the centralized, secure builder. var dir = PathUtility.BuildSanitizedPath(_baseDir, CONVERSATION_FOLDER, conversationId, ...); // ... } Suggestion importance[1-10]: 10 __ Why: The suggestion correctly identifies a critical path traversal security vulnerability in the file handling logic, which the PR's changes overlook, and proposes a robust, centralized solution.	High
Possible issue	Guard against empty message IDs Skip empty messageId entries before building paths to prevent deleting unintended directories or throwing on null path segments. This mirrors the validation pattern used elsewhere in this file. src/Infrastructure/BotSharp.Core/Files/Services/Storage/LocalFileStorageService.Conversation.cs [242-252] foreach (var messageId in messageIds) { + if (string.IsNullOrWhiteSpace(messageId)) + { + continue; + } + var dir = GetConversationFileDirectory(conversationId, messageId); if (!ExistDirectory(dir)) { continue; } DeleteDirectory(dir); Thread.Sleep(100); } Apply / Chat Suggestion importance[1-10]: 8 __ Why: The suggestion correctly points out a missing null/whitespace check for messageId within the loop, which is a pattern applied to other methods in this PR. This change prevents potential exceptions and makes the DeleteMessageFiles method more robust and consistent.	Medium
	Skip invalid conversation IDs Add a per-item check to skip null/whitespace conversationId values to avoid passing invalid inputs into path builders. This prevents potential exceptions and accidental deletions. src/Infrastructure/BotSharp.Core/Files/Services/Storage/LocalFileStorageService.Conversation.cs [264-274] foreach (var conversationId in conversationIds) { + if (string.IsNullOrWhiteSpace(conversationId)) + { + continue; + } + var convDir = GetConversationDirectory(conversationId); if (!ExistDirectory(convDir)) { continue; } DeleteDirectory(convDir); } return true; Apply / Chat Suggestion importance[1-10]: 8 __ Why: This suggestion correctly identifies that conversationId is not validated inside the loop, which could lead to an exception if a null or empty value is present in conversationIds. Adding this check improves robustness and aligns with the defensive coding style used elsewhere in the PR.	Medium
	Validate the source parameter Validate the source parameter before using it in Path.Combine to avoid an ArgumentNullException. If source is null or whitespace, return early like other guards. This keeps behavior consistent with the rest of the validations added in this PR. src/Infrastructure/BotSharp.Core/Files/Services/Storage/LocalFileStorageService.Conversation.cs [54-61] public IEnumerable<MessageFileModel> GetMessageFiles(string conversationId, IEnumerable<string> messageIds, string source, IEnumerable<string>? contentTypes = null) { var files = new List<MessageFileModel>(); - if (string.IsNullOrWhiteSpace(conversationId) || messageIds.IsNullOrEmpty()) + if (string.IsNullOrWhiteSpace(conversationId) || messageIds.IsNullOrEmpty() || string.IsNullOrWhiteSpace(source)) { return files; } [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: The suggestion correctly identifies that the source parameter is used in Path.Combine without validation, which could cause an ArgumentNullException. Adding this check improves the method's robustness and aligns with the PR's goal of adding comprehensive validation.	Medium
More