From cf8006dc1172d4426d4957a17541079072c42842 Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Tue, 17 Mar 2026 15:36:40 -0700 Subject: [PATCH 1/2] fix: improve input validation, error handling, and path safety - Harden path resolution to prevent directory traversal in extensions and templates - Add containment checks for vault IDs and local artifact paths - Enforce authenticated context for gRPC streaming RPCs - Add WebSocket origin validation for localhost-only connections - Pin JWT signing algorithm and increase key size - Replace sync.Once with retry-capable lazy init for tool downloads - Add sensitive data redaction in error paths - Validate constructor inputs and escape URL path segments - Add comprehensive test coverage for new validation logic Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/.vscode/cspell.yaml | 6 + cli/azd/cmd/vs_server.go | 3 +- .../azure.ai.agents/internal/cmd/init.go | 5 +- .../internal/cmd/init_from_code.go | 5 +- .../pkg/azure/foundry_projects_client.go | 72 +++++- .../pkg/azure/foundry_projects_client_test.go | 235 ++++++++++++++++++ cli/azd/internal/agent/tools/mcp/redact.go | 24 ++ .../internal/agent/tools/mcp/redact_test.go | 51 ++++ .../internal/grpcserver/extension_claims.go | 6 +- .../grpcserver/extension_claims_test.go | 155 ++++++++++++ cli/azd/internal/grpcserver/server.go | 94 ++++++- cli/azd/internal/grpcserver/server_test.go | 119 +++++++++ .../telemetry/appinsights-exporter/logging.go | 18 +- cli/azd/internal/telemetry/telemetry.go | 25 +- cli/azd/internal/telemetry/telemetry_test.go | 25 +- cli/azd/internal/vsrpc/origin_case_test.go | 34 +++ cli/azd/internal/vsrpc/server.go | 28 ++- cli/azd/internal/vsrpc/server_test.go | 28 +++ cli/azd/pkg/alpha/alpha_feature_manager.go | 25 +- cli/azd/pkg/alpha/alpha_feature_test.go | 41 ++- cli/azd/pkg/azapi/function_app.go | 4 +- cli/azd/pkg/azapi/permissions.go | 2 +- cli/azd/pkg/azapi/webapp.go | 4 +- cli/azd/pkg/azdext/azd_client.go | 34 ++- cli/azd/pkg/azdext/azd_client_test.go | 35 +++ cli/azd/pkg/azdext/debugger.go | 5 +- cli/azd/pkg/config/file_config_manager.go | 40 ++- .../pkg/config/file_config_manager_test.go | 139 ++++++++++- cli/azd/pkg/entraid/entraid.go | 2 +- cli/azd/pkg/extensions/claims.go | 38 +-- cli/azd/pkg/extensions/claims_test.go | 46 ++++ cli/azd/pkg/extensions/file_source.go | 18 +- cli/azd/pkg/extensions/manager.go | 16 ++ cli/azd/pkg/extensions/manager_test.go | 64 +++++ .../pkg/infra/azure_resource_manager_test.go | 4 +- .../provisioning/bicep/bicep_provider_test.go | 4 +- cli/azd/pkg/input/console_test.go | 2 +- cli/azd/pkg/osutil/lazy_retry_init.go | 42 ++++ cli/azd/pkg/osutil/lazy_retry_init_test.go | 120 +++++++++ cli/azd/pkg/osutil/path.go | 65 +++++ cli/azd/pkg/osutil/path_test.go | 171 +++++++++++++ cli/azd/pkg/project/container_helper.go | 2 +- cli/azd/pkg/templates/file_source.go | 18 +- cli/azd/pkg/tools/bicep/bicep.go | 11 +- cli/azd/pkg/tools/github/github.go | 9 +- cli/azd/pkg/tools/maven/maven.go | 19 +- cli/azd/pkg/tools/pack/pack.go | 9 +- cli/azd/test/recording/sanitize.go | 4 +- 48 files changed, 1763 insertions(+), 163 deletions(-) create mode 100644 cli/azd/extensions/azure.ai.agents/internal/pkg/azure/foundry_projects_client_test.go create mode 100644 cli/azd/internal/agent/tools/mcp/redact.go create mode 100644 cli/azd/internal/agent/tools/mcp/redact_test.go create mode 100644 cli/azd/internal/vsrpc/origin_case_test.go create mode 100644 cli/azd/pkg/azdext/azd_client_test.go create mode 100644 cli/azd/pkg/extensions/claims_test.go create mode 100644 cli/azd/pkg/osutil/lazy_retry_init.go create mode 100644 cli/azd/pkg/osutil/lazy_retry_init_test.go create mode 100644 cli/azd/pkg/osutil/path.go create mode 100644 cli/azd/pkg/osutil/path_test.go diff --git a/cli/azd/.vscode/cspell.yaml b/cli/azd/.vscode/cspell.yaml index 8787173187d..e2c3555f5e9 100644 --- a/cli/azd/.vscode/cspell.yaml +++ b/cli/azd/.vscode/cspell.yaml @@ -255,6 +255,12 @@ overrides: - filename: pkg/project/service_target_dotnet_containerapp.go words: - IMAGENAME + - filename: internal/vsrpc/server.go + words: + - CSWSH + - filename: pkg/extensions/manager.go + words: + - myext - filename: extensions/microsoft.azd.extensions/internal/resources/languages/**/.gitignore words: - rsuser diff --git a/cli/azd/cmd/vs_server.go b/cli/azd/cmd/vs_server.go index b680783d7c6..cad9f409ea5 100644 --- a/cli/azd/cmd/vs_server.go +++ b/cli/azd/cmd/vs_server.go @@ -101,7 +101,8 @@ func (s *vsServerAction) Run(ctx context.Context) (*actions.ActionResult, error) } listener = tls.NewListener(listener, config) - res.CertificateBytes = new(base64.StdEncoding.EncodeToString(derBytes)) + certBytesStr := base64.StdEncoding.EncodeToString(derBytes) + res.CertificateBytes = &certBytesStr } resString, err := json.Marshal(res) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/init.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/init.go index a5a6146cc6e..69c9fc6e413 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/init.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/init.go @@ -767,7 +767,10 @@ func (a *InitAction) parseAndSetProjectResourceId(ctx context.Context) error { } // Create FoundryProjectsClient and get connections - foundryClient := azure.NewFoundryProjectsClient(foundryProject.AiAccountName, foundryProject.AiProjectName, a.credential) + foundryClient, err := azure.NewFoundryProjectsClient(foundryProject.AiAccountName, foundryProject.AiProjectName, a.credential) + if err != nil { + return fmt.Errorf("creating Foundry client: %w", err) + } connections, err := foundryClient.GetAllConnections(ctx) if err != nil { fmt.Printf("Could not get Microsoft Foundry project connections to initialize AZURE_CONTAINER_REGISTRY_ENDPOINT: %v. Please set this environment variable manually.\n", err) diff --git a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_from_code.go b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_from_code.go index 773c713c965..5185bc23739 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/cmd/init_from_code.go +++ b/cli/azd/extensions/azure.ai.agents/internal/cmd/init_from_code.go @@ -1351,7 +1351,10 @@ func (a *InitFromCodeAction) processExistingFoundryProject(ctx context.Context, } // Create FoundryProjectsClient and get connections - foundryClient := azure.NewFoundryProjectsClient(foundryProject.AccountName, foundryProject.ProjectName, a.credential) + foundryClient, err := azure.NewFoundryProjectsClient(foundryProject.AccountName, foundryProject.ProjectName, a.credential) + if err != nil { + return fmt.Errorf("creating Foundry client: %w", err) + } connections, err := foundryClient.GetAllConnections(ctx) if err != nil { fmt.Printf("Could not get Microsoft Foundry project connections to initialize AZURE_CONTAINER_REGISTRY_ENDPOINT: %v. Please set this environment variable manually.\n", err) diff --git a/cli/azd/extensions/azure.ai.agents/internal/pkg/azure/foundry_projects_client.go b/cli/azd/extensions/azure.ai.agents/internal/pkg/azure/foundry_projects_client.go index 9a5938a57cc..9bcbafc02e4 100644 --- a/cli/azd/extensions/azure.ai.agents/internal/pkg/azure/foundry_projects_client.go +++ b/cli/azd/extensions/azure.ai.agents/internal/pkg/azure/foundry_projects_client.go @@ -10,6 +10,8 @@ import ( "fmt" "io" "net/http" + "net/url" + "strings" "github.com/Azure/azure-sdk-for-go/sdk/azcore" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" @@ -19,13 +21,25 @@ import ( // FoundryProjectsClient provides methods to interact with Microsoft Foundry projects type FoundryProjectsClient struct { - baseEndpoint string - apiVersion string - pipeline runtime.Pipeline + baseEndpoint string + baseOriginURL *url.URL // cached parsed base URL for SSRF origin checks + apiVersion string + pipeline runtime.Pipeline } // NewFoundryProjectsClient creates a new instance of FoundryProjectsClient -func NewFoundryProjectsClient(accountName string, projectName string, cred azcore.TokenCredential) *FoundryProjectsClient { +func NewFoundryProjectsClient( + accountName string, + projectName string, + cred azcore.TokenCredential, +) (*FoundryProjectsClient, error) { + if strings.TrimSpace(accountName) == "" { + return nil, fmt.Errorf("accountName must not be empty") + } + if strings.TrimSpace(projectName) == "" { + return nil, fmt.Errorf("projectName must not be empty") + } + baseEndpoint := fmt.Sprintf("https://%s.services.ai.azure.com/api/projects/%s", accountName, projectName) userAgent := fmt.Sprintf("azd-ext-azure-ai-agents/%s", version.Version) @@ -48,11 +62,17 @@ func NewFoundryProjectsClient(accountName string, projectName string, cred azcor clientOptions, ) - return &FoundryProjectsClient{ - baseEndpoint: baseEndpoint, - apiVersion: "2025-11-15-preview", - pipeline: pipeline, + parsedBase, err := url.Parse(baseEndpoint) + if err != nil { + return nil, fmt.Errorf("invalid base endpoint URL: %w", err) } + + return &FoundryProjectsClient{ + baseEndpoint: baseEndpoint, + baseOriginURL: parsedBase, + apiVersion: "2025-11-15-preview", + pipeline: pipeline, + }, nil } // Connection-related types @@ -144,7 +164,8 @@ func (c *FoundryProjectsClient) GetPagedConnections(ctx context.Context) (*Paged // GetConnectionWithCredentials retrieves a specific connection with its credentials func (c *FoundryProjectsClient) GetConnectionWithCredentials(ctx context.Context, name string) (*Connection, error) { targetEndpoint := fmt.Sprintf( - "%s/connections/%s/getConnectionWithCredentials?api-version=%s", c.baseEndpoint, name, c.apiVersion) + "%s/connections/%s/getConnectionWithCredentials?api-version=%s", + c.baseEndpoint, url.PathEscape(name), c.apiVersion) req, err := runtime.NewRequest(ctx, http.MethodPost, targetEndpoint) if err != nil { @@ -191,6 +212,9 @@ func (c *FoundryProjectsClient) GetAllConnections(ctx context.Context) ([]Connec // Continue fetching pages while there's a next link for nextLink != nil && *nextLink != "" { + if err := c.validateNextLinkOrigin(*nextLink); err != nil { + return nil, fmt.Errorf("refusing to follow pagination link: %w", err) + } pagedConnections, err := c.getNextPage(ctx, *nextLink) if err != nil { return nil, err @@ -204,6 +228,36 @@ func (c *FoundryProjectsClient) GetAllConnections(ctx context.Context) ([]Connec return allConnections, nil } +// validateNextLinkOrigin ensures that a pagination nextLink URL points to the same +// origin (scheme + host) as the client's base endpoint. This prevents SSRF attacks +// where a malicious API response redirects pagination to an attacker-controlled server. +func (c *FoundryProjectsClient) validateNextLinkOrigin(nextLink string) error { + if c.baseOriginURL == nil { + return fmt.Errorf("base endpoint URL not initialized") + } + + linkURL, err := url.Parse(nextLink) + if err != nil { + return fmt.Errorf("invalid nextLink URL: %w", err) + } + + // Reject scheme-relative URLs (e.g., "//evil.com/path") and URLs without an explicit scheme. + // These could bypass origin checks or behave unpredictably. + if linkURL.Scheme == "" { + return fmt.Errorf("nextLink must have an explicit scheme, got %q", nextLink) + } + + if !strings.EqualFold(linkURL.Scheme, c.baseOriginURL.Scheme) || + !strings.EqualFold(linkURL.Host, c.baseOriginURL.Host) { + return fmt.Errorf( + "nextLink origin mismatch: expected %s://%s, got %s://%s", + c.baseOriginURL.Scheme, c.baseOriginURL.Host, linkURL.Scheme, linkURL.Host, + ) + } + + return nil +} + // getNextPage fetches a single page of connections from the given URL func (c *FoundryProjectsClient) getNextPage(ctx context.Context, url string) (*PagedConnection, error) { req, err := runtime.NewRequest(ctx, http.MethodGet, url) diff --git a/cli/azd/extensions/azure.ai.agents/internal/pkg/azure/foundry_projects_client_test.go b/cli/azd/extensions/azure.ai.agents/internal/pkg/azure/foundry_projects_client_test.go new file mode 100644 index 00000000000..1c5c26c9f24 --- /dev/null +++ b/cli/azd/extensions/azure.ai.agents/internal/pkg/azure/foundry_projects_client_test.go @@ -0,0 +1,235 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package azure + +import ( + "fmt" + "net/url" + "strings" + "testing" + + "github.com/stretchr/testify/require" +) + +func TestValidateNextLinkOrigin(t *testing.T) { + baseURL, _ := url.Parse("https://myaccount.services.ai.azure.com/api/projects/myproject") + + client := &FoundryProjectsClient{ + baseOriginURL: baseURL, + } + + tests := []struct { + name string + link string + wantErr bool + }{ + { + name: "valid same-origin link", + link: "https://myaccount.services.ai.azure.com/api/projects/myproject/connections?skip=10", + wantErr: false, + }, + { + name: "cross-origin attack URL", + link: "https://evil.com/steal?data=secret", + wantErr: true, + }, + { + name: "scheme-relative URL", + link: "//evil.com/path", + wantErr: true, + }, + { + name: "missing scheme", + link: "evil.com/path", + wantErr: true, + }, + { + name: "malformed URL", + link: "://invalid", + wantErr: true, + }, + { + name: "different scheme", + link: "http://myaccount.services.ai.azure.com/api/projects/myproject", + wantErr: true, + }, + { + name: "different host same scheme", + link: "https://other.services.ai.azure.com/api/projects/myproject", + wantErr: true, + }, + { + name: "case-insensitive scheme match", + link: "HTTPS://myaccount.services.ai.azure.com/api/projects/myproject", + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + err := client.validateNextLinkOrigin(tt.link) + if tt.wantErr { + require.Error(t, err) + } else { + require.NoError(t, err) + } + }) + } +} + +func TestValidateNextLinkOrigin_NilBaseURL(t *testing.T) { + client := &FoundryProjectsClient{ + baseOriginURL: nil, + } + + err := client.validateNextLinkOrigin("https://example.com/path") + require.Error(t, err) + require.Contains(t, err.Error(), "not initialized") +} + +func TestNewFoundryProjectsClient_InvalidInputs(t *testing.T) { + tests := []struct { + name string + accountName string + projectName string + }{ + { + name: "account name with control characters", + accountName: "account\x00name", + projectName: "project", + }, + { + name: "project name with control characters", + accountName: "account", + projectName: "project\x00name", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client, err := NewFoundryProjectsClient(tt.accountName, tt.projectName, nil) + // If the URL parses successfully (Go is permissive), client should still be non-nil + // The key thing is that the constructor doesn't panic + if err != nil { + require.Nil(t, client) + } else { + require.NotNil(t, client) + } + }) + } +} + +func TestNewFoundryProjectsClient_EmptyInputs(t *testing.T) { + tests := []struct { + name string + accountName string + projectName string + errContains string + }{ + { + name: "empty account name", + accountName: "", + projectName: "myproject", + errContains: "accountName must not be empty", + }, + { + name: "whitespace-only account name", + accountName: " ", + projectName: "myproject", + errContains: "accountName must not be empty", + }, + { + name: "empty project name", + accountName: "myaccount", + projectName: "", + errContains: "projectName must not be empty", + }, + { + name: "whitespace-only project name", + accountName: "myaccount", + projectName: " \t ", + errContains: "projectName must not be empty", + }, + { + name: "both empty", + accountName: "", + projectName: "", + errContains: "accountName must not be empty", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + client, err := NewFoundryProjectsClient(tt.accountName, tt.projectName, nil) + require.Error(t, err) + require.Nil(t, client) + require.Contains(t, err.Error(), tt.errContains) + }) + } +} + +func TestGetConnectionWithCredentials_AdversarialNames(t *testing.T) { + // Verify that url.PathEscape properly sanitizes adversarial connection names + // so they cannot cause path traversal in the constructed URL. + tests := []struct { + name string + connectionName string + mustNotContain string // the raw dangerous substring that must NOT appear unescaped in the path + }{ + { + name: "path traversal with dotdotslash", + connectionName: "../../../evil", + mustNotContain: "../", + }, + { + name: "slashes in name", + connectionName: "name/with/slashes", + mustNotContain: "name/with/slashes", + }, + { + name: "null byte injection", + connectionName: "name%00null", + mustNotContain: "name%00null/", + }, + { + name: "backslash traversal", + connectionName: `..\..\evil`, + mustNotContain: `..\..\evil/`, + }, + { + name: "encoded slash attempt", + connectionName: "..%2F..%2Fevil", + mustNotContain: "../", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + escaped := url.PathEscape(tt.connectionName) + + // The escaped value must not contain raw slashes that could alter the URL path. + // url.PathEscape encodes '/' to '%2F', '.' to raw '.', but the key invariant + // is that no unescaped '/' appears in the escaped output. + require.NotContains(t, escaped, "/", + "url.PathEscape must escape slashes to prevent path traversal") + + // Build the URL the same way the production code does and verify + // the escaped name appears as a single opaque segment in the raw URL string. + rawURL := fmt.Sprintf( + "https://example.com/api/projects/proj/connections/%s/getConnectionWithCredentials", + escaped) + + // Count the path segments in the raw (non-parsed) URL to confirm + // the adversarial name didn't inject extra segments. + rawPath := strings.TrimPrefix(rawURL, "https://example.com") + rawSegments := strings.Split(strings.Trim(rawPath, "/"), "/") + require.Equal(t, 6, len(rawSegments), + "adversarial name %q should not inject extra path segments in raw URL, got: %s", + tt.connectionName, rawPath) + require.Equal(t, "connections", rawSegments[3]) + require.Equal(t, escaped, rawSegments[4]) + require.Equal(t, "getConnectionWithCredentials", rawSegments[5]) + }) + } +} diff --git a/cli/azd/internal/agent/tools/mcp/redact.go b/cli/azd/internal/agent/tools/mcp/redact.go new file mode 100644 index 00000000000..9930e1b4489 --- /dev/null +++ b/cli/azd/internal/agent/tools/mcp/redact.go @@ -0,0 +1,24 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package mcp + +import "regexp" + +// redactSensitiveData removes sensitive information from strings that may be logged. +// It handles: Authorization headers, bearer tokens, API tokens, and raw token strings. +func redactSensitiveData(s string) string { + // Redact Authorization header values + s = authHeaderRedactor.ReplaceAllString(s, "${1}[REDACTED]") + // Redact bearer tokens + s = bearerTokenRedactor.ReplaceAllString(s, "Bearer [REDACTED]") + // Redact token= query parameters + s = tokenParamRedactor.ReplaceAllString(s, "token=[REDACTED]") + return s +} + +var ( + authHeaderRedactor = regexp.MustCompile(`(?i)(Authorization:\s*)(.+)`) + bearerTokenRedactor = regexp.MustCompile(`(?i)Bearer\s+[A-Za-z0-9\-._~+/]+=*`) + tokenParamRedactor = regexp.MustCompile(`(?i)token=[A-Za-z0-9\-._~+/]+=*`) +) diff --git a/cli/azd/internal/agent/tools/mcp/redact_test.go b/cli/azd/internal/agent/tools/mcp/redact_test.go new file mode 100644 index 00000000000..4100ca4de9b --- /dev/null +++ b/cli/azd/internal/agent/tools/mcp/redact_test.go @@ -0,0 +1,51 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package mcp + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func Test_RedactSensitiveData(t *testing.T) { + tests := []struct { + name string + input string + expected string + }{ + { + name: "redacts authorization header", + input: "Authorization: Bearer eyJhbGciOiJSUzI1NiIsInR5cCI6IkpXVCJ9", + expected: "Authorization: [REDACTED]", + }, + { + name: "redacts bearer token in text", + input: "got error with Bearer eyJhbGciOiJSUzI1NiJ9.eyJzdWIiOiIxMjM0NTY3ODkwIn0.signature", + expected: "got error with Bearer [REDACTED]", + }, + { + name: "redacts token query parameter", + input: "https://example.com/api?token=abc123def456&other=value", + expected: "https://example.com/api?token=[REDACTED]&other=value", + }, + { + name: "no redaction needed", + input: "normal log message without sensitive data", + expected: "normal log message without sensitive data", + }, + { + name: "multiple sensitive values", + input: "Authorization: secret123 and token=abc456 found", + expected: "Authorization: [REDACTED]", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := redactSensitiveData(tt.input) + require.Equal(t, tt.expected, result) + }) + } +} diff --git a/cli/azd/internal/grpcserver/extension_claims.go b/cli/azd/internal/grpcserver/extension_claims.go index 214e20cbff9..a4273c57a36 100644 --- a/cli/azd/internal/grpcserver/extension_claims.go +++ b/cli/azd/internal/grpcserver/extension_claims.go @@ -25,7 +25,7 @@ func GenerateExtensionToken(extension *extensions.Extension, serverInfo *ServerI Capabilities: extension.Capabilities, } - jwtToken, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString([]byte(serverInfo.SigningKey)) + jwtToken, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString(serverInfo.SigningKey) if err != nil { return "", err } @@ -38,11 +38,11 @@ func ParseExtensionToken(tokenValue string, serverInfo *ServerInfo) (*extensions claims := &extensions.ExtensionClaims{} token, err := jwt.ParseWithClaims(tokenValue, claims, func(t *jwt.Token) (any, error) { - if _, ok := t.Method.(*jwt.SigningMethodHMAC); !ok { + if t.Method != jwt.SigningMethodHS256 { return nil, fmt.Errorf("unexpected signing method: %v", t.Header["alg"]) } - return []byte(serverInfo.SigningKey), nil + return serverInfo.SigningKey, nil }, jwt.WithAudience(serverInfo.Address), jwt.WithIssuer("azd")) if err != nil { diff --git a/cli/azd/internal/grpcserver/extension_claims_test.go b/cli/azd/internal/grpcserver/extension_claims_test.go index e85fd45acbc..394c766d557 100644 --- a/cli/azd/internal/grpcserver/extension_claims_test.go +++ b/cli/azd/internal/grpcserver/extension_claims_test.go @@ -4,12 +4,23 @@ package grpcserver import ( + "crypto/rand" + "crypto/rsa" + "strings" "testing" + "time" "github.com/azure/azure-dev/cli/azd/pkg/extensions" + "github.com/golang-jwt/jwt/v5" "github.com/stretchr/testify/require" ) +func Test_GenerateSigningKey_Length(t *testing.T) { + key, err := generateSigningKey() + require.NoError(t, err) + require.Len(t, key, 32, "signing key must be 32 bytes (256-bit)") +} + func Test_GenerateParse_TokenClaims(t *testing.T) { signingKey, err := generateSigningKey() require.NoError(t, err) @@ -59,3 +70,147 @@ func Test_GenerateParse_TokenClaims(t *testing.T) { require.Nil(t, claims) }) } + +// Test_ParseExtensionToken_AlgorithmSubstitution verifies that a token signed +// with RS256 (asymmetric) is rejected by the HMAC-only key function guard. +func Test_ParseExtensionToken_AlgorithmSubstitution(t *testing.T) { + signingKey, err := generateSigningKey() + require.NoError(t, err) + + serverInfo := &ServerInfo{ + Address: "localhost:5678", + Port: 5678, + SigningKey: signingKey, + } + + // Generate an RSA key and sign a token with RS256 — the parser must reject this. + rsaKey, err := rsa.GenerateKey(rand.Reader, 2048) + require.NoError(t, err) + + claims := extensions.ExtensionClaims{ + RegisteredClaims: jwt.RegisteredClaims{ + Issuer: "azd", + Subject: "microsoft.azd.evil", + Audience: []string{serverInfo.Address}, + IssuedAt: jwt.NewNumericDate(time.Now()), + ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour)), + }, + Capabilities: []extensions.CapabilityType{extensions.CustomCommandCapability}, + } + + rs256Token, err := jwt.NewWithClaims(jwt.SigningMethodRS256, claims).SignedString(rsaKey) + require.NoError(t, err) + + parsed, err := ParseExtensionToken(rs256Token, serverInfo) + require.Error(t, err) + require.Nil(t, parsed) + require.Contains(t, err.Error(), "unexpected signing method") +} + +// Test_ParseExtensionToken_AlgorithmSubstitution_HS384 verifies that a token signed +// with HS384 (a valid HMAC variant, but not the pinned HS256) is rejected. +func Test_ParseExtensionToken_AlgorithmSubstitution_HS384(t *testing.T) { + signingKey, err := generateSigningKey() + require.NoError(t, err) + + serverInfo := &ServerInfo{ + Address: "localhost:5678", + Port: 5678, + SigningKey: signingKey, + } + + claims := extensions.ExtensionClaims{ + RegisteredClaims: jwt.RegisteredClaims{ + Issuer: "azd", + Subject: "microsoft.azd.evil", + Audience: []string{serverInfo.Address}, + IssuedAt: jwt.NewNumericDate(time.Now()), + ExpiresAt: jwt.NewNumericDate(time.Now().Add(time.Hour)), + }, + Capabilities: []extensions.CapabilityType{extensions.CustomCommandCapability}, + } + + // Sign with HS384 — a valid HMAC variant but not the pinned HS256 + hs384Token, err := jwt.NewWithClaims(jwt.SigningMethodHS384, claims).SignedString(signingKey) + require.NoError(t, err) + + parsed, err := ParseExtensionToken(hs384Token, serverInfo) + require.Error(t, err) + require.Nil(t, parsed) + require.Contains(t, err.Error(), "unexpected signing method") +} + +// Test_ParseExtensionToken_TamperedToken verifies that a structurally valid +// token whose payload has been tampered with is rejected (signature mismatch). +func Test_ParseExtensionToken_TamperedToken(t *testing.T) { + signingKey, err := generateSigningKey() + require.NoError(t, err) + + serverInfo := &ServerInfo{ + Address: "localhost:5678", + Port: 5678, + SigningKey: signingKey, + } + + ext := &extensions.Extension{ + Id: "microsoft.azd.test", + Namespace: "test", + Capabilities: []extensions.CapabilityType{ + extensions.CustomCommandCapability, + }, + DisplayName: "Test", + Version: "0.0.1", + } + + token, err := GenerateExtensionToken(ext, serverInfo) + require.NoError(t, err) + + // Tamper with the token: flip a character in the payload segment (middle part) + parts := strings.SplitN(token, ".", 3) + require.Len(t, parts, 3) + + // Corrupt the payload by replacing the first char + payload := []byte(parts[1]) + if payload[0] == 'A' { + payload[0] = 'B' + } else { + payload[0] = 'A' + } + tampered := parts[0] + "." + string(payload) + "." + parts[2] + + parsed, err := ParseExtensionToken(tampered, serverInfo) + require.Error(t, err) + require.Nil(t, parsed) +} + +// Test_ParseExtensionToken_ExpiredToken verifies that an expired token is +// rejected by the expiration check inside ParseExtensionToken. +func Test_ParseExtensionToken_ExpiredToken(t *testing.T) { + signingKey, err := generateSigningKey() + require.NoError(t, err) + + serverInfo := &ServerInfo{ + Address: "localhost:5678", + Port: 5678, + SigningKey: signingKey, + } + + // Create a token that expired 2 hours ago + claims := extensions.ExtensionClaims{ + RegisteredClaims: jwt.RegisteredClaims{ + Issuer: "azd", + Subject: "microsoft.azd.expired", + Audience: []string{serverInfo.Address}, + IssuedAt: jwt.NewNumericDate(time.Now().Add(-3 * time.Hour)), + ExpiresAt: jwt.NewNumericDate(time.Now().Add(-2 * time.Hour)), + }, + Capabilities: []extensions.CapabilityType{extensions.CustomCommandCapability}, + } + + expiredToken, err := jwt.NewWithClaims(jwt.SigningMethodHS256, claims).SignedString(signingKey) + require.NoError(t, err) + + parsed, err := ParseExtensionToken(expiredToken, serverInfo) + require.Error(t, err) + require.Nil(t, parsed) +} diff --git a/cli/azd/internal/grpcserver/server.go b/cli/azd/internal/grpcserver/server.go index 645ff0f134d..ef3965683ba 100644 --- a/cli/azd/internal/grpcserver/server.go +++ b/cli/azd/internal/grpcserver/server.go @@ -14,6 +14,7 @@ import ( "github.com/azure/azure-dev/cli/azd/internal" "github.com/azure/azure-dev/cli/azd/pkg/auth" "github.com/azure/azure-dev/cli/azd/pkg/azdext" + "github.com/azure/azure-dev/cli/azd/pkg/extensions" "google.golang.org/grpc" "google.golang.org/grpc/codes" "google.golang.org/grpc/metadata" @@ -91,6 +92,10 @@ func (s *Server) Start() (*ServerInfo, error) { s.errorWrappingInterceptor(), s.tokenAuthInterceptor(&serverInfo), ), + grpc.ChainStreamInterceptor( + s.errorWrappingStreamInterceptor(), + s.tokenAuthStreamInterceptor(&serverInfo), + ), ) // Use ":0" to let the system assign an available random port @@ -167,6 +172,48 @@ func (s *Server) errorWrappingInterceptor() grpc.UnaryServerInterceptor { } } +// errorWrappingStreamInterceptor is the streaming counterpart of errorWrappingInterceptor. +// It wraps ErrorWithSuggestion errors from stream handlers so that actionable suggestions +// are preserved in gRPC stream error responses. +func (s *Server) errorWrappingStreamInterceptor() grpc.StreamServerInterceptor { + return func( + srv any, + ss grpc.ServerStream, + info *grpc.StreamServerInfo, + handler grpc.StreamHandler, + ) error { + err := handler(srv, ss) + if err != nil { + err = wrapErrorWithSuggestion(err) + } + return err + } +} + +// validateAuthToken extracts and validates the authorization token from gRPC metadata, +// returning a new context with validated claims attached. This shared helper ensures +// both unary and stream RPCs enforce the same token validation. +func (s *Server) validateAuthToken(ctx context.Context, serverInfo *ServerInfo) (context.Context, error) { + md, ok := metadata.FromIncomingContext(ctx) + if !ok { + return ctx, status.Error(codes.Unauthenticated, "metadata missing") + } + + // Extract the authorization token from metadata + token := md["authorization"] + if len(token) == 0 { + return ctx, status.Error(codes.Unauthenticated, "invalid token") + } + + claims, err := ParseExtensionToken(token[0], serverInfo) + if err != nil { + return ctx, status.Error(codes.Unauthenticated, "invalid token") + } + + // Store validated claims in context for downstream handlers + return extensions.WithClaimsContext(ctx, claims), nil +} + func (s *Server) tokenAuthInterceptor(serverInfo *ServerInfo) grpc.UnaryServerInterceptor { return func( ctx context.Context, @@ -174,27 +221,48 @@ func (s *Server) tokenAuthInterceptor(serverInfo *ServerInfo) grpc.UnaryServerIn info *grpc.UnaryServerInfo, handler grpc.UnaryHandler, ) (any, error) { - md, ok := metadata.FromIncomingContext(ctx) - if !ok { - return nil, status.Error(codes.Unauthenticated, "metadata missing") + ctx, err := s.validateAuthToken(ctx, serverInfo) + if err != nil { + return nil, err } - // Extract the authorization token from metadata - token := md["authorization"] - if len(token) == 0 { - return nil, status.Error(codes.Unauthenticated, "invalid token") - } + // Proceed to the handler with enriched context + return handler(ctx, req) + } +} - _, err := ParseExtensionToken(token[0], serverInfo) +func (s *Server) tokenAuthStreamInterceptor(serverInfo *ServerInfo) grpc.StreamServerInterceptor { + return func( + srv any, + ss grpc.ServerStream, + info *grpc.StreamServerInfo, + handler grpc.StreamHandler, + ) error { + ctx, err := s.validateAuthToken(ss.Context(), serverInfo) if err != nil { - return nil, status.Error(codes.Unauthenticated, "invalid token") + return err } - // Proceed to the handler - return handler(ctx, req) + // Wrap the stream to inject validated claims into its context + wrappedStream := &authenticatedStream{ + ServerStream: ss, + ctx: ctx, + } + + return handler(srv, wrappedStream) } } +// authenticatedStream wraps a grpc.ServerStream to provide a context with validated claims. +type authenticatedStream struct { + grpc.ServerStream + ctx context.Context +} + +func (s *authenticatedStream) Context() context.Context { + return s.ctx +} + // wrapErrorWithSuggestion checks if the error contains an ErrorWithSuggestion and if so, // returns a new error that includes the suggestion text in the error message. // This ensures that helpful suggestions (like "run azd auth login") are preserved @@ -226,7 +294,7 @@ func wrapErrorWithSuggestion(err error) error { } func generateSigningKey() ([]byte, error) { - bytes := make([]byte, 16) // 128-bit token + bytes := make([]byte, 32) // 256-bit HMAC signing key if _, err := rand.Read(bytes); err != nil { return nil, err } diff --git a/cli/azd/internal/grpcserver/server_test.go b/cli/azd/internal/grpcserver/server_test.go index 0f5ec4de65d..80758d7ac6e 100644 --- a/cli/azd/internal/grpcserver/server_test.go +++ b/cli/azd/internal/grpcserver/server_test.go @@ -108,6 +108,125 @@ func Test_Server_Start(t *testing.T) { }) } +// Test_Server_StreamInterceptor validates that the streaming RPC interceptor +// enforces authentication the same way as the unary interceptor. +func Test_Server_StreamInterceptor(t *testing.T) { + server := NewServer( + azdext.UnimplementedProjectServiceServer{}, + azdext.UnimplementedEnvironmentServiceServer{}, + azdext.UnimplementedPromptServiceServer{}, + azdext.UnimplementedUserConfigServiceServer{}, + azdext.UnimplementedDeploymentServiceServer{}, + azdext.UnimplementedEventServiceServer{}, + azdext.UnimplementedComposeServiceServer{}, + azdext.UnimplementedWorkflowServiceServer{}, + azdext.UnimplementedExtensionServiceServer{}, + azdext.UnimplementedServiceTargetServiceServer{}, + azdext.UnimplementedFrameworkServiceServer{}, + azdext.UnimplementedContainerServiceServer{}, + azdext.UnimplementedAccountServiceServer{}, + azdext.UnimplementedAiModelServiceServer{}, + ) + + serverInfo, err := server.Start() + require.NotNil(t, serverInfo) + require.NoError(t, err) + defer func() { + err := server.Stop() + require.NoError(t, err) + }() + + extension := &extensions.Extension{ + Id: "azd.internal.test", + Capabilities: []extensions.CapabilityType{ + extensions.CustomCommandCapability, + }, + Namespace: "test", + } + + t.Run("ValidToken", func(t *testing.T) { + accessToken, err := GenerateExtensionToken(extension, serverInfo) + require.NoError(t, err) + + ctx := azdext.WithAccessToken(context.Background(), accessToken) + client, err := azdext.NewAzdClient(azdext.WithAddress(serverInfo.Address)) + require.NoError(t, err) + defer client.Close() + + stream, err := client.Events().EventStream(ctx) + // With a valid token, the stream should open successfully. + // The underlying service is unimplemented, so we may get Unimplemented on Send/Recv, + // but the stream itself should be created without an auth error. + if err != nil { + st, ok := status.FromError(err) + require.True(t, ok) + // Unimplemented is acceptable (mock service), but Unauthenticated is not. + require.NotEqual(t, codes.Unauthenticated, st.Code(), + "valid token should not get Unauthenticated") + } else { + require.NotNil(t, stream) + // Try to close the send side; any error should be Unimplemented, not Unauthenticated. + err = stream.CloseSend() + if err != nil { + st, ok := status.FromError(err) + if ok { + require.NotEqual(t, codes.Unauthenticated, st.Code()) + } + } + } + }) + + t.Run("MissingToken", func(t *testing.T) { + ctx := context.Background() + client, err := azdext.NewAzdClient(azdext.WithAddress(serverInfo.Address)) + require.NoError(t, err) + defer client.Close() + + stream, err := client.Events().EventStream(ctx) + if err != nil { + st, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, codes.Unauthenticated, st.Code()) + } else { + // For bidi streams, the auth error may surface on Recv rather than stream creation. + _, recvErr := stream.Recv() + require.Error(t, recvErr) + st, ok := status.FromError(recvErr) + require.True(t, ok) + require.Equal(t, codes.Unauthenticated, st.Code()) + } + }) + + t.Run("InvalidToken", func(t *testing.T) { + invalidServerInfo := &ServerInfo{ + Address: serverInfo.Address, + Port: serverInfo.Port, + SigningKey: []byte("invalid"), + } + accessToken, err := GenerateExtensionToken(extension, invalidServerInfo) + require.NoError(t, err) + + ctx := azdext.WithAccessToken(context.Background(), accessToken) + client, err := azdext.NewAzdClient(azdext.WithAddress(serverInfo.Address)) + require.NoError(t, err) + defer client.Close() + + stream, err := client.Events().EventStream(ctx) + if err != nil { + st, ok := status.FromError(err) + require.True(t, ok) + require.Equal(t, codes.Unauthenticated, st.Code()) + } else { + // For bidi streams, the auth error may surface on Recv rather than stream creation. + _, recvErr := stream.Recv() + require.Error(t, recvErr) + st, ok := status.FromError(recvErr) + require.True(t, ok) + require.Equal(t, codes.Unauthenticated, st.Code()) + } + }) +} + func Test_wrapErrorWithSuggestion(t *testing.T) { tests := []struct { name string diff --git a/cli/azd/internal/telemetry/appinsights-exporter/logging.go b/cli/azd/internal/telemetry/appinsights-exporter/logging.go index 6e9d63fa534..6ac03073529 100644 --- a/cli/azd/internal/telemetry/appinsights-exporter/logging.go +++ b/cli/azd/internal/telemetry/appinsights-exporter/logging.go @@ -3,23 +3,33 @@ package appinsightsexporter -import "fmt" +import ( + "fmt" + "sync" +) // Provides simple local logging functionality for the telemetry library. type logger struct { + mu sync.RWMutex listen func(s string) } // Process-wide logger var diagLog logger = logger{listen: func(string) {}} -// Sets the diagnostics logging listener for telemetry related warnings. -// This is NOT thread-safe, and thus should be set once, early in application lifecycle. +// SetListener sets the diagnostics logging listener for telemetry related warnings. +// This is safe to call concurrently. func SetListener(listener func(s string)) { + diagLog.mu.Lock() + defer diagLog.mu.Unlock() diagLog.listen = listener } func (log *logger) Printf(format string, a ...any) { - log.listen(fmt.Sprintf(format, a...)) + log.mu.RLock() + fn := log.listen + log.mu.RUnlock() + // Safe: fn is an immutable function reference; calling outside lock avoids potential deadlocks + fn(fmt.Sprintf(format, a...)) } diff --git a/cli/azd/internal/telemetry/telemetry.go b/cli/azd/internal/telemetry/telemetry.go index 0a00d6b36f2..d6a5730f2cb 100644 --- a/cli/azd/internal/telemetry/telemetry.go +++ b/cli/azd/internal/telemetry/telemetry.go @@ -11,7 +11,6 @@ import ( "net/url" "os" "path/filepath" - "sync" "time" "github.com/azure/azure-dev/cli/azd/internal" @@ -19,6 +18,7 @@ import ( appinsightsexporter "github.com/azure/azure-dev/cli/azd/internal/telemetry/appinsights-exporter" "github.com/azure/azure-dev/cli/azd/internal/tracing/resource" "github.com/azure/azure-dev/cli/azd/pkg/config" + "github.com/azure/azure-dev/cli/azd/pkg/osutil" "github.com/benbjohnson/clock" "github.com/gofrs/flock" "github.com/spf13/pflag" @@ -52,8 +52,16 @@ type TelemetrySystem struct { telemetryDirectory string } -var once sync.Once -var instance *TelemetrySystem +var ( + telemetryInit osutil.LazyRetryInit + instance *TelemetrySystem +) + +// resetTelemetryForTest resets the telemetry singleton state, allowing re-initialization in tests. +func resetTelemetryForTest() { + telemetryInit = osutil.LazyRetryInit{} + instance = nil +} func getTelemetryDirectory() (string, error) { configDir, err := config.GetUserConfigDir() @@ -82,15 +90,18 @@ func IsTelemetryEnabled() bool { // Returns the singleton TelemetrySystem instance. // Returns nil if telemetry failed to initialize, or user has disabled telemetry. func GetTelemetrySystem() *TelemetrySystem { - once.Do(func() { + err := telemetryInit.Do(func() error { telemetrySystem, err := initialize() if err != nil { log.Printf("failed to initialize telemetry: %v\n", err) - } else { - instance = telemetrySystem + return err } + instance = telemetrySystem + return nil }) - + if err != nil { + return nil + } return instance } diff --git a/cli/azd/internal/telemetry/telemetry_test.go b/cli/azd/internal/telemetry/telemetry_test.go index 11f125bf2fd..f16bb337e94 100644 --- a/cli/azd/internal/telemetry/telemetry_test.go +++ b/cli/azd/internal/telemetry/telemetry_test.go @@ -5,7 +5,6 @@ package telemetry import ( "context" - "sync" "testing" "github.com/azure/azure-dev/cli/azd/internal" @@ -50,12 +49,29 @@ func TestGetTelemetrySystem(t *testing.T) { devEndpointConfig, }, - {"ProdVersion", args{"1.0.0 (commit 13ec2b11aa755b11640fa16b8664cb8741d5d300)", "no"}, true, prodEndpointConfig}, - {"ProdVersion", args{"1.0.0 (commit 13ec2b11aa755b11640fa16b8664cb8741d5d300)", "unset"}, false, prodEndpointConfig}, - {"ProdVersion", args{"1.0.0 (commit 13ec2b11aa755b11640fa16b8664cb8741d5d300)", "yes"}, false, prodEndpointConfig}, + { + "ProdVersion_telemetryDisabled", + args{"1.0.0 (commit 13ec2b11aa755b11640fa16b8664cb8741d5d300)", "no"}, + true, + prodEndpointConfig, + }, + { + "ProdVersion_telemetryUnset", + args{"1.0.0 (commit 13ec2b11aa755b11640fa16b8664cb8741d5d300)", "unset"}, + false, + prodEndpointConfig, + }, + { + "ProdVersion_telemetryEnabled", + args{"1.0.0 (commit 13ec2b11aa755b11640fa16b8664cb8741d5d300)", "yes"}, + false, + prodEndpointConfig, + }, } for _, tt := range tests { t.Run(tt.name, func(t *testing.T) { + defer resetTelemetryForTest() + orig := internal.Version defer func() { internal.Version = orig }() internal.Version = tt.args.version @@ -79,7 +95,6 @@ func TestGetTelemetrySystem(t *testing.T) { err := ts.Shutdown(context.Background()) assert.NoError(t, err) } - once = sync.Once{} }) } } diff --git a/cli/azd/internal/vsrpc/origin_case_test.go b/cli/azd/internal/vsrpc/origin_case_test.go new file mode 100644 index 00000000000..c239d20392a --- /dev/null +++ b/cli/azd/internal/vsrpc/origin_case_test.go @@ -0,0 +1,34 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package vsrpc + +import ( + "net/http/httptest" + "testing" + + "github.com/stretchr/testify/require" +) + +// Test that case variations are handled +func TestCheckLocalhostOrigin_CaseSensitivity(t *testing.T) { + tests := []struct { + name string + origin string + expected bool + }{ + {"lowercase localhost", "http://localhost:8080", true}, + {"uppercase LOCALHOST", "http://LOCALHOST:8080", true}, // Hostname comparison should be case-insensitive. + {"mixed case LocAlHost", "http://LocAlHost:8080", true}, // Hostname comparison should be case-insensitive. + {"uppercase 127.0.0.1", "HTTP://127.0.0.1:8080", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + req.Header.Set("Origin", tt.origin) + result := checkLocalhostOrigin(req) + require.Equal(t, tt.expected, result, "origin: %s", tt.origin) + }) + } +} diff --git a/cli/azd/internal/vsrpc/server.go b/cli/azd/internal/vsrpc/server.go index 148a784ee6f..3666af3cbac 100644 --- a/cli/azd/internal/vsrpc/server.go +++ b/cli/azd/internal/vsrpc/server.go @@ -11,8 +11,10 @@ import ( "log" "net" "net/http" + "net/url" "os" "strconv" + "strings" "sync" "time" @@ -46,7 +48,31 @@ func NewServer(rootContainer *ioc.NestedContainer) *Server { } // upgrader is the websocket.Upgrader used by the server to upgrade each request to a websocket connection. -var upgrader = websocket.Upgrader{} +var upgrader = websocket.Upgrader{ + CheckOrigin: checkLocalhostOrigin, +} + +// checkLocalhostOrigin prevents Cross-Site WebSocket Hijacking (CSWSH) by rejecting +// WebSocket upgrade requests from non-localhost origins. Without this check, any web +// page the user visits could open a WebSocket connection to the local RPC server. +func checkLocalhostOrigin(r *http.Request) bool { + origin := r.Header.Get("Origin") + if origin == "" { + return true // same-origin requests don't send Origin + } + + u, err := url.Parse(origin) + if err != nil { + return false + } + + host := u.Hostname() + if strings.EqualFold(host, "localhost") { + return true + } + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} // Serve calls http.Serve with the given listener and a handler that serves the VS RPC protocol. func (s *Server) Serve(l net.Listener) error { diff --git a/cli/azd/internal/vsrpc/server_test.go b/cli/azd/internal/vsrpc/server_test.go index 9fdebed3edd..a45bff1917f 100644 --- a/cli/azd/internal/vsrpc/server_test.go +++ b/cli/azd/internal/vsrpc/server_test.go @@ -193,3 +193,31 @@ func TestPanic(t *testing.T) { require.Error(t, err) require.Contains(t, err.Error(), "this is the panic office, again.") } + +func TestCheckLocalhostOrigin(t *testing.T) { + tests := []struct { + name string + origin string + expected bool + }{ + {"empty origin (same-origin)", "", true}, + {"localhost", "http://localhost:8080", true}, + {"127.0.0.1", "http://127.0.0.1:3000", true}, + {"ipv6 loopback", "http://[::1]:8080", true}, + {"external domain rejected", "http://evil.com", false}, + {"localhost subdomain rejected", "http://localhost.evil.com", false}, + {"non-localhost IP rejected", "http://192.168.1.1:8080", false}, + {"malformed URL rejected", "not-a-url://%%%", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + req := httptest.NewRequest("GET", "/", nil) + if tt.origin != "" { + req.Header.Set("Origin", tt.origin) + } + result := checkLocalhostOrigin(req) + require.Equal(t, tt.expected, result) + }) + } +} diff --git a/cli/azd/pkg/alpha/alpha_feature_manager.go b/cli/azd/pkg/alpha/alpha_feature_manager.go index fca1860af22..6f56add0461 100644 --- a/cli/azd/pkg/alpha/alpha_feature_manager.go +++ b/cli/azd/pkg/alpha/alpha_feature_manager.go @@ -114,7 +114,7 @@ func (m *FeatureManager) IsEnabled(featureId FeatureId) bool { } else if featureOn := isEnabled(m.userConfigCache, featureId); featureOn { // check if the feature is ON enabled = true - } else if val, ok := defaultEnablement[strings.ToLower(string(featureId))]; ok { + } else if val, ok := getDefaultEnablement(string(featureId)); ok { // check if the feature has been set with a default value internally enabled = val } @@ -129,13 +129,34 @@ func (m *FeatureManager) IsEnabled(featureId FeatureId) bool { // defaultEnablement is a map of lower-cased feature ids to their default enablement values. // // This is used to determine if a feature is enabled by default, when no user configuration is specified. -var defaultEnablement = map[string]bool{} +var ( + defaultEnablement = map[string]bool{} + defaultEnablementMu sync.RWMutex +) // SetDefaultEnablement sets the default enablement value for the given feature id. func SetDefaultEnablement(id string, val bool) { + defaultEnablementMu.Lock() + defer defaultEnablementMu.Unlock() defaultEnablement[strings.ToLower(id)] = val } +// ResetDefaultEnablement removes the default enablement value for the given feature id. +// This is intended for test cleanup to avoid leaking state between tests. +func ResetDefaultEnablement(id string) { + defaultEnablementMu.Lock() + defer defaultEnablementMu.Unlock() + delete(defaultEnablement, strings.ToLower(id)) +} + +// getDefaultEnablement returns the default enablement value for the given feature id. +func getDefaultEnablement(id string) (bool, bool) { + defaultEnablementMu.RLock() + defer defaultEnablementMu.RUnlock() + val, ok := defaultEnablement[strings.ToLower(id)] + return val, ok +} + func isEnabled(config config.Config, id FeatureId) bool { longKey := fmt.Sprintf("%s.%s", parentKey, string(id)) value, exists := config.Get(longKey) diff --git a/cli/azd/pkg/alpha/alpha_feature_test.go b/cli/azd/pkg/alpha/alpha_feature_test.go index fd10b95207f..a66c7e9e5af 100644 --- a/cli/azd/pkg/alpha/alpha_feature_test.go +++ b/cli/azd/pkg/alpha/alpha_feature_test.go @@ -5,7 +5,6 @@ package alpha import ( "fmt" - "os" "sync" "testing" @@ -188,18 +187,56 @@ func Test_AlphaFeature_IsEnabled(t *testing.T) { }) t.Run("enabled from env var", func(t *testing.T) { - os.Setenv("AZD_ALPHA_ENABLE_CATEGORY_FEATURE_3", "true") + t.Setenv("AZD_ALPHA_ENABLE_CATEGORY_FEATURE_3", "true") actual := alphaManager.IsEnabled(FeatureId("category.feature.3")) require.True(t, actual) }) t.Run("enabled from default", func(t *testing.T) { SetDefaultEnablement("category.feature.9", true) + t.Cleanup(func() { + ResetDefaultEnablement("category.feature.9") + }) actual := alphaManager.IsEnabled(FeatureId("category.feature.9")) require.True(t, actual) }) } +func Test_DefaultEnablement_ConcurrentAccess(t *testing.T) { + t.Parallel() + + mockConfig := config.NewConfig(nil) + alphaManager := newFeatureManagerForTest(func() []Feature { return nil }, mockConfig) + + const goroutines = 50 + var wg sync.WaitGroup + wg.Add(goroutines * 2) + + t.Cleanup(func() { + for i := range goroutines { + ResetDefaultEnablement(fmt.Sprintf("race.feature.%d", i)) + } + }) + + // Concurrent writers + for i := range goroutines { + go func(i int) { + defer wg.Done() + SetDefaultEnablement(fmt.Sprintf("race.feature.%d", i), i%2 == 0) + }(i) + } + + // Concurrent readers + for i := range goroutines { + go func(i int) { + defer wg.Done() + _ = alphaManager.IsEnabled(FeatureId(fmt.Sprintf("race.feature.%d", i))) + }(i) + } + + wg.Wait() +} + func newFeatureManagerForTest(alphaFeatureResolver func() []Feature, config config.Config) *FeatureManager { return &FeatureManager{ alphaFeaturesResolver: alphaFeatureResolver, diff --git a/cli/azd/pkg/azapi/function_app.go b/cli/azd/pkg/azapi/function_app.go index 07f0ebdae1e..9f17fd10544 100644 --- a/cli/azd/pkg/azapi/function_app.go +++ b/cli/azd/pkg/azapi/function_app.go @@ -96,7 +96,7 @@ func (cli *AzureClient) DeployFunctionAppUsingZipFileFlexConsumption( if err != nil { return nil, fmt.Errorf("publishing zip file: %w", err) } - return new(response.StatusText), nil + return &response.StatusText, nil } // DeployFunctionAppUsingZipFileRegular deploys to a regular (non-Flex Consumption) function app @@ -123,7 +123,7 @@ func (cli *AzureClient) DeployFunctionAppUsingZipFileRegular( return nil, err } - return new(response.StatusText), nil + return &response.StatusText, nil } // functionAppRepositoryHost finds the SCM host name from function app properties. diff --git a/cli/azd/pkg/azapi/permissions.go b/cli/azd/pkg/azapi/permissions.go index 867439c1481..d687d066375 100644 --- a/cli/azd/pkg/azapi/permissions.go +++ b/cli/azd/pkg/azapi/permissions.go @@ -77,7 +77,7 @@ func (s *PermissionsService) HasRequiredPermissions( pager := roleAssignmentsClient.NewListForScopePager( subscriptionScope, &armauthorization.RoleAssignmentsClientListForScopeOptions{ - Filter: new(filter), + Filter: &filter, }, ) diff --git a/cli/azd/pkg/azapi/webapp.go b/cli/azd/pkg/azapi/webapp.go index c21be8bed9e..2a048427b5a 100644 --- a/cli/azd/pkg/azapi/webapp.go +++ b/cli/azd/pkg/azapi/webapp.go @@ -188,7 +188,7 @@ func (cli *AzureClient) DeployAppServiceZip( return nil, err } - return new(response.StatusText), nil + return &response.StatusText, nil } func (cli *AzureClient) createWebAppsClient( @@ -335,5 +335,5 @@ func (cli *AzureClient) DeployAppServiceSlotZip( return nil, err } - return new(response.StatusText), nil + return &response.StatusText, nil } diff --git a/cli/azd/pkg/azdext/azd_client.go b/cli/azd/pkg/azdext/azd_client.go index 22557bd687f..93ff3dd23c5 100644 --- a/cli/azd/pkg/azdext/azd_client.go +++ b/cli/azd/pkg/azdext/azd_client.go @@ -5,10 +5,12 @@ package azdext import ( "context" + "net" "os" "strings" "google.golang.org/grpc" + "google.golang.org/grpc/credentials" "google.golang.org/grpc/credentials/insecure" "google.golang.org/grpc/metadata" ) @@ -36,7 +38,16 @@ type AzdClient struct { // WithAddress sets the address of the `azd` gRPC server. func WithAddress(address string) AzdClientOption { return func(c *AzdClient) error { - connection, err := grpc.NewClient(address, grpc.WithTransportCredentials(insecure.NewCredentials())) + var opts []grpc.DialOption + + if isLocalhostAddress(address) { + opts = append(opts, grpc.WithTransportCredentials(insecure.NewCredentials())) + } else { + // For non-localhost connections, require TLS to prevent MITM attacks + opts = append(opts, grpc.WithTransportCredentials(credentials.NewTLS(nil))) + } + + connection, err := grpc.NewClient(address, opts...) if err != nil { return err } @@ -46,6 +57,27 @@ func WithAddress(address string) AzdClientOption { } } +// isLocalhostAddress checks if the given address refers to the local machine. +// It parses host:port format and checks against known localhost identifiers. +func isLocalhostAddress(address string) bool { + host := address + // Strip port if present + if h, _, err := net.SplitHostPort(address); err == nil { + host = h + } + + host = strings.TrimSpace(strings.ToLower(host)) + + switch host { + case "localhost", "127.0.0.1", "::1", "[::1]": + return true + } + + // Check if it's a loopback IP + ip := net.ParseIP(host) + return ip != nil && ip.IsLoopback() +} + // WithAccessToken sets the access token for the `azd` client into a new Go context. func WithAccessToken(ctx context.Context, params ...string) context.Context { tokenValue := strings.Join(params, "") diff --git a/cli/azd/pkg/azdext/azd_client_test.go b/cli/azd/pkg/azdext/azd_client_test.go new file mode 100644 index 00000000000..06bd5c46981 --- /dev/null +++ b/cli/azd/pkg/azdext/azd_client_test.go @@ -0,0 +1,35 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package azdext + +import ( + "testing" + + "github.com/stretchr/testify/require" +) + +func Test_IsLocalhostAddress(t *testing.T) { + tests := []struct { + name string + address string + expected bool + }{ + {"localhost with port", "localhost:8080", true}, + {"127.0.0.1 with port", "127.0.0.1:3000", true}, + {"ipv6 loopback with port", "[::1]:8080", true}, + {"localhost without port", "localhost", true}, + {"127.0.0.1 without port", "127.0.0.1", true}, + {"external hostname", "example.com:8080", false}, + {"external IP", "192.168.1.1:8080", false}, + {"empty string", "", false}, + {"loopback IP 127.0.0.2", "127.0.0.2:8080", true}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := isLocalhostAddress(tt.address) + require.Equal(t, tt.expected, result) + }) + } +} diff --git a/cli/azd/pkg/azdext/debugger.go b/cli/azd/pkg/azdext/debugger.go index 31f2487b200..fe96d5d8131 100644 --- a/cli/azd/pkg/azdext/debugger.go +++ b/cli/azd/pkg/azdext/debugger.go @@ -85,7 +85,10 @@ func getExtensionId(ctx context.Context) string { return "" } - // Parse the JWT token without validation (we just need the subject claim) + // ParseUnverified is intentionally used here for debug/diagnostic logging only. + // This token has already been validated by the gRPC auth interceptor before reaching + // this code path. We only parse it here to extract claims for debug output, not for + // any security decision. Using ParseUnverified avoids needing access to the signing key. token, _, err := jwt.NewParser().ParseUnverified(tokenString, jwt.MapClaims{}) if err != nil { return "" diff --git a/cli/azd/pkg/config/file_config_manager.go b/cli/azd/pkg/config/file_config_manager.go index 388379f9626..63c577b6ef4 100644 --- a/cli/azd/pkg/config/file_config_manager.go +++ b/cli/azd/pkg/config/file_config_manager.go @@ -7,10 +7,14 @@ import ( "fmt" "os" "path/filepath" + "regexp" "github.com/azure/azure-dev/cli/azd/pkg/osutil" ) +// validVaultIDPattern matches vault IDs containing only alphanumeric characters, hyphens, and underscores. +var validVaultIDPattern = regexp.MustCompile(`^[a-zA-Z0-9][a-zA-Z0-9_-]*$`) + // FileConfigManager provides the ability to load, parse and save azd configuration files type FileConfigManager interface { // Saves the azd configuration to the specified file path @@ -48,12 +52,11 @@ func (m *fileConfigManager) Load(filePath string) (Config, error) { // If the configuration contains a vault, then also load the vault configuration vaultId, ok := azdConfig.GetString(vaultKeyName) if ok { - configPath, err := GetUserConfigDir() + vaultPath, err := resolveVaultPath(vaultId) if err != nil { - return nil, fmt.Errorf("failed getting user config directory: %w", err) + return nil, err } - vaultPath := filepath.Join(configPath, "vaults", fmt.Sprintf("%s.json", vaultId)) vaultConfig, err := m.Load(vaultPath) if err != nil { return nil, fmt.Errorf("failed loading vault configuration from '%s': %w", vaultPath, err) @@ -96,12 +99,11 @@ func (m *fileConfigManager) Save(c Config, filePath string) error { // If the configuration contains a vault, then also save the vault configuration // Vault configuration always gets saved in a separate file in the users HOME directory. if baseConfig.vaultId != "" { - configPath, err := GetUserConfigDir() + vaultPath, err := resolveVaultPath(baseConfig.vaultId) if err != nil { - return fmt.Errorf("failed getting user config directory: %w", err) + return err } - vaultPath := filepath.Join(configPath, "vaults", fmt.Sprintf("%s.json", baseConfig.vaultId)) if err = os.MkdirAll(filepath.Dir(vaultPath), osutil.PermissionDirectory); err != nil { return fmt.Errorf("failed creating vaults directory: %w", err) } @@ -111,3 +113,29 @@ func (m *fileConfigManager) Save(c Config, filePath string) error { return nil } + +// resolveVaultPath validates a vault ID and returns the full path to the vault JSON file. +// It enforces an allowlist of safe characters and verifies the resolved path stays within the vaults directory. +func resolveVaultPath(vaultId string) (string, error) { + if !validVaultIDPattern.MatchString(vaultId) { + return "", fmt.Errorf( + "invalid vault ID %q: must contain only alphanumeric characters, hyphens, and underscores", + vaultId, + ) + } + + configPath, err := GetUserConfigDir() + if err != nil { + return "", fmt.Errorf("failed getting user config directory: %w", err) + } + + vaultsDir := filepath.Join(configPath, "vaults") + vaultPath := filepath.Join(vaultsDir, fmt.Sprintf("%s.json", vaultId)) + + // Defense-in-depth: also verify the resolved path stays within the vaults directory + if !osutil.IsPathContained(vaultsDir, vaultPath) { + return "", fmt.Errorf("invalid vault ID %q: resolved path is outside the vaults directory", vaultId) + } + + return vaultPath, nil +} diff --git a/cli/azd/pkg/config/file_config_manager_test.go b/cli/azd/pkg/config/file_config_manager_test.go index eb95b974947..3536aa74907 100644 --- a/cli/azd/pkg/config/file_config_manager_test.go +++ b/cli/azd/pkg/config/file_config_manager_test.go @@ -52,8 +52,7 @@ func Test_FileConfigManager_GetSetSecrets(t *testing.T) { tempDir := t.TempDir() azdConfigDir := filepath.Join(tempDir, ".azd") - err := os.Setenv("AZD_CONFIG_DIR", azdConfigDir) - require.NoError(t, err) + t.Setenv("AZD_CONFIG_DIR", azdConfigDir) // Set and save secrets configFilePath := filepath.Join(tempDir, "config.json") @@ -62,7 +61,7 @@ func Test_FileConfigManager_GetSetSecrets(t *testing.T) { // Standard secrets expectedPassword := "P@55w0rd!" - err = azdConfig.SetSecret("secrets.password", expectedPassword) + err := azdConfig.SetSecret("secrets.password", expectedPassword) require.NoError(t, err) err = azdConfig.SetSecret("infra.provisioning.sqlPassword", expectedPassword) @@ -119,15 +118,14 @@ func Test_FileConfigManager_GetSetSecretsInSection(t *testing.T) { tempDir := t.TempDir() azdConfigDir := filepath.Join(tempDir, ".azd") - err := os.Setenv("AZD_CONFIG_DIR", azdConfigDir) - require.NoError(t, err) + t.Setenv("AZD_CONFIG_DIR", azdConfigDir) // Set and save secrets configFilePath := filepath.Join(tempDir, "config.json") configManager := NewFileConfigManager(NewManager()) azdConfig := NewConfig(nil) - err = azdConfig.SetSecret("infra.provisioning.secret1", "secrect1Value") + err := azdConfig.SetSecret("infra.provisioning.secret1", "secrect1Value") require.NoError(t, err) err = azdConfig.SetSecret("infra.provisioning.secret2", "secrect2Value") @@ -156,3 +154,132 @@ func Test_FileConfigManager_GetSetSecretsInSection(t *testing.T) { require.True(t, ok) require.Equal(t, "normalValue", normalValue) } + +func Test_FileConfigManager_VaultID_PathTraversal(t *testing.T) { + tests := []struct { + name string + vaultId string + wantErr bool + }{ + {"valid vault ID", "my-vault-123", false}, + {"double dot traversal", "../../../etc/shadow", true}, + {"forward slash", "path/to/vault", true}, + {"backslash", `path\to\vault`, true}, + {"embedded double dot", "vault..id", true}, + {"clean vault ID with dash and underscore", "vault-id_123", false}, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tempDir := t.TempDir() + configFilePath := filepath.Join(tempDir, "config.json") + configManager := NewFileConfigManager(NewManager()) + + // Create a config with the vault reference + azdConfig := NewConfig(map[string]any{ + vaultKeyName: tt.vaultId, + }) + + err := configManager.Save(azdConfig, configFilePath) + require.NoError(t, err) + // Load will validate the vault ID + if tt.wantErr { + // For save: the vaultId in the raw config won't trigger Save's vault path, + // but Load will reject it + _, err = configManager.Load(configFilePath) + require.Error(t, err) + require.Contains(t, err.Error(), "invalid vault ID") + } else { + // Valid vault IDs may still fail if the vault file doesn't exist, but + // they should NOT fail with "invalid vault ID" + _, err = configManager.Load(configFilePath) + if err != nil { + require.NotContains(t, err.Error(), "invalid vault ID") + } + } + }) + } +} + +// Test_FileConfigManager_Save_VaultID_PathTraversal exercises the vault ID +// path-traversal guard inside Save (line 105-109 of file_config_manager.go). +// Save checks baseConfig.vaultId (the struct field), so we must set it directly +// on the *config struct along with a non-nil vault to trigger the vault-save branch. +func Test_FileConfigManager_Save_VaultID_PathTraversal(t *testing.T) { + tests := []struct { + name string + vaultId string + wantErr bool + errMsg string + }{ + { + name: "traversal with parent directory sequence", + vaultId: "../../../etc/passwd", + wantErr: true, + errMsg: "invalid vault ID", + }, + { + name: "bare double dot", + vaultId: "..", + wantErr: true, + errMsg: "invalid vault ID", + }, + { + name: "forward slash in vault ID", + vaultId: "malicious/vault", + wantErr: true, + errMsg: "invalid vault ID", + }, + { + name: "backslash in vault ID", + vaultId: `malicious\vault`, + wantErr: true, + errMsg: "invalid vault ID", + }, + { + name: "valid vault ID succeeds", + vaultId: "good-vault-id-123", + wantErr: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + tempDir := t.TempDir() + azdConfigDir := filepath.Join(tempDir, ".azd") + + t.Setenv("AZD_CONFIG_DIR", azdConfigDir) + + configFilePath := filepath.Join(tempDir, "config.json") + configManager := NewFileConfigManager(NewManager()) + + // Build a config with the vaultId struct field set directly. + // This is the field Save checks (line 104), not the "vault" key in data. + cfg := &config{ + vaultId: tt.vaultId, + vault: NewConfig(map[string]any{"secret-key": "secret-value"}), + data: map[string]any{"key": "value"}, + } + + err := configManager.Save(cfg, configFilePath) + + if tt.wantErr { + require.Error(t, err) + require.Contains(t, err.Error(), tt.errMsg) + + // Verify no file was written to the vaults directory for malicious IDs + vaultsDir := filepath.Join(azdConfigDir, "vaults") + if _, statErr := os.Stat(vaultsDir); statErr == nil { + entries, _ := os.ReadDir(vaultsDir) + require.Empty(t, entries, "no vault files should be created for malicious vault IDs") + } + } else { + require.NoError(t, err) + + // Verify the vault file was written to the expected location + expectedVaultPath := filepath.Join(azdConfigDir, "vaults", fmt.Sprintf("%s.json", tt.vaultId)) + require.FileExists(t, expectedVaultPath) + } + }) + } +} diff --git a/cli/azd/pkg/entraid/entraid.go b/cli/azd/pkg/entraid/entraid.go index 513ad925713..364206814e9 100644 --- a/cli/azd/pkg/entraid/entraid.go +++ b/cli/azd/pkg/entraid/entraid.go @@ -552,7 +552,7 @@ func (ad *entraIdService) CreateRbac( subscriptionId, scope, &armauthorization.RoleDefinition{ - ID: new(fullRoleId), + ID: &fullRoleId, Name: new(roleId), }, &graphsdk.ServicePrincipal{ diff --git a/cli/azd/pkg/extensions/claims.go b/cli/azd/pkg/extensions/claims.go index 8f31ac6b15f..1d302388d8a 100644 --- a/cli/azd/pkg/extensions/claims.go +++ b/cli/azd/pkg/extensions/claims.go @@ -8,7 +8,6 @@ import ( "fmt" "github.com/golang-jwt/jwt/v5" - "google.golang.org/grpc/metadata" ) // ExtensionClaims represents the claims in the JWT token for the extension. @@ -17,33 +16,22 @@ type ExtensionClaims struct { Capabilities []CapabilityType `json:"cap,omitempty"` } -// GetClaimsFromContext retrieves the extension claims from the incoming gRPC context. -func GetClaimsFromContext(ctx context.Context) (*ExtensionClaims, error) { - // First check the incoming context - md, ok := metadata.FromIncomingContext(ctx) - if !ok { - // Otherwise check the outgoing context - md, ok = metadata.FromOutgoingContext(ctx) - } - - if !ok { - return nil, fmt.Errorf("failed to get metadata from context") - } +// extensionClaimsKeyType is the context key for storing validated extension claims. +type extensionClaimsKeyType struct{} - authHeaders := md.Get("authorization") - if len(authHeaders) == 0 { - return nil, fmt.Errorf("missing authorization header") - } +var extensionClaimsKey = extensionClaimsKeyType{} - tokenValue := authHeaders[0] - if tokenValue == "" { - return nil, fmt.Errorf("missing token value") - } +// WithClaimsContext returns a new context with the validated extension claims stored in it. +func WithClaimsContext(ctx context.Context, claims *ExtensionClaims) context.Context { + return context.WithValue(ctx, extensionClaimsKey, claims) +} - claims := &ExtensionClaims{} - _, _, err := jwt.NewParser().ParseUnverified(tokenValue, claims) - if err != nil { - return nil, fmt.Errorf("failed to parse token: %w", err) +// GetClaimsFromContext retrieves validated extension claims from the context. +// Claims must have been stored by the gRPC auth interceptor via WithClaimsContext. +func GetClaimsFromContext(ctx context.Context) (*ExtensionClaims, error) { + claims, ok := ctx.Value(extensionClaimsKey).(*ExtensionClaims) + if !ok || claims == nil { + return nil, fmt.Errorf("no validated extension claims found in context") } return claims, nil diff --git a/cli/azd/pkg/extensions/claims_test.go b/cli/azd/pkg/extensions/claims_test.go new file mode 100644 index 00000000000..857b84319be --- /dev/null +++ b/cli/azd/pkg/extensions/claims_test.go @@ -0,0 +1,46 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package extensions + +import ( + "context" + "testing" + + "github.com/golang-jwt/jwt/v5" + "github.com/stretchr/testify/require" +) + +func Test_GetClaimsFromContext_WithValidClaims(t *testing.T) { + claims := &ExtensionClaims{ + RegisteredClaims: jwt.RegisteredClaims{ + Subject: "test-extension", + Issuer: "azd", + }, + Capabilities: []CapabilityType{CustomCommandCapability}, + } + + ctx := WithClaimsContext(context.Background(), claims) + retrieved, err := GetClaimsFromContext(ctx) + require.NoError(t, err) + require.NotNil(t, retrieved) + require.Equal(t, "test-extension", retrieved.Subject) + require.Equal(t, "azd", retrieved.Issuer) + require.Contains(t, retrieved.Capabilities, CustomCommandCapability) +} + +func Test_GetClaimsFromContext_WithoutClaims_ReturnsError(t *testing.T) { + ctx := context.Background() + claims, err := GetClaimsFromContext(ctx) + require.Error(t, err) + require.Nil(t, claims) + require.Contains(t, err.Error(), "no validated extension claims found in context") +} + +func Test_GetClaimsFromContext_NilClaims_ReturnsError(t *testing.T) { + ctx := context.WithValue(context.Background(), extensionClaimsKey, (*ExtensionClaims)(nil)) + claims, err := GetClaimsFromContext(ctx) + require.Error(t, err) + require.Nil(t, claims) + require.Contains(t, err.Error(), "no validated extension claims found in context") +} diff --git a/cli/azd/pkg/extensions/file_source.go b/cli/azd/pkg/extensions/file_source.go index 091990184ad..a848d46d1ea 100644 --- a/cli/azd/pkg/extensions/file_source.go +++ b/cli/azd/pkg/extensions/file_source.go @@ -9,6 +9,7 @@ import ( "path/filepath" "github.com/azure/azure-dev/cli/azd/pkg/config" + "github.com/azure/azure-dev/cli/azd/pkg/osutil" ) // newFileSource creates a new file base registry source. @@ -49,20 +50,5 @@ func getAbsolutePath(filePath string) (string, error) { roots = append(roots, cwd) roots = append(roots, azdConfigPath) - for _, root := range roots { - // Join the root directory with the relative path - absolutePath := filepath.Join(root, filePath) - - // Normalize the path to handle any ".." or "." segments - absolutePath, err = filepath.Abs(absolutePath) - if err != nil { - return "", err - } - - if _, err := os.Stat(absolutePath); err == nil { - return absolutePath, nil - } - } - - return "", fmt.Errorf("file '%s' was not found, %w", filePath, os.ErrNotExist) + return osutil.ResolveContainedPath(roots, filePath) } diff --git a/cli/azd/pkg/extensions/manager.go b/cli/azd/pkg/extensions/manager.go index 92a12f2b2e2..157d195905b 100644 --- a/cli/azd/pkg/extensions/manager.go +++ b/cli/azd/pkg/extensions/manager.go @@ -482,6 +482,13 @@ func (m *Manager) Install( targetPath := filepath.Join(targetDir, entryPoint) + // Prevent path traversal attacks by ensuring entryPoint stays within the extension install directory + if !osutil.IsPathContained(targetDir, targetPath) { + return nil, fmt.Errorf( + "invalid entry point path: entry point %q resolves outside extension directory. "+ + "Use relative paths (e.g., 'bin/myext' or 'bin\\myext') without '..' sequences", entryPoint) + } + // Need to set the executable permission for the binary // This change is specifically required for Linux but will apply consistently across all platforms if err := os.Chmod(targetPath, osutil.PermissionExecutableFile); err != nil { @@ -692,6 +699,15 @@ func (m *Manager) copyFromLocalPath(artifactPath string) (string, error) { } artifactPath = filepath.Join(userConfigDir, artifactPath) + + // Prevent path traversal attacks by ensuring artifact path stays within the user config directory. + // This validation only applies to relative paths resolved against userConfigDir. + // Absolute paths are trusted since they are explicitly configured by the user/admin. + if !osutil.IsPathContained(userConfigDir, artifactPath) { + return "", fmt.Errorf( + "invalid artifact path: path %q resolves outside user config directory %q. "+ + "Use an absolute path or a path relative to %q", artifactPath, userConfigDir, userConfigDir) + } } if _, err := os.Stat(artifactPath); os.IsNotExist(err) { diff --git a/cli/azd/pkg/extensions/manager_test.go b/cli/azd/pkg/extensions/manager_test.go index 82bacbbc7ad..0eed001f56d 100644 --- a/cli/azd/pkg/extensions/manager_test.go +++ b/cli/azd/pkg/extensions/manager_test.go @@ -12,6 +12,7 @@ import ( "net/http" "os" "path/filepath" + "runtime" "slices" "strings" "testing" @@ -19,6 +20,7 @@ import ( "github.com/azure/azure-dev/cli/azd/pkg/config" "github.com/azure/azure-dev/cli/azd/pkg/exec" "github.com/azure/azure-dev/cli/azd/pkg/lazy" + "github.com/azure/azure-dev/cli/azd/pkg/osutil" "github.com/azure/azure-dev/cli/azd/test/mocks" "github.com/stretchr/testify/require" ) @@ -1180,3 +1182,65 @@ func Test_GetInstalled_WithSourceFilter(t *testing.T) { require.ErrorIs(t, err, ErrInstalledExtensionNotFound) }) } + +func Test_EntryPoint_PathTraversal_Blocked(t *testing.T) { + // Test that path traversal in entryPoint is detected via the shared IsPathContained utility + testCases := []struct { + name string + entryPoint string + shouldFail bool + windowsOnly bool + }{ + {"normal entry point", "myext.exe", false, false}, + {"subdirectory entry point", "bin/myext", false, false}, + {"path traversal attempt", "../../malicious", true, false}, + // While backslashes are traditionally path separators only on Windows, IsPathContained normalizes + // them on all platforms and will treat `..\..` as a traversal sequence even on non-Windows. + {"path traversal with backslash", `..\..\malicious`, true, false}, + {"double dot deep traversal", "../../../../../../../tmp/evil", true, false}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + if tc.windowsOnly && runtime.GOOS != "windows" { + t.Skip("backslash path traversal only applies on Windows") + } + baseDir := t.TempDir() + targetPath := filepath.Join(baseDir, tc.entryPoint) + isContained := osutil.IsPathContained(baseDir, targetPath) + + if tc.shouldFail { + require.False(t, isContained, "path traversal should have been detected for %q", tc.entryPoint) + } else { + require.True(t, isContained, "legitimate entry point %q should be allowed", tc.entryPoint) + } + }) + } +} + +func Test_CopyFromLocalPath_PathTraversal_Blocked(t *testing.T) { + // Verify the containment check logic for relative artifact paths via shared IsPathContained + testCases := []struct { + name string + path string + shouldFail bool + }{ + {"normal relative path", "extensions/myext.zip", false}, + {"path traversal attempt", "../../etc/passwd", true}, + {"path traversal with nested dirs", "../../../tmp/malicious", true}, + } + + for _, tc := range testCases { + t.Run(tc.name, func(t *testing.T) { + baseDir := t.TempDir() + resolvedPath := filepath.Join(baseDir, tc.path) + isContained := osutil.IsPathContained(baseDir, resolvedPath) + + if tc.shouldFail { + require.False(t, isContained, "path traversal should have been detected for %q", tc.path) + } else { + require.True(t, isContained, "legitimate path %q should be allowed", tc.path) + } + }) + } +} diff --git a/cli/azd/pkg/infra/azure_resource_manager_test.go b/cli/azd/pkg/infra/azure_resource_manager_test.go index e949b54e7d4..cd2ee37f277 100644 --- a/cli/azd/pkg/infra/azure_resource_manager_test.go +++ b/cli/azd/pkg/infra/azure_resource_manager_test.go @@ -194,7 +194,7 @@ func createNestedDeploymentOperation( ProvisioningState: new(string(state)), TargetResource: &armresources.TargetResource{ ResourceType: to.Ptr(string(azapi.AzureResourceTypeDeployment)), - ID: new(resourceID), + ID: &resourceID, ResourceName: new(deploymentName), }, Timestamp: new(time.Now().UTC().Add(time.Hour)), @@ -216,7 +216,7 @@ func createLeafOperation(id string, resourceType string, resourceName string) *a ProvisioningState: to.Ptr(string(armresources.ProvisioningStateSucceeded)), TargetResource: &armresources.TargetResource{ ResourceType: new(resourceType), - ID: new(resourceID), + ID: &resourceID, ResourceName: new(resourceName), }, Timestamp: new(time.Now().UTC().Add(time.Hour)), diff --git a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go index 54dec11d82c..1bb52ef0a11 100644 --- a/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go +++ b/cli/azd/pkg/infra/provisioning/bicep/bicep_provider_test.go @@ -566,7 +566,7 @@ func prepareDestroyMocks(mockContext *mocks.MockContext) { string(resourceType), resourceName) return &armresources.GenericResourceExpanded{ - ID: new(id), + ID: &id, Name: new(resourceName), Type: new(string(resourceType)), Location: new("eastus2"), @@ -846,7 +846,7 @@ func prepareLogAnalyticsDestroyMocks(mockContext *mocks.MockContext) { string(resourceType), resourceName) return &armresources.GenericResourceExpanded{ - ID: new(id), + ID: &id, Name: new(resourceName), Type: new(string(resourceType)), Location: new("eastus2"), diff --git a/cli/azd/pkg/input/console_test.go b/cli/azd/pkg/input/console_test.go index d7a41868d64..0c9535c30f2 100644 --- a/cli/azd/pkg/input/console_test.go +++ b/cli/azd/pkg/input/console_test.go @@ -260,7 +260,7 @@ func newTestExternalPromptServer(handler func(promptOptions) json.RawMessage) *h respBody, _ := json.Marshal(promptResponse{ Status: "success", - Value: new(res), + Value: &res, }) _, _ = w.Write(respBody) diff --git a/cli/azd/pkg/osutil/lazy_retry_init.go b/cli/azd/pkg/osutil/lazy_retry_init.go new file mode 100644 index 00000000000..dbc3da0def7 --- /dev/null +++ b/cli/azd/pkg/osutil/lazy_retry_init.go @@ -0,0 +1,42 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package osutil + +import ( + "sync" + "sync/atomic" +) + +// LazyRetryInit provides a thread-safe initialization pattern that caches success +// but allows retries on failure. Unlike sync.Once, failed calls are not cached, +// so subsequent calls can retry the initialization. +// Uses atomic.Bool for a lock-free fast path after successful initialization. +type LazyRetryInit struct { + mu sync.Mutex + done atomic.Bool +} + +// Do calls f if initialization has not yet succeeded. If f returns nil, +// the result is cached and subsequent calls return nil immediately (lock-free fast path). +// If f returns an error, the error is returned and f will be called again +// on the next invocation. +func (l *LazyRetryInit) Do(f func() error) error { + if l.done.Load() { + return nil + } + + l.mu.Lock() + defer l.mu.Unlock() + + if l.done.Load() { + return nil + } + + if err := f(); err != nil { + return err + } + + l.done.Store(true) + return nil +} diff --git a/cli/azd/pkg/osutil/lazy_retry_init_test.go b/cli/azd/pkg/osutil/lazy_retry_init_test.go new file mode 100644 index 00000000000..a3f54b9537e --- /dev/null +++ b/cli/azd/pkg/osutil/lazy_retry_init_test.go @@ -0,0 +1,120 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package osutil + +import ( + "errors" + "sync" + "sync/atomic" + "testing" + "time" + + "github.com/stretchr/testify/require" +) + +func TestLazyRetryInit_SuccessCached(t *testing.T) { + var l LazyRetryInit + callCount := int32(0) + + for range 3 { + err := l.Do(func() error { + atomic.AddInt32(&callCount, 1) + return nil + }) + require.NoError(t, err) + } + + require.Equal(t, int32(1), atomic.LoadInt32(&callCount), "f should only be called once on success") +} + +func TestLazyRetryInit_FailureRetried(t *testing.T) { + var l LazyRetryInit + callCount := int32(0) + errTemporary := errors.New("temporary failure") + + // First call fails + err := l.Do(func() error { + atomic.AddInt32(&callCount, 1) + return errTemporary + }) + require.ErrorIs(t, err, errTemporary) + require.Equal(t, int32(1), atomic.LoadInt32(&callCount)) + + // Second call retries and succeeds + err = l.Do(func() error { + atomic.AddInt32(&callCount, 1) + return nil + }) + require.NoError(t, err) + require.Equal(t, int32(2), atomic.LoadInt32(&callCount)) + + // Third call is cached + err = l.Do(func() error { + atomic.AddInt32(&callCount, 1) + return nil + }) + require.NoError(t, err) + require.Equal(t, int32(2), atomic.LoadInt32(&callCount)) +} + +func TestLazyRetryInit_ConcurrentSuccess(t *testing.T) { + var l LazyRetryInit + var callCount int32 + + var wg sync.WaitGroup + for range 50 { + wg.Go(func() { + _ = l.Do(func() error { + atomic.AddInt32(&callCount, 1) + time.Sleep(10 * time.Millisecond) + return nil + }) + }) + } + wg.Wait() + + // Only one goroutine should have executed the init function (since it succeeded) + require.Equal(t, int32(1), atomic.LoadInt32(&callCount)) +} + +func TestLazyRetryInit_ConcurrentFailureRetry(t *testing.T) { + var l LazyRetryInit + var callCount int32 + errTemporary := errors.New("temporary failure") + + // First wave: all goroutines should see the failure + var wg sync.WaitGroup + for range 20 { + wg.Go(func() { + err := l.Do(func() error { + atomic.AddInt32(&callCount, 1) + time.Sleep(5 * time.Millisecond) + return errTemporary + }) + require.ErrorIs(t, err, errTemporary) + }) + } + wg.Wait() + + // At least one goroutine must have called the init function, but since the mutex + // serializes access and each call fails (not cached), multiple calls are expected. + firstWaveCount := atomic.LoadInt32(&callCount) + require.GreaterOrEqual(t, firstWaveCount, int32(1)) + + // Second wave: succeed this time + atomic.StoreInt32(&callCount, 0) + for range 20 { + wg.Go(func() { + _ = l.Do(func() error { + atomic.AddInt32(&callCount, 1) + return nil + }) + }) + } + wg.Wait() + + // After success, only one goroutine should execute the init (the first to acquire the lock). + // Subsequent goroutines find done==true and skip. + require.Equal(t, int32(1), atomic.LoadInt32(&callCount)) +} diff --git a/cli/azd/pkg/osutil/path.go b/cli/azd/pkg/osutil/path.go new file mode 100644 index 00000000000..5b1ac4c229a --- /dev/null +++ b/cli/azd/pkg/osutil/path.go @@ -0,0 +1,65 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package osutil + +import ( + "fmt" + "os" + "path/filepath" + "strings" +) + +// IsPathContained checks whether targetPath is contained within basePath after +// cleaning both paths. This is used to prevent path traversal attacks where a +// resolved path might escape its intended directory. +// Backslashes in targetPath are normalized to the OS separator before checking, +// so paths like "..\..\malicious" are caught on all platforms. +func IsPathContained(basePath, targetPath string) bool { + // Normalize backslashes to OS separator so traversal via "..\.." is caught on Linux too. + normalizedTarget := strings.ReplaceAll(targetPath, "\\", string(os.PathSeparator)) + + cleanedBase := filepath.Clean(basePath) + string(os.PathSeparator) + cleanedTarget := filepath.Clean(normalizedTarget) + + // Also handle the case where target IS exactly the base directory + if cleanedTarget == filepath.Clean(basePath) { + return true + } + + return strings.HasPrefix(cleanedTarget, cleanedBase) +} + +// ResolveContainedPath attempts to resolve filePath within one of the given root directories. +// It returns the first absolute path that exists and is contained within its root. +// If the path resolves outside all roots, it returns a path-traversal error. +// If the path is contained but does not exist in any root, it returns a not-found error. +func ResolveContainedPath(roots []string, filePath string) (string, error) { + pathBlocked := false + + for _, root := range roots { + absolutePath := filepath.Join(root, filePath) + + absolutePath, err := filepath.Abs(absolutePath) + if err != nil { + return "", err + } + + if !IsPathContained(root, absolutePath) { + pathBlocked = true + continue + } + + if _, err := os.Stat(absolutePath); err == nil { + return absolutePath, nil + } + } + + if pathBlocked { + return "", fmt.Errorf( + "path %q resolves outside all root directories; "+ + "use an absolute path or a path within the root directory", filePath) + } + + return "", fmt.Errorf("file '%s' was not found: %w", filePath, os.ErrNotExist) +} diff --git a/cli/azd/pkg/osutil/path_test.go b/cli/azd/pkg/osutil/path_test.go new file mode 100644 index 00000000000..72e7458cfe2 --- /dev/null +++ b/cli/azd/pkg/osutil/path_test.go @@ -0,0 +1,171 @@ +// Copyright (c) Microsoft Corporation. All rights reserved. +// Licensed under the MIT License. + +package osutil + +import ( + "os" + "path/filepath" + "testing" + + "github.com/stretchr/testify/require" +) + +func Test_ResolveContainedPath(t *testing.T) { + // Create temp directories to serve as roots + root1 := filepath.Join(os.TempDir(), "resolvetest_root1") + root2 := filepath.Join(os.TempDir(), "resolvetest_root2") + err := os.MkdirAll(filepath.Join(root1, "subdir"), 0755) + require.NoError(t, err) + err = os.MkdirAll(root2, 0755) + require.NoError(t, err) + + t.Cleanup(func() { + os.RemoveAll(root1) + os.RemoveAll(root2) + }) + + // Create test files + file1 := filepath.Join(root1, "file.txt") + err = os.WriteFile(file1, []byte("hello"), 0600) + require.NoError(t, err) + + file2 := filepath.Join(root2, "other.txt") + err = os.WriteFile(file2, []byte("world"), 0600) + require.NoError(t, err) + + subFile := filepath.Join(root1, "subdir", "nested.txt") + err = os.WriteFile(subFile, []byte("nested"), 0600) + require.NoError(t, err) + + tests := []struct { + name string + roots []string + filePath string + wantPath string // empty means expect error + wantErr bool + errSubstr string + }{ + { + name: "valid file in first root", + roots: []string{root1, root2}, + filePath: "file.txt", + wantPath: file1, + wantErr: false, + }, + { + name: "valid file in second root", + roots: []string{root1, root2}, + filePath: "other.txt", + wantPath: file2, + wantErr: false, + }, + { + name: "nested file in subdirectory", + roots: []string{root1}, + filePath: filepath.Join("subdir", "nested.txt"), + wantPath: subFile, + wantErr: false, + }, + { + name: "path traversal attempt", + roots: []string{root1}, + filePath: "../../../etc/passwd", + wantErr: true, + errSubstr: "resolves outside all root directories", + }, + { + name: "empty roots list", + roots: []string{}, + filePath: "file.txt", + wantErr: true, + errSubstr: "was not found", + }, + { + name: "file not found in any root", + roots: []string{root1, root2}, + filePath: "nonexistent.txt", + wantErr: true, + errSubstr: "was not found", + }, + { + name: "backslash path traversal", + roots: []string{root1}, + filePath: `..\..\evil`, + wantErr: true, + errSubstr: "resolves outside all root directories", + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result, err := ResolveContainedPath(tt.roots, tt.filePath) + if tt.wantErr { + require.Error(t, err) + if tt.errSubstr != "" { + require.Contains(t, err.Error(), tt.errSubstr) + } + require.Empty(t, result) + } else { + require.NoError(t, err) + // Clean both paths for comparison to handle OS-specific separators + require.Equal(t, filepath.Clean(tt.wantPath), filepath.Clean(result)) + } + }) + } +} + +func Test_IsPathContained(t *testing.T) { + base := filepath.Join(os.TempDir(), "testbase") + + tests := []struct { + name string + base string + target string + expected bool + }{ + { + name: "child path is contained", + base: base, + target: filepath.Join(base, "child", "file.txt"), + expected: true, + }, + { + name: "exact base path is contained", + base: base, + target: base, + expected: true, + }, + { + name: "traversal escapes base", + base: base, + target: filepath.Join(base, "..", "escape"), + expected: false, + }, + { + name: "sibling directory not contained", + base: base, + target: base + "-sibling", + expected: false, + }, + { + name: "double traversal escapes base", + base: base, + target: filepath.Join(base, "sub", "..", "..", "escape"), + expected: false, + }, + { + name: "backslash traversal escapes base on all platforms", + base: base, + target: base + `\..\..\escape`, + expected: false, + }, + } + + for _, tt := range tests { + t.Run(tt.name, func(t *testing.T) { + result := IsPathContained(tt.base, tt.target) + require.Equal(t, tt.expected, result) + }) + } +} diff --git a/cli/azd/pkg/project/container_helper.go b/cli/azd/pkg/project/container_helper.go index b1c19a20a29..7d4f37d5f07 100644 --- a/cli/azd/pkg/project/container_helper.go +++ b/cli/azd/pkg/project/container_helper.go @@ -839,7 +839,7 @@ func (ch *ContainerHelper) runRemoteBuild( buildRequest := &armcontainerregistry.DockerBuildRequest{ SourceLocation: source.RelativePath, - DockerFilePath: new(dockerPath), + DockerFilePath: &dockerPath, IsPushEnabled: new(true), ImageNames: []*string{new(imageName)}, Platform: &armcontainerregistry.PlatformProperties{ diff --git a/cli/azd/pkg/templates/file_source.go b/cli/azd/pkg/templates/file_source.go index 26989df64a7..d4a6ef7b871 100644 --- a/cli/azd/pkg/templates/file_source.go +++ b/cli/azd/pkg/templates/file_source.go @@ -9,6 +9,7 @@ import ( "path/filepath" "github.com/azure/azure-dev/cli/azd/pkg/config" + "github.com/azure/azure-dev/cli/azd/pkg/osutil" ) // newFileTemplateSource creates a new template source from a file. @@ -48,20 +49,5 @@ func getAbsolutePath(filePath string) (string, error) { roots = append(roots, cwd) roots = append(roots, azdConfigPath) - for _, root := range roots { - // Join the root directory with the relative path - absolutePath := filepath.Join(root, filePath) - - // Normalize the path to handle any ".." or "." segments - absolutePath, err = filepath.Abs(absolutePath) - if err != nil { - return "", err - } - - if _, err := os.Stat(absolutePath); err == nil { - return absolutePath, nil - } - } - - return "", fmt.Errorf("file '%s' was not found, %w", filePath, os.ErrNotExist) + return osutil.ResolveContainedPath(roots, filePath) } diff --git a/cli/azd/pkg/tools/bicep/bicep.go b/cli/azd/pkg/tools/bicep/bicep.go index 354cfa41052..cd7b6376e80 100644 --- a/cli/azd/pkg/tools/bicep/bicep.go +++ b/cli/azd/pkg/tools/bicep/bicep.go @@ -14,7 +14,6 @@ import ( "path/filepath" "runtime" "strings" - "sync" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/azure/azure-dev/cli/azd/pkg/config" @@ -32,7 +31,7 @@ var Version semver.Version = semver.MustParse("0.41.2") // Cli is a wrapper around the bicep CLI. // The CLI automatically ensures bicep is installed before executing commands. // -// Concurrency notes: The sync.Once is per-instance, not global. In normal operation, the IoC +// Concurrency notes: The sync.Mutex is per-instance, not global. In normal operation, the IoC // container registers Cli as a singleton, so one shared instance is used. However, some code paths // (e.g., service_target_containerapp.go) create new instances inline. If multiple instances race to // install bicep concurrently, this is safe because downloadBicep uses atomic file operations @@ -43,8 +42,7 @@ type Cli struct { console input.Console transporter policy.Transporter - installOnce sync.Once - installErr error + installInit osutil.LazyRetryInit } // NewCli creates a new Bicep CLI wrapper. @@ -69,10 +67,9 @@ func newCliWithTransporter( // ensureInstalledOnce checks if bicep is available and downloads/upgrades if needed. // This is safe to call multiple times; installation only happens once. func (cli *Cli) ensureInstalledOnce(ctx context.Context) error { - cli.installOnce.Do(func() { - cli.installErr = cli.ensureInstalled(ctx) + return cli.installInit.Do(func() error { + return cli.ensureInstalled(ctx) }) - return cli.installErr } func (cli *Cli) ensureInstalled(ctx context.Context) error { diff --git a/cli/azd/pkg/tools/github/github.go b/cli/azd/pkg/tools/github/github.go index 03f0e6a5eb5..7894b35397f 100644 --- a/cli/azd/pkg/tools/github/github.go +++ b/cli/azd/pkg/tools/github/github.go @@ -19,7 +19,6 @@ import ( "regexp" "runtime" "strings" - "sync" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/azure/azure-dev/cli/azd/pkg/config" @@ -96,18 +95,16 @@ type Cli struct { extractImplementation extractGitHubCliFromFileImplementation path string - installOnce sync.Once - installErr error + installInit osutil.LazyRetryInit } // EnsureInstalled checks if GitHub CLI is available and downloads/upgrades if needed. // This is safe to call multiple times; installation only happens once. // Should be called with a request-scoped context before first use. func (cli *Cli) EnsureInstalled(ctx context.Context) error { - cli.installOnce.Do(func() { - cli.installErr = cli.ensureInstalled(ctx) + return cli.installInit.Do(func() error { + return cli.ensureInstalled(ctx) }) - return cli.installErr } func (cli *Cli) ensureInstalled(ctx context.Context) error { diff --git a/cli/azd/pkg/tools/maven/maven.go b/cli/azd/pkg/tools/maven/maven.go index 00840926fa9..3e0a84b61ca 100644 --- a/cli/azd/pkg/tools/maven/maven.go +++ b/cli/azd/pkg/tools/maven/maven.go @@ -13,11 +13,11 @@ import ( "path/filepath" "regexp" "strings" - "sync" osexec "os/exec" "github.com/azure/azure-dev/cli/azd/pkg/exec" + "github.com/azure/azure-dev/cli/azd/pkg/osutil" "github.com/azure/azure-dev/cli/azd/pkg/tools" ) @@ -30,8 +30,7 @@ type Cli struct { // Lazily initialized. Access through mvnCmd. mvnCmdStr string - mvnCmdOnce sync.Once - mvnCmdErr error + mvnCmdInit osutil.LazyRetryInit } func (m *Cli) Name() string { @@ -61,17 +60,17 @@ func (m *Cli) SetPath(projectPath string, rootProjectPath string) { } func (m *Cli) mvnCmd() (string, error) { - m.mvnCmdOnce.Do(func() { + err := m.mvnCmdInit.Do(func() error { mvnCmd, err := getMavenPath(m.projectPath, m.rootProjectPath) if err != nil { - m.mvnCmdErr = err - } else { - m.mvnCmdStr = mvnCmd + return err } - }) - if m.mvnCmdErr != nil { - return "", m.mvnCmdErr + m.mvnCmdStr = mvnCmd + return nil + }) + if err != nil { + return "", err } return m.mvnCmdStr, nil diff --git a/cli/azd/pkg/tools/pack/pack.go b/cli/azd/pkg/tools/pack/pack.go index b71cfcbdd89..e19528c8a1f 100644 --- a/cli/azd/pkg/tools/pack/pack.go +++ b/cli/azd/pkg/tools/pack/pack.go @@ -19,7 +19,6 @@ import ( "runtime" "strconv" "strings" - "sync" "github.com/Azure/azure-sdk-for-go/sdk/azcore/policy" "github.com/azure/azure-dev/cli/azd/pkg/config" @@ -115,18 +114,16 @@ type Cli struct { transporter policy.Transporter extract func(string, string) (string, error) - installOnce sync.Once - installErr error + installInit osutil.LazyRetryInit } // EnsureInstalled checks if pack CLI is available and downloads/upgrades if needed. // This is safe to call multiple times; installation only happens once. // Should be called with a request-scoped context before first use. func (cli *Cli) EnsureInstalled(ctx context.Context) error { - cli.installOnce.Do(func() { - cli.installErr = cli.ensureInstalled(ctx) + return cli.installInit.Do(func() error { + return cli.ensureInstalled(ctx) }) - return cli.installErr } func (cli *Cli) ensureInstalled(ctx context.Context) error { diff --git a/cli/azd/test/recording/sanitize.go b/cli/azd/test/recording/sanitize.go index 8e6c849537c..da58b4cb9d5 100644 --- a/cli/azd/test/recording/sanitize.go +++ b/cli/azd/test/recording/sanitize.go @@ -93,7 +93,7 @@ func sanitizeContainerRegistryListBuildSourceUploadUrl(i *cassette.Interaction) if err != nil { return fmt.Errorf("sanitizing UploadURL: %w", err) } - body.UploadURL = new(s) + body.UploadURL = &s sanitized, err := json.Marshal(body) if err != nil { @@ -122,7 +122,7 @@ func sanitizeContainerRegistryListLogSasUrl(i *cassette.Interaction) error { if err != nil { return fmt.Errorf("sanitizing UploadURL: %w", err) } - body.LogLink = new(s) + body.LogLink = &s sanitized, err := json.Marshal(body) if err != nil { From 5a994ecb79bea14e2a8bf681611a322e33634607 Mon Sep 17 00:00:00 2001 From: Jon Gallant <2163001+jongio@users.noreply.github.com> Date: Tue, 17 Mar 2026 15:51:26 -0700 Subject: [PATCH 2/2] =?UTF-8?q?fix:=20address=20review=20feedback=20?= =?UTF-8?q?=E2=80=94=20root=20path=20handling,=20test=20isolation,=20stale?= =?UTF-8?q?=20docstrings?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit - Fix IsPathContained double-separator on root paths (e.g., '/' or 'C:\') - Add case-insensitive path comparison for Windows filesystem compatibility - Switch Test_ResolveContainedPath to use t.TempDir() for test isolation - Add root path containment test cases - Update EnsureInstalled docstrings to reflect LazyRetryInit retry semantics Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com> --- cli/azd/pkg/osutil/path.go | 35 ++++++++++++++++++++++++++---- cli/azd/pkg/osutil/path_test.go | 27 ++++++++++++++--------- cli/azd/pkg/tools/bicep/bicep.go | 2 +- cli/azd/pkg/tools/github/github.go | 2 +- cli/azd/pkg/tools/pack/pack.go | 2 +- 5 files changed, 51 insertions(+), 17 deletions(-) diff --git a/cli/azd/pkg/osutil/path.go b/cli/azd/pkg/osutil/path.go index 5b1ac4c229a..a171e16c2c4 100644 --- a/cli/azd/pkg/osutil/path.go +++ b/cli/azd/pkg/osutil/path.go @@ -7,6 +7,7 @@ import ( "fmt" "os" "path/filepath" + "runtime" "strings" ) @@ -19,15 +20,41 @@ func IsPathContained(basePath, targetPath string) bool { // Normalize backslashes to OS separator so traversal via "..\.." is caught on Linux too. normalizedTarget := strings.ReplaceAll(targetPath, "\\", string(os.PathSeparator)) - cleanedBase := filepath.Clean(basePath) + string(os.PathSeparator) + cleanedBase := filepath.Clean(basePath) cleanedTarget := filepath.Clean(normalizedTarget) - // Also handle the case where target IS exactly the base directory - if cleanedTarget == filepath.Clean(basePath) { + // Handle exact match (target IS the base directory). + if pathEqual(cleanedTarget, cleanedBase) { return true } - return strings.HasPrefix(cleanedTarget, cleanedBase) + // Build prefix for containment check. + // filepath.Clean on root paths (e.g., "/" or "C:\") retains the trailing separator, + // so only add one if not already present to avoid a "//"-prefix false negative. + basePrefix := cleanedBase + if !strings.HasSuffix(basePrefix, string(os.PathSeparator)) { + basePrefix += string(os.PathSeparator) + } + + return pathHasPrefix(cleanedTarget, basePrefix) +} + +// pathEqual compares two cleaned file paths. On Windows, paths are compared +// case-insensitively to match the filesystem's behavior. +func pathEqual(a, b string) bool { + if runtime.GOOS == "windows" { + return strings.EqualFold(a, b) + } + return a == b +} + +// pathHasPrefix checks if target starts with prefix. On Windows, the comparison +// is case-insensitive to match the filesystem's behavior. +func pathHasPrefix(target, prefix string) bool { + if runtime.GOOS == "windows" { + return strings.HasPrefix(strings.ToLower(target), strings.ToLower(prefix)) + } + return strings.HasPrefix(target, prefix) } // ResolveContainedPath attempts to resolve filePath within one of the given root directories. diff --git a/cli/azd/pkg/osutil/path_test.go b/cli/azd/pkg/osutil/path_test.go index 72e7458cfe2..265a7fa19c0 100644 --- a/cli/azd/pkg/osutil/path_test.go +++ b/cli/azd/pkg/osutil/path_test.go @@ -12,18 +12,11 @@ import ( ) func Test_ResolveContainedPath(t *testing.T) { - // Create temp directories to serve as roots - root1 := filepath.Join(os.TempDir(), "resolvetest_root1") - root2 := filepath.Join(os.TempDir(), "resolvetest_root2") + // Use t.TempDir() for unique per-test roots that auto-cleanup + root1 := t.TempDir() + root2 := t.TempDir() err := os.MkdirAll(filepath.Join(root1, "subdir"), 0755) require.NoError(t, err) - err = os.MkdirAll(root2, 0755) - require.NoError(t, err) - - t.Cleanup(func() { - os.RemoveAll(root1) - os.RemoveAll(root2) - }) // Create test files file1 := filepath.Join(root1, "file.txt") @@ -117,6 +110,8 @@ func Test_ResolveContainedPath(t *testing.T) { func Test_IsPathContained(t *testing.T) { base := filepath.Join(os.TempDir(), "testbase") + // Construct OS-appropriate filesystem root for root path tests + root := filepath.VolumeName(os.TempDir()) + string(os.PathSeparator) tests := []struct { name string @@ -160,6 +155,18 @@ func Test_IsPathContained(t *testing.T) { target: base + `\..\..\escape`, expected: false, }, + { + name: "root path contains child", + base: root, + target: filepath.Join(root, "somechild"), + expected: true, + }, + { + name: "root path exact match", + base: root, + target: root, + expected: true, + }, } for _, tt := range tests { diff --git a/cli/azd/pkg/tools/bicep/bicep.go b/cli/azd/pkg/tools/bicep/bicep.go index cd7b6376e80..4778712206c 100644 --- a/cli/azd/pkg/tools/bicep/bicep.go +++ b/cli/azd/pkg/tools/bicep/bicep.go @@ -65,7 +65,7 @@ func newCliWithTransporter( } // ensureInstalledOnce checks if bicep is available and downloads/upgrades if needed. -// This is safe to call multiple times; installation only happens once. +// This is safe to call multiple times; successful installation is cached and failed attempts are retried. func (cli *Cli) ensureInstalledOnce(ctx context.Context) error { return cli.installInit.Do(func() error { return cli.ensureInstalled(ctx) diff --git a/cli/azd/pkg/tools/github/github.go b/cli/azd/pkg/tools/github/github.go index 7894b35397f..95934537f40 100644 --- a/cli/azd/pkg/tools/github/github.go +++ b/cli/azd/pkg/tools/github/github.go @@ -99,7 +99,7 @@ type Cli struct { } // EnsureInstalled checks if GitHub CLI is available and downloads/upgrades if needed. -// This is safe to call multiple times; installation only happens once. +// This is safe to call multiple times; successful installation is cached and failed attempts are retried. // Should be called with a request-scoped context before first use. func (cli *Cli) EnsureInstalled(ctx context.Context) error { return cli.installInit.Do(func() error { diff --git a/cli/azd/pkg/tools/pack/pack.go b/cli/azd/pkg/tools/pack/pack.go index e19528c8a1f..2f4f98f5912 100644 --- a/cli/azd/pkg/tools/pack/pack.go +++ b/cli/azd/pkg/tools/pack/pack.go @@ -118,7 +118,7 @@ type Cli struct { } // EnsureInstalled checks if pack CLI is available and downloads/upgrades if needed. -// This is safe to call multiple times; installation only happens once. +// This is safe to call multiple times; successful installation is cached and failed attempts are retried. // Should be called with a request-scoped context before first use. func (cli *Cli) EnsureInstalled(ctx context.Context) error { return cli.installInit.Do(func() error {