Identify each CLI install to HEY with its own install_id - #355
Conversation
There was a problem hiding this comment.
Pull request overview
Adds persistent per-install UUIDs so HEY can distinguish OAuth devices and refresh-token lineages.
Changes:
- Persists a locked, mode-0600 UUIDv4 install identifier.
- Sends the identifier during authorization, token exchange, and refresh.
- Exposes it through authenticated status output and adds lifecycle/OAuth tests.
Tip
If you aren't ready for review, convert to a draft PR.
Click "Convert to draft" or run gh pr ready --undo.
Click "Ready for review" or run gh pr ready to reengage.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
internal/cmd/auth.go |
Adds install ID to status output. |
internal/auth/install_id.go |
Creates and persists per-install UUIDs. |
internal/auth/install_id_test.go |
Tests persistence, permissions, and uniqueness. |
internal/auth/auth.go |
Supplies install ID during OAuth and refresh. |
internal/auth/auth_test.go |
Verifies install ID transmission. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
![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: 6951a4a3aa
ℹ️ 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".
| return "", err | ||
| } | ||
|
|
||
| id := newInstallID() |
There was a problem hiding this comment.
For users upgrading from the old password-grant CLI who supplied its supported --install-id option, migrateOldCredentials migrates the tokens but OldConfig does not read the existing install_id, and ScrubLegacyCredentials then deletes it. This path consequently mints a different UUID here before the next refresh; unlike the documented placeholder-to-real transition, that changes an already-custom device identity and can make a refresh-token lineage bound to the old ID fail. Migrate a valid legacy installation ID into this file before scrubbing the old config.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Not doing this — leaving it for a human call. The premise is partly right: install_id is in legacyCredentialKeys, so the old config.json can carry one. But OldConfig deliberately omits install_id together with client_id/client_secret — the redesign does not carry the old password-grant client identity forward at all. Re-adopting only the legacy install_id while still dropping the client it was minted under would be incoherent: a password-grant refresh token is not refreshable by the new OAuth client regardless of install_id, so install_id is not the load-bearing factor in that lineage. A fresh device identity (and HEYs new-device signal) on the first OAuth login after upgrade is the intended behavior of this change, not a regression to paper over. It also reaches into migration code (OldConfig, migrateOldCredentials) outside this PRs diff. Flagging for a human decision rather than acting on it.
Every install sent the constant "hey-cli" as its install_id, so HEY saw every CLI sign-in as the same known device and never sent a new-device alert for one. Mint a v4 UUID per install on first use, keep it in <config dir>/install_id beside the credentials so it outlives a logout, and send it on login and refresh. Existing installs pick one up on their next run and HEY binds the grant to it from there. `hey auth status` shows it.
Write the identifier via a temp file and rename so an interrupted write can't leave a truncated value that the next run adopts and sends to HEY; validate a stored value against the v4 UUID shape on read, reminting a malformed one rather than transmitting garbage on login and refresh. Surface install_id in every auth status path — env token and logged-out included, both of which return before the signed-in path — so JSON and styled output stay consistent. Cover both output formats with tests.
6951a4a to
146ff77
Compare
Codex Review SummaryThis comment shows the latest Codex review activity on this pull request.
ℹ️ About Codex in GitHubYour team has set up Codex to review pull requests in this repo. Reviews are triggered when you
Codex reacts with 👀 while any review is running, comments if it has suggestions, and reacts with 👍 once all reviews finish with no findings. |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 146ff7765a
ℹ️ 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 err := tmp.Close(); err != nil { | ||
| return err | ||
| } | ||
| if err := os.Rename(tmpName, path); err != nil { |
There was a problem hiding this comment.
On Windows, which this repository ships, Go explicitly does not guarantee that os.Rename is atomic even within one directory. If the process or machine stops while replacing a malformed install_id, the destination can disappear; the next refresh then mints another UUID, which no longer matches the server-bound refresh-token lineage and prevents authentication until login is repeated. This is fresh evidence beyond the earlier comment: the new hardening now relies on os.Rename for its crash-safety guarantee, so the replacement needs a Windows-specific atomic implementation.
Useful? React with 👍 / 👎.
Why
Every hey-cli install sent the constant
install_id=hey-clion login and refresh, so HEY treated every CLI sign-in, on any machine, as one already-known device and never sent a new-device alert for it. This is the CLI half of the HEY OAuth blast-radius work (Security Hardening card 10248716553, H1 #3945131); the server half binds each refresh-token lineage to the install that presents it and stops accepting the placeholder for new sign-ins.What
<config dir>/install_id(0600). It lives beside the credentials, not inside them: a device outlives a logout, and re-logging in from the same machine should not look like a new device.install_idon the authorization request, the code exchange, and every refresh. The refresh path reads it under the store lock it already holds, sohey tuiandhey watchpresent the same id.hey auth statusshows it (Install:/install_id) for support.Rollout
No migration step. Existing installs mint an id on their next run; their next refresh presents it and the server rebinds the existing grant to it (quietly — the server treats the placeholder-to-real transition as the same device). An old binary left running (
hey watch) keeps sending the placeholder, which the server accepts as a no-op.Tests
Logout.go test ./internal/...,go vet,golangci-lintclean.Summary by cubic
Sends a unique per-install identifier to HEY on login and refresh so HEY can treat each CLI install as a distinct device and send new-device alerts. Previously every install sent the constant
hey-cli.<config dir>/install_idwith mode 0600.hey auth statussurfaces the ID in every status path, in JSON and styled output.Written for commit 146ff77. Summary will update on new commits.