feat(env): improve host command env injection and trust#45
Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e4feec9934
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| const exit = await runMacPrivilegedCommand({ | ||
| command: [brew, "services", "restart", "dnsmasq"], | ||
| command: [MAC_DNS_RECOVERY_HELPER_PATH, "restart-dnsmasq"], | ||
| interactive, |
There was a problem hiding this comment.
ensureMacDnsmasqRunning now always invokes sudo /usr/local/libexec/hack-dns-recovery restart-dnsmasq, but this path is never checked for existence here. On systems that have dnsmasq installed but do not yet have the new helper binary (for example, existing installs that only had the previous sudoers rule), hack global up will fail to restart dnsmasq and local wildcard DNS can remain broken. This path previously worked through brew services restart dnsmasq, so we should guard on helper presence and fall back (or force a refresh) to avoid breaking upgraded environments.
Useful? React with 👍 / 👎.
| if (await pathExists(MAC_DNS_RECOVERY_HELPER_PATH)) { | ||
| await runMacPrivilegedCommand({ | ||
| command: [brew, "services", "stop", "dnsmasq"], | ||
| command: [MAC_DNS_RECOVERY_HELPER_PATH, "stop-dnsmasq"], | ||
| interactive: isInteractiveTerminal(), |
There was a problem hiding this comment.
In the macOS shutdown flow, confirming “Stop dnsmasq” now only executes a stop command if the helper exists; otherwise it just warns and continues. That means users who have not installed the helper can confirm shutdown yet leave dnsmasq running, so host DNS behavior may stay active even though the command reports success. This should either fall back to the old sudo+b brew stop path or surface a hard failure so the resulting state is explicit.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 7e5044bcf9
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| command: (await pathExists(MAC_DNS_RECOVERY_HELPER_PATH)) | ||
| ? [MAC_DNS_RECOVERY_HELPER_PATH, "restart-dnsmasq"] | ||
| : [brew, "services", "restart", "dnsmasq"], |
There was a problem hiding this comment.
This branch always prefers /usr/local/libexec/hack-dns-recovery when the file exists, but the helper hardcodes the Homebrew path captured at global authorize time. If Homebrew is later moved/reinstalled to a different prefix, the helper’s restart-dnsmasq will fail and hack global up cannot recover DNS even though resolveBrewPath() can find the current brew binary. Add a fallback to brew services restart dnsmasq when the helper command fails so upgraded environments don’t get stuck on a stale helper.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: f63067f67f
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| if (await pathExists(MAC_DNS_RECOVERY_HELPER_PATH)) { | ||
| await runMacPrivilegedCommand({ | ||
| command: [brew, "services", "stop", "dnsmasq"], | ||
| command: [MAC_DNS_RECOVERY_HELPER_PATH, "stop-dnsmasq"], | ||
| interactive: isInteractiveTerminal(), | ||
| }); |
There was a problem hiding this comment.
When hack global down sees /usr/local/libexec/hack-dns-recovery, it calls runMacPrivilegedCommand once and ignores the exit code. If that helper exists but fails (for example, it embeds an old Homebrew path after a Homebrew reinstall or sudoers doesn’t permit that subcommand), the command still completes and prints success while dnsmasq remains running. This path should mirror the restart logic by checking the helper exit and falling back to brew services stop dnsmasq (or surfacing an explicit failure).
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 2c6fa839c4
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "sudo", | ||
| "install", | ||
| "-d", | ||
| "-m", | ||
| "0755", |
There was a problem hiding this comment.
The new sudoers rule grants passwordless execution of /usr/local/libexec/hack-dns-recovery, but this install step only sets mode and never enforces owner/group on the containing directory. On macOS setups where /usr/local/libexec is user-writable (common with Homebrew-managed /usr/local), a local user can replace the helper binary and then execute arbitrary code as root through the allowed sudo command. Please ensure the helper path is under a root-controlled directory or explicitly enforce root ownership before trusting it in sudoers.
Useful? React with 👍 / 👎.
| return [ | ||
| "#!/bin/sh", | ||
| "set -eu", | ||
| `brew_path=${JSON.stringify(opts.brewPath)}`, |
There was a problem hiding this comment.
renderMacDnsRecoveryHelper injects brewPath into a double-quoted shell assignment, which still performs $VAR and command-substitution expansion when the helper runs. Because brewPath is sourced from PATH during authorization, a crafted path containing shell metacharacters can be persisted and later evaluated as root via the passwordless helper. Use POSIX-safe single-quote escaping (or avoid shell interpolation entirely) when writing this script.
Useful? React with 👍 / 👎.
## 2.5.0 (2026-04-07) * test: add env matrix coverage and PR verification guardrails ([6d63ef1](6d63ef1)) * test(global): mock macOS trust roots in CI fixtures ([3550a2e](3550a2e)) * Merge branch 'main' into fix/host-runtime-ca-trust ([e4feec9](e4feec9)) * Merge branch 'main' into skill-progression-map ([b2e87c6](b2e87c6)) * Merge pull request #43 from hack-dance/fix/host-runtime-ca-trust ([be98476](be98476)), closes [#43](#43) * Merge pull request #44 from hack-dance/skill-progression-map ([031991c](031991c)), closes [#44](#44) * Merge pull request #45 from hack-dance/fix/host-runtime-ca-trust ([1e966f5](1e966f5)), closes [#45](#45) * Merge pull request #46 from hack-dance/fix/dns-helper-hardening ([e3fca8a](e3fca8a)), closes [#46](#46) * Merge pull request #47 from hack-dance/fix/dns-helper-preflight-order ([8850b5c](8850b5c)), closes [#47](#47) * fix(env): clarify host command env inspection ([3c10f3b](3c10f3b)) * fix(global): allow missing libexec during preflight ([dc2e15a](dc2e15a)) * fix(global): harden dns recovery helper authorization ([fa4bb7e](fa4bb7e)) * fix(global): invalidate sudo cache before verifying dns auth ([9eeca18](9eeca18)) * fix(global): limit host bundle to macos roots ([bd2c074](bd2c074)) * fix(global): preserve brew fallback around dns helper ([7e5044b](7e5044b)) * fix(global): refresh dns recovery sudoers rule ([6cfe755](6cfe755)) * fix(global): retry brew when dns helper is stale ([f63067f](f63067f)) * fix(global): retry brew when helper stop fails ([2c6fa83](2c6fa83)) * fix(global): skip host tls env when trust is declined ([7cbd139](7cbd139)) * fix(global): use a root-owned dns recovery helper ([d9abbb5](d9abbb5)) * fix(global): validate dns helper path before install ([ade0909](ade0909)) * fix(runtime): cover compose-target host trust fallback ([a8044ef](a8044ef)) * fix(runtime): preserve explicit host TLS env ([3f1c928](3f1c928)) * fix(runtime): tighten host trust env typing ([2ac7b51](2ac7b51)) * fix(runtime): trust local hack CA for host workflows ([bdab2bd](bdab2bd)) * feat(doctor): repair host tls trust drift ([7da28f5](7da28f5)) * feat(env): add shell mode for host env commands ([7337d59](7337d59))
1 participant
Summary
Verification