feat(auth): use os keyring for secure credential storage by l2ysho · Pull Request #1148 · apify/apify-cli

l2ysho · 2026-05-20T11:19:27Z

Store sensitive API tokens and proxy passwords in OS keyring for improved security, falling back to file.

getApifyClientOptions became async but several test sites still passed the returned Promise directly to `new ApifyClient(...)`, leaving the client with undefined token/baseUrl. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Persist the backend marker in auth.json on first login and honor it on subsequent runs so we don't re-probe the OS keyring on every CLI invocation. Also drop the write/delete side of the probe — getPassword on a non-existent entry is enough to detect an unavailable backend and avoids unnecessary Keychain access. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

The OS keyring is machine-global, so the unique __APIFY_INTERNAL_TEST_AUTH_PATH__ per test only isolates auth.json, not the keyring. After one test logged in, the leaked token made later tests see getToken() return a value with no matching username/id and throw "Corrupted local user info". useAuthSetup already pins in-process tests to the file backend; do the same for the dist subprocesses runCli spawns. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

Previously, clearSecrets() was a no-op when the active backend was 'file', so a user who logged in normally and later set APIFY_DISABLE_KEYRING=1 before running logout would leave their token in the OS keyring with no in-CLI way to remove it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

…in message When the user explicitly opts out via APIFY_DISABLE_KEYRING=1, calling the keyring "unavailable" and telling them to set the var they already set is misleading. Split the file-backend branch into two: env-disabled vs probe failure. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>

… keyring Scoping the keyring change down: stripping the entire proxy object from the userInfo write in getLoggedClient also dropped proxy.groups, which breaks the log_in_out API test that compares auth.json to the API user response. Leave proxy in the file as it was before and exclude the internal secretsBackend marker from the test comparison.

- Move `ensureApifyDirectory` from utils.ts to files.ts to break the credentials.ts <-> utils.ts circular import. - Drop the synthetic write/read/delete probe at backend selection time and treat the first real keyring write as the probe. On failure, downgrade to the file backend and persist the marker so future runs skip the keyring entirely. Avoids a duplicate macOS Keychain prompt on first login. - Reword the "Corrupted local user info" error to accurately describe the stale-keyring-without-metadata state.

The shared runId in the runs lifecycle test points at a hello-world actor that finishes in well under a second. After the resurrect step, the abort CLI's startup overhead (1–3s) meant the resurrected run had typically already SUCCEEDED by the time abort fired its API call, causing abort to print `Error: Run with ID "..." is already aborted.` to stdout with exit code 0 — which then broke `JSON.parse` in the test. Start a dedicated abort target run with a 30s `sleepMs` input so the abort always wins the race. Teach the basic test actor to honor the new `sleepMs` input.

l2ysho · 2026-05-28T15:27:33Z

Lets do some first round of review while I am testing on different systems cc @DaveHanns @patrikbraborec @vladfrangu

When the CLI runs from a Bun-compiled bundle (install-cli.sh path), the @napi-rs/keyring native module isn't included in the binary, so login always falls back to file storage. The new branch surfaces this to the user with an actionable hint — install via npm for keyring storage — instead of the generic "OS keyring unavailable" message, which suggests a fix (APIFY_DISABLE_KEYRING=1) that doesn't apply here. Tracking issue for actually shipping the native binary inside the bundle: see BUNDLE_KEYRING_ISSUE.md in the repo root.

DaveHanns

Few nits and suggestion, some questions.
Looks great otherwise 🚀

DaveHanns · 2026-06-02T12:05:32Z

+import { ensureApifyDirectory } from './files.js';
+import { cliDebugPrint } from './utils/cliDebugPrint.js';
+
+const KEYRING_SERVICE = 'com.apify.cli';


Q: each macOS Keychain item carries an ACL (Access Control List) of binaries allowed to read it, keyed by the creating binary's code signature, not to the service name (com.apify.cli). This CLI is commonly run as different binaries (a globally installed apify, npx apify-cli, a project-local node_modules/.bin/apify), so a token written by one may not be readable by another. macOS prompts (AFAIK) for access in an interactive shell, or fails outright in a headless/CI session. The auth.json had no such gate.

Not a blocker (and a non-issue if @napi-rs/keyring stores with a permissive ACL), but worth validating on macOS before merge.
The failure has to be graceful (clear error + relogin prompt), especially when headless where there's no dialog to approve.

Would this be a time where we want to start signing our macOS bundles? https://bun.com/docs/bundler/executables#code-signing-on-macos bun supports it, but we would need an apple certificate

