fix(azure): refuse stale-credential accounts and validate OData filter input

providers/azure/accounts_cache.go

@@ -97,6 +97,11 @@ func accountsSFKey(gen uint64) string {
 	return accountsCacheSFKeyPrefix + strconv.FormatUint(gen, 10)
 }
 
+// errAccountsStale signals that the credential, subscriptions client or cache
+// generation changed while a fetch was in flight (a SetCredential contract
+// violation), so the fetched list belongs to the previous principal.
+var errAccountsStale = errors.New("azure accounts cache: credential or subscriptions client changed during fetch")
+
 // fetchAccountsShared collapses concurrent cold-cache callers onto a single
 // in-flight ARM subscriptions.List and returns the SHARED (un-cloned) slice.
 // Callers must clone before handing the result outside the package.
@@ -122,14 +127,19 @@ func (p *Provider) fetchAccountsShared(ctx context.Context) ([]common.Account, e
 		}
 
 		p.accountsMu.Lock()
-		// Publish only when no InvalidateAccountsCache landed while the ARM
-		// call was in flight. A fetch that started before an invalidation
-		// carries a pre-invalidation snapshot; writing it now would silently
-		// resurrect exactly the data the caller asked to discard.
-		if p.accountsGen == gen {
-			p.cachedAccounts = accounts
+		defer p.accountsMu.Unlock()
+		// A generation bump while the ARM call was in flight (SetCredential,
+		// SetSubscriptionsClient or InvalidateAccountsCache) makes this list a
+		// snapshot of what the PREVIOUS principal could see. Publishing it would
+		// resurrect discarded data, and handing it to the callers that joined
+		// the singleflight would let GetRecommendationsClient fan out over the
+		// old credential's subscriptions using the new credential. Fail loud
+		// instead of re-fetching: a swap mid-fetch is a caller contract
+		// violation, and one retry loop here would hide it.
+		if p.accountsGen != gen {
+			return nil, errAccountsStale
 		}
-		p.accountsMu.Unlock()
+		p.cachedAccounts = accounts
 
 		return accounts, nil
 	})

providers/azure/internal/pricing/odata.go

@@ -0,0 +1,22 @@
+package pricing
+
+import (
+	"fmt"
+	"regexp"
+	"strings"
+)
+
+// odataTokenRE is the character set of every Azure SKU and region name the
+// Retail Prices filters are built from. Anything else is rejected rather than
+// escaped, so a value can never change the shape of the $filter.
+var odataTokenRE = regexp.MustCompile(`^[A-Za-z0-9_.-]+$`)
+
+// ODataString returns v as a single-quoted OData string literal for a Retail
+// Prices $filter. It rejects values outside [A-Za-z0-9_.-]; the quote doubling
+// is defense in depth should the allowed set ever grow.
+func ODataString(v string) (string, error) {
+	if !odataTokenRE.MatchString(v) {
+		return "", fmt.Errorf("invalid value %q for Retail Prices filter: must match %s", v, odataTokenRE)
+	}
+	return "'" + strings.ReplaceAll(v, "'", "''") + "'", nil
+}

providers/azure/internal/pricing/odata_test.go

@@ -0,0 +1,32 @@
+package pricing
+
+import "testing"
+
+func TestODataString(t *testing.T) {
+	tests := []struct {
+		name    string
+		in      string
+		want    string
+		wantErr bool
+	}{
+		{"region", "eastus", "'eastus'", false},
+		{"valid sku", "GP_Gen5_2", "'GP_Gen5_2'", false},
+		{"dots and dashes", "Azure_Redis_Cache-E10.1", "'Azure_Redis_Cache-E10.1'", false},
+		{"single quote", "a'b", "", true},
+		{"injection attempt", "x' or '1' eq '1", "", true},
+		{"space", "East US", "", true},
+		{"empty", "", "", true},
+		{"newline", "a\nb", "", true},
+	}
+	for _, tt := range tests {
+		t.Run(tt.name, func(t *testing.T) {
+			got, err := ODataString(tt.in)
+			if (err != nil) != tt.wantErr {
+				t.Fatalf("err = %v, wantErr %v", err, tt.wantErr)
+			}
+			if got != tt.want {
+				t.Fatalf("got %q, want %q", got, tt.want)
+			}
+		})
+	}
+}

