Fixed 1Password op:// secrets being clobbered by .env loading#413
Fixed 1Password op:// secrets being clobbered by .env loading#413
Conversation
|
👋 ejacquier, thanks for creating this pull request! To help reviewers, please consider creating future PRs as drafts first. This allows you to self-review and make any final changes before notifying the team. Once you're ready, you can mark it as "Ready for review" to request feedback. Thanks! |
There was a problem hiding this comment.
Pull request overview
This PR fixes a configuration-loading edge case when running cre under op run --env-file ..., where unresolved op://... placeholders from the .env file were overwriting secrets that op run had already resolved and injected into the process environment.
Changes:
- Replace
godotenv.Overloadusage withgodotenv.Readplus a manualos.Setenvloop. - Skip applying
.enventries whose values begin withop://to avoid clobbering 1Password-resolved secrets. - Add a regression test ensuring
op://values don’t overwrite an already-set environment variable.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.
| File | Description |
|---|---|
internal/settings/settings.go |
Changes env-file loading to selectively apply vars while preserving op run-resolved secrets. |
internal/settings/settings_load_test.go |
Adds coverage for the new “skip op:// values” behavior. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| // op:// reference must not clobber the value injected by op run. | ||
| assert.Equal(t, "resolved-value", os.Getenv("MY_SECRET"), | ||
| "op:// reference in .env should not overwrite the value resolved by op run") | ||
| // Plain values still override the shell env (Overload semantics preserved). | ||
| assert.Equal(t, "plain-value", os.Getenv("PLAIN_VAR")) | ||
| // loadedEnvVars still reflects the raw file content. |
Summary
When using op run --env-file .env -- cre workflow simulate ... (the documented 1Password workflow), secrets were not being resolved — the literal op://vault/item/field string was injected instead of the actual secret value.
Root cause: loadEnvFile used godotenv.Overload, which reads the .env file from disk and overwrites all matching env vars already in the process. Since op run injects resolved secrets before launching cre, and cre then re-reads the same .env file with
Overload, the unresolved op:// reference would clobber the value op run had already set.
Fix
Replace godotenv.Overload with a godotenv.Read + manual loop. For each key in the file: