fix: do not require a keychain token read for commands that do not need auth#2947
Draft
spenserhale wants to merge 1 commit into
Draft
Conversation
5 tasks
…ed auth On macOS over SSH (and other headless sessions), reading the stored token from the OS keychain fails because the keychain ACL authorization prompt requires a GUI. On trunk, rootCmd() in src/bin/vip.js called Token.get() unconditionally before checking whether the invocation even needs auth, so `vip -h`, `vip -v`, `vip logout`, and local `vip dev-env ...` crashed with an opaque "Unexpected error" even though none of them require a token. keytar's native rejection carries no stack frames, and the crash happened before argument parsing so --debug could not help. Changes: - vip.js: compute the argv classification flags before fetching the token, and add resolveToken() which skips Token.get() entirely for commands that do not consult the token (help, version, logout, tokenless dev-env, custom-deploy with key). When the token is actually needed and the keychain read fails, surface an actionable error explaining the SSH/headless keychain limitation instead of crashing. - vip.js: stop doesArgvHaveAtLeastOneParam() from scanning past the `--` terminator so `vip wp <env> -- --help` is not misclassified as top-level help. - token.ts: Token.uuid() now tolerates an unreachable keychain by returning a stable per-process UUID, so analytics cannot crash an otherwise-working command. - logout.ts: tolerate an unavailable keychain by skipping the server-side logout call when the stored token cannot be read or is invalid, and by wrapping Token.purge() so local cleanup always completes. - startup-args.ts: extract doesArgvHaveAtLeastOneParam() and isTokenReadRequired() into a pure, unit-tested module so the startup classification logic is covered outside the bin script.
spenserhale
force-pushed
the
fix/skip-token-read-for-tokenless-commands
branch
from
f0c3695 to
0de1bcd
Compare
|
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
1 participant
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.
Description
Fixes a startup crash affecting commands that don't require authentication, in sessions where the OS keychain is unavailable.
The bug: on macOS over SSH, every
vipcommand — includingvip -h— fails with:Root cause:
rootCmd()insrc/bin/vip.jscallsToken.get()unconditionally, before checking whether the invocation needs auth at all. Reading the stored token from the macOS keychain requires an ACL authorization prompt that can only be shown in a graphical session, so over SSH keytar rejects with a bare native error (no stack frames). The crash happens before argument parsing, so--debugcan't produce any more information.The fix:
help/-h/--help,-v/--version,logout,dev-envwithout an env argument, and custom deploys keyed byWPVIP_DEPLOY_TOKEN.vip -hnow performs zero keychain access.DEBUG=@automattic/vip:*pointer) instead of the opaque crash.Token.uuid()(analytics) degrades to a stable per-process UUID when the keychain is unavailable, so telemetry cannot crash an otherwise-working command.vip logouttolerates an unreadable keychain: it skips the server-side/logoutcall when there is no readable valid token and always completes local cleanup.doesArgvHaveAtLeastOneParam()now stops scanning at the--terminator, so e.g.vip wp <env> -- --helpis no longer misclassified as top-level help.doesArgvHaveAtLeastOneParam()and the token-read predicate) are extracted into a puresrc/lib/cli/startup-args.tsmodule with unit tests covering the--terminator handling and the full skip-flag matrix.Related: pairs with #2946, which adds
VIP_CLI_TOKENauthentication for these environments; the two are independent and address different use cases.Changelog Description
Fixed
vip -h,vip -v, and localvip dev-envcommands) in sessions where the OS keychain is unavailable, such as over SSH.Pull request checklist
Steps to Test
npm run build.DEBUG=@automattic/vip:keychain node ./dist/bin/vip.js -h— help prints and the debug output shows no keychain activity.node ./dist/bin/vip.js -vandnode ./dist/bin/vip.js dev-env info— no keychain access required.vip -hcrashes with "Unexpected error"; with this change it prints help, and auth-requiring commands (e.g.vip app list) print an actionable keychain message instead of crashing.