bedrock provider cleanup #1276
bedrock provider cleanup #1276
Conversation
|
DescriptionSummary By MatterAI
🔄 What ChangedThis pull request focuses on refining the Bedrock provider's API configuration and utility functions. Key changes include enhancing the 🔍 Impact of the ChangeThese changes significantly improve the flexibility and accuracy of the Bedrock provider. The enhanced 📁 Total Files Changed
🧪 Test AddedN/A 🔒Security VulnerabilitiesN/A MotivationThis PR aims to improve the robustness, flexibility, and accuracy of the Bedrock provider. Specifically, it addresses the need for better proxy handling, consistent AWS endpoint resolution, and accurate usage reporting for AI model interactions. These changes contribute to a more reliable and maintainable integration with AWS Bedrock services. Type of Change
How Has This Been Tested?
Screenshots (if applicable)N/A Checklist
Related IssuesN/A Tip Quality Recommendations
Tanka Poem ♫
|
![matter-code-review[bot]](https://avatars.githubusercontent.com/in/474549?s=60&v=4){kind=small}
| export function getValueOrFileContents(value?: string, ignore?: boolean) { | ||
| if (!value || ignore) return value; | ||
|
|
||
| try { | ||
| // Check if value looks like a file path | ||
| if ( | ||
| value.startsWith('/') || | ||
| value.startsWith('./') || | ||
| value.startsWith('../') | ||
| ) { | ||
| // Resolve the path (handle relative paths) | ||
| const resolvedPath = path.resolve(value); | ||
|
|
||
| // Check if file exists | ||
| if (fs.existsSync(resolvedPath)) { | ||
| // File exists, read and return its contents | ||
| return fs.readFileSync(resolvedPath, 'utf8').trim(); | ||
| } | ||
| } | ||
|
|
||
| // If not a file path or file doesn't exist, return value as is | ||
| return value; | ||
| } catch (error: any) { | ||
| console.log(`Error reading file at ${value}: ${error.message}`); | ||
| // Return the original value if there's an error | ||
| return value; | ||
| } | ||
| } |
There was a problem hiding this comment.
🔒 Security Issue Fix
Issue: Path traversal vulnerability - the function accepts any file path without validation, potentially allowing access to sensitive files outside intended directories
Fix: Add path validation to restrict file access to safe directories
Impact: Prevents potential security breach through malicious file path injection
| export function getValueOrFileContents(value?: string, ignore?: boolean) { | |
| if (!value || ignore) return value; | |
| try { | |
| // Check if value looks like a file path | |
| if ( | |
| value.startsWith('/') || | |
| value.startsWith('./') || | |
| value.startsWith('../') | |
| ) { | |
| // Resolve the path (handle relative paths) | |
| const resolvedPath = path.resolve(value); | |
| // Check if file exists | |
| if (fs.existsSync(resolvedPath)) { | |
| // File exists, read and return its contents | |
| return fs.readFileSync(resolvedPath, 'utf8').trim(); | |
| } | |
| } | |
| // If not a file path or file doesn't exist, return value as is | |
| return value; | |
| } catch (error: any) { | |
| console.log(`Error reading file at ${value}: ${error.message}`); | |
| // Return the original value if there's an error | |
| return value; | |
| } | |
| } | |
| export function getValueOrFileContents(value?: string, ignore?: boolean) { | |
| if (!value || ignore) return value; | |
| try { | |
| // Check if value looks like a file path | |
| if ( | |
| value.startsWith('/') || | |
| value.startsWith('./') || | |
| value.startsWith('../') | |
| ) { | |
| // Resolve the path (handle relative paths) | |
| const resolvedPath = path.resolve(value); | |
| // Security: Validate path is within allowed directories | |
| const allowedPaths = [process.cwd(), '/etc/ssl/certs', '/var/secrets']; | |
| const isPathAllowed = allowedPaths.some(allowedPath => | |
| resolvedPath.startsWith(path.resolve(allowedPath)) | |
| ); | |
| if (!isPathAllowed) { | |
| console.warn(`Access denied to path outside allowed directories: ${resolvedPath}`); | |
| return value; | |
| } | |
| // Check if file exists | |
| if (fs.existsSync(resolvedPath)) { | |
| // File exists, read and return its contents | |
| return fs.readFileSync(resolvedPath, 'utf8').trim(); | |
| } | |
| } | |
| // If not a file path or file doesn't exist, return value as is | |
| return value; | |
| } catch (error: any) { | |
| console.log(`Error reading file at ${value}: ${error.message}`); | |
| // Return the original value if there's an error | |
| return value; | |
| } | |
| } |
| const nodeEnv = { | ||
| NODE_ENV: getValueOrFileContents(process.env.NODE_ENV, true), | ||
| PORT: getValueOrFileContents(process.env.PORT) || 8787, | ||
|
|
||
| TLS_KEY_PATH: getValueOrFileContents(process.env.TLS_KEY_PATH, true), | ||
| TLS_CERT_PATH: getValueOrFileContents(process.env.TLS_CERT_PATH, true), | ||
| TLS_CA_PATH: getValueOrFileContents(process.env.TLS_CA_PATH, true), | ||
|
|
||
| AWS_ACCESS_KEY_ID: getValueOrFileContents(process.env.AWS_ACCESS_KEY_ID), | ||
| AWS_SECRET_ACCESS_KEY: getValueOrFileContents( | ||
| process.env.AWS_SECRET_ACCESS_KEY | ||
| ), | ||
| AWS_SESSION_TOKEN: getValueOrFileContents(process.env.AWS_SESSION_TOKEN), | ||
| AWS_ROLE_ARN: getValueOrFileContents(process.env.AWS_ROLE_ARN), | ||
| AWS_PROFILE: getValueOrFileContents(process.env.AWS_PROFILE, true), | ||
| AWS_WEB_IDENTITY_TOKEN_FILE: getValueOrFileContents( | ||
| process.env.AWS_WEB_IDENTITY_TOKEN_FILE, | ||
| true | ||
| ), | ||
| AWS_CONTAINER_CREDENTIALS_RELATIVE_URI: getValueOrFileContents( | ||
| process.env.AWS_CONTAINER_CREDENTIALS_RELATIVE_URI, | ||
| true | ||
| ), | ||
| AWS_ASSUME_ROLE_ACCESS_KEY_ID: getValueOrFileContents( | ||
| process.env.AWS_ASSUME_ROLE_ACCESS_KEY_ID | ||
| ), | ||
| AWS_ASSUME_ROLE_SECRET_ACCESS_KEY: getValueOrFileContents( | ||
| process.env.AWS_ASSUME_ROLE_SECRET_ACCESS_KEY | ||
| ), | ||
| AWS_ASSUME_ROLE_REGION: getValueOrFileContents( | ||
| process.env.AWS_ASSUME_ROLE_REGION | ||
| ), | ||
| AWS_REGION: getValueOrFileContents(process.env.AWS_REGION), | ||
| AWS_ENDPOINT_DOMAIN: getValueOrFileContents(process.env.AWS_ENDPOINT_DOMAIN), | ||
| AWS_IMDS_V1: getValueOrFileContents(process.env.AWS_IMDS_V1), | ||
|
|
||
| AZURE_AUTH_MODE: getValueOrFileContents(process.env.AZURE_AUTH_MODE), | ||
| AZURE_ENTRA_CLIENT_ID: getValueOrFileContents( | ||
| process.env.AZURE_ENTRA_CLIENT_ID | ||
| ), | ||
| AZURE_ENTRA_CLIENT_SECRET: getValueOrFileContents( | ||
| process.env.AZURE_ENTRA_CLIENT_SECRET | ||
| ), | ||
| AZURE_ENTRA_TENANT_ID: getValueOrFileContents( | ||
| process.env.AZURE_ENTRA_TENANT_ID | ||
| ), | ||
| AZURE_MANAGED_CLIENT_ID: getValueOrFileContents( | ||
| process.env.AZURE_MANAGED_CLIENT_ID | ||
| ), | ||
| AZURE_MANAGED_VERSION: getValueOrFileContents( | ||
| process.env.AZURE_MANAGED_VERSION | ||
| ), | ||
| AZURE_IDENTITY_ENDPOINT: getValueOrFileContents( | ||
| process.env.IDENTITY_ENDPOINT, | ||
| true | ||
| ), | ||
| AZURE_MANAGED_IDENTITY_HEADER: getValueOrFileContents( | ||
| process.env.IDENTITY_HEADER | ||
| ), | ||
|
|
||
| SSE_ENCRYPTION_TYPE: getValueOrFileContents(process.env.SSE_ENCRYPTION_TYPE), | ||
| KMS_KEY_ID: getValueOrFileContents(process.env.KMS_KEY_ID), | ||
| KMS_BUCKET_KEY_ENABLED: getValueOrFileContents( | ||
| process.env.KMS_BUCKET_KEY_ENABLED | ||
| ), | ||
| KMS_ENCRYPTION_CONTEXT: getValueOrFileContents( | ||
| process.env.KMS_ENCRYPTION_CONTEXT | ||
| ), | ||
| KMS_ENCRYPTION_ALGORITHM: getValueOrFileContents( | ||
| process.env.KMS_ENCRYPTION_ALGORITHM | ||
| ), | ||
| KMS_ENCRYPTION_CUSTOMER_KEY: getValueOrFileContents( | ||
| process.env.KMS_ENCRYPTION_CUSTOMER_KEY | ||
| ), | ||
| KMS_ENCRYPTION_CUSTOMER_KEY_MD5: getValueOrFileContents( | ||
| process.env.KMS_ENCRYPTION_CUSTOMER_KEY_MD5 | ||
| ), | ||
| KMS_ROLE_ARN: getValueOrFileContents(process.env.KMS_ROLE_ARN), | ||
|
|
||
| HTTP_PROXY: getValueOrFileContents(process.env.HTTP_PROXY), | ||
| HTTPS_PROXY: getValueOrFileContents(process.env.HTTPS_PROXY), | ||
| }; | ||
|
|
||
| export const Environment = (c?: Context) => { | ||
| if (isNodeInstance) { | ||
| return nodeEnv; | ||
| } | ||
| if (c) { | ||
| return env(c); | ||
| } | ||
| return {}; | ||
| }; |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Runtime check will fail in non-Node environments - getRuntimeKey() is called at module level, potentially causing errors in other runtimes
Fix: Move runtime check inside the function to avoid module-level execution issues
Impact: Prevents runtime errors when module is loaded in non-Node environments
| const nodeEnv = { | |
| NODE_ENV: getValueOrFileContents(process.env.NODE_ENV, true), | |
| PORT: getValueOrFileContents(process.env.PORT) || 8787, | |
| TLS_KEY_PATH: getValueOrFileContents(process.env.TLS_KEY_PATH, true), | |
| TLS_CERT_PATH: getValueOrFileContents(process.env.TLS_CERT_PATH, true), | |
| TLS_CA_PATH: getValueOrFileContents(process.env.TLS_CA_PATH, true), | |
| AWS_ACCESS_KEY_ID: getValueOrFileContents(process.env.AWS_ACCESS_KEY_ID), | |
| AWS_SECRET_ACCESS_KEY: getValueOrFileContents( | |
| process.env.AWS_SECRET_ACCESS_KEY | |
| ), | |
| AWS_SESSION_TOKEN: getValueOrFileContents(process.env.AWS_SESSION_TOKEN), | |
| AWS_ROLE_ARN: getValueOrFileContents(process.env.AWS_ROLE_ARN), | |
| AWS_PROFILE: getValueOrFileContents(process.env.AWS_PROFILE, true), | |
| AWS_WEB_IDENTITY_TOKEN_FILE: getValueOrFileContents( | |
| process.env.AWS_WEB_IDENTITY_TOKEN_FILE, | |
| true | |
| ), | |
| AWS_CONTAINER_CREDENTIALS_RELATIVE_URI: getValueOrFileContents( | |
| process.env.AWS_CONTAINER_CREDENTIALS_RELATIVE_URI, | |
| true | |
| ), | |
| AWS_ASSUME_ROLE_ACCESS_KEY_ID: getValueOrFileContents( | |
| process.env.AWS_ASSUME_ROLE_ACCESS_KEY_ID | |
| ), | |
| AWS_ASSUME_ROLE_SECRET_ACCESS_KEY: getValueOrFileContents( | |
| process.env.AWS_ASSUME_ROLE_SECRET_ACCESS_KEY | |
| ), | |
| AWS_ASSUME_ROLE_REGION: getValueOrFileContents( | |
| process.env.AWS_ASSUME_ROLE_REGION | |
| ), | |
| AWS_REGION: getValueOrFileContents(process.env.AWS_REGION), | |
| AWS_ENDPOINT_DOMAIN: getValueOrFileContents(process.env.AWS_ENDPOINT_DOMAIN), | |
| AWS_IMDS_V1: getValueOrFileContents(process.env.AWS_IMDS_V1), | |
| AZURE_AUTH_MODE: getValueOrFileContents(process.env.AZURE_AUTH_MODE), | |
| AZURE_ENTRA_CLIENT_ID: getValueOrFileContents( | |
| process.env.AZURE_ENTRA_CLIENT_ID | |
| ), | |
| AZURE_ENTRA_CLIENT_SECRET: getValueOrFileContents( | |
| process.env.AZURE_ENTRA_CLIENT_SECRET | |
| ), | |
| AZURE_ENTRA_TENANT_ID: getValueOrFileContents( | |
| process.env.AZURE_ENTRA_TENANT_ID | |
| ), | |
| AZURE_MANAGED_CLIENT_ID: getValueOrFileContents( | |
| process.env.AZURE_MANAGED_CLIENT_ID | |
| ), | |
| AZURE_MANAGED_VERSION: getValueOrFileContents( | |
| process.env.AZURE_MANAGED_VERSION | |
| ), | |
| AZURE_IDENTITY_ENDPOINT: getValueOrFileContents( | |
| process.env.IDENTITY_ENDPOINT, | |
| true | |
| ), | |
| AZURE_MANAGED_IDENTITY_HEADER: getValueOrFileContents( | |
| process.env.IDENTITY_HEADER | |
| ), | |
| SSE_ENCRYPTION_TYPE: getValueOrFileContents(process.env.SSE_ENCRYPTION_TYPE), | |
| KMS_KEY_ID: getValueOrFileContents(process.env.KMS_KEY_ID), | |
| KMS_BUCKET_KEY_ENABLED: getValueOrFileContents( | |
| process.env.KMS_BUCKET_KEY_ENABLED | |
| ), | |
| KMS_ENCRYPTION_CONTEXT: getValueOrFileContents( | |
| process.env.KMS_ENCRYPTION_CONTEXT | |
| ), | |
| KMS_ENCRYPTION_ALGORITHM: getValueOrFileContents( | |
| process.env.KMS_ENCRYPTION_ALGORITHM | |
| ), | |
| KMS_ENCRYPTION_CUSTOMER_KEY: getValueOrFileContents( | |
| process.env.KMS_ENCRYPTION_CUSTOMER_KEY | |
| ), | |
| KMS_ENCRYPTION_CUSTOMER_KEY_MD5: getValueOrFileContents( | |
| process.env.KMS_ENCRYPTION_CUSTOMER_KEY_MD5 | |
| ), | |
| KMS_ROLE_ARN: getValueOrFileContents(process.env.KMS_ROLE_ARN), | |
| HTTP_PROXY: getValueOrFileContents(process.env.HTTP_PROXY), | |
| HTTPS_PROXY: getValueOrFileContents(process.env.HTTPS_PROXY), | |
| }; | |
| export const Environment = (c?: Context) => { | |
| if (isNodeInstance) { | |
| return nodeEnv; | |
| } | |
| if (c) { | |
| return env(c); | |
| } | |
| return {}; | |
| }; | |
| const getNodeEnv = () => ({ | |
| NODE_ENV: getValueOrFileContents(process.env.NODE_ENV, true), | |
| PORT: getValueOrFileContents(process.env.PORT) || 8787, | |
| TLS_KEY_PATH: getValueOrFileContents(process.env.TLS_KEY_PATH, true), | |
| TLS_CERT_PATH: getValueOrFileContents(process.env.TLS_CERT_PATH, true), | |
| TLS_CA_PATH: getValueOrFileContents(process.env.TLS_CA_PATH, true), | |
| AWS_ACCESS_KEY_ID: getValueOrFileContents(process.env.AWS_ACCESS_KEY_ID), | |
| AWS_SECRET_ACCESS_KEY: getValueOrFileContents( | |
| process.env.AWS_SECRET_ACCESS_KEY | |
| ), | |
| AWS_SESSION_TOKEN: getValueOrFileContents(process.env.AWS_SESSION_TOKEN), | |
| AWS_ROLE_ARN: getValueOrFileContents(process.env.AWS_ROLE_ARN), | |
| AWS_PROFILE: getValueOrFileContents(process.env.AWS_PROFILE, true), | |
| AWS_WEB_IDENTITY_TOKEN_FILE: getValueOrFileContents( | |
| process.env.AWS_WEB_IDENTITY_TOKEN_FILE, | |
| true | |
| ), | |
| AWS_CONTAINER_CREDENTIALS_RELATIVE_URI: getValueOrFileContents( | |
| process.env.AWS_CONTAINER_CREDENTIALS_RELATIVE_URI, | |
| true | |
| ), | |
| AWS_ASSUME_ROLE_ACCESS_KEY_ID: getValueOrFileContents( | |
| process.env.AWS_ASSUME_ROLE_ACCESS_KEY_ID | |
| ), | |
| AWS_ASSUME_ROLE_SECRET_ACCESS_KEY: getValueOrFileContents( | |
| process.env.AWS_ASSUME_ROLE_SECRET_ACCESS_KEY | |
| ), | |
| AWS_ASSUME_ROLE_REGION: getValueOrFileContents( | |
| process.env.AWS_ASSUME_ROLE_REGION | |
| ), | |
| AWS_REGION: getValueOrFileContents(process.env.AWS_REGION), | |
| AWS_ENDPOINT_DOMAIN: getValueOrFileContents(process.env.AWS_ENDPOINT_DOMAIN), | |
| AWS_IMDS_V1: getValueOrFileContents(process.env.AWS_IMDS_V1), | |
| AZURE_AUTH_MODE: getValueOrFileContents(process.env.AZURE_AUTH_MODE), | |
| AZURE_ENTRA_CLIENT_ID: getValueOrFileContents( | |
| process.env.AZURE_ENTRA_CLIENT_ID | |
| ), | |
| AZURE_ENTRA_CLIENT_SECRET: getValueOrFileContents( | |
| process.env.AZURE_ENTRA_CLIENT_SECRET | |
| ), | |
| AZURE_ENTRA_TENANT_ID: getValueOrFileContents( | |
| process.env.AZURE_ENTRA_TENANT_ID | |
| ), | |
| AZURE_MANAGED_CLIENT_ID: getValueOrFileContents( | |
| process.env.AZURE_MANAGED_CLIENT_ID | |
| ), | |
| AZURE_MANAGED_VERSION: getValueOrFileContents( | |
| process.env.AZURE_MANAGED_VERSION | |
| ), | |
| AZURE_IDENTITY_ENDPOINT: getValueOrFileContents( | |
| process.env.IDENTITY_ENDPOINT, | |
| true | |
| ), | |
| AZURE_MANAGED_IDENTITY_HEADER: getValueOrFileContents( | |
| process.env.IDENTITY_HEADER | |
| ), | |
| SSE_ENCRYPTION_TYPE: getValueOrFileContents(process.env.SSE_ENCRYPTION_TYPE), | |
| KMS_KEY_ID: getValueOrFileContents(process.env.KMS_KEY_ID), | |
| KMS_BUCKET_KEY_ENABLED: getValueOrFileContents( | |
| process.env.KMS_BUCKET_KEY_ENABLED | |
| ), | |
| KMS_ENCRYPTION_CONTEXT: getValueOrFileContents( | |
| process.env.KMS_ENCRYPTION_CONTEXT | |
| ), | |
| KMS_ENCRYPTION_ALGORITHM: getValueOrFileContents( | |
| process.env.KMS_ENCRYPTION_ALGORITHM | |
| ), | |
| KMS_ENCRYPTION_CUSTOMER_KEY: getValueOrFileContents( | |
| process.env.KMS_ENCRYPTION_CUSTOMER_KEY | |
| ), | |
| KMS_ENCRYPTION_CUSTOMER_KEY_MD5: getValueOrFileContents( | |
| process.env.KMS_ENCRYPTION_CUSTOMER_KEY_MD5 | |
| ), | |
| KMS_ROLE_ARN: getValueOrFileContents(process.env.KMS_ROLE_ARN), | |
| HTTP_PROXY: getValueOrFileContents(process.env.HTTP_PROXY), | |
| HTTPS_PROXY: getValueOrFileContents(process.env.HTTPS_PROXY), | |
| }); | |
| export const Environment = (c?: Context) => { | |
| const isNodeInstance = getRuntimeKey() === 'node'; | |
| if (isNodeInstance) { | |
| return getNodeEnv(); | |
| } | |
| if (c) { | |
| return env(c); | |
| } | |
| return {}; | |
| }; |
| const model = params.foundationModel || params.model || ''; | ||
| if (g1EmbedModels.includes(model)) return undefined; |
There was a problem hiding this comment.
🐛 Bug Fix
Issue: Potential null/undefined access - params.foundationModel and params.model could be undefined, causing the includes() check to fail
Fix: Add proper null/undefined checks before accessing model properties
Impact: Prevents runtime errors when model parameters are missing
| const model = params.foundationModel || params.model || ''; | |
| if (g1EmbedModels.includes(model)) return undefined; | |
| const model = params?.foundationModel || params?.model || ''; | |
| if (model && g1EmbedModels.includes(model)) return undefined; |
| headers: async ({ providerOptions, fn }) => { | ||
| const { | ||
| apiKey, | ||
| azureExtraParams, | ||
| azureExtraParameters, | ||
| azureDeploymentName, | ||
| azureAdToken, | ||
| azureAuthMode, | ||
| } = providerOptions; |
There was a problem hiding this comment.
📝 Documentation Gap
Issue: Missing JSDoc documentation for the azureExtraParameters field and its purpose
Fix: Add comprehensive documentation explaining the parameter's usage
Impact: Improves code maintainability and developer understanding
| headers: async ({ providerOptions, fn }) => { | |
| const { | |
| apiKey, | |
| azureExtraParams, | |
| azureExtraParameters, | |
| azureDeploymentName, | |
| azureAdToken, | |
| azureAuthMode, | |
| } = providerOptions; | |
| headers: async ({ providerOptions, fn }) => { | |
| const { | |
| apiKey, | |
| /** | |
| * Azure extra parameters configuration | |
| * Controls how extra parameters are handled in the request | |
| * @default 'drop' - drops extra parameters not supported by the model | |
| */ | |
| azureExtraParameters, | |
| azureDeploymentName, | |
| azureAdToken, | |
| azureAuthMode, | |
| } = providerOptions; |
|
Important PR Review SkippedPR review skipped as per the configuration setting. Run a manually review by commenting /matter review 💡Tips to use Matter AICommand List
|
Changes included in this PR:
Changes not included:
Support for IRSA and IMDS based auth