Skip to content

feat: key secrets by user ID on both backends - #1450

Draft
l2ysho wants to merge 5 commits into
claude/auth-json-v2-1419from
claude/secret-storage-v2-1420
Draft

l2ysho wants to merge 5 commits into
claude/auth-json-v2-1419from
claude/secret-storage-v2-1420

Conversation

@l2ysho

@l2ysho l2ysho commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Note

TL;DR — in place. No UX change.

Stacked on #1434 (Stage-1, subtask 4). Base branch is claude/auth-json-v2-1419, not master.

Closes #1420. Part of #1383.

Change

Before After
keyring com.apify.cli / token, proxy-password com.apify.cli.token, com.apify.cli.proxy-password / <userId>
file auth.json.token, auth.json.proxy.password inside profiles[<userId>]

Service per kind, not a composite account. token:<userId> under one service would depend on : being legal in an account name on macOS Keychain, libsecret and Windows Credential Manager, and it reads worse in Keychain Access.

getToken / setToken / getProxyPassword / setProxyPassword / deleteProxyPassword collapse into getSecret(userId, kind), setSecret(userId, kind, value) and deleteSecret(userId, kind). One implementation, and the key is visible at every call site.

Both backends in one PR, because a keyring write can fail and send that account to the file mid-run. Split across two releases, that fallback would write the secret under a name the next read does not look for.

AuthProfile.secretsBackend goes live

This branch reserved the field for the moment secrets became per-profile. That is here, so it is now written and read.

  • backendFor(userId) is the profile's own secretsBackend, falling back to the file-level one. Every read and write goes through it.
  • A failed keyring write records secretsBackend: 'file' on that profile, in the same write as the secret, and leaves the file-level marker alone. Previously it flipped the file-level marker, which would send every other account to a file that does not hold their secrets — exactly what the field's comment warned about.
  • downgradeBackendToFile() survives with one caller, ensureMigrated(), which runs before any profile exists — there the file is the only place a fallback can be recorded.

Migration

ensureSecretsKeyed() is a separate step from the two that already exist. A v2 file whose secrets still sit under the old names is a supported state — every user is in it between #1434's release and this one — so the two migrations stay independent.

  • Write the new entry, verify it reads back, then delete the old one. The reverse order loses the secret when the delete succeeds and the write does not.
  • File backend: one atomic write moves the secrets into the profile and clears the top level.
  • A keyring write failure moves that profile to the file, and the rest of the loop follows it there rather than writing under a keyring name nothing will read.
  • Idempotent, single-flight, never throws.

It runs after ensureAuthFileCurrent(), which is where the user ID comes from. resolveAuth() keeps this branch's deliberate split — ensureMigrated() early, the shape migration only once APIFY_TOKEN has been ruled out — so the new step sits with the second half rather than being folded into one helper.

Behavior worth calling out

  • logout reordered. auth.json is the only index of what the keyring holds, so removing the profile first would strand its entries. The keyring is cleared first now. Legacy fixed-name entries are still deleted too, so logout works for anyone who never re-keyed. No version guard needed — removeActiveProfile() already discards a file it cannot read rather than refusing.
  • Switching accounts clears the outgoing account's entries. replaceStoredAccount() drops the previous profile and the secrets stored in the file, but keyring entries under the old user's key would survive and become unreachable — logout only ever clears the active profile. Two regression tests cover the switch and the same-account re-login.
  • A v1 file with a token but no id has no key to file the secret under. The secret is dropped and the next command asks for a re-login. That state already required a re-login — there is no account to attach the token to — so nothing reachable is lost. The v1 backup does not hold it either: this branch deliberately strips secrets from auth.json.v1.bak.
  • getLocalUserInfo()'s "Stale credentials found without user metadata" branch is gone. It was the error for exactly the state above, and dropping the secret makes it unreachable. A dangling activeProfile now reports the missing profile by name whether or not a secret was found — previously it could only tell when a token happened to be readable.
  • A hand-deleted auth.json still strands keyring entries. The keyring has no listing API, and reaching for fixed names on a machine with no account would touch the keyring on every command. Asserted in a test so the trade-off is on the record.

Verification

  • pnpm run test:local — 675 passed, 4 skipped (63 files), on this branch rebased onto the current base. Net +17 test cases (30 added, 13 replaced).
  • pnpm run lint, pnpm run format, pnpm run build — clean.
  • pnpm run update-docs — no change; no flag, arg, description or registration moved.
  • pnpm run test:api not run — no token in this environment.
  • The keyring mock keys entries as ${service}:${account}; com.apify.cli.token:uid and com.apify.cli:token are distinct strings, so the new names do not collide with the legacy ones in that map.
  • Covered: old key → new key on the keyring; top-level secret → profile on the file backend; a keyring failure moving one profile to the file while a second profile keeps reading the keyring; APIFY_DISABLE_KEYRING toggled between login and logout; logout leaving other profiles alone; two accounts holding their own entries; deleteSecret touching only one account and kind; idempotency and single-flight.
  • Install size unchanged — no dependency added or removed.

Not tested by hand: the macOS Keychain prompt. Creating an item under a new service may prompt, and this migration runs on the first command after the upgrade. Worth one manual check before release; if it prompts, re-key at next login instead of at first command.

Left out

  • No additive login, no --profile, no auth switch, no auth list.
  • clearKeyringSecrets() takes the profile whose entries to clear, but only ever sees one profile today — Stage-2 (Stage-2: Login - token multi account support #1386) is what puts a second one there.

🤖 Generated with Claude Code

@l2ysho l2ysho added the t-builders Issues owned by the Builders team. label Sep 17, 2026
@l2ysho
l2ysho force-pushed the claude/auth-json-v2-1419 branch from d903722 to 0452abf Compare September 23, 2026 09:14
@l2ysho
l2ysho force-pushed the claude/secret-storage-v2-1420 branch 2 times, most recently from a39ad9f to 95f8bfd Compare September 24, 2026 12:38
l2ysho and others added 2 commits September 24, 2026 16:05
Secrets lived under one fixed name per kind, so a second account would
overwrite the first one's token. Both the keyring and the file backend
are now keyed by user ID, and existing secrets are re-keyed in place.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A keyring write that fails for one account used to flip the file-level
marker, sending every other account to a file that does not hold their
secrets. The fallback is now recorded on the profile that hit it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
l2ysho and others added 3 commits September 24, 2026 18:07
auth.json is the only index of what the keyring holds, so both commands
destroyed entries before a step that can throw. A failed account switch
left the outgoing account's token deleted and the new one unwritten, and
a failed logout destroyed the secrets while auth.json still named the
account. Login now clears after the switch is on disk; logout attempts
both steps and reports what is left behind instead of claiming success.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Secret reads are v2-only, so a failed shape migration leaves the CLI
unable to read the token. It still told the user their login worked,
and the next command said they were not logged in. The warning now
states what is true and how to fix it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
auth.json is replaced through a temp file and a rename, so the write
needs the directory to be writable and the file's own mode never
matters. Telling the user to make the file writable sends them to
change something that has no effect.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-builders Issues owned by the Builders team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants