feat(auth): use os keyring for secure credential storage#1148
Merged
Conversation
Store sensitive API tokens and proxy passwords in OS keyring for improved security, falling back to file.
github-actions
Bot
added
t-tooling
l2ysho
added
t-dx
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>
github-actions
Bot
added
the
tested
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.
Contributor
Author
|
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
requested changes
Jun 2, 2026
DaveHanns
left a comment
DaveHanns
left a comment
Contributor
There was a problem hiding this comment.
Few nits and suggestion, some questions.
Looks great otherwise 🚀
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.
…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.
DaveHanns
reviewed
Jun 4, 2026
Co-authored-by: David Hanuš <dh.david.hanus@gmail.com>
… siblings
The merged suggestion changed `delete file.proxy` to `delete file.proxy?.password`
in ensureMigrated(), which broke the build (TS2790: delete operand must be
optional) and left a vestigial empty `proxy: {}` in auth.json for the
password-only case.
- Make `proxy.password` optional in StoredAuthFile (reflects on-disk reality).
- Strip only the password, then drop the proxy object when nothing else remains.
- Add a test asserting sibling proxy fields (e.g. groups) survive migration.
Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DaveHanns
approved these changes
Jun 8, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
4 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Store sensitive API tokens and proxy passwords in OS keyring for improved security, falling back to file.