Skip to content

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

Open
l2ysho wants to merge 40 commits into
masterfrom
claude/secret-storage-v2-1420
Open

l2ysho wants to merge 40 commits into
masterfrom
claude/secret-storage-v2-1420

Conversation

@l2ysho

@l2ysho l2ysho commented Sep 17, 2026 •

Copy link
Copy Markdown
Contributor

Note

TL;DR —

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

Closes #1420. Part of #1383.

What changes for users

Nothing in normal use. Same commands, same output. Stored logins move to the new names on the first command after the upgrade, with nothing to run or re-enter.

Three differences, all on failure paths:

  • logout used to exit 0 and say the keyring was cleared even when it was not. It now exits 1 and names the entry left behind, by service/account.
  • login warns when it could not remove a secret it meant to replace. The proxy-password case was silent before, and the stale password kept being injected into every local run.
  • A keyring that cannot answer at all still reports nothing, so a machine with no secret service is unaffected.

One contract change worth a release note: on a machine where the keyring exists but refuses a delete, logout now fails loudly. apify logout && … stops there instead of continuing.

This is also what makes more than one account storable. Nothing here exposes that — no --profile, no auth switch.

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.

getToken / setToken / getProxyPassword / setProxyPassword / deleteProxyPassword collapse into getSecret(userId, kind), setSecret(userId, kind, value) and deleteSecret(userId, kind).

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.

Which backend an account is on

backendFor(userId) answers from the token alone: a token in auth.json means the file, otherwise the keyring unless it is disabled or will not load. No marker is persisted, so one keyring failure never pins a later write to the file.

When a token write falls back to the file, the account's other secrets move with it — reads for that account all go to the file from that point.

Migration

ensureCredentialsCurrent() runs the three steps in their one valid order: plaintext secrets into the keyring, the file into its current shape, then the secrets onto keyed names. Every reader calls it.

  • Write the new entry, verify it reads back, then delete the old one.
  • Nothing the account already has is overwritten, and nothing under the fixed names is claimed for an account that already has a keyed token.
  • File backend: one atomic write moves the secrets into the profile and clears the top level.
  • Idempotent, single-flight, never throws.

loginWithToken() clears the fixed names on every login, including a repeat of the same account.

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 one.

A hand-deleted auth.json orphans keyed entries. The CLI never enumerates the keyring, so the file is its only index of what it holds. findCredentials() would enumerate a service on every platform but the Linux keyutils fallback, so a repair path stays open.

Verification

  • pnpm run test:local — 691 passed, 4 skipped (63 files).
  • 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 — green in CI.
  • Install size unchanged — no dependency added or removed.
  • Covered: old name → keyed name on the keyring; top-level secret → profile on the file; a keyring failure moving one account to the file while a second keeps reading the keyring; a login that runs before the migration; one account not claiming another's secret; a keyring that refuses a delete, and one that answers nothing at all; APIFY_DISABLE_KEYRING toggled between login and logout.
  • Tests now mock @napi-rs/keyring globally. __APIFY_INTERNAL_TEST_AUTH_PATH__ relocates auth.json but not the OS keyring, and two test files that run logout were reaching the developer's own stored login.

Manual, on macOS. Login and logout on both backends, the migration off the fixed names, and a login landing before the migration. A real Keychain ACL denial was hit by accident, which exercised the failure path end to end: logout exited 1, named com.apify.cli/proxy-password, left the entry in place and removed the profile.

Not verified: whether creating an item under a new service prompts on macOS. The migration runs on the first command after the upgrade, so if it prompts, re-key at next login instead.

Known and deferred

  • ensureSecretsKeyed() has no terminating condition, so it probes the two fixed names on every command — 2 of 4 keyring reads, unchanged from the start of this branch. On a machine where those entries cannot be deleted, it never stops.
  • The same fix covers both guards above: MIGRATION_STEPS in auth-file.ts is (file) => file, synchronous and pure, so keying could not be a versioned step. Widening it and bumping AUTH_FILE_VERSION to 3 retires the guards, the probe, and the file-shape helpers still in credentials.ts.
  • test/ is not typechecked (tsconfig.json is "include": ["src"]), which is how the test:api imports broke unnoticed.

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 and others added 4 commits September 23, 2026 10:57
auth.json was a flat blob describing one account, holding the whole
user('me') response. It is now { version, activeProfile, profiles,
secretsBackend }, so it can hold N accounts. Nothing puts a second one
there yet, and users see no change.

