From a30c866724760c6b325e5c324ce6dad80ade688c Mon Sep 17 00:00:00 2001 From: Cristian Magherusan-Stanciu Date: Mon, 19 Oct 2026 23:23:54 +0200 Subject: [PATCH] fix(azure): validate remaining Retail Prices filter values with ODataString Route the SKU and region values in the search, compute, cosmosdb and savingsplans Retail Prices filters through pricing.ODataString so an injected value is rejected before any request is built. The search display SKU comes from a fixed map and keeps its literal. Closes #526 Co-Authored-By: Claude Opus 5.5 --- CHANGELOG.md | 4 ++++ providers/azure/services/compute/client.go | 12 ++++++++-- .../azure/services/compute/client_test.go | 22 +++++++++++++++---- providers/azure/services/cosmosdb/client.go | 8 +++++-- .../azure/services/cosmosdb/client_test.go | 19 ++++++++++++++++ .../cosmosdb/retail_selection_test.go | 4 ++-- .../azure/services/savingsplans/client.go | 8 +++++-- .../services/savingsplans/client_test.go | 7 ++++++ providers/azure/services/search/client.go | 6 ++++- .../azure/services/search/client_test.go | 15 +++++++++---- 10 files changed, 88 insertions(+), 17 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 0c0fd273..9f8deafb 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -176,6 +176,10 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/). ### Fixed +- **`azure`:** the Retail Prices `$filter` values built from SKU and region in + the search, compute, cosmosdb and savingsplans clients are now validated with + `pricing.ODataString`, so an injected value is rejected before any request is + built (#526). - **`gcp/computeengine`:** a Recommender cost projection with a non-zero cost and a non-USD or missing currency code is now skipped with a logged warning instead of being reported as USD `EstimatedSavings`. diff --git a/providers/azure/services/compute/client.go b/providers/azure/services/compute/client.go index 6f4e820d..5ef34fa0 100644 --- a/providers/azure/services/compute/client.go +++ b/providers/azure/services/compute/client.go @@ -696,8 +696,16 @@ type VMPricing struct { // getVMPricing gets real VM pricing from Azure Retail Prices API. func (c *Client) getVMPricing(ctx context.Context, vmSize, region string, termYears int) (*VMPricing, error) { - filter := fmt.Sprintf("serviceName eq 'Virtual Machines' and armRegionName eq '%s' and armSkuName eq '%s' and priceType eq 'Reservation'", - strings.ReplaceAll(region, "'", "''"), strings.ReplaceAll(vmSize, "'", "''")) + qRegion, err := pricing.ODataString(region) + if err != nil { + return nil, err + } + qSKU, err := pricing.ODataString(vmSize) + if err != nil { + return nil, err + } + filter := fmt.Sprintf("serviceName eq 'Virtual Machines' and armRegionName eq %s and armSkuName eq %s and priceType eq 'Reservation'", + qRegion, qSKU) priceData, err := c.fetchAzurePricing(ctx, filter) if err != nil { diff --git a/providers/azure/services/compute/client_test.go b/providers/azure/services/compute/client_test.go index a4aeb37c..e13d196e 100644 --- a/providers/azure/services/compute/client_test.go +++ b/providers/azure/services/compute/client_test.go @@ -1735,16 +1735,30 @@ func TestCheckAndRegisterCapacityProvider_NonTwoxx(t *testing.T) { assert.Contains(t, err.Error(), "403", "error must surface the HTTP status code") } -func TestGetVMPricing_EscapesOData(t *testing.T) { +func TestGetVMPricing_RejectsInjectedFilterValues(t *testing.T) { + for _, tc := range []struct{ name, vmSize, region string }{ + {"vmSize", "x' or armSkuName ne '", "eastus"}, + {"region", "Standard_D2s_v3", "eastus' or '1' eq '1"}, + } { + t.Run(tc.name, func(t *testing.T) { + client := NewClientWithHTTP(nil, "test-subscription", "eastus", &mocks.MockHTTPClient{}) + _, err := client.getVMPricing(context.Background(), tc.vmSize, tc.region, 1) + require.Error(t, err) + assert.Contains(t, err.Error(), "Retail Prices filter") + }) + } +} + +func TestGetVMPricing_LegitimateValuesBuildExactFilter(t *testing.T) { mockHTTP := &mocks.MockHTTPClient{} - client := NewClientWithHTTP(nil, "test-subscription", "east'us", mockHTTP) + client := NewClientWithHTTP(nil, "test-subscription", "eastus", mockHTTP) response := mocks.CreateMockHTTPResponse(http.StatusOK, `{"Items": []}`) t.Cleanup(func() { require.NoError(t, response.Body.Close()) }) mockHTTP.On("Do", mock.Anything).Run(func(args mock.Arguments) { req := args.Get(0).(*http.Request) - assert.Equal(t, "serviceName eq 'Virtual Machines' and armRegionName eq 'east''us' and armSkuName eq 'Standard_D2''s_v3' and priceType eq 'Reservation'", req.URL.Query().Get("$filter")) + assert.Equal(t, "serviceName eq 'Virtual Machines' and armRegionName eq 'eastus' and armSkuName eq 'Standard_D2s_v3' and priceType eq 'Reservation'", req.URL.Query().Get("$filter")) }).Return(response, nil).Once() - _, err := client.getVMPricing(context.Background(), "Standard_D2's_v3", "east'us", 1) + _, err := client.getVMPricing(context.Background(), "Standard_D2s_v3", "eastus", 1) require.Error(t, err) mockHTTP.AssertExpectations(t) } diff --git a/providers/azure/services/cosmosdb/client.go b/providers/azure/services/cosmosdb/client.go index aa9df5e3..18de7a9b 100644 --- a/providers/azure/services/cosmosdb/client.go +++ b/providers/azure/services/cosmosdb/client.go @@ -574,8 +574,12 @@ func (c *Client) getCosmosPricing(ctx context.Context, sku, region string, termY return nil, fmt.Errorf("unsupported Cosmos DB pricing SKU %q", sku) } } - filter := fmt.Sprintf("serviceName eq 'Azure Cosmos DB' and productName eq 'Azure Cosmos DB' and armRegionName eq 'Global' and armSkuName eq '%s' and priceType eq 'Reservation'", - strings.ReplaceAll(sku, "'", "''")) + qSKU, err := pricing.ODataString(sku) + if err != nil { + return nil, err + } + filter := fmt.Sprintf("serviceName eq 'Azure Cosmos DB' and productName eq 'Azure Cosmos DB' and armRegionName eq 'Global' and armSkuName eq %s and priceType eq 'Reservation'", + qSKU) priceData, err := c.fetchAzurePricing(ctx, filter) if err != nil { diff --git a/providers/azure/services/cosmosdb/client_test.go b/providers/azure/services/cosmosdb/client_test.go index c878a525..afc5475e 100644 --- a/providers/azure/services/cosmosdb/client_test.go +++ b/providers/azure/services/cosmosdb/client_test.go @@ -1722,3 +1722,22 @@ func TestNewClientWithHTTP_NilFallbackIsHardened(t *testing.T) { require.Error(t, err, "nil-fallback client must reject IMDS connections") assert.Contains(t, err.Error(), "blocked") } + +func TestGetCosmosPricing_RejectsInjectedSKU(t *testing.T) { + client := NewClientWithHTTP(nil, "sub", "eastus", &MockHTTPClient{}) + _, err := client.getCosmosPricing(context.Background(), "Cosmos_DB_x' or armSkuName ne '", "eastus", 1) + require.Error(t, err) + assert.Contains(t, err.Error(), "Retail Prices filter") +} + +func TestGetCosmosPricing_LegitimateSKUBuildsExactFilter(t *testing.T) { + response := createMockHTTPResponse(http.StatusOK, `{"Items":[]}`) + t.Cleanup(func() { require.NoError(t, response.Body.Close()) }) + mockHTTP := &MockHTTPClient{} + mockHTTP.On("Do", mock.Anything).Return(response, nil).Once() + _, err := NewClientWithHTTP(nil, "sub", "eastus", mockHTTP).getCosmosPricing(context.Background(), "100RU", "eastus", 1) + require.Error(t, err) + req := mockHTTP.Calls[0].Arguments.Get(0).(*http.Request) + assert.Equal(t, "serviceName eq 'Azure Cosmos DB' and productName eq 'Azure Cosmos DB' and armRegionName eq 'Global' and armSkuName eq 'Cosmos_DB_100_RUs' and priceType eq 'Reservation'", req.URL.Query().Get("$filter")) + mockHTTP.AssertExpectations(t) +} diff --git a/providers/azure/services/cosmosdb/retail_selection_test.go b/providers/azure/services/cosmosdb/retail_selection_test.go index 22243c89..7f2342f5 100644 --- a/providers/azure/services/cosmosdb/retail_selection_test.go +++ b/providers/azure/services/cosmosdb/retail_selection_test.go @@ -177,7 +177,7 @@ func TestCosmosOffering_CapturedReservations(t *testing.T) { } func TestCosmosOffering_FilterAndEscaping(t *testing.T) { - for _, sku := range []string{"100RU", "100RUperSecond", "Cosmos_DB_100_RUs", "Cosmos_DB_test'RU"} { + for _, sku := range []string{"100RU", "100RUperSecond", "Cosmos_DB_100_RUs"} { t.Run(sku, func(t *testing.T) { arm := "Cosmos_DB_100_RUs" if strings.HasPrefix(sku, "Cosmos_DB_") { @@ -196,7 +196,7 @@ func TestCosmosOffering_FilterAndEscaping(t *testing.T) { } func TestCosmosOffering_UnknownAliasesAvoidHTTP(t *testing.T) { - for _, sku := range []string{"", "1000RU", "1000000RU", "100mRU", "100ru", "100RU'", " 100RU", "CosmosDB_RU_1000", "EnableCassandra"} { + for _, sku := range []string{"", "1000RU", "1000000RU", "100mRU", "100ru", "100RU'", " 100RU", "Cosmos_DB_test'RU", "CosmosDB_RU_1000", "EnableCassandra"} { t.Run(sku, func(t *testing.T) { q, err := cosmosQuote(t, sku, nil) require.Error(t, err) diff --git a/providers/azure/services/savingsplans/client.go b/providers/azure/services/savingsplans/client.go index 2e7f56b2..a24d94b7 100644 --- a/providers/azure/services/savingsplans/client.go +++ b/providers/azure/services/savingsplans/client.go @@ -627,9 +627,13 @@ func (c *Client) GetOfferingDetails(ctx context.Context, rec common.Recommendati // Returns an error when no price record is found rather than (0, nil), which would // conflate "zero rate" with "not found" (feedback_nullable_not_zero). func (c *Client) fetchOnDemandRate(ctx context.Context, planType string) (float64, error) { + qPlan, err := pricing.ODataString(planType) + if err != nil { + return 0, err + } filter := fmt.Sprintf( - "serviceFamily eq 'Compute' and priceType eq 'Consumption' and armSkuName eq '%s'", - planType, + "serviceFamily eq 'Compute' and priceType eq 'Consumption' and armSkuName eq %s", + qPlan, ) params := url.Values{} params.Add("$filter", filter) diff --git a/providers/azure/services/savingsplans/client_test.go b/providers/azure/services/savingsplans/client_test.go index 3663d5ce..424cfd05 100644 --- a/providers/azure/services/savingsplans/client_test.go +++ b/providers/azure/services/savingsplans/client_test.go @@ -811,3 +811,10 @@ func TestCommitmentCurrency_LowercaseNormalised(t *testing.T) { require.NoError(t, c.ValidateOffering(context.Background(), currencyRec("eur"))) assert.Equal(t, "EUR", *rp.captured.Benefits[0].Properties.Commitment.CurrencyCode) } + +func TestFetchOnDemandRate_RejectsInjectedPlanType(t *testing.T) { + c := NewClientWithHTTP(nil, "sub", "eastus", &mockHTTPClient{}) + _, err := c.fetchOnDemandRate(context.Background(), "x' or armSkuName ne '") + require.Error(t, err) + assert.Contains(t, err.Error(), "Retail Prices filter") +} diff --git a/providers/azure/services/search/client.go b/providers/azure/services/search/client.go index bb1ded54..1b426a00 100644 --- a/providers/azure/services/search/client.go +++ b/providers/azure/services/search/client.go @@ -470,7 +470,11 @@ func (c *Client) getSearchPricing(ctx context.Context, sku, region string, termY if !ok { return nil, fmt.Errorf("unsupported Azure Search SKU %q", sku) } - filter := fmt.Sprintf("serviceName eq 'Azure Cognitive Search' and armRegionName eq '%s' and skuName eq '%s' and priceType eq 'Reservation'", strings.ReplaceAll(region, "'", "''"), displaySKU) + qRegion, err := pricing.ODataString(region) + if err != nil { + return nil, err + } + filter := fmt.Sprintf("serviceName eq 'Azure Cognitive Search' and armRegionName eq %s and skuName eq '%s' and priceType eq 'Reservation'", qRegion, displaySKU) priceData, err := c.fetchAzurePricing(ctx, filter) if err != nil { diff --git a/providers/azure/services/search/client_test.go b/providers/azure/services/search/client_test.go index 677284b6..197267ae 100644 --- a/providers/azure/services/search/client_test.go +++ b/providers/azure/services/search/client_test.go @@ -580,9 +580,16 @@ func TestSearchClient_GetOfferingDetails_Pagination(t *testing.T) { } } -func TestSearchClient_GetOfferingDetails_EscapesRegionAndRejectsUnknownTier(t *testing.T) { - const region = "east'us" - response := createMockHTTPResponse(http.StatusOK, `{"Items":[{"serviceName":"Azure Cognitive Search","armRegionName":"east'us","skuName":"Standard S1","type":"Reservation","reservationTerm":"1 Year","unitOfMeasure":"1 Hour","retailPrice":800,"currencyCode":"USD"}]}`) +func TestGetSearchPricing_RejectsInjectedRegion(t *testing.T) { + client := NewClientWithHTTP(nil, "sub", "eastus", &MockHTTPClient{}) + _, err := client.getSearchPricing(context.Background(), "standard", "eastus' or '1' eq '1", 1) + require.Error(t, err) + assert.Contains(t, err.Error(), "Retail Prices filter") +} + +func TestSearchClient_GetOfferingDetails_BuildsExactFilterAndRejectsUnknownTier(t *testing.T) { + const region = "eastus" + response := createMockHTTPResponse(http.StatusOK, `{"Items":[{"serviceName":"Azure Cognitive Search","armRegionName":"eastus","skuName":"Standard S1","type":"Reservation","reservationTerm":"1 Year","unitOfMeasure":"1 Hour","retailPrice":800,"currencyCode":"USD"}]}`) t.Cleanup(func() { require.NoError(t, response.Body.Close()) }) mockHTTP := &MockHTTPClient{} mockHTTP.On("Do", mock.Anything).Return(response, nil).Once() @@ -592,7 +599,7 @@ func TestSearchClient_GetOfferingDetails_EscapesRegionAndRejectsUnknownTier(t *t require.NoError(t, err) assert.Equal(t, 800.0, details.TotalCost) req := mockHTTP.Calls[0].Arguments.Get(0).(*http.Request) - assert.Equal(t, "serviceName eq 'Azure Cognitive Search' and armRegionName eq 'east''us' and skuName eq 'Standard S1' and priceType eq 'Reservation'", req.URL.Query().Get("$filter")) + assert.Equal(t, "serviceName eq 'Azure Cognitive Search' and armRegionName eq 'eastus' and skuName eq 'Standard S1' and priceType eq 'Reservation'", req.URL.Query().Get("$filter")) assert.Contains(t, req.URL.RawQuery, "%24filter=") mockHTTP.AssertExpectations(t)