fix(dashlane-plugin): fail fast on a locked vault, add allowMissing option - #989
Merged
Conversation
…i calls, add onLocked option) On a locked vault, dcli prompts for the master password on a pipe that never answers, so any load whose schema contains a dashlane() entry hung forever, even for optional items the current command doesn't need. - run every dcli call with stdin closed so interactive prompts hit EOF and fail immediately instead of blocking - add a spawn timeout to every dcli call as a backstop (default 30s, configurable via @initDashlane(timeoutMs=...)); timeouts map to the existing locked-vault ResolutionError - add @initDashlane(onLocked=error|warn), default error. With warn, the locked-vault error is thrown as a warning so optional dashlane() items resolve empty and the load can still pass; required items still fail as empty - varlock core: warning-severity resolution errors no longer skip required/optional handling (previously they bypassed the required check entirely)
…fails A locked/unavailable vault is a hard failure regardless of required/optional. Only a reference that genuinely does not exist in the vault can resolve empty, and only when allowMissing is set (per item or as an instance default in @initDashlane), matching the other secrets plugins. Reverts the core change that let warning-severity resolution errors fall through to required/optional handling.
Contributor
|
The changes in this PR will be included in the next version bump.
|
Deploying with
|
| Status | Name | Latest Commit | Preview URL | Updated (UTC) |
|---|---|---|---|---|
| ✅ Deployment successful! View logs |
varlock-website | 2e97173 | Commit Preview URL Branch Preview URL |
Aug 07 2026, 08:29 PM |
![pullfrog[bot]](https://avatars.githubusercontent.com/in/1768019?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Important
The timeout backstop can still wait forever when dcli does not terminate on SIGTERM; this should be fixed before merging because bounded execution is the central behavior of the PR.
Reviewed changes in both commits through 2e971733, covering the Dashlane process lifecycle, missing-entry semantics, tests, release note, and user documentation.
- Bounded CLI calls: closes stdin for all asynchronous
dclicalls and adds a configurable timeout with locked-vault guidance. - Missing-entry policy: adds instance-level and per-call
allowMissingcontrols while preserving hard errors for lock, authentication, and timeout failures. - Coverage and docs: adds fake-CLI scenarios for prompts, hangs, locks, and missing entries, and documents the new behavior in both Dashlane references.
azure/gpt-5.6-sol ｜ 𝕏
commit: |
The spawn timeout option only sends SIGTERM and the promise still waited on the exit event, so a dcli that ignores SIGTERM would hang the load past the advertised timeout. Now an explicit timer sends SIGTERM at timeoutMs, then after a 2s grace period sends SIGKILL, releases the pipes, and rejects without waiting on exit. Adds a fake-dcli mode that traps SIGTERM to cover this.
Contributor
There was a problem hiding this comment.
✅ No new issues found.
Reviewed changes since the prior Pullfrog review at 2e971733, focused on the owned process deadline and its regression coverage.
- Owned the deadline: replaced Node's signal-only spawn timeout with explicit
SIGTERMandSIGKILLescalation that rejects without waiting for child exit. - Covered ignored termination: added a fake
dclimode that ignoresSIGTERMand verified the plugin still fails within the bounded timeout window.
azure/gpt-5.6-sol ｜ 𝕏
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
2 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.
Builds on #959 by @anuragbanerjee (the hang fix is cherry-picked from there with authorship preserved), with the failure-handling behavior reworked.
Problem
dashlane()resolves by spawningdcli readwith piped stdio and no timeout. On a locked vault,dcliprompts for the master password on a pipe that never answers, so anyvarlock load/varlock runwhose schema contains adashlane()entry hangs forever.Changes
dclicalls (from fix(dashlane-plugin): don't hang forever on a locked vault (bound dcli calls, add onLocked option) #959): stdin is closed on every spawn so interactive prompts fail immediately, plus an explicit deadline as a backstop (default 30s, configurable via@initDashlane(timeoutMs=...)): SIGTERM attimeoutMs, escalating to SIGKILL and rejecting after a short grace period even if the child never exits. A locked vault now fails fast with an actionable error instead of hanging.allowMissingoption, matching the other secrets plugins (infisical, 1password, pass, etc.): settable per item (dashlane("dl://...", allowMissing=true)) or as an instance default in@initDashlane. When set, a reference that does not exist in the vault resolves empty instead of failing. Normal required/optional handling then applies, so a@requireditem still fails as empty.What changed vs #959
#959 added
@initDashlane(onLocked=warn), which threw the locked-vault error with warning severity and changed core so warning-severity resolution errors fall through to required/optional handling. That coupled fetch-failure tolerance to@optional: a locked vault or bad reference on an optional item would silently resolve empty.This PR keeps failure-to-fetch a hard error regardless of required/optional. A locked or unavailable vault (including timeouts) always fails the item, even with
allowMissingset. Only a genuinely missing entry can resolve empty, and only whenallowMissingis opted into. The core change inconfig-item.tsis reverted; this is now purely a plugin change.Tests
Locked-vault tests drive a fake
dclishim: blocked master-password prompt fails immediately, hung call fails withintimeoutMs(including adclithat traps SIGTERM), locked vault fails even optional/allowMissing items. NewallowMissingtests cover per-call, instance default, per-call override, and required-still-fails. Docs updated in the plugin README and website plugins page.Closes #959