Fix environment variable leak and flag propagation for extensions#7314
Fix environment variable leak and flag propagation for extensions#7314
Conversation
There was a problem hiding this comment.
Pull request overview
This PR fixes global flag propagation (notably -e/--environment, --debug, --cwd) to extension commands that run with DisableFlagParsing: true, and standardizes invalid environment-name errors.
Changes:
- Introduces
GlobalCommandOptions.EnvironmentNameand parses-e/--environmentearly viaParseGlobalFlags(). - Updates extension invocation and DI env resolver to read from pre-parsed
globalOptionsrather thancmd.Flags(). - Centralizes invalid environment-name error formatting and updates help/usage snapshots to include the new global flag.
Reviewed changes
Copilot reviewed 71 out of 71 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| cli/azd/pkg/environment/manager.go | Replaces ad-hoc invalid env name messaging with shared InvalidEnvironmentNameError() |
| cli/azd/pkg/environment/environment.go | Adds shared exported invalid env name error helper |
| cli/azd/internal/global_command_options.go | Adds EnvironmentName to carry pre-parsed -e/--environment value |
| cli/azd/cmd/extensions.go | Propagates debug/cwd/env/no-prompt to extensions via globalOptions |
| cli/azd/cmd/container.go | DI resolver for EnvFlag falls back to globalOptions.EnvironmentName |
| cli/azd/cmd/auto_install.go | Adds global -e/--environment and validates it in ParseGlobalFlags() |
| cli/azd/cmd/auto_install_test.go | Adds tests for parsing/validating -e/--environment |
| cli/azd/cmd/testdata/TestFigSpec.ts | Moves --environment/-e to persistent options; removes per-command env options in a few places |
| cli/azd/cmd/testdata/TestUsage-azd.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-x.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-version.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-template.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-template-source.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-template-source-remove.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-template-source-list.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-template-source-add.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-template-show.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-template-list.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-pipeline.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-mcp.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-mcp-start.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-infra.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-hooks.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-extension.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-extension-upgrade.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-extension-uninstall.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-extension-source.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-extension-source-validate.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-extension-source-remove.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-extension-source-list.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-extension-source-add.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-extension-show.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-extension-list.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-extension-install.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-env.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-env-select.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-env-new.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-env-list.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-env-config.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-demo.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-copilot.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-copilot-consent.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-copilot-consent-revoke.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-copilot-consent-list.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-copilot-consent-grant.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-config.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-config-unset.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-config-show.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-config-set.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-config-reset.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-config-options.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-config-list-alpha.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-config-get.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-concurx.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-completion.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-completion-zsh.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-completion-powershell.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-completion-fish.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-completion-fig.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-completion-bash.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-coding-agent.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-auth.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-auth-status.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-auth-logout.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-auth-login.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-appservice.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-ai.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-ai-models.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-ai-finetuning.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-ai-agent.snap | Updates help snapshot to include -e, --environment |
| cli/azd/cmd/testdata/TestUsage-azd-add.snap | Updates help snapshot to include -e, --environment |
Comments suppressed due to low confidence (2)
cli/azd/internal/global_command_options.go:1
- This comment says
EnvironmentNameis empty when the passed-evalue is not a valid environment name (e.g., extensions reuse-efor URLs), butParseGlobalFlags()now returns an error for invalid values. Update the comment to match the new strict-validation behavior (or relax validation if the intent is still to allow extensions to reuse-e).
cli/azd/pkg/environment/environment.go:1 - The standardized error message hard-codes the allowed character set as 'only alphanumeric characters and hyphens'. In this PR,
TestParseGlobalFlags_EnvironmentNametreats a name containing a dot (my-env.v2) as valid. Either adjust the test expectations/validation to disallow dots, or update the error message to accurately describe whatIsValidEnvironmentNamepermits so users get correct guidance.
spboyer
left a comment
There was a problem hiding this comment.
Reviewed the core fix (switching extensionAction from cmd.Flags() to globalOptions for DisableFlagParsing extensions), IoC plumbing, AZD_DISABLE_AGENT_DETECT kill switch, InvalidEnvironmentNameError refactor, and require.Fail -> require.Failf fix. No issues found.
14fb280 to
ff5d8a2
Compare
ff5d8a2 to
8060843
Compare
wbreza
left a comment
There was a problem hiding this comment.
Code Review — PR #7314
Fix environment variable leak and flag propagation for extensions by @jongio
Findings
| Priority | Count |
|---|---|
| Low | 4 |
| Total | 4 |
Backwards Compatibility Assessment
| Scenario | Status |
|---|---|
Strict -e validation |
✅ Safe (with #7313 merged first) |
EnvFlag DI fallback chain |
✅ Correct precedence |
extensions.go using globalOptions |
✅ Correct fix for DisableFlagParsing |
-e on non-env commands |
✅ Benign — silently accepted, unused |
| Empty/default env names | ✅ Properly guarded |
| Merge order dependency |
Findings Detail
1. [Low] Doc comment contradicts actual behavior
File: global_command_options.go — EnvironmentName field
The comment says it's "empty when the value was not a valid environment name" — but ParseGlobalFlags() actually returns a hard error for invalid names. No code path silently leaves it empty for invalid input. Consider: "It is empty when the user did not pass -e. If the value fails validation, ParseGlobalFlags returns an error."
2. [Low] Error message understates allowed characters
File: environment.go — InvalidEnvironmentNameError
Says "only alphanumeric characters and hyphens" but the regex also allows _, ., and (). Now that this is a shared public function called in 3 places, consider listing all valid characters accurately.
3. [Low] Missing max-length boundary test cases
File: auto_install_test.go
The regex enforces {1,64} and there's a EnvironmentNameMaxLength = 64 constant, but neither valid nor invalid test sets exercise the boundary. Consider adding a 64-char valid name and a 65-char invalid name.
4. [Low] Unrelated detect_confirm_apphost.go change
Modifies the init confirmation flow (removes warningMessage, adds magenta "Azure Container Apps" text) — unrelated to the env leak fix. Same stray commit appears in #7313.
What's Done Well
- Root cause analysis is excellent — the insight that
DisableFlagParsing=truemakes allcmd.Flags().Get*()return zero values is correct and non-obvious - Clean separation of concerns —
ParseGlobalFlags → globalOptions → DI fallbackchain is well-layered - Thorough tests — 11 new subtests covering valid names, invalid names (URLs, colons, slashes, special chars, spaces), flag combinations
AZD_DISABLE_AGENT_DETECTenv var — excellent addition for test isolationInvalidEnvironmentNameErrorconsolidation — shared public function eliminates message drift across 3 call sitesrequire.Fail→require.Failffix — catches a real bug where format args were silently ignored
Overall: Approve — the core fix is architecturally sound and backwards-compatible. Findings are all low-priority improvements.
Review performed with GitHub Copilot CLI
wbreza
left a comment
There was a problem hiding this comment.
Follow-up: Changing review state to COMMENT
My previous review was posted as APPROVE but should have been COMMENT — posting this to correct the review state. Same findings apply:
Findings
| Priority | Count |
|---|---|
| Low | 4 |
| Total | 4 |
1. [Low] Doc comment contradicts actual behavior
File: global_command_options.go — EnvironmentName field
The comment says it's "empty when the value was not a valid environment name" — but ParseGlobalFlags() actually returns a hard error for invalid names. Consider: "It is empty when the user did not pass -e. If the value fails validation, ParseGlobalFlags returns an error."
2. [Low] Error message understates allowed characters
File: environment.go — InvalidEnvironmentNameError
Says "only alphanumeric characters and hyphens" but the regex also allows _, ., and (). Now that this is a shared public function called in 3 places, consider listing all valid characters accurately.
3. [Low] Missing max-length boundary test cases
File: auto_install_test.go
The regex enforces {1,64} and there's a EnvironmentNameMaxLength = 64 constant, but neither valid nor invalid test sets exercise the boundary. Consider adding a 64-char valid name and a 65-char invalid name.
4. [Low] Unrelated detect_confirm_apphost.go change
Modifies the init confirmation flow (removes warningMessage, adds magenta "Azure Container Apps" text) — unrelated to the env leak fix.
Backwards Compatibility: ✅ Safe
| Scenario | Status |
|---|---|
Strict -e validation |
✅ Safe (with #7313 merged first) |
EnvFlag DI fallback chain |
✅ Correct precedence |
extensions.go using globalOptions |
✅ Correct fix for DisableFlagParsing |
| Merge order dependency |
What's Done Well
- Excellent root cause analysis —
DisableFlagParsing=truemakescmd.Flags().Get*()return zero values - Clean
ParseGlobalFlags → globalOptions → DI fallbacklayering - 11 new subtests with good coverage
AZD_DISABLE_AGENT_DETECTenv var for test isolationInvalidEnvironmentNameErrorconsolidation across 3 call sitesrequire.Fail→require.Failfbugfix
Review performed with GitHub Copilot CLI
|
Updated approach: strict → lenient validation for -e/--environment Changed Why this is safe: Valid azd environment names match What this means for PR #7313: It is no longer a hard prerequisite. Extensions reusing Test results: All 11 subtests pass (6 valid env names + 5 invalid values). Full |
4549543 to
c417f41
Compare
1d3f407 to
95884ab
Compare
wbreza
left a comment
There was a problem hiding this comment.
Code Review — Environment Variable Leak & Flag Propagation Fix
All 4 prior review findings from the previous review cycle have been verified as resolved ✅
✅ What Looks Good
- Excellent test design — table-driven tests cover the full flag interleaving matrix including the subtle pflag last-value-wins edge case, with explicit regression references to reverted PR #7274
- Elegant lenient validation — silently ignoring invalid environment names cleanly resolves the extension -e conflict without breaking extension compatibility
- Centralized InvalidEnvironmentNameError — eliminates duplication and follows Go conventions
- DisableAgentDetectEnvVar test isolation — ensures functional tests work reliably in AI agent environments
Findings
| Priority | Count |
|---|---|
| Low | 3 |
[Low] Flag name literal "environment" in CreateGlobalFlagSet is cross-referenced via constant internal.EnvironmentNameFlagName in container.go — consider using the constant in both places to prevent drift.
[Low] ParseGlobalFlags doesn't check AZURE_ENV_NAME env var for extension commands (pre-existing gap, not a regression — extensions inherit the env var from the parent process).
[Low] Silent override of valid --environment by later invalid -e due to pflag last-value-wins is well-documented in tests but could benefit from debug-level logging.
Overall: All prior feedback resolved. Clean, well-tested fix.
Extension commands use DisableFlagParsing, so cobra never parses global flags like -e/--environment, --debug, or --cwd. This caused two problems: 1. The DI-resolved environment always loaded the default instead of the one specified with -e, leaking wrong env vars into extension processes and never setting AZD_ENVIRONMENT (#7034). 2. --debug and --cwd were also not propagated to extensions because extensions.go read them from cmd.Flags() which returns defaults. Fix by: - Adding -e/--environment to ParseGlobalFlags() with lenient validation: valid env names are accepted, non-env values (like URLs that extensions pass via -e) are silently skipped so extensions still work. - Adding EnvironmentName to GlobalCommandOptions so the pre-parsed value is available to the DI container and extension runner. - Updating container.go EnvFlag resolver to fall back to globalOptions when cmd.Flags() returns empty (extension commands). - Updating extensions.go to use globalOptions for all InvokeOptions fields (debug, cwd, environment, no-prompt) instead of cmd.Flags(). Closes #7034 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Agent detection (agentdetect package) walks the parent process tree and auto-enables --no-prompt when it finds an AI coding agent. In CI and local dev under Copilot CLI, this causes functional tests to fail because piped stdin is ignored when no-prompt is active. Changes: - detect.go: Early return from detectAgent() when AZD_DISABLE_AGENT_DETECT is set, suppressing both env var and parent process detection - cli.go: Set AZD_DISABLE_AGENT_DETECT=1 on all child azd processes in RunCommandWithStdIn(), with nil-Env safety (nil means inherit-all in Go) - detect_test.go: Test that AZD_DISABLE_AGENT_DETECT suppresses detection - env_test.go: Fix require.Fail -> require.Failf format string bug Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The gosec linter flags os.LookupEnv values as tainted input for log injection (G706). Remove the env var value from the log message since only the presence of the env var matters, not its value. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Workflow steps that specify their own -e/--environment flag (e.g. 'azd: env set KEY VALUE -e env1') were getting the parent command's --environment appended via extractGlobalArgs(), causing the parent's value to override the step's explicit value. The environment flag is now excluded from extractGlobalArgs() since environment propagation to workflow steps is already handled by the globalOptions DI fallback in the EnvFlag resolver. Fixes Test_CLI_Up_EnvironmentFlags. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Change ParseGlobalFlags to silently ignore invalid environment names
instead of returning an error. Valid environment names match the regex
[a-zA-Z0-9-()_.]{1,64} which can never match URLs containing ://, so
there is natural disambiguation between azd env names and extension
flags that reuse -e for other purposes (e.g., --project-endpoint).
This fixes the environment variable leak (azd env name not propagating
to workflow steps) while preserving backward compatibility with all
extensions — including third-party ones that use -e for their own flags.
PR 7313 (migrating first-party extensions off -e) is no longer a hard
prerequisite since invalid values are silently skipped, not rejected.
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Remove stale --host option from ai agents init that was incorrectly kept during rebase conflict resolution. The current azure.ai.agents extension no longer exposes this option. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Add TestParseGlobalFlags_ExtensionCompatibility that verifies real extension scenarios (azure.ai.models, azure.ai.finetune) using -e for --project-endpoint URLs are not broken by global -e/--environment flag parsing. Covers: URL endpoints, --project-endpoint passthrough, valid env before extension args, mixed global flags, and pflag last-value-wins behavior. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Break long args slices across multiple lines to comply with the 125-char lll linter rule enforced by golangci-lint. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
- Replace string literal 'environment' with internal.EnvironmentNameFlagName constant in CreateGlobalFlagSet and ParseGlobalFlags to prevent drift - Add debug-level log when an invalid environment name is silently ignored, making the lenient skip observable when --debug is enabled Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
95ca27f to
95c9fc1
Compare
Azure Dev CLI Install InstructionsInstall scriptsMacOS/Linux
bash: pwsh: WindowsPowerShell install MSI install Standalone Binary
MSI
Documentationlearn.microsoft.com documentationtitle: Azure Developer CLI reference
|
Summary
This is a redo of #7035 (which was reverted by #7274). It fixes two problems:
-e/--environmentvalue because extension commands useDisableFlagParsing: true, so cobra never parsed the flag. The DI resolver always fell back to the default environment.--debugand--cwdpropagation to extensions, since it changedextensions.goback to usingcmd.Flags()which returns defaults for extension commands.What changed
global_command_options.go: AddedEnvironmentNamefield andEnvironmentNameFlagNameconstantauto_install.go: Added-e/--environmenttoCreateGlobalFlagSet()using theEnvironmentNameFlagNameconstant (prevents drift withcontainer.go). Lenient validation inParseGlobalFlags()— valid env names are accepted, invalid values (e.g., URLs) are silently ignored with debug-level logging when--debugis enabledcontainer.go: UpdatedEnvFlagDI resolver to fall back toglobalOptions.EnvironmentNameextensions.go: UsesglobalOptions(populated before cobra) for ALLInvokeOptionsfields (debug, cwd, environment, no-prompt)environment.go: Added exportedInvalidEnvironmentNameError()for shared validation across call sitesmanager.go: Replaced 3 inconsistent error message formats with the sharedInvalidEnvironmentNameError()-e, --environmentflag in help textauto_install_test.go: 37 subtests across 5 test functions (6 valid env name, 5 invalid env name, 6 extension compatibility, agent detection, and core flag tests)Key difference from #7035
PR #7035 added strict
-evalidation which broke extensions that reused-efor URLs (e.g.,azd ai models custom create -e https://endpoint.com). The errorenvironment name 'https://...' is invalidkilled the entire command.This PR uses lenient validation instead: valid environment names (matching
[a-zA-Z0-9-()_.]{1,64}) are accepted, and anything else is silently ignored. Since valid env names can never contain://,/, or:, there is natural disambiguation — no ambiguity between env names and URLs is possible.This means:
azd deploy -e myenv—myenvcaptured as environment nameazd ai models custom create -e https://...— URL silently ignored, extension receives-ein its raw args and parses it normally-efor any non-env-name value — works unchangedHow
globalOptionssolves itParseGlobalFlags()runs before cobra, manually parsing the rawos.Args. For extension commands (DisableFlagParsing: true), cobra skips all flag parsing, butglobalOptionsalready has the correct values. Both the DI resolver and extension invocation now read fromglobalOptionsinstead ofcmd.Flags().Closes #7034
Closes #7271
Co-authored-by: Copilot 223556219+Copilot@users.noreply.github.com