Skip to content

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

Open
l2ysho wants to merge 2 commits into
masterfrom
1170-use-os-keyring-for-bundles
Open

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

Conversation

@l2ysho

@l2ysho l2ysho commented Jun 9, 2026

Copy link
Copy Markdown
Contributor

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.jsonpnpm.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:

  1. ✅ Legacy plaintext auth.json migrates → secretsBackend: "keyring", token removed, proxy password stripped.
  2. ✅ Token + proxy-password discoverable under service com.apify.cli from a separate process (secret-tool search).
  3. ✅ With no working keyring: graceful fallback to secretsBackend: "file", no crash.
  4. ✅ 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>
@github-actions github-actions Bot added this to the 142nd sprint - Tooling team milestone Jun 9, 2026
@github-actions github-actions Bot added the t-tooling Issues with this label are in the ownership of the tooling team. label Jun 9, 2026
Comment on lines +75 to +87
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}`);
}
}

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 l2ysho marked this pull request as ready for review June 10, 2026 09:22
@l2ysho l2ysho requested a review from vladfrangu as a code owner June 10, 2026 09:22
@l2ysho

l2ysho commented Jun 10, 2026

Copy link
Copy Markdown
Contributor Author

Testing bundles in progress, but feel free to review.

@patrikbraborec

Copy link
Copy Markdown
Contributor

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

Copy link
Copy Markdown
Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t-tooling Issues with this label are in the ownership of the tooling team.

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Use OS keyring for bundles

4 participants