feat(azdext): Extension SDK improvements — integration, security, runtime, cleanup#7025
Draft
jongio wants to merge 1 commit intoAzure:mainfrom
Draft
feat(azdext): Extension SDK improvements — integration, security, runtime, cleanup#7025jongio wants to merge 1 commit intoAzure:mainfrom
jongio wants to merge 1 commit intoAzure:mainfrom
Conversation
jongio
requested review from
JeffreyCA,
hemarina,
rajeshkamal5050,
tg-msft,
therealjohn,
trangevi,
trrwilson,
vhvb1989 and
weikanglim
as code owners
This was referenced Mar 6, 2026
Contributor
There was a problem hiding this comment.
Pull request overview
This PR extends the azdext Extension SDK with additional integration, security, output/logging, and runtime helper utilities, and updates docs/extensions to use the improved entrypoints and patterns.
Changes:
- Add new
azdexthelper modules: TUI/interactive detection, tool discovery & PATH management, shell execution, atomic file operations, process utilities, structured output, structured logging, Key Vault secret resolution, SSRF guard, and input validation helpers. - Harden/adjust existing primitives (pager, scope detector, resilient HTTP client) and expand tests across the new surfaces.
- Update extensions and docs to use
azdext.Runand document new SDK helper guidance.
Reviewed changes
Copilot reviewed 47 out of 51 changed files in this pull request and generated 7 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/azd/pkg/azdext/tui.go | Adds interactive capability detection helpers. |
| cli/azd/pkg/azdext/tui_test.go | Unit tests for interactive detection and flags. |
| cli/azd/pkg/azdext/tooldiscovery.go | Adds tool lookup + PATH management utilities. |
| cli/azd/pkg/azdext/tooldiscovery_test.go | Tests for tool lookup and PATH helpers. |
| cli/azd/pkg/azdext/shell.go | Adds shell detection + shell command helpers + TTY detection. |
| cli/azd/pkg/azdext/shell_test.go | Tests for shell helpers and terminal detection. |
| cli/azd/pkg/azdext/security_validation.go | Adds validation helpers (service name, hostname, script name, container env detection). |
| cli/azd/pkg/azdext/security_validation_test.go | Tests for validation and container env detection. |
| cli/azd/pkg/azdext/ssrf_guard.go | Adds standalone SSRF guard utility (non-MCP usage). |
| cli/azd/pkg/azdext/ssrf_common.go | Common SSRF constants + IP classification helpers. |
| cli/azd/pkg/azdext/process.go | Adds cross-platform process utilities API surface. |
| cli/azd/pkg/azdext/process_windows.go | Windows implementation for process utilities. |
| cli/azd/pkg/azdext/process_darwin.go | macOS implementation for process utilities. |
| cli/azd/pkg/azdext/process_linux.go | Linux implementation for process utilities. |
| cli/azd/pkg/azdext/process_test.go | Tests for process utilities. |
| cli/azd/pkg/azdext/atomicfile.go | Adds atomic write/copy/backup + EnsureDir helpers. |
| cli/azd/pkg/azdext/atomicfile_test.go | Tests for atomic file helpers. |
| cli/azd/pkg/azdext/output.go | Adds format-aware output helper (default vs JSON) + table rendering. |
| cli/azd/pkg/azdext/output_test.go | Tests for output helper behavior. |
| cli/azd/pkg/azdext/logger.go | Adds slog-based logger + global setup helper. |
| cli/azd/pkg/azdext/logger_test.go | Tests for logger levels, structured output, and chaining. |
| cli/azd/pkg/azdext/keyvault_resolver.go | Adds Key Vault akvs:// secret reference resolver. |
| cli/azd/pkg/azdext/mcp_server_builder.go | Adds note about potential future package split for MCP library coupling. |
| cli/azd/pkg/azdext/extension_command.go | Adds note about potential future package split for Cobra coupling. |
| cli/azd/pkg/azdext/context.go | Deprecates NewContext in favor of Run/command helpers. |
| cli/azd/pkg/azdext/token_provider.go | Updates TokenProvider doc snippet (signature change). |
| cli/azd/pkg/azdext/scope_detector.go | Adjusts rule handling + error formatting behavior. |
| cli/azd/pkg/azdext/resilient_http_client.go | Adjusts retry defaults, jitter, and Retry-After parsing behavior. |
| cli/azd/pkg/azdext/resilient_http_client_test.go | Updates tests for resilient client behavior changes. |
| cli/azd/pkg/azdext/pagination.go | Adjusts pager init/validation and truncation semantics. |
| cli/azd/pkg/azdext/pagination_test.go | Updates tests for pager behavior changes. |
| cli/azd/pkg/azdext/run_test.go | Tests for error message/suggestion extraction helpers. |
| cli/azd/extensions/azure.appservice/main.go | Switches extension entrypoint to azdext.Run. |
| cli/azd/extensions/azure.appservice/internal/cmd/swap.go | Adds selection index bounds checks + slot name validation. |
| cli/azd/extensions/azure.appservice/go.mod | Updates module deps/replace comment for monorepo development. |
| cli/azd/extensions/azure.appservice/go.sum | Updates dependency checksums. |
| cli/azd/extensions/azure.ai.agents/internal/pkg/agents/agent_api/models.go | Formatting fix (go fmt). |
| cli/azd/extensions/azure.ai.agents/internal/cmd/show.go | Formatting fix (alignment). |
| cli/azd/extensions/azure.ai.agents/internal/cmd/monitor.go | Formatting fix (alignment). |
| cli/azd/docs/extensions/extension-framework.md | Updates docs to recommend azdext.Run and links related guides. |
| cli/azd/CHANGELOG.md | Changelog entries for new SDK helper surfaces and docs. |
| cli/azd/.vscode/cspell.yaml | Adds dictionary words for new SDK terminology. |
Comments suppressed due to low confidence (1)
cli/azd/pkg/azdext/pagination.go:234
validateNextLinkallowsnextLinkvalues with an empty scheme/host (e.g. relative URLs like/page2) and will accept them without SSRF host validation. This will later fail inhttp.NewRequest/Do() (missing scheme/host) and is also inconsistent with the comment that nextLink “must stay on the same host with HTTPS”. Consider requiring an absolute HTTPS URL (or explicitly resolving relative URLs against the current/origin URL and then enforcing same-host + https).
// validateNextLink checks that a nextLink URL is safe to follow.
// It rejects non-HTTPS schemes, URLs with embedded credentials, and
// URLs pointing to a different host than the original request (SSRF protection).
func (p *Pager[T]) validateNextLink(nextLink string) error {
u, err := url.Parse(nextLink)
if err != nil {
return fmt.Errorf("invalid nextLink URL: %w", err)
}
if u.Scheme != "" && u.Scheme != "https" {
return fmt.Errorf("nextLink must use HTTPS (got %q)", u.Scheme)
}
if u.User != nil {
return errors.New("nextLink must not contain user credentials")
}
host := strings.ToLower(u.Hostname())
if host != "" && p.originHost != "" && host != p.originHost {
return fmt.Errorf("nextLink host %q does not match origin host %q (possible SSRF)", host, p.originHost)
}
return nil
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
jongio
force-pushed
the
feature/ext-sdk-improvements
branch
2 times, most recently
from
9554fed to
fa5b1f7
Compare
Member
Author
|
@copilot please re-review the latest changes. All 6 agreed-upon review comments have been addressed. |
…time, cleanup Consolidated PR covering issues Azure#6945, Azure#6946, Azure#6947, Azure#6948, Azure#6949: - Key Vault resolver + config helpers (Azure#6945) - Output + structured logging helpers (Azure#6946) - Security validation + SSRF guard (Azure#6947) - Runtime utilities: shell, file, process, TUI, tool discovery (Azure#6948) - Post-Azure#6856 cleanup: context ownership, package boundaries (Azure#6949) Fixes Azure#6945 Fixes Azure#6946 Fixes Azure#6947 Fixes Azure#6948 Fixes Azure#6949
jongio
force-pushed
the
feature/ext-sdk-improvements
branch
from
fa5b1f7 to
6603758
Compare
Contributor
There was a problem hiding this comment.
Pull request overview
Copilot reviewed 47 out of 51 changed files in this pull request and generated 10 comments.
Comments suppressed due to low confidence (1)
cli/azd/pkg/azdext/pagination.go:193
- Pager.NextPage reads the response body with io.LimitReader(resp.Body, maxPageResponseSize) and then unmarshals it. If the server returns more than maxPageResponseSize bytes, the body will be silently truncated and the JSON decode error will be misleading. Consider restoring the prior pattern of reading maxPageResponseSize+1 and returning an explicit "response exceeds max page size" error when the limit is exceeded.
data, err := io.ReadAll(io.LimitReader(resp.Body, maxPageResponseSize))
if err != nil {
return nil, fmt.Errorf("azdext.Pager.NextPage: failed to read response: %w", err)
}
var page PageResponse[T]
if err := json.Unmarshal(data, &page); err != nil {
return nil, fmt.Errorf("azdext.Pager.NextPage: failed to decode response: %w", err)
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+34
to
+41
| type SSRFGuard struct { | ||
| mu sync.RWMutex | ||
| blockMetadata bool | ||
| blockPrivate bool | ||
| requireHTTPS bool | ||
| blockedCIDRs []*net.IPNet | ||
| blockedHosts map[string]bool | ||
| allowedHosts map[string]bool |
There was a problem hiding this comment.
SSRFGuard has a blockMetadata field that is set in BlockMetadataEndpoints but never read, and SSRFError.Reason’s doc lists "metadata_endpoint" even though the current implementation reports metadata blocks as "blocked_host". Either remove the unused field and adjust the documented reason values, or use blockMetadata to classify metadata endpoint violations distinctly.
Comment on lines
+270
to
275
| idx := int(prompt.GetValue()) | ||
| if idx < 0 || idx >= len(srcChoices) { | ||
| return fmt.Errorf("invalid source slot selection index: %d", idx) | ||
| } | ||
| srcSlot = srcChoices[idx].Value | ||
| } |
There was a problem hiding this comment.
This block appears to have lost gofmt indentation (the idx bounds check is not indented under the surrounding if). Please run gofmt on this file to ensure it builds cleanly and matches repo formatting expectations.
| // spaces to prevent log-forging attacks in stored error bodies. | ||
| func sanitizeControlChars(s string) string { | ||
| return strings.Map(func(r rune) rune { | ||
| if unicode.IsControl(r) && r != '\n' && r != '\t' { |
There was a problem hiding this comment.
sanitizeControlChars’ comment says it replaces control characters including LF and tabs, but the implementation explicitly preserves '\n' and '\t'. Please align the comment and behavior (and consider whether preserving newlines is acceptable if this field may be logged directly).
Suggested change
| if unicode.IsControl(r) && r != '\n' && r != '\t' { | |
| if unicode.IsControl(r) { |
Comment on lines
+59
to
+63
| // Detection strategy (in order): | ||
| // 1. SHELL environment variable (Unix) — most reliable on macOS/Linux. | ||
| // 2. ComSpec environment variable (Windows) — standard Windows shell path. | ||
| // 3. PSModulePath environment variable — indicates PowerShell on any platform. | ||
| // 4. Platform default fallback (sh on Unix, cmd on Windows). |
There was a problem hiding this comment.
DetectShell’s docstring lists the detection order as SHELL → ComSpec → PSModulePath, but the implementation checks PSModulePath before ComSpec. Please update either the comment or the ordering so they match.
Comment on lines
+19
to
+26
| // KeyVaultResolver resolves Azure Key Vault secret references for extension | ||
| // scenarios. It uses the extension's [TokenProvider] for authentication and | ||
| // the Azure SDK data-plane client for secret retrieval. | ||
| // | ||
| // Secret references use the akvs:// URI scheme: | ||
| // | ||
| // akvs://<subscription-id>/<vault-name>/<secret-name> | ||
| // |
There was a problem hiding this comment.
PR description mentions resolving Key Vault references like @Microsoft.KeyVault(...) and SecretUri/VaultName forms, but KeyVaultResolver/ParseSecretReference currently only supports the akvs://// format (via pkg/keyvault). Either expand parsing to cover the documented formats or update the PR description/docs to match the implemented behavior.
Comment on lines
+299
to
304
| idx := int(prompt.GetValue()) | ||
| if idx < 0 || idx >= len(dstChoices) { | ||
| return fmt.Errorf("invalid destination slot selection index: %d", idx) | ||
| } | ||
| dstSlot = dstChoices[idx].Value | ||
| } |
There was a problem hiding this comment.
This block appears to have lost gofmt indentation (the idx bounds check is not indented under the surrounding if). Please run gofmt on this file to ensure it builds cleanly and matches repo formatting expectations.
Comment on lines
216
to
224
| func (p *Pager[T]) validateNextLink(nextLink string) error { | ||
| u, err := url.Parse(nextLink) | ||
| if err != nil { | ||
| return fmt.Errorf("invalid nextLink URL: %w", err) | ||
| } | ||
|
|
||
| if u.Scheme != "https" { | ||
| if u.Scheme != "" && u.Scheme != "https" { | ||
| return fmt.Errorf("nextLink must use HTTPS (got %q)", u.Scheme) | ||
| } |
There was a problem hiding this comment.
validateNextLink currently allows an empty scheme (relative URLs) by only rejecting when u.Scheme != "" && u.Scheme != "https". A relative nextLink like "/page2" will pass validation but will then fail later when the client tries to create an HTTP request (missing protocol scheme). Either reject non-absolute nextLink values, or resolve relative nextLink values against the current page URL before storing p.nextURL.
Comment on lines
+16
to
+25
| // isProcessRunningOS checks if a process is running on Linux using signal 0. | ||
| func isProcessRunningOS(pid int) bool { | ||
| proc, err := os.FindProcess(pid) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| // Signal 0 does not send a signal but performs error checking. | ||
| // If the process exists, err is nil. If it doesn't, err is non-nil. | ||
| err = proc.Signal(syscall.Signal(0)) | ||
| return err == nil |
There was a problem hiding this comment.
On Unix, proc.Signal(0) can return EPERM when the process exists but the caller lacks permission to signal it. Returning err == nil will incorrectly report such processes as not running and contradicts the IsProcessRunning doc. Consider treating EPERM as "running" (e.g., return err == nil || errors.Is(err, syscall.EPERM)).
Comment on lines
+16
to
+24
| // isProcessRunningOS checks if a process is running on macOS using signal 0. | ||
| func isProcessRunningOS(pid int) bool { | ||
| proc, err := os.FindProcess(pid) | ||
| if err != nil { | ||
| return false | ||
| } | ||
| err = proc.Signal(syscall.Signal(0)) | ||
| return err == nil | ||
| } |
There was a problem hiding this comment.
On macOS, proc.Signal(0) can return EPERM when the process exists but the caller lacks permission to signal it. Returning err == nil will incorrectly report such processes as not running and contradicts the IsProcessRunning doc. Consider treating EPERM as "running" (e.g., return err == nil || errors.Is(err, syscall.EPERM)).
Comment on lines
+182
to
+198
| // IsInteractiveTerminal reports whether the given file descriptor is connected | ||
| // to an interactive terminal (TTY). | ||
| // | ||
| // Platform behavior: | ||
| // - Unix: Uses [os.File.Stat] to check for character device mode. | ||
| // - Windows: Uses [os.File.Stat] to check for character device mode. | ||
| // | ||
| // This function is safe to call with nil (returns false). | ||
| func IsInteractiveTerminal(f *os.File) bool { | ||
| if f == nil { | ||
| return false | ||
| } | ||
| fi, err := f.Stat() | ||
| if err != nil { | ||
| return false | ||
| } | ||
| return fi.Mode()&os.ModeCharDevice != 0 |
There was a problem hiding this comment.
IsInteractiveTerminal uses os.File.Stat() and checks os.ModeCharDevice. A character device is not necessarily a TTY (e.g., /dev/null is a char device but not interactive), so this can mis-detect interactivity and lead to prompts in non-interactive contexts. Consider using an isatty implementation (e.g., github.com/mattn/go-isatty or golang.org/x/term.IsTerminal) on f.Fd() instead of ModeCharDevice.
Member
Author
Architecture Note: KeyVault Resolver Design DecisionThe What IS shared (no duplication):
What is NOT shared (by design):
Core keyvault requires Azdext resolver accepts a simple If we consolidate in the future, the azdext pattern is the better foundation — simpler credential model, better error handling, and inherently testable. Core's |
Collaborator
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Extension SDK Improvements — Integration, Security, Runtime, Cleanup
Summary
Consolidated PR delivering the remaining Extension SDK and MCP Framework improvements from the Extension Framework Improvements initiative. These features were identified through a multi-model code review across 5 production extensions (azd-app, azd-exec, azd-copilot, azd-rest) and a shared library (azd-core), revealing ~2,500-4,000 lines of duplicated infrastructure across the ecosystem.
Changes
P1 Integration Helpers (#6945)
Key Vault Resolver
@Microsoft.KeyVault(SecretUri=...),@Microsoft.KeyVault(VaultName=...;SecretName=...),akvs://vault/...) embedded in environment variables. Without framework support, each extension imports the azd-core keyvault package which implements 3 regex patterns, thread-safe per-vault client caching, and credential management.StopOnKeyVaultErrorconfig flag and factory pattern wrapping azd-core's resolver.Config Helpers
~/.azd/config.jsonwith defaults. Each extension builds its own config loader.P2 Output & Structured Logging (#6946)
Output Helpers
Success(),Error(),Warning(),Table(),SetFormat()— used by azd-app and all other extensions. Without framework support, every extension depends on azd-core for consistent output.Structured Logger
SetupLogger(debug, structured)with JSON/text format selection and component-scoped loggers.P2 Security Validation & SSRF Guard (#6947)
Security Validation
ValidatePath()(L33 — detects.., resolves symlinks),ValidateServiceName()(L85 — DNS-safe regex),SanitizeScriptName()(L129 — blocks shell metacharacters), andIsContainerEnvironment()(L148 — detects Docker, Codespaces, K8s).SSRF Guard
blockedHeaders(L34),blockedHostsincluding169.254.169.254andfd00:ec2::254(L42), 7blockedCIDRsfor IPv4/IPv6 loopback, link-local, and RFC 1918 (L49-66), plusisBlockedIP()(L84) andisBlockedURL()with DNS resolution (L96-134).P3 Runtime Utilities (#6948)
Shell Detection & Execution
DetectShell(). azd-exec duplicates shell argument building in its MCP handler separately from its CLI handler.Atomic File Operations
AtomicWriteJSON()andAtomicWriteFile()— write to temp, sync, set permissions, rename.Process Management
os.FindProcessisn't reliable.gopsutilfor accurate detection across Windows/Linux/macOS.TUI Helpers
SetStdHandle()for CONIN$/CONOUT$, Unix/dev/ttybypass.Tool Discovery
FindToolInPath()with Windows .exe handling andGetInstallSuggestion()for 18+ tools.Cleanup (#6949)
Context Ownership & Package Boundaries
Related Work
Downstream Impact
WalktoWalkDir#22): Deprecated stopgap helpers in favor of SDK-provided onesTesting
Fixes #6945, Fixes #6946, Fixes #6947, Fixes #6948, Fixes #6949