New src/lib/auth-file.ts owns the file: reading, an atomic write, the
v1 to v2 migration, and the profile accessors. credentials.ts, login,
logout, getLocalUserInfo() and the rental notice all go through it.

The migration backs the old file up as auth.json.v1.bak, runs after
ensureMigrated() as a separate step, is idempotent and single-flight,
and never throws. Fields nothing reads are dropped: email, plan,
effectivePlatformFeatures, isPaying, createdAt and proxy.groups.

Closes #1419

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Both parsed auth.json by hand and asserted the v1 flat shape, so neither
could pass against a v2 file. log_in_out deep-equalled the file against
the whole user('me') response, which v2 deliberately no longer stores;
info read a top-level id that is now the profile key.

Both now read the active profile through the test helper, and
log_in_out checks the token through getToken() rather than the file.

Not run here — test:api needs a live token.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
copyFileSync inherits the source mode. An auth.json written before the
CLI started passing mode 0600 is still 0644, and writeFileSync's mode
applies only on create, so it stayed that way. The new atomic write
fixes auth.json on the first v2 write, but the backup is copied before
that and never rewritten — leaving a plaintext token at 0644.

Also fixes two tests: apify info prints three rows since the token
source line landed, and the idempotency check called the migration
twice without resetting the memoised promise, so the second call never
touched the file.

Adds the missing cover for logout removing the backup, which is the
only path that erases that token from disk.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The backup is written once and never refreshed, and only logout deletes
it. So after `apify login` as a second account, auth.json holds the new
token while auth.json.v1.bak still holds the previous one — for as long
as the user never logs out. Nothing reads the backup, and a downgraded
CLI finds its token through the keyring or auth.json rather than here,
so the secrets are dropped when writing it.

Also pins the two lines that make the migration run for users. Deleting
`await ensureAuthFileCurrent()` from either resolveAuth() or
getLocalUserInfo() left the whole suite green: every migration test
called it by hand.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@l2ysho
l2ysho force-pushed the claude/auth-json-v2-1419 branch from d903722 to 0452abf Compare September 23, 2026 09:14
l2ysho and others added 7 commits September 23, 2026 11:25
Windows has no POSIX modes. Node reports 0o666 and chmod only moves the
read-only bit, so the assertion read 438 where it wanted 384. The two
other mode tests in the suite already skip on win32; this one now
matches them.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Moving data between shapes later needed this module reworked: the
migration was one function gated on "no version field", so the next
format change had nowhere to go. It is now a table keyed by the version
each step upgrades from, and a file runs every step from its own version
upwards. Adding a step is an entry in the table.

A failed migration now says so once instead of only under APIFY_DEBUG.
It still never blocks a command, because the readers understand the old
shape, but failing on every run should be visible.

Other fixes from review:

- Reserve AuthProfile.secretsBackend. Once secrets are keyed per
  profile, a keyring failure on one profile must not silently redirect
  another profile's reads to the file backend.
- ensureMigrated() skips a file a newer CLI wrote. It runs before the
  shape migration reports the version, and would otherwise rewrite it.
- Write the backup through the atomic writer, the one plain write left
  in a module built around temp file plus rename.
- Treat a non-object JSON payload as unusable. JSON.parse('"abc"')
  succeeds and Object.keys('abc') is ['0','1','2'], so it passed both
  migration guards and got replaced.
- Drop the comment calling the backup a way back. Nothing reads it and
  no procedure restores it; the comment beside it already said so.

Tests:

- getLocalUserInfo() returns organizationOwnerUserId. Deleting that line
  left the suite green while demoting every organization login to a
  personal account.
- A pre-existing 0644 auth.json is tightened to 0600. Only temp file
  plus rename does that; writeFileSync's mode applies on create only.
- The three apify run tests read the token and proxy password from disk
  again. They had come to assert getLocalUserInfo() against itself.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
setActiveProfile(userId, profile, backend) read as "mark this one
active". It means "make this the only account, and drop the previous
one's secrets". It is now replaceStoredAccount, and the docblock says
why it replaces rather than adds.

Replacing is deliberate twice over. Until each profile has its own
secret, a second profile would name an account that cannot
authenticate. And dropping the old secrets is what makes the write
safe: loginWithToken writes the new token straight after, so a failure
there leaves no token at all rather than the previous account's token
sitting beside the new account's name.

Two tests pin that second half, which nothing covered. Making the write
preserve siblings and root secrets — the shape Stage-2 will need —
fails both: the old profile survives, and so does the old token.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
AuthFile carried `[k: string]: unknown`, so it typed the v1 shape, the
v2 shape and an empty object identically. Reading a field that no
longer exists stayed legal, which matters because #1420 moves the
token and proxy password off the top level and into the profile.

