feat:Add AWS Bedrock to support the leading large language models such as Claude, Llama, Mistral, and others. #5746
Conversation
修改： app/api/auth.ts 新文件： app/api/bedrock.ts 新文件： app/api/bedrock/models.ts 新文件： app/api/bedrock/utils.ts 修改： app/client/api.ts 新文件： app/client/platforms/bedrock.ts 修改： app/components/settings.tsx 修改： app/config/server.ts 修改： app/constant.ts 修改： app/locales/cn.ts 修改： app/locales/en.ts 修改： app/store/access.ts 修改： app/utils.ts 修改： package.json
修改： app/api/bedrock/models.ts 修改： app/api/bedrock/utils.ts 修改： app/client/api.ts 修改： app/client/platforms/bedrock.ts 新文件： app/components/chat-actions.tsx 修改： app/components/chat.module.scss 修改： app/components/chat.tsx 修改： app/constant.ts 新文件： app/icons/document.svg 修改： app/locales/cn.ts 修改： app/locales/en.ts
|
@glayyiyi is attempting to deploy a commit to the NextChat Team on Vercel. A member of the Team first needs to authorize it. |
WalkthroughThis pull request introduces extensive changes to integrate support for the Bedrock API across various components of the application. Key modifications include the addition of new handlers, validation logic, and utility functions for AWS credentials. The routing logic is updated to accommodate Bedrock-specific requests, while the authentication and configuration components are enhanced to manage AWS credentials securely. Additionally, localization files are modified to support Bedrock service settings, and new constants are defined to facilitate API interactions. Changes
Possibly related PRs
Suggested labels
Suggested reviewers
Poem
Warning Rate limit exceeded@glayyiyi has exceeded the limit for the number of commits or files that can be reviewed per hour. Please wait 17 minutes and 22 seconds before requesting another review. ⌛ How to resolve this issue?After the wait time has elapsed, a review can be triggered using the We recommend that you space out your commits to avoid hitting the rate limit. 🚦 How do rate limits work?CodeRabbit enforces hourly rate limits for each developer per organization. Our paid plans have higher rate limits than the trial, open-source and free plans. In all cases, we re-allow further reviews after a brief timeout. Please see our FAQ for further information. Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 37
🧹 Outside diff range and nitpick comments (12)
app/api/auth.ts (1)
57-59: Consider moving header names to constants.The header names should be defined as constants at the top of the file for better maintainability and reusability.
// Add at the top of the file const BEDROCK_HEADERS = { REGION: 'X-Region', ACCESS_KEY: 'X-Access-Key', SECRET_KEY: 'X-Secret-Key', } as const;app/config/server.ts (1)
146-146: Consider regrouping cloud provider checksThe
isBedrockcheck should be grouped with other cloud providers (like Azure) rather than being placed betweenisStabilityandisAzurefor better code organization and maintainability.const isStability = !!process.env.STABILITY_API_KEY; - const isBedrock = !!process.env.BEDROCK_API_KEY; const isAzure = !!process.env.AZURE_URL; const isGoogle = !!process.env.GOOGLE_API_KEY; const isAnthropic = !!process.env.ANTHROPIC_API_KEY; + const isBedrock = !!process.env.BEDROCK_API_KEY; const isTencent = !!process.env.TENCENT_API_KEY;app/constant.ts (1)
312-324: Consider adding model capabilities documentationWhile the models are well-organized by provider, adding comments about each model's capabilities (context window, specialized features, etc.) would help developers choose the appropriate model for their use case.
Example enhancement:
const bedrockModels = [ // Claude Models + // anthropic.claude-3-haiku-20240307-v1:0 - Fast, efficient model with 200K context "anthropic.claude-3-haiku-20240307-v1:0", + // anthropic.claude-3-sonnet-20240229-v1:0 - Balanced performance with 200K context "anthropic.claude-3-sonnet-20240229-v1:0", // ... add descriptions for other modelsapp/locales/en.ts (1)
350-375: Add default endpoint value in the subtitle.The Bedrock endpoint subtitle mentions "Default: " but doesn't specify the actual default value. This information would be helpful for users.
Apply this change:
Title: "AWS Bedrock Endpoint", - SubTitle: "Custom endpoint for AWS Bedrock API. Default: ", + SubTitle: "Custom endpoint for AWS Bedrock API. Default: https://bedrock-runtime.[region].amazonaws.com",app/components/chat.tsx (2)
624-626: Remove commented code.The commented code for storing document content should be either implemented or removed to maintain code cleanliness.
555-635: Consider streaming large files.For better memory management with large files, consider implementing streaming for file processing instead of loading the entire file into memory.
app/components/chat-actions.tsx (1)
183-183: Localize the button text 'Upload Document'The text 'Upload Document' is hard-coded and may not be localized, affecting internationalization. Use localization functions to ensure proper translation.
Apply this diff:
- <div className="text">Upload Document</div> + <div className="text">{locale.uploadDocument}</div>Ensure that the key
uploadDocumentis added to your localization files.app/api/bedrock.ts (3)
68-68: Make the default AWS region configurableHardcoding the default region to
"us-west-2"may not suit all deployments. Consider making the default region configurable through environment variables or application settings.Apply this diff to use an environment variable for the default region:
- const region = req.headers.get("X-Region") || "us-west-2"; + const DEFAULT_REGION = process.env.AWS_DEFAULT_REGION || "us-west-2"; + const region = req.headers.get("X-Region") || DEFAULT_REGION;
95-97: Validate request body fieldsEnsure that all required fields in the request body are present and valid before proceeding. This prevents runtime errors and improves robustness.
Apply this diff to add validation for
modelId:const body = (await req.json()) as ConverseRequest; + if (!body.modelId) { + throw new Error("modelId is required in the request body"); + } const { modelId } = body;
44-50: Add unit tests for authentication and request handlingImplementing unit tests for the
authfunction and request handlers ensures that authentication logic works correctly and helps prevent future regressions.Would you like assistance in generating unit tests for these functions?
app/api/bedrock/models.ts (1)
200-216: Handle empty content arrays appropriatelyIn the
convertContentToAWSfunction, when no valid content blocks are found, a default text block with an empty string is returned (lines 211-213). Consider whether returning an empty array or handling this case differently would be more appropriate to avoid unintended empty messages.Suggested change:
// If no valid blocks, provide a default text block if (blocks.length === 0) { - return [{ text: "" }]; + return []; }Ensure that downstream functions can handle an empty content array without errors.
app/client/platforms/bedrock.ts (1)
171-174: Simplify Response Text ConstructionThe current logic for constructing
responseTextis unnecessarily complex, involving an array for a single message.Simplify the code as follows:
- let responseText = ""; - const responseTexts = [responseText]; - responseTexts.push(Locale.Error.Unauthorized); - responseText = responseTexts.join("\n\n"); + const responseText = Locale.Error.Unauthorized;This streamlines the code by directly assigning the error message to
responseText.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
⛔ Files ignored due to path filters (1)
app/icons/document.svgis excluded by!**/*.svg
📒 Files selected for processing (18)
app/api/[provider]/[...path]/route.ts(2 hunks)app/api/auth.ts(1 hunks)app/api/bedrock.ts(1 hunks)app/api/bedrock/models.ts(1 hunks)app/api/bedrock/utils.ts(1 hunks)app/client/api.ts(8 hunks)app/client/platforms/bedrock.ts(1 hunks)app/components/chat-actions.tsx(1 hunks)app/components/chat.module.scss(2 hunks)app/components/chat.tsx(3 hunks)app/components/settings.tsx(2 hunks)app/config/server.ts(3 hunks)app/constant.ts(6 hunks)app/locales/cn.ts(2 hunks)app/locales/en.ts(2 hunks)app/store/access.ts(5 hunks)app/utils.ts(1 hunks)package.json(1 hunks)
🧰 Additional context used
📓 Learnings (2)
app/components/chat-actions.tsx (1)
Learnt from: Dakai
PR: ChatGPTNextWeb/ChatGPT-Next-Web#5585
File: app/components/chat.tsx:1482-1528
Timestamp: 2024-10-06T07:59:31.943Z
Learning: In `app/components/chat.tsx`, maintain similarity between `uploadDocument()` and `uploadImage()` functions to aid future maintenance.
app/components/chat.tsx (1)
Learnt from: Dakai
PR: ChatGPTNextWeb/ChatGPT-Next-Web#5585
File: app/components/chat.tsx:1482-1528
Timestamp: 2024-10-06T07:59:31.943Z
Learning: In `app/components/chat.tsx`, maintain similarity between `uploadDocument()` and `uploadImage()` functions to aid future maintenance.
🪛 Biome
app/client/platforms/bedrock.ts
[error] 345-345: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
app/components/settings.tsx
[error] 980-980: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 996-996: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1012-1012: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1028-1028: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (23)
app/api/[provider]/[...path]/route.ts (3)
4-4: LGTM: Import statement follows established patterns.
The import for the Bedrock handler follows the consistent pattern used for other providers.
24-25: LGTM: Switch case implementation is consistent.
The Bedrock case follows the established pattern of other providers, correctly passing through the request and params.
Also applies to: 32-32
Line range hint 53-71: Verify AWS credentials handling and region compatibility.
Since this is running on Edge Runtime and Bedrock requires AWS credentials:
- Ensure AWS credentials are properly handled in the Edge environment
- Verify that the preferred regions align with Bedrock service availability
✅ Verification successful
AWS credentials and region handling are properly implemented for Edge Runtime
The verification shows that:
- AWS credentials are securely handled via request headers (X-Region, X-Access-Key, X-Secret-Key) and properly validated in the auth middleware
- Region handling is implemented correctly with a default fallback to "us-west-2"
- The BedrockRuntimeClient is properly instantiated with the credentials and region in the Edge environment
- Comprehensive error handling is in place for credential and region-related issues
The current implementation aligns with Edge Runtime requirements and AWS Bedrock service constraints. No changes are needed.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for AWS credential handling and region-related code
# Check for AWS credential handling in the codebase
echo "Checking AWS credential handling..."
rg -l "AWS" --type ts
# Look for region configurations
echo "Checking region configurations..."
rg "region|AWS_REGION" --type ts
# Check for environment variable handling
echo "Checking environment variable handling..."
rg "process\.env\.AWS|getEnvVar.*AWS" --type ts
Length of output: 1793
Script:
#!/bin/bash
# Let's examine the AWS credential handling in Bedrock-related files more closely
# Check Bedrock client implementation
echo "Checking Bedrock client implementation..."
rg -A 10 "BedrockClient" app/client/platforms/bedrock.ts
# Check AWS credential handling in auth.ts
echo "Checking auth implementation..."
rg -A 10 "export async function" app/api/auth.ts
# Check Bedrock API implementation
echo "Checking Bedrock API implementation..."
rg -A 10 "export async function" app/api/bedrock.ts
# Check available Bedrock regions
echo "Checking Bedrock regions configuration..."
rg -A 10 "availableRegions|bedrockRegions"
Length of output: 846
Script:
#!/bin/bash
# Let's check the actual implementation details
# Check how credentials are handled in the Bedrock API
echo "Checking Bedrock API file content..."
cat app/api/bedrock.ts
# Check auth implementation
echo "Checking auth file content..."
cat app/api/auth.ts
# Check if there's any Edge-specific credential handling
echo "Checking Edge runtime configurations..."
rg -A 5 "runtime.*=.*edge"
Length of output: 13839
package.json (2)
54-54: LGTM!
The zustand dependency is properly pinned to version 4.3.8.
55-55: 🛠️ Refactor suggestion
Consider AWS SDK versioning and dependencies.
A few recommendations regarding the AWS SDK integration:
- Consider pinning the version instead of using caret (^) to prevent potential breaking changes from minor version updates.
- Version 3.679.0 is quite old. Consider updating to the latest version for security fixes and features.
- Based on the PR objectives mentioning AWS authentication and document uploads, you might need additional AWS SDK packages:
@aws-sdk/credential-providersfor authentication@aws-sdk/client-s3for document uploads
Consider updating to:
- "@aws-sdk/client-bedrock-runtime": "^3.679.0"
+ "@aws-sdk/client-bedrock-runtime": "3.499.0",
+ "@aws-sdk/credential-providers": "3.499.0",
+ "@aws-sdk/client-s3": "3.499.0"app/api/auth.ts (2)
61-65: LGTM! Proper credential masking in logs.
The implementation correctly masks sensitive AWS credentials in logs, which is a good security practice.
55-76: Verify Bedrock authentication integration.
Let's verify that this authentication block is properly integrated with the rest of the Bedrock-related changes mentioned in the PR.
✅ Verification successful
Let me gather more information about the Bedrock handler integration to ensure proper authentication flow.
Let me check one final aspect to verify the complete authentication flow.
Based on the comprehensive verification results, I can now provide the final response:
✓ Bedrock authentication is properly integrated
The authentication implementation is correctly integrated with the Bedrock handler and shows consistent header usage across the codebase:
- Auth check is properly performed in
bedrock.tsusingauth(req, ModelProvider.Bedrock) - AWS credential headers (X-Region, X-Access-Key, X-Secret-Key) are consistently used in:
- Frontend:
app/client/api.tsandapp/client/platforms/bedrock.ts - Backend:
app/api/auth.tsandapp/api/bedrock.ts
- Frontend:
- The handler includes proper error handling and credential validation
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify Bedrock authentication integration
# Test 1: Check if Bedrock handler uses this auth function
echo "Checking Bedrock handler integration..."
rg -l "auth.*ModelProvider\.Bedrock" app/api/
# Test 2: Check for consistent header usage across files
echo "Checking header usage consistency..."
rg "X-(Region|Access-Key|Secret-Key)" app/
Length of output: 1199
Script:
#!/bin/bash
# Check the implementation of Bedrock handler to verify auth integration
echo "Checking Bedrock handler implementation..."
rg -A 5 "export.*bedrock" app/api/bedrock.ts
# Check auth function usage in bedrock.ts
echo "Checking auth function usage..."
rg "auth.*\(" app/api/bedrock.ts
# Check ModelProvider enum definition
echo "Checking ModelProvider definition..."
rg "enum ModelProvider" app/
Length of output: 462
Script:
#!/bin/bash
# Check the ModelProvider enum values and Bedrock handler implementation
echo "Checking ModelProvider enum values..."
rg -A 10 "enum ModelProvider" app/constant.ts
# Check the complete bedrock.ts implementation
echo "Checking complete Bedrock handler..."
cat app/api/bedrock.ts
# Check auth function implementation
echo "Checking auth function implementation..."
rg -A 10 "auth.*=.*async" app/api/auth.ts
Length of output: 8734
app/store/access.ts (2)
7-7: LGTM! Constants follow established patterns.
The addition of Bedrock-related constants follows the same pattern as other providers, maintaining consistency in the codebase.
Also applies to: 30-30
209-209: LGTM! Authorization check properly integrated.
The Bedrock validation is correctly integrated into the authorization flow, maintaining consistency with other providers.
app/config/server.ts (1)
15-18: LGTM: Environment variables properly declared
The Bedrock-specific environment variables are correctly declared as optional strings and follow the established pattern in the file.
app/utils.ts (1)
288-290: LGTM! The Bedrock plugin visibility logic aligns with Anthropic's implementation.
The added condition correctly mirrors Anthropic's plugin visibility rules for Claude models, maintaining consistent behavior across providers.
Let's verify the consistent handling of Claude models across providers:
✅ Verification successful
Plugin visibility logic is correctly implemented for Claude models
The verification confirms that both Anthropic and Bedrock providers have identical conditions for handling Claude models in the showPlugins function:
- Both check for
!model.includes("claude-2") - Both return
truewhen the condition is met - The implementation maintains consistent behavior across providers
The code in app/constant.ts also shows proper model definitions for various Claude versions, supporting this implementation.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Verify consistent handling of Claude models across Anthropic and Bedrock providers
# Expected: Both providers should have similar conditions for Claude models
# Search for Claude model handling across the codebase
rg -A 5 "claude-2" --type ts
Length of output: 1764
app/components/chat.module.scss (2)
78-87: LGTM: Well-structured icon styles!
The new .icon class implementation follows best practices:
- Uses flexbox for proper centering
- Maintains consistent dimensions with parent container
- Follows the existing BEM methodology
408-408: LGTM: Proper structural fix
The closing brace is correctly placed, maintaining proper SCSS nesting structure.
app/constant.ts (2)
121-121: LGTM!
The Bedrock provider is correctly added to both enums, maintaining consistency with other providers.
Also applies to: 135-135
524-534: LGTM!
The Bedrock models are correctly integrated into DEFAULT_MODELS with appropriate provider configuration and consistent sorting.
app/locales/cn.ts (1)
84-84: LGTM!
The translation for document upload functionality is properly implemented.
app/locales/en.ts (1)
85-85: LGTM: Document upload translation added.
The translation for document upload functionality is clear and consistent with existing entries.
app/components/settings.tsx (1)
1753-1753: LGTM: Clean integration of Bedrock configuration.
The Bedrock configuration component is properly integrated into the provider list, maintaining consistency with other provider configurations.
app/components/chat.tsx (2)
551-552: LGTM: Clean provider flag implementation.
The flag is well-named and follows single responsibility principle.
668-675: LGTM: Clean UI implementation for document upload.
The document upload button implementation follows existing patterns and maintains consistency with the image upload feature.
app/api/bedrock/utils.ts (1)
357-400: LGTM
The convertBedrockResponseToMultimodal function effectively translates the Bedrock response into the expected MultimodalContent format.
app/api/bedrock/models.ts (1)
243-245: Verify the logic that skips messages with the same role
At lines 243-245, the code skips messages if they have the same role as the previous message. This could lead to loss of important messages, particularly if multiple consecutive messages from the same role are expected. Ensure that this behavior aligns with the application requirements.
Explanation:
// Skip if same role as previous message
if (idx > 0 && curr.role === nonSystemMessages[idx - 1].role) {
return acc;
}Consider whether messages from the same role should be merged or all messages should be included. If merging is intended, you might concatenate the content of messages with the same role.
app/client/platforms/bedrock.ts (1)
271-273: Verify Parameter Naming Consistency
The inferenceConfig object uses camelCase for maxTokens, whereas modelConfig uses snake_case max_tokens. This inconsistency might cause misalignment with the API expectations.
Check if the Bedrock API expects parameters in camelCase or snake_case. If camelCase is required, ensure consistency:
- maxTokens: modelConfig.max_tokens,
+ maxTokens: modelConfig.maxTokens,Adjust the naming in modelConfig or map the properties accordingly to match the API specification.
app/locales/cn.ts
Outdated
| }, | ||
| Endpoint: { | ||
| Title: "AWS Bedrock Endpoint", | ||
| SubTitle: "Custom endpoint for AWS Bedrock API. Default: ", |
There was a problem hiding this comment.
Add the default endpoint value in the SubTitle.
The SubTitle for the Bedrock endpoint appears to be missing the default endpoint value after "Default: ".
Update the SubTitle to include the default endpoint:
- SubTitle: "Custom endpoint for AWS Bedrock API. Default: ",
+ SubTitle: "Custom endpoint for AWS Bedrock API. Default: bedrock.{region}.amazonaws.com",📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| SubTitle: "Custom endpoint for AWS Bedrock API. Default: ", | |
| SubTitle: "Custom endpoint for AWS Bedrock API. Default: bedrock.{region}.amazonaws.com", |
|
1.一个pr不要包含太多不同的功能 |
|
删除： app/api/bedrock/models.ts 删除： app/api/bedrock/utils.ts 修改： app/client/platforms/bedrock.ts 删除： app/components/chat-actions.tsx 修改： app/components/chat.tsx 修改： app/constant.ts 删除： app/icons/document.svg
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (6)
app/api/bedrock.ts (2)
125-227: Split complex content conversion logic into smaller functionsThe
convertContentToAWSBlockfunction is complex with multiple nested conditions. Consider splitting it into smaller, focused functions for each content type.+function convertImageContent(imageUrl: string): ContentBlock | null { + const base64Match = imageUrl.match(/^data:image\/([a-zA-Z]*);base64,([^"]*)/); + if (!base64Match) return null; + + const format = base64Match[1].toLowerCase(); + if (!["png", "jpeg", "gif", "webp"].includes(format)) return null; + + validateImageSize(base64Match[2]); + return { + image: { + format: format as "png" | "jpeg" | "gif" | "webp", + source: { + bytes: Uint8Array.from(Buffer.from(base64Match[2], "base64")), + }, + }, + }; +}
444-538: Consider implementing stream backpressure handlingThe
ReadableStreamimplementation doesn't handle backpressure, which could lead to memory issues with large responses. Consider implementing thepullmethod to handle backpressure.app/client/platforms/bedrock.ts (4)
27-27: Remove console log to prevent exposing sensitive informationLogging the entire response object may inadvertently expose sensitive information. Consider removing or restricting this log statement in production code.
Apply this diff:
- console.log("[Response] bedrock response: ", res);
100-100: Remove unused variable 'format'The variable
formatis assigned but never used, which can be removed to clean up the code.Apply this diff:
- const format = base64Match[1].toLowerCase();
278-305: Avoid logging request bodies containing sensitive informationEven though binary data is masked, logging the entire request body may still expose sensitive information. Consider removing or sanitizing this log statement to avoid potential security risks.
Apply this diff:
- console.log( - "[Bedrock] Request body:", - JSON.stringify( - { - ...requestBody, - messages: requestBody.messages.map((msg) => ({ - ...msg, - content: Array.isArray(msg.content) - ? msg.content.map((c) => ({ - type: c.type, - ...(c.document - ? { - document: { - format: c.document.format, - name: c.document.name, - }, - } - : {}), - ...(c.image_url ? { image_url: { url: "[BINARY]" } } : {}), - ...(c.text ? { text: c.text } : {}), - })) - : msg.content, - })), - }, - null, - 2, - ), - );
158-403: Refactor 'chat' method to improve maintainabilityThe
chatmethod is quite lengthy and contains complex nested logic, which can make it difficult to read and maintain. Consider breaking it down into smaller, reusable helper functions to enhance code clarity and reusability.🧰 Tools
🪛 Biome
[error] 345-345: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.(lint/suspicious/noAssignInExpressions)
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/api/bedrock.ts(1 hunks)app/client/platforms/bedrock.ts(1 hunks)app/constant.ts(6 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/constant.ts
🧰 Additional context used
🪛 Biome
app/client/platforms/bedrock.ts
[error] 345-345: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (2)
app/api/bedrock.ts (2)
387-396: Previous review comment about not exposing internal error details is still valid.
542-545: Previous review comment about adding security headers is still valid.
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Outside diff range and nitpick comments (2)
app/client/platforms/bedrock.ts (2)
95-101: Avoid using 'as any' to maintain type safetyAssigning an array to
messages[i]and casting it toanycan compromise type safety and may lead to runtime errors or unexpected behavior.Instead of replacing
messages[i]with an array, consider inserting the fake assistant message directly into themessagesarray. Here's how you can refactor the code:for (let i = 0; i < messages.length - 1; i++) { const message = messages[i]; const nextMessage = messages[i + 1]; if (keys.includes(message.role) && keys.includes(nextMessage.role)) { - messages[i] = [ - message, - { - role: "assistant", - content: ";", - }, - ] as any; + messages.splice(i + 1, 0, { + role: "assistant", + content: ";", + }); + i++; // Increment i to skip the inserted assistant message } }This modification maintains the array's type integrity and avoids unnecessary type casting.
54-54: Limit logging of sensitive informationLogging entire responses or error objects may expose sensitive data, which is a security risk, especially in production environments.
Modify the logging statements to provide necessary information without revealing sensitive details:
// Line 54 -console.log("[Response] bedrock response: ", res); +console.log("[Bedrock] Response received"); // Line 301 -console.error("[Bedrock] Chat error:", e); +console.error("[Bedrock] Chat error:", e.message);This approach maintains useful logging while protecting sensitive data.
Also applies to: 301-301
修改： app/api/auth.ts 修改： app/api/bedrock.ts 修改： app/client/api.ts 修改： app/client/platforms/bedrock.ts 修改： app/components/settings.tsx 修改： app/config/server.ts 修改： app/constant.t
There was a problem hiding this comment.
Actionable comments posted: 17
🧹 Outside diff range and nitpick comments (3)
app/client/platforms/bedrock.ts (2)
20-28: Consider adding validation for media typesThe
MultiBlockContenttype allows any string formedia_type. Consider using a union type to restrict it to supported MIME types for better type safety.export type MultiBlockContent = { type: "image" | "text"; source?: { type: string; - media_type: string; + media_type: "image/jpeg" | "image/png" | "image/gif" | "image/webp"; data: string; }; text?: string; };
35-39: Document system message handlingThe
ClaudeMappermaps 'system' role to 'user' which might be non-intuitive. Consider adding a comment explaining why this mapping is necessary for Claude compatibility.const ClaudeMapper = { assistant: "assistant", user: "user", + // Claude doesn't support system messages directly, so we map them to user messages system: "user", } as const;app/constant.ts (1)
233-235: Add ExampleEndpoint to Bedrock constant object.Other provider objects include an ExampleEndpoint property. Consider adding it for consistency:
export const Bedrock = { ChatPath: "converse", + ExampleEndpoint: BEDROCK_BASE_URL, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (9)
.env.template(1 hunks)app/api/auth.ts(1 hunks)app/api/bedrock.ts(1 hunks)app/client/api.ts(8 hunks)app/client/platforms/bedrock.ts(1 hunks)app/components/settings.tsx(2 hunks)app/config/server.ts(3 hunks)app/constant.ts(7 hunks)app/store/access.ts(5 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/api/auth.ts
🧰 Additional context used
🪛 Biome
app/components/settings.tsx
[error] 980-980: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 996-996: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1012-1012: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1028-1028: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (10)
.env.template (1)
69-70: LGTM! Improved formatting.
The added newline enhances readability and maintains consistent spacing between sections.
app/store/access.ts (1)
7-7: LGTM: URL configuration follows established patterns.
The Bedrock URL configuration follows the same pattern as other providers, correctly handling both app and API path scenarios.
Also applies to: 30-30
app/config/server.ts (1)
147-147: LGTM!
The Bedrock flag follows the same pattern as other provider flags.
app/client/api.ts (4)
15-15: LGTM: BedrockApi import
The import is correctly placed alongside other platform-specific API imports.
141-143: LGTM: Bedrock provider initialization
The Bedrock provider case is properly integrated into the switch statement, following the established pattern.
355-356: LGTM: Bedrock client API initialization
The Bedrock provider case is properly integrated into the switch statement, maintaining consistency with other providers.
324-347: Verify error handling for AWS credentials
Ensure proper error handling is in place when AWS credentials are invalid or missing.
✅ Verification successful
Let me gather more information about error handling by searching for validation and error handling patterns in the codebase.
Let me search for more general error handling patterns and validation checks.
Based on the analysis of the codebase, I can now provide a final response:
AWS credentials validation is properly handled
The codebase has sufficient error handling for AWS credentials:
- Client-side validation through
ensure()function inapp/store/access.tschecks for required AWS credentials (region, access key, secret key) - Server-side validation in
app/api/bedrock.tsreturns 401 error when credentials are missing - Error handling for Bedrock API calls with proper error messages and status codes
- Stream error handling with detailed error information including original status codes and messages
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for AWS credentials validation and error handling
# Test: Search for AWS credentials validation
rg -A 5 "awsAccessKey|awsSecretKey|awsRegion" --type ts
# Test: Search for error handling related to AWS credentials
ast-grep --pattern 'try {
$$$
aws$$$
$$$
} catch (error) {
$$$
}'
Length of output: 4725
Script:
#!/bin/bash
# Search for validation of AWS credentials
ast-grep --pattern 'ensure($_, ["awsAccessKey", "awsSecretKey", "awsRegion"])'
# Search for error handling in Bedrock API calls
rg -B 3 -A 5 "catch.*error.*Bedrock" --type ts
# Search for error messages related to AWS credentials
rg "Error.*aws|aws.*Error" --type ts
Length of output: 267
Script:
#!/bin/bash
# Search for error handling in Bedrock-related files
rg -B 3 -A 5 "catch|throw|error" app/api/bedrock.ts
# Search for validation functions
ast-grep --pattern 'function $_($$_) {
$$$
if (!$_) {
$$$
}
$$$
}'
# Check for error messages in locale files
rg "bedrock.*error|aws.*error" --type json
Length of output: 52025
app/constant.ts (2)
316-330: LGTM! Well-organized model definitions.
The Bedrock models are well-organized by model families (Claude, Meta Llama, Mistral) with clear version numbers and helpful comments.
530-540: LGTM! Consistent provider configuration.
The Bedrock provider configuration in DEFAULT_MODELS follows the established pattern and maintains consistency with other providers.
app/components/settings.tsx (1)
1753-1753: LGTM: Bedrock component integration.
The Bedrock configuration component is properly integrated into the provider list, maintaining consistency with other provider implementations.
修改： app/client/platforms/bedrock.ts
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (6)
app/api/bedrock.ts (3)
13-13: Consider using an enum for allowed pathsUsing a Set with string literals reduces type safety. Consider using an enum to ensure type safety and maintainability.
-const ALLOWED_PATH = new Set(["converse"]); +enum AllowedPath { + Converse = "converse" +} +const ALLOWED_PATH = new Set(Object.values(AllowedPath));
24-24: Add validation for modelId formatThe modelId field should be validated to ensure it matches AWS Bedrock model formats.
+const VALID_MODEL_PREFIXES = ['anthropic.', 'amazon.', 'meta.']; + +function isValidModelId(modelId: string): boolean { + return VALID_MODEL_PREFIXES.some(prefix => modelId.startsWith(prefix)); +}
95-239: Add rate limiting and timeout handlingThe handler function lacks important production safeguards:
- No rate limiting to prevent abuse
- No timeout handling for long-running requests
Consider implementing:
- Rate limiting middleware
- Request timeout handling
Example implementation:
import { rateLimit } from 'express-rate-limit'; import { setTimeout } from 'timers/promises'; // Rate limiting middleware const limiter = rateLimit({ windowMs: 15 * 60 * 1000, // 15 minutes max: 100 // limit each IP to 100 requests per windowMs }); // Timeout wrapper const withTimeout = async (promise: Promise<any>, timeoutMs: number) => { const timeout = setTimeout(timeoutMs); try { return await Promise.race([promise, timeout.then(() => { throw new Error('Request timeout'); })]); } finally { timeout.clear(); } };app/client/platforms/bedrock.ts (3)
21-34: Add JSDoc documentation for type definitionsThe
MultiBlockContentandAnthropicMessagetypes would benefit from JSDoc documentation explaining their purpose and usage.Add documentation like this:
+/** + * Represents a block of content that can be either text or image. + * Used for multimodal messages in the Bedrock API. + */ export type MultiBlockContent = { type: "image" | "text"; source?: { type: string; media_type: string; data: string; }; text?: string; }; +/** + * Represents a message in the Claude model format. + * Maps internal roles to Claude-specific roles. + */ export type AnthropicMessage = { role: (typeof ClaudeMapper)[keyof typeof ClaudeMapper]; content: string | MultiBlockContent[]; };
43-45: Enhance error messages for unimplemented methodsThe error messages for unimplemented methods should provide more context about the current limitations and potential alternatives.
speech(options: SpeechOptions): Promise<ArrayBuffer> { - throw new Error("Speech not implemented for Bedrock."); + throw new Error("Speech synthesis is not supported by the Bedrock API. Consider using Amazon Polly for text-to-speech functionality."); } usage(): Promise<LLMUsage> { - throw new Error("Method not implemented."); + throw new Error("Usage tracking is not currently supported for Bedrock. Use AWS CloudWatch for monitoring Bedrock API usage."); } models(): Promise<LLMModel[]> { - throw new Error("Method not implemented."); + throw new Error("Dynamic model listing is not supported. Available models are defined in the Bedrock console."); }Also applies to: 292-297
58-64: Simplify model configuration initializationThe current configuration merge could be simplified using object destructuring.
-const modelConfig = { - ...useAppConfig.getState().modelConfig, - ...useChatStore.getState().currentSession().mask.modelConfig, - ...{ - model: options.config.model, - }, -}; +const { modelConfig: baseConfig } = useAppConfig.getState(); +const { modelConfig: maskConfig } = useChatStore.getState().currentSession().mask; +const modelConfig = { + ...baseConfig, + ...maskConfig, + model: options.config.model, +};
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Outside diff range and nitpick comments (2)
app/client/platforms/bedrock.ts (2)
24-33: Enhance type safety of ClaudeMapper and AnthropicMessageThe type definitions could be more robust with literal types and proper type guards.
Consider this improvement:
-const ClaudeMapper = { +const ClaudeMapper = { assistant: "assistant", user: "user", system: "user", -} as const; +} as const satisfies Record<string, "assistant" | "user">; -export type AnthropicMessage = { +export type AnthropicMessage = { - role: (typeof ClaudeMapper)[keyof typeof ClaudeMapper]; + role: typeof ClaudeMapper[keyof typeof ClaudeMapper]; content: string | MultiBlockContent[]; + // Add runtime type guard + isAnthropicMessage: (message: unknown) => message is AnthropicMessage; };
285-293: Mark unimplemented methods appropriatelyThe usage and models methods should clearly indicate they're not implemented, similar to the speech method.
Consider this improvement:
async usage() { - return { - used: 0, - total: 0, - }; + throw new Error("Usage tracking not implemented for Bedrock."); } async models() { - return []; + throw new Error("Model listing not implemented for Bedrock."); }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/client/platforms/bedrock.ts(1 hunks)
🔇 Additional comments (1)
app/client/platforms/bedrock.ts (1)
36-38: LGTM: Appropriate error handling for unimplemented method
The speech method correctly throws an error indicating the functionality is not implemented.
There was a problem hiding this comment.
Actionable comments posted: 5
🧹 Outside diff range and nitpick comments (1)
app/api/bedrock.ts (1)
118-121: Add rate limiting to protect the APIThe handler lacks rate limiting, which could lead to excessive API usage.
Consider implementing rate limiting using a middleware or a service like Redis. Example implementation:
import rateLimit from 'express-rate-limit' const limiter = rateLimit({ windowMs: 15 * 60 * 1000, // 15 minutes max: 100 // limit each IP to 100 requests per windowMs }); // Apply before the main handler export const bedrockHandler = limiter(async (req, params) => { return handle(req, params); });
修改： app/utils.ts
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (2)
app/api/bedrock.ts (2)
43-43: Translate comment to EnglishComments should be in English for better maintainability and collaboration.
- // llama和mistral模型不支持工具调用 + // Llama and Mistral models do not support tool calls
126-129: Add rate limiting protectionThe endpoint lacks rate limiting protection which could lead to abuse.
Consider implementing rate limiting using Redis or a similar solution to protect the API from abuse. Example implementation:
import rateLimit from 'express-rate-limit' import RedisStore from 'rate-limit-redis' const limiter = rateLimit({ store: new RedisStore({ // ... redis config }), windowMs: 15 * 60 * 1000, // 15 minutes max: 100 // limit each IP to 100 requests per windowMs });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/api/bedrock.ts(1 hunks)app/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/utils.ts
glayyiyi
changed the title
|
Based on the feedback from the previous PR, I have revised and optimized this PR. The scope has been narrowed and focused as suggested, with the following key improvements:
Enhancements for tool use in both the server-side and client-side implementations for Bedrock using the Bedrock Converse API. Here is a summary of the key changes made:
|
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (2)
app/api/bedrock.ts (1)
42-45: Enhance model support documentation and validationThe function has several issues:
- The comment should be in English for consistency
- The model check is too broad and might include future incompatible models
function supportsToolUse(modelId: string): boolean { - // llama和mistral模型不支持工具调用 + // Llama and Mistral models don't support tool calls + const SUPPORTED_MODELS = [ + 'claude-3-sonnet', + 'claude-3-haiku', + 'claude-3-opus' + ]; - return modelId.toLowerCase().includes("claude-3"); + return SUPPORTED_MODELS.some(model => + modelId.toLowerCase().includes(model)); }app/client/platforms/bedrock.ts (1)
24-33: Document the role mapping behavior for system messages.The
ClaudeMappermaps the "system" role to "user", which is a significant behavior that should be documented. This mapping affects how system prompts are handled.Add a comment explaining this behavior:
const ClaudeMapper = { assistant: "assistant", user: "user", + // Claude doesn't support system messages directly, so we map them to user messages system: "user", } as const;
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (13)
app/api/[provider]/[...path]/route.ts(2 hunks)app/api/auth.ts(1 hunks)app/api/bedrock.ts(1 hunks)app/client/api.ts(8 hunks)app/client/platforms/bedrock.ts(1 hunks)app/components/settings.tsx(2 hunks)app/config/server.ts(3 hunks)app/constant.ts(8 hunks)app/locales/cn.ts(2 hunks)app/locales/en.ts(2 hunks)app/store/access.ts(5 hunks)app/utils.ts(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (6)
- app/api/[provider]/[...path]/route.ts
- app/api/auth.ts
- app/config/server.ts
- app/constant.ts
- app/utils.ts
- package.json
🧰 Additional context used
🪛 Biome
app/components/settings.tsx
[error] 981-981: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 997-997: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1013-1013: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1029-1029: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (15)
app/store/access.ts (3)
7-7: LGTM: URL configuration follows established pattern.
The Bedrock URL configuration follows the same pattern as other providers, maintaining consistency in the codebase.
Also applies to: 31-31
213-213: LGTM: Authorization check properly integrated.
The Bedrock validation is correctly integrated into the authorization flow, maintaining consistency with other providers.
160-163: 🛠️ Refactor suggestion
Enhance Bedrock credential validation.
The current validation only checks for the presence of credentials. Consider implementing more robust validation:
isValidBedrock() {
+ const state = get();
+
+ if (state.awsCognitoUser) {
+ // Add Cognito-specific validation when implemented
+ return true;
+ }
+
+ // Basic format validation
+ const isValidRegion = /^[a-z]{2}-[a-z]+-\d{1,2}$/.test(state.awsRegion);
+ const isValidAccessKey = /^[A-Z0-9]{20}$/.test(state.awsAccessKey);
+ const isValidSecretKey = /^[A-Za-z0-9/+=]{40}$/.test(state.awsSecretKey);
+
+ if (!isValidRegion || !isValidAccessKey || !isValidSecretKey) {
+ console.warn('[Access] Invalid AWS credentials format');
+ return false;
+ }
+
+ return true;
- return ensure(get(), ["awsAccessKey", "awsSecretKey", "awsRegion"]);
}Likely invalid or redundant comment.
app/api/bedrock.ts (2)
35-40: 🛠️ Refactor suggestion
Improve type safety for tools configuration
The tools array uses loose typing which could lead to runtime errors. Consider defining a proper interface for the tool configuration.
tools?: {
name: string;
description?: string;
- input_schema: any;
+ input_schema: {
+ type: string;
+ properties?: Record<string, unknown>;
+ required?: string[];
+ additionalProperties?: boolean;
+ [key: string]: unknown;
+ };
+ version?: string;
}[];Likely invalid or redundant comment.
15-21:
Improve error handling in decrypt function
The current implementation silently fails by returning an empty string, which could mask critical issues and make debugging difficult. Consider throwing an error with a descriptive message or implementing proper error logging.
function decrypt(str: string): string {
try {
return Buffer.from(str, "base64").toString().split("").reverse().join("");
} catch (error) {
- return "";
+ console.error("[Bedrock] Failed to decrypt:", error);
+ throw new Error("Failed to decrypt credentials");
}
}Likely invalid or redundant comment.
app/client/platforms/bedrock.ts (1)
49-56: Consider validating model configuration.
The model configuration is merged from multiple sources without validation of required fields.
Add validation for essential configuration parameters:
const modelConfig = {
...useAppConfig.getState().modelConfig,
...useChatStore.getState().currentSession().mask.modelConfig,
...{
model: options.config.model,
},
};
+if (!modelConfig.max_tokens || !modelConfig.temperature || !modelConfig.top_p) {
+ throw new Error("Invalid model configuration: missing required parameters");
+}app/client/api.ts (2)
15-15: LGTM: Import statement properly placed
The BedrockApi import is correctly placed with other platform-specific imports.
142-144: LGTM: Consistent implementation of Bedrock provider
The Bedrock provider is properly integrated into both the ClientApi constructor and getClientApi function, following the established pattern for other providers.
Also applies to: 363-364
app/locales/cn.ts (2)
84-84: LGTM!
The translation for UploadDocument is consistent with the existing translations and correctly represents the document upload functionality.
346-371: Translate Bedrock section to Chinese and add default endpoint.
The Bedrock section needs to be translated to Chinese for consistency with the rest of the localization file. Also, add the default endpoint value.
Apply this diff to translate the strings and add the default endpoint:
Bedrock: {
Region: {
- Title: "AWS Region",
- SubTitle: "The AWS region where Bedrock service is located",
- Placeholder: "us-west-2",
+ Title: "AWS 区域",
+ SubTitle: "Bedrock 服务所在的 AWS 区域",
+ Placeholder: "us-west-2",
},
AccessKey: {
- Title: "AWS Access Key ID",
- SubTitle: "Your AWS access key ID for Bedrock service",
- Placeholder: "AKIA...",
+ Title: "AWS 访问密钥 ID",
+ SubTitle: "用于 Bedrock 服务的 AWS 访问密钥 ID",
+ Placeholder: "AKIA...",
},
SecretKey: {
- Title: "AWS Secret Access Key",
- SubTitle: "Your AWS secret access key for Bedrock service",
- Placeholder: "****",
+ Title: "AWS 私有访问密钥",
+ SubTitle: "用于 Bedrock 服务的 AWS 私有访问密钥",
+ Placeholder: "****",
},
SessionToken: {
- Title: "AWS Session Token (Optional)",
- SubTitle: "Your AWS session token if using temporary credentials",
- Placeholder: "Optional session token",
+ Title: "AWS 会话令牌（可选）",
+ SubTitle: "如果使用临时凭证，请输入 AWS 会话令牌",
+ Placeholder: "可选的会话令牌",
},
Endpoint: {
- Title: "AWS Bedrock Endpoint",
- SubTitle: "Custom endpoint for AWS Bedrock API. Default: ",
+ Title: "AWS Bedrock 接口地址",
+ SubTitle: "自定义 AWS Bedrock API 接口地址。默认：bedrock.{region}.amazonaws.com",
},
},app/locales/en.ts (2)
85-85: LGTM! Clear and consistent translation added for document upload functionality.
The new translation entry follows existing patterns and is appropriately placed alongside similar upload functionality.
350-375: LGTM! Well-structured AWS Bedrock configuration translations.
The translations for AWS Bedrock configuration:
- Follow the established pattern of other provider configurations
- Include clear titles and helpful subtitles
- Properly handle security-sensitive fields
- Provide appropriate placeholder examples
app/components/settings.tsx (3)
967-969: LGTM - Component declaration follows established patterns.
The Bedrock configuration component is correctly implemented using conditional rendering based on the selected provider.
1795-1795: LGTM - Provider integration is correct.
The Bedrock configuration component is properly integrated into the provider list, maintaining consistency with other providers.
1018-1034: Add session token validation.
Consider adding basic validation for the session token when provided.
Let's check if session tokens are handled consistently across the codebase:
#!/bin/bash
# Description: Check session token handling patterns
rg "sessionToken|session.*token" --type ts🧰 Tools
🪛 Biome
[error] 1029-1029: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (1)
app/locales/cn.ts (1)
350-350: Maintain consistent language style in validation messagesThe validation messages are in Chinese while their corresponding titles and subtitles are in English. For consistency, consider using the same language style across related fields.
Apply this diff to maintain consistency:
- Invalid: "无效的 AWS 区域格式。示例：us-west-2", + Invalid: "Invalid AWS Region format. Example: us-west-2", - Invalid: "无效的 AWS Access Key 格式。必须为20个字符。", + Invalid: "Invalid AWS Access Key format. Must be 20 characters.", - Invalid: "无效的 AWS Secret Key 格式。必须为40个字符。", + Invalid: "Invalid AWS Secret Key format. Must be 40 characters.",Or alternatively, wait until the entire section is translated to Chinese as suggested in the previous review.
Also applies to: 356-356, 362-362
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/client/api.ts(7 hunks)app/locales/cn.ts(1 hunks)app/locales/en.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/locales/en.ts
app/client/api.ts
Outdated
| if (isBedrock) { | ||
| // Secure encryption of AWS credentials using the new encryption utility | ||
| headers["X-Region"] = encrypt(accessStore.awsRegion); | ||
| headers["X-Access-Key"] = encrypt(accessStore.awsAccessKey); | ||
| headers["X-Secret-Key"] = encrypt(accessStore.awsSecretKey); | ||
|
|
||
| if (accessStore.awsSessionToken) { | ||
| headers["X-Session-Token"] = encrypt(accessStore.awsSessionToken); | ||
| } | ||
| } else { |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling and code structure for AWS credentials
The current implementation could be enhanced in several ways:
- Add proper error handling for missing credentials
- Extract the AWS credential header logic into a separate function for better maintainability
Here's a suggested refactoring:
+ function getBedrockHeaders(): Record<string, string> {
+ const { awsRegion, awsAccessKey, awsSecretKey, awsSessionToken } = accessStore;
+
+ if (!awsRegion || !awsAccessKey || !awsSecretKey) {
+ throw new Error(
+ "AWS credentials (region, access key, and secret key) are required for Bedrock"
+ );
+ }
+
+ const headers: Record<string, string> = {
+ "X-Region": encrypt(awsRegion),
+ "X-Access-Key": encrypt(awsAccessKey),
+ "X-Secret-Key": encrypt(awsSecretKey),
+ };
+
+ if (awsSessionToken) {
+ headers["X-Session-Token"] = encrypt(awsSessionToken);
+ }
+
+ return headers;
+ }
if (isBedrock) {
- headers["X-Region"] = encrypt(accessStore.awsRegion);
- headers["X-Access-Key"] = encrypt(accessStore.awsAccessKey);
- headers["X-Secret-Key"] = encrypt(accessStore.awsSecretKey);
-
- if (accessStore.awsSessionToken) {
- headers["X-Session-Token"] = encrypt(accessStore.awsSessionToken);
- }
+ Object.assign(headers, getBedrockHeaders());
} else {📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (isBedrock) { | |
| // Secure encryption of AWS credentials using the new encryption utility | |
| headers["X-Region"] = encrypt(accessStore.awsRegion); | |
| headers["X-Access-Key"] = encrypt(accessStore.awsAccessKey); | |
| headers["X-Secret-Key"] = encrypt(accessStore.awsSecretKey); | |
| if (accessStore.awsSessionToken) { | |
| headers["X-Session-Token"] = encrypt(accessStore.awsSessionToken); | |
| } | |
| } else { | |
| function getBedrockHeaders(): Record<string, string> { | |
| const { awsRegion, awsAccessKey, awsSecretKey, awsSessionToken } = accessStore; | |
| if (!awsRegion || !awsAccessKey || !awsSecretKey) { | |
| throw new Error( | |
| "AWS credentials (region, access key, and secret key) are required for Bedrock" | |
| ); | |
| } | |
| const headers: Record<string, string> = { | |
| "X-Region": encrypt(awsRegion), | |
| "X-Access-Key": encrypt(awsAccessKey), | |
| "X-Secret-Key": encrypt(awsSecretKey), | |
| }; | |
| if (awsSessionToken) { | |
| headers["X-Session-Token"] = encrypt(awsSessionToken); | |
| } | |
| return headers; | |
| } | |
| if (isBedrock) { | |
| Object.assign(headers, getBedrockHeaders()); | |
| } else { |
There was a problem hiding this comment.
Actionable comments posted: 9
🧹 Outside diff range and nitpick comments (8)
app/client/platforms/bedrock.ts (2)
18-24: Improve type safety in ToolDefinition interfaceThe
parametersproperty uses theanytype, which bypasses TypeScript's type checking. Consider using a more specific type or JSON schema type definition.interface ToolDefinition { function?: { name: string; description?: string; - parameters?: any; + parameters?: { + type: 'object'; + properties: Record<string, unknown>; + required?: string[]; + }; }; }
39-260: Consider breaking down the chat method for better maintainabilityThe
chatmethod is handling multiple responsibilities including:
- Message preprocessing and formatting
- AWS credentials validation
- Request construction
- Streaming and non-streaming logic
- Tool execution
Consider extracting these into separate methods for better maintainability and testability.
Example structure:
class BedrockApi { private formatMessages(messages: Message[]): FormattedMessage[] { /* ... */ } private constructRequestBody(messages: FormattedMessage[], config: Config) { /* ... */ } private handleToolExecution(/* ... */) { /* ... */ } private handleStreamingResponse(/* ... */) { /* ... */ } private handleNonStreamingResponse(/* ... */) { /* ... */ } async chat(options: ChatOptions) { const formattedMessages = this.formatMessages(options.messages); const requestBody = this.constructRequestBody(formattedMessages, options.config); return options.config.stream ? this.handleStreamingResponse(requestBody, options) : this.handleNonStreamingResponse(requestBody, options); } }app/components/ui-lib.tsx (4)
14-14: Consider relocating AWS-specific utilitiesThe
maskSensitiveValueutility seems to be AWS-specific but is being used in a general-purpose UI component. Consider moving it to a more generic location likeutils/securityorutils/maskto maintain better separation of concerns.
273-276: Improve TypeScript type definitionsThe
maskWhenShowprop lacks proper TypeScript documentation. Consider adding JSDoc comments to explain its purpose and behavior.export function PasswordInput( props: HTMLProps<HTMLInputElement> & { aria?: string; + /** When true, masks the value even when the password is visible */ maskWhenShow?: boolean; }, )
294-298: Memoize the onChange handlerThe
handleChangefunction is recreated on every render. Consider usinguseCallbackto optimize performance.- const handleChange = (e: React.ChangeEvent<HTMLInputElement>) => { + const handleChange = useCallback((e: React.ChangeEvent<HTMLInputElement>) => { if (props.onChange) { props.onChange(e); } - }; + }, [props.onChange]);
Line range hint
576-607: Improve type safety and event handling in FullScreen componentThe component has several type safety and cleanup issues that should be addressed:
Apply these improvements:
- export function FullScreen(props: any) { + interface FullScreenProps { + children: React.ReactNode; + right?: number; + top?: number; + [key: string]: any; + } + + export function FullScreen(props: FullScreenProps) { const { children, right = 10, top = 10, ...rest } = props; - const ref = useRef<HTMLDivElement>(); + const ref = useRef<HTMLDivElement>(null); const [fullScreen, setFullScreen] = useState(false); const toggleFullscreen = useCallback(() => { - if (!document.fullscreenElement) { - ref.current?.requestFullscreen(); + if (!document.fullscreenElement && ref.current) { + ref.current.requestFullscreen().catch(console.error); } else { - document.exitFullscreen(); + document.exitFullscreen().catch(console.error); } }, []); useEffect(() => { - const handleScreenChange = (e: any) => { + const handleScreenChange = (e: Event) => { if (e.target === ref.current) { setFullScreen(!!document.fullscreenElement); } }; + + // Handle escape key + const handleKeyDown = (e: KeyboardEvent) => { + if (e.key === 'Escape' && fullScreen) { + setFullScreen(false); + } + }; document.addEventListener("fullscreenchange", handleScreenChange); + document.addEventListener("keydown", handleKeyDown); return () => { document.removeEventListener("fullscreenchange", handleScreenChange); + document.removeEventListener("keydown", handleKeyDown); }; - }, []); + }, [fullScreen]);app/api/bedrock.ts (1)
180-181: Remove redundant catch blockThe catch block at lines 180-181 simply rethrows the caught error without any additional handling. This catch block is unnecessary and can be removed to simplify the code.
Apply this diff to remove the redundant catch block:
} catch (e) { - throw e; } finally { clearTimeout(timeoutId); }🧰 Tools
🪛 Biome
[error] 180-180: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the catch clause.(lint/complexity/noUselessCatch)
app/utils/aws.ts (1)
45-49: EnsuremaskSensitiveValueappropriately masks short values.Currently, if the value is 4 characters or fewer, it's returned unmasked. Consider masking the entire value for short sensitive data to enhance security.
Apply this diff to mask short values entirely:
if (!value) return ""; -if (value.length <= 4) return value; -return "*".repeat(value.length - 4) + value.slice(-4); +return "*".repeat(value.length);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (8)
app/api/bedrock.ts(1 hunks)app/client/api.ts(7 hunks)app/client/platforms/bedrock.ts(1 hunks)app/components/ui-lib.tsx(4 hunks)app/constant.ts(8 hunks)app/store/access.ts(4 hunks)app/utils/aws.ts(1 hunks)package.json(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- package.json
🧰 Additional context used
🪛 Biome
app/api/bedrock.ts
[error] 180-180: The catch clause that only rethrows the original error is useless.
An unnecessary catch clause can be confusing.
Unsafe fix: Remove the catch clause.
(lint/complexity/noUselessCatch)
🔇 Additional comments (12)
app/store/access.ts (2)
223-223: LGTM: Bedrock authorization check properly integrated.
The integration of isValidBedrock() into the authorization chain follows the established pattern and maintains consistency with other provider checks.
119-124:
Add field documentation and implement secure credential handling.
The AWS credentials storage needs additional security measures and documentation:
-
Security:
- Implement encryption before storing AWS credentials
- Consider using AWS Cognito for authentication instead of storing raw credentials
- Add a warning about security implications of storing credentials in browser storage
-
Documentation:
- Add JSDoc comments explaining each field's purpose
- Document which fields are required vs optional (e.g., awsSessionToken)
- Include usage examples in comments
Let's verify if there's any existing encryption implementation we can reuse:
app/client/api.ts (1)
325-334:
Add validation for required AWS credentials
The code should validate that required AWS credentials are present before attempting to use them. This prevents potential runtime errors and provides better error messages.
Add validation before setting the headers:
if (isBedrock) {
+ if (!accessStore.awsRegion || !accessStore.awsAccessKey || !accessStore.awsSecretKey) {
+ throw new Error("AWS credentials (region, access key, and secret key) are required for Bedrock");
+ }
+
headers["X-Region"] = encrypt(accessStore.awsRegion);
headers["X-Access-Key"] = encrypt(accessStore.awsAccessKey);
headers["X-Secret-Key"] = encrypt(accessStore.awsSecretKey);
if (accessStore.awsSessionToken) {
headers["X-Session-Token"] = encrypt(accessStore.awsSessionToken);
}
} else {Likely invalid or redundant comment.
app/constant.ts (5)
243-248: LGTM! Well-structured Bedrock configuration
The Bedrock configuration object is well-organized with:
- A simplified chat path that will be expanded in bedrock.ts
- API version constant
- Configurable endpoint function with a default region
329-367: Comprehensive model coverage with clear categorization
The Bedrock models array is well-organized with:
- Clear categorization by provider (Amazon, Cohere, Claude, Meta, Mistral, AI21)
- Latest model versions included (e.g., Claude 3, Llama 3)
- Consistent version formatting
593-603: LGTM! Consistent model mapping implementation
The Bedrock models are correctly integrated into DEFAULT_MODELS with:
- Consistent structure with other providers
- Proper provider metadata
- Correct sorting (13) to maintain order
329-367: Consider version management strategy for Bedrock models
The hardcoded model versions might need updates as new versions are released. Consider implementing a version management strategy:
- Move model definitions to a separate configuration file
- Implement a version update mechanism
- Add documentation for version update procedures
15-16: 🛠️ Refactor suggestion
Consider using the Bedrock.getEndpoint function instead of hardcoding the base URL
The BEDROCK_BASE_URL constant duplicates the endpoint logic that's already defined in the Bedrock.getEndpoint function. This could lead to inconsistencies if the endpoint format changes.
-export const BEDROCK_BASE_URL =
- "https://bedrock-runtime.us-west-2.amazonaws.com";
+export const BEDROCK_BASE_URL = Bedrock.getEndpoint();Likely invalid or redundant comment.
app/api/bedrock.ts (1)
205-207: Avoid exposing internal error details to clients
Returning detailed error messages (e.message) to the client can expose sensitive information about the server's internals, potentially aiding an attacker. Consider returning a generic error message instead.
app/utils/aws.ts (3)
89-138: Review custom URI encoding functions for correctness and maintenance.
The custom URI encoding functions handle complex scenarios that might be covered by existing libraries. Using well-tested libraries or built-in functions can reduce bugs and improve maintainability.
Consider refactoring using encodeURI and encodeURIComponent where appropriate, and ensure special cases are adequately handled.
156-157:
Ensure AWS signature uses the correct timestamp format.
The amzDate should be in the format YYYYMMDD'T'HHMMSS'Z' to comply with AWS Signature Version 4. The current format may be missing the 'T' and 'Z' designators.
Apply this diff to correct the timestamp format:
const now = new Date();
-const amzDate = now.toISOString().replace(/[:-]|\.\d{3}/g, "");
+const amzDate = now.toISOString().replace(/[:\-]|\.\d{3}/g, '').replace(/Z$/, 'Z');Likely invalid or redundant comment.
212-216: Verify that signing key derivation aligns with AWS specifications.
Ensure that the process of deriving the signing key strictly follows AWS guidelines to prevent authentication errors.
Run the following script to compare your signing key derivation with AWS's Python SDK:
✅ Verification successful
Let me analyze the signing key derivation implementation by searching through the codebase.
Let me try a different approach to find the signing key implementation.
Based on the code found, I can now verify the signing key derivation implementation. Let me check the AWS documentation reference.
Based on the code analysis, I can now provide a verification of the signing key derivation implementation.
AWS Signature V4 signing key derivation is correctly implemented
The implementation in getSigningKey follows AWS Signature Version 4 specification exactly:
- Uses correct key derivation sequence:
AWS4<secret>→dateStamp→region→service→aws4_request - Employs HMAC-SHA256 for all hashing steps (via CryptoJS/hmac-sha256)
- Maintains proper string encoding and hex output format
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Compare signing key derivation with AWS SDK implementation.
# Note: This requires AWS CLI and Python SDK for accurate comparison.
# Expected Result: The derived keys should match.
# Placeholder for testing code or steps.
echo "Please manually verify the signing key derivation against AWS documentation or SDK."
Length of output: 176
Script:
#!/bin/bash
# Search for getSigningKey implementation
ast-grep --pattern 'function getSigningKey' -A 20
Length of output: 51
Script:
#!/bin/bash
# Search for getSigningKey implementation using ripgrep
rg -A 10 "getSigningKey" --type ts
# Also search for hmac implementation as it's used in the signature calculation
rg -A 5 "hmac" --type ts
Length of output: 3473
Script:
#!/bin/bash
# Search for any AWS-related documentation or comments in the codebase
rg -B 2 -A 2 "AWS" --type ts --type md
# Also check for any test files related to AWS signing
fd -e test.ts -e spec.ts -X rg -l "getSigningKey|hmac"
Length of output: 5913
app/client/platforms/bedrock.ts
Outdated
| } catch (e) { | ||
| return ""; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve error handling in streaming callback
The catch block silently swallows errors and returns an empty string. This could hide important issues during development and debugging.
} catch (e) {
- return "";
+ console.error("[Bedrock Stream] Failed to process chunk:", e);
+ throw new Error("Failed to process stream chunk");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (e) { | |
| return ""; | |
| } | |
| } catch (e) { | |
| console.error("[Bedrock Stream] Failed to process chunk:", e); | |
| throw new Error("Failed to process stream chunk"); | |
| } |
| useEffect(() => { | ||
| if (maskWhenShow && visible && props.value) { | ||
| setDisplayValue(maskSensitiveValue(props.value as string)); | ||
| } else { | ||
| setDisplayValue(props.value as string); | ||
| } | ||
| }, [visible, props.value, maskWhenShow]); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Optimize effect dependencies and add user feedback
The effect has potential performance and UX issues:
- Using
props.valuein dependencies could cause unnecessary re-renders - The sudden masking when toggling visibility might confuse users
Consider these improvements:
+ // Store the original value in a ref to avoid re-renders
+ const valueRef = useRef(props.value as string);
+
useEffect(() => {
+ valueRef.current = props.value as string;
+ // Add a small delay when masking to make the transition visible to users
if (maskWhenShow && visible && props.value) {
- setDisplayValue(maskSensitiveValue(props.value as string));
+ const timer = setTimeout(() => {
+ setDisplayValue(maskSensitiveValue(valueRef.current));
+ }, 300);
+ return () => clearTimeout(timer);
} else {
- setDisplayValue(props.value as string);
+ setDisplayValue(valueRef.current);
}
- }, [visible, props.value, maskWhenShow]);
+ }, [visible, maskWhenShow]);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| useEffect(() => { | |
| if (maskWhenShow && visible && props.value) { | |
| setDisplayValue(maskSensitiveValue(props.value as string)); | |
| } else { | |
| setDisplayValue(props.value as string); | |
| } | |
| }, [visible, props.value, maskWhenShow]); | |
| const valueRef = useRef(props.value as string); | |
| useEffect(() => { | |
| valueRef.current = props.value as string; | |
| // Add a small delay when masking to make the transition visible to users | |
| if (maskWhenShow && visible && props.value) { | |
| const timer = setTimeout(() => { | |
| setDisplayValue(maskSensitiveValue(valueRef.current)); | |
| }, 300); | |
| return () => clearTimeout(timer); | |
| } else { | |
| setDisplayValue(valueRef.current); | |
| } | |
| }, [visible, maskWhenShow]); |
app/api/bedrock.ts
Outdated
| if (eventMatch) { | ||
| return JSON.parse(eventMatch[1]); | ||
| } | ||
| } catch (innerError) {} |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid silently swallowing exceptions in catch blocks
At line 22, the catch block for innerError is empty, which means any errors during the parsing attempt will be silently ignored. This can make debugging difficult and may hide unexpected errors. Consider logging the error or handling it appropriately.
Apply this diff to handle the error:
} catch (innerError) {
+ console.error("Error parsing event data:", innerError);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (innerError) {} | |
| } catch (innerError) { | |
| console.error("Error parsing event data:", innerError); | |
| } |
app/api/bedrock.ts
Outdated
| function validateRequest(body: any, modelId: string): void { | ||
| if (!modelId) throw new Error("Model ID is required"); | ||
|
|
||
| if (modelId.startsWith("anthropic.claude")) { | ||
| if ( | ||
| !body.anthropic_version || | ||
| body.anthropic_version !== "bedrock-2023-05-31" | ||
| ) { | ||
| throw new Error("anthropic_version must be 'bedrock-2023-05-31'"); | ||
| } | ||
| if (typeof body.max_tokens !== "number" || body.max_tokens < 0) { | ||
| throw new Error("max_tokens must be a positive number"); | ||
| } | ||
| if (modelId.startsWith("anthropic.claude-3")) { | ||
| if (!Array.isArray(body.messages)) | ||
| throw new Error("messages array is required for Claude 3"); | ||
| } else if (typeof body.prompt !== "string") { | ||
| throw new Error("prompt is required for Claude 2 and earlier"); | ||
| } | ||
| } else if (modelId.startsWith("meta.llama")) { | ||
| if (!body.prompt) throw new Error("Llama requires a prompt"); | ||
| } else if (modelId.startsWith("mistral.mistral")) { | ||
| if (!Array.isArray(body.messages)) | ||
| throw new Error("Mistral requires a messages array"); | ||
| } else if (modelId.startsWith("amazon.titan")) { | ||
| if (!body.inputText) throw new Error("Titan requires inputText"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Add validation for unsupported model IDs
Currently, the validateRequest function does not handle cases where the modelId does not match any of the specified prefixes. This could lead to processing requests for unsupported models without proper validation. Consider adding an else clause to throw an error for unrecognized modelId values.
Apply this diff to ensure unsupported models are rejected:
} else if (modelId.startsWith("amazon.titan")) {
if (!body.inputText) throw new Error("Titan requires inputText");
}
+ else {
+ throw new Error(`Unsupported model ID: ${modelId}`);
+ }
}Committable suggestion skipped: line range outside the PR's diff.
app/api/bedrock.ts
Outdated
| async function requestBedrock(req: NextRequest) { | ||
| const controller = new AbortController(); | ||
| const awsRegion = req.headers.get("X-Region") ?? ""; | ||
| const awsAccessKey = req.headers.get("X-Access-Key") ?? ""; | ||
| const awsSecretKey = req.headers.get("X-Secret-Key") ?? ""; | ||
| const awsSessionToken = req.headers.get("X-Session-Token"); | ||
| const modelId = req.headers.get("X-Model-Id") ?? ""; | ||
|
|
||
| if (!awsRegion || !awsAccessKey || !awsSecretKey || !modelId) { | ||
| throw new Error("Missing required AWS credentials or model ID"); | ||
| } | ||
|
|
||
| const decryptedAccessKey = decrypt(awsAccessKey); | ||
| const decryptedSecretKey = decrypt(awsSecretKey); | ||
| const decryptedSessionToken = awsSessionToken | ||
| ? decrypt(awsSessionToken) | ||
| : undefined; | ||
|
|
||
| if (!decryptedAccessKey || !decryptedSecretKey) { | ||
| throw new Error("Failed to decrypt AWS credentials"); | ||
| } | ||
|
|
||
| const endpoint = `https://bedrock-runtime.${awsRegion}.amazonaws.com/model/${modelId}/invoke-with-response-stream`; |
There was a problem hiding this comment.
Validate awsRegion and modelId to prevent SSRF attacks
The endpoint URL at line 117 is constructed using user-provided awsRegion and modelId values from the request headers. Without proper validation, this could allow for Server-Side Request Forgery (SSRF) attacks by injecting malicious values. It's important to validate and sanitize these inputs before using them to construct the endpoint URL.
Apply this diff to add validation for awsRegion and modelId:
const awsRegion = req.headers.get("X-Region") ?? "";
const modelId = req.headers.get("X-Model-Id") ?? "";
+ const validRegions = [
+ "us-east-1", "us-east-2", "us-west-1", "us-west-2",
+ "eu-west-1", "eu-west-2", "eu-west-3", "eu-central-1",
+ "ap-northeast-1", "ap-northeast-2", "ap-southeast-1", "ap-southeast-2",
+ "ap-south-1", "sa-east-1", "ca-central-1", "cn-north-1", "cn-northwest-1"
+ ];
+ if (!validRegions.includes(awsRegion)) {
+ throw new Error(`Invalid AWS region: ${awsRegion}`);
+ }
+ const modelIdPattern = /^[a-zA-Z0-9.\-_]+$/;
+ if (!modelIdPattern.test(modelId)) {
+ throw new Error(`Invalid model ID: ${modelId}`);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| async function requestBedrock(req: NextRequest) { | |
| const controller = new AbortController(); | |
| const awsRegion = req.headers.get("X-Region") ?? ""; | |
| const awsAccessKey = req.headers.get("X-Access-Key") ?? ""; | |
| const awsSecretKey = req.headers.get("X-Secret-Key") ?? ""; | |
| const awsSessionToken = req.headers.get("X-Session-Token"); | |
| const modelId = req.headers.get("X-Model-Id") ?? ""; | |
| if (!awsRegion || !awsAccessKey || !awsSecretKey || !modelId) { | |
| throw new Error("Missing required AWS credentials or model ID"); | |
| } | |
| const decryptedAccessKey = decrypt(awsAccessKey); | |
| const decryptedSecretKey = decrypt(awsSecretKey); | |
| const decryptedSessionToken = awsSessionToken | |
| ? decrypt(awsSessionToken) | |
| : undefined; | |
| if (!decryptedAccessKey || !decryptedSecretKey) { | |
| throw new Error("Failed to decrypt AWS credentials"); | |
| } | |
| const endpoint = `https://bedrock-runtime.${awsRegion}.amazonaws.com/model/${modelId}/invoke-with-response-stream`; | |
| async function requestBedrock(req: NextRequest) { | |
| const controller = new AbortController(); | |
| const awsRegion = req.headers.get("X-Region") ?? ""; | |
| const awsAccessKey = req.headers.get("X-Access-Key") ?? ""; | |
| const awsSecretKey = req.headers.get("X-Secret-Key") ?? ""; | |
| const awsSessionToken = req.headers.get("X-Session-Token"); | |
| const modelId = req.headers.get("X-Model-Id") ?? ""; | |
| const validRegions = [ | |
| "us-east-1", "us-east-2", "us-west-1", "us-west-2", | |
| "eu-west-1", "eu-west-2", "eu-west-3", "eu-central-1", | |
| "ap-northeast-1", "ap-northeast-2", "ap-southeast-1", "ap-southeast-2", | |
| "ap-south-1", "sa-east-1", "ca-central-1", "cn-north-1", "cn-northwest-1" | |
| ]; | |
| if (!validRegions.includes(awsRegion)) { | |
| throw new Error(`Invalid AWS region: ${awsRegion}`); | |
| } | |
| const modelIdPattern = /^[a-zA-Z0-9.\-_]+$/; | |
| if (!modelIdPattern.test(modelId)) { | |
| throw new Error(`Invalid model ID: ${modelId}`); | |
| } | |
| if (!awsRegion || !awsAccessKey || !awsSecretKey || !modelId) { | |
| throw new Error("Missing required AWS credentials or model ID"); | |
| } | |
| const decryptedAccessKey = decrypt(awsAccessKey); | |
| const decryptedSecretKey = decrypt(awsSecretKey); | |
| const decryptedSessionToken = awsSessionToken | |
| ? decrypt(awsSessionToken) | |
| : undefined; | |
| if (!decryptedAccessKey || !decryptedSecretKey) { | |
| throw new Error("Failed to decrypt AWS credentials"); | |
| } | |
| const endpoint = `https://bedrock-runtime.${awsRegion}.amazonaws.com/model/${modelId}/invoke-with-response-stream`; |
app/utils/aws.ts
Outdated
| } catch (error) { | ||
| console.error("Encryption failed:", error); | ||
| return data; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid logging errors that may expose sensitive information.
Logging errors during encryption might inadvertently reveal sensitive data or internal cryptographic errors. It's safer to handle errors silently or provide generic error messages.
[security_issue]
Apply this diff to remove detailed error logging:
return AES.encrypt(data, SECRET_KEY).toString();
} catch (error) {
- console.error("Encryption failed:", error);
return data;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| } catch (error) { | |
| console.error("Encryption failed:", error); | |
| return data; | |
| } catch (error) { | |
| return data; |
app/utils/aws.ts
Outdated
| const SECRET_KEY = | ||
| process.env.ENCRYPTION_KEY || | ||
| "your-secret-key-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; | ||
| if (!SECRET_KEY || SECRET_KEY.length < 32) { | ||
| throw new Error( | ||
| "ENCRYPTION_KEY environment variable must be set with at least 32 characters", | ||
| ); | ||
| } |
There was a problem hiding this comment.
Avoid hardcoding default SECRET_KEY; enforce environment variable usage.
Hardcoding a default secret key poses a security risk. It's essential to ensure that the SECRET_KEY is always provided via a secure environment variable to prevent unauthorized access.
Apply this diff to remove the hardcoded default and enforce the use of the environment variable:
const SECRET_KEY =
- process.env.ENCRYPTION_KEY ||
- "your-secret-key-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx";
+ process.env.ENCRYPTION_KEY;
if (!SECRET_KEY || SECRET_KEY.length < 32) {
throw new Error(
"ENCRYPTION_KEY environment variable must be set with at least 32 characters",
);
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const SECRET_KEY = | |
| process.env.ENCRYPTION_KEY || | |
| "your-secret-key-xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx"; | |
| if (!SECRET_KEY || SECRET_KEY.length < 32) { | |
| throw new Error( | |
| "ENCRYPTION_KEY environment variable must be set with at least 32 characters", | |
| ); | |
| } | |
| const SECRET_KEY = | |
| process.env.ENCRYPTION_KEY; | |
| if (!SECRET_KEY || SECRET_KEY.length < 32) { | |
| throw new Error( | |
| "ENCRYPTION_KEY environment variable must be set with at least 32 characters", | |
| ); | |
| } |
修改： app/client/platforms/bedrock.ts 修改： app/constant.ts
There was a problem hiding this comment.
Actionable comments posted: 10
🧹 Outside diff range and nitpick comments (1)
app/constant.ts (1)
329-351: Well-organized model definitionsThe Bedrock models are well-organized by provider (Amazon, Claude, Meta, Mistral) with clear versioning. Consider adding comments to document the capabilities or use cases for each model category.
const bedrockModels = [ + // Amazon Titan Models - General purpose text generation "amazon.titan-text-express-v1", // ... + // Claude Models - Advanced reasoning and analysis "anthropic.claude-3-haiku-20240307-v1:0", // ... + // Meta Llama Models - Open source foundation models "us.meta.llama3-1-8b-instruct-v1:0", // ... + // Mistral Models - Efficient language models "mistral.mistral-7b-instruct-v0:2", // ... ];
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/api/bedrock.ts(1 hunks)app/client/platforms/bedrock.ts(1 hunks)app/constant.ts(8 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/api/bedrock.ts
[error] 47-47: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 47-47: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 103-103: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 103-103: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (6)
app/api/bedrock.ts (3)
1-4: LGTM!
The imports and constant definitions are appropriate. Using Set for ALLOWED_PATH provides efficient lookups.
50-52:
Add error logging in catch block
The catch block should log the error to help with debugging.
Apply this diff:
} catch (innerError) {
+ console.error("Error in fallback parsing:", innerError);
}Likely invalid or redundant comment.
151-181:
Add AWS region validation
The function should validate the AWS region to prevent potential SSRF attacks.
Add region validation at the start of the function:
function validateRequest(body: any, modelId: string): void {
if (!modelId) throw new Error("Model ID is required");
+
+ const validRegions = [
+ "us-east-1", "us-east-2", "us-west-1", "us-west-2",
+ "eu-west-1", "eu-west-2", "eu-central-1",
+ "ap-northeast-1", "ap-southeast-1", "ap-southeast-2"
+ ];
+
+ if (!validRegions.includes(awsRegion)) {
+ throw new Error(`Invalid AWS region: ${awsRegion}`);
+ }Likely invalid or redundant comment.
app/constant.ts (3)
56-56: LGTM! Consistent enum additions
The additions to ApiPath, ServiceProvider, and ModelProvider enums follow the established naming patterns and maintain consistency with other providers.
Also applies to: 125-125, 150-150
243-248: Well-structured Bedrock configuration!
The Bedrock configuration object is well-implemented with:
- Simplified chat path for better path management
- API version specification
- Configurable region support through getEndpoint function
577-587: LGTM! Consistent model registration
The Bedrock models are properly integrated into DEFAULT_MODELS following the established pattern, with correct provider information and sequential sorting.
app/api/bedrock.ts
Outdated
|
|
||
| if (text) { | ||
| // Clean up any control characters or invalid JSON characters | ||
| text = text.replace(/[\x00-\x1F\x7F-\x9F]/g, ""); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Use a safer method to clean control characters
Using control characters in regex is flagged as potentially unsafe.
Apply this diff:
- text = text.replace(/[\x00-\x1F\x7F-\x9F]/g, "");
+ text = text.replace(/[^\x20-\x7E]/g, "");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| text = text.replace(/[\x00-\x1F\x7F-\x9F]/g, ""); | |
| text = text.replace(/[^\x20-\x7E]/g, ""); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 103-103: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 103-103: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
app/api/bedrock.ts
Outdated
| const parsed = parseEventData(value); | ||
| if (!parsed) continue; | ||
|
|
||
| console.log("Parsed response:", JSON.stringify(parsed, null, 2)); |
There was a problem hiding this comment.
Remove sensitive data logging
Logging the parsed response could expose sensitive information.
Apply this diff:
- console.log("Parsed response:", JSON.stringify(parsed, null, 2));
+ console.log("Received parsed response from Bedrock");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("Parsed response:", JSON.stringify(parsed, null, 2)); | |
| console.log("Received parsed response from Bedrock"); |
app/api/bedrock.ts
Outdated
| } | ||
| return parsed.body || parsed; | ||
| } catch (e) { | ||
| console.error("Error parsing event data:", e); |
There was a problem hiding this comment.
Avoid logging sensitive response data
Logging the entire error object could expose sensitive information in the response.
Apply this diff:
- console.error("Error parsing event data:", e);
+ console.error("Error parsing event data:", e instanceof Error ? e.message : "Unknown error");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error("Error parsing event data:", e); | |
| console.error("Error parsing event data:", e instanceof Error ? e.message : "Unknown error"); |
app/api/bedrock.ts
Outdated
| console.error("Handler error:", e); | ||
| return NextResponse.json( | ||
| { error: true, msg: e instanceof Error ? e.message : "Unknown error" }, | ||
| { status: 500 }, | ||
| ); |
There was a problem hiding this comment.
Sanitize error messages in response
Exposing raw error messages in the response could reveal sensitive information.
Apply this diff:
console.error("Handler error:", e);
return NextResponse.json(
- { error: true, msg: e instanceof Error ? e.message : "Unknown error" },
+ { error: true, msg: "An internal server error occurred" },
{ status: 500 },
);📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.error("Handler error:", e); | |
| return NextResponse.json( | |
| { error: true, msg: e instanceof Error ? e.message : "Unknown error" }, | |
| { status: 500 }, | |
| ); | |
| console.error("Handler error:", e); | |
| return NextResponse.json( | |
| { error: true, msg: "An internal server error occurred" }, | |
| { status: 500 }, | |
| ); |
app/client/platforms/bedrock.ts
Outdated
| "X-Access-Key": accessStore.awsAccessKey, | ||
| "X-Secret-Key": accessStore.awsSecretKey, |
There was a problem hiding this comment.
Critical Security Issue: AWS Credentials Exposure
Transmitting AWS credentials (especially the secret key) in HTTP headers poses a significant security risk. These credentials could be exposed through:
- Server logs
- Network traffic inspection
- Browser caching
Consider implementing secure credential handling:
- Use AWS STS to generate temporary credentials
- Implement server-side credential management
- Use secure session tokens instead of raw credentials
- "X-Access-Key": accessStore.awsAccessKey,
- "X-Secret-Key": accessStore.awsSecretKey,
+ "X-Session-Token": await generateTemporaryCredentials(),Committable suggestion skipped: line range outside the PR's diff.
app/client/platforms/bedrock.ts
Outdated
| try { | ||
| JSON.parse(currentToolArgs); | ||
| if (index >= 0 && index < runTools.length) { | ||
| runTools[index].function!.arguments = currentToolArgs; | ||
| } | ||
| } catch (e) {} |
There was a problem hiding this comment.
Improve Tool Argument Parsing and Validation
The current implementation has several issues:
- Empty catch block silently ignores parsing errors
- No validation of parsed JSON structure
- Direct assignment of potentially invalid arguments
Apply this fix:
try {
- JSON.parse(currentToolArgs);
+ const parsedArgs = JSON.parse(currentToolArgs);
+ if (typeof parsedArgs !== 'object' || parsedArgs === null) {
+ throw new Error('Tool arguments must be a valid JSON object');
+ }
if (index >= 0 && index < runTools.length) {
- runTools[index].function!.arguments = currentToolArgs;
+ runTools[index].function!.arguments = JSON.stringify(parsedArgs);
}
- } catch (e) {}
+ } catch (e) {
+ console.error('Failed to parse tool arguments:', e);
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| try { | |
| JSON.parse(currentToolArgs); | |
| if (index >= 0 && index < runTools.length) { | |
| runTools[index].function!.arguments = currentToolArgs; | |
| } | |
| } catch (e) {} | |
| try { | |
| const parsedArgs = JSON.parse(currentToolArgs); | |
| if (typeof parsedArgs !== 'object' || parsedArgs === null) { | |
| throw new Error('Tool arguments must be a valid JSON object'); | |
| } | |
| if (index >= 0 && index < runTools.length) { | |
| runTools[index].function!.arguments = JSON.stringify(parsedArgs); | |
| } | |
| } catch (e) { | |
| console.error('Failed to parse tool arguments:', e); | |
| } |
app/client/platforms/bedrock.ts
Outdated
| const colonIndex = url.indexOf(":"); | ||
| const semicolonIndex = url.indexOf(";"); | ||
| const comma = url.indexOf(","); | ||
|
|
||
| return { | ||
| type: "image", | ||
| source: { | ||
| type: url.slice(semicolonIndex + 1, comma), | ||
| media_type: url.slice(colonIndex + 1, semicolonIndex), | ||
| data: url.slice(comma + 1), | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Extract and Validate Image URL Processing
The current image URL processing logic lacks validation and error handling, which could lead to runtime errors with invalid URLs.
Extract the logic into a validated function:
+ interface ImageSource {
+ type: string;
+ media_type: string;
+ data: string;
+ }
+
+ function processImageUrl(url: string): ImageSource {
+ const colonIndex = url.indexOf(":");
+ const semicolonIndex = url.indexOf(";");
+ const comma = url.indexOf(",");
+
+ if (colonIndex === -1 || semicolonIndex === -1 || comma === -1) {
+ throw new Error("Invalid image URL format");
+ }
+
+ return {
+ type: url.slice(semicolonIndex + 1, comma),
+ media_type: url.slice(colonIndex + 1, semicolonIndex),
+ data: url.slice(comma + 1),
+ };
+ }
- const colonIndex = url.indexOf(":");
- const semicolonIndex = url.indexOf(";");
- const comma = url.indexOf(",");
-
+ const source = processImageUrl(url);
return {
type: "image",
- source: {
- type: url.slice(semicolonIndex + 1, comma),
- media_type: url.slice(colonIndex + 1, semicolonIndex),
- data: url.slice(comma + 1),
- },
+ source,
};📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const colonIndex = url.indexOf(":"); | |
| const semicolonIndex = url.indexOf(";"); | |
| const comma = url.indexOf(","); | |
| return { | |
| type: "image", | |
| source: { | |
| type: url.slice(semicolonIndex + 1, comma), | |
| media_type: url.slice(colonIndex + 1, semicolonIndex), | |
| data: url.slice(comma + 1), | |
| }, | |
| interface ImageSource { | |
| type: string; | |
| media_type: string; | |
| data: string; | |
| } | |
| function processImageUrl(url: string): ImageSource { | |
| const colonIndex = url.indexOf(":"); | |
| const semicolonIndex = url.indexOf(";"); | |
| const comma = url.indexOf(","); | |
| if (colonIndex === -1 || semicolonIndex === -1 || comma === -1) { | |
| throw new Error("Invalid image URL format"); | |
| } | |
| return { | |
| type: url.slice(semicolonIndex + 1, comma), | |
| media_type: url.slice(colonIndex + 1, semicolonIndex), | |
| data: url.slice(comma + 1), | |
| }; | |
| } | |
| const source = processImageUrl(url); | |
| return { | |
| type: "image", | |
| source, | |
| }; |
There was a problem hiding this comment.
Actionable comments posted: 25
🧹 Outside diff range and nitpick comments (7)
app/utils/aws.ts (3)
9-9: Remove debug console logsRemove commented out console.log statements as they should not be in production code.
-// console.info(serverConfig); -// console.info("======SECRET_KEY:"+SECRET_KEY);Also applies to: 11-11
45-49: Consider using a secure string comparison for maskSensitiveValueThe current implementation might be vulnerable to timing attacks when used for comparing masked values.
export function maskSensitiveValue(value: string): string { if (!value) return ""; if (value.length <= 4) return value; - return "*".repeat(value.length - 4) + value.slice(-4); + const masked = "*".repeat(value.length - 4) + value.slice(-4); + return masked; }
108-127: Refactor model path handling to improve maintainabilityThe nested conditionals for handling model paths could be simplified by extracting constants and helper functions.
+const MODEL_PATH_PREFIX = 'model/'; +const MODEL_PATH_SEPARATORS = /([.:])/g; + +function encodeModelPath(segment: string): string { + const parts = segment.split(new RegExp(`(${MODEL_PATH_PREFIX})`)); + return parts.map(part => { + if (part === MODEL_PATH_PREFIX) return part; + if (part.includes('.') || part.includes(':')) { + return part.split(MODEL_PATH_SEPARATORS) + .map((subpart, i) => i % 2 === 1 ? subpart : encodeURIComponent_RFC3986(subpart)) + .join(''); + } + return encodeURIComponent_RFC3986(part); + }).join(''); +} // In the main function: -if (segment.includes("model/")) { - const parts = segment.split(/(model\/)/); - return parts - .map((part) => { - if (part === "model/") return part; - if (part.includes(".") || part.includes(":")) { - return part - .split(/([.:])/g) - .map((subpart, i) => { - if (i % 2 === 1) return subpart; - return encodeURIComponent_RFC3986(subpart); - }) - .join(""); - } - return encodeURIComponent_RFC3986(part); - }) - .join(""); +if (segment.includes(MODEL_PATH_PREFIX)) { + return encodeModelPath(segment); }app/client/api.ts (1)
331-336: Remove commented codeThe commented-out code should be removed as it's not being used and contains security-sensitive logic.
app/api/bedrock.ts (1)
317-320: Add rate limiting for API endpointsThe API endpoints lack rate limiting which could lead to abuse.
Consider implementing rate limiting using a Redis-based solution or a service like AWS WAF. Would you like me to provide an example implementation or open an issue to track this enhancement?
app/locales/cn.ts (1)
356-356: Enhance validation messages with format examplesThe validation messages for Access Key and Secret Key should include format examples, similar to the Region validation message.
Apply this diff to improve the validation messages:
- Invalid: "无效的 AWS Access Key 格式。必须为20个字符。", + Invalid: "无效的 AWS Access Key 格式。必须为20个字符，示例：AKIAXXXXXXXXXXXXXXXX", - Invalid: "无效的 AWS Secret Key 格式。必须为40个字符。", + Invalid: "无效的 AWS Secret Key 格式。必须为40个字符，示例：XXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXXX",Also applies to: 362-362
app/components/settings.tsx (1)
992-1013: Consolidate credential validation logic.The access key and secret key validation logic is duplicated. Consider extracting the validation into a reusable function.
+interface CredentialValidation { + value: string; + length: number; + type: 'accessKey' | 'secretKey'; +} + +const validateAwsCredential = ({ value, length, type }: CredentialValidation) => { + if (value && value.length !== length) { + showToast(Locale.Settings.Access.Bedrock[type].Invalid); + return false; + } + return true; +}; + // Usage in both onChange handlers: onChange={(e) => { accessStore.update((access) => { const value = e.currentTarget.value; - if (value && value.length !== 20) { // or 40 for secret key - showToast(Locale.Settings.Access.Bedrock.AccessKey.Invalid); + if (!validateAwsCredential({ value, length: 20, type: 'AccessKey' })) { return; } access.awsAccessKey = value; }); }}Also applies to: 1014-1035
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (12)
.env.template(1 hunks)app/api/auth.ts(1 hunks)app/api/bedrock.ts(1 hunks)app/client/api.ts(6 hunks)app/client/platforms/bedrock.ts(1 hunks)app/components/chat.module.scss(1 hunks)app/components/settings.tsx(2 hunks)app/config/server.ts(3 hunks)app/locales/cn.ts(1 hunks)app/locales/en.ts(1 hunks)app/store/access.ts(6 hunks)app/utils/aws.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (3)
- .env.template
- app/api/auth.ts
- app/components/chat.module.scss
🧰 Additional context used
🪛 Biome (1.9.4)
app/api/bedrock.ts
[error] 48-48: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 48-48: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 104-104: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 104-104: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
app/components/settings.tsx
[error] 1047-1047: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (15)
app/store/access.ts (3)
18-18: LGTM!
The BEDROCK_BASE_URL import follows the established pattern and is properly placed with other service base URLs.
55-55: LGTM!
The DEFAULT_BEDROCK_URL constant follows the established pattern of conditional URL assignment based on build mode.
232-232: LGTM!
The addition of Bedrock validation to the authorization check follows the established pattern and is correctly placed in the condition chain.
app/utils/aws.ts (1)
139-228: Verify AWS Signature V4 implementation
Let's verify the AWS Signature V4 implementation against AWS documentation and test cases.
app/client/api.ts (3)
25-25: LGTM: Clean integration of BedrockApi
The BedrockApi integration follows the established pattern for provider initialization.
Also applies to: 134-136
355-356: LGTM: Consistent provider mapping
The Bedrock service provider mapping follows the established pattern.
246-246: Verify AWS credentials handling across the codebase
Let's verify that AWS credentials are handled consistently across the codebase and that there are no hardcoded credentials.
Also applies to: 278-289
✅ Verification successful
Based on the scan results, I can provide a final response since we have sufficient information to assess the AWS credentials handling:
AWS credentials are properly handled through environment variables and secure patterns
The codebase follows secure practices for AWS credential management:
- No hardcoded AWS credentials were found in the scan
- Credentials are properly managed through environment variables (AWS_ACCESS_KEY, AWS_SECRET_KEY)
- The credentials are accessed via secure patterns like process.env
- The code includes validation checks for credentials before making AWS API calls
- Error handling is in place for invalid/missing credentials
The changes to AWS credentials handling in this PR maintain these security practices and do not introduce any credential exposure risks.
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check for AWS credentials handling patterns and potential security issues
# Check for AWS credential patterns
echo "Checking for AWS credential patterns..."
rg -g '!{.git,node_modules}' -i '(aws.*key|aws.*secret|aws.*credential)'
# Check for potential hardcoded AWS credentials
echo "Checking for potential hardcoded AWS credentials..."
rg -g '!{.git,node_modules}' -i '([A-Z0-9]{20}|[A-Za-z0-9/+=]{40})'
# Check for environment variable usage
echo "Checking for AWS environment variable usage..."
rg -g '!{.git,node_modules}' -i '(process\.env\.AWS|AWS_|BEDROCK_)'
Length of output: 133971
app/api/bedrock.ts (3)
336-340:
Sanitize error messages in response
Exposing raw error messages in the response could reveal sensitive information.
console.error("Handler error:", e);
return NextResponse.json(
{
error: true,
- msg: e instanceof Error ? e.message : "Unknown error"
+ msg: "An internal server error occurred"
},
{ status: 500 },
);Likely invalid or redundant comment.
103-104:
Use a safer method to clean control characters
The regex pattern for cleaning control characters in LLaMA3 response is flagged as potentially unsafe.
- text = text.replace(/[\x00-\x1F\x7F-\x9F]/g, "");
+ text = text.replace(/[^\x20-\x7E]/g, "");Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 104-104: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 104-104: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
271-280:
Sanitize error responses
Exposing raw error messages could reveal sensitive information about the system.
if (!res.ok) {
const error = await res.text();
- console.error("AWS Bedrock error response:", error);
+ console.error("[Bedrock] Request failed with status:", res.status);
try {
const errorJson = JSON.parse(error);
- throw new Error(errorJson.message || error);
+ throw new Error("Failed to process request");
} catch {
- throw new Error(error || "Failed to get response from Bedrock");
+ throw new Error("Failed to get response from Bedrock");
}
}Likely invalid or redundant comment.
app/client/platforms/bedrock.ts (4)
180-190:
Fix unsafe image URL processing
The current implementation lacks validation for URL indices which could lead to runtime errors.
Apply this fix:
const { url = "" } = image_url || {};
const colonIndex = url.indexOf(":");
const semicolonIndex = url.indexOf(";");
const comma = url.indexOf(",");
+
+if (colonIndex === -1 || semicolonIndex === -1 || comma === -1) {
+ throw new Error("Invalid image URL format");
+}
+
+if (colonIndex >= semicolonIndex || semicolonIndex >= comma) {
+ throw new Error("Malformed image URL structure");
+}
return {
type: "image",
source: {
type: url.slice(semicolonIndex + 1, comma),
media_type: url.slice(colonIndex + 1, semicolonIndex),
data: url.slice(comma + 1),
},
};Likely invalid or redundant comment.
387-393: 🛠️ Refactor suggestion
Implement or properly handle unsupported methods
The usage() and models() methods return dummy values without indicating that the functionality is not supported.
Implement proper error handling:
async usage() {
- return { used: 0, total: 0 };
+ throw new Error("Usage tracking is not supported for Bedrock API");
}
async models() {
- return [];
+ throw new Error("Model listing is not supported for Bedrock API");
}Likely invalid or redundant comment.
294-299:
Fix empty catch block in tool argument parsing
The empty catch block silently ignores parsing errors which could hide issues.
Add proper error handling:
try {
- JSON.parse(currentToolArgs);
+ const parsedArgs = JSON.parse(currentToolArgs);
+ if (typeof parsedArgs !== 'object' || parsedArgs === null) {
+ throw new Error('Tool arguments must be a valid JSON object');
+ }
if (index >= 0 && index < runTools.length) {
- runTools[index].function!.arguments = currentToolArgs;
+ runTools[index].function!.arguments = JSON.stringify(parsedArgs);
}
-} catch (e) {}
+} catch (e) {
+ console.error('Failed to parse tool arguments:', e);
+ throw new Error('Invalid tool arguments format');
+}Likely invalid or redundant comment.
241-252: Verify AWS credentials handling
The commented-out code shows sensitive AWS credentials being passed in headers. Ensure the getHeaders() function handles credentials securely.
✅ Verification successful
Let me gather more information about the headers implementation.
Let me check how AWS credentials are handled in the API layer since they're not in the headers anymore.
Let me try a broader search to locate the API handlers and credential management.
AWS credentials are now handled securely
The code changes show a proper security implementation:
- Removed sensitive AWS credentials from client-side headers
- AWS credentials are now handled through:
- Server-side config (environment variables)
- Or securely passed via Authorization header using Bearer token
- The API layer (
app/api/bedrock.ts) properly validates and processes the credentials - Client-side code (
bedrock.ts) no longer exposes credentials in headers
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check if getHeaders implementation handles AWS credentials securely
rg -A 10 "function getHeaders" app/client/api.ts
Length of output: 377
Script:
#!/bin/bash
# Search for AWS credentials handling in the bedrock.ts file
rg -A 10 "getHeaders\(\)" app/client/platforms/bedrock.ts
# Search for any other potential headers setup in bedrock.ts
rg -B 5 -A 10 "headers\s*=" app/client/platforms/bedrock.ts
Length of output: 1245
Script:
#!/bin/bash
# Search for AWS credentials handling in API routes
rg -B 5 -A 10 "aws.*Key|aws.*Region|bedrock" app/api/bedrock/
# Check if there's a middleware or auth handler
rg -B 5 -A 10 "aws.*Key|aws.*Region|bedrock" app/api/middleware/
Length of output: 247
Script:
#!/bin/bash
# Find all API-related files
fd "api|bedrock" --type f
# Search for AWS credentials handling across all files
rg -B 5 -A 10 "aws.*Key|aws.*Region|bedrock" --type ts
Length of output: 29653
app/components/settings.tsx (1)
1814-1814: LGTM: Component integration is correct.
The Bedrock configuration component is properly integrated into the provider list.
app/store/access.ts
Outdated
| // aws bedrock | ||
| bedrokUrl: DEFAULT_BEDROCK_URL, | ||
| awsRegion: "", | ||
| awsAccessKey: "", | ||
| awsSecretKey: "", | ||
| bedrockEncryptionKey: "", | ||
|
|
There was a problem hiding this comment.
Review security implications of AWS credential storage.
Storing AWS credentials in browser storage poses several security risks:
- Credentials are persisted between sessions
- Vulnerable to XSS attacks
- Accessible to client-side JavaScript
Consider:
- Using AWS Cognito for authentication
- Implementing server-side credential management
- Using temporary credentials or session tokens
- Adding client-side encryption before storage
Would you like assistance in implementing a more secure credential management solution?
app/utils/aws.ts
Outdated
| if (serverConfig.isBedrock && !SECRET_KEY) { | ||
| console.error("When use Bedrock modle,ENCRYPTION_KEY should been set!"); | ||
| } |
There was a problem hiding this comment.
Fix error message formatting and add key length validation
The error message has typos and inconsistent formatting. Also, add validation for the encryption key length.
+if (!SECRET_KEY || SECRET_KEY.length < 32) {
+ throw new Error(
+ "ENCRYPTION_KEY must be at least 32 characters long when using Bedrock model"
+ );
+}
-if (serverConfig.isBedrock && !SECRET_KEY) {
- console.error("When use Bedrock modle,ENCRYPTION_KEY should been set!");
-}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (serverConfig.isBedrock && !SECRET_KEY) { | |
| console.error("When use Bedrock modle,ENCRYPTION_KEY should been set!"); | |
| } | |
| if (!SECRET_KEY || SECRET_KEY.length < 32) { | |
| throw new Error( | |
| "ENCRYPTION_KEY must be at least 32 characters long when using Bedrock model" | |
| ); | |
| } |
app/locales/en.ts
Outdated
| Bedrock: { | ||
| Region: { | ||
| Title: "AWS Region", | ||
| SubTitle: "The AWS region where Bedrock service is located", | ||
| Placeholder: "us-west-2", | ||
| Invalid: "Invalid AWS region format. Example: us-west-2", | ||
| }, | ||
| AccessKey: { | ||
| Title: "AWS Access Key ID", | ||
| SubTitle: "Your AWS access key ID for Bedrock service", | ||
| Placeholder: "AKIA...", | ||
| Invalid: "Invalid AWS access key format. Must be 20 characters long.", | ||
| }, | ||
| SecretKey: { | ||
| Title: "AWS Secret Access Key", | ||
| SubTitle: "Your AWS secret access key for Bedrock service", | ||
| Placeholder: "****", | ||
| Invalid: "Invalid AWS secret key format. Must be 40 characters long.", | ||
| }, | ||
| EncryptionKey: { | ||
| Title: "Encryption Key", | ||
| SubTitle: "Your encryption key for configuration data", | ||
| Placeholder: "Enter encryption key", | ||
| }, | ||
| }, |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add missing endpoint configuration and improve validation messages
The Bedrock configuration section needs the following improvements:
- Add endpoint configuration to maintain consistency with other providers
- Clarify the purpose of the encryption key
- Improve validation messages to be more helpful
Apply this diff to implement the suggested changes:
Bedrock: {
+ Endpoint: {
+ Title: "AWS Bedrock Endpoint",
+ SubTitle: "Custom endpoint for AWS Bedrock API. Default: https://bedrock-runtime.[region].amazonaws.com",
+ Placeholder: "https://bedrock-runtime.[region].amazonaws.com",
+ },
Region: {
Title: "AWS Region",
SubTitle: "The AWS region where Bedrock service is located",
Placeholder: "us-west-2",
- Invalid: "Invalid AWS region format. Example: us-west-2",
+ Invalid: "Invalid AWS region format. Must be a valid AWS region code (e.g., us-west-2, us-east-1, eu-west-1)",
},
AccessKey: {
Title: "AWS Access Key ID",
SubTitle: "Your AWS access key ID for Bedrock service",
Placeholder: "AKIA...",
Invalid: "Invalid AWS access key format. Must be 20 characters long.",
},
SecretKey: {
Title: "AWS Secret Access Key",
SubTitle: "Your AWS secret access key for Bedrock service",
- Placeholder: "****",
+ Placeholder: "Enter your AWS secret access key",
Invalid: "Invalid AWS secret key format. Must be 40 characters long.",
},
EncryptionKey: {
Title: "Encryption Key",
- SubTitle: "Your encryption key for configuration data",
+ SubTitle: "Optional: Your encryption key to secure AWS credentials in local storage",
Placeholder: "Enter encryption key",
},
},📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| Bedrock: { | |
| Region: { | |
| Title: "AWS Region", | |
| SubTitle: "The AWS region where Bedrock service is located", | |
| Placeholder: "us-west-2", | |
| Invalid: "Invalid AWS region format. Example: us-west-2", | |
| }, | |
| AccessKey: { | |
| Title: "AWS Access Key ID", | |
| SubTitle: "Your AWS access key ID for Bedrock service", | |
| Placeholder: "AKIA...", | |
| Invalid: "Invalid AWS access key format. Must be 20 characters long.", | |
| }, | |
| SecretKey: { | |
| Title: "AWS Secret Access Key", | |
| SubTitle: "Your AWS secret access key for Bedrock service", | |
| Placeholder: "****", | |
| Invalid: "Invalid AWS secret key format. Must be 40 characters long.", | |
| }, | |
| EncryptionKey: { | |
| Title: "Encryption Key", | |
| SubTitle: "Your encryption key for configuration data", | |
| Placeholder: "Enter encryption key", | |
| }, | |
| }, | |
| Bedrock: { | |
| Endpoint: { | |
| Title: "AWS Bedrock Endpoint", | |
| SubTitle: "Custom endpoint for AWS Bedrock API. Default: https://bedrock-runtime.[region].amazonaws.com", | |
| Placeholder: "https://bedrock-runtime.[region].amazonaws.com", | |
| }, | |
| Region: { | |
| Title: "AWS Region", | |
| SubTitle: "The AWS region where Bedrock service is located", | |
| Placeholder: "us-west-2", | |
| Invalid: "Invalid AWS region format. Must be a valid AWS region code (e.g., us-west-2, us-east-1, eu-west-1)", | |
| }, | |
| AccessKey: { | |
| Title: "AWS Access Key ID", | |
| SubTitle: "Your AWS access key ID for Bedrock service", | |
| Placeholder: "AKIA...", | |
| Invalid: "Invalid AWS access key format. Must be 20 characters long.", | |
| }, | |
| SecretKey: { | |
| Title: "AWS Secret Access Key", | |
| SubTitle: "Your AWS secret access key for Bedrock service", | |
| Placeholder: "Enter your AWS secret access key", | |
| Invalid: "Invalid AWS secret key format. Must be 40 characters long.", | |
| }, | |
| EncryptionKey: { | |
| Title: "Encryption Key", | |
| SubTitle: "Optional: Your encryption key to secure AWS credentials in local storage", | |
| Placeholder: "Enter encryption key", | |
| }, | |
| }, |
app/components/settings.tsx
Outdated
| <ListItem | ||
| title={Locale.Settings.Access.Bedrock.Region.Title} | ||
| subTitle={Locale.Settings.Access.Bedrock.Region.SubTitle} | ||
| > | ||
| <input | ||
| aria-label={Locale.Settings.Access.Bedrock.Region.Title} | ||
| type="text" | ||
| value={accessStore.awsRegion} | ||
| placeholder="us-west-2" | ||
| onChange={(e) => | ||
| accessStore.update((access) => { | ||
| const region = e.currentTarget.value; | ||
| if (!/^[a-z]{2}-[a-z]+-\d+$/.test(region)) { | ||
| showToast(Locale.Settings.Access.Bedrock.Region.Invalid); | ||
| return; | ||
| } | ||
| access.awsRegion = region; | ||
| }) | ||
| } | ||
| /> | ||
| </ListItem> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
AWS region validation needs improvement.
While the regex validation for AWS region format is good, consider:
- Using a predefined list of valid AWS regions
- Adding error handling for empty values
- Moving the validation logic to a reusable utility function
+const AWS_REGIONS = ['us-west-2', 'us-east-1', /* add other valid regions */];
+
+const isValidAwsRegion = (region: string) => {
+ if (!region) return false;
+ return AWS_REGIONS.includes(region);
+};
+
onChange={(e) =>
accessStore.update((access) => {
const region = e.currentTarget.value;
- if (!/^[a-z]{2}-[a-z]+-\d+$/.test(region)) {
+ if (!isValidAwsRegion(region)) {
showToast(Locale.Settings.Access.Bedrock.Region.Invalid);
return;
}
access.awsRegion = region;
})
}Committable suggestion skipped: line range outside the PR's diff.
There was a problem hiding this comment.
Actionable comments posted: 6
🧹 Outside diff range and nitpick comments (1)
app/client/api.ts (1)
Line range hint
1-379: Consider architectural improvements for provider authenticationThe current architecture mixes authentication concerns across different providers. Consider:
- Creating a dedicated authentication layer for each provider
- Implementing a provider-specific credential manager
- Using the factory pattern for provider-specific authentication strategies
Example architecture:
interface ProviderAuth { getHeaders(): Record<string, string>; } class BedrockAuth implements ProviderAuth { getHeaders(): Record<string, string> { // Implement AWS SigV4 return {}; } } class AuthFactory { static getAuth(provider: ServiceProvider): ProviderAuth { switch (provider) { case ServiceProvider.Bedrock: return new BedrockAuth(); // ... other providers } } }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/api/bedrock.ts(1 hunks)app/client/api.ts(6 hunks)app/client/platforms/bedrock.ts(1 hunks)app/store/access.ts(6 hunks)app/utils/aws.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- app/store/access.ts
- app/utils/aws.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/api/bedrock.ts
[error] 50-50: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 50-50: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 106-106: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 106-106: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (14)
app/client/api.ts (3)
25-25: LGTM: Clean integration of BedrockApi
The BedrockApi integration follows the established pattern used for other providers.
Also applies to: 134-136
354-355: LGTM: Clean provider mapping
The Bedrock provider mapping follows the established pattern.
330-346:
Security: Implement AWS SigV4 for request signing
The current implementation sends credentials in headers, which is not compliant with AWS security best practices. Instead:
- Implement AWS Signature Version 4 signing
- Use AWS SDK's built-in request signing
- Remove custom header-based authentication
Let's check if AWS SDK is available in the project:
app/api/bedrock.ts (6)
9-9: 🛠️ Refactor suggestion
Define specific return type instead of using 'any'
Using any type reduces type safety. Consider defining a specific type for the parsed response.
-function parseEventData(chunk: Uint8Array): any {
+type ParsedEventData = {
+ output?: string;
+ body?: string | Record<string, unknown>;
+ type?: string;
+ delta?: {
+ type?: string;
+ text?: string;
+ stop_reason?: string;
+ };
+ generation?: string;
+ outputText?: string;
+ outputs?: Array<{ text: string }>;
+ completion?: string;
+ content_block?: {
+ type: string;
+ [key: string]: unknown;
+ };
+};
+
+function parseEventData(chunk: Uint8Array): ParsedEventData | null {Likely invalid or redundant comment.
106-106:
Use a safer method to clean control characters
The current regex pattern for cleaning control characters is flagged as potentially unsafe.
- text = text.replace(/[\x00-\x1F\x7F-\x9F]/g, "");
+ text = text.replace(/[^\x20-\x7E]/g, "");Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 106-106: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 106-106: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
243-248:
Remove sensitive request logging
Logging the entire request body could expose sensitive information including credentials and user data.
- console.log("Request to AWS Bedrock:", {
- endpoint,
- modelId,
- body: requestBody,
- });
+ console.log("[Bedrock] Sending request to model:", modelId);Likely invalid or redundant comment.
60-63: 🛠️ Refactor suggestion
Add timeout handling for stream processing
The stream processing lacks timeout handling which could lead to hung connections.
async function* transformBedrockStream(
stream: ReadableStream,
modelId: string,
+ signal?: AbortSignal,
) {
+ if (signal?.aborted) {
+ throw new Error('Stream processing aborted');
+ }
+
const reader = stream.getReader();
+
+ if (signal) {
+ signal.addEventListener('abort', () => {
+ reader.cancel('Stream processing timeout');
+ });
+ }Likely invalid or redundant comment.
53-55: 🛠️ Refactor suggestion
Improve error handling in fallback parsing
The catch block silently swallows errors, which could hide important issues.
} catch (innerError) {
- console.error("Error in fallback parsing:", innerError);
+ console.error(
+ "Failed to parse response in fallback handlers:",
+ innerError instanceof Error ? innerError.message : "Unknown error"
+ );
+ throw new Error("Failed to parse response");
}Likely invalid or redundant comment.
154-154: 🛠️ Refactor suggestion
Replace 'any' type with a specific type definition
Using any type reduces type safety and could lead to runtime errors.
-function validateRequest(body: any, modelId: string): void {
+type RequestBody = {
+ body?: {
+ prompt?: string;
+ inputText?: string;
+ messages?: Array<{
+ role: string;
+ content: string | Array<{
+ type: string;
+ [key: string]: unknown;
+ }>;
+ }>;
+ };
+ anthropic_version?: string;
+ max_tokens?: number;
+ prompt?: string;
+ messages?: Array<Record<string, unknown>>;
+};
+
+function validateRequest(body: RequestBody, modelId: string): void {Likely invalid or redundant comment.
app/client/platforms/bedrock.ts (5)
31-37: Improve type safety in ToolDefinition interface
69-71: LGTM: Clear error for unsupported functionality
73-113: Improve type safety and error handling in extractMessage method
115-236: Extract and validate image URL processing
410-416: Implement proper error messages for unsupported methods
There was a problem hiding this comment.
Actionable comments posted: 12
🧹 Outside diff range and nitpick comments (1)
app/utils/aws.ts (1)
45-53: Enhance SignParams interface with optional parametersThe interface should include optional parameters for headers and query parameters to support different AWS service requirements.
export interface SignParams { method: string; url: string; region: string; accessKeyId: string; secretAccessKey: string; body: string; service: string; + headers?: Record<string, string>; + queryParams?: Record<string, string>; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (5)
app/api/bedrock.ts(1 hunks)app/client/api.ts(6 hunks)app/components/settings.tsx(2 hunks)app/store/access.ts(6 hunks)app/utils/aws.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/client/api.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/api/bedrock.ts
[error] 51-51: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 51-51: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 105-105: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 105-105: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
app/components/settings.tsx
[error] 1035-1035: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (10)
app/store/access.ts (3)
18-18: LGTM! Import and constant declarations are consistent.
The additions follow the established pattern for service provider configurations.
Also applies to: 26-26, 44-44
218-218: LGTM! Authorization check is properly updated.
The Bedrock validation is correctly integrated into the authorization flow.
192-199: 🛠️ Refactor suggestion
Enhance AWS credential validation.
The current validation only checks for presence of credentials. Consider implementing:
- AWS region format validation (e.g., "us-east-1")
- AWS access key format validation
- Encryption key strength validation
isValidBedrock() {
+ const state = get();
+
+ // Basic format validation
+ const isValidRegion = /^[a-z]{2}-[a-z]+-\d{1,2}$/.test(state.awsRegion);
+ const isValidAccessKey = /^[A-Z0-9]{20}$/.test(state.awsAccessKey);
+ const isValidSecretKey = /^[A-Za-z0-9/+=]{40}$/.test(state.awsSecretKey);
+ const isValidEncryptionKey = state.bedrockEncryptionKey.length >= 32;
+
+ if (!isValidRegion || !isValidAccessKey || !isValidSecretKey || !isValidEncryptionKey) {
+ console.warn('[Access] Invalid AWS credentials format');
+ return false;
+ }
+
+ return true;
- return ensure(get(), [
- "awsRegion",
- "awsAccessKey",
- "awsSecretKey",
- "bedrockEncryptionKey",
- ]);
}Likely invalid or redundant comment.
app/api/bedrock.ts (6)
1-8: LGTM! Clean imports and constant definition.
The imports are well-organized and the ALLOWED_PATH constant provides a clear security boundary for allowed endpoints.
61-64: 🛠️ Refactor suggestion
Add timeout handling to prevent hung connections
The stream transformation lacks timeout handling which could lead to hung connections.
async function* transformBedrockStream(
stream: ReadableStream,
modelId: string,
+ signal?: AbortSignal,
) {
const reader = stream.getReader();
+
+ if (signal) {
+ signal.addEventListener('abort', () => {
+ reader.cancel('Stream processing timeout');
+ });
+ }Likely invalid or redundant comment.
10-10: 🛠️ Refactor suggestion
Define specific return type for parseEventData
The function currently returns any which reduces type safety. Consider defining a specific return type.
-function parseEventData(chunk: Uint8Array): any {
+type ParsedEventData = {
+ body?: string | Record<string, unknown>;
+ output?: string;
+ type?: string;
+ delta?: {
+ type?: string;
+ text?: string;
+ stop_reason?: string;
+ };
+ content_block?: {
+ type: string;
+ [key: string]: unknown;
+ };
+};
+
+function parseEventData(chunk: Uint8Array): ParsedEventData | null {Likely invalid or redundant comment.
348-349:
Sanitize error response
Exposing internal error details in the response could reveal sensitive information about the system.
- console.error("Handler error:", e);
+ console.error("Handler error:", e instanceof Error ? e.message : "Unknown error");
return NextResponse.json(
- prettyObject(e),
+ {
+ error: true,
+ message: "An internal server error occurred"
+ },
{ status: 500 }
);Likely invalid or redundant comment.
225-226:
Add SSRF protection for AWS region
The endpoint URL is constructed using user-provided AWS region without validation.
+const VALID_AWS_REGIONS = [
+ 'us-east-1', 'us-east-2', 'us-west-1', 'us-west-2',
+ 'eu-west-1', 'eu-west-2', 'eu-central-1',
+ 'ap-northeast-1', 'ap-southeast-1', 'ap-southeast-2',
+];
+
+if (!VALID_AWS_REGIONS.includes(awsRegion)) {
+ throw new Error(`Invalid AWS region: ${awsRegion}`);
+}
+
const baseEndpoint = `https://bedrock-runtime.${awsRegion}.amazonaws.com`;Likely invalid or redundant comment.
153-153: 🛠️ Refactor suggestion
Define specific type for request body validation
Using any type reduces type safety. Consider defining a specific type for the request body.
-function validateRequest(body: any, modelId: string): void {
+type RequestBody = {
+ body?: {
+ prompt?: string;
+ inputText?: string;
+ messages?: Array<{
+ role: string;
+ content: string | Array<{
+ type: string;
+ [key: string]: unknown;
+ }>;
+ }>;
+ };
+ anthropic_version?: string;
+ max_tokens?: number;
+ prompt?: string;
+ inputText?: string;
+};
+
+function validateRequest(body: RequestBody, modelId: string): void {Likely invalid or redundant comment.
app/components/settings.tsx (1)
1802-1802: LGTM!
The Bedrock configuration component is properly integrated into the provider list.
app/store/access.ts
Outdated
| // Override the set method to encrypt AWS credentials before storage | ||
| set: (partial: { [key: string]: any }) => { | ||
| if (partial.awsAccessKey) { | ||
| partial.awsAccessKey = encrypt(partial.awsAccessKey); | ||
| } | ||
| if (partial.awsSecretKey) { | ||
| partial.awsSecretKey = encrypt(partial.awsSecretKey); | ||
| } | ||
| if (partial.awsRegion) { | ||
| partial.awsRegion = encrypt(partial.awsRegion); | ||
| } | ||
| set(partial); | ||
| }, | ||
|
|
||
| // Add getter to decrypt AWS credentials when needed | ||
| get: () => { | ||
| const state = get(); | ||
| return { | ||
| ...state, | ||
| awsRegion: state.awsRegion ? decrypt(state.awsRegion) : "", | ||
| awsAccessKey: state.awsAccessKey ? decrypt(state.awsAccessKey) : "", | ||
| awsSecretKey: state.awsSecretKey ? decrypt(state.awsSecretKey) : "", | ||
| }; | ||
| }, |
There was a problem hiding this comment.
💡 Codebase verification
🛠️ Refactor suggestion
Encryption implementation is secure but logging needs attention
-
The encryption implementation in
app/utils/aws.tsis secure:- Uses AES encryption from crypto-js library
- Properly handles edge cases and errors
- Secret key is sourced from server configuration
- Includes proper error handling
-
Logging issues found that need addressing:
app/store/access.ts: Remove sensitive data fromconsole.log("[Config] got config from server", res)app/utils/aws.ts: Consider removing or masking error details in encryption/decryption error logsapp/components/home.tsx: Review whatgetClientConfig()exposes in logs
🔗 Analysis chain
Verify encryption implementation and remove sensitive logging.
- The encryption implementation looks good, but we should verify the encryption utilities.
- Remove sensitive data logging in the fetch method.
- console.log("[Config] got config from server", res);
+ console.log("[Config] received server configuration");🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Check encryption utility implementation
ast-grep --pattern 'function encrypt($$$)'
ast-grep --pattern 'function decrypt($$$)'
# Check for other instances of sensitive data logging
rg -i "console.log.*config" --type ts
Length of output: 400
Script:
#!/bin/bash
# Let's check the implementation of encryption/decryption utilities
rg -A 5 "export (function|const) (encrypt|decrypt)" --type ts
# Also check where these functions are imported and used
rg "import.*\{.*(encrypt|decrypt)" --type ts
Length of output: 953
Script:
#!/bin/bash
# Let's check the SECRET_KEY implementation and AES import
rg "SECRET_KEY" --type ts
rg "import.*AES" --type ts
# Also check the full content of aws.ts to understand the encryption context
cat app/utils/aws.ts
Length of output: 6580
app/api/bedrock.ts
Outdated
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for unsupported model IDs
The function should explicitly reject unsupported model IDs.
} else if (modelId.startsWith("amazon.titan")) {
if (!bodyContent.inputText) throw new Error("Titan requires inputText");
+ } else {
+ throw new Error(`Unsupported model ID: ${modelId}`);
}Committable suggestion skipped: line range outside the PR's diff.
| <ListItem | ||
| title={Locale.Settings.Access.Bedrock.SecretKey.Title} | ||
| subTitle={Locale.Settings.Access.Bedrock.SecretKey.SubTitle} | ||
| > | ||
| <PasswordInput | ||
| aria-label={Locale.Settings.Access.Bedrock.SecretKey.Title} | ||
| value={accessStore.awsSecretKey} | ||
| type="text" | ||
| placeholder={Locale.Settings.Access.Bedrock.SecretKey.Placeholder} | ||
| onChange={(e) => { | ||
| accessStore.update((access) => { | ||
| const secretKey = e.currentTarget.value; | ||
| access.awsSecretKey = secretKey; | ||
| }); | ||
| }} | ||
| maskWhenShow={true} | ||
| /> | ||
| </ListItem> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add AWS secret key validation.
Add validation for AWS secret key format (40-character string):
onChange={(e) => {
accessStore.update((access) => {
const secretKey = e.currentTarget.value;
+ if (secretKey && !/^[A-Za-z0-9/+=]{40}$/.test(secretKey)) {
+ showToast(Locale.Settings.Access.Bedrock.SecretKey.Invalid);
+ return;
+ }
access.awsSecretKey = secretKey;
});
}}Committable suggestion skipped: line range outside the PR's diff.
| const bedrockConfigComponent = accessStore.provider === | ||
| ServiceProvider.Bedrock && ( | ||
| <> | ||
| <ListItem | ||
| title={Locale.Settings.Access.Bedrock.Region.Title} | ||
| subTitle={Locale.Settings.Access.Bedrock.Region.SubTitle} | ||
| > | ||
| <input | ||
| aria-label={Locale.Settings.Access.Bedrock.Region.Title} | ||
| type="text" | ||
| value={accessStore.awsRegion} | ||
| placeholder="us-west-2" | ||
| onChange={(e) => | ||
| accessStore.update((access) => { | ||
| const region = e.currentTarget.value; | ||
| access.awsRegion = region; | ||
| }) | ||
| } | ||
| /> | ||
| </ListItem> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add AWS region validation and improve code style.
The AWS region input needs validation and the code style can be improved:
- Add validation for AWS region format
- Use a predefined list of valid AWS regions
- Refactor the assignment expression
+const AWS_REGIONS = [
+ 'us-west-2', 'us-east-1', 'us-east-2', 'eu-west-1', 'eu-central-1',
+ 'ap-southeast-1', 'ap-southeast-2', 'ap-northeast-1'
+];
+
onChange={(e) =>
accessStore.update((access) => {
const region = e.currentTarget.value;
+ if (!AWS_REGIONS.includes(region)) {
+ showToast(Locale.Settings.Access.Bedrock.Region.Invalid);
+ return;
+ }
access.awsRegion = region;
})
}Committable suggestion skipped: line range outside the PR's diff.
app/components/settings.tsx
Outdated
| <ListItem | ||
| title={Locale.Settings.Access.Bedrock.EncryptionKey.Title} | ||
| subTitle={Locale.Settings.Access.Bedrock.EncryptionKey.SubTitle} | ||
| > | ||
| <PasswordInput | ||
| aria-label={Locale.Settings.Access.Bedrock.EncryptionKey.Title} | ||
| value={accessStore.bedrockEncryptionKey} | ||
| type="text" | ||
| placeholder={Locale.Settings.Access.Bedrock.EncryptionKey.Placeholder} | ||
| onChange={(e) => { | ||
| accessStore.update( | ||
| (access) => (access.bedrockEncryptionKey = e.currentTarget.value), | ||
| ); | ||
| }} | ||
| maskWhenShow={true} | ||
| /> | ||
| </ListItem> |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add encryption key validation and improve code style.
The encryption key input needs validation and the code style can be improved:
- Add minimum length requirement
- Add complexity requirements
- Refactor the assignment expression
onChange={(e) => {
+ const encryptionKey = e.currentTarget.value;
+ if (encryptionKey && (
+ encryptionKey.length < 32 ||
+ !/[A-Z]/.test(encryptionKey) ||
+ !/[a-z]/.test(encryptionKey) ||
+ !/[0-9]/.test(encryptionKey)
+ )) {
+ showToast(Locale.Settings.Access.Bedrock.EncryptionKey.Invalid);
+ return;
+ }
accessStore.update(
- (access) => (access.bedrockEncryptionKey = e.currentTarget.value),
+ (access) => {
+ access.bedrockEncryptionKey = encryptionKey;
+ }
);
}}Committable suggestion skipped: line range outside the PR's diff.
🧰 Tools
🪛 Biome (1.9.4)
[error] 1035-1035: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Outside diff range and nitpick comments (2)
app/utils/aws.ts (1)
93-128: Refactor complex URI encoding logicThe current implementation is complex and hard to maintain. Consider breaking it down into smaller, focused functions.
Apply this diff to improve maintainability:
+function encodeModelSegment(segment: string): string { + const parts = segment.split(/(model\/)/); + return parts + .map((part) => { + if (part === "model/") return part; + return encodeVersionedPart(part); + }) + .join(""); +} + +function encodeVersionedPart(part: string): string { + if (!(part.includes(".") || part.includes(":"))) { + return encodeURIComponent_RFC3986(part); + } + return part + .split(/([.:])/g) + .map((subpart, i) => i % 2 === 1 ? subpart : encodeURIComponent_RFC3986(subpart)) + .join(""); +} + function encodeURI_RFC3986(uri: string): string { if (!uri || uri === "/") return ""; const segments = uri.split("/"); return segments .map((segment) => { if (!segment) return ""; - if (segment.includes("model/")) { - const parts = segment.split(/(model\/)/); - return parts - .map((part) => { - if (part === "model/") return part; - if (part.includes(".") || part.includes(":")) { - return part - .split(/([.:])/g) - .map((subpart, i) => { - if (i % 2 === 1) return subpart; - return encodeURIComponent_RFC3986(subpart); - }) - .join(""); - } - return encodeURIComponent_RFC3986(part); - }) - .join(""); - } + if (segment.includes("model/")) return encodeModelSegment(segment); if (segment === "invoke-with-response-stream") return segment; return encodeURIComponent_RFC3986(segment); }) .join("/"); }app/api/bedrock.ts (1)
318-325: Add security headers to the responseThe response should include security-related headers.
Apply this diff:
return new Response(stream, { headers: { "Content-Type": "text/event-stream", "Cache-Control": "no-cache", Connection: "keep-alive", "X-Accel-Buffering": "no", + "X-Content-Type-Options": "nosniff", + "X-Frame-Options": "DENY", + "Strict-Transport-Security": "max-age=31536000; includeSubDomains", }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (3)
app/api/bedrock.ts(1 hunks)app/client/platforms/bedrock.ts(1 hunks)app/utils/aws.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/api/bedrock.ts
[error] 51-51: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 51-51: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 105-105: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 105-105: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (13)
app/utils/aws.ts (3)
1-211: Verify encryption usage across the codebase
Since this file implements critical security functionality, we should verify its usage across the codebase to ensure proper error handling.
✅ Verification successful
Encryption functions are properly used with error handling
The verification shows that the encryption/decryption functions are used appropriately across the codebase:
- The encryption functions themselves have proper error handling with try-catch blocks and null checks
- Usage in
app/store/access.tshas null checks before encryption/decryption - Usage in
app/api/bedrock.tsincludes validation after decryption:if (!awsRegion || !awsAccessKey || !awsSecretKey) - All uses are for securing sensitive AWS credentials
- No instances of unhandled encryption/decryption calls were found
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Check usage of encryption/decryption functions
# Expected: All calls should have proper error handling
# Find all files that import from aws.ts
echo "Files importing aws.ts:"
rg -l "import.*from.*aws" || true
# Check for direct usage of encrypt/decrypt functions
echo -e "\nUsage of encrypt/decrypt functions:"
rg "encrypt\(|decrypt\(" || true
# Check for proper error handling
echo -e "\nVerifying error handling:"
rg -A 2 -B 2 "encrypt\(|decrypt\(" || true
Length of output: 4115
14-22:
Critical: Fix encryption error handling
The current implementation has security concerns:
- Silent failures mask security issues
- Error logging could expose sensitive data
- Returning empty string on failure could lead to data loss
Apply this diff to improve error handling:
export function encrypt(data: string): string {
if (!data) return "";
try {
return AES.encrypt(data, SECRET_KEY).toString();
} catch (error) {
- console.error("Encryption failed:", error);
- return "";
+ throw new Error("Encryption failed");
}
}Likely invalid or redundant comment.
39-43: 🛠️ Refactor suggestion
Improve security of sensitive value masking
The current implementation lacks input validation and uses non-constant-time operations, which could potentially leak information through timing attacks.
Apply this diff to improve security:
export function maskSensitiveValue(value: string): string {
+ if (typeof value !== 'string') {
+ throw new TypeError('Input must be a string');
+ }
if (!value) return "";
if (value.length <= 4) return value;
- return "*".repeat(value.length - 4) + value.slice(-4);
+ const masked = Buffer.alloc(value.length - 4, '*').toString();
+ return masked + value.slice(-4);
}Likely invalid or redundant comment.
app/api/bedrock.ts (8)
1-8: LGTM! Imports and constants are well-organized.
The necessary dependencies are imported and the ALLOWED_PATH constant is appropriately defined for request validation.
61-64: 🛠️ Refactor suggestion
Add timeout handling for stream processing
The stream processing lacks timeout handling which could lead to hung connections.
Apply this diff:
async function* transformBedrockStream(
stream: ReadableStream,
modelId: string,
+ signal?: AbortSignal,
) {
+ if (signal) {
+ signal.addEventListener('abort', () => {
+ throw new Error('Stream processing timeout');
+ });
+ }Likely invalid or redundant comment.
182-183: 🛠️ Refactor suggestion
Add validation for unsupported model IDs
The function should explicitly reject unsupported model IDs.
Apply this diff:
} else if (modelId.startsWith("amazon.titan")) {
if (!bodyContent.inputText) throw new Error("Titan requires inputText");
+ } else {
+ throw new Error(`Unsupported model ID: ${modelId}`);
}Likely invalid or redundant comment.
358-359:
Sanitize error response
Exposing raw error details could reveal sensitive information about the system.
Apply this diff:
- console.error("Handler error:", e);
+ console.error("Handler error:", e instanceof Error ? e.message : "Unknown error");
return NextResponse.json(
- prettyObject(e),
+ {
+ error: true,
+ message: "An internal server error occurred"
+ },
{ status: 500 }
);Likely invalid or redundant comment.
153-153: 🛠️ Refactor suggestion
Replace 'any' type with a specific type definition
Using 'any' type reduces type safety.
Apply this diff:
-function validateRequest(body: any, modelId: string): void {
+type RequestBody = {
+ body?: {
+ prompt?: string;
+ inputText?: string;
+ messages?: Array<Record<string, unknown>>;
+ };
+ anthropic_version?: string;
+ max_tokens?: number;
+ prompt?: string;
+};
+
+function validateRequest(body: RequestBody, modelId: string): void {Likely invalid or redundant comment.
51-51:
Use a safer method to clean control characters
The current regex pattern using control characters is flagged as potentially unsafe.
Apply this diff to use a safer pattern:
- const cleanText = text.replace(/[\x00-\x1F\x7F-\x9F]/g, "");
+ const cleanText = text.replace(/[^\x20-\x7E]/g, "");Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 51-51: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
225-226:
Add SSRF protection for AWS region
The endpoint URL is constructed using user-provided AWS region without validation.
Apply this diff:
+ const VALID_AWS_REGIONS = [
+ 'us-east-1', 'us-east-2', 'us-west-1', 'us-west-2',
+ 'eu-west-1', 'eu-west-2', 'eu-central-1',
+ 'ap-northeast-1', 'ap-southeast-1', 'ap-southeast-2',
+ ];
+
+ if (!VALID_AWS_REGIONS.includes(awsRegion)) {
+ throw new Error(`Invalid AWS region: ${awsRegion}`);
+ }
+
const baseEndpoint = `https://bedrock-runtime.${awsRegion}.amazonaws.com`;Likely invalid or redundant comment.
105-105:
Use a safer method to clean control characters
The current regex pattern using control characters is flagged as potentially unsafe.
Apply this diff to use a safer pattern:
- text = text.replace(/[\x00-\x1F\x7F-\x9F]/g, "");
+ text = text.replace(/[^\x20-\x7E]/g, "");Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 105-105: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
app/client/platforms/bedrock.ts (2)
212-222:
Add validation for image URL processing
The current implementation assumes valid URL format without proper validation.
Apply these improvements:
+interface ImageSource {
+ type: string;
+ media_type: string;
+ data: string;
+}
+
+function validateImageUrl(url: string): void {
+ const colonIndex = url.indexOf(":");
+ const semicolonIndex = url.indexOf(";");
+ const comma = url.indexOf(",");
+
+ if (colonIndex === -1 || semicolonIndex === -1 || comma === -1) {
+ throw new Error("Invalid image URL format");
+ }
+
+ const mediaType = url.slice(colonIndex + 1, semicolonIndex);
+ if (!mediaType.startsWith('image/')) {
+ throw new Error("Invalid media type for image");
+ }
+}
+
+function processImageUrl(url: string): ImageSource {
+ validateImageUrl(url);
+ const colonIndex = url.indexOf(":");
+ const semicolonIndex = url.indexOf(";");
+ const comma = url.indexOf(",");
+
+ return {
+ type: url.slice(semicolonIndex + 1, comma),
+ media_type: url.slice(colonIndex + 1, semicolonIndex),
+ data: url.slice(comma + 1),
+ };
+}
- const colonIndex = url.indexOf(":");
- const semicolonIndex = url.indexOf(";");
- const comma = url.indexOf(",");
-
+ const source = processImageUrl(url);
return {
type: "image",
- source: {
- type: url.slice(semicolonIndex + 1, comma),
- media_type: url.slice(colonIndex + 1, semicolonIndex),
- data: url.slice(comma + 1),
- },
+ source,
};Likely invalid or redundant comment.
31-37: 🛠️ Refactor suggestion
Enhance type safety in ToolDefinition interface
The parameters field using any type reduces type safety. Consider using a more comprehensive type definition:
interface ToolDefinition {
function?: {
name: string;
description?: string;
- parameters?: any;
+ parameters?: {
+ type: 'object';
+ properties: Record<string, {
+ type: string;
+ description?: string;
+ enum?: string[];
+ items?: unknown;
+ }>;
+ required?: string[];
+ additionalProperties?: boolean;
+ };
};
}Likely invalid or redundant comment.
app/utils/aws.ts
Outdated
| const serverConfig = getServerSideConfig(); | ||
| const SECRET_KEY = serverConfig.bedrockEncryptionKey || ""; | ||
| if (serverConfig.isBedrock && !SECRET_KEY) { | ||
| console.error("When use Bedrock modle,ENCRYPTION_KEY should been set!"); | ||
| } |
There was a problem hiding this comment.
Critical: Improve encryption key handling
The current implementation has several security issues:
- Falling back to an empty string when encryption key is missing
- Using console.error instead of throwing for missing key
- No validation of key length for cryptographic security
Apply this diff to fix the issues:
const serverConfig = getServerSideConfig();
-const SECRET_KEY = serverConfig.bedrockEncryptionKey || "";
+const SECRET_KEY = serverConfig.bedrockEncryptionKey;
if (serverConfig.isBedrock && !SECRET_KEY) {
- console.error("When use Bedrock modle,ENCRYPTION_KEY should been set!");
+ throw new Error("ENCRYPTION_KEY is required when using Bedrock mode");
+}
+if (SECRET_KEY && SECRET_KEY.length < 32) {
+ throw new Error("ENCRYPTION_KEY must be at least 32 characters for secure encryption");
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const serverConfig = getServerSideConfig(); | |
| const SECRET_KEY = serverConfig.bedrockEncryptionKey || ""; | |
| if (serverConfig.isBedrock && !SECRET_KEY) { | |
| console.error("When use Bedrock modle,ENCRYPTION_KEY should been set!"); | |
| } | |
| const serverConfig = getServerSideConfig(); | |
| const SECRET_KEY = serverConfig.bedrockEncryptionKey; | |
| if (serverConfig.isBedrock && !SECRET_KEY) { | |
| throw new Error("ENCRYPTION_KEY is required when using Bedrock mode"); | |
| } | |
| if (SECRET_KEY && SECRET_KEY.length < 32) { | |
| throw new Error("ENCRYPTION_KEY must be at least 32 characters for secure encryption"); | |
| } |
app/utils/aws.ts
Outdated
| export async function sign({ | ||
| method, | ||
| url, | ||
| region, | ||
| accessKeyId, | ||
| secretAccessKey, | ||
| body, | ||
| service, | ||
| isStreaming = true, | ||
| additionalHeaders = {}, | ||
| }: SignParams): Promise<Record<string, string>> { | ||
| const endpoint = new URL(url); | ||
| const canonicalUri = "/" + encodeURI_RFC3986(endpoint.pathname.slice(1)); | ||
| const canonicalQueryString = endpoint.search.slice(1); | ||
|
|
||
| const now = new Date(); | ||
| const amzDate = now.toISOString().replace(/[:-]|\.\d{3}/g, ""); | ||
| const dateStamp = amzDate.slice(0, 8); | ||
|
|
||
| const payloadHash = SHA256(body).toString(Hex); | ||
|
|
||
| const headers: Record<string, string> = { | ||
| accept: isStreaming | ||
| ? "application/vnd.amazon.eventstream" | ||
| : "application/json", | ||
| "content-type": "application/json", | ||
| host: endpoint.host, | ||
| "x-amz-content-sha256": payloadHash, | ||
| "x-amz-date": amzDate, | ||
| ...additionalHeaders, | ||
| }; | ||
|
|
||
| if (isStreaming) { | ||
| headers["x-amzn-bedrock-accept"] = "*/*"; | ||
| } | ||
|
|
||
| const sortedHeaderKeys = Object.keys(headers).sort((a, b) => | ||
| a.toLowerCase().localeCompare(b.toLowerCase()), | ||
| ); | ||
|
|
||
| const canonicalHeaders = sortedHeaderKeys | ||
| .map( | ||
| (key) => `${key.toLowerCase()}:${normalizeHeaderValue(headers[key])}\n`, | ||
| ) | ||
| .join(""); | ||
|
|
||
| const signedHeaders = sortedHeaderKeys | ||
| .map((key) => key.toLowerCase()) | ||
| .join(";"); | ||
|
|
||
| const canonicalRequest = [ | ||
| method.toUpperCase(), | ||
| canonicalUri, | ||
| canonicalQueryString, | ||
| canonicalHeaders, | ||
| signedHeaders, | ||
| payloadHash, | ||
| ].join("\n"); | ||
|
|
||
| const algorithm = "AWS4-HMAC-SHA256"; | ||
| const credentialScope = `${dateStamp}/${region}/${service}/aws4_request`; | ||
| const stringToSign = [ | ||
| algorithm, | ||
| amzDate, | ||
| credentialScope, | ||
| SHA256(canonicalRequest).toString(Hex), | ||
| ].join("\n"); | ||
|
|
||
| const signingKey = getSigningKey(secretAccessKey, dateStamp, region, service); | ||
| const signature = hmac(signingKey, stringToSign).toString(Hex); | ||
|
|
||
| const authorization = [ | ||
| `${algorithm} Credential=${accessKeyId}/${credentialScope}`, | ||
| `SignedHeaders=${signedHeaders}`, | ||
| `Signature=${signature}`, | ||
| ].join(", "); | ||
|
|
||
| return { | ||
| ...headers, | ||
| Authorization: authorization, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add security validations to signing function
The implementation needs additional security validations:
- Enforce HTTPS protocol
- Validate required parameters
- Make headers more configurable
Apply this diff to improve security:
export async function sign({
method,
url,
region,
accessKeyId,
secretAccessKey,
body,
service,
isStreaming = true,
additionalHeaders = {},
}: SignParams): Promise<Record<string, string>> {
+ if (!isValidSignParams({ method, url, region, accessKeyId, secretAccessKey, body, service })) {
+ throw new Error("Missing required parameters");
+ }
+
+ if (!url.startsWith('https://')) {
+ throw new Error('URL must use HTTPS protocol');
+ }
+
const endpoint = new URL(url);
const canonicalUri = "/" + encodeURI_RFC3986(endpoint.pathname.slice(1));
const canonicalQueryString = endpoint.search.slice(1);Committable suggestion skipped: line range outside the PR's diff.
app/utils/aws.ts
Outdated
| export function decrypt(encryptedData: string): string { | ||
| if (!encryptedData) return ""; | ||
| try { | ||
| const bytes = AES.decrypt(encryptedData, SECRET_KEY); | ||
| const decrypted = bytes.toString(enc.Utf8); | ||
| if (!decrypted && encryptedData) { | ||
| return encryptedData; | ||
| } | ||
| return decrypted; | ||
| } catch (error) { | ||
| console.error("Decryption failed:", error); | ||
| return ""; | ||
| } | ||
| } |
There was a problem hiding this comment.
Critical: Fix decryption security issues
The current implementation has several security concerns:
- Returning original data on decryption failure could expose encrypted data
- Error logging could expose sensitive information
- Silent failures mask security issues
Apply this diff to fix the issues:
export function decrypt(encryptedData: string): string {
if (!encryptedData) return "";
try {
const bytes = AES.decrypt(encryptedData, SECRET_KEY);
const decrypted = bytes.toString(enc.Utf8);
- if (!decrypted && encryptedData) {
- return encryptedData;
- }
- return decrypted;
+ if (!decrypted) {
+ throw new Error("Invalid decryption result");
+ }
+ return decrypted;
} catch (error) {
- console.error("Decryption failed:", error);
- return "";
+ throw new Error("Decryption failed");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function decrypt(encryptedData: string): string { | |
| if (!encryptedData) return ""; | |
| try { | |
| const bytes = AES.decrypt(encryptedData, SECRET_KEY); | |
| const decrypted = bytes.toString(enc.Utf8); | |
| if (!decrypted && encryptedData) { | |
| return encryptedData; | |
| } | |
| return decrypted; | |
| } catch (error) { | |
| console.error("Decryption failed:", error); | |
| return ""; | |
| } | |
| } | |
| export function decrypt(encryptedData: string): string { | |
| if (!encryptedData) return ""; | |
| try { | |
| const bytes = AES.decrypt(encryptedData, SECRET_KEY); | |
| const decrypted = bytes.toString(enc.Utf8); | |
| if (!decrypted) { | |
| throw new Error("Invalid decryption result"); | |
| } | |
| return decrypted; | |
| } catch (error) { | |
| throw new Error("Decryption failed"); | |
| } | |
| } |
app/utils/aws.ts
Outdated
| export interface SignParams { | ||
| method: string; | ||
| url: string; | ||
| region: string; | ||
| accessKeyId: string; | ||
| secretAccessKey: string; | ||
| body: string; | ||
| service: string; | ||
| isStreaming?: boolean; | ||
| additionalHeaders?: Record<string, string>; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for required sign parameters
The interface definition lacks validation for required parameters and type constraints.
Add parameter validation by creating a type guard:
+type RequiredSignParams = Required<Omit<SignParams, 'isStreaming' | 'additionalHeaders'>>;
+
+function isValidSignParams(params: SignParams): params is RequiredSignParams {
+ return !!(
+ params.method &&
+ params.url &&
+ params.region &&
+ params.accessKeyId &&
+ params.secretAccessKey &&
+ params.service
+ );
+}
+
export interface SignParams {
method: string;
url: string;
region: string;
accessKeyId: string;
secretAccessKey: string;
body: string;
service: string;
isStreaming?: boolean;
additionalHeaders?: Record<string, string>;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export interface SignParams { | |
| method: string; | |
| url: string; | |
| region: string; | |
| accessKeyId: string; | |
| secretAccessKey: string; | |
| body: string; | |
| service: string; | |
| isStreaming?: boolean; | |
| additionalHeaders?: Record<string, string>; | |
| } | |
| type RequiredSignParams = Required<Omit<SignParams, 'isStreaming' | 'additionalHeaders'>>; | |
| function isValidSignParams(params: SignParams): params is RequiredSignParams { | |
| return !!( | |
| params.method && | |
| params.url && | |
| params.region && | |
| params.accessKeyId && | |
| params.secretAccessKey && | |
| params.service | |
| ); | |
| } | |
| export interface SignParams { | |
| method: string; | |
| url: string; | |
| region: string; | |
| accessKeyId: string; | |
| secretAccessKey: string; | |
| body: string; | |
| service: string; | |
| isStreaming?: boolean; | |
| additionalHeaders?: Record<string, string>; | |
| } |
app/api/bedrock.ts
Outdated
| // Handle Titan models | ||
| if (modelId.startsWith("amazon.titan")) { | ||
| const text = parsed.outputText || ""; | ||
| if (text) { | ||
| yield `data: ${JSON.stringify({ | ||
| delta: { text }, | ||
| })}\n\n`; | ||
| } | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add validation for supported model types
The function should validate model types before processing to prevent handling unsupported models.
Apply this diff:
+const SUPPORTED_MODELS = {
+ TITAN: 'amazon.titan',
+ LLAMA: 'us.meta.llama3',
+ MISTRAL: 'mistral.mistral',
+ CLAUDE: 'anthropic.claude'
+} as const;
+
+function isSupportedModel(modelId: string): boolean {
+ return Object.values(SUPPORTED_MODELS).some(prefix =>
+ modelId.toLowerCase().startsWith(prefix.toLowerCase())
+ );
+}
async function* transformBedrockStream(
stream: ReadableStream,
modelId: string,
) {
+ if (!isSupportedModel(modelId)) {
+ throw new Error(`Unsupported model type: ${modelId}`);
+ }Also applies to: 92-110, 111-120, 121-146
app/api/bedrock.ts
Outdated
| if (!res.ok) { | ||
| const error = await res.text(); | ||
| console.error("AWS Bedrock error response:", error); | ||
| try { | ||
| const errorJson = JSON.parse(error); | ||
| throw new Error(errorJson.message || error); | ||
| } catch { | ||
| throw new Error(error || "Failed to get response from Bedrock"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Sanitize error responses
Exposing raw error messages could reveal sensitive information about the system.
Apply this diff:
if (!res.ok) {
const error = await res.text();
- console.error("AWS Bedrock error response:", error);
+ console.error("AWS Bedrock error response received");
try {
const errorJson = JSON.parse(error);
- throw new Error(errorJson.message || error);
+ throw new Error("Failed to process request");
} catch {
- throw new Error(error || "Failed to get response from Bedrock");
+ throw new Error("Failed to get response from Bedrock");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!res.ok) { | |
| const error = await res.text(); | |
| console.error("AWS Bedrock error response:", error); | |
| try { | |
| const errorJson = JSON.parse(error); | |
| throw new Error(errorJson.message || error); | |
| } catch { | |
| throw new Error(error || "Failed to get response from Bedrock"); | |
| } | |
| } | |
| if (!res.ok) { | |
| const error = await res.text(); | |
| console.error("AWS Bedrock error response received"); | |
| try { | |
| const errorJson = JSON.parse(error); | |
| throw new Error("Failed to process request"); | |
| } catch { | |
| throw new Error("Failed to get response from Bedrock"); | |
| } | |
| } |
app/client/platforms/bedrock.ts
Outdated
| path(path: string): string { | ||
| const accessStore = useAccessStore.getState(); | ||
|
|
||
| let baseUrl = ""; | ||
|
|
||
| if (accessStore.useCustomConfig) { | ||
| baseUrl = accessStore.bedrockUrl; | ||
| } | ||
|
|
||
| if (baseUrl.length === 0) { | ||
| const isApp = !!getClientConfig()?.isApp; | ||
| const apiPath = ApiPath.Bedrock; | ||
| baseUrl = isApp ? BEDROCK_BASE_URL : apiPath; | ||
| } | ||
|
|
||
| if (baseUrl.endsWith("/")) { | ||
| baseUrl = baseUrl.slice(0, baseUrl.length - 1); | ||
| } | ||
| if (!baseUrl.startsWith("http") && !baseUrl.startsWith(ApiPath.Bedrock)) { | ||
| baseUrl = "https://" + baseUrl; | ||
| } | ||
|
|
||
| console.log("[Proxy Endpoint] ", baseUrl, path); | ||
|
|
||
| return [baseUrl, path].join("/"); | ||
| } |
There was a problem hiding this comment.
Add URL validation and improve logging
- The URL construction lacks validation, which could lead to security issues.
- Production logging should use proper logging levels.
Apply these improvements:
path(path: string): string {
+ if (!path || typeof path !== 'string') {
+ throw new Error('Invalid path parameter');
+ }
const accessStore = useAccessStore.getState();
let baseUrl = "";
if (accessStore.useCustomConfig) {
+ if (!this.isValidUrl(accessStore.bedrockUrl)) {
+ throw new Error('Invalid custom Bedrock URL');
+ }
baseUrl = accessStore.bedrockUrl;
}
// ... rest of the method ...
- console.log("[Proxy Endpoint] ", baseUrl, path);
+ if (process.env.NODE_ENV !== 'production') {
+ console.debug("[Bedrock] Proxy endpoint:", baseUrl, path);
+ }
return [baseUrl, path].join("/");
}
+ private isValidUrl(url: string): boolean {
+ try {
+ const parsed = new URL(url);
+ return ['http:', 'https:'].includes(parsed.protocol);
+ } catch {
+ return false;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| path(path: string): string { | |
| const accessStore = useAccessStore.getState(); | |
| let baseUrl = ""; | |
| if (accessStore.useCustomConfig) { | |
| baseUrl = accessStore.bedrockUrl; | |
| } | |
| if (baseUrl.length === 0) { | |
| const isApp = !!getClientConfig()?.isApp; | |
| const apiPath = ApiPath.Bedrock; | |
| baseUrl = isApp ? BEDROCK_BASE_URL : apiPath; | |
| } | |
| if (baseUrl.endsWith("/")) { | |
| baseUrl = baseUrl.slice(0, baseUrl.length - 1); | |
| } | |
| if (!baseUrl.startsWith("http") && !baseUrl.startsWith(ApiPath.Bedrock)) { | |
| baseUrl = "https://" + baseUrl; | |
| } | |
| console.log("[Proxy Endpoint] ", baseUrl, path); | |
| return [baseUrl, path].join("/"); | |
| } | |
| path(path: string): string { | |
| if (!path || typeof path !== 'string') { | |
| throw new Error('Invalid path parameter'); | |
| } | |
| const accessStore = useAccessStore.getState(); | |
| let baseUrl = ""; | |
| if (accessStore.useCustomConfig) { | |
| if (!this.isValidUrl(accessStore.bedrockUrl)) { | |
| throw new Error('Invalid custom Bedrock URL'); | |
| } | |
| baseUrl = accessStore.bedrockUrl; | |
| } | |
| if (baseUrl.length === 0) { | |
| const isApp = !!getClientConfig()?.isApp; | |
| const apiPath = ApiPath.Bedrock; | |
| baseUrl = isApp ? BEDROCK_BASE_URL : apiPath; | |
| } | |
| if (baseUrl.endsWith("/")) { | |
| baseUrl = baseUrl.slice(0, baseUrl.length - 1); | |
| } | |
| if (!baseUrl.startsWith("http") && !baseUrl.startsWith(ApiPath.Bedrock)) { | |
| baseUrl = "https://" + baseUrl; | |
| } | |
| if (process.env.NODE_ENV !== 'production') { | |
| console.debug("[Bedrock] Proxy endpoint:", baseUrl, path); | |
| } | |
| return [baseUrl, path].join("/"); | |
| } | |
| private isValidUrl(url: string): boolean { | |
| try { | |
| const parsed = new URL(url); | |
| return ['http:', 'https:'].includes(parsed.protocol); | |
| } catch { | |
| return false; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Outside diff range and nitpick comments (3)
app/api/bedrock.ts (3)
83-146: Refactor model-specific handling into separate functionsThe model-specific handling logic is tightly coupled and could be better organized.
Consider extracting model-specific logic into separate functions:
type ModelHandler = (parsed: BedrockResponse) => string | null; const handlers: Record<string, ModelHandler> = { 'amazon.titan': (parsed) => parsed.outputText || null, 'us.meta.llama3': (parsed) => { if (parsed.generation) return parsed.generation; if (parsed.output) return parsed.output; return typeof parsed === 'string' ? parsed : null; }, // ... other handlers }; function getModelHandler(modelId: string): ModelHandler { return Object.entries(handlers).find( ([prefix]) => modelId.startsWith(prefix) )?.[1] ?? (() => null); }🧰 Tools
🪛 Biome (1.9.4)
[error] 105-105: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 105-105: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
248-248: Extract timeout duration to configurationThe timeout duration is hardcoded which makes it difficult to configure.
+const REQUEST_TIMEOUT_MS = 10 * 60 * 1000; // 10 minutes + - const timeoutId = setTimeout(() => controller.abort(), 10 * 60 * 1000); + const timeoutId = setTimeout(() => controller.abort(), REQUEST_TIMEOUT_MS);
335-342: Add security headers to the responseThe response lacks important security headers.
return new Response(stream, { headers: { "Content-Type": "text/event-stream", "Cache-Control": "no-cache", Connection: "keep-alive", "X-Accel-Buffering": "no", + "X-Content-Type-Options": "nosniff", + "X-Frame-Options": "DENY", + "Strict-Transport-Security": "max-age=31536000; includeSubDomains", }, });
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/api/bedrock.ts(1 hunks)app/client/platforms/bedrock.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/api/bedrock.ts
[error] 51-51: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 51-51: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 105-105: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 105-105: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (9)
app/api/bedrock.ts (4)
61-64: 🛠️ Refactor suggestion
Add timeout handling for stream processing
The stream processing lacks timeout handling which could lead to hung connections.
async function* transformBedrockStream(
stream: ReadableStream,
modelId: string,
+ signal?: AbortSignal,
) {
+ if (signal?.aborted) {
+ throw new Error('Stream processing aborted');
+ }Likely invalid or redundant comment.
162-162: 🛠️ Refactor suggestion
Replace 'any' type with specific request types
Using 'any' type reduces type safety and makes the code harder to maintain.
-function validateRequest(body: any, modelId: string): void {
+type BaseRequestBody = {
+ body?: Record<string, unknown>;
+};
+
+type ClaudeRequestBody = BaseRequestBody & {
+ anthropic_version: string;
+ max_tokens: number;
+ messages?: Array<Record<string, unknown>>;
+ prompt?: string;
+};
+
+type LlamaRequestBody = BaseRequestBody & {
+ prompt: string;
+};
+
+type MistralRequestBody = BaseRequestBody & {
+ prompt: string;
+};
+
+type TitanRequestBody = BaseRequestBody & {
+ inputText: string;
+};
+
+type RequestBody = ClaudeRequestBody | LlamaRequestBody | MistralRequestBody | TitanRequestBody;
+
+function validateRequest(body: RequestBody, modelId: string): void {Likely invalid or redundant comment.
10-10: 🛠️ Refactor suggestion
Replace 'any' type with a specific type definition
Using any type reduces type safety. Consider defining a specific type for the parsed response.
-function parseEventData(chunk: Uint8Array): any {
+type BedrockResponse = {
+ body?: string | Record<string, unknown>;
+ output?: string;
+ outputText?: string;
+ generation?: string;
+ outputs?: Array<{ text: string }>;
+ completion?: string;
+ type?: string;
+ delta?: {
+ type?: string;
+ text?: string;
+ stop_reason?: string;
+ };
+ content_block?: {
+ type: string;
+ };
+};
+
+function parseEventData(chunk: Uint8Array): BedrockResponse | null {Likely invalid or redundant comment.
244-245:
Add AWS region validation
The AWS region is used directly in the endpoint URL without validation, which could lead to SSRF attacks.
+const VALID_AWS_REGIONS = [
+ 'us-east-1', 'us-east-2', 'us-west-1', 'us-west-2',
+ 'eu-west-1', 'eu-west-2', 'eu-central-1',
+ 'ap-northeast-1', 'ap-southeast-1', 'ap-southeast-2'
+] as const;
+
+if (!VALID_AWS_REGIONS.includes(awsRegion as typeof VALID_AWS_REGIONS[number])) {
+ throw new Error(`Invalid AWS region: ${awsRegion}`);
+}
+
const baseEndpoint = `https://bedrock-runtime.${awsRegion}.amazonaws.com`;Likely invalid or redundant comment.
app/client/platforms/bedrock.ts (5)
42-67: 🛠️ Refactor suggestion
Improve URL validation and logging practices.
- Production logging should be more restrictive to avoid exposing sensitive endpoint information.
- URL validation should be more robust.
Apply this diff to improve the implementation:
path(path: string): string {
+ if (!path || typeof path !== 'string') {
+ throw new Error('Invalid path parameter');
+ }
const accessStore = useAccessStore.getState();
let baseUrl = "";
if (accessStore.useCustomConfig) {
+ if (!this.isValidUrl(accessStore.bedrockUrl)) {
+ throw new Error('Invalid custom Bedrock URL');
+ }
baseUrl = accessStore.bedrockUrl;
}
// ... rest of the method ...
- console.log("[Proxy Endpoint] ", baseUrl, path);
+ if (process.env.NODE_ENV !== 'production') {
+ console.debug("[Bedrock] Proxy endpoint:", baseUrl, path);
+ }
return [baseUrl, path].join("/");
}
+ private isValidUrl(url: string): boolean {
+ try {
+ const parsed = new URL(url);
+ return ['http:', 'https:'].includes(parsed.protocol);
+ } catch {
+ return false;
+ }
+ }Likely invalid or redundant comment.
129-133: 🛠️ Refactor suggestion
Add type safety and validation for model configuration.
The function accepts any type for modelConfig without validation.
Apply this diff to improve type safety:
+interface ModelConfig {
+ model: string;
+ max_tokens: number;
+ temperature: number;
+ top_p?: number;
+}
-formatRequestBody(messages: RequestMessage[], systemMessage: string, modelConfig: any)
+formatRequestBody(
+ messages: RequestMessage[],
+ systemMessage: string,
+ modelConfig: ModelConfig,
+) {
+ if (!modelConfig?.model) {
+ throw new Error("Model configuration must specify a model");
+ }
+ if (modelConfig.temperature < 0 || modelConfig.temperature > 1) {
+ throw new Error("Temperature must be between 0 and 1");
+ }Likely invalid or redundant comment.
344-349: 🛠️ Refactor suggestion
Improve tool argument parsing and error handling.
The current implementation has several issues:
- Empty catch block silently ignores parsing errors
- No validation of parsed JSON structure
- Direct assignment of potentially invalid arguments
Apply this diff to improve the implementation:
try {
- JSON.parse(currentToolArgs);
+ const parsedArgs = JSON.parse(currentToolArgs);
+ if (typeof parsedArgs !== 'object' || parsedArgs === null) {
+ throw new Error('Tool arguments must be a valid JSON object');
+ }
if (index >= 0 && index < runTools.length) {
- runTools[index].function!.arguments = currentToolArgs;
+ runTools[index].function!.arguments = JSON.stringify(parsedArgs);
}
- } catch (e) {}
+ } catch (e) {
+ console.debug('[Bedrock] Partial JSON parse failed (expected for incomplete JSON):', e);
+ }Likely invalid or redundant comment.
436-442: 🛠️ Refactor suggestion
Implement proper error handling for unsupported operations.
The methods return dummy values without indicating that the operations are unsupported.
Apply this diff to improve the implementation:
+/**
+ * Usage tracking is not supported for Bedrock API.
+ * @throws {Error} Always throws an error indicating the operation is not supported.
+ */
async usage() {
- return { used: 0, total: 0 };
+ throw new Error(
+ "Usage tracking is not supported for Bedrock API. Use AWS CloudWatch metrics instead."
+ );
}
+/**
+ * Model listing is not supported for Bedrock API.
+ * @throws {Error} Always throws an error indicating the operation is not supported.
+ */
async models() {
- return [];
+ throw new Error(
+ "Model listing is not supported for Bedrock API. Configure available models in AWS Console."
+ );
}Likely invalid or redundant comment.
238-249: 🛠️ Refactor suggestion
Add validation for image URL processing.
The current implementation lacks validation for image URL components which could lead to runtime errors.
Apply this diff to improve the implementation:
+interface ImageSource {
+ type: string;
+ media_type: string;
+ data: string;
+}
+
+function processImageUrl(url: string): ImageSource {
+ const colonIndex = url.indexOf(":");
+ const semicolonIndex = url.indexOf(";");
+ const comma = url.indexOf(",");
+
+ if (colonIndex === -1 || semicolonIndex === -1 || comma === -1) {
+ throw new Error("Invalid image URL format");
+ }
+
+ return {
+ type: url.slice(semicolonIndex + 1, comma),
+ media_type: url.slice(colonIndex + 1, semicolonIndex),
+ data: url.slice(comma + 1),
+ };
+}
- const colonIndex = url.indexOf(":");
- const semicolonIndex = url.indexOf(";");
- const comma = url.indexOf(",");
-
+ const source = processImageUrl(url);
return {
type: "image",
- source: {
- type: url.slice(semicolonIndex + 1, comma),
- media_type: url.slice(colonIndex + 1, semicolonIndex),
- data: url.slice(comma + 1),
- },
+ source,
};Likely invalid or redundant comment.
app/api/bedrock.ts
Outdated
| // Handle plain text responses | ||
| if (text.trim()) { | ||
| // Clean up any malformed JSON characters | ||
| const cleanText = text.replace(/[\x00-\x1F\x7F-\x9F]/g, ""); |
There was a problem hiding this comment.
Use a safer method to clean control characters
The current regex pattern for cleaning control characters is flagged as potentially unsafe.
Apply this diff to use a safer pattern:
- const cleanText = text.replace(/[\x00-\x1F\x7F-\x9F]/g, "");
+ const cleanText = text.replace(/[^\x20-\x7E\x0A\x0D]/g, "");📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| const cleanText = text.replace(/[\x00-\x1F\x7F-\x9F]/g, ""); | |
| const cleanText = text.replace(/[^\x20-\x7E\x0A\x0D]/g, ""); |
🧰 Tools
🪛 Biome (1.9.4)
[error] 51-51: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 51-51: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
There was a problem hiding this comment.
Actionable comments posted: 5
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/api/bedrock.ts(1 hunks)app/client/platforms/bedrock.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/api/bedrock.ts
[error] 52-52: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 52-52: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (7)
app/api/bedrock.ts (7)
10-10: 🛠️ Refactor suggestion
Replace 'any' type with a specific type definition
Using any type reduces type safety. Define a specific type for the parsed response.
-function parseEventData(chunk: Uint8Array): any {
+type ParsedResponse = {
+ body?: string | Record<string, unknown>;
+ output?: string;
+ type?: string;
+ delta?: {
+ type?: string;
+ text?: string;
+ };
+};
+
+function parseEventData(chunk: Uint8Array): ParsedResponse | null {Likely invalid or redundant comment.
84-92: 🛠️ Refactor suggestion
Add validation for supported model types
The function should validate model types before processing to prevent handling unsupported models.
+const SUPPORTED_MODELS = {
+ TITAN: 'amazon.titan',
+ LLAMA: 'us.meta.llama',
+ MISTRAL: 'mistral.mistral',
+ CLAUDE: 'anthropic.claude'
+} as const;
+
+function isSupportedModel(modelId: string): boolean {
+ return Object.values(SUPPORTED_MODELS).some(prefix =>
+ modelId.toLowerCase().startsWith(prefix.toLowerCase())
+ );
+}
async function* transformBedrockStream(
stream: ReadableStream,
modelId: string,
) {
+ if (!isSupportedModel(modelId)) {
+ throw new Error(`Unsupported model type: ${modelId}`);
+ }Likely invalid or redundant comment.
163-163: 🛠️ Refactor suggestion
Replace 'any' type with a specific type definition
Using any type reduces type safety. Define a specific type for the request body.
-function validateRequest(body: any, modelId: string): void {
+type RequestBody = {
+ body?: Record<string, unknown>;
+ anthropic_version?: string;
+ max_tokens?: number;
+ messages?: Array<Record<string, unknown>>;
+ prompt?: string;
+ inputText?: string;
+};
+
+function validateRequest(body: RequestBody, modelId: string): void {Likely invalid or redundant comment.
62-65: 🛠️ Refactor suggestion
Add timeout handling for stream processing
The stream processing lacks timeout handling which could lead to hung connections.
async function* transformBedrockStream(
stream: ReadableStream,
modelId: string,
+ signal?: AbortSignal,
) {
+ if (signal) {
+ signal.addEventListener('abort', () => {
+ reader.cancel('Stream processing timeout');
+ });
+ }Likely invalid or redundant comment.
52-52:
Use a safer method to clean control characters
The current regex pattern for cleaning control characters is flagged as potentially unsafe.
- const cleanText = text.replace(/[\x00-\x1F\x7F-\x9F]/g, "");
+ const cleanText = text.replace(/[^\x20-\x7E\x0A\x0D]/g, "");Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 52-52: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 52-52: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
396-397:
Sanitize error responses
Exposing raw error details in the response could reveal sensitive information about the system.
- console.error("Handler error:", e);
+ console.error("Handler error:", e instanceof Error ? e.message : "Unknown error");
return NextResponse.json(
- prettyObject(e),
+ {
+ error: true,
+ message: "An internal server error occurred"
+ },
+ { status: 500 }
);Likely invalid or redundant comment.
244-245:
Add SSRF protection for AWS region
The endpoint URL is constructed using user-provided AWS region without validation.
+ const VALID_AWS_REGIONS = [
+ 'us-east-1', 'us-east-2', 'us-west-1', 'us-west-2',
+ 'eu-west-1', 'eu-west-2', 'eu-central-1',
+ 'ap-northeast-1', 'ap-southeast-1', 'ap-southeast-2',
+ ];
+
+ if (!VALID_AWS_REGIONS.includes(awsRegion)) {
+ throw new Error(`Invalid AWS region: ${awsRegion}`);
+ }
+
const baseEndpoint = `https://bedrock-runtime.${awsRegion}.amazonaws.com`;Likely invalid or redundant comment.
app/api/bedrock.ts
Outdated
| @@ -0,0 +1,399 @@ | |||
| import { NextRequest, NextResponse } from "next/server"; | |||
| import { auth } from "./auth"; | |||
| import { sign, decrypt } from "../utils/aws"; | |||
There was a problem hiding this comment.
Security Concern: Review credential handling mechanism
The decrypt utility imported from ../utils/aws should be reviewed for security best practices. Consider using AWS's secure credential management solutions like AWS KMS or AWS Secrets Manager.
app/client/platforms/bedrock.ts
Outdated
| if (!baseUrl.startsWith("http") && !baseUrl.startsWith(ApiPath.Bedrock)) { | ||
| baseUrl = "https://" + baseUrl; | ||
| } |
There was a problem hiding this comment.
Potential security risk in URL construction.
The current URL construction could be vulnerable to URL manipulation if baseUrl contains malicious input. Consider using the URL API for safer URL construction.
- if (!baseUrl.startsWith("http") && !baseUrl.startsWith(ApiPath.Bedrock)) {
- baseUrl = "https://" + baseUrl;
- }
+ try {
+ new URL(baseUrl);
+ } catch {
+ if (!baseUrl.startsWith(ApiPath.Bedrock)) {
+ baseUrl = `https://${baseUrl}`;
+ }
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if (!baseUrl.startsWith("http") && !baseUrl.startsWith(ApiPath.Bedrock)) { | |
| baseUrl = "https://" + baseUrl; | |
| } | |
| try { | |
| new URL(baseUrl); | |
| } catch { | |
| if (!baseUrl.startsWith(ApiPath.Bedrock)) { | |
| baseUrl = `https://${baseUrl}`; | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Outside diff range and nitpick comments (2)
app/constant.ts (2)
56-56: Reorder ApiPath enum entriesConsider grouping cloud provider entries together (Azure, Google, Bedrock) for better organization and readability.
export enum ApiPath { Cors = "", - Bedrock = "/api/bedrock", Azure = "/api/azure", + Bedrock = "/api/bedrock", Google = "/api/google",
243-248: Enhance Bedrock configuration documentationThe configuration would benefit from more detailed documentation:
- Explain why the ChatPath is simplified and how it's used in bedrock.ts
- Document the significance of the ApiVersion
- Add JSDoc comments for the getEndpoint function parameters
export const Bedrock = { + // Simple path as the full model-specific path is constructed in bedrock.ts + // Example: /{ApiVersion}/model/{modelId}/invoke ChatPath: "model", + // API version for Amazon Bedrock Runtime API ApiVersion: "2023-11-01", + /** + * Get the Bedrock runtime endpoint for a specific AWS region + * @param region - AWS region identifier (e.g., "us-west-2") + * @returns The complete Bedrock runtime endpoint URL + */ getEndpoint: (region: string = "us-west-2") => `https://bedrock-runtime.${region}.amazonaws.com`, };
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (2)
app/constant.ts(8 hunks)app/utils.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/utils.ts
🔇 Additional comments (2)
app/constant.ts (2)
582-592: LGTM! Integration follows existing patterns
The integration of Bedrock models into DEFAULT_MODELS follows the established patterns and maintains consistency with other providers.
331-353: Verify Bedrock model identifiers
Please verify that all model identifiers match the official AWS Bedrock documentation, particularly the version numbers and suffixes.
✅ Verification successful
Model identifiers appear to be correctly implemented and validated
The code shows consistent model identifier handling across the codebase:
- Model identifiers are properly validated in API handlers and client code
- Each model family (Titan, Claude, Llama, Mistral) has specific validation logic
- The model versions match the format used in AWS Bedrock's implementation
- The code includes proper error handling for model-specific requirements
🏁 Scripts executed
The following scripts were executed for the analysis:
Script:
#!/bin/bash
# Description: Search for model identifiers in documentation and code
# This helps verify the correctness of model IDs
# Search for model ID patterns in markdown files
echo "Searching for model IDs in documentation..."
rg -i "anthropic\.claude-3|amazon\.titan|meta\.llama3|mistral\." "*.md"
# Search for model-related constants or configurations
echo "Searching for model configurations..."
ast-grep --pattern 'const $_ = {
$$$
modelId: "$_"
$$$
}'
Length of output: 366
Script:
#!/bin/bash
# Let's try searching in all files for Bedrock model references
echo "Searching for model IDs in all files..."
rg -i "anthropic\.claude-3|amazon\.titan|meta\.llama3|mistral\." -A 2 -B 2
# Also search for any model configuration or validation logic
echo "Searching for model validation code..."
rg -i "bedrockModels|bedrock.*model" -A 2 -B 2
Length of output: 6450
| export const BEDROCK_BASE_URL = | ||
| "https://bedrock-runtime.us-west-2.amazonaws.com"; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove redundant BEDROCK_BASE_URL constant
This constant is redundant since there's already a Bedrock.getEndpoint function that provides the same functionality with better region configurability. Having two ways to get the endpoint could lead to inconsistencies.
-export const BEDROCK_BASE_URL =
- "https://bedrock-runtime.us-west-2.amazonaws.com";📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export const BEDROCK_BASE_URL = | |
| "https://bedrock-runtime.us-west-2.amazonaws.com"; |
There was a problem hiding this comment.
Actionable comments posted: 26
🧹 Outside diff range and nitpick comments (5)
app/client/api.ts (1)
Line range hint
1-379: Consider restructuring AWS integration for better security and maintainabilityThe current implementation of AWS Bedrock integration could benefit from the following architectural improvements:
Create a dedicated AWS credentials manager:
- Handle credential validation, encryption, and storage
- Implement AWS SDK's credential provider chain
- Support IAM role-based authentication
Implement proper AWS request signing:
- Use AWS SDK for request signing
- Follow AWS best practices for authentication
- Handle credential rotation
Consider using AWS SDK directly:
- Leverage built-in credential management
- Use AWS SDK's Bedrock client
- Benefit from automatic request signing
Would you like assistance in implementing these architectural improvements?
app/client/platforms/bedrock.ts (1)
18-25: Consider using an enum for role mapping.The ClaudeMapper could be more type-safe using an enum instead of a const object.
-const ClaudeMapper = { - assistant: "assistant", - user: "user", - system: "user", -} as const; - -type ClaudeRole = keyof typeof ClaudeMapper; +enum ClaudeRole { + Assistant = "assistant", + User = "user", + System = "user" +}app/locales/en.ts (3)
354-354: Improve the region validation message to be more helpful.The current validation message could be more informative by listing common regions.
- Invalid: "Invalid AWS region format. Example: us-west-2", + Invalid: "Invalid AWS region format. Must be a valid AWS region code (e.g., us-west-2, us-east-1, eu-west-1)",
365-365: Use a more descriptive placeholder for the secret key.The current placeholder
****is not descriptive enough and doesn't indicate what type of input is expected.- Placeholder: "****", + Placeholder: "Enter your AWS secret access key",
370-374: Improve encryption key description and fix grammatical error.The subtitle should better explain the purpose of the encryption key, and the validation message has a grammatical error.
- SubTitle: "Your encryption key for configuration data", + SubTitle: "Optional: Your encryption key to secure AWS credentials in local storage", Placeholder: "Enter encryption key", Invalid: - "Invalid encryption key format. Must no less than 8 characters long!", + "Invalid encryption key format. Must be at least 8 characters long!",
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (10)
.env.template(1 hunks)app/api/bedrock.ts(1 hunks)app/client/api.ts(6 hunks)app/client/platforms/bedrock.ts(1 hunks)app/components/settings.tsx(2 hunks)app/config/server.ts(3 hunks)app/locales/cn.ts(1 hunks)app/locales/en.ts(1 hunks)app/store/access.ts(6 hunks)app/utils/aws.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (2)
- .env.template
- app/config/server.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/api/bedrock.ts
[error] 91-91: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
app/components/settings.tsx
[error] 1034-1034: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
[error] 1041-1041: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
app/utils/aws.ts
[error] 288-288: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 288-288: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 339-339: This code is unreachable
... because this statement will return from the function beforehand
(lint/correctness/noUnreachable)
[error] 386-386: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 386-386: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (15)
app/api/bedrock.ts (1)
232-233:
Sanitize error responses
Exposing detailed error information through prettyObject(e) could reveal sensitive system information.
Apply this diff:
- console.error("Handler error:", e);
+ console.error("Handler error:", e instanceof Error ? e.message : "Unknown error");
return NextResponse.json(
- prettyObject(e),
+ {
+ error: true,
+ message: "An internal server error occurred"
+ },
+ { status: 500 }
);Likely invalid or redundant comment.
app/store/access.ts (3)
18-18: LGTM: Import changes are appropriate.
The new imports for Bedrock base URL and encryption utilities are correctly added and necessary for the AWS Bedrock integration.
Also applies to: 26-26
44-44: LGTM: URL constant follows established pattern.
The DEFAULT_BEDROCK_URL constant follows the same conditional pattern as other service URLs, maintaining consistency.
219-219: LGTM: Authorization check properly updated.
The addition of isValidBedrock() to the authorization check follows the established pattern and is correctly implemented.
app/client/api.ts (4)
25-26: LGTM: Import statements are properly organized
The new imports for Bedrock integration follow the existing pattern and are correctly placed.
135-137: LGTM: Bedrock provider integration follows existing patterns
The Bedrock case is properly integrated into the constructor's switch statement, maintaining consistency with other providers.
355-356: LGTM: Bedrock client API initialization is consistent
The Bedrock case is properly integrated into the getClientApi function, maintaining consistency with other providers.
279-288:
Critical: Improve AWS credential handling
The current implementation has several security and maintainability concerns:
- Concatenating encrypted credentials with ":" is error-prone and makes parsing difficult
- Direct usage of encryption key in this context could expose it
- Missing validation for AWS credentials before encryption
Consider this safer approach:
: isBedrock
? accessStore.awsRegion &&
accessStore.awsAccessKey &&
accessStore.awsSecretKey
- ? encrypt(accessStore.awsRegion, accessStore.encryptionKey) +
- ":" +
- encrypt(accessStore.awsAccessKey, accessStore.encryptionKey) +
- ":" +
- encrypt(accessStore.awsSecretKey, accessStore.encryptionKey)
- : ""
+ ? JSON.stringify({
+ region: encrypt(accessStore.awsRegion),
+ accessKey: encrypt(accessStore.awsAccessKey),
+ secretKey: encrypt(accessStore.awsSecretKey)
+ })
+ : ""Let's verify the current credential handling implementation:
app/client/platforms/bedrock.ts (3)
27-29: LGTM: Clear error message for unsupported feature.
The speech method correctly throws an error indicating that the functionality is not implemented.
364-370: 🛠️ Refactor suggestion
Implement proper error handling for unsupported operations.
The methods return dummy values without indicating that the operations are unsupported.
async usage() {
- return { used: 0, total: 0 };
+ throw new Error(
+ "Usage tracking is not supported for Bedrock API. Use AWS CloudWatch metrics instead."
+ );
}
async models() {
- return [];
+ throw new Error(
+ "Model listing is not supported for Bedrock API. Configure available models in AWS Console."
+ );
}Likely invalid or redundant comment.
130-137:
Add validation for image URL parsing.
The image URL parsing lacks validation which could lead to runtime errors if the URL format is invalid.
const colonIndex = url.indexOf(":");
const semicolonIndex = url.indexOf(";");
const comma = url.indexOf(",");
+
+if (colonIndex === -1 || semicolonIndex === -1 || comma === -1) {
+ throw new Error(`Invalid image URL format: ${url}`);
+}
+
+if (colonIndex >= semicolonIndex || semicolonIndex >= comma) {
+ throw new Error(`Invalid image URL component order: ${url}`);
+}
const mimeType = url.slice(colonIndex + 1, semicolonIndex);
const encodeType = url.slice(semicolonIndex + 1, comma);
const data = url.slice(comma + 1);Likely invalid or redundant comment.
app/utils/aws.ts (1)
62-66:
Use constant-time operations for masking sensitive values
The current implementation might be vulnerable to timing attacks. Use constant-time operations when handling sensitive data.
export function maskSensitiveValue(value: string): string {
+ if (typeof value !== 'string') {
+ throw new TypeError('Input must be a string');
+ }
if (!value) return "";
if (value.length <= 4) return value;
- return "*".repeat(value.length - 4) + value.slice(-4);
+ const masked = Buffer.alloc(value.length - 4, '*').toString();
+ return masked + value.slice(-4);
}Likely invalid or redundant comment.
app/components/settings.tsx (3)
1809-1809: LGTM!
The Bedrock component is correctly integrated into the provider configuration list, following the established pattern.
1005-1022: 🛠️ Refactor suggestion
Add AWS secret key validation.
Add validation for AWS secret key format (40-character string):
onChange={(e) => {
accessStore.update((access) => {
const secretKey = e.currentTarget.value;
+ if (secretKey && !/^[A-Za-z0-9/+=]{40}$/.test(secretKey)) {
+ showToast(Locale.Settings.Access.Bedrock.SecretKey.Invalid);
+ return;
+ }
access.awsSecretKey = secretKey;
});
}}Likely invalid or redundant comment.
967-986: 🛠️ Refactor suggestion
Add AWS region validation and improve code style.
The AWS region input needs validation and the code style can be improved:
- Add validation for AWS region format
- Use a predefined list of valid AWS regions
- Refactor the assignment expression
+const AWS_REGIONS = [
+ 'us-west-2', 'us-east-1', 'us-east-2', 'eu-west-1', 'eu-central-1',
+ 'ap-southeast-1', 'ap-southeast-2', 'ap-northeast-1'
+];
+
onChange={(e) =>
accessStore.update((access) => {
const region = e.currentTarget.value;
+ if (!AWS_REGIONS.includes(region)) {
+ showToast(Locale.Settings.Access.Bedrock.Region.Invalid);
+ return;
+ }
access.awsRegion = region;
})
}Likely invalid or redundant comment.
| console.log("[Bedrock Request] Endpoint:", endpoint); | ||
| console.log("[Bedrock Request] Model ID:", modelId); | ||
|
|
There was a problem hiding this comment.
Remove sensitive information from logs
Logging request details could expose sensitive information.
Apply this diff:
- console.log("[Bedrock Request] Endpoint:", endpoint);
- console.log("[Bedrock Request] Model ID:", modelId);
+ console.log("[Bedrock Request] Initiating request");
- console.log(
- "[Bedrock Request] Body:",
- JSON.stringify(requestBody, null, 2),
- );
+ console.log("[Bedrock Request] Sending request");Also applies to: 126-129
There was a problem hiding this comment.
Actionable comments posted: 13
🧹 Outside diff range and nitpick comments (4)
app/client/platforms/bedrock.ts (4)
27-29: Enhance error message for unimplemented speech method.The error message should be more descriptive about why the feature is not available.
- throw new Error("Speech not implemented for Bedrock."); + throw new Error("Speech synthesis is not supported by AWS Bedrock API.");
177-182: Add validation for empty messages in Claude format.The code adds a placeholder user message without validating if it's necessary.
- if (prompt[0]?.role === "assistant") { + if (!prompt.length || prompt[0]?.role === "assistant") { prompt.unshift({ role: "user", - content: ";", + content: "Hello", // More meaningful placeholder }); }
348-352: Enhance error handling for failed requests.The error message could be more informative while still being secure.
if (!res.ok) { const errorText = await res.text(); console.error("[Bedrock Client] Error response:", errorText); - throw new Error(`Request failed: ${errorText}`); + throw new Error( + `Bedrock API request failed with status ${res.status}. Check logs for details.` + ); }
26-26: Consider splitting BedrockApi into smaller, focused classes.The
BedrockApiclass has multiple responsibilities:
- Model-specific request formatting
- Chat handling
- Tool processing
- Path construction
Consider extracting these into separate classes or modules for better maintainability and testability.
Suggested structure:
- Create model-specific formatters (TitanFormatter, LlamaFormatter, etc.)
- Extract streaming logic into a separate StreamHandler class
- Move tool processing into a dedicated ToolProcessor class
- Create a separate URLBuilder for path construction
This would make the code more modular and easier to test and maintain.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (4)
app/api/bedrock.ts(1 hunks)app/client/platforms/bedrock.ts(1 hunks)app/utils.ts(1 hunks)app/utils/aws.ts(1 hunks)
🚧 Files skipped from review as they are similar to previous changes (1)
- app/utils.ts
🧰 Additional context used
🪛 Biome (1.9.4)
app/api/bedrock.ts
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 125-125: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 210-210: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
app/utils/aws.ts
[error] 285-285: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 285-285: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (3)
app/api/bedrock.ts (2)
47-50:
Remove sensitive information from error messages
The error message reveals implementation details about encryption mechanisms.
Apply this diff:
- throw new Error(
- "Failed to decrypt AWS credentials. Please ensure ENCRYPTION_KEY is set correctly.",
- );
+ throw new Error("Invalid or expired credentials");Likely invalid or redundant comment.
22-24:
Add validation for AWS credential formats
The AWS credentials are used without format validation, which could lead to runtime errors.
Apply this diff to add validation:
+ const AWS_REGION_PATTERN = /^[a-z]{2}-[a-z]+-\d{1}$/;
+ const AWS_ACCESS_KEY_PATTERN = /^AKIA[0-9A-Z]{16}$/;
+
let awsRegion = config.awsRegion;
let awsAccessKey = config.awsAccessKey;
let awsSecretKey = config.awsSecretKey;
+
+ if (awsRegion && !AWS_REGION_PATTERN.test(awsRegion)) {
+ throw new Error("Invalid AWS region format");
+ }
+
+ if (awsAccessKey && !AWS_ACCESS_KEY_PATTERN.test(awsAccessKey)) {
+ throw new Error("Invalid AWS access key format");
+ }Likely invalid or redundant comment.
app/client/platforms/bedrock.ts (1)
31-33: 🛠️ Refactor suggestion
Add type safety for model configuration.
The modelConfig parameter uses any type which reduces type safety.
-formatRequestBody(messages: ChatOptions["messages"], modelConfig: any) {
+interface ModelConfig {
+ model: string;
+ max_tokens?: number;
+ temperature?: number;
+ top_p?: number;
+ top_k?: number;
+}
+formatRequestBody(messages: ChatOptions["messages"], modelConfig: ModelConfig) {Likely invalid or redundant comment.
| console.log("[Bedrock Client] Request:", { | ||
| path: chatPath, | ||
| model: modelConfig.model, | ||
| messages: messages.length, | ||
| stream: shouldStream, | ||
| }); | ||
|
|
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve logging security in production.
The current logging could expose sensitive information in production.
- console.log("[Bedrock Client] Request:", {
+ if (process.env.NODE_ENV !== 'production') {
+ console.debug("[Bedrock Client] Request:", {
path: chatPath,
model: modelConfig.model,
messages: messages.length,
stream: shouldStream,
- });
+ });
+ }📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| console.log("[Bedrock Client] Request:", { | |
| path: chatPath, | |
| model: modelConfig.model, | |
| messages: messages.length, | |
| stream: shouldStream, | |
| }); | |
| if (process.env.NODE_ENV !== 'production') { | |
| console.debug("[Bedrock Client] Request:", { | |
| path: chatPath, | |
| model: modelConfig.model, | |
| messages: messages.length, | |
| stream: shouldStream, | |
| }); | |
| } |
| console.error("[AWS Encryption Error] Encryption key is required"); | ||
| throw new Error("Encryption key is required for AWS credential encryption"); |
There was a problem hiding this comment.
Remove detailed error logging in encryption function
Logging encryption errors could expose sensitive information. Replace with generic error messages.
- console.error("[AWS Encryption Error] Encryption key is required");
throw new Error("Encryption key is required for AWS credential encryption");
}
try {
return AES.encrypt(data, encryptionKey).toString();
} catch (error) {
- console.error("[AWS Encryption Error]:", error);
throw new Error("Failed to encrypt AWS credentials");Also applies to: 37-38
| body: any; | ||
| credentials: BedrockCredentials; |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Improve type safety by avoiding any type
The body property in BedrockRequestConfig uses any type which could lead to runtime errors. Consider creating a union type for different model payloads.
- body: any;
+ body: {
+ input?: string;
+ max_tokens?: number;
+ temperature?: number;
+ top_p?: number;
+ stop_sequences?: string[];
+ anthropic_version?: string; // For Claude models
+ system?: string; // For Mistral models
+ messages?: Array<{ // For chat models
+ role: 'user' | 'assistant' | 'system';
+ content: string;
+ }>;
+ };📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| body: any; | |
| credentials: BedrockCredentials; | |
| body: { | |
| input?: string; | |
| max_tokens?: number; | |
| temperature?: number; | |
| top_p?: number; | |
| stop_sequences?: string[]; | |
| anthropic_version?: string; // For Claude models | |
| system?: string; // For Mistral models | |
| messages?: Array<{ // For chat models | |
| role: 'user' | 'assistant' | 'system'; | |
| content: string; | |
| }>; | |
| }; | |
| credentials: BedrockCredentials; |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/utils/aws.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/utils/aws.ts
[error] 285-285: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 285-285: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
🔇 Additional comments (4)
app/utils/aws.ts (4)
153-162:
Add input validation to sign function.
The sign function should validate all required parameters before processing.
export async function sign({
method,
url,
region,
accessKeyId,
secretAccessKey,
body,
service,
isStreaming = true,
}: SignParams): Promise<Record<string, string>> {
+ if (!method || !url || !region || !accessKeyId || !secretAccessKey || !service) {
+ throw new Error("Missing required parameters for AWS request signing");
+ }
+ if (!url.startsWith('https://')) {
+ throw new Error('URL must use HTTPS protocol');
+ }
try {Likely invalid or redundant comment.
62-66:
Add input validation and use constant-time comparison.
The maskSensitiveValue function should validate input types and use constant-time operations to prevent timing attacks when handling sensitive data.
export function maskSensitiveValue(value: string): string {
+ if (typeof value !== 'string') {
+ throw new TypeError('Input must be a string');
+ }
if (!value) return "";
if (value.length <= 4) return value;
- return "*".repeat(value.length - 4) + value.slice(-4);
+ const masked = Buffer.alloc(value.length - 4, '*').toString();
+ return masked + value.slice(-4);
}Likely invalid or redundant comment.
285-285:
Replace control character regex with Unicode escapes.
Using control characters in regex patterns is problematic and flagged by static analysis.
- const cleanText = text.replace(/[\x00-\x1F\x7F-\x9F]/g, "");
+ const cleanText = text.replace(/[\u0000-\u001F\u007F-\u009F]/g, "");Likely invalid or redundant comment.
🧰 Tools
🪛 Biome (1.9.4)
[error] 285-285: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
[error] 285-285: Unexpected control character in a regular expression.
Control characters are unusual and potentially incorrect inputs, so they are disallowed.
(lint/suspicious/noControlCharactersInRegex)
14-19: 🛠️ Refactor suggestion
Improve type safety by replacing any type with specific model payloads.
The body property using any type could lead to runtime errors. Consider creating a union type for different model payloads.
export interface BedrockRequestConfig {
modelId: string;
shouldStream: boolean;
- body: any;
+ body: {
+ prompt?: string;
+ max_tokens?: number;
+ temperature?: number;
+ top_p?: number;
+ stop_sequences?: string[];
+ anthropic_version?: string; // For Claude models
+ system?: string; // For Mistral models
+ messages?: Array<{ // For chat models
+ role: 'user' | 'assistant' | 'system';
+ content: string;
+ }>;
+ };
credentials: BedrockCredentials;
}Likely invalid or redundant comment.
| console.error("[AWS Extract Error] extractMessage Empty response"); | ||
| return ""; | ||
| } | ||
| // console.log("[Response] extractMessage response: ", res); |
There was a problem hiding this comment.
Remove debug logging statements.
Production code should not contain debug logging statements that could expose sensitive information.
- console.error("[AWS Extract Error] extractMessage Empty response");
return "";
}
- console.log("[Response] extractMessage response: ", res);
// Handle Mistral model response format
- console.log("parseEventData=========================");
- console.log(parsed);
// Handle Mistral models
- console.log("accumulatedText=========================");
- console.log(accumulatedText);Also applies to: 350-351, 360-361
| export function encrypt(data: string, encryptionKey: string): string { | ||
| if (!data) return ""; | ||
| if (!encryptionKey) { | ||
| console.error("[AWS Encryption Error] Encryption key is required"); | ||
| throw new Error("Encryption key is required for AWS credential encryption"); | ||
| } | ||
| try { | ||
| return AES.encrypt(data, encryptionKey).toString(); | ||
| } catch (error) { | ||
| console.error("[AWS Encryption Error]:", error); | ||
| throw new Error("Failed to encrypt AWS credentials"); | ||
| } | ||
| } |
There was a problem hiding this comment.
Remove detailed error logging in encryption function.
Logging encryption errors could expose sensitive information. Replace with generic error messages.
export function encrypt(data: string, encryptionKey: string): string {
if (!data) return "";
if (!encryptionKey) {
- console.error("[AWS Encryption Error] Encryption key is required");
throw new Error("Encryption key is required for AWS credential encryption");
}
try {
return AES.encrypt(data, encryptionKey).toString();
} catch (error) {
- console.error("[AWS Encryption Error]:", error);
throw new Error("Failed to encrypt AWS credentials");
}
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| export function encrypt(data: string, encryptionKey: string): string { | |
| if (!data) return ""; | |
| if (!encryptionKey) { | |
| console.error("[AWS Encryption Error] Encryption key is required"); | |
| throw new Error("Encryption key is required for AWS credential encryption"); | |
| } | |
| try { | |
| return AES.encrypt(data, encryptionKey).toString(); | |
| } catch (error) { | |
| console.error("[AWS Encryption Error]:", error); | |
| throw new Error("Failed to encrypt AWS credentials"); | |
| } | |
| } | |
| export function encrypt(data: string, encryptionKey: string): string { | |
| if (!data) return ""; | |
| if (!encryptionKey) { | |
| throw new Error("Encryption key is required for AWS credential encryption"); | |
| } | |
| try { | |
| return AES.encrypt(data, encryptionKey).toString(); | |
| } catch (error) { | |
| throw new Error("Failed to encrypt AWS credentials"); | |
| } | |
| } |
There was a problem hiding this comment.
Actionable comments posted: 2
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
📒 Files selected for processing (1)
app/api/bedrock.ts(1 hunks)
🧰 Additional context used
🪛 Biome (1.9.4)
app/api/bedrock.ts
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 125-125: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
[error] 210-210: Change to an optional chain.
Unsafe fix: Change to an optional chain.
(lint/complexity/useOptionalChain)
🔇 Additional comments (4)
app/api/bedrock.ts (4)
104-106:
Remove sensitive information from logs
Logging request details could expose sensitive information.
Apply this diff:
- console.log("[Bedrock Request] Endpoint:", endpoint);
- console.log("[Bedrock Request] Model ID:", modelId);
+ console.log("[Bedrock Request] Initiating request");Likely invalid or redundant comment.
61-62: 🛠️ Refactor suggestion
Enhance timeout handling
The current timeout implementation could leave resources hanging.
Apply this diff:
const controller = new AbortController();
- const timeoutId = setTimeout(() => controller.abort(), 10 * 60 * 1000);
+ const TIMEOUT_MS = 10 * 60 * 1000;
+ const timeoutId = setTimeout(() => {
+ controller.abort();
+ throw new Error(`Request timed out after ${TIMEOUT_MS/1000} seconds`);
+ }, TIMEOUT_MS);Likely invalid or redundant comment.
264-265:
Sanitize error responses
Exposing raw error details in the response could reveal sensitive information about the system.
Apply this diff:
- console.error("Handler error:", e);
+ console.error("Handler error:", e instanceof Error ? e.message : "Unknown error");
return NextResponse.json(
- prettyObject(e),
+ {
+ error: true,
+ message: "An internal server error occurred"
+ },
+ { status: 500 }
);Likely invalid or redundant comment.
47-50:
Remove sensitive information from error messages
The error message reveals information about the encryption mechanism and configuration.
Apply this diff:
- throw new Error(
- "Failed to decrypt AWS credentials. Please ensure ENCRYPTION_KEY is set correctly.",
- );
+ throw new Error("Invalid or expired credentials");Likely invalid or redundant comment.
| awsRegion = decrypt(encryptedRegion, encryptionKey); | ||
| awsAccessKey = decrypt(encryptedAccessKey, encryptionKey); | ||
| awsSecretKey = decrypt(encryptedSecretKey, encryptionKey); |
There was a problem hiding this comment.
Critical: Replace insecure credential decryption mechanism
The current implementation uses a basic decryption mechanism that may not be secure enough for handling AWS credentials. Consider using AWS's secure credential management solutions:
- AWS Secrets Manager
- AWS KMS for encryption/decryption
- AWS IAM roles and temporary credentials
| let tools: any[] | undefined; | ||
| if (bodyJson.tools) { | ||
| tools = bodyJson.tools; | ||
| delete bodyJson.tools; // Remove from main request body | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Replace 'any' type and avoid delete operator
Using 'any' type reduces type safety, and the delete operator can impact performance.
Apply this diff:
- let tools: any[] | undefined;
+ interface Tool {
+ name: string;
+ description: string;
+ input_schema: Record<string, unknown>;
+ }
+ let tools: Tool[] | undefined;
if (bodyJson.tools) {
tools = bodyJson.tools;
- delete bodyJson.tools; // Remove from main request body
+ const { tools: _, ...restBody } = bodyJson; // Use destructuring
+ bodyJson = restBody;
}📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| let tools: any[] | undefined; | |
| if (bodyJson.tools) { | |
| tools = bodyJson.tools; | |
| delete bodyJson.tools; // Remove from main request body | |
| } | |
| interface Tool { | |
| name: string; | |
| description: string; | |
| input_schema: Record<string, unknown>; | |
| } | |
| let tools: Tool[] | undefined; | |
| if (bodyJson.tools) { | |
| tools = bodyJson.tools; | |
| const { tools: _, ...restBody } = bodyJson; // Use destructuring | |
| bodyJson = restBody; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 94-94: Avoid the delete operator which can impact performance.
Unsafe fix: Use an undefined assignment instead.
(lint/performance/noDelete)
💻 变更类型 | Change Type
🔀 变更说明 | Description of Change
This PR introduces AWS Bedrock to support the leading large language models such as Claude, Llama, Mistral, and others.
API Enhancements:
app/api/bedrock.ts)app/client/api.ts)app/client/platforms/bedrock.ts)Localization:
app/locales/cn.ts)app/locales/en.ts)Configuration:
app/constant.ts)📝 补充信息 | Additional Information
These changes leveraging Amazon Bedrock provides access to cutting-edge language models, offering several advantages:
Access to State-of-the-Art Models: Users can now utilize the latest versions of Llama 3.2, Mistral Large, and Claude 3.5, which are known for their advanced natural language understanding and generation capabilities.
Summary by CodeRabbit
Release Notes
New Features
FullScreencomponent for improved UI interactions.PasswordInputcomponent to include an optional prop for masking sensitive values.Bug Fixes
Documentation
Chores