providers/azure/provider_test.go

@@ -1559,11 +1559,13 @@ func TestAzureProvider_ConcurrentCredentialSwapAndFetch_NoDataRace(t *testing.T)
 				// Both readers of the swapped fields: GetAccounts reaches
 				// fetchAccounts, GetServiceClientForAccount builds a client
 				// straight from the credential.
-				if _, err := p.GetAccounts(context.Background()); err != nil {
+				// The swappers invalidate constantly, so a fetch failing as
+				// stale is the expected outcome here, not a defect.
+				if _, err := p.GetAccounts(context.Background()); err != nil && !errors.Is(err, errAccountsStale) {
 					errs <- err
 					return
 				}
-				if _, err := p.GetServiceClientForAccount(context.Background(), common.ServiceCompute, "eastus", "sub-1"); err != nil {
+				if _, err := p.GetServiceClientForAccount(context.Background(), common.ServiceCompute, "eastus", "sub-1"); err != nil && !errors.Is(err, errAccountsStale) {
 					errs <- err
 					return
 				}
@@ -1702,9 +1704,9 @@ func TestAzureProvider_InvalidateAccountsCache_DuringInFlightFetch(t *testing.T)
 	close(gated.release)
 
 	first := <-done
-	require.NoError(t, first.err)
-	require.Len(t, first.accounts, 1)
-	assert.Equal(t, "sub-gen-1", first.accounts[0].ID)
+	require.ErrorIs(t, first.err, errAccountsStale,
+		"a fetch invalidated mid-flight must fail, not hand back the stale snapshot")
+	assert.Empty(t, first.accounts)
 
 	// The invalidated snapshot must not have been published: this read has to
 	// re-hit ARM and observe the current subscription list.
@@ -1717,6 +1719,39 @@ func TestAzureProvider_InvalidateAccountsCache_DuringInFlightFetch(t *testing.T)
 		"read after invalidation must see fresh data, not the resurrected pre-invalidation snapshot")
 }
 
+// A credential swap landing during an in-flight fetch must not hand the old
+// credential's subscription list to the caller that joined the singleflight
+// (GetRecommendationsClient would fan out over it using the new credential),
+// and must not populate the cache.
+func TestAzureProvider_SetCredential_DuringInFlightFetch_ReturnsError(t *testing.T) {
+	clearAzureSubscriptionEnv(t)
+	gated := &gatedGenerationSubscriptionsClient{
+		firstInFlight: make(chan struct{}),
+		release:       make(chan struct{}),
+	}
+	p := &Provider{cred: &mockTokenCredential{}}
+	p.SetSubscriptionsClient(gated)
+
+	type fetchResult struct {
+		accounts []common.Account
+		err      error
+	}
+	done := make(chan fetchResult, 1)
+	go func() {
+		accts, err := p.getOrFetchAccounts(context.Background())
+		done <- fetchResult{accounts: accts, err: err}
+	}()
+
+	<-gated.firstInFlight
+	p.SetCredential(&mockTokenCredential{})
+	close(gated.release)
+
+	res := <-done
+	require.ErrorIs(t, res.err, errAccountsStale)
+	assert.Nil(t, res.accounts)
+	assert.Nil(t, p.readCachedAccounts(), "stale fetch must not write the cache")
+}
+
 func TestAzureProvider_GetRecommendationsClient_MultiSubscriptionFanOut(t *testing.T) {
 	clearAzureSubscriptionEnv(t)
 

providers/azure/services/cache/client.go

@@ -572,8 +572,16 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe
 	if err != nil {
 		return nil, err
 	}
-	filter := fmt.Sprintf("serviceName eq 'Redis Cache' and armRegionName eq '%s' and armSkuName eq '%s' and priceType eq 'Reservation'",
-		strings.ReplaceAll(region, "'", "''"), identity.ArmSKUName)
+	qRegion, err := pricing.ODataString(region)
+	if err != nil {
+		return nil, err
+	}
+	qSKU, err := pricing.ODataString(identity.ArmSKUName)
+	if err != nil {
+		return nil, err
+	}
+	filter := fmt.Sprintf("serviceName eq 'Redis Cache' and armRegionName eq %s and armSkuName eq %s and priceType eq 'Reservation'",
+		qRegion, qSKU)
 
 	params := url.Values{}
 	params.Add("$filter", filter)

providers/azure/services/cache/retail_selection_test.go

@@ -191,16 +191,15 @@ func TestRedisOffering_CapturedReservations(t *testing.T) {
 	}
 }
 
