Conversation
hsh now fetches the OS/arch-matched daemon, verifies it against the release SHA256SUMS, and hands it to the system service manager via sudo. Runs implicitly on first `hsh login`; `--no-setup` opts out. 🤖 Generated with Mister Maluco Co-authored-by: MisterMal <teskeslab@lucasteske.dev>
Contributor
Author
|
Tested on Linux and macOS. Automated by MisterMal |
racerxdl
marked this pull request as ready for review
August 24, 2026 17:24
PR Summary by QodoInstall hsh-tunneld automatically via new
AI Description
Diagram
High-Level Assessment
Files changed (7)
|
Code Review by Qodo
1. TOCTOU root binary exec
|
![qodo-code-review[bot]](https://avatars.githubusercontent.com/in/484649?s=60&v=4){kind=small}
Comment on lines
+119
to
+120
| export const setupCommand = new Command("setup") | ||
| .description("Install the Hoop networking components (hsh-tunneld system service)") |
There was a problem hiding this comment.
1. setup.ts missing command export 📘 Rule violation ≡ Correctness
The new command file exports setupCommand but does not export a Command object as required, making command exports inconsistent across src/commands. This can break conventions/tools that rely on a standard Command export name.
Agent Prompt
## Issue description
Compliance rule requires each file under `src/commands/` to define and export a value named `Command`. The new `src/commands/setup.ts` instead exports `setupCommand`.
## Issue Context
`src/index.ts` currently imports `setupCommand` from `./commands/setup.ts`.
## Fix Focus Areas
- src/commands/setup.ts[119-125]
- src/index.ts[4-4]
- src/index.ts[27-29]
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+383
to
+386
| progress(`Verified ${downloaded.assetName} (sha256 ${downloaded.sha256})`); | ||
| progress("Registering the daemon with your system service manager (sudo required)"); | ||
| runPrivilegedInstall(downloaded.path, opts.installArgs); | ||
|
|
There was a problem hiding this comment.
2. Toctou root binary exec 🐞 Bug ⛨ Security
installDaemon downloads hsh-tunneld into a user-writable hsh state dir and then executes that staged path via sudo; another process running as the same user can swap the file between verification and sudo exec, leading to arbitrary code execution as root when the user enters their password.
Agent Prompt
### Issue description
`hsh setup` verifies the downloaded daemon while writing it to `~/.hsh/bin/hsh-tunneld`, but later runs that *path* as root (`sudo <path> install`). Because the staged file is user-writable, another process running as the same user can replace it after verification (including while the user is typing their sudo password). That creates a TOCTOU window where unverified bytes may execute as root.
### Issue Context
- The staging location is inside the user’s hsh home directory.
- The privileged step executes the staged file directly via `sudo`.
### Fix Focus Areas
- src/tunnel/installer.ts[161-165]
- src/tunnel/installer.ts[280-295]
- src/tunnel/installer.ts[383-386]
### Implementation guidance
Implement a privileged “verify-then-exec” step so the hash check is performed *inside* the sudo session on a root-owned (non-user-modifiable) file:
1. Change `downloadDaemon()` (or `installDaemon()`) to return the expected SHA as well (or compute it again).
2. In `runPrivilegedInstall`, when not already root:
- Run a single `sudo` command that:
- `chown root:root` + `chmod 0755` the staged file **first** (so the user can’t modify it anymore),
- recomputes and checks sha256 against the expected value (use `sha256sum` on Linux and `shasum -a 256` on macOS, or detect availability),
- only then `exec`s `"$daemonPath" install ...`.
3. If the checksum verification fails inside sudo, abort and delete the staged file.
This closes the TOCTOU window without requiring changes to the Go daemon installer logic.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+76
to
+81
| function downloadBase(): string { | ||
| const override = process.env.HSH_DAEMON_RELEASE_BASE; | ||
| if (override && override.trim() !== "") { | ||
| return override.trim().replace(/\/+$/, ""); | ||
| } | ||
| return "https://github.com"; |
There was a problem hiding this comment.
3. Untrusted daemon source override 🐞 Bug ⛨ Security
HSH_DAEMON_RELEASE_BASE lets callers redirect both the manifest and daemon download to an arbitrary base URL, so an attacker who can influence the environment can change the trust root and cause installation of a root-executed binary from a non-GitHub host.
Agent Prompt
### Issue description
`downloadBase()` honors `HSH_DAEMON_RELEASE_BASE` and uses it for both `SHA256SUMS` and the daemon asset. This effectively makes the trust root configurable at runtime for code that will be executed as root.
### Issue Context
Even if the checksum is verified, allowing an arbitrary host as the checksum source means an attacker who can influence the environment for `hsh login`/`hsh setup` can point the flow at a malicious repo mirror and still satisfy checksum verification.
### Fix Focus Areas
- src/tunnel/installer.ts[64-82]
- src/tunnel/installer.ts[190-203]
- src/tunnel/installer.ts[231-240]
### Implementation guidance
Pick one of these (strongest first):
1. Remove the override entirely in production builds (keep a test-only injection via dependency injection or a test helper).
2. Require an explicit opt-in env flag like `HSH_UNSAFE_ALLOW_DAEMON_RELEASE_BASE=1` before honoring `HSH_DAEMON_RELEASE_BASE`.
3. Restrict the override to `https://github.com`-like origins (or an allowlist), and reject non-HTTPS URLs.
Add a unit test asserting the override is ignored/rejected unless explicitly enabled.
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Comment on lines
+39
to
+42
| export function daemonInstalled(): boolean { | ||
| if (readControlToken()) return true; | ||
| const tok = resolveTokenPath(); | ||
| return tok.exists || tok.fromEnv; |
There was a problem hiding this comment.
4. Env override masks missing daemon 🐞 Bug ≡ Correctness
daemonInstalled() returns true when resolveTokenPath().fromEnv is true even if the token file does not exist, which can cause hsh login implicit setup (and hsh setup without --force) to skip installation on machines where the daemon is actually absent but env vars are set.
Agent Prompt
### Issue description
`daemonInstalled()` currently treats `resolveTokenPath().fromEnv` as proof the daemon is installed. But `fromEnv` only means an env var was set (e.g., `HSH_TUNNELD_TOKEN_FILE`), not that the file exists.
This can incorrectly suppress:
- the implicit install path in `hsh login` (setupWanted becomes false), and
- `hsh setup` early-exit messaging (claims already installed),
when the env var points to a missing file.
### Issue Context
`resolveTokenPath()` sets `fromEnv: true` regardless of existence when an override is present.
### Fix Focus Areas
- src/commands/setup.ts[39-43]
- src/tunnel/socket-path.ts[90-108]
- src/commands/login.ts[79-88]
### Implementation guidance
1. Change `daemonInstalled()` to only return true on *actual presence* indicators:
- `readControlToken()` OR `resolveTokenPath().exists` (and optionally `resolveSocketPath().exists`).
2. If you still need “don’t implicitly install when user has env overrides”, introduce a separate predicate like `daemonEnvOverridesPresent()` and use it only to gate the *implicit* setup path (not the explicit `hsh setup` command).
3. Add tests for:
- `HSH_TUNNELD_TOKEN_FILE` set to a nonexistent path → `daemonInstalled()` should be false; implicit setup should still be allowed (or should print a clear hint, depending on desired behavior).
ⓘ Copy this prompt and use it to remediate the issue with your preferred AI generation tools
Qodo Fixer🍒 Ready to be cherry-picked — ✅ Merged (0) · ☑ Fixed (1) 🔗 Fix PR: #35 This fix PR was closed automatically. Its branch is preserved so you can cherry pick the changes into the original PR. Prompt for coding agent Process — 1 fixed
|
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.
Installs the
hsh-tunnelddaemon automatically so a freshhshreaches a working tunnel without the user knowing a second binary exists (DEP-141).hsh setup [--force]downloads the OS/arch-matched daemon from thehoophq/hooprelease pinned byBUNDLED_DAEMON_VERSION, then delegates registration tosudo hsh-tunneld install— no reimplementation of the systemd unit / LaunchDaemon logic that already has Go-side test coverage. It also runs implicitly on firsthsh login;--no-setupopts out.hsh updatewhich only replaces the unprivileged CLI. A failed verify deletes any previously staged binary, so a stale daemon can never survive a failed install. hsh never reads the password —sudoprompts on the inherited TTY, keeping Touch ID/PAM intact.hsh-tunneld-darwin-arm64downloaded, sha2565512b0ed…matched against the publishedSHA256SUMS, sudo argv correct, non-zero installer exit surfaced as a typed error. 15 new tests (tampered asset discarded, stale binary removed, missing manifest/entry fatal, exit-code propagation);bunx tsc --noEmitclean; 466 pass.Worth reviewer attention: implicit setup is gated on a
/dev/ttyprobe rather thanprocess.stdin.isTTY, becausesudoreads the password from/dev/tty— I confirmed sudo still prompts and blocks with stdin at/dev/null, so the stdin check both skipped setup wrongly forhsh login < fileand failed to protect the CI case it was meant to.Automated by MisterMal