-
Notifications
You must be signed in to change notification settings - Fork 786
fix: support custom-host for azure plugin to support privatelink #1117
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.
| Original file line number | Diff line number | Diff line change | ||||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
|
|
@@ -123,6 +123,10 @@ | |||||||||||||
| "tenantId": { | ||||||||||||||
| "type": "string", | ||||||||||||||
| "description": "Tenant ID for Azure Entra ID authentication" | ||||||||||||||
| }, | ||||||||||||||
| "customHost": { | ||||||||||||||
| "type": "string", | ||||||||||||||
| "description": "Custom host for Azure AI services (Private Link etc.)" | ||||||||||||||
|
Comment on lines
+127
to
+129
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 📝 Documentation Improvement Issue: The description for customHost doesn't mention the security implications of using this feature.
Suggested change
|
||||||||||||||
| } | ||||||||||||||
| }, | ||||||||||||||
| "anyOf": [ | ||||||||||||||
|
|
||||||||||||||
| Original file line number | Diff line number | Diff line change | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|---|
| @@ -1,3 +1,4 @@ | ||||||||||||
| import { Agent } from 'https'; | ||||||||||||
| import { | ||||||||||||
| HookEventType, | ||||||||||||
| PluginContext, | ||||||||||||
|
|
@@ -29,7 +30,7 @@ const redact = async ( | |||||||||||
|
|
||||||||||||
| const apiVersion = parameters.apiVersion || '2024-11-01'; | ||||||||||||
|
|
||||||||||||
| const url = `https://${credentials?.resourceName}.cognitiveservices.azure.com/language/:analyze-text?api-version=${apiVersion}`; | ||||||||||||
| const url = `${credentials?.customHost || `https://${credentials?.resourceName}.cognitiveservices.azure.com`}/language/:analyze-text?api-version=${apiVersion}`; | ||||||||||||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 🛠️ Code Refactor Issue: The URL construction doesn't validate if the customHost is a valid URL format.
Suggested change
|
||||||||||||
|
|
||||||||||||
| const { token, error: tokenError } = await getAccessToken( | ||||||||||||
| credentials as any, | ||||||||||||
|
|
@@ -53,8 +54,24 @@ const redact = async ( | |||||||||||
| throw new Error('Unable to get access token'); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| let agent: Agent | null = null; | ||||||||||||
|
Collaborator
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. would suggest moving the initialization to a fetchUtils file and creating a method like |
||||||||||||
| // privatelink doesn't contain a valid certificate, skipping verification if it's customHost. | ||||||||||||
| // SECURITY NOTE: The following disables SSL certificate validation for custom hosts. | ||||||||||||
| // This is necessary for Azure Private Link endpoints that may use self-signed certificates, | ||||||||||||
| // but should only be used with trusted private endpoints. | ||||||||||||
| if (credentials?.customHost) { | ||||||||||||
| agent = new Agent({ | ||||||||||||
| rejectUnauthorized: false, | ||||||||||||
| }); | ||||||||||||
| } | ||||||||||||
|
|
||||||||||||
| const timeout = parameters.timeout || 5000; | ||||||||||||
| const response = await post(url, body, { headers }, timeout); | ||||||||||||
| const response = await post( | ||||||||||||
| url, | ||||||||||||
| body, | ||||||||||||
| { headers, dispatcher: agent }, | ||||||||||||
| timeout | ||||||||||||
| ); | ||||||||||||
| return response; | ||||||||||||
| }; | ||||||||||||
|
|
||||||||||||
|
|
||||||||||||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
🛠️ Code Refactor
Issue: The URL construction doesn't validate if the customHost is a valid URL format.
Fix: Add a simple validation to ensure the customHost is properly formatted.
Impact: Prevents runtime errors from malformed URLs.