Skip to content

Store CLI OAuth refresh tokens in keyring#1608

Merged
ChiragAgg5k merged 1 commit into
mainfrom
cli-refresh-token-keyring
Jun 23, 2026
Merged

Store CLI OAuth refresh tokens in keyring#1608
ChiragAgg5k merged 1 commit into
mainfrom
cli-refresh-token-keyring

Conversation

@ChiragAgg5k

Copy link
Copy Markdown
Member

What

Store CLI OAuth refresh tokens in the OS secure credential store through @napi-rs/keyring, while keeping access tokens and token expiry in prefs.json for the existing fast path.

This adds a small refresh-token storage helper for the generated CLI and wires OAuth login, token refresh, logout, session cleanup, and auth classification through it.

Why

The OAuth device login flow currently stores both access and refresh tokens in prefs.json. Access tokens are useful to keep in prefs for fast reads, but refresh tokens are longer-lived and should be kept in secure storage when available.

The OAuth feature is still feature-flagged, so this intentionally does not preserve refresh tokens in prefs as the normal path for new sessions.

Changes

  • Added templates/cli/lib/auth/refresh-token.ts.
    • Uses @napi-rs/keyring with one credential per CLI session ID.
    • Removes refreshToken from prefs after successful keyring writes.
    • Falls back to prefs storage if keyring read/write is unavailable.
    • Deletes both keyring and prefs fallback copies during cleanup.
    • Exposes a scoped keyring factory override for generated CLI tests.
  • Updated OAuth login to store refresh_token through the helper instead of globalConfig.setRefreshToken().
  • Updated token refresh to read and rotate refresh tokens through the helper.
  • Updated logout/session deletion/local-only classification to detect OAuth sessions from keyring-backed refresh tokens.
  • Updated unauthorized-session cleanup to clear the secure refresh token before removing local session data.
  • Added @napi-rs/keyring to the CLI package templates and regenerated package-lock.json.twig and bun.lock.twig with ./scripts/update-lockfiles.sh cli.
  • Registered the new CLI auth helper template in src/SDK/Language/CLI.php.
  • Added narrow CLI e2e coverage for:
    • keyring-backed refresh token storage,
    • no refreshToken persisted in prefs on the keyring happy path,
    • prefs fallback when keyring is unavailable,
    • OAuth session classification when the refresh token is outside prefs.

Testing

  • php example.php cli
  • composer lint-twig
  • vendor/bin/phpunit --testsuite Unit
  • composer refactor:check
  • vendor/bin/phpunit tests/e2e/CLIBun13Test.php
  • git diff --check

Notes

  • No PR template exists at .github/pull_request_template.md in this repository, so this body uses the project’s existing practical summary/test format.
  • Plaintext prefs fallback is intentionally retained only for keyring failures, per the requested behavior.

@greptile-apps

greptile-apps Bot commented Jun 23, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR moves CLI OAuth refresh tokens out of plaintext prefs.json and into the OS secure credential store via @napi-rs/keyring, while keeping access tokens and expiry in prefs for fast reads. A prefs fallback is retained for environments where the keyring is unavailable.

  • New refresh-token.ts helper — routes all refresh-token reads, writes, and deletions through a keyring-backed factory with a transparent prefs fallback; getPassword() → null (keyring miss) correctly falls through to prefs via an if (refreshToken) guard rather than a ?? \"\" short-circuit.
  • Wired throughout auth flows — OAuth login, token rotation, logout, isLocalOnlySession, and the 401 unauthorized-session path all use the new helper; deleteStoredRefreshToken is consistently called before every removeSession invocation.
  • Test coverage — unit-style e2e checks for keyring-happy-path storage, prefs-fallback when keyring throws, and isLocalOnlySession classification when the token lives in the keyring rather than prefs.

Confidence Score: 5/5

Safe to merge — all keyring read/write/delete paths are correct, the prefs fallback is handled properly, and every session deletion flow calls deleteStoredRefreshToken before removing the session.

The core logic in refresh-token.ts is sound: the if (refreshToken) guard after getPassword() ensures a keyring miss (null return) correctly falls through to the prefs fallback rather than masking it. Token rotation in sdks.ts is safe because an empty currentSession would produce an empty refreshToken and trigger the early throw before the write path is reached. The test cleanup is performed at the end of runAuthChecks but not inside a finally block — this is a test-quality concern rather than a production defect.

tests/e2e/languages/cli/test.js — teardown of NODE_ENV and the keyring factory singleton is not guarded by finally.

Important Files Changed

Filename Overview
templates/cli/lib/auth/refresh-token.ts New helper that routes refresh-token reads/writes through the OS keyring with a prefs fallback; logic is correct — null from getPassword() correctly falls through to prefs via the if (refreshToken) guard.
templates/cli/lib/sdks.ts Replaces direct globalConfig.getRefreshToken() calls with keyring-backed helpers; the write path setStoredRefreshToken(currentSession, ...) is safe because an empty currentSession would have produced an empty refreshToken and caused an early throw before token rotation is reached.
templates/cli/lib/auth/session.ts Wires logout, unauthorized-session cleanup, and isLocalOnlySession through the keyring helpers; deleteStoredRefreshToken is consistently called before removeSession on every deletion path.
templates/cli/lib/client.ts Adds deleteStoredRefreshToken before removeSession in the 401 unauthorised-session cleanup path.
templates/cli/lib/auth/login.ts Replaces globalConfig.setRefreshToken with setStoredRefreshToken at OAuth device-login completion.
tests/e2e/languages/cli/test.js Adds keyring storage, prefs-fallback, and classification tests; restoreKeyringEntryFactory and NODE_ENV teardown are called at the end of runAuthChecks but not inside a finally block.
src/SDK/Language/CLI.php Registers the new refresh-token.ts template as a copy scope entry.
templates/cli/package.json Adds @napi-rs/keyring ^1.3.0 as a runtime dependency and marks it --external in all three esbuild targets.
tests/e2e/Base.php Registers two new expected test outcomes for the keyring storage and prefs-fallback checks.

Reviews (2): Last reviewed commit: "Store CLI OAuth refresh tokens in keyrin..." | Re-trigger Greptile

Comment thread templates/cli/lib/auth/refresh-token.ts
Comment thread templates/cli/lib/auth/refresh-token.ts
Comment thread tests/e2e/languages/cli/test.js Outdated
@ChiragAgg5k ChiragAgg5k force-pushed the cli-refresh-token-keyring branch from 262a1f3 to d7143dd Compare June 23, 2026 12:41
@ChiragAgg5k ChiragAgg5k merged commit eb46dc4 into main Jun 23, 2026
59 checks passed
@ChiragAgg5k ChiragAgg5k deleted the cli-refresh-token-keyring branch June 23, 2026 13:01
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant