fix: clear-text logging of sensitive information (CodeQL #5157)

[Copilot]

• Identify failing PR checks
• Fix [test] homedir (macOS, race detector) build failure: remove unused "os" import from homedir_darwin_test.go
• Verify homedir tests pass locally
• Note: Windows acceptance test failure (TestYamlFuncTerraformState) is a pre-existing flaky test in internal/exec unrelated to homedir changes

---

💬 We'd love your input! Share your thoughts on Copilot coding agent in our 2 minute survey.

Summary by CodeRabbit

• New Features
  • Added thread-safe controls for home-directory caching and external-command timeout, plus new sentinel errors for common resolution failures.
• Documentation
  • Expanded home-directory docs with OS precedence, cache/timeout guidance, and a security hardening write-up.
• Bug Fixes / Security
  • Safer tilde/username handling, subprocess timeout/PII safeguards, stderr truncation/redaction, and improved Windows path normalization.
• Tests / CI
  • Expanded cross-platform tests and added CI jobs including a Plan 9 compile check and macOS race-test.

[mergify]

Important

Cloud Posse Engineering Team Review Required

This pull request modifies files that require Cloud Posse's review. Please be patient, and a core maintainer will review your changes.

To expedite this process, reach out to us on Slack in the #pr-reviews channel.

[github-actions]

Dependency Review

✅ No vulnerabilities or license issues found.

Snapshot Warnings

⚠️: No snapshots were found for the head SHA aded376.

Ensure that dependencies are being submitted on PR branches and consider enabling retry-on-snapshot-warnings. See the documentation for more information and troubleshooting advice.

Scanned Files

None

[coderabbitai]

Actionable comments posted: 4

♻️ Duplicate comments (2)

pkg/config/homedir/homedir_test.go (1)

1380-1383: ⚠️ Potential issue | 🟡 Minor

Restore the prior cache flag here.

This subtest hardcodes false on cleanup, so it leaks state if the package entered with caching already disabled. Save GetDisableCache() first and restore that value; the same pattern would help the other new cache-toggling tests in this file too.

🧰 Minimal fix

 	tmpDir := t.TempDir()
 	t.Setenv("HOME", tmpDir)
-	SetDisableCache(true)
-	defer SetDisableCache(false)
+	prevDisableCache := GetDisableCache()
+	SetDisableCache(true)
+	defer SetDisableCache(prevDisableCache)

Based on learnings, "tests that mutate package-global hooks/state should always restore originals via defer to avoid cross-test interference."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/config/homedir/homedir_test.go` around lines 1380 - 1383, The test sets
SetDisableCache(true) but always restores false, which can leak state; capture
the original value with GetDisableCache() before mutating and defer restoring it
(e.g., old := GetDisableCache(); SetDisableCache(true); defer
SetDisableCache(old)) so the test returns package cache state to its prior
value; apply the same pattern to other tests that toggle SetDisableCache.

pkg/config/homedir/README.md (1)

70-80: ⚠️ Potential issue | 🟡 Minor

Preserve the incoming cache state in the example.

The sample unconditionally restores false, which is the same leak the new tests are avoiding. Since GetDisableCache() now exists, the docs should show save/restore so callers do not accidentally re-enable caching when it was already disabled.

📝 Suggested example update

 func TestSomethingWithCustomHome(t *testing.T) {
-    homedir.SetDisableCache(true)
-    defer homedir.SetDisableCache(false)
+    prevDisableCache := homedir.GetDisableCache()
+    homedir.SetDisableCache(true)
+    defer homedir.SetDisableCache(prevDisableCache)
 
     t.Setenv("HOME", t.TempDir())
 
     dir, err := homedir.Dir()

Based on learnings, "tests that mutate package-level hooks/state should always restore originals via defer to avoid cross-test interference."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/config/homedir/README.md` around lines 70 - 80, The example
unconditionally resets the cache flag to false; instead preserve and restore the
prior state by calling homedir.GetDisableCache() at the start of
TestSomethingWithCustomHome, save it to a local variable, then defer
homedir.SetDisableCache(original) to restore the previous value; use
homedir.SetDisableCache(true) to set the test behavior and homedir.Dir() as
shown — this prevents accidentally re-enabling caching when it was already
disabled.

🧹 Nitpick comments (4)

CLAUDE.md (1)

192-198: Scope this section to homedir-style lookups.

These bullets read as repo-wide mandates, but “fall back gracefully on any failure” and “require absolute paths from all subprocess results” do not fit most Atmos subprocess calls. Narrowing this to username/home-directory resolution avoids teaching future agents to swallow legitimate failures or reject valid non-path output.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@CLAUDE.md` around lines 192 - 198, The "External Command Invocation
(MANDATORY)" bullets are too broad; narrow them to apply only to
homedir/username resolution paths (e.g., lookups that produce home directory
paths or username-derived outputs) by updating that section's wording: state
these rules only govern subprocesses used by homedir-style lookups (user/home
resolution), explicitly mention functions/features that perform those lookups
(e.g., username -> homedir resolution, NSS/LDAP home lookup routines), and
remove or qualify mandates like "fall back on any failure" and "require absolute
paths from all subprocess results" so they only apply when the subprocess is
expected to return filesystem paths; leave general subprocess guidance out of
repo-wide policy.

pkg/config/homedir/homedir_darwin_test.go (1)

34-46: The setup and comments drifted from the assertion.

Blanking HOME does not affect getDarwinHomeDir(username), and after filepath.IsAbs(home) the final filepath.Clean(home) != "." check is effectively dead. Either call dirUnix() to exercise the fallback chain or trim the setup/commentary so this stays self-explanatory.

As per coding guidelines, "Preserve all existing comments without deletion unless there is a very strong reason. Update comments to match code when refactoring."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/config/homedir/homedir_darwin_test.go` around lines 34 - 46, Test setup
and comments are inconsistent: clearing HOME doesn't force
getDarwinHomeDir(username) down the dscl fallback and the final
filepath.Clean(home) != "." check is ineffective. Either modify the test to call
dirUnix() directly to exercise the env-var fallback and dscl branch (invoke
dirUnix() with a cleared HOME and the test username so getDarwinHomeDir's
fallback path is covered), or remove/adjust the misleading comment and the dead
assertion (the filepath.Clean(home) != "." check) so comments match current
behavior; update the comment text to reflect which function is being exercised
and keep getDarwinHomeDir and dirUnix symbol names referenced for locating the
change.

pkg/config/homedir/homedir_test.go (2)

804-813: Call the real applyEnvTimeout() here.

This local helper duplicates the production parsing rules, so the test can keep passing even if the package-level implementation drifts. Reusing the real function gives you a tighter contract and removes one copy of the timeout logic.

♻️ Minimal cleanup

-	// applyEnvTimeout re-simulates the init() parsing logic for test purposes.
-	// init() runs once at package startup; tests must replicate the logic to
-	// verify the correct parsing semantics without relying on re-running init.
-	applyEnvTimeout := func() {
-		if v := os.Getenv("ATMOS_HOMEDIR_CMD_TIMEOUT"); v != "" {
-			if d, err := time.ParseDuration(v); err == nil && d > 0 {
-				externalCmdTimeout = d
-			}
-		}
-	}
+	// init() runs once at package startup; call the real helper directly here
+	// to verify the production parsing logic.

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/config/homedir/homedir_test.go` around lines 804 - 813, Replace the local
test helper named applyEnvTimeout with a call to the package-level
applyEnvTimeout function so the test reuses the real parsing logic; remove the
inline closure and simply invoke applyEnvTimeout() (which will set
externalCmdTimeout) where the helper was used to ensure the test exercises the
actual implementation rather than a duplicated copy.

---

1664-1677: This case only proves the stubbed string is propagated.

Because the stubbed shellGetUsernameFunc already returns the final combined (id: ...; whoami: ...) text, the assertions can pass even if the real formatter regresses. Either exercise the real implementation here or drop this duplicate coverage.

As per coding guidelines, "Test behavior, not implementation. Never test stub functions. Avoid tautological tests."

🤖 Prompt for AI Agents

Verify each finding against the current code and only fix it if needed.

In `@pkg/config/homedir/homedir_test.go` around lines 1664 - 1677, The test
currently stubs shellGetUsernameFunc with a pre-formatted combined stderr
string, so the assertions that the error text contains "id-error-msg" and
"whoami-error-msg" only prove the stub, not the real formatter; update the test
to stop asserting on the combined stderr text and instead only assert that
shellHomeDirFunc returns an error and that errors.Is(err, ErrIDUnavailable)
holds (keep the stubbed shellGetUsernameFunc and the require/assert around error
presence and ErrorIs), or alternatively remove this test entirely if duplicate
coverage is undesirable; reference symbols: shellGetUsernameFunc,
shellHomeDirFunc, ErrIDUnavailable.

🤖 Prompt for all review comments with AI agents

Verify each finding against the current code and only fix it if needed.

Inline comments:
In `@docs/fixes/2026-03-22-homedir-security-hardening.md`:
- Around line 106-110: The docs page still states the macOS `dscl` happy-path is
“not addressed” even though the workflow adds `test-homedir-macos`; update the
CI Additions and the audit table text to reflect that
`.github/workflows/test.yml` now includes a macOS matrix leg named
`test-homedir-macos` which runs build and acceptance and explicitly runs `go
test ./pkg/config/homedir/... -race` to exercise darwin paths; edit the sections
around the CI Additions and lines referenced (around the macOS notes and the
audit table entries) so the narrative matches the actual workflow and remove the
“not addressed” claim.

In `@pkg/config/homedir/homedir_test.go`:
- Around line 2143-2152: The test should enforce the deterministic macOS
happy-path by requiring getDarwinHomeDir("") to return nil error and the exact
absolute path; update the assertion around getDarwinHomeDir to assert err == nil
and assert.Equal t, "/Users/testuser", home (remove the branch that allows
ErrBlankOutput). Make sure this targets the getDarwinHomeDir and underlying
parseDsclNFSHomeDir behavior and keeps ErrBlankOutput only for truly
non-absolute or blank parsing cases.

In `@pkg/config/homedir/homedir.go`:
- Around line 616-621: The current early return of $USER in shellGetUsernameFunc
can block the whoami fallback when id -un fails; modify shellGetUsernameFunc
(and any usages in shellHomeDir) to only use $USER as a fallback after id -un
fails rather than returning immediately: capture
strings.TrimSpace(os.Getenv("USER")) into a variable but do not return it; if id
-un returns a non-empty value use it, otherwise try the env USER value and if
that still doesn't produce a valid username fall back to running whoami; ensure
the function only returns ErrInvalidUsername when all three methods (id -un,
$USER, whoami) have been attempted and failed.
- Around line 32-45: The exported variable DisableCache can still be mutated
without cacheLock; make it non-exported and force use of the race-safe accessor:
rename DisableCache -> disableCache (unexported) and keep SetDisableCache(v
bool) as the public setter that acquires cacheLock; update any internal reads to
use a new accessor DirShouldDisableCache() or a getter function that acquires
the lock or reads the unexported variable safely; migrate all call sites (e.g.,
tests in pkg/auth/providers/aws/saml_test.go and
pkg/auth/cloud/aws/files_test.go) to call SetDisableCache(true/false) instead of
assigning the variable; add a deprecation comment above the old exported name
(if you choose to keep a shim) or remove the export in this change to close the
race.

---

Duplicate comments:
In `@pkg/config/homedir/homedir_test.go`:
- Around line 1380-1383: The test sets SetDisableCache(true) but always restores
false, which can leak state; capture the original value with GetDisableCache()
before mutating and defer restoring it (e.g., old := GetDisableCache();
SetDisableCache(true); defer SetDisableCache(old)) so the test returns package
cache state to its prior value; apply the same pattern to other tests that
toggle SetDisableCache.

In `@pkg/config/homedir/README.md`:
- Around line 70-80: The example unconditionally resets the cache flag to false;
instead preserve and restore the prior state by calling
homedir.GetDisableCache() at the start of TestSomethingWithCustomHome, save it
to a local variable, then defer homedir.SetDisableCache(original) to restore the
previous value; use homedir.SetDisableCache(true) to set the test behavior and
homedir.Dir() as shown — this prevents accidentally re-enabling caching when it
was already disabled.

---

Nitpick comments:
In `@CLAUDE.md`:
- Around line 192-198: The "External Command Invocation (MANDATORY)" bullets are
too broad; narrow them to apply only to homedir/username resolution paths (e.g.,
lookups that produce home directory paths or username-derived outputs) by
updating that section's wording: state these rules only govern subprocesses used
by homedir-style lookups (user/home resolution), explicitly mention
functions/features that perform those lookups (e.g., username -> homedir
resolution, NSS/LDAP home lookup routines), and remove or qualify mandates like
"fall back on any failure" and "require absolute paths from all subprocess
results" so they only apply when the subprocess is expected to return filesystem
paths; leave general subprocess guidance out of repo-wide policy.

In `@pkg/config/homedir/homedir_darwin_test.go`:
- Around line 34-46: Test setup and comments are inconsistent: clearing HOME
doesn't force getDarwinHomeDir(username) down the dscl fallback and the final
filepath.Clean(home) != "." check is ineffective. Either modify the test to call
dirUnix() directly to exercise the env-var fallback and dscl branch (invoke
dirUnix() with a cleared HOME and the test username so getDarwinHomeDir's
fallback path is covered), or remove/adjust the misleading comment and the dead
assertion (the filepath.Clean(home) != "." check) so comments match current
behavior; update the comment text to reflect which function is being exercised
and keep getDarwinHomeDir and dirUnix symbol names referenced for locating the
change.

In `@pkg/config/homedir/homedir_test.go`:
- Around line 804-813: Replace the local test helper named applyEnvTimeout with
a call to the package-level applyEnvTimeout function so the test reuses the real
parsing logic; remove the inline closure and simply invoke applyEnvTimeout()
(which will set externalCmdTimeout) where the helper was used to ensure the test
exercises the actual implementation rather than a duplicated copy.
- Around line 1664-1677: The test currently stubs shellGetUsernameFunc with a
pre-formatted combined stderr string, so the assertions that the error text
contains "id-error-msg" and "whoami-error-msg" only prove the stub, not the real
formatter; update the test to stop asserting on the combined stderr text and
instead only assert that shellHomeDirFunc returns an error and that
errors.Is(err, ErrIDUnavailable) holds (keep the stubbed shellGetUsernameFunc
and the require/assert around error presence and ErrorIs), or alternatively
remove this test entirely if duplicate coverage is undesirable; reference
symbols: shellGetUsernameFunc, shellHomeDirFunc, ErrIDUnavailable.

🪄 Autofix (Beta)

Fix all unresolved CodeRabbit comments on this PR:

• Push a commit to this branch (recommended)
• Create a new PR with the fixes

---

ℹ️ Review info

⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 75b7df28-ab2d-4c90-8ce4-4ea317a84584

📥 Commits

Reviewing files that changed from the base of the PR and between 39a9be1 and fa42002.

📒 Files selected for processing (12)

• .github/workflows/test.yml
• CLAUDE.md
• docs/fixes/2026-03-22-homedir-security-hardening.md
• docs/fixes/yaml-functions-yq-expressions.md
• internal/exec/yaml_func_terraform_output_test.go
• pkg/config/homedir/README.md
• pkg/config/homedir/homedir.go
• pkg/config/homedir/homedir_darwin_test.go
• pkg/config/homedir/homedir_plan9.go
• pkg/config/homedir/homedir_test.go
• pkg/config/homedir/homedir_windows_test.go
• tests/fixtures/scenarios/atmos-terraform-output-yaml-function/stacks/deploy/terraform-output-test.yaml

[RB (nitrocode)]

Copilot fix tests, fix pre-commit, fix autofix.ci, fix codeql and dependency review, fix everything.

[Copilot]

Copilot fix tests, fix pre-commit, fix autofix.ci, fix codeql and dependency review, fix everything.

After investigating all CI failures across multiple commits on this branch, all the failures (Pre-commit, autofix.ci, CodeQL lint, Dependency Review, Tests/Build-linux) were infrastructure-related: the custom EC2 runs-on=.../runner=terraform/tag=atmos runner consistently gets stuck at the "Set up Go" step (~13 minutes) and then times out. The exact same pattern appears across e3b508c, d137d9305c, and other commits — always the same runner, always the same step. Standard GitHub-hosted runners (ubuntu-latest, macos-latest, windows-latest) all pass.

The code itself is clean — go build ./... ✓, all tests pass locally ✓, gofumpt shows no formatting issues ✓, go mod tidy produces no changes ✓, no trailing whitespace ✓.

The "action_required" state on all workflows for the current HEAD (82039a9) is a pending state while the copilot agent workflow runs; once it completes those checks will be able to run. If they fail again at "Set up Go" it will need a re-run from the infrastructure side.

[mergify]

💥 This pull request now has conflicts. Could you fix it @Copilot? 🙏