feat: add Bitwarden Password Manager provider - #166
Open
ashebanow wants to merge 62 commits into
Open
Conversation
Integrate Bitwarden Password Manager (bw CLI) as a secretspec provider. The provider supports vault-wide item access across all Bitwarden item types (Login, Card, Identity, SSH Key, Secure Note) with smart field extraction, URI-based configuration (bitwarden://[org@]collection?server=...&type=...&field=...), and session-based authentication via BW_SESSION. Co-Authored-By: deepseek-v4-pro <noreply@deepseek.com>
Upstream now ships a native BWS SDK provider (bws.rs), so the unified Password Manager + Secrets Manager provider must be reduced to Password Manager only. Remove the BitwardenService enum, BWS-specific config fields, execute_bws_command method, and bws:// scheme handling from register_provider! and TryFrom. Co-Authored-By: deepseek-v4-pro <noreply@deepseek.com>
Upstream's Provider trait has evolved from dynamic dispatch with (project, key, profile) signatures to a static-registration pattern with Address, ProviderUrl, and ProviderCredentials. Rewrite BitwardenConfig's TryFrom to accept &ProviderUrl, add convention_address/supported_coords/with_credentials/uri, and update get/set to accept Address<'_> parameters resolved via resolve_coords. Field resolution respects the native address coordinate first, then BITWARDEN_DEFAULT_FIELD, then config. Co-Authored-By: deepseek-v4-pro <noreply@deepseek.com>
Wire the bitwarden module into the provider registry with a bw feature flag (bw = ["dep:base64"], enabled by default). The base64 dependency encodes JSON for the bw CLI's item create/edit stdin path. Add four unit tests covering provider creation, collection, org-collection URI parsing, and bws:// scheme rejection. Co-Authored-By: deepseek-v4-pro <noreply@deepseek.com>
The bitwarden.md page was drafted for a dual-service (bw + bws) provider. Strip empty code blocks, the Secrets Manager access token section, and a stray bullet left behind after BWS content removal. Co-Authored-By: deepseek-v4-pro <noreply@deepseek.com>
Add .pi/ to .gitignore for pi-agent task output. The collapsible_if lint in config::migrate_macos_config is a pre-existing upstream warning surfaced by clippy during this branch's development. Co-Authored-By: deepseek-v4-pro <noreply@deepseek.com>
Replace the manual setup requirements with an auto-provisioning setup_test_data() function that creates Bitwarden items if they don't exist, and registers an EXIT trap to clean them up unless --keep-test-data is passed. Co-Authored-By: deepseek-v4-pro <noreply@deepseek.com>
Replace invalid TOML keys ("Deploy SSH Key", etc.) with valid
bw_integration_test_* identifiers and ref = { item, field }
mapping, matching the new Provider API address model.
Co-Authored-By: deepseek-v4-pro <noreply@deepseek.com>
Co-Authored-By: deepseek-v4-pro <noreply@deepseek.com>
Co-Authored-By: deepseek-v4-pro <noreply@deepseek.com>
Co-Authored-By: deepseek-v4-pro <noreply@deepseek.com>
The entry landed in the already-released 0.15.0 section during the rebase; the provider targets the next release.
The provider wraps the `bw` CLI, so the id now matches the tool it drives and the existing cargo feature, mirroring how the bws provider is named after the bws CLI. URIs change from bitwarden:// to bw://. The provider is unreleased, so there is no compatibility impact. Module, source file, and docs page follow the id (provider::bw, providers/bw.md). BITWARDEN_DEFAULT_TYPE/FIELD env vars keep their prefix to stay clear of the bw CLI's own BW_* namespace.
Add the Bitwarden Password Manager provider to every listing the adding-providers checklist names: sidebar and llms.txt description, concepts and reference tables (with security row), landing page metadata/hero/bento (provider count 14 -> 15), quick-start and README config-init examples, and the rustdoc provider summary. All entries carry the 0.16+ label; the provider page gains the version notice. The landing grid's existing plain 'Bitwarden' label becomes 'Bitwarden Secrets Manager' to disambiguate the two Bitwarden providers.
bw can prompt on stdin (e.g. for the master password when a session is missing or expired). stdin is null or closed on our invocations so a prompt could not hang us, but --nointeraction makes bw fail fast with its own clear error instead of a confusing EOF-related one, which matters in CI and other headless contexts.
ashebanow
marked this pull request as ready for review
July 18, 2026 02:15
Implements the FUTURE WORK plan from bitwarden_integration.sh: vaultwarden + caddy TLS proxy (the 2026+ bw CLI refuses plain http servers, so an internal self-signed cert is required), fixture account registered via the identity API (vaultwarden_bootstrap.py implements the client-side registration crypto bw doesn't expose), bw CLI isolated via BITWARDENCLI_APPDATA_DIR so the developer's real config is untouched. No repository secrets needed; works on fork PRs. Verified locally: full bitwarden_integration.sh suite 19/19 PASSED end-to-end from a cold start (containers, registration, login, suite, cleanup) on macOS + colima. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015CDSY4yJXvof9Am8acxzHf
Contributor
|
Vaultwarden testing done, as promised on #15: the full And since you asked about automating it: I've opened ashebanow#2 against your branch implementing the FUTURE WORK plan from the script header — disposable Vaultwarden + fixture account, no repository secrets, fork-PR-friendly. Two gotchas discovered on the way, handled in the harness:
|
domenkozar
reviewed
Jul 23, 2026
One test per finding (R1 linked-field type 3 poisons writes; R2 named custom field lost on create; R3 wrong default field on non-login update; R4 case-sensitive update vs case-insensitive read). Each reports REPRODUCED/FIXED and the set exits 0 only when all findings are fixed — opt-in via RUN_REGRESSIONS=1 in the harness until then. Verified against disposable Vaultwarden: currently 0 fixed / 4 reproduced, matching the review descriptions verbatim. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_015CDSY4yJXvof9Am8acxzHf
Contributor
|
Following up on the review: the four findings that are reproducible against a live vault are now regression tests in the Vaultwarden harness (added to ashebanow#2) — one test per finding, each reporting Current run against disposable Vaultwarden confirms every one of them verbatim: So @ashebanow has an executable checklist for those four ( |
Author
|
I'm traveling this long weekend, not sure how much time I'll have. But b as
CK Tuesday at the latest.
…On Thu, Jul 23, 2026 at 9:45 AM bsorescu ***@***.***> wrote:
*bsorescu* left a comment (cachix/secretspec#166)
<#166 (comment)>
Following up on the review: the four findings that are reproducible
against a live vault are now regression tests in the Vaultwarden harness
(added to ashebanow#2 <ashebanow#2>) —
one test per finding, each reporting REPRODUCED/FIXED, exiting green only
when all are fixed.
Current run against disposable Vaultwarden confirms every one of them
verbatim:
── R1: linked custom field (type 3) poisons unrelated writes ──
REPRODUCED R1 — any write aborts while a linked field exists (Unknown field type: 3)
── R2: named custom field preserved when creating a new item ──
REPRODUCED R2 — value not readable via field=api_key after create (stored in login.password instead)
── R3: type-specific default on update (secure note) ──
REPRODUCED R3 — set wrote a password field; get still returns 'old-note-value'
── R4: case-insensitive field matching on update ──
REPRODUCED R4 — update added a duplicate field; get still returns 'old-value'
Findings fixed: 0 · reproduced: 4
So @ashebanow <https://github.com/ashebanow> has an executable checklist
for those four (RUN_REGRESSIONS=1 tests/vaultwarden_harness.sh) — they'll
flip to FIXED as the fixes land. The collection-filtering finding needs an
org + two collections in the fixture account; happy to extend the bootstrap
for that as a follow-up if useful. On the self-hosted config finding: the
only supported mechanism the CLI offers is bw config server (which
requires being logged out), so the provider likely needs to shell that out
— or document it as a user prerequisite, which is what Kamal's bitwarden
adapter effectively does.
—
Reply to this email directly, view it on GitHub
<#166?email_source=notifications&email_token=AAACX7Y6NB7OMN67M32A2J35GI6J5A5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBWGEYDCMRRHE2KM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5061012194>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACX73WHV2OLXU7QP7CVW35GI6J5AVCNFSNUABGKJSXA33TNF2G64TZHMYTAMJTHE4DINJRGI5US43TOVSTWNBZGE2TKOBSG4YDBILWAI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Member
|
@ashebanow I'm planning to do a 0.17 release on Monday if you'd like to get this in. No issue if we target 0.18. |
test: fully-automated integration run against disposable Vaultwarden
|
Fwiw this is the provider I am missing to "complete the puzzle" for me, so I am very very much looking forward to this landing. Cheers to all involved. |
Author
|
I'm at a family event this weekend, let's target 0.18, thanks!
…On Fri, Jul 24, 2026 at 3:11 PM Domen Kožar ***@***.***> wrote:
*domenkozar* left a comment (cachix/secretspec#166)
<#166 (comment)>
@ashebanow <https://github.com/ashebanow> I'm planning to do a 0.17
release on Monday if you'd like to get this in. No issue if we target 0.18.
—
Reply to this email directly, view it on GitHub
<#166?email_source=notifications&email_token=AAACX77CKFUGM2Q6OOS7ASD5GPNILA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMBXGM3DGNJRGQZKM4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5073635142>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACX7ZBMHUTG7PQST47ITL5GPNILAVCNFSNUABGKJSXA33TNF2G64TZHMYTAMJTHE4DINJRGI5US43TOVSTWNBZGE2TKOBSG4YDBILWAI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
…back Every fixture in this suite is built with raw `bw`, so nothing exercised an item the provider itself created. The creation tests asserted only on the "Secret ... saved" message, which cannot tell a stored secret from a discarded one -- the exact blind spot R2 lived in. For each of the five item types: create through the provider, read it back, update the item the provider just made, and read it back again. The secrets carry no `ref`, so the item is named after the key and the provider both writes and finds it on its own terms. This found a real defect on its first run: SSH key items were unreadable after create. See #3. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
An SSH key item has to carry a non-empty string in all three of privateKey,
publicKey and keyFingerprint. `ssh_key_template` set the two it was not
writing to null, and no server accepts that: Bitwarden cloud refuses the
request ("invalid type: unit value, expected a valid string"), while
Vaultwarden 1.37.0 accepts the upload, stores `sshKey: null` and silently
discards the secret -- `set` reports success and `get` then returns
"[error: cannot decrypt]".
Measured on both servers: arbitrary non-empty strings are accepted, so this is
about presence rather than parseable key material. Initialise all three
members to "(not set by SecretSpec)" and overwrite only the addressed one.
Pinned by two tests. ssh_key_template_never_emits_a_null_member walks every
addressable field, including an unrecognised one that routes to `fields[]`
while the sshKey object still has to be well-formed;
ssh_key_template_puts_the_secret_in_the_addressed_member checks the
placeholders do not displace the value.
Fixes #3.
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
c59a8f1 applied clippy's collapsible_if suggestion to migrate_macos_config in secretspec/src/config.rs and added .pi/ to .gitignore. Neither belongs in the Bitwarden provider PR: config.rs is a file the provider never touches, and .pi/ is a local scratch directory. The lint is a warning, not an error. Upstream's clippy hook runs without -D warnings, so main carries the same warning today and CI stays green; nothing here depends on the fix. Reverting restores both files to their upstream state, so the PR diff no longer touches them. .pi/ is excluded through .git/info/exclude instead. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Upstream cachix#195 converted the bws provider to shell out to the `bws` CLI and removed the `bitwarden` SDK crate along with 799 lines of Cargo.lock. This branch's earlier merge of upstream/main (d8865aa) kept our side of secretspec/Cargo.toml, restoring `bitwarden` and `rustls` as optional dependencies and `bws = ["dep:bitwarden", "dep:rustls"]`. Nothing uses either one. bws.rs at this commit is the CLI implementation with no reference to the SDK, and `rustls` appears nowhere in secretspec/src. The declarations pulled roughly 99 crates (bitwarden-*, rusqlite, zxcvbn, mockall and their trees) into every build and accounted for almost all of this PR's lockfile churn: the Cargo.lock delta drops from +1453 to +10/-9. Restores `bws = []` to match upstream. The `base64` dependency and the `bw` feature stay, since bw.rs uses them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Brings in secret scopes (--scope), the Scaleway Secret Manager provider, capped get_each concurrency (SECRETSPEC_PROVIDER_CONCURRENCY, default 8) and the Vault/OpenBao client reuse and connect retries. Four conflicts, each one both sides appending to the same list: - secretspec/Cargo.toml: default features, ours adds `bw`, upstream adds `scaleway`; kept both. - docs/astro.config.ts: the starlightLlmsTxt provider sentence; kept both entries in their respective positions. - CHANGELOG.md: two Unreleased provider bullets; kept both, upstream's first so its entry stays where it was written. - Cargo.lock: regenerated. bw.rs did not conflict, and the Provider trait is unchanged: upstream's provider/mod.rs edit is internal to get_each. The bw provider does not override get_many, so it now inherits the concurrency cap, which suits a provider that spawns one `bw` CLI process per get. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
The `secretspec config init` mini-terminal on the landing page listed `bw: Bitwarden Password Manager` with no version, in both the hero and the copy further down. Every other unreleased provider in those same blocks carries one (kdbx, openbao, age, systemd-credential), and quick-start.mdx already labelled bw, so the landing page was the odd one out. The sidebar entry spelled its version inline in the label. Every other versioned entry in astro.config.ts uses a badge, so this one now does too. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
`bw create org-collection` returns the id the server assigned, but `bw list collections` answers from the locally-synced vault. A single blind `bw sync` after creating dev-secrets and prod-secrets was enough most of the time; when it wasn't, the fixture still printed both ids and the collection addressing suite failed 9 of 11 with "No collection matching 'prod-secrets' is visible", naming only the organization's `default` collection. Poll until both collections are listable, the same shape as the organization wait immediately above, and fail naming them if they never arrive. Collection addressing goes from 2 passed / 9 failed to 11 passed / 0 failed. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
OK, a large set of changes incoming, just waiting for the integration tests to pass after merging latest from this repo. Before it arrives, I want to thank @bsorescu for the vaultwarden help, and @domenkozar for his fantastic code review. Its clear to me that I was too trusting of my AI code to do the merge of all of secretspec's changes since the last time the code was submitted, and that I had not paid enough personal attention to the changes in rules for providers. The platform has really matured impressively. My response to all those review comments: add tests, and more tests. Those tests in turn found more issues, which have been addressed. I had Claude Opus 5 do a detailed code review as well, looking at the code with a strong critical eye. Even so, there are holes: the biggest being that the vaultwarden integration tests are still required to be run manually, primarily since the setup requirements are still not sufficiently automated to allow for regular, unattended runs IMHO. We should file a separate bug to improve and integrate these tests into the larger integration test process. There is also a vaultwarden issue with ssh entries that we found and will report to vaultwarden. |
Brings the branch up to the real current main, which had moved 42 commits past the ref the previous merge used: the 0.17.0 release, the Dashlane and SOPS providers, cached provider aliases with fallback routes, and the Go SDK libdbus and release-packaging fixes. Six conflicts, each one both sides adding a provider to the same list: - secretspec/Cargo.toml: default features, ours adds `bw`, upstream adds `sops`; kept both. - .github/workflows/test.yml: the standalone-feature loop; kept both. - secretspec/src/provider/mod.rs: the module doc provider list; kept both. - docs/astro.config.ts: the starlightLlmsTxt sentence; took upstream's, which adds Dashlane and SOPS, and reinserted Bitwarden Password Manager. - docs/src/pages/index.astro: providerMetadata; took upstream's entries, including their rename of the `bws` label to plain "Bitwarden", and added ours alongside. - CHANGELOG.md: upstream cut 0.17.0, so its Unreleased section is now the short post-release one while ours still held everything 0.17.0 shipped. Took upstream's file whole and reinserted only the bw bullet under the new Unreleased, rather than reconciling two divergent Unreleased sections. The merge also carries upstream's removal of `.envrc` (7cb830b, direnv replaced by `devenv allow`) and the empty root `secretspec.toml` (a479b4f). Those are the only two deletions. bw.rs did not conflict and the Provider trait is unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Upstream removed the checked-in root secretspec.toml in a479b4f, so two claims in this comment no longer describe the repository: that the harness runs "where the project's own secretspec.toml lives", and that the unconditional write "has already deleted that tracked file". The guard still matters, for a reason that does not depend on upstream tracking the file. Anyone dogfooding secretspec inside its own checkout can have an untracked secretspec.toml at the root, and upstream's own tests/cli-integration.sh writes one there too. Comment only. The guard is already conditional and correctly no-ops when there is nothing to preserve. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
2 tasks
Contributor
|
Independent verification of the review fixes, on
Full harness exit 0. Nice work — from where I sit this is ready for 0.18. Two environment findings from the runs, in case they're worth a small follow-up:
|
domenkozar
reviewed
Jul 30, 2026
domenkozar
left a comment
domenkozar
left a comment
Member
There was a problem hiding this comment.
Eight additional findings validated against the current PR head.
| else | ||
| echo -e "\n${RED}❌ SOME TESTS FAILED${NC}" | ||
| echo "Please review the failed tests above." | ||
| fi |
Member
There was a problem hiding this comment.
P2 — Exit nonzero when integration assertions fail — When any run_test increments TESTS_FAILED, this branch only prints a message and the final echo still makes the script exit successfully. Consequently vaultwarden_harness.sh treats a failing integration suite as green, allowing provider regressions to pass the documented harness.
The review found six ways an address could resolve to the wrong secret. All six shared a cause: the provider treated a name as a search term rather than as a coordinate. - Reads took the first result of `bw list --search`, which matches substrings across name, username, URI and notes. A request for API_KEY could answer with API_KEY_OLD, non-deterministically. - Writes fell back to a substring match when no exact name existed, so setting API_KEY with only OLD_API_KEY present overwrote OLD_API_KEY and never created anything. Unrecoverable, and reported as success. - `?type=` was consulted only when creating an item, so it could not tell a Card from a same-named Login on a read or an update. - An explicit Secure Note field that was absent fell through to the legacy `value` field and then to the note body, answering a request for one secret with another. The other four item types already returned None here. - Creation did not recognise the built-in field aliases that reading and updating do, so `set --field exp_month` stored a custom field while `get` read the untouched card.expMonth and found nothing. - `uri()` dropped `type`, `field` and `folder`. SecretSpec fingerprints cached routes with that string, so repointing a source from `?field=password` to `?field=api_key` left the cached password fresh and served it for the API key. Item resolution is now one function used by reads and writes alike: full-name match, then the addressed type, then a hard error listing the colliding ids rather than a guess. This is what `bw get item` does; the CLI accepts a substring only because it prints the candidates for a human to choose between, a backstop a config file does not have. Names still fold case, because `bw` folds case -- with `to_lowercase` rather than `eq_ignore_ascii_case`, so `ÜBERBLICK` stays addressable as `überblick`. Organization and collection resolution now folds the same way; it was ASCII-only, and diverged from the CLI for non-ASCII names. Also found while verifying: `?folder=` was dropped from `uri()` too, `server=` was interpolated without percent-encoding, and `bw://?org=` alone rendered as `bw://myorg@`, whose empty host re-parses to no organization at all. An unsupported `?type=` or an unknown query key is now rejected when the address is parsed. Both were discarded silently, so `?type=sshkee` created a Login and `?feild=api_key` did nothing whatsoever. The alias tables that reading, updating and creating each maintained by hand are now one table, which is what let them drift apart. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Three faults, each of which made a run report something other than what happened. `ensure_item` adopted a pre-existing item as a fixture when the name was already taken. The names were ordinary enough to collide in a real vault -- "Test Database", "GitHub API", "Payment Gateway" -- and the suite advertises running against one. The adopted id never reached CREATED_ITEM_IDS, so cleanup neither deleted nor restored it, while a later test replaced its password. Fixtures now sit behind a distinctive prefix and the suite refuses to touch a name it did not create. Cleanup swept with `bw list --search`, which matches notes, usernames and URIs as well as names, so any vault item merely mentioning the prefix was deleted. It now matches on the name prefix alone. Verified against a fixture list: "My Real Bank" and "Notes about secretspec-it stuff" survive where they previously would not have. The suite counted failures rather than propagating them, so it ended on a successful `echo` and exited 0 no matter what. vaultwarden_harness.sh reads that status, which left the documented harness unable to fail on a provider regression. The harness now runs every suite and takes the worst status instead of aborting on the first failure under `set -e` -- one unrelated integration failure used to hide every regression finding, which is precisely the report you want when checking what is still reproduced. It also isolates XDG_CONFIG_HOME the way it already isolates BITWARDENCLI_APPDATA_DIR: a `[defaults] profile` in the developer's own config failed every get and set with "Invalid profile", which reads as total provider failure and has nothing to do with the provider. Reported by bsorescu. R5-R12 extend the findings script for this review round, in the shape R1-R4 already use. Each reproduces against a live vault before its fix and reports FIXED after, so the harness stays the thing that decides. The abort path disarms cleanup explicitly. It currently touches nothing only because the EXIT trap is installed one line after setup_test_data; moving that trap earlier -- which reads like a safety improvement -- would turn a check that refuses to modify someone's item into one that deletes it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Case handling is decided per provider in this repo, not globally: env.md documents case-sensitive matching, dashlane.md case-insensitive, and each justifies itself by reference to its own backing store. bw.md said how organization and collection names match but nothing about how the `item` in a `ref` does, so the rule a user needs was the one not written down. Records full-name, case-insensitive matching, the refusal to guess between same-named items, and that `?type=` narrows reads and writes alike -- with the 0.18+ label these pages need, since the site publishes from main ahead of the release. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # .github/workflows/test.yml # CHANGELOG.md # docs/astro.config.ts # secretspec/Cargo.toml
Member
|
@ashebanow 0.18 targeting Monday, let me know when it's ready for review. |
Author
|
changes done, just reviewing and testing. Should be done in a few hours.
…On Fri, Jul 31, 2026 at 8:35 AM Domen Kožar ***@***.***> wrote:
*domenkozar* left a comment (cachix/secretspec#166)
<#166 (comment)>
@ashebanow <https://github.com/ashebanow> 0.18 targeting Monday, let me
know when it's ready for review.
—
Reply to this email directly, view it on GitHub
<#166?email_source=notifications&email_token=AAACX75XWVCRBDP72SQBC2T5HS4FBA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMJUGQ3DIOJZGUZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5144649953>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACX75OYESTMIBVNDOXRNT5HS4FBAVCNFSNUABGKJSXA33TNF2G64TZHMYTAMJTHE4DINJRGI5US43TOVSTWNBZGE2TKOBSG4YDBILWAI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Upstream's error-reporting pass (15e3f72) wrapped stringified errors with context and routed them through `display_error_chain` across sixteen providers. It could not touch this one, which does not exist upstream, so after the merge `bw` was the only provider not following the convention. Three sites returned the CLI's stderr as the entire error. A `get` or a `set` runs several `bw` calls, so the reader was left to guess which had failed, and the exit status was dropped on the floor. These now name the invocation and its status, and fall back to stdout when stderr is empty -- `bw` is not consistent about which stream carries a diagnostic. Naming the command is safe: secret values never reach argv, only base64 JSON on stdin. `display_error_chain` does not help there, because a subprocess's output is not a `std::error::Error` and has no `source()` to walk. It does help at the seven sites that stringified a real error bare -- spawn, stdin write, wait, and UTF-8 decoding -- where the underlying cause was being discarded. Four sites that already had context keep their wording and gain the chain; two of them wrap a SecretSpecError whose Io variant carried a source that was lost. The "bw is not installed" branch and the two authentication messages are untouched: they are better than anything generic, and stay ahead of it. A test covers the install instructions, since that is the sort of thing a rewrap quietly eats. The new tests synthesize a finished process, which needs ExitStatusExt and so is unix-only, gated the way bws gates its own process tests. What they assert is string formatting, which does not vary by platform. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Author
|
I' ve checked in my changes, so the PR is updated, but I haven't had time
to write notes on the PR or your specific comments yet. But you can start
reviewing now if you'd lke.
…On Fri, Jul 31, 2026 at 9:14 AM Andrew Shebanow ***@***.***> wrote:
changes done, just reviewing and testing. Should be done in a few hours.
On Fri, Jul 31, 2026 at 8:35 AM Domen Kožar ***@***.***>
wrote:
> *domenkozar* left a comment (cachix/secretspec#166)
> <#166 (comment)>
>
> @ashebanow <https://github.com/ashebanow> 0.18 targeting Monday, let me
> know when it's ready for review.
>
> —
> Reply to this email directly, view it on GitHub
> <#166?email_source=notifications&email_token=AAACX75XWVCRBDP72SQBC2T5HS4FBA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMJUGQ3DIOJZGUZ2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5144649953>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAACX75OYESTMIBVNDOXRNT5HS4FBAVCNFSNUABGKJSXA33TNF2G64TZHMYTAMJTHE4DINJRGI5US43TOVSTWNBZGE2TKOBSG4YDBILWAI>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
Contributor
|
Independent verification of the round-2 fixes, on
One finding still reproduces: An item created as Everything else looks release-ready from here. I'm closing ashebanow#4 — all three of its changes (fail-closed integration exit, XDG_CONFIG_HOME isolation, round-2 regressions) landed natively in this branch in more complete form. |
Author
|
Well, R11 passes on my machine. But your failure has shown me the test itself is weak, because it depends on 'bw' cli behavior which is version dependent. They've changed behavior fairly recently to strip diacriticals in search (which I'm not convinced is a good way to do this). I'm going to fix up the test to be a little more robust to this. |
Author
|
Also, I should mention that the right way to do this is by using unicode
normalization and compare on the search terms. I've not done this here
because it would add a unicode dependency to the secretspec binary, and I
didn't want to do that without a go ahead from Domen. It also won't agree
perfectly with the 'bw' cli, bit what its doing is sloppy anyhow.
…On Jul 31, 2026 at 11:11:35 AM, bsorescu ***@***.***> wrote:
*bsorescu* left a comment (cachix/secretspec#166)
<#166 (comment)>
Independent verification of the round-2 fixes, on 433aa48 against a
disposable Vaultwarden (macOS, bw CLI 2026.6.0, current
vaultwarden/server:latest), via RUN_REGRESSIONS=1
tests/vaultwarden_harness.sh:
- *Regressions: 11/12 FIXED* — R1–R10 and R12 all green, including the
substring read/write findings, type-narrowed addressing, the secure-note
fallback, built-in fields on create, unknown query-key rejection, and the
fixture-adoption fix.
- *Integration suite: all tests passed* (40 assertions) · *collection
addressing: 11/11* — and the new config isolation means the harness
now survives a developer's ~/.config/secretspec/config.toml, confirmed
with one present.
One finding still reproduces:
REPRODUCED R11 — a non-ASCII name is unreachable in lower case
(got 'Error: × Failed to get secret ╰─▶ Secret 'regr_umlaut' not found')
An item created as Überblick isn't found when addressed as überblick.
Since find_addressed_item now folds with to_lowercase, my read is that
the candidate never reaches it: the bw list items --search <name>
pre-filter (bw.rs:1442) does its own matching, and the lowercase query
returns no candidates for the umlaut name. If that's right, a fallback that
re-lists without --search (or post-filters the full list) when the
narrowed query comes back empty would close it — though as a P2 it may also
be fine to document and ship.
Everything else looks release-ready from here. I'm closing ashebanow#4
<ashebanow#4> — all three of its
changes (fail-closed integration exit, XDG_CONFIG_HOME isolation, round-2
regressions) landed natively in this branch in more complete form.
—
Reply to this email directly, view it on GitHub
<#166?email_source=notifications&email_token=AAACX75PMSOWQGABODGY3735HTONPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMJUGYYDMOJTHAY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5146069381>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AAACX77LUX3QHDQONMYDCE35HTONPAVCNFSNUABGKJSXA33TNF2G64TZHMYTAMJTHE4DINJRGI5US43TOVSTWNBZGE2TKOBSG4YDBILWAI>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
`get` narrowed with `bw list items --search <name>` and treated an empty result as "no such item". That search is the CLI's own fuzzy matcher, and it has been wrong: before bitwarden/clients e1aa943b (2026-07-13, first released in CLI 2026.7.0), `searchCiphersBasic` stripped diacritics from the query but not from the item names it compared against. On any older CLI -- 2026.6.0 shipped three weeks earlier -- `--search überblick` returned nothing for an item named `Überblick`, so the candidate was discarded before this provider ever compared it, and the secret was reported missing. An empty narrowed result now means "the prefilter matched nothing" rather than "the secret is absent", and the read falls back to the full listing. The narrowing stays, because it is worth having on a large vault; it is just no longer authoritative. A genuine miss costs a second `bw` call as a result, which is the deliberate trade. `set` has always listed unfiltered, so both paths now go through one `list_items` helper and consider the same candidates -- the asymmetry where a read could rule out an item a write would have found is gone. Not addressed: `to_lowercase` compares without normalizing, so a name that round-trips as NFD still will not match an NFC `ref`. That is ours rather than the CLI's, needs a normalization dependency, and no report so far points at it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
R5, R6, R7, R11 and R12 all discarded `mk_item`'s exit status, and the script runs without `set -e`, so a fixture that was never created reported exactly what a provider that could not find one reported: `got ''`. That made a third-party report of R11 impossible to diagnose from its output -- the umlaut item is the only non-ASCII value in the file, so it is the one fixture whose creation can fail where the others' cannot, and the two explanations were indistinguishable. `require_item` now creates a fixture and confirms the CLI can see it under that exact name. When it cannot, it lists what did land, with byte-level hex for anything non-ASCII, so a name mangled on the way in -- NFC arriving as NFD -- is visible rather than inferred. The harness also notes when `bw` predates 2026.7.0, whose `searchCiphersBasic` fixed the diacritic normalization that R11 turned out to depend on. A note rather than a gate: the provider no longer depends on that fix, so an older CLI is precisely the environment worth exercising, and refusing to run there would hide the case instead of testing it. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
# Conflicts: # docs/astro.config.ts # secretspec/Cargo.toml
Author
|
All changes in now, and individual comments replied to. Thanks to you both
again for all your help.
On Fri, Jul 31, 2026 at 12:28 PM Andrew Shebanow ***@***.***>
wrote:
… Also, I should mention that the right way to do this is by using unicode
normalization and compare on the search terms. I've not done this here
because it would add a unicode dependency to the secretspec binary, and I
didn't want to do that without a go ahead from Domen. It also won't agree
perfectly with the 'bw' cli, bit what its doing is sloppy anyhow.
On Jul 31, 2026 at 11:11:35 AM, bsorescu ***@***.***> wrote:
> *bsorescu* left a comment (cachix/secretspec#166)
> <#166 (comment)>
>
> Independent verification of the round-2 fixes, on 433aa48 against a
> disposable Vaultwarden (macOS, bw CLI 2026.6.0, current
> vaultwarden/server:latest), via RUN_REGRESSIONS=1
> tests/vaultwarden_harness.sh:
>
> - *Regressions: 11/12 FIXED* — R1–R10 and R12 all green, including
> the substring read/write findings, type-narrowed addressing, the
> secure-note fallback, built-in fields on create, unknown query-key
> rejection, and the fixture-adoption fix.
> - *Integration suite: all tests passed* (40 assertions) · *collection
> addressing: 11/11* — and the new config isolation means the harness
> now survives a developer's ~/.config/secretspec/config.toml,
> confirmed with one present.
>
> One finding still reproduces:
>
> REPRODUCED R11 — a non-ASCII name is unreachable in lower case
> (got 'Error: × Failed to get secret ╰─▶ Secret 'regr_umlaut' not found')
>
> An item created as Überblick isn't found when addressed as überblick.
> Since find_addressed_item now folds with to_lowercase, my read is that
> the candidate never reaches it: the bw list items --search <name>
> pre-filter (bw.rs:1442) does its own matching, and the lowercase query
> returns no candidates for the umlaut name. If that's right, a fallback that
> re-lists without --search (or post-filters the full list) when the
> narrowed query comes back empty would close it — though as a P2 it may also
> be fine to document and ship.
>
> Everything else looks release-ready from here. I'm closing ashebanow#4
> <ashebanow#4> — all three of its
> changes (fail-closed integration exit, XDG_CONFIG_HOME isolation, round-2
> regressions) landed natively in this branch in more complete form.
>
> —
> Reply to this email directly, view it on GitHub
> <#166?email_source=notifications&email_token=AAACX75PMSOWQGABODGY3735HTONPA5CNFSNUABFM5UWIORPF5TWS5BNNB2WEL2JONZXKZKDN5WW2ZLOOQXTKMJUGYYDMOJTHAY2M4TFMFZW63VHNVSW45DJN5XKKZLWMVXHJLDGN5XXIZLSL5RWY2LDNM#issuecomment-5146069381>,
> or unsubscribe
> <https://github.com/notifications/unsubscribe-auth/AAACX77LUX3QHDQONMYDCE35HTONPAVCNFSNUABGKJSXA33TNF2G64TZHMYTAMJTHE4DINJRGI5US43TOVSTWNBZGE2TKOBSG4YDBILWAI>
> .
> You are receiving this because you were mentioned.Message ID:
> ***@***.***>
>
|
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
4 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.
Adds a Bitwarden Password Manager provider (
bw://) backed by the officialbwCLI.This resumes the effort from #24 — the renewed discussion in #15 is what inspired picking this work back up. It starts from the Password Manager core of that PR and reworks it for today's codebase.
What's included
bw://provider shelling out to thebwCLI, with vault-wide access to all item types: logins, cards, identities, SSH keys, and secure notesbwmatches the CLI it drives and the cargo feature, mirroring howbws://is named after thebwsCLIbw://?type=login&field=username) andBITWARDEN_DEFAULT_TYPE/BITWARDEN_DEFAULT_FIELDenv vars (prefix kept clear of thebwCLI's ownBW_*namespace)ref = { item = "...", field = "..." }mapping insecretspec.toml, so secret keys stay valid identifiers while pointing at arbitrarily named vault itemsbw://org@collection) and self-hosted servers (?server=)secretspec setbwfailure modes: CLI not installed (with install instructions), not logged in, vault locked; secret values pass via stdin and the session via the environment, never argvbwcargo feature (default-enabled; adds only abase64dependency since the provider drives the CLI rather than linking an SDK)docs/providers/bw.mdwith 0.16 version notice) and all provider listings per the adding-providers checklist — sidebar, llms.txt description, concepts/reference tables, security table, landing page, quick-start, README — each labeled(0.16+)tests/bitwarden_integration.sh: end-to-end suite against a real vault covering all five item types,refmapping, env-var defaults, error paths, and set/update; auto-creates and cleans up its own test itemsbitwarden-cliadded to devenv for integration testingChanges relative to #24
bws://provider, so this PR is Password Manager onlyProviderUrl/AddressAPI and provider registrationrefmappingTesting
cargo test --allgreen locally (macOS)refmapping, env-var defaults, error handling, and set/update pathsNotes for reviewers
Refs #15 (inspiration for resuming), supersedes #24.
AI Note: this project's code and documentation were produced with the help of UI. The old pull request was developed with the help of Claude Code and Claude Opus/Sonnet, the additional work for this pull request was done using my custom pi environment and Deepseek v4 Pro/Flash.