Measured: remove `token` from the type and the old signature reported
2 errors. It now reports 12 — every reader, across auth-file.ts and
credentials.ts. Ten sites would have gone unnamed.

The v1 fields move to LegacyAuthFile, which extends AuthFile with the
three the flat shape carried. Only v1Profile, toV2 and the fallback in
lookUpActiveProfile take it; that fallback is the one cast left, and it
is where a file this CLI cannot version lands.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The version guard ran before resolveAuth read APIFY_TOKEN, so a stored
file written by a newer CLI stopped every command — including ones that
never read that file. A platform run or a CI job has APIFY_TOKEN as its
only credential and no interest in the stored login, and `apify run`
calls resolveAuth uncaught while deliberately catching the account
lookup on the next line. The guard now runs on the stored-login path
only.

Logout was refused by the same guard, which left no way out of the
state: the error offered "Upgrade the CLI" and never mentioned the
file. Logout exists to discard credentials, so it no longer checks the
version. A shape this CLI cannot read is discarded whole rather than
edited — the old code deleted fields from it and wrote it back, which
left a mangled file when it held more than one profile.

The error names `apify logout` as the escape, and drops the
parenthetical aside the repo's copy style does not take.

Removing the auth files also passes maxRetries again. rimrafPromised
carried 10 retries against Windows EBUSY when an antivirus or a second
process holds the file; the bare rmSync that replaced it had none.

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

- Restating the signature. "The parsed file, or an empty object when it
  is missing or unreadable" above a function that returns exactly that.
- Narrating the branch. "No index signature: removing one names every
  reader at compile time" justifies a commit, in a place that will rot
  once nobody remembers there was one.
- Repeated verbatim. The same three-line rationale sat above all three
  apify run assertions; one earns its keep.

Three fields carrying the same "unused until the device flow lands"
share one line now, and two of the longer blocks say the same thing
shorter.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
"Could not update auth.json to the current format, so it was left as it
is" reads like something broke. Nothing did — the readers understand the
old shape, so the command that triggered it carries on and the next one
tries again. A user with a read-only ~/.apify saw an alarming line on
every command with no action to take.

The message now leads with what matters to them and names the debug
variable for the part that does not.

Adds the failure-path test. The whole migration sits in one try/catch
and nothing covered it: removing the warning left the suite green.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@l2ysho
l2ysho force-pushed the claude/secret-storage-v2-1420 branch from 35a698c to 510f799 Compare September 23, 2026 21:31
l2ysho and others added 2 commits September 24, 2026 09:04
Two were wrong. ensureAuthFileCurrent said it brings the file "to the v2
profile shape", which predates the step chain, and that migrating "never
throws" — the function does, through the version assert one line below.
lookUpActiveProfile described the pre-profile read path as something
that covers commands running before the migration, which reads like
scaffolding; useRentalSunsetNotice calls it without migrating on
purpose, so that path is permanent.

The rest were restating the signature, saying the same thing in two
docblocks, or taking five lines for one idea.

Comment-only: the diff has no non-comment lines.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@l2ysho
l2ysho force-pushed the claude/secret-storage-v2-1420 branch from a39ad9f to 95f8bfd Compare September 24, 2026 12:38
l2ysho and others added 4 commits September 24, 2026 15:51
auth.json.v1.bak described the account it was taken from and is never
refreshed, so only logout removed it. Log in as someone else and one
user's details sat on disk under another user's login. A login now
discards it.

Profiles carry loggedInAt. Nothing reads it — `auth list` will order by
it, and a logout will fall back to the most recent profile left — but
neither can backfill a time nobody recorded, so it has to be written
from the start. A profile migrated from the pre-profile file gets null,
because that file never held one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Four lines for a field whose name and type say it. The reason it exists
belongs in the commit that added it.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
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>
l2ysho and others added 3 commits September 28, 2026 23:20
The keyring is used unless APIFY_DISABLE_KEYRING=1 is set or the module
cannot load. A token stored in the profile is the only record that its
secrets live in the file, so a past keyring failure no longer pins later
logins to plaintext, and a successful keyring write clears the file copies.

Co-Authored-By: Claude Opus 5.5 <noreply@anthropic.com>
l2ysho and others added 15 commits September 30, 2026 10:21
`keyKeyringSecrets()` copied a legacy keyring entry onto the keyed name
without checking what was already there. A login runs before the
migration does, so an upgraded user who logged in first had the new
token replaced by the old one on their next command.

