Features/add conv thumbnail#1297
Merged
Oceania2018 merged 3 commits intoSciSharp:masterfrom Feb 26, 2026
Merged
Conversation
Review Summary by QodoAdd conversation thumbnail support and dialog ordering functionality WalkthroughsDescription• Add conversation thumbnail support with file management • Implement dialog ordering (ascending/descending) via ConversationDialogFilter • Add ConversationFile model and filter classes for file operations • Enhance SideCarAttribute to handle null arguments with proper type matching • Extend repository interfaces with conversation file CRUD operations Diagramflowchart LR
A["ConversationFile Model"] --> B["File Management APIs"]
C["ConversationDialogFilter"] --> D["Dialog Ordering"]
B --> E["Repository Methods"]
D --> E
E --> F["FileRepository Implementation"]
E --> G["MongoRepository Implementation"]
F --> H["API Endpoints"]
G --> H
H --> I["ConversationViewModel with Thumbnail"]
File Changes1. src/Infrastructure/BotSharp.Abstraction/Conversations/IConversationService.cs
|
Code Review by Qodo
1.
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
121
to
130
| public async Task<bool> SaveConversationThumbnail([FromRoute] string conversationId, [FromBody] ConversationFileRequest request) | ||
| { | ||
| var db = _services.GetRequiredService<IBotSharpRepository>(); | ||
| var result = await db.SaveConversationFiles( | ||
| [ | ||
| new() | ||
| { | ||
| ConversationId = conversationId, | ||
| Thumbnail = request.Thumbnail | ||
| } |
There was a problem hiding this comment.
1. request not null-checked 📘 Rule violation ⛯ Reliability
SaveConversationThumbnail dereferences request.Thumbnail without guarding against a null body, which can throw at this API boundary. This violates the requirement to add null/empty guards and safe fallbacks for boundary inputs.
Agent Prompt
## Issue description
`SaveConversationThumbnail` dereferences `request.Thumbnail` without guarding `request` against null, which can throw when the request body is missing/invalid.
## Issue Context
This is an API boundary (`[FromBody]` binding). The compliance rules require explicit null/empty guards and safe fallbacks at system boundaries.
## Fix Focus Areas
- src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.File.cs[121-133]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
94
to
+100
| { | ||
| dialogs = JsonSerializer.Deserialize<List<DialogElement>>(texts, _options) ?? new List<DialogElement>(); | ||
| dialogs = JsonSerializer.Deserialize<List<DialogElement>>(texts, _options) ?? []; | ||
| } | ||
| catch | ||
| { | ||
| dialogs = new List<DialogElement>(); | ||
| dialogs = []; | ||
| } |
There was a problem hiding this comment.
2. getconversationdialogs swallows deserialize errors 📘 Rule violation ✧ Quality
JSON deserialization failures are caught without logging, which hides corrupt/invalid dialog data and makes production debugging difficult. This violates the requirement to avoid swallowed exceptions without logging or actionable context.
Agent Prompt
## Issue description
`GetConversationDialogs` catches JSON deserialization exceptions without logging, resulting in silent data corruption/format issues being undetectable.
## Issue Context
The code reads and deserializes a dialogs JSON file; failures currently degrade to `[]` with no telemetry.
## Fix Focus Areas
- src/Infrastructure/BotSharp.Core/Repository/FileRepository/FileRepository.Conversation.cs[93-105]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+57
to
60
| public async Task<List<DialogElement>> GetConversationDialogs(string conversationId, ConversationDialogFilter? filter = null) | ||
| { | ||
| if (!IsValid(conversationId)) | ||
| { |
There was a problem hiding this comment.
3. Sidecar ignores order filter 🐞 Bug ✓ Correctness
ConversationService slices dialogs differently for order="desc" vs "asc" and assumes the underlying dialog list is already ordered accordingly. BotSharpConversationSideCar.GetConversationDialogs ignores the filter and returns dialogs as-is, so order="desc" can return the wrong subset/order when sidecar is enabled.
Agent Prompt
### Issue description
When `ConversationDialogFilter.Order == "desc"`, `ConversationService.GetDialogHistory` uses `Take(count)` which assumes dialogs are already sorted newest-first. The sidecar implementation (`BotSharpConversationSideCar.GetConversationDialogs`) ignores the filter and returns dialogs in insertion order, so descending requests can return the wrong subset/order when sidecar is enabled.
### Issue Context
Other repository implementations (FileRepository, MongoRepository) explicitly sort dialogs when `Order == "desc"`, so the sidecar path is now inconsistent.
### Fix Focus Areas
- src/Infrastructure/BotSharp.Core.SideCar/Services/BotSharpConversationSideCar.cs[57-67]
- src/Infrastructure/BotSharp.Core/Conversations/Services/ConversationService.cs[157-194]
### Suggested fix
- In `BotSharpConversationSideCar.GetConversationDialogs`, if `filter?.Order == "desc"`, return `Dialogs.OrderByDescending(d => d.MetaData?.CreatedTime).ToList()`.
- Optionally harden `ConversationService.GetDialogHistory` by sorting based on `CreatedAt` when a filter is provided, so all storage backends (including sidecar) behave correctly.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
962
to
970
| var convDir = Path.Combine(baseDir, file.ConversationId); | ||
| if (!Directory.Exists(convDir)) | ||
| { | ||
| Directory.CreateDirectory(convDir); | ||
| } | ||
|
|
||
| var filesFile = Path.Combine(convDir, CONV_FILES_FILE); | ||
| var json = JsonSerializer.Serialize(file, _options); | ||
| await File.WriteAllTextAsync(filesFile, json); |
There was a problem hiding this comment.
4. Thumbnail path traversal 🐞 Bug ⛨ Security
FileRepository.SaveConversationFiles builds filesystem paths using a user-controlled ConversationId (from the thumbnail API route) without validating or constraining it. A crafted conversationId containing path separators/.. segments can escape the intended base directory and write/delete files outside conversation storage.
Agent Prompt
### Issue description
`SaveConversationFiles` and related methods construct filesystem paths from `ConversationId` without validation. Since `ConversationId` comes from an HTTP route for the thumbnail endpoints, an attacker can use path traversal sequences to escape the intended directory.
### Issue Context
The code uses `Path.Combine(baseDir, conversationId)` and then writes/deletes `conv_files.json`.
### Fix Focus Areas
- src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.File.cs[119-141]
- src/Infrastructure/BotSharp.Core/Repository/FileRepository/FileRepository.Conversation.cs[891-1021]
### Suggested fix
- Add a safe path helper in FileRepository (or shared utility) that:
- Rejects IDs containing `Path.DirectorySeparatorChar`, `Path.AltDirectorySeparatorChar`, or `..` segments.
- Computes `fullPath = Path.GetFullPath(Path.Combine(baseDir, conversationId))` and ensures it starts with `Path.GetFullPath(baseDir) + Path.DirectorySeparatorChar`.
- Use that helper for Get/Save/Delete conversation files.
- Optionally add early validation in the controller to reject invalid `conversationId` (defense-in-depth).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
120
to
141
| [HttpPost("/conversation/{conversationId}/thumbnail")] | ||
| public async Task<bool> SaveConversationThumbnail([FromRoute] string conversationId, [FromBody] ConversationFileRequest request) | ||
| { | ||
| var db = _services.GetRequiredService<IBotSharpRepository>(); | ||
| var result = await db.SaveConversationFiles( | ||
| [ | ||
| new() | ||
| { | ||
| ConversationId = conversationId, | ||
| Thumbnail = request.Thumbnail | ||
| } | ||
| ]); | ||
| return result; | ||
| } | ||
|
|
||
| [HttpDelete("/conversation/{conversationId}/thumbnail")] | ||
| public async Task<bool> DeleteConversationThumbnail([FromRoute] string conversationId) | ||
| { | ||
| var db = _services.GetRequiredService<IBotSharpRepository>(); | ||
| var result = await db.DeleteConversationFiles([conversationId]); | ||
| return result; | ||
| } |
There was a problem hiding this comment.
5. Thumbnail ownership bypass 🐞 Bug ⛨ Security
The new thumbnail save/delete endpoints do not verify the conversation exists or belongs to the caller, unlike other conversation endpoints that scope queries by user for non-admins. Any authenticated user who knows a conversationId can modify/delete another user's thumbnail metadata.
Agent Prompt
### Issue description
Thumbnail modification endpoints bypass the ownership filtering used by other conversation endpoints. This allows unauthorized modification if a user can obtain another conversation's ID.
### Issue Context
`GetConversation` and `GetConversations` scope non-admin queries by `ConversationFilter.UserId`. Thumbnail endpoints currently call the repository directly without checking ownership.
### Fix Focus Areas
- src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.File.cs[119-141]
- src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.cs[50-61]
- src/Infrastructure/BotSharp.OpenAPI/Controllers/Conversation/ConversationController.cs[172-180]
### Suggested fix
- In `SaveConversationThumbnail` and `DeleteConversationThumbnail`:
- Call `userService.IsAdminUser(_user.Id)`.
- If not admin, verify the conversation exists and has `UserId == _user.Id` (e.g., via `IConversationService.GetConversations(new ConversationFilter { Id = conversationId, UserId = _user.Id })`).
- Return 404/403 when not found/unauthorized (prefer IActionResult over bool to represent errors).
- Only after authorization passes, call `SaveConversationFiles/DeleteConversationFiles`.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Oceania2018
approved these changes
Feb 25, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.