fix(cli): prefer local GitHub credentials [risk:high] - #414
Conversation
Centralize gh credential selection in the wrapper, preserve valid user credentials, and fall back to requester-bound managed credentials only when authentication is missing or revoked. Model: gpt-6
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. |
|
Desktop PR regression failed on commit
Recordings not attached
The Actions artifact retains the complete trace, screenshots, logs, and runtime evidence. |
![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: 511c12f210
ℹ️ 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 contextToken = getContextToken(); | ||
| if (!contextToken) { | ||
| // Old managed sessions without context must not adopt the machine owner's login. | ||
| return !process.env.LODY_GITHUB_REPO_FULL_NAME; | ||
| } |
There was a problem hiding this comment.
A teammate-controlled command can remove LODY_GIT_CRED_CONTEXT_TOKEN and LODY_GITHUB_REPO_FULL_NAME (for example, env -u LODY_GIT_CRED_CONTEXT_TOKEN -u LODY_GITHUB_REPO_FULL_NAME gh api user), causing this branch to authorize use of the daemon owner's saved gh login. These are ordinary child environment variables and ACP terminal overrides, so their absence is not proof of a local-owner invocation; fail closed unless the broker explicitly authorizes local authentication.
AGENTS.md reference: apps/cli/AGENTS.md:L337-L339
Useful? React with 👍 / 👎.
| for (const key of tokenKeys) { | ||
| if (!env[key]) continue; | ||
| if (await hasUsableGhAuth(ghCommand, env, target.host)) return { env }; |
There was a problem hiding this comment.
Session.buildShellEnv merges process.env into every ACP terminal, so a daemon launched with GH_TOKEN, GITHUB_TOKEN, or their enterprise equivalents passes the owner's ambient credential into teammate sessions. This loop accepts that token before consulting mayUseLocalAuth, allowing a requester whose broker context has allowLocalAuth: false to run gh as the daemon owner; inherited tokens must be stripped or ownership-gated before they are treated as explicit session credentials.
AGENTS.md reference: apps/cli/AGENTS.md:L334-L339
Useful? React with 👍 / 👎.
Bind managed gh wrappers to workspace brokers, fail closed without context, and remove inherited GitHub credentials from non-owner child environments. Replace ACP on requester changes while preserving active turn finalization. Model: gpt-6
|
Desktop PR regression failed on commit
Recordings not attached
The Actions artifact retains the complete trace, screenshots, logs, and runtime evidence. |
Merge current main to pick up the corrected public Codex lockfile. Read the frozen invocation requester in the cross-user steer guard and migrate stale runtime fixtures to the current identity model. Model: gpt-6
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: efbe2d93f6
ℹ️ About Codex in GitHub
Codex has been enabled to automatically 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 👍.
When you sign up for Codex through ChatGPT, Codex can also answer questions or update the PR, like "@codex address that feedback".
| if (!this.machineOwnerUserId || this.gitIdentity.id !== this.machineOwnerUserId) { | ||
| clearGitHubTokenEnv(agentEnv); |
There was a problem hiding this comment.
Fresh evidence after the earlier inherited-environment finding: this scrubs only the environment passed to the process, but the generated BASH_ENV and ZDOTDIR files subsequently source the owner's original startup files (lody-bashenv.ts:20-22 and lody-zdotdir.ts:34-36,56-58). When one of those files exports a GitHub token—especially ~/.zshenv on macOS—a teammate-owned ACP can launch bash/zsh and read or use the restored token directly, bypassing the gh shim and broker authorization; non-owner startup wrappers must not reintroduce these variables.
AGENTS.md reference: apps/cli/AGENTS.md:L342-L347
Useful? React with 👍 / 👎.
1 participant
Risk: 🔴 high | Confidence: medium — changes credential selection and requester boundaries; regression tests and adversarial re-reviews passed, live Windows/keychain flows remain unverified.
Lody's gh wrapper could override a working local GitHub login with a restricted App token. Prefer the machine owner's valid environment credentials and active local gh login; use requester-bound managed credentials when owner authentication is missing or returns HTTP 401, or when the requester is another workspace member.
Credential selection now lives in the wrapper. Remove session-wide managed token injection and its refresh state while retaining legacy token cleanup. Authorize the requester before considering any environment token or local login. Missing or expired contexts fail closed, and each workspace wrapper uses a daemon-selected broker state path that child environment overrides cannot replace. Managed wrappers and shell startup files live outside the ordinary CLI bin, so unmanaged local shells continue using native gh.
Non-owner Session processes lose GitHub token variables after every environment overlay, including case variants on Windows. Requester changes terminate and restore ACP before delivering input; cross-requester steer queues a normal follow-up. Internal ACP replacement preserves the active turn rather than racing an unscoped termination finalizer. Commands are never replayed, and managed GitHub.com tokens are never selected for another host.
Validation:
runtime.invocationidentity model. Main also supplies the corrected public Codex lockfile, resolving the Desktop E2E frozen-install failure.pnpm install --frozen-lockfile,pnpm typecheck,pnpm format:check,pnpm check:quick, andpnpm test:ciall passed. Explicit E2E TypeScript compilation passed too.Scope: this change prevents automatic credential inheritance and incorrect managed credential selection. The existing same-OS-user execution environment is not filesystem/keychain isolation; a hostile process with native credential access requires an OS/container boundary. An authentication-area owner should explicitly acknowledge this change before merge.