feat(auth): use OS keyring in bundle distributions by l2ysho · Pull Request #1197 · apify/apify-cli

l2ysho · 2026-06-09T15:03:40Z

Closes #1170.

Problem

The OS-keyring storage from #1148 works for npm install -g apify-cli but not for the bundle distributions (install-cli.sh / install-cli.ps1), which are built with Bun's --compile. The @napi-rs/keyring wrapper loads its native .node via createRequire(__filename)('@napi-rs/keyring-<platform>'), and --compile doesn't follow that indirection — so no native module gets embedded and bundle users silently fall back to plaintext file storage (token in cleartext) even with a working keyring present.

Fix — embed one platform subpackage per bundle

Bun's docs: "the .node file must be directly required or it won't bundle correctly." Only a literal dynamic import('@napi-rs/keyring-<platform>') embeds and runs; static imports fail to bundle or crash with __require is not defined, and createRequire isn't bundled at all.

scripts/build-cli-bundles.ts — a placeholder keyring specifier stays external in the fat-JS step so the literal import() survives, then is rewritten per target to @napi-rs/keyring-<platform> before --compile resolves and embeds that one .node. Rewrite runs once per subpackage (baseline variants share their sibling's). Build throws if the placeholder is missing, so a bundle can never silently ship with plaintext fallback.
src/lib/credentials.ts — imports the placeholder in bundle mode (useCLIMetadata().installMethod === 'bundle') and the @napi-rs/keyring wrapper otherwise. Fallback/migration behavior unchanged.
pnpm-workspace.yaml — supportedArchitectures so every target's native subpackage installs at build time. Affects only pnpm install in this repo; npm consumers and the shipped bundle are unaffected.
src/lib/typings/keyring-native-subpackage.d.ts — ambient declaration so tsc/oxlint accept the placeholder specifier.
src/commands/auth/login.ts — drops the now-obsolete "install via npm for keyring" hint.

Verification

Compiled the real credentials.ts into a bun-linux-arm64 bundle, ran against real D-Bus + gnome-keyring:

✅ Legacy plaintext auth.json migrates → secretsBackend: "keyring", token removed, proxy password stripped.
✅ Token + proxy password discoverable under service com.apify.cli from another process (secret-tool search).
✅ No working keyring → graceful fallback to secretsBackend: "file", no crash.
✅ npm path unchanged — 23/23 credential tests pass.

lint, format, build, tsc green.

Note for reviewers

Criteria 1 & 2 were validated in a hand-rolled keyring environment; a sanity check on a real macOS/Windows/Linux desktop with pnpm run build-bundles is worth doing before release.

Bundle binaries (bun --compile) silently fell back to plaintext file storage because @napi-rs/keyring's createRequire-based platform loader isn't followed by --compile, so no native .node was embedded. Embed exactly one platform subpackage per bundle instead: - build-cli-bundles.ts keeps a placeholder keyring import external in the fat-JS step so the literal import() survives, then rewrites it per target to @napi-rs/keyring-<platform> so --compile resolves and embeds that one .node. The rewrite/write runs once per subpackage, not per target (baseline variants share their sibling's subpackage). - credentials.ts imports that placeholder in bundle mode (useCLIMetadata().installMethod === 'bundle') and the @napi-rs/keyring wrapper otherwise; all fallback/migration behavior is unchanged. - package.json pins pnpm.supportedArchitectures so every target's native subpackage is installed at build time (affects this repo's installs only, never npm consumers or the shipped bundle). - login.ts drops the now-obsolete "install via npm for keyring" hint. Closes #1170 Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

l2ysho · 2026-06-09T15:18:31Z

+function keyringSubpackage(os: string, arch: string, musl: boolean): string {
+	switch (os) {
+		case 'linux':
+			return `@napi-rs/keyring-linux-${arch}-${musl ? 'musl' : 'gnu'}`;
+		case 'darwin':
+			return `@napi-rs/keyring-darwin-${arch}`;
+		case 'windows':
+			return `@napi-rs/keyring-win32-${arch}-msvc`;
+		default:
+			throw new Error(`No @napi-rs/keyring subpackage known for ${os}-${arch}`);
+	}
+}
+


This is the main hack, --compile (bun) skipped napi-rs due to its dynamic require shenanigans -> we provide literal for specific bundle which is shipped as external subpackage (per arch).

https://bun.com/docs/bundler/executables

Move the keyring cross-platform target config out of package.json's pnpm field into pnpm-workspace.yaml, alongside the other pnpm settings (allowBuilds, overrides, nodeLinker). Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

l2ysho · 2026-06-10T09:22:28Z

Testing bundles in progress, but feel free to review.

patrikbraborec · 2026-06-10T10:49:30Z

I did review using Claude Code and Fable 5. Here is the one thing that might be interesting:

In build-cli-bundles.ts, the keyring rewrite was inserted after the Windows ARM64 special case that overrides arch = 'arm64'. CI has a windows-11-arm matrix job, so this path is live. Those bundles are still compiled with target: 'bun-windows-x64' — the output is an x64 PE that runs under x64 emulation on ARM64 Windows. The rewrite, however, computes keyringSubpackage('windows', 'arm64', …) → @napi-rs/keyring-win32-arm64-msvc, a plain ARM64 DLL. An x64 process (even emulated on ARM64 Windows) can only load x64 or ARM64EC DLLs, so LoadLibrary will fail, the dynamic import throws, and the code falls back to file storage — recreating the exact plaintext-fallback bug this PR fixes, for Windows ARM users only. Fix: capture the target's declared arch before the override (or move the rewrite above the override block) so these bundles embed win32-x64-msvc, which loads fine under emulation. It degrades gracefully today, so this isn't a blocker, but it's a two-line fix and otherwise the issue will be invisible until someone debugs it again.

vladfrangu · 2026-06-10T10:53:20Z

Ocne we merge my PR for unified CLI bundles the arm64 Windows remark won't matter anyways, so lets deal with that first

DaveHanns

Few nits, suggestions and questions.
Great job otherwise 🚀

DaveHanns · 2026-06-10T11:32:02Z

+	// platform loader isn't followed by Bun's `--compile`, so the native module never makes it
+	// into the binary. Instead each bundle embeds exactly one platform subpackage, and the
+	// specifier below is rewritten to it at build time (see scripts/build-cli-bundles.ts).
+	if (useCLIMetadata().installMethod === 'bundle') {


Q: in detectInstallMethod, we first check if (process.env.APIFY_CLI_MARKED_INSTALL_METHOD) {.

Once we publish bundled versions to NPM, the APIFY_CLI_MARKED_INSTALL_METHOD will be npm, but the underlying source will be bundle, right?

That will break this check AFAIK 🤔

bundles are not shiped to npm, install.sh download them from release artefacts -> https://github.com/apify/apify-cli/releases/tag/v1.6.2

Yeah, now they are not shipped through npm, but, and correct me if I'm wrong, there either was or still is a plan to ship bundles through npm instead of plain JS.

At least the comment above following check in function detectInstallMethod() indicates so:

// Will be useful once npm versions move to running from bundles (and for testing) if (process.env.APIFY_CLI_MARKED_INSTALL_METHOD) { return process.env.APIFY_CLI_MARKED_INSTALL_METHOD as InstallMethod; }

In such case, the APIFY_CLI_MARKED_INSTALL_METHOD would get set to npm even tho a bundle would be shipped and the affected if (useCLIMetadata().installMethod === 'bundle') { statement would not pass. We would then treat bundle as a plain JS, not applying the workaround this PR introduces.

That being said, I think this is not an issue as the APIFY_CLI_MARKED_INSTALL_METHOD is currently not used / set anywhere really. Let's just get rid of the check in function detectInstallMethod(). We can re-introduce it once it is needed, if ever.

Yes I see your point now, lets remove that.

DaveHanns · 2026-06-10T11:32:02Z

+	export class Entry {
+		constructor(service: string, account: string);
+		getPassword(): string | null;
+		setPassword(password: string): void;
+		deletePassword(): boolean;
+	}


Nit: @napi-rs/keyring exports Entry. Re-export should be possible and avoid type drift (if the upstream Entry in @napi-rs/keyring ever change).

Suggested change

export class Entry {

constructor(service: string, account: string);

getPassword(): string | null;

setPassword(password: string): void;

deletePassword(): boolean;

}

export { Entry } from '@napi-rs/keyring';

this is intentional, the placeholder resolves to the @napi-rs/keyring- native subpackage, which ships no .d.ts

I will update that commentary about this information.

Yeah, my point was that the '@napi-rs/keyring' exports Entry and it should be possible to simply re-export it from here as a type without having to "mirror" the Entry with hand written class 🤔 That way, if the Entry ever changes upstream, we get the update for free.

So, we keep the keyring-native-subpackage.d.ts, but its content would be just:

export type { Entry } from '@napi-rs/keyring';

or

export { Entry } from '@napi-rs/keyring';

Should be alright, AFAIK. Not tested tho.
That being said, it is just a nit, so feel free to ignore.

I remember I was fighting any in some point, but I tried reexport now and all 🟢

DaveHanns · 2026-06-11T08:51:22Z

+		// `.node`. Skip the rewrite when the on-disk file already targets this subpackage.
+		const subpackage = keyringSubpackage(os, arch, Boolean(musl));
+		if (subpackage !== writtenSubpackage) {
+			await writeFile(entrypointResultFilePath, fatEntrypointContent.replaceAll(KEYRING_PLACEHOLDER, subpackage));


Important: if, for any reason, the if (subpackage !== writtenSubpackage) { never passes, the proxy-agent patch at line 126 is now dropped (never written) as well.

Should not happen, AFAIK, but good to keep in mind and maybe future-proof.

good catch 👍

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

Explain why keyring native subpackage typings are declared by hand, not re-exported.

replaceAll is silent when the placeholder is absent, which would silently ship a bundle that falls back to plaintext storage. Throw at build time instead. Co-Authored-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>

… into 1170-use-os-keyring-for-bundles

DaveHanns

LGTM 👍

…ak it (#1224) The `apify push` standby test does an exact `.eql()` match on `createdActor.actorStandby` and pins `hostedBy: 'user'`. The platform has swapped that undocumented field for `tenancy: "SINGLE_TENANT"`, so the assertion now fails on every branch that hits the live API (seen on #1197 and on unrelated branches) — not flaky, a contract drift. Neither `hostedBy` nor `tenancy` is in the [public API docs](https://docs.apify.com/api/v2/act-get); only the eight documented standby fields are. So assert just those via `deep.include` and stop pinning server-owned, undocumented fields.

github-actions Bot assigned l2ysho Jun 9, 2026

github-actions Bot added this to the 142nd sprint - Tooling team milestone Jun 9, 2026

github-actions Bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Jun 9, 2026

l2ysho commented Jun 9, 2026

View reviewed changes

l2ysho requested review from DaveHanns and patrikbraborec June 10, 2026 09:21

l2ysho marked this pull request as ready for review June 10, 2026 09:22

l2ysho requested a review from vladfrangu as a code owner June 10, 2026 09:22

DaveHanns requested changes Jun 11, 2026

View reviewed changes

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. labels Jun 16, 2026

l2ysho modified the milestones: 142nd sprint - Tooling team, 143rd sprint - DX team Jun 16, 2026

l2ysho and others added 8 commits June 22, 2026 14:08

Merge branch 'master' into 1170-use-os-keyring-for-bundles

658d6c9

Update scripts/build-cli-bundles.ts

4795779

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

Update pnpm-workspace.yaml

db4902f

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

docs(keyring): clarify native subpackage d.ts rationale

4ee0153

Explain why keyring native subpackage typings are declared by hand, not re-exported.

Merge branch 'master' into 1170-use-os-keyring-for-bundles

40287da

fix(bundles): always write patched fat js before compile

0b2c7b0

Merge remote-tracking branch 'origin/1170-use-os-keyring-for-bundles'…

deea1e4

… into 1170-use-os-keyring-for-bundles

l2ysho requested a review from DaveHanns June 24, 2026 15:41

l2ysho added 2 commits June 25, 2026 07:56

refactor(auth): re-export keyring Entry type from wrapper

57b43e2

refactor: drop unused APIFY_CLI_MARKED_INSTALL_METHOD override

4c84daa

DaveHanns approved these changes Jun 25, 2026

View reviewed changes

Merge branch 'master' into 1170-use-os-keyring-for-bundles

ef6b422

l2ysho mentioned this pull request Jun 25, 2026

test(push): match standby subset so undocumented API fields don't break it #1224

Merged

Uh oh!

Conversation

l2ysho commented Jun 9, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Problem

Fix — embed one platform subpackage per bundle

Verification

Note for reviewers

Uh oh!

Choose a reason for hiding this comment

Uh oh!

l2ysho commented Jun 10, 2026

Uh oh!

patrikbraborec commented Jun 10, 2026

Uh oh!

vladfrangu commented Jun 10, 2026

Uh oh!

DaveHanns left a comment

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!

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!

l2ysho Jun 22, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

DaveHanns Jun 24, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

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!

DaveHanns left a comment

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

l2ysho commented Jun 9, 2026 •

edited

Loading

l2ysho Jun 22, 2026 •

edited

Loading

DaveHanns Jun 24, 2026 •

edited

Loading