v3.1.0: per-identity API tokens, whoami, vendor gitignore - #14
Conversation
Org logins no longer hijack the global identity. `init` keeps `githubUsername` personal and stores the org in per-repo `playAsUsername`; API tokens are now keyed by username via an `apiTokens` map, with the legacy single token auto-migrated. Adds `git-slot-machine whoami` to show the resolved identity for the current repo. Security review of this diff (see PR body) turned up one hardening item fixed here: `vendor/` is now gitignored, so the 3,898 vendored PHP files pulled in by laravel/boost can't be staged by `git add .`. `composer.json`/`composer.lock` stay tracked. npm was never exposed — the `files` allowlist in package.json already excluded vendor/. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
ReviewThe core idea is right and the But the change is incomplete: three call sites still assume the old single-token world, and one of them hands the user the exact command that re-creates the bug this PR exists to fix. Blockinga)
where
.action(async (githubUsername: string) => {
await authLoginCommand(githubUsername, getPlayAsUsername() !== githubUsername);
});b) The new unauthenticated notice fires in
Gate it on c)
d)
Have On the file-permission finding you flaggedShip the To your question — unconditional One thing to get right, though: wrap it in try/catch. export function saveGlobalConfig(config: Config): void {
const configPath = getGlobalConfigPath();
fs.writeFileSync(configPath, JSON.stringify(config, null, 2), { mode: 0o600 });
try { fs.chmodSync(configPath, 0o600); } catch { /* best effort */ }
}Same treatment for the Should fixe)
Migration shouldn't be able to destroy a read: try {
saveGlobalConfig(migrated);
} catch {
// couldn't persist; the in-memory migration still stands for this run
}
return migrated;f) Token keys are case-sensitive; GitHub usernames are not
Related: the fallback branch at Smaller notes
|
Blocking items:
- login computes persistGlobalUsername from the repo's playAsUsername
(same rule as init), so the hint play.ts prints can no longer re-create
the org hijack this PR exists to fix
- Both play-time notices (no-remote, unauthenticated) gate on !--small:
that's the post-commit hook's only mode and its single-line contract
- config get all reads token state via getApiToken(), not the removed
legacy field through merged config
- authLoginCommand throws instead of process.exit, so init's catch is
reachable and org-token failures degrade to local play as designed;
the top-level CLI action owns the exit code
Security (the PR-body question, per review guidance):
- Global config dir 0700, file 0600, plus unconditional best-effort
chmodSync in saveGlobalConfig — self-heals drifted modes, safe where
chmod throws (Windows, bind mounts, CI volumes)
Should-fix:
- migrateLegacyToken persists inside try/catch: a failed write-back
(read-only HOME, ENOSPC) keeps the in-memory migration instead of
blanking a valid on-disk config to {}
- Token keys lowercased on write and lookup (GitHub usernames are
case-insensitive); legacy fallbacks in getApiToken and
getAuthenticatedUsernames removed as dead post-migration; lingering
legacy apiToken deleted even when the username already has a token
- logout --all wires the orphaned clearAllApiTokens
- whoami shows the per-repo override even when no GitHub remote parses
- CHANGELOG restored from main (formatter mangling reverted) with a
hand-written 3.1.0 entry
Tests: new config suite over a mocked homedir + temp cwd — legacy
migration, idempotent re-read, playAs token miss, org logout keeping the
personal token, case-insensitive lookup, failed write-back not blanking
the read, 0600 mode. 66 tests pass.
composer.json/lock stay tracked (deliberate, for the enumeration
tooling) — noted per review; happy to split on request.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review — round twoMost of round one landed cleanly. Confirming what's now correct:
One hole is still open, and it's the same hijack. Blocking —
|
3.1.0 stored apiTokens keys with whatever casing the user typed (login Broomfitters -> key 'Broomfitters'); this branch lowercases every lookup, so those tokens would silently miss and plays would go local-only with no visible cause. Normalize keys on read, first writer winning a casing collision to match pre-3.1.1 lookup behavior. Found live: a real 3.1.0 config held 'Broomfitters'/'PlaydownApp'/ 'MunksApp' keys that the new lookup missed. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
…rl pinned - init resolves the personal identity from getGlobalConfig() .githubUsername, not getGitHubUsername() — resolving playAsUsername first meant re-running init in an org repo adopted the org globally (skipped prompt, persistGlobalUsername true), reachable for every 3.0 upgrader with an org repo; test pins the rule - status hints name the real command (login, not the nonexistent auth login) - logout surfaces a failed server revocation instead of printing success while deleting the only local copy of a still-live token; logout --all says what actually happens (cleared locally for all, revoked server-side for the active identity) - config dir chmod 0o700 self-heals pre-3.1 0755 dirs, best-effort, matching the file treatment and the CHANGELOG claim - getApiUrl reads global config only: getHeaders() attaches the bearer token to whatever host apiUrl names, so a repo-local override could redirect syncs, token attached - write-back-failure test uses a mocked writeFileSync throw instead of chmod 0o400 (root ignores permission bits) and asserts the on-disk file still holds the legacy token Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review — round threeOne commit since round two (
But round two's blocking item is untouched, and the CHANGELOG in this diff now asserts it's fixed. Blocking —
|
Review — round threeRound two's blocking item is genuinely closed. Confirming the fixes I can verify by reading:
The One real defect left, and it's in code this PR adds. Should fix —
|
- api.logout takes an optional explicit bearer and returns false when there is no token — 'nothing to revoke' must not masquerade as 'revoked'. logout --all loops every held identity's token through a real server revocation and names the ones that failed; the previous wording claimed a revocation that never happened when the active identity held no token (reachable via logout then logout --all) - Single-logout failure wording covers both causes (unreachable server, rejected request), not just network - migrateLegacyToken keeps an unattributable legacy token instead of destroying the config's only credential; no write-back when there is nothing to migrate - Directory chmod moves off the read path into saveGlobalConfig — modes only change on writes, and the hook path was paying ~8 chmod syscalls per play - status shows 'Tokens held for' consistently at length > 0 - Version 3.1.1 (3.1.0 is live on npm and contains the bugs this branch fixes); CHANGELOG split into an accurate 3.1.0 entry and a 3.1.1 entry covering the review rounds Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review — round four
Also re-verified: One new defect, introduced by round three's own fix. Blocking — choosing "personal" in
|
- clearPlayAsUsername() + call in init's personal branch: the round-3 identity fix made the credit prompt render on re-runs, but choosing '1) personal' printed success while the org override survived — plays kept crediting the org - login adopts the name globally only when no identity is established or it matches the established one; a first org login (no override yet) previously overwrote the global identity silently. The deliberate change stays username:set - Re-authenticating best-effort revokes the token it replaces (overwriting without revoking left the old bearer live forever) - logout --all keeps tokens it couldn't revoke so a re-run can finish the job, instead of destroying the only credential able to - clearAllApiTokens deleted (orphaned by the above); sync hint names the real sync:disable command; tests for the override clear and the casing-collision first-writer rule (71 pass) Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review — round fiveRound four's blocking item is closed and the two "should fix" items landed. Confirming what I can verify by reading:
Three things worth addressing, one of which is a side effect of round four's own fix. Should fixa)
For a token that's already dead server-side — revoked from the web, user deleted, DB reset — that means:
The only escapes are hand-editing Either add While you're here, the two paths now disagree on the same failure: single b) The persist rule at Two reachable cases where that's actively misleading:
The rule is correct; it just needs to be legible. Thread the decision into the output — when c) Confirm the server revokes only the presented bearer before shipping the re-auth revocation
Reordering does not help: Smaller items
Same caveat as the previous rounds: dependency install requires approval in this environment, so I did not run |
…an token - logout gets --force (both paths): a server rejection is indistinguishable from an unreachable server through fetchWithFallback, so a token already revoked elsewhere could never be cleared — every re-run kept it forever. Single logout and --all now share one rule: keep on failed revocation, --force to clear locally anyway - login says when a stored token changes nothing: token minted for a name that neither is the global identity nor this repo's override now prints who the repo actually credits, pointing at init / username:set instead of a bare success message - Re-auth revocation confirmed safe against the real backend: DELETE /auth/token is currentAccessToken()->delete(), so revoking the replaced bearer can't kill the fresh one - clearApiToken no longer deletes the unattributable legacy token when no target resolves — the one call site that could undo migrateLegacyToken's preservation Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Review — round six (post-merge)This landed (merged 03:51:37, ~27s after Round five's three items are closed, and I can verify all of them by reading:
Re-verified for this round: One new defect, introduced by the Should fix —
|
What
Org logins no longer hijack the global identity.
initkeepsgithubUsernamepersonal and stores the org in per-repoplayAsUsername; API tokens are now keyed by username via anapiTokensmap, with the legacy single token auto-migrated. Addsgit-slot-machine whoamito show the resolved identity for the current repo.Security review
I ran a security review over this diff before opening the PR. Reviewer: I'd particularly like your read on the one open finding below.
Open — MEDIUM, deliberately not fixed here
src/config.ts:22-32and:106-109—mkdirSync/writeFileSyncpass nomode, so~/.git-slot-machine/lands at0755andconfig.jsonat0644. Verified on a real machine.That file now holds the plaintext
apiTokensmap, so this diff changes the blast radius: a single readable file used to yield one bearer token, and now yields one per identity you've logged in as. Tokens are long-lived — nothing expires or rotates them.Reachable by any other local UID, a co-tenant on a shared CI box, or an npm
postinstallrunning under a different user.Proposed fix, not yet applied:
Is the one-time
chmodSyncmigration the right call here, or would you rather see it gated behind a version check?Fixed here — LOW
vendor/is now gitignored. 3,898 vendored PHP files (laravel/boost, guzzle, symfony) were untracked with no ignore rule, sogit add .would have committed all of them. npm was never exposed — thefilesallowlist inpackage.jsonalready excludedvendor/.composer.json/composer.lockstay tracked.Checked and clean
execSynccall (config.ts:232,utils/git.ts,balance.ts:20,amendDetector.ts:18) uses a constant string. The only interpolation isamendDetector.ts:18's numeric${timeWindowMinutes}. Commit hashes and usernames never reach a shell.package.jsononly bumps the version. No typosquats incomposer.lock.apiTokensis built with object spread, so a__proto__key fromJSON.parsestays an own property.getApiToken(config.ts:136-156) now reads global config only and keys by exact username, so a repo-local config can no longer inject anapiToken.eval/Function, no TLS bypass, no path traversal, no secret logging (auth.ts:105prints a 10-char fingerprint only).Pre-existing, out of scope for this PR
Server-side, tracked separately:
POST /api/playis unauthenticated and trustsgithub_usernamefrom the body;play.tscomputespayout,balance_afterand thesuspiciousflag client-side and POSTs them, so all three are forgeable.Verification
npm run build— cleannpm test— 58 passed, 2 suites🤖 Generated with Claude Code