From 85215b0fb2962029862950de1ac8b99647812d20 Mon Sep 17 00:00:00 2001 From: Daniel Riccio Date: Thu, 17 Jul 2025 13:38:34 -0500 Subject: [PATCH] fix: address PR review comments - Fix security vulnerability in URL validation by using endsWith instead of includes - Add UI notification for detected API version from Base URL - Add validation for Azure API version format - Improve test coverage with edge cases - Exclude Azure AI Inference Service URLs from Azure OpenAI detection --- src/utils/__tests__/azure-url-parser.test.ts | 172 ++++++++++++------ src/utils/azure-url-parser.ts | 43 ++++- .../settings/providers/OpenAICompatible.tsx | 7 + 3 files changed, 162 insertions(+), 60 deletions(-) diff --git a/src/utils/__tests__/azure-url-parser.test.ts b/src/utils/__tests__/azure-url-parser.test.ts index 28ad8e1820..44151c53b3 100644 --- a/src/utils/__tests__/azure-url-parser.test.ts +++ b/src/utils/__tests__/azure-url-parser.test.ts @@ -1,124 +1,190 @@ -import { describe, it, expect } from 'vitest' -import { extractApiVersionFromUrl, isAzureOpenAiUrl, removeApiVersionFromUrl } from '../azure-url-parser' +import { describe, it, expect } from "vitest" +import { + extractApiVersionFromUrl, + isAzureOpenAiUrl, + removeApiVersionFromUrl, + isValidAzureApiVersion, +} from "../azure-url-parser" -describe('azure-url-parser', () => { - describe('extractApiVersionFromUrl', () => { - it('should extract API version from Azure OpenAI URL', () => { - const url = 'https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?api-version=2024-05-01-preview' - const result = extractApiVersionFromUrl(url) - expect(result).toBe('2024-05-01-preview') +describe("azure-url-parser", () => { + describe("isValidAzureApiVersion", () => { + it("should return true for valid API version format YYYY-MM-DD", () => { + expect(isValidAzureApiVersion("2024-05-01")).toBe(true) + expect(isValidAzureApiVersion("2023-12-31")).toBe(true) }) - it('should extract API version from URL with multiple query parameters', () => { - const url = 'https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?foo=bar&api-version=2024-12-01-preview&baz=qux' - const result = extractApiVersionFromUrl(url) - expect(result).toBe('2024-12-01-preview') + it("should return true for valid API version format YYYY-MM-DD-preview", () => { + expect(isValidAzureApiVersion("2024-05-01-preview")).toBe(true) + expect(isValidAzureApiVersion("2024-12-01-preview")).toBe(true) }) - it('should return null when no api-version parameter exists', () => { - const url = 'https://api.openai.com/v1/chat/completions' + it("should return false for invalid API version formats", () => { + expect(isValidAzureApiVersion("2024-5-1")).toBe(false) // Missing leading zeros + expect(isValidAzureApiVersion("24-05-01")).toBe(false) // Two-digit year + expect(isValidAzureApiVersion("2024/05/01")).toBe(false) // Wrong separator + expect(isValidAzureApiVersion("2024-05-01-alpha")).toBe(false) // Wrong suffix + expect(isValidAzureApiVersion("invalid-version")).toBe(false) + expect(isValidAzureApiVersion("")).toBe(false) + }) + }) + + describe("extractApiVersionFromUrl", () => { + it("should extract API version from Azure OpenAI URL", () => { + const url = + "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?api-version=2024-05-01-preview" + const result = extractApiVersionFromUrl(url) + expect(result).toBe("2024-05-01-preview") + }) + + it("should extract API version from URL with multiple query parameters", () => { + const url = + "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?foo=bar&api-version=2024-12-01-preview&baz=qux" + const result = extractApiVersionFromUrl(url) + expect(result).toBe("2024-12-01-preview") + }) + + it("should return null when no api-version parameter exists", () => { + const url = "https://api.openai.com/v1/chat/completions" const result = extractApiVersionFromUrl(url) expect(result).toBeNull() }) - it('should return null for invalid URLs', () => { - const invalidUrl = 'not-a-valid-url' + it("should return null for invalid URLs", () => { + const invalidUrl = "not-a-valid-url" const result = extractApiVersionFromUrl(invalidUrl) expect(result).toBeNull() }) - it('should handle empty api-version parameter', () => { - const url = 'https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?api-version=' + it("should handle empty api-version parameter", () => { + const url = "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?api-version=" const result = extractApiVersionFromUrl(url) - expect(result).toBe('') + expect(result).toBe("") }) - it('should handle URL without query parameters', () => { - const url = 'https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions' + it("should handle URL without query parameters", () => { + const url = "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions" const result = extractApiVersionFromUrl(url) expect(result).toBeNull() }) + + it("should handle URL with duplicate api-version parameters", () => { + const url = + "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?api-version=2024-05-01&api-version=2024-12-01" + const result = extractApiVersionFromUrl(url) + // URL.searchParams.get returns the first value + expect(result).toBe("2024-05-01") + }) + + it("should handle URL with malformed api-version parameter", () => { + const url = + "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?api-version=invalid-format" + const result = extractApiVersionFromUrl(url) + expect(result).toBe("invalid-format") // Still extracts it, validation is separate + }) }) - describe('isAzureOpenAiUrl', () => { - it('should return true for Azure OpenAI URLs with .openai.azure.com', () => { - const url = 'https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions' + describe("isAzureOpenAiUrl", () => { + it("should return true for Azure OpenAI URLs with .openai.azure.com", () => { + const url = "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions" const result = isAzureOpenAiUrl(url) expect(result).toBe(true) }) - it('should return true for Azure URLs ending with .azure.com', () => { - const url = 'https://myservice.azure.com/api/v1' + it("should return true for Azure URLs ending with .azure.com", () => { + const url = "https://myservice.azure.com/api/v1" const result = isAzureOpenAiUrl(url) expect(result).toBe(true) }) - it('should return true for URLs with /openai/deployments/ path', () => { - const url = 'https://custom-domain.com/openai/deployments/mymodel/chat/completions' + it("should return true for URLs with /openai/deployments/ path", () => { + const url = "https://custom-domain.com/openai/deployments/mymodel/chat/completions" const result = isAzureOpenAiUrl(url) expect(result).toBe(true) }) - it('should return false for regular OpenAI URLs', () => { - const url = 'https://api.openai.com/v1/chat/completions' + it("should return false for regular OpenAI URLs", () => { + const url = "https://api.openai.com/v1/chat/completions" const result = isAzureOpenAiUrl(url) expect(result).toBe(false) }) - it('should return false for other API URLs', () => { - const url = 'https://api.anthropic.com/v1/messages' + it("should return false for other API URLs", () => { + const url = "https://api.anthropic.com/v1/messages" const result = isAzureOpenAiUrl(url) expect(result).toBe(false) }) - it('should return false for invalid URLs', () => { - const invalidUrl = 'not-a-valid-url' + it("should return false for invalid URLs", () => { + const invalidUrl = "not-a-valid-url" const result = isAzureOpenAiUrl(invalidUrl) expect(result).toBe(false) }) - it('should handle case insensitive hostname matching', () => { - const url = 'https://MYRESOURCE.OPENAI.AZURE.COM/openai/deployments/mymodel' + it("should handle case insensitive hostname matching", () => { + const url = "https://MYRESOURCE.OPENAI.AZURE.COM/openai/deployments/mymodel" const result = isAzureOpenAiUrl(url) expect(result).toBe(true) }) + + it("should return false for malicious URLs trying to include Azure domain", () => { + const maliciousUrl = "https://evil.openai.azure.com.attacker.com/api/v1" + const result = isAzureOpenAiUrl(maliciousUrl) + expect(result).toBe(false) + }) + + it("should return true for root openai.azure.com domain", () => { + const url = "https://openai.azure.com/api/v1" + const result = isAzureOpenAiUrl(url) + expect(result).toBe(true) + }) + + it("should return false for Azure AI Inference Service URLs", () => { + const url = "https://myservice.services.ai.azure.com/models/deployments" + const result = isAzureOpenAiUrl(url) + expect(result).toBe(false) + }) }) - describe('removeApiVersionFromUrl', () => { - it('should remove api-version parameter from URL', () => { - const url = 'https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?api-version=2024-05-01-preview' + describe("removeApiVersionFromUrl", () => { + it("should remove api-version parameter from URL", () => { + const url = + "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?api-version=2024-05-01-preview" const result = removeApiVersionFromUrl(url) - expect(result).toBe('https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions') + expect(result).toBe("https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions") }) - it('should remove api-version parameter while preserving other parameters', () => { - const url = 'https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?foo=bar&api-version=2024-05-01-preview&baz=qux' + it("should remove api-version parameter while preserving other parameters", () => { + const url = + "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?foo=bar&api-version=2024-05-01-preview&baz=qux" const result = removeApiVersionFromUrl(url) - expect(result).toBe('https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?foo=bar&baz=qux') + expect(result).toBe( + "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?foo=bar&baz=qux", + ) }) - it('should return original URL when no api-version parameter exists', () => { - const url = 'https://api.openai.com/v1/chat/completions?foo=bar' + it("should return original URL when no api-version parameter exists", () => { + const url = "https://api.openai.com/v1/chat/completions?foo=bar" const result = removeApiVersionFromUrl(url) expect(result).toBe(url) }) - it('should return original URL for invalid URLs', () => { - const invalidUrl = 'not-a-valid-url' + it("should return original URL for invalid URLs", () => { + const invalidUrl = "not-a-valid-url" const result = removeApiVersionFromUrl(invalidUrl) expect(result).toBe(invalidUrl) }) - it('should handle URL with only api-version parameter', () => { - const url = 'https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?api-version=2024-05-01-preview' + it("should handle URL with only api-version parameter", () => { + const url = + "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions?api-version=2024-05-01-preview" const result = removeApiVersionFromUrl(url) - expect(result).toBe('https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions') + expect(result).toBe("https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions") }) - it('should handle URL without query parameters', () => { - const url = 'https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions' + it("should handle URL without query parameters", () => { + const url = "https://myresource.openai.azure.com/openai/deployments/mymodel/chat/completions" const result = removeApiVersionFromUrl(url) expect(result).toBe(url) }) }) -}) \ No newline at end of file +}) diff --git a/src/utils/azure-url-parser.ts b/src/utils/azure-url-parser.ts index 1188932476..674e11dd14 100644 --- a/src/utils/azure-url-parser.ts +++ b/src/utils/azure-url-parser.ts @@ -2,6 +2,19 @@ * Utility functions for parsing Azure OpenAI URLs and extracting API versions */ +/** + * Validates if a string is a valid Azure API version format + * @param version The version string to validate + * @returns True if the version follows Azure API version format (YYYY-MM-DD or YYYY-MM-DD-preview) + */ +export function isValidAzureApiVersion(version: string): boolean { + if (!version) return false + + // Azure API versions follow the pattern: YYYY-MM-DD or YYYY-MM-DD-preview + const versionPattern = /^\d{4}-\d{2}-\d{2}(-preview)?$/ + return versionPattern.test(version) +} + /** * Extracts the API version from an Azure OpenAI URL query parameter * @param url The Azure OpenAI URL that may contain an api-version query parameter @@ -10,7 +23,14 @@ export function extractApiVersionFromUrl(url: string): string | null { try { const urlObj = new URL(url) - return urlObj.searchParams.get('api-version') + const apiVersion = urlObj.searchParams.get("api-version") + + // Validate the extracted version format + if (apiVersion && !isValidAzureApiVersion(apiVersion)) { + console.warn(`Invalid Azure API version format: ${apiVersion}`) + } + + return apiVersion } catch (error) { // Invalid URL format return null @@ -26,11 +46,20 @@ export function isAzureOpenAiUrl(url: string): boolean { try { const urlObj = new URL(url) const host = urlObj.host.toLowerCase() - + + // Exclude Azure AI Inference Service URLs + if (host.endsWith(".services.ai.azure.com")) { + return false + } + // Check for Azure OpenAI hostname patterns - return host.includes('.openai.azure.com') || - host.endsWith('.azure.com') || - urlObj.pathname.includes('/openai/deployments/') + // Use endsWith to prevent matching malicious URLs like evil.openai.azure.com.attacker.com + return ( + host.endsWith(".openai.azure.com") || + host === "openai.azure.com" || + host.endsWith(".azure.com") || + urlObj.pathname.includes("/openai/deployments/") + ) } catch (error) { return false } @@ -44,10 +73,10 @@ export function isAzureOpenAiUrl(url: string): boolean { export function removeApiVersionFromUrl(url: string): string { try { const urlObj = new URL(url) - urlObj.searchParams.delete('api-version') + urlObj.searchParams.delete("api-version") return urlObj.toString() } catch (error) { // Return original URL if parsing fails return url } -} \ No newline at end of file +} diff --git a/webview-ui/src/components/settings/providers/OpenAICompatible.tsx b/webview-ui/src/components/settings/providers/OpenAICompatible.tsx index bbf567183e..65f405dd47 100644 --- a/webview-ui/src/components/settings/providers/OpenAICompatible.tsx +++ b/webview-ui/src/components/settings/providers/OpenAICompatible.tsx @@ -202,6 +202,13 @@ export const OpenAICompatible = ({ }}> {t("settings:modelInfo.azureApiVersion")} + {showApiVersionExtraction && ( +
+ API version detected in Base URL: {extractedApiVersion} +
+ This will be used automatically. Enable the checkbox above to override. +
+ )} {azureApiVersionSelected && (