Open
Conversation
added 2 commits
April 16, 2026 18:08
Contributor
Review Summary by QodoIntegrate OpenAI Response API with Chat Completion fallback support
WalkthroughsDescription• Integrate OpenAI Response API alongside existing Chat Completion API • Add response API implementation with streaming and non-streaming support • Introduce web search configuration and user location models • Refactor ChatCompletionProvider into partial classes for better code organization • Add file metadata fields (FileName, FileExtension) to file handlers Diagramflowchart LR
A["OpenAI Plugin Settings"] -->|UseResponseApi flag| B["ChatCompletionProvider"]
B -->|true| C["Response API Implementation"]
B -->|false| D["Chat Completion API"]
C -->|streaming| E["InnerGetResponseStreamingAsync"]
C -->|non-streaming| F["InnerGetResponse"]
D -->|streaming| G["InnerGetChatCompletionsStreamingAsync"]
D -->|non-streaming| H["InnerGetChatCompletions"]
I["WebSearchSettings"] -->|config| C
J["WebSearchUserLocation"] -->|location data| I
File Changes1. src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.Chat.cs
|
Contributor
Code Review by Qodo
1. Sync-over-async GetResult() used
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+509
to
+514
| var http = _services.GetRequiredService<IHttpClientFactory>(); | ||
| using var client = http.CreateClient(); | ||
| var bytes = client.GetByteArrayAsync(file.FileUrl).GetAwaiter().GetResult(); | ||
| var binary = BinaryData.FromBytes(bytes); | ||
| var contentPart = ChatMessageContentPart.CreateFilePart(binary, contentType, file.FileFullName); | ||
| contentParts.Add(contentPart); |
Contributor
There was a problem hiding this comment.
1. Sync-over-async getresult() used 📘 Rule violation ☼ Reliability
The new code blocks on HttpClient.GetByteArrayAsync(...).GetAwaiter().GetResult(), which is a sync-over-async pattern that can deadlock and hides async I/O failures under load. This violates the async/I-O reliability requirement at integration boundaries.
Agent Prompt
## Issue description
`HttpClient.GetByteArrayAsync(...).GetAwaiter().GetResult()` is a sync-over-async call that can deadlock and reduces observability of async I/O failures.
## Issue Context
This code runs during chat completion request building and performs external HTTP I/O; blocking here can stall request threads.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.Chat.cs[473-523]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
| var settings = settingsService.GetSetting(Provider, _model); | ||
|
|
||
| // Reasoning | ||
| float? temperature = float.Parse(_state.GetState("temperature", "0.0")); |
Contributor
There was a problem hiding this comment.
2. float.parse without guard 📘 Rule violation ☼ Reliability
The new code parses temperature using float.Parse on conversation state/config input without validation, which can throw on invalid values and crash the request. Integration-boundary inputs should be guarded and default safely instead of throwing.
Agent Prompt
## Issue description
`float.Parse(_state.GetState("temperature", "0.0"))` can throw if the state/config value is missing/invalid.
## Issue Context
Conversation state/config values are integration-boundary inputs and must not crash execution when malformed.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.Chat.cs[592-625]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+507
to
+515
| try | ||
| { | ||
| var http = _services.GetRequiredService<IHttpClientFactory>(); | ||
| using var client = http.CreateClient(); | ||
| var bytes = client.GetByteArrayAsync(file.FileUrl).GetAwaiter().GetResult(); | ||
| var binary = BinaryData.FromBytes(bytes); | ||
| var contentPart = ChatMessageContentPart.CreateFilePart(binary, contentType, file.FileFullName); | ||
| contentParts.Add(contentPart); | ||
| } |
Contributor
There was a problem hiding this comment.
3. Ssrf via fileurl fetch 🐞 Bug ⛨ Security
OpenAI ChatCompletionProvider downloads non-image attachments by fetching BotSharpFile.FileUrl server-side without validation, enabling SSRF if FileUrl is attacker-controlled. Incoming instruct chat requests directly map request FileUrl into BotSharpFile, so a caller can force the server to request arbitrary internal/external URLs.
Agent Prompt
## Issue description
`ChatCompletionProvider.Chat.cs` downloads bytes from `BotSharpFile.FileUrl` for non-image attachments. Because `FileUrl` can come from external request payloads, this enables SSRF (server making requests to attacker-chosen URLs), plus uncontrolled data download.
## Issue Context
At least one public endpoint (`/instruct/chat-completion`) maps `IncomingInstructRequest.Files[].FileUrl` directly into `BotSharpFile.FileUrl`, so callers can supply arbitrary URLs.
## Fix Focus Areas
- src/Plugins/BotSharp.Plugin.OpenAI/Providers/Chat/ChatCompletionProvider.Chat.cs[496-520]
- src/Infrastructure/BotSharp.OpenAPI/Controllers/Instruct/InstructModeController.cs[104-131]
## Recommended fix (one of these approaches)
1. **Do not fetch remote URLs in the provider**:
- Require `FileData` or `FileStorageUrl` for non-image attachments; reject/ignore `FileUrl` for non-image files.
- Alternatively, only allow `FileUrl` pointing to your own storage domain/path (strict allowlist).
2. If remote fetching is required:
- Validate scheme (`https` only), validate host via allowlist, and block private/link-local IP ranges.
- Enforce size limits and timeouts; stream and cap bytes.
- Pass cancellation tokens from conversation/request lifecycle.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
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
1 participant
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.