-func TestRedisOffering_FilterAndEscaping(t *testing.T) {
+func TestRedisOffering_RejectsQuoteInRegion(t *testing.T) {
 	for _, c := range redisClients() {
 		t.Run(c.name, func(t *testing.T) {
-			r := redisCatalog()[0].row(1)
-			r["armRegionName"] = "east'us"
-			filter := []string{"serviceName eq 'Redis Cache'", "armRegionName eq 'east''us'", "armSkuName eq 'Azure_Redis_Cache_Premium_P1_Cache'", "priceType eq 'Reservation'"}
-			h, _ := redisHTTP(t, [][]map[string]any{{redisConsumption(), r}}, filter)
+			h, requests := redisHTTP(t, nil, nil)
 			q, err := c.newClient("east'us", h).GetOfferingDetails(context.Background(), common.Recommendation{ResourceType: "Premium_P1", Term: "1yr", PaymentOption: "upfront"})
-			require.NoError(t, err)
-			assert.Equal(t, 1553.0, q.TotalCost)
+			require.Error(t, err)
+			assert.Nil(t, q)
+			assert.Contains(t, err.Error(), "Retail Prices filter")
+			assert.Equal(t, int32(0), requests.Load())
 		})
 	}
 }

providers/azure/services/database/client.go

@@ -587,8 +587,16 @@ type SQLPricing struct {
 
 // getSQLPricing gets real pricing from Azure Retail Prices API.
 func (c *Client) getSQLPricing(ctx context.Context, sku, region string, termYears int) (*SQLPricing, error) {
-	filter := fmt.Sprintf("serviceName eq 'SQL Database' and armRegionName eq '%s' and armSkuName eq '%s' and priceType eq 'Reservation'",
-		strings.ReplaceAll(region, "'", "''"), strings.ReplaceAll(sku, "'", "''"))
+	qRegion, err := pricing.ODataString(region)
+	if err != nil {
+		return nil, err
+	}
+	qSKU, err := pricing.ODataString(sku)
+	if err != nil {
+		return nil, err
+	}
+	filter := fmt.Sprintf("serviceName eq 'SQL Database' and armRegionName eq %s and armSkuName eq %s and priceType eq 'Reservation'",
+		qRegion, qSKU)
 
 	priceData, err := c.fetchAzurePricing(ctx, filter)
 	if err != nil {

providers/azure/services/database/client_test.go

@@ -558,37 +558,16 @@ func TestDatabaseClient_GetOfferingDetails_SelectsSyntheticVCoreRow(t *testing.T
 	}
 }
 
-func TestDatabaseClient_GetOfferingDetails_ExactEscapedReservationFilter(t *testing.T) {
-	const region, sku = "east'us", "GeneralPurpose_Gen5_'2"
-	row := azpricing.RetailPriceItem{
-		ServiceName: "SQL Database", ArmRegionName: region, ArmSKUName: sku,
-		Type: "Reservation", ReservationTerm: "1 Year", UnitOfMeasure: "1 Hour",
-		CurrencyCode: "EUR", RetailPrice: 1200,
-	}
-	body, err := json.Marshal(AzureRetailPrice{Items: []azpricing.RetailPriceItem{row}})
-	require.NoError(t, err)
-	mockHTTP := &MockHTTPClient{}
-	response := createMockHTTPResponse(http.StatusOK, string(body))
-	t.Cleanup(func() {
-		require.NoError(t, response.Body.Close())
+func TestDatabaseClient_GetOfferingDetails_RejectsQuoteInRegionAndSKU(t *testing.T) {
+	// Quote characters are rejected outright (not escaped); the mock has no
+	// expectations, so any request would panic.
+	client := NewClientWithHTTP(nil, "sub", "east'us", &MockHTTPClient{})
+	details, err := client.GetOfferingDetails(context.Background(), common.Recommendation{
+		ResourceType: "GeneralPurpose_Gen5_'2", Term: "1yr", PaymentOption: "upfront",
 	})
-	mockHTTP.On("Do", mock.Anything).Return(response, nil).Run(func(args mock.Arguments) {
-		req := args.Get(0).(*http.Request)
-		assert.Equal(t, http.MethodGet, req.Method)
-		assert.Equal(t, "https", req.URL.Scheme)
-		assert.Equal(t, "prices.azure.com", req.URL.Host)
-		assert.Equal(t, "/api/retail/prices", req.URL.Path)
-		assert.Equal(t, "2023-01-01-preview", req.URL.Query().Get("api-version"))
-		assert.Equal(t, "serviceName eq 'SQL Database' and armRegionName eq 'east''us' and armSkuName eq 'GeneralPurpose_Gen5_''2' and priceType eq 'Reservation'", req.URL.Query().Get("$filter"))
-	}).Once()
-	details, err := NewClientWithHTTP(nil, "sub", region, mockHTTP).GetOfferingDetails(context.Background(), common.Recommendation{
-		ResourceType: sku, Term: "1yr", PaymentOption: "upfront",
-	})
-	require.NoError(t, err)
-	require.NotNil(t, details)
-	assert.Equal(t, 1200.0, details.TotalCost)
-	assert.Equal(t, "EUR", details.Currency)
-	mockHTTP.AssertExpectations(t)
+	require.Error(t, err)
+	assert.Nil(t, details)
+	assert.Contains(t, err.Error(), "Retail Prices filter")
 }
 
 func TestDatabaseClient_GetOfferingDetails_ReservationAcrossPages(t *testing.T) {
@@ -2119,6 +2098,50 @@ func TestDatabaseClient_ServerInfo_FetchedOnce(t *testing.T) {
 		"servers pager NextPage not called when it has no pages")
 }
 
+// An injected SKU or region must be rejected before any Retail Prices request
+// is built; the mock has no expectations, so any HTTP call would panic.
+func TestGetSQLPricing_RejectsInjectedFilterValues(t *testing.T) {
+	for _, tc := range []struct{ name, sku, region string }{
+		{"sku", "x' or armSkuName ne '", "eastus"},
+		{"region", "GP_Gen5_2", "eastus' or '1' eq '1"},
+	} {
+		t.Run(tc.name, func(t *testing.T) {
+			client := NewClientWithHTTP(nil, "test-subscription", "eastus", &MockHTTPClient{})
+			_, err := client.getSQLPricing(context.Background(), tc.sku, tc.region, 1)
+			require.Error(t, err)
+			assert.Contains(t, err.Error(), "Retail Prices filter")
+		})
+	}
+}
+
+func TestDatabaseClient_GetOfferingDetails_ExactReservationFilter(t *testing.T) {
+	const region, sku = "eastus", "GP_Gen5_2"
+	row := azpricing.RetailPriceItem{
+		ServiceName: "SQL Database", ArmRegionName: region, ArmSKUName: sku,
+		Type: "Reservation", ReservationTerm: "1 Year", UnitOfMeasure: "1 Hour",
+		CurrencyCode: "EUR", RetailPrice: 1200,
+	}
+	body, err := json.Marshal(AzureRetailPrice{Items: []azpricing.RetailPriceItem{row}})
+	require.NoError(t, err)
+	mockHTTP := &MockHTTPClient{}
+	response := createMockHTTPResponse(http.StatusOK, string(body))
+	t.Cleanup(func() {
+		require.NoError(t, response.Body.Close())
+	})
+	mockHTTP.On("Do", mock.Anything).Return(response, nil).Run(func(args mock.Arguments) {
+		req := args.Get(0).(*http.Request)
+		assert.Equal(t, "2023-01-01-preview", req.URL.Query().Get("api-version"))
+		assert.Equal(t, "serviceName eq 'SQL Database' and armRegionName eq 'eastus' and armSkuName eq 'GP_Gen5_2' and priceType eq 'Reservation'", req.URL.Query().Get("$filter"))
+	}).Once()
+	details, err := NewClientWithHTTP(nil, "sub", region, mockHTTP).GetOfferingDetails(context.Background(), common.Recommendation{
+		ResourceType: sku, Term: "1yr", PaymentOption: "upfront",
+	})
+	require.NoError(t, err)
+	require.NotNil(t, details)
+	assert.Equal(t, 1200.0, details.TotalCost)
+	mockHTTP.AssertExpectations(t)
+}
+
 // TestNewClientWithHTTP_NilFallbackIsHardened verifies that passing nil as the
 // httpClient falls back to the SSRF-hardened httpclient.New(), which rejects
 // IMDS connections, instead of leaving a nil client that panics on first use.

providers/azure/services/managedredis/client.go

@@ -518,8 +518,16 @@ func (c *Client) getRedisPricing(ctx context.Context, sku, region string, termYe
 	if err != nil {
 		return nil, err
 	}
-	filter := fmt.Sprintf("serviceName eq 'Redis Cache' and armRegionName eq '%s' and armSkuName eq '%s' and priceType eq 'Reservation'",
-		strings.ReplaceAll(region, "'", "''"), identity.ArmSKUName)
+	qRegion, err := pricing.ODataString(region)
+	if err != nil {
+		return nil, err
+	}
+	qSKU, err := pricing.ODataString(identity.ArmSKUName)
+	if err != nil {
+		return nil, err
+	}
+	filter := fmt.Sprintf("serviceName eq 'Redis Cache' and armRegionName eq %s and armSkuName eq %s and priceType eq 'Reservation'",
+		qRegion, qSKU)
 
 	params := url.Values{}
 	params.Add("$filter", filter)

providers/azure/services/synapse/client.go

@@ -464,8 +464,16 @@ type Pricing struct {
 
 // getSynapsePricing fetches pricing from the Azure Retail Prices API.
 func (c *Client) getSynapsePricing(ctx context.Context, sku, region string, termYears int) (*Pricing, error) {
-	filter := fmt.Sprintf("serviceName eq 'Azure Synapse Analytics' and productName eq 'Azure Synapse Analytics Dedicated SQL Pool' and armRegionName eq '%s' and skuName eq '%s' and priceType eq 'Reservation'",
-		strings.ReplaceAll(region, "'", "''"), strings.ReplaceAll(sku, "'", "''"))
+	qRegion, err := pricing.ODataString(region)
+	if err != nil {
+		return nil, err
+	}
+	qSKU, err := pricing.ODataString(sku)
+	if err != nil {
+		return nil, err
+	}
+	filter := fmt.Sprintf("serviceName eq 'Azure Synapse Analytics' and productName eq 'Azure Synapse Analytics Dedicated SQL Pool' and armRegionName eq %s and skuName eq %s and priceType eq 'Reservation'",
+		qRegion, qSKU)
 
 	params := url.Values{}
 	params.Add("$filter", filter)

providers/azure/services/synapse/client_test.go

@@ -957,16 +957,13 @@ func TestGetOfferingDetails_Pagination(t *testing.T) {
 	}
 }
 
-func TestGetOfferingDetails_QueryEscapesRegionAndSKU(t *testing.T) {
-	client, requests := synapseQuoteClient(t, "east'us", synapsePricePage(t, []map[string]any{
-		synapseCapturedDW100c(map[string]any{"armRegionName": "east'us", "skuName": "DW'100c"}),
-	}, ""))
+func TestGetOfferingDetails_RejectsQuoteInRegionAndSKU(t *testing.T) {
+	client, requests := synapseQuoteClient(t, "east'us", synapsePricePage(t, nil, ""))
 	quote, err := client.GetOfferingDetails(context.Background(), common.Recommendation{ResourceType: "DW'100c", Term: "1yr", PaymentOption: "upfront"})
-	require.NoError(t, err)
-	assert.Equal(t, 8333.0, quote.TotalCost)
-	require.Len(t, *requests, 1)
-	assert.Equal(t, "serviceName eq 'Azure Synapse Analytics' and productName eq 'Azure Synapse Analytics Dedicated SQL Pool' and armRegionName eq 'east''us' and skuName eq 'DW''100c' and priceType eq 'Reservation'", (*requests)[0].URL.Query().Get("$filter"))
-	assert.Contains(t, (*requests)[0].URL.RawQuery, "%24filter=")
+	require.Error(t, err)
+	assert.Nil(t, quote)
+	assert.Contains(t, err.Error(), "Retail Prices filter")
+	assert.Empty(t, *requests)
 }
 
 func TestGetOfferingDetails_RejectsUnsupportedTermAndPayment(t *testing.T) {