Fix env var leak when running extension commands with -e flag#7035
Fix env var leak when running extension commands with -e flag#7035
Conversation
|
@copilot that looks like a lazy implementation, fix it with better design principles |
Redesigned in 79d1f16. Instead of scattered
No more Warning Firewall rules blocked me from connecting to one or more addresses (expand for details)I tried to connect to the following addresses, but was blocked by firewall rules:
If you need me to access, download, or install something from one of these locations, you can either:
|
999ac6d to
b0cd863
Compare
|
@copilot please re-review the latest changes. |
373f429 to
e72868c
Compare
wbreza
left a comment
There was a problem hiding this comment.
Code Review Summary
What Looks Good
- Follows established patterns: The fix mirrors how --cwd, --debug, and --no-prompt are handled via GlobalCommandOptions
- Clean separation of concerns: ParseGlobalFlags pre-parses, DI resolver falls back, extensionAction reads from single source
- Comprehensive unit tests for parsing: 11 table-driven test cases
- Good comments: Every change is well-commented explaining why, not just what
- No concurrency or performance concerns: Write-once/read-many pattern is safe
Findings Summary
| Priority | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 4 |
| Low | 3 |
| Total | 7 |
Overall Assessment: Comment. Solid fix that follows established patterns.
@jongio This is a well-structured fix and the ParseGlobalFlags tests are great. It would be worth adding some additional integration-style tests to validate the end-to-end behavior for workflow-based commands like azd up -e env, particularly around how the environment propagates through the DI resolver and into extension InvokeOptions.
5c12d71 to
108fe90
Compare
Co-authored-by: jongio <2163001+jongio@users.noreply.github.com>
Instead of scattered parseEnvFlagFromArgs + DisableFlagParsing checks, follow the established pattern for --cwd/--debug/--no-prompt: 1. Add EnvironmentName to GlobalCommandOptions 2. Parse -e/--environment in ParseGlobalFlags() before cobra runs 3. Use globalOptions.EnvironmentName as fallback in EnvFlag DI resolver 4. Use a.globalOptions directly in extensionAction.Run() for InvokeOptions Co-authored-by: jongio <2163001+jongio@users.noreply.github.com>
- ParseGlobalFlags now uses os.Getenv("AZURE_ENV_NAME") as default for -e flag,
matching EnvFlag.Bind() behavior so CI pipelines setting AZURE_ENV_NAME work
for extension commands
- Export EnvNameEnvVarName constant for cross-package reuse
- Add test cases for env var fallback, flag override, concatenated -e, multi -e
- Use exported constant in tests instead of hardcoded string
- Remove non-idiomatic "Warning:" prefix from error message
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
…tions Address review feedback from @wbreza and @vhvb1989: > 'The globalOptions fallback is placed before the context check (envFlagCtxKey). > The context mechanism exists for workflow composition. azd up uses > context.WithValue to propagate environment to sub-commands. Consider swapping > the order so context takes precedence over globalOptions.' Swapped the fallback chain in the EnvFlag DI resolver so the ordering is now: Cobra flag -> context value -> globalOptions This preserves the design intent where workflow composition (azd up) takes precedence over the pre-parsed CLI flag. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses @wbreza's review comment on extensions.go:258: > "The InvokeOptions construction and mapping is an important > part of the contract between azd and extensions. I'd like to > see test coverage for this." Adds TestRunnerInvoke_GlobalFlagPropagation with 3 cases: - all global flags set (Debug, NoPrompt, Cwd, Environment) - only environment set (verifies others absent) - no global flags (verifies no AZD_* env vars) Uses a mock CommandRunner to capture RunArgs and verify that InvokeOptions fields are correctly propagated as AZD_DEBUG, AZD_NO_PROMPT, AZD_CWD, AZD_ENVIRONMENT. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses @wbreza's review comment on auto_install.go:678: > "[Medium] Input Validation: Consider validating environment > name early. The raw -e value is stored without validation. > For extension commands, extensionAction.Run() passes it > directly to runner.Invoke() as AZD_ENVIRONMENT in the child > process." Uses environment.IsValidEnvironmentName() to reject invalid names before they reach GlobalCommandOptions. Adds test TestParseGlobalFlags_InvalidEnvironmentName with cases for spaces and special characters. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses @wbreza's review comment on auto_install_test.go:497: > "[Medium] A few edge cases and cross-field assertions would > strengthen this. 1. -e without a value to verify the > error/behavior when the flag is at end of args. 2. Cross-field > assertions for mixed-flag cases — assert EnableDebugLogging > and NoPrompt are true alongside -e." Adds: - Cross-field assertion: verifies --debug and --no-prompt are correctly parsed alongside -e dev - Edge case: -e at end of args with no value returns an error Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses @wbreza's review comment on env_flag.go:23: > "[Low] Two exported constants with the same name and value in > different packages (internal.EnvNameEnvVarName and > environment.EnvNameEnvVarName) may confuse callers. Consider > keeping it unexported if only ParseGlobalFlags (in cmd) needs > it." Renames internal.EnvNameEnvVarName to unexported envNameEnvVarName since it's only used within the internal package (EnvFlag.Bind). Updates cmd/ references to use environment.EnvNameEnvVarName instead. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses @wbreza's review comment on auto_install.go:644: > "[Low] pflag.StringP panics on duplicate names. A > globalFlagSet.Lookup guard would protect against a future > refactor that adds environment to CreateGlobalFlagSet()." Wraps the StringP call in a Lookup nil-check so a future refactor that moves the environment flag into CreateGlobalFlagSet won't cause a panic from duplicate registration. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Addresses @wbreza's review comment on container.go:204: > "[Low] The log says 'using default environment instead' but > with this fallback, the resolver may actually use the > pre-parsed -e value or AZURE_ENV_NAME. Consider adding a brief > log line when this fallback fires to aid debugging." Adds a log.Printf when the DI resolver falls back to globalOptions.EnvironmentName, making it clear in debug logs that the pre-parsed value is being used instead of the default. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
The rebase dropped the internal package import needed for GlobalCommandOptions in TestNewRootCmd. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
108fe90 to
a2e26fe
Compare
wbreza
left a comment
There was a problem hiding this comment.
Code Review Summary
Prior Review Resolution Status
All 8 prior review threads from @wbreza and @vhvb1989 have been fully resolved:
| # | Finding | Status | Verification |
|---|---|---|---|
| 1 | Fallback ordering: Cobra → context → globalOptions | ✅ Fixed | container.go confirms correct precedence chain |
| 2 | InvokeOptions from globalOptions untested | ✅ Fixed | runner_test.go with 3 test cases verifying AZD_* propagation |
| 3 | Validate environment name early | ✅ Fixed | IsValidEnvironmentName() check in ParseGlobalFlags |
| 4 | Edge cases: -e without value, cross-field | ✅ Fixed | Both tests present in auto_install_test.go |
| 5 | Unexport envNameEnvVarName |
✅ Fixed | env_flag.go uses lowercase envNameEnvVarName |
| 6 | Lookup guard before StringP |
✅ Fixed | globalFlagSet.Lookup() == nil guard present |
| 7 | Debug log when fallback fires | ✅ Fixed | log.Printf when globalOptions fallback is used |
| 8 | vhvb1989: Swap fallback ordering | ✅ Fixed | Context value checked before globalOptions |
Findings Summary
| Priority | Count |
|---|---|
| Critical | 0 |
| High | 0 |
| Medium | 1 |
| Low | 0 |
| Total | 1 |
🟡 Medium (1 finding)
DI resolver fallback chain lacks direct unit test (container.go lines 192-230)
The 3-level fallback chain (Cobra flag → context value → globalOptions) in the EnvFlag DI resolver has no direct unit test. runner_test.go tests the downstream InvokeOptions → AZD_* mapping but doesn't exercise the resolver itself. @wbreza raised this in the original review and accepted runner_test.go as partial coverage — testing DI resolvers directly requires significant IoC infrastructure. Known accepted gap, not a blocker.
✅ What Looks Good
- Follows established patterns: Mirrors exactly how
--cwd,--debug, and--no-promptare handled viaGlobalCommandOptions - Clean separation of concerns:
ParseGlobalFlagspre-parses once → DI resolver falls back →extensionActionreads from single source - Comprehensive tests: 11 table-driven parsing tests + cross-field assertions + edge cases + 3 runner propagation tests + invalid name tests
- Well-documented: Every change has comments explaining why, not just what
- All CI checks passing: golangci-lint (Linux + Windows), go-fix, copyright, cspell, bicep-lint, CLA — all green
- Input validation: Environment names validated early with
IsValidEnvironmentName(), preventing injection
Overall Assessment: Approve. Well-structured fix that correctly addresses a real data isolation bug following established patterns.
| globalOptions.EnvironmentName, | ||
| ) | ||
| envValue = globalOptions.EnvironmentName | ||
| } |
There was a problem hiding this comment.
[Medium] DI resolver fallback chain lacks direct unit test
The 3-level fallback chain (Cobra flag → context value → globalOptions) has no direct unit test. runner_test.go tests the downstream InvokeOptions → AZD_* mapping but doesn't exercise this resolver itself — e.g., verifying that a context value overrides globalOptions when both are set.
@wbreza raised this in the original review and accepted runner_test.go as partial coverage. Testing DI resolvers directly requires significant IoC infrastructure. Known accepted gap, not a blocker.
Extension commands use
DisableFlagParsing: true, so cobra never parses-e/--environment. This causedlazyEnvto always resolve to the default environment, leaking its variables into the extension process even when a different environment was explicitly requested.AZD_ENVIRONMENTwas also never set in these cases.Root cause
Two call sites both relied on
cmd.Flags().GetString("environment")which returns""for any command withDisableFlagParsing: true:EnvFlagDI resolver incontainer.go— controls which environmentlazyEnvloadsextensionAction.Run()inextensions.go— controlsInvokeOptions.Environment(i.e.,AZD_ENVIRONMENTpropagation)Changes
Follows the established
GlobalCommandOptionspattern used by--cwd,--debug, and--no-prompt:internal/global_command_options.go— AddedEnvironmentNamefield toGlobalCommandOptionscmd/auto_install.go—ParseGlobalFlags()now pre-parses-e/--environmentfrom raw args before cobra runs, storing the value inGlobalCommandOptions.EnvironmentName. The flag is added only to the parsing flag set (not toCreateGlobalFlagSet()) so it doesn't appear in every command's help text. Includes aLookupguard to prevent panic if a future refactor adds environment toCreateGlobalFlagSet(). Invalid environment names are validated early usingenvironment.IsValidEnvironmentName().cmd/container.go—EnvFlagDI resolver now acceptsglobalOptions *internal.GlobalCommandOptionswith a three-level fallback chain: Cobra flag → context value (envFlagCtxKey, used by workflow composition likeazd up) →globalOptions.EnvironmentName. A debug log line fires when the globalOptions fallback is used.cmd/extensions.go—extensionAction.Run()now usesa.globalOptionsdirectly for allInvokeOptionsfields (Debug,Cwd,Environment,NoPrompt), removing the fragilecmd.Flags().GetString()calls that silently fail for extension commandsinternal/env_flag.go— UnexportedenvNameEnvVarNamesince it's only used within theinternalpackage, avoiding confusion withenvironment.EnvNameEnvVarNameTestParseGlobalFlags_EnvironmentNamevalidates early parsing (short flags, long flags, equals syntax, concatenated-edev,AZURE_ENV_NAMEfallback/override, multiple-elast-wins, mixed with unknown flags, cross-field assertions,-ewithout value error).TestParseGlobalFlags_InvalidEnvironmentNamevalidates rejection of invalid names.TestRunnerInvoke_GlobalFlagPropagationverifies end-to-end propagation ofInvokeOptionsfields toAZD_*environment variables in the extension child process.Original prompt
💡 You can make Copilot smarter by setting up custom instructions, customizing its development environment and configuring Model Context Protocol (MCP) servers. Learn more Copilot coding agent tips in the docs.