Skip the copy when the account already has a secret of that kind, and
delete the legacy entry instead.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The three migrations run in a fixed order, because keying secrets by
user needs the ID the shape migration produces. That order was typed
out at both call sites and documented in a comment.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A login writes its secrets under keyed names, so anything left under
the fixed names is stale. It survived a repeat login of the same
account, and the next migration cannot tell it from a current secret:
a proxy password the account had dropped came back.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`deleteKeyring()` swallowed every error, so `clearKeyringSecrets()`
could not reject and logout's failure branch was unreachable. On a
locked keyring, logout said the secrets were removed and exited 0,
while deleting the only index of what the keyring holds.

Every key is still attempted, and login warns rather than failing when
it cannot remove the previous account's entries.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`getToken()` and `getProxyPassword()` are gone. Vitest resolved the
missing imports to undefined instead of failing, so the suite died at
the call.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`backendFor()` sends every read for an account to the file once its
token is there, so a proxy password left in the keyring could not be
read. A local run lost its proxy password until the next login.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The keyring module loads on machines where the secret service does not
answer, so every entry throws although nothing was ever stored. Logout
took that for a refused delete and exited 1, telling the user to clean
a keyring they do not have, after removing their account cleanly.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A keyed token means a login already stored this account's secrets
under the new names, so the fixed names hold whatever a previous login
left. The per-kind guard could not see that: a login clears the proxy
password it has none of, and the migration then read the previous
account's under the new one.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The check for whether the fixed names are this account's ran on every
command. On a keyed account they hold nothing, so it never decided
anything.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`clearKeyringSecrets` threw an AggregateError of bare messages, so
neither caller knew which entry survived. Logout named the account it
had just cleared successfully and never named the entry that was still
there. Login said nothing at all when the account had not changed,
leaving a live token in the keyring with nothing to ever mention it.

It now returns the surviving entries. Nothing wanted an exception:
both callers turned it straight back into a value.

Logout also keeps its report of APIFY_TOKEN and clears the telemetry
ID when the profile is gone, which the early return used to skip.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`deleteSecret()` is what stops a revoked proxy password surviving a
re-login, and it was the one delete whose failure nothing reported.
The stored value stays, and reads serve it before the file, so every
later local run gets a password the account no longer has.

It now returns what it left behind, and login folds that into the
warning it already prints.

Also corrects a claim repeated in four places: `@napi-rs/keyring`
does export `findCredentials()`. The CLI choosing not to enumerate is
the real reason auth.json is its only index.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A comment moved with login's warning and kept describing the narrower
set it used to cover: `getSecret` reads the keyring before the file,
so the proxy password it now reports is read on every command.

`describeLeftovers` does not print what a keyring app shows — Windows
and Linux Secret Service both name an entry differently.
`ensureCredentialsCurrent` can throw, through `ensureAuthFileCurrent`.
And `findCredentials()` throws on a keyutils machine rather than
returning nothing.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`__APIFY_INTERNAL_TEST_AUTH_PATH__` moves auth.json somewhere scratch,
but the OS keyring is per-user and `clearKeyringSecrets()` deletes the
fixed names whatever the backend is. Two of the seven test files that
reach credentials never mocked the module, and one of them runs
`logout`, so `test:api` deleted the developer's own stored login.

Mocked for every test file instead of per file, with `useAuthSetup`
refusing to run if the real module ever resolves again.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
@l2ysho
l2ysho marked this pull request as ready for review October 2, 2026 07:08
@l2ysho
l2ysho requested a review from DaveHanns as a code owner October 2, 2026 07:08
Base automatically changed from claude/auth-json-v2-1419 to master October 5, 2026 10:10
…ge-v2-1420-f1

# Conflicts:
#	src/commands/auth/logout.ts
#	src/lib/auth-file.ts
#	src/lib/auth.ts
#	src/lib/credentials.ts
#	src/lib/utils.ts
#	test/__setup__/auth-file.ts
#	test/api/commands/log_in_out.test.ts
#	test/local/commands/auth.test.ts
#	test/local/commands/run.test.ts
#	test/local/lib/auth-file.test.ts
#	test/local/lib/auth.test.ts
#	test/local/lib/credentials.test.ts
@apify-service-account apify-service-account added the tested Temporary label used only programatically for some analytics. label Oct 5, 2026

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. tested Temporary label used only programatically for some analytics.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Secret storage v2 Stage-1: Auth storage v2 and token resolution (no UX change)

2 participants