DaveHanns · 2026-06-02T12:05:32Z

+	}
+
+	const data = readAuthFile();
+	data.proxy = { password };


Suggestion: data.proxy = { password } replaces the whole proxy object. AuthJSON.proxy is typed only { password }, but the file on disk can also hold other properties (proxy.groups) as it's written from the full User when fetched from API.

Harmless today (nothing reads groups or other proxy properties), but it might introduce unnecessary bugs or confusion in future.

Handling the "patch" gracefully, retaining the rest of the proxy properties, would be more pragmatic approach.

yes make sense, fixed + unit test -> 9380075

DaveHanns · 2026-06-02T12:05:32Z

+export async function clearSecrets(): Promise<void> {
+	await deleteKeyring(TOKEN_ACCOUNT);
+	await deleteKeyring(PROXY_PASSWORD_ACCOUNT);
+}


Nit: clearSecrets only clears the keyring. It works because logout rimrafs auth.json immediately after.

Either rename (clearKeyringSecrets) or also strip the secrets from auth.json so the function matches its name (and is correctly set up for broader usage outside the logout command)

DaveHanns · 2026-06-02T12:05:32Z

+		try {
+			const file = readAuthFile();
+			if (file.secretsBackend) return;
+			if (!file.token) return;


Suggestion: here we skip migration entirely even when there's a proxy.password but no token.

Unlikely (proxy without a token), but in that case the proxy never moves to the keyring and no marker is written.

DaveHanns · 2026-06-02T12:05:32Z

+			delete file.token;
+			if (file.proxy?.password) {
+				await writeKeyring(PROXY_PASSWORD_ACCOUNT, file.proxy.password);
+				delete file.proxy;
+			}


Nit: delete file.token is functionally fine, but moving it inside the try right after the write co-locates it and reads clearer.

Suggestion: The proxy writeKeyring is not wrapped in try/catch like the token write is. If it throws, it bubbles to the outer try and is swallowed -> no marker set, token already in the keyring but still in auth.json (this writeAuthFile never runs).
Re-migrates next run so no data loss, but the two writes should be handled consistently (wrap both, or write both then save once).

DaveHanns · 2026-06-02T12:05:32Z

+	if (backend === 'keyring') {
+		delete fileContents.token;
+		delete fileContents.proxy;


Nit: same as above, on the keyring backend this deletes the entire proxy object from auth.json, not just the password.
Harmless now but might be eventually.

DaveHanns · 2026-06-02T12:05:33Z

 */
 export async function getLoggedClient(token?: string, apiBaseUrl?: string) {
-	token = getTokenWithAuthFileFallback(token);
+	token = await resolveToken(token);


Suggestion: reassigning the token parameter is a code smell - prefer a new local, e.g. const resolvedToken = await resolveToken(token).
Same applies in getApifyClientOptions.

We should also explore options to extend the oxlint to catch issues like this automatically.

DaveHanns · 2026-06-02T12:49:26Z

+	migrationPromise = (async () => {
+		try {
+			const file = readAuthFile();
+			if (file.secretsBackend) return;


Suggestion:
with any failure to get / set keyring secret, we fall back to the file backend and this early return makes it permanent.

The failure might be temporary service outage and yet, user never recovers from this up until their logout.

Maybe we should attempt to migrate more often, for instance, when the getBackend is first called (backendPromise is undefined).

If we go this route, then we also have to deal with getBackend's if (marker === 'file') return 'file' as that locks in the file as well.

true but I think the percentage of users with this problem will be pretty low (temporary service outage scenario). I believe for someone with some exotic Unix distro this can happen, but 95% of all users should be fine. But if you have a concerns about it we can create a followup issue (this may be a big chunk of work to just add it here)

DaveHanns · 2026-06-02T13:20:17Z

+	const hasSomething = result.username || result.id || result.token;
+	if (!hasSomething) return {};
+
 	if (!result.username && !result.id) {
-		throw new Error('Corrupted local user info was found. Please run "apify login" to fix it.');
+		throw new Error('Stale credentials found without user metadata. Please run "apify login" again.');
 	}


Nit: readability and DRY fix:

Suggested change

const hasSomething = result.username || result.id || result.token;

if (!hasSomething) return {};

if (!result.username && !result.id) {

throw new Error('Corrupted local user info was found. Please run "apify login" to fix it.');

throw new Error('Stale credentials found without user metadata. Please run "apify login" again.');

}

const hasUserMetadata = !!(result.username || result.id);

const isComplete = hasUserMetadata || !!result.token;

if (!isComplete) return {};

if (!hasUserMetadata) throw new Error('Stale credentials found without user metadata. Please run "apify login" again.');

for some reason I cant apply it so it is here d5a9c11

Co-authored-by: David Hanuš <dh.david.hanus@gmail.com>

…al-storage' into 1115-use-os-keyring-for-credential-storage

The function only deletes keyring entries; auth.json cleanup is the caller's responsibility (logout rimrafs the whole file). Renaming makes the scope honest and avoids misleading future callers.

…rite

…rameter Avoids the param-reassignment smell in getLoggedClient and getApifyClientOptions. oxlint's no-param-reassign rule is currently disabled in config; enabling it broadly is left for a follow-up.

Factor out hasUserMetadata so the stale-credentials branch reads directly. Behaviorally identical to the previous two-stage check.

feat(auth): use os keyring for secure credential storage

966c365

Store sensitive API tokens and proxy passwords in OS keyring for improved security, falling back to file.

l2ysho linked an issue May 20, 2026 that may be closed by this pull request

Use OS keyring for credential storage #1115

Open

github-actions Bot assigned l2ysho May 20, 2026

github-actions Bot added this to the 141st sprint - Tooling team milestone May 20, 2026

github-actions Bot added t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels May 20, 2026

l2ysho added t-dx Issues owned by the DX team. and removed t-tooling Issues with this label are in the ownership of the tooling team. tested Temporary label used only programatically for some analytics. labels May 20, 2026

github-actions Bot added the tested Temporary label used only programatically for some analytics. label May 20, 2026

l2ysho and others added 14 commits May 20, 2026 13:35

Merge branch 'master' into 1115-use-os-keyring-for-credential-storage

078d098

wire up proxy.password

6891911

Align with mcpc

79a983a

Merge branch 'master' into 1115-use-os-keyring-for-credential-storage

e50795c

Merge branch 'master' into 1115-use-os-keyring-for-credential-storage

81b0aca

Merge branch 'master' into 1115-use-os-keyring-for-credential-storage

556f641

Merge branch 'master' into 1115-use-os-keyring-for-credential-storage

4d4ec6c

l2ysho marked this pull request as ready for review May 28, 2026 15:26

l2ysho requested a review from vladfrangu as a code owner May 28, 2026 15:26

l2ysho requested review from DaveHanns and patrikbraborec May 28, 2026 15:26

l2ysho added 2 commits May 29, 2026 13:22

Merge branch 'master' into 1115-use-os-keyring-for-credential-storage

faf9a41

l2ysho mentioned this pull request Jun 1, 2026

Use OS keyring for bundles #1170

Open

DaveHanns requested changes Jun 2, 2026

View reviewed changes

l2ysho and others added 12 commits June 2, 2026 22:25

Update src/lib/credentials.ts

5c24f1c

Co-authored-by: David Hanuš <dh.david.hanus@gmail.com>

Merge branch 'master' into 1115-use-os-keyring-for-credential-storage

1daee77

fix proxy spread and add test

9380075

Merge remote-tracking branch 'origin/1115-use-os-keyring-for-credenti…

b90fa2f

…al-storage' into 1115-use-os-keyring-for-credential-storage

refactor(auth): rename clearSecrets to clearKeyringSecrets

24cf143

The function only deletes keyring entries; auth.json cleanup is the caller's responsibility (logout rimrafs the whole file). Renaming makes the scope honest and avoids misleading future callers.

migrates proxy password to the keyring when token is absent

8406367

fix(auth): consolidate keyring migration writes into single try/catch

345ee8e

refactor(auth): skip redundant credential overlay on file backend

744c7e0

refactor(auth): drop redundant cast on userInfo.proxy access

c5bd02f

refactor(auth): preserve non-secret proxy fields on keyring backend w…

b047dec

…rite

refactor(auth): use resolvedToken local instead of reassigning the pa…

e42d9e5

…rameter Avoids the param-reassignment smell in getLoggedClient and getApifyClientOptions. oxlint's no-param-reassign rule is currently disabled in config; enabling it broadly is left for a follow-up.

refactor(auth): name the user-metadata check in getLocalUserInfo

d5a9c11

Factor out hasUserMetadata so the stale-credentials branch reads directly. Behaviorally identical to the previous two-stage check.

l2ysho requested a review from DaveHanns June 2, 2026 22:51

Conversation

l2ysho commented May 20, 2026

Uh oh!

l2ysho commented May 28, 2026

Uh oh!

DaveHanns left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants