Fix getter mode baking env vars into generated code#8
Merged
Conversation
📊 Code Coverage ReportCoverage by file |
There was a problem hiding this comment.
Pull request overview
Fixes a security/behavior issue in “getter” generation mode where environment-variable overrides were being applied during code generation, potentially baking runtime env values (including secrets) into generated source code.
Changes:
- Default
modeis determined earlier inGenerateFromFileso it can gate env-override behavior. envoverride.Apply()is skipped when mode isgetter.- Adds a regression test ensuring getter-mode output doesn’t embed env var values and still generates
os.Getenv(...)lookups.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
| cfgx.go | Skips generation-time env overrides in getter mode by computing mode earlier and gating envoverride.Apply(). |
| cfgx_test.go | Adds a test verifying getter-mode generation doesn’t bake env values into generated code. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+103
to
+119
| err = GenerateFromFile(opts) | ||
| require.NoError(t, err, "GenerateFromFile() should not error") | ||
|
|
||
| output, err := os.ReadFile(outputFile) | ||
| require.NoError(t, err) | ||
|
|
||
| outputStr := string(output) | ||
|
|
||
| // The generated code should use the TOML default, NOT the env var value | ||
| require.Contains(t, outputStr, `"set-from-env"`, | ||
| "getter mode should use TOML default, not env var value") | ||
| require.NotContains(t, outputStr, "sk-real-secret-key-12345", | ||
| "getter mode must not bake env var values into generated code") | ||
|
|
||
| // It should still have the os.Getenv call for runtime override | ||
| require.Contains(t, outputStr, `os.Getenv("CONFIG_SECRETS_API_KEY")`, | ||
| "getter mode should still generate runtime env var lookups") |
There was a problem hiding this comment.
This test asserts on generated source text but doesn't verify the generated getter-mode file compiles (unlike the other GenerateFromFile tests below). Adding a quick go build on outputFile would catch missing imports / syntax issues and make the regression test stronger.
Comment on lines
+103
to
+107
| // Set default mode if not specified | ||
| mode := opts.Mode | ||
| if mode == "" { | ||
| mode = "static" | ||
| } |
There was a problem hiding this comment.
GenerateFromFile’s behavior (including whether env overrides are applied) depends on the exact opts.Mode string matching "getter". Consider normalizing/validating opts.Mode here (e.g., trim + strings.ToLower and reject unknown values) so typos/casing differences don’t accidentally fall back to static-mode behavior and reintroduce secret-baking during generation.
Comment on lines
+92
to
+93
| os.Setenv("CONFIG_SECRETS_API_KEY", "sk-real-secret-key-12345") | ||
| defer os.Unsetenv("CONFIG_SECRETS_API_KEY") |
There was a problem hiding this comment.
Use testing.T.Setenv (or capture/restore the prior value) instead of os.Setenv + defer os.Unsetenv. Unsetting can clobber an existing value in the parent environment and makes this test less isolated, especially if tests are run in parallel or the env var is already set on the machine/CI runner.
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
1 participant
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.
Summary
In getter mode,
envoverride.Apply()was running at generation time, which baked runtime environment variable values (e.g. secrets) into the generated source code as defaults. This fix skips env overrides during generation when mode isgetter, since getter mode already generatesos.Getenv()calls for runtime resolution.Changes
envoverride.Apply()when in getter mode