Repository navigation
Keep the install_id with the tokens it was issued to - #545
Conversation
HEY is about to bind each refresh-token lineage to the install that holds it and revoke the session when another install presents the token. The install_id lives in a file per config directory while the tokens live in one keychain entry, so two directories sharing that entry (a different HOME or XDG_CONFIG_HOME, a sandboxed agent) would present two installs for one lineage and revoke each other. Credentials now record the install they were issued to. Login stores it; a refresh presents it. Credentials saved before this adopt the directory's install_id on their first refresh and keep it, so whichever directory refreshes first settles the id for the rest. A refresh with a known install never reads or mints a directory's file. auth status shows the credentials' install when signed in.
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. |
![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: 3cd2c68566
ℹ️ 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".
![cubic-dev-ai[bot]](https://avatars.githubusercontent.com/in/1082092?s=60&v=4){kind=small}
There was a problem hiding this comment.
All reported issues were addressed across 5 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
…directory Credentials saved before they carried an install adopted the refreshing directory's install_id. Two directories sharing the keychain entry hold different ones, and their store locks are per directory, so racing first refreshes could each adopt their own and present two installs for one lineage; a refresh whose save failed lost the adoption the same way. Derive the adopted id one-way from the refresh token instead: every holder of that credential arrives at the same id with nothing to coordinate or persist first. auth status reports the id the next refresh will present. The test fake keyring is safe for concurrent use, as the keychain it stands in for is.
There was a problem hiding this comment.
All reported issues were addressed across 6 files (changes from recent commits).
Requires human review: Auto-approval blocked because this review re-detected 1 unresolved issue already reported by Cubic.
Re-trigger cubic
The test waited unconditionally for both refreshes to reach the server and discarded their errors, so a refresh that failed first hung it, and a failure after arrival passed unnoticed. Bound the wait, release the held handler on every path, and assert both refreshes succeeded.
HEY_TOKEN bypasses the stored credentials, so the install they refresh as isn't the one in use. Report the directory's install_id on that path, as before, and don't touch the keychain for it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4aec9578a9
ℹ️ 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".
HEY would read a malformed id as another install and revoke the session, and deriving a replacement would be another install too. Refuse it locally and ask for a fresh sign-in, which stores a well-formed one.
There was a problem hiding this comment.
All reported issues were addressed across 2 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 6efeffb3ce
ℹ️ 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".
RefreshInstallID dropped the malformed-id error, so status fell back to the directory's install_id, which no refresh would present. Return the error; status then reports no install and says why under install_id_error. A credential that fails to load for another reason now errs too, rather than reading as signed out.
There was a problem hiding this comment.
All reported issues were addressed across 4 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 860b98d83e
ℹ️ 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".
…e status contract Styled status printed Logged in with no install and no reason when the stored install_id was malformed; it now prints the reason and the hey login remedy. The --help text and docs/cli.md describe what install_id means signed in, signed out and under HEY_TOKEN, and what install_id_error reports.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
Mirroring the credentials' install in auth status opened a new edge each round (HEY_TOKEN, a malformed id, styled output, token and cookie logins) for a diagnostic this change doesn't need. What has to agree across config directories is the install a refresh presents, and that path already refuses a malformed stored id with an error any command surfaces. Status goes back to reporting the directory's install_id, as on main.
Credentials saved before they carried an install were issued to the logging-in directory's install_id, which every login and refresh on main presented and which HEY binds the family to at issuance. Deriving an id from the refresh token for them presented a different install on the first refresh after upgrading and got the session revoked. They now take the directory's install_id when its file holds a well-formed one, read without minting, and the token-derived id only when it doesn't. The refresh's save keeps the id, so later refreshes from any directory sharing the keychain entry present it. The concurrency test asserted every holder adopts the derived id, so it passed against a server with no binding while the upgrade path revoked. It's replaced by a server that binds and revokes: an upgrade bound to the directory's id survives, two directories converge sequentially, and a missing or malformed file takes the derived id and mints nothing.
There was a problem hiding this comment.
All reported issues were addressed across 3 files (changes from recent commits).
Tip: Review your code locally with the cubic CLI to iterate faster.
Re-trigger cubic
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 8d262b045a
ℹ️ 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".
The binding-server tests read what the server saw without its mutex. Read a snapshot under it. The install_id read extracted from installID lost its gosec annotation; restore it.
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 67e1298e28
ℹ️ 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".
HEY is about to bind each refresh-token lineage to the install that holds it: basecamp/haystack#8710. The binding is set when the family is issued, to the
install_idpresented at login. When a different install presents a bound family's token, HEY treats it as a copied token and revokes that session. Today hey-cli keepsinstall_idin a file per config directory (from #355), while the tokens live in one keychain entry per origin. Two config directories that share a keychain entry would therefore present two installs for one lineage, and both would be signed out. Examples are a differentHOMEorXDG_CONFIG_HOME, or a sandboxed agent.Change
Credentialsnow recordsinstall_id, the install the tokens were issued to. Login stores it; refresh presents it; the refresh's save keeps it.install_id, which every login and refresh on main presented, and which HEY binds the family to. So they take this directory'sinstall_idwhen its file holds a well-formed one. That's read, never minted. Otherwise they take an id derived one-way from the refresh token (SHA-256, formatted as a version-4 UUID), the same for every holder. Whichever they take is saved with the rotated tokens, so later refreshes from any config directory sharing the entry present it too.install_idis refused locally with ahey loginremedy and never sent: HEY would read it as another install.hey auth statusstill reports the config directory's install_id, as on main. What has to agree across directories is the install a refresh presents, and the refresh path refuses a malformed stored id with an error any command surfaces.Rollout order: release this, and get every binary that shares a keychain entry onto it, before haystack#8710 deploys.
Older binaries still present their own directory's id on refresh, and drop the new field when they re-save the credential. With the plaintext file store that happens on any origin's save, since it rewrites the whole file. A mixed old/new pair on one keychain entry can still revoke each other. The release floor is the answer.
Residual: shared entries the client can't attribute. Credentials saved before this release don't record which config directory logged in. So when two directories share a keychain entry, the client can't know which one HEY bound the family to. Two cases revoke the session once, after which both directories sign in again:
The token-derived id can't help either case, because it matches neither directory. Logins made before #8710 is live aren't exposed: their families are unbound, the first refresh claims them, and sequential refreshes then converge on the saved id. The release floor is what keeps this population small.
Tests:
make checkandgo test -race ./internal/...pass. A fake HEY that binds on first use and revokes on a mismatch covers: