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). Those bundles are built with Bun's --compile. The @napi-rs/keyring wrapper resolves its native .node at runtime via createRequire(__filename)('@napi-rs/keyring-<platform>'), and --compile does not follow that indirection — so no native module was embedded and bundle users silently fell back to plaintext file storage (secretsBackend: "file", token in cleartext) even with a working keyring present.

Approach — embed one platform subpackage per bundle

The key constraint (per Bun's docs: "the .node file must be directly required or it won't bundle correctly") is that the native module must be referenced by a literal specifier that survives to the --compile step. Verified empirically: only a literal dynamic import('@napi-rs/keyring-<platform>') embeds and runs; static named/default imports either fail to bundle or crash with __require is not defined in ESM output, and createRequire isn't bundled at all.

scripts/build-cli-bundles.ts — a placeholder keyring specifier is kept external in the fat-JS step so the literal import() survives, then rewritten per target to @napi-rs/keyring-<platform> before --compile resolves and embeds that one .node. The rewrite/write runs once per subpackage rather than per target (baseline variants share their sibling's subpackage).
src/lib/credentials.ts — imports that placeholder in bundle mode (useCLIMetadata().installMethod === 'bundle', the codebase's standard bundle signal) and the @napi-rs/keyring wrapper otherwise. All fallback/migration behavior is unchanged.
package.json — pnpm.supportedArchitectures so every target's native subpackage is installed at build time. Scope: affects only pnpm install inside this repo (contributors/CI); npm consumers and the shipped bundle are unaffected (npm ignores the pnpm field; bundle users get a single embedded .node).
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

All four acceptance criteria verified end-to-end by compiling the real credentials.ts into a bun-linux-arm64 bundle and running it against a real D-Bus + gnome-keyring Secret Service:

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

lint, format, build, tsc all green. test:local: 304 passed; the only 2 failures are pre-existing Python actor-run fixtures (no Python runtime in CI sandbox), unrelated to this change.

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.

🤖 Generated with Claude Code

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

+		// `.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));


Suggestion: the replaceAll is silent if there is no KEYRING_PLACEHOLDER to replace in the fat js. That should not happen and would probably indicate a problem in the credentials.ts (typo in the KEYRING_PLACEHOLDER, ...) or during the build (possibly minimisation, content corruption, ...).

Let's check that the fat js includes at least one reference to the KEYRING_PLACEHOLDER and throw at build time.

if (!fatEntrypointContent.contains(KEYRING_PLACEHOLDER)) { throw ... }

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 🤔

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

+const KEYRING_PLACEHOLDER = '__APIFY_KEYRING_NATIVE_SUBPACKAGE__';
+
+// Maps the compiled (os, arch, libc) to the napi-rs keyring subpackage that ships its `.node`.
+// `pnpm.supportedArchitectures` (package.json) forces all of these into node_modules at build


Nit: stale reference — the second commit moved this config from package.json to pnpm-workspace.yaml.

Suggested change

// `pnpm.supportedArchitectures` (package.json) forces all of these into node_modules at build

// `supportedArchitectures` (pnpm-workspace.yaml) forces all of these into node_modules at build

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';

DaveHanns · 2026-06-11T07:31:59Z

 overrides:
    tar: 7.5.15

+supportedArchitectures:


Nit: let's comment on why we do this...

Something like ...

Suggested change

supportedArchitectures:

# Install native deps for all platforms, not just the current machine's, so the

# cross-platform bundle builds (scripts/build-cli-bundles.ts) can embed them.

supportedArchitectures:

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.

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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

feat(auth): use OS keyring in bundle distributions#1197

feat(auth): use OS keyring in bundle distributions#1197
l2ysho wants to merge 2 commits into
masterfrom
1170-use-os-keyring-for-bundles

l2ysho commented Jun 9, 2026

Uh oh!

l2ysho Jun 9, 2026

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

Uh oh!

DaveHanns Jun 10, 2026

Uh oh!

DaveHanns Jun 10, 2026

Uh oh!

DaveHanns Jun 10, 2026

Uh oh!

DaveHanns Jun 10, 2026

Uh oh!

DaveHanns Jun 11, 2026

Uh oh!

DaveHanns Jun 11, 2026

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

	// `pnpm.supportedArchitectures` (package.json) forces all of these into node_modules at build
	// `supportedArchitectures` (pnpm-workspace.yaml) forces all of these into node_modules at build

Conversation

l2ysho commented Jun 9, 2026

Problem

Approach — embed one platform subpackage per bundle

Verification

Note for reviewers

Uh oh!

l2ysho Jun 9, 2026

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!

DaveHanns Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

DaveHanns Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

DaveHanns Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

DaveHanns Jun 10, 2026

Choose a reason for hiding this comment

Uh oh!

DaveHanns Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

DaveHanns Jun 11, 2026

Choose a reason for hiding this comment

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants