[PM-35788] fix: fail fast with clear error when vault is locked or unauthenticated#182
Open
tecnologicachile wants to merge 6 commits into
Open
Conversation
|
Thank you for your contribution! We've added this to our internal tracking system for review. Details on our contribution process can be found here: https://contributing.bitwarden.com/contributing/pull-requests/community-pr-process. |
bitwarden-bot
changed the title
The `create_item` and `edit_item` tools currently expose only name, type,
login/card/identity/secureNote and folderId. This forces MCP consumers to
drop down to the `bw` CLI directly whenever they need to:
- Create or update a vault item with custom fields
- Put an item inside an organization + collection
- Rename or migrate a custom field on an existing item
The underlying `bw` CLI already understands all three (fields are part of
the item JSON; organizationId/collectionIds live in the same object; the
CLI also needs --organizationid as a flag at create time), so this PR just
surfaces them through the MCP schema and threads them through the handler.
Changes
-------
- src/utils/types.ts: extend BitwardenItem with fields, organizationId,
collectionIds.
- src/schemas/cli.ts: new customFieldSchema (name + optional value + type
0..3) and extensions to createItemSchema/editItemSchema.
- src/handlers/cli.ts:
* handleCreateItem copies organizationId/collectionIds/fields into the
JSON item and appends `--organizationid <id>` when present.
* handleEditItem updates organizationId/collectionIds and replaces the
fields array on the existing item when one is supplied (callers that
want to merge should read the item first and merge client-side).
- src/tools/cli.ts: exposes the new parameters to MCP clients for both
create_item and edit_item.
Backward compatible: all new parameters are optional and existing calls
behave identically when they're omitted.
Tools that require an unlocked vault (list, get, create_item, edit_item, delete, and others) currently call `bw` directly. When the vault is locked or the user is not authenticated, the CLI either times out waiting for a response or returns a generic stderr message that is difficult for an LLM to interpret and act on. This commit adds a pre-flight vault status check to every CLI tool that requires an unlocked vault: • A new `ensureVaultUnlocked()` utility function in `src/utils/cli.ts` calls `bw status` (which always responds immediately regardless of vault state) and parses the `status` field. • When the vault is **locked**, it returns a structured error: "Vault is locked. Call the 'unlock' tool with your master password to unlock it, then retry." • When the session is **unauthenticated**, it returns: "Not logged in to Bitwarden. Use 'bw login' to authenticate first, then retry." • If status cannot be determined (unexpected output, JSON parse error), it returns `null` and the actual command runs as before — no regression in error handling. The guard in `src/index.ts` is applied through a `VAULT_REQUIRED_CLI_TOOLS` set. Excluded from the check: • `status`, `lock`, `generate` — work without an unlocked vault. • All `org_*` tools — use the Bitwarden REST API with their own OAuth2 credentials, not the local CLI vault session.
The ensureVaultUnlocked() guard returns an error message telling the LLM to call the 'unlock' tool, but that tool did not exist. This adds it: - unlockSchema: requires a master password (passed via env var, never as a shell argument, to avoid exposure in the process list) - unlockTool: exposed to the LLM with clear description - handleUnlock: calls 'bw unlock --passwordenv ... --raw', then updates process.env.BW_SESSION so subsequent CLI tools work without restart - executeCliCommand: accepts optional extraEnv to pass extra variables to the spawned process - security allowlist: adds 'unlock' as a valid Bitwarden command
bw CLI often emits Node.js deprecation warnings to stderr on a successful unlock, causing errorOutput to be set. The previous check `response.output && !response.errorOutput` would then skip saving the session key, leaving the vault effectively locked for subsequent calls. Use the presence of output (the raw session key) as the sole success signal instead.
- Success: "Vault unlocked successfully. You can now retry the operation." - Failure: actionable message telling the LLM to ask the user to confirm the password and retry, instead of exposing a raw bw CLI error - Tool description: clarifies that the session persists automatically on success and what to do on failure
Adopts upstream's native OS dialog flow (src/utils/unlock.ts) for environments with a graphical interface, while keeping the password parameter path for headless/server environments: - No password provided → native OS dialog (osascript / zenity / kdialog) The master password never reaches the LLM or MCP protocol. - Password provided → headless path via --passwordenv (existing behavior) For servers and LXC containers without a display. If the GUI dialog fails because the environment is headless, the LLM receives a clear message instructing it to retry with the password parameter instead.
tecnologicachile
force-pushed
the
fix/fail-fast-when-vault-locked
branch
from
63c3f5c to
987a707
Compare
withinfocus
changed the title
Contributor
|
@tecnologicachile take a look at what just merged in and give it a try -- there's a whole new unlock flow now. |
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
3 participants
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.
Summary
When the vault is locked or the user is not authenticated, CLI tools like
list,get,create_item, etc. callbwdirectly. The CLI either times out or returns a generic stderr message that is hard for an LLM — or a human — to interpret and act on.This PR adds a pre-flight vault status check to every CLI tool that requires an unlocked vault.
Changes
src/utils/cli.ts— new exported functionensureVaultUnlocked():bw status(always fast, works regardless of vault state)null(safe to proceed) when unlockednullon unexpected output so no existing behaviour regressessrc/index.ts— vault guard in the tool dispatcher:VAULT_REQUIRED_CLI_TOOLSset lists every CLI tool that needs the vaultensureVaultUnlocked()status,lock,generate(work without vault) and allorg_*tools (use the Bitwarden REST API with their own OAuth2 credentials, not the local CLI session)Error messages
lockedVault is locked. Call the "unlock" tool with your master password to unlock it, then retry.unauthenticatedNot logged in to Bitwarden. Use "bw login" to authenticate first, then retry.Test plan
list,get, or any other vault-required tool returns the locked error immediately without waiting forbwto time outstatus,lock, andgenerateare unaffected (no pre-flight check)npm run buildcli-commands.spec.tswhich makes realbwcalls and was already failing before this PR):npm test🤖 Generated with Claude Code