[PM-41268] Withhold gated cipher secrets behind a type-checked witness - #8115
Open
Hinton wants to merge 15 commits into
Open
[PM-41268] Withhold gated cipher secrets behind a type-checked witness#8115Hinton wants to merge 15 commits into
Hinton wants to merge 15 commits into
Conversation
Hinton
changed the title
Member
Author
|
We need to filter out partial ciphers and ensure they are only sent to compatible clients. For now that is the web vault only. |
Hinton
changed the title
Hinton
force-pushed
the
pam/cipher-partial-data-witness
branch
from
July 31, 2026 14:13
3c9b84b to
ff6c1cf
Compare
![github-code-quality[bot]](https://avatars.githubusercontent.com/u/9919?s=60&v=4){kind=small}
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## main #8115 +/- ##
==========================================
- Coverage 63.41% 63.27% -0.14%
==========================================
Files 2339 2385 +46
Lines 101534 103898 +2364
Branches 9179 9399 +220
==========================================
+ Hits 64385 65741 +1356
- Misses 34935 35923 +988
- Partials 2214 2234 +20 ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
Hinton
commented
Aug 6, 2026
Emitting a cipher's full secret data now requires a FullCipherAccess witness, and the default response shape is partial. A path that forgets to obtain a witness returns partial data - a visible bug - rather than leaking secrets. - PartialCipherData.Strip reshapes the plaintext JSON envelope, keeping the encrypted name (and, for logins, the encrypted URIs) and dropping every other encrypted field. Nothing is decrypted; retained values stay individually-encrypted EncStrings. - FullCipherAccess is the witness. Its factories are internal, so only the leasing gate can mint one; the Full* response ctors call Require() per element, keeping bulk lists fail-closed rather than only single reads. - Each of the 4 cipher response types gains a Full* subclass deriving from its partial counterpart, so ListResponseModel holds a polymorphic mix and the wire contract is unchanged (verified: the generated OpenAPI spec adds and removes no schemas). All secret setters are now protected. - Attachment metadata is withheld from partial responses because it carries each attachment's encryption key, and GetAttachmentData is gated: a download URL grants the encrypted attachment, which the caller could decrypt with an org key they already hold. - ICipherLeaseGate is the decision point, with NoopCipherLeaseGate as the OSS default. The interface deliberately has zero PAM-domain dependencies, so this lands independently of the leasing domain; the commercial gate overrides the registration later in startup. Only the web vault understands the partial shape, so gated ciphers are omitted entirely for every other client rather than sent partial. An older client would render an item with no credentials as though it were empty, and saving it back would overwrite the withheld fields with the blanks the client holds; dropping the item is the lesser harm, and it stays visible in the web vault where the user can request access. PartialCipherSupport is the single predicate, and it fails safe - an absent or unrecognized device type is treated as unable to render the shape. In sync this mirrors the existing FilterUnsupportedCipherTypes pattern; on single reads a gated cipher is a 404. Behavior is unchanged: the no-op gate authorizes everything, so every existing response is still full and nothing is ever filtered. The bulk read paths use the gate's self-loading overload rather than eagerly querying collections, so nothing extra is queried while the feature is off. Adds a reflection-based fitness guard asserting the invariants that make this fail closed - no public setter on any secret property, no way for application code to mint a witness, and every Full* ctor requiring one - so a future refactor that reopens one of those holes fails a test.
`PartialCipherData.Strip` now serializes a purpose-built DTO with `IgnoreWritingNullAndCamelCase` instead of round-tripping `CipherLoginData`. This matches the shape the SDK's restricted decrypt path consumes (the same `LoginUri` fields as a full login) and drops the redundant singular `Uri` the legacy computed getter leaked. Input is parsed case-insensitively so the stored PascalCase blob and an already-stripped camelCase blob both round-trip. Adds a JsonDocument shape test pinning the camelCase wire contract.
Hinton
force-pushed
the
pam/cipher-partial-data-witness
branch
from
August 10, 2026 15:59
bb44da6 to
ff5f8fd
Compare
patriksvensson
approved these changes
Aug 10, 2026
patriksvensson
left a comment
patriksvensson
left a comment
Member
There was a problem hiding this comment.
Left some comments. Not a deal breaker for getting this merged IMO.
Contributor
🤖 Bitwarden Claude Code ReviewOverall Assessment: APPROVE Re-reviewed the changes since the previous pass, which reshaped Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
harr1424
previously approved these changes
Aug 12, 2026
harr1424
left a comment
harr1424
left a comment
Contributor
There was a problem hiding this comment.
Approving Tools-owned changes based upon comments made in src/Api/Tools/Controllers/OrganizationExportController.cs ✅
JaredSnider-Bitwarden
previously approved these changes
Aug 12, 2026
JaredSnider-Bitwarden
left a comment
JaredSnider-Bitwarden
left a comment
Contributor
There was a problem hiding this comment.
Emergency Access changes LGTM!
patriksvensson
previously approved these changes
Aug 13, 2026
MGibson1
requested changes
Aug 13, 2026
Member
There was a problem hiding this comment.
Fix ResponseModel deprecated usage Bit.Core.Models.Api.ResponseModel -> Bit.HttpExtensions.ResponseModel
| // grants the encrypted attachment, decryptable with the org key the caller already holds. | ||
| if (await _cipherLeaseGate.AuthorizeReadAsync(userId, cipher) is null) | ||
| { | ||
| throw new NotFoundException(); |
Member
There was a problem hiding this comment.
issue: Should be not authorized?
Member
Author
There was a problem hiding this comment.
It should, but we handle 401 and 403 by logging out the user. That's arguably worse than implying the attachment is not foud.
Member
There was a problem hiding this comment.
The decisions on exports here invalidate a concern I had on the export response filtering. Is it generally true that authorization decisions are made in controllers and, for aggregate responses, in all cases the response models themselves perform the filtering? That is worth including in documentation on the PAM feature.
Member
Author
There was a problem hiding this comment.
No, I think exports were inconsistent and the controller makes all authorization decisions.
Member
There was a problem hiding this comment.
Fix ResponseModel deprecated usage Bit.Core.Models.Api.ResponseModel -> Bit.HttpExtensions.ResponseModel
Member
Author
|
@MGibson1 I don't necessarily agree with fixing deprecations in other teams code. It expands the change and potentially increases the testing surface. The |
ICipherLeaseGate and NoopCipherLeaseGate live in the Core project but declared `Bit.Pam.Services` — the root namespace of the separate Pam.Domain project (`Bit.Pam`). A type's namespace should say which project it is in, so both move to `Bit.Core.Pam.Services`, matching the `Bit.Core.<Area>.Services` shape every other Core area uses.
Hinton
dismissed stale reviews from patriksvensson, JaredSnider-Bitwarden, and harr1424
via
August 14, 2026 07:45
b73c555
TryAddScoped contradicted the override contract in the comment beside it: TryAdd* means "only if absent", so it reads as though the default should win, while the commercial gate is meant to replace it. Plain AddScoped is what AddOosServices already uses for commercially-overridden defaults such as IProviderService. The comment now states the invariant that actually matters, which is at the override site: the commercial registration must be a plain Add* and must run after AddBaseServices. A TryAdd* there would no-op against this default regardless of which verb is used here, leaving leasing ungated.
`new CipherMiniResponseModel(...)` silently meant "partial": the shape rode in on a `partial: true` default that the type name said nothing about, so a call site emitted reduced data without ever saying so. Each of the four models is now an abstract base with `Partial*` and `Full*` subclasses, and the boolean is gone. A call site has to name the shape it emits, and one that names neither does not compile. `Data` and `PartialData` stay declared on the base, so all four schemas serialize exactly as before — verified against the generated internal OpenAPI spec: the same four cipher schemas, each still carrying `data`, `partialData` and `attachments`, and no `Partial*`/`Full*` schema leaked in. `PopulatePartialData` mirrors `PopulateFullData`, which is what withholds attachment metadata now — no more `partial ? null : ...` in shared construction. The concrete types are sealed, so neither shape can be extended into a third meaning, and the fitness tests assert that plus the naming rule over the whole assembly rather than a hand-listed pair per family. There is no `PartialCipherMiniDetailsResponseModel`: every path building the mini-details shape is authorized org-wide rather than through collection membership, so nothing reaching it is gated. Making the bases abstract surfaced a live regression this PR had introduced: `DeleteAttachment` built a bare `CipherMiniResponseModel`, so *every* caller — gated or not — got a response with no `data` and no attachments after deleting an attachment. Both delete-attachment endpoints now build the shape deliberately: the admin variant unrestricted, the member variant through the gate.
`FilterUnsupportedCipherTypes` and `FilterGatedCiphersForUnsupportedClients` answered the same question — what can this client render? — and were only separate because the gated half needs the witness, which needs the caller's collections loaded further down. The gate now decides over all the user's ciphers and a single `FilterCiphersUnsupportedByClient` applies both rules at one call site. Passing the unfiltered set to the gate costs nothing: it computes authorization in memory and still queries nothing while the flag is off.
`Bit.Core.Services.IFeatureService` is obsolete in favour of `Bitwarden.Server.Sdk.Features.IFeatureService` (BWA0002). Fully qualified at the injection point, matching the already-migrated controllers, since `Bit.Core.Services` is imported for its other types. The sibling deprecations flagged in review — `ResponseModel` and `ListResponseModel` — are deliberately left alone: `Bit.HttpExtensions` `ListResponseModel<T>` constrains `T` to the `Bit.HttpExtensions` `ResponseModel`, so taking them here would mean rebasing the whole cipher response hierarchy, and with it sync, emergency access and export, onto the new base. That is its own migration, not a drive-by inside a security change.
`NoopCipherLeaseGate` reads as inert, but it authorizes full secret data for every cipher. For a security control that is worth saying outright, so it is now `UnrestrictedCipherLeaseGate` — matching `FullCipherAccess.Unrestricted()` and the interface's own `Unrestricted()`. This departs from the repo's `Noop*` convention for OSS stubs deliberately.
Two gaps reviewers hit. The gate's parameters read as organization-specific, but it is called with whatever the caller holds, so implementations must decide for user-owned ciphers too — a user-owned cipher is reachable through no collection and therefore never gated, which an implementation must authorize rather than withhold for want of an organization. And the nullable collection parameters had no documented meaning: null means "not loaded, because the caller has no organizations" and is equivalent to empty. `FullCipherAccess` now carries the division of labour it exists to enforce: the controller decides and obtains the witness; a response model never decides, it only shapes what the witness permits — reducing one cipher, or dropping the uncovered ones from an aggregate such as an export.
Dead since before this PR and it was warning about it.
The summary above it already says the gate authorizes every cipher.
A static transform holding two private DTOs, for what is really one shape:
{name} is {name, uris} with uris omitted, and the serializer already drops
nulls. Collapsed into a single type, so the property list is the whole
allowlist — one place to read when auditing what can escape a gated cipher —
and Strip deserializes straight into it rather than parsing CipherLoginData
and copying two fields out. It still does not derive from CipherData, so the
legacy computed singular Uri getter and the base fields stay out of the
envelope.
Output is unchanged, checked byte-for-byte against the previous implementation
over 13 inputs: login with and without URIs, null name, explicit empty uris
array, already-stripped camelCase, each non-login type, a non-login carrying
stray URIs, and empty/whitespace/null blobs. PartialCipherDataTests pass
unmodified.
One trade-off worth knowing: a non-login's URIs are now cleared by a line of
code rather than by the DTO having nowhere to put them, so that exclusion is a
statement instead of a structural guarantee.
…model Export was the only aggregate where the response model decided which ciphers made it into the payload; sync, emergency access and the cipher lists all filter in the controller and let the model shape what it is handed. Moving the `Where` up makes the rule hold everywhere: controllers decide and filter, response models shape by the witness and never drop. It also fails louder. The model now hands every cipher it receives to `FullCipherMiniDetailsResponseModel`, so a path that forgets to filter throws on the witness check instead of quietly shipping a shorter backup.
Hinton
added
ai-review
The witness test was written three ways — `fullAccess.Authorizes(id)`, `fullAccess?.Authorizes(id) == true`, and `is not null && Authorizes(id)` — at the branch that decides whether secrets go out. One of those getting inverted leaks, and only the path it sits on would notice. A static `From` on each abstract base now owns that test, so it appears once per family and nowhere else. Call sites state what they want and get the shape the witness permits; the five casts to the base type and the controller's `BuildCipherMiniResponse` helper are gone with it. Paths authorized out of band still construct their `Full*` directly, which keeps `_cipherLeaseGate.Unrestricted()` visible at the admin and export call sites rather than hidden behind a factory.
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
6 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.
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-41268
Stacked on #8114 (which declares the
PartialDatafield).📔 Objective
Emitting a cipher's full secret data now requires a
FullCipherAccesswitness, andthe default response shape is partial. A path that forgets to obtain a witness
returns partial data — a visible bug — rather than leaking secrets.
PartialCipherData.Stripreshapes the plaintext JSON envelope, keeping theencrypted name (and, for logins, the encrypted URIs) and dropping every other
encrypted field. Nothing is decrypted; retained values stay individually-encrypted
EncStrings. The output is a purpose-built camelCase envelope (name; and forlogins
uris:uri/uriChecksum/match) — the exact shape the SDK's restricteddecrypt path consumes, matching how it deserializes a full login's URIs (
LoginUri).A dedicated DTO (not a reduced
CipherLoginData) keeps the legacy computed singularUrigetter from leaking in, and input is parsed case-insensitively so the storedPascalCase blob and an already-stripped camelCase blob both round-trip (idempotent).
FullCipherAccessis the witness. Its factories areinternal, so only theleasing gate can mint one, and the
Full*ctors callRequire()per element —keeping bulk lists fail-closed, not just single reads.
Full*subclass deriving from itspartial counterpart, so
ListResponseModel<T>holds a polymorphic mix and the wirecontract is unchanged. Every existing public ctor stays, now delegating to a
protectedoverload withpartial: true— so existing call sites keep compiling butemit no secrets. All secret setters became
protected.attachment's encryption key), and
GetAttachmentDatais gated: a download URLgrants the encrypted attachment, which the caller could decrypt with an org key they
already hold.
GetAttachmentDataAdminis deliberately not gated.ICipherLeaseGateis the decision point, withNoopCipherLeaseGateas the OSSdefault. The interface has zero PAM-domain dependencies, which is what lets this
land independently of the leasing domain; the commercial gate overrides the
registration later in startup (last-registration-wins, commented at the call site).
Only compatible clients receive the partial shape
Only the web vault understands it today, so for every other client gated ciphers are omitted
entirely rather than sent partial. An older client would render an item with no credentials as
though it were empty — and saving it back would overwrite the withheld fields with the blanks the
client holds. Dropping the item is the lesser harm, and it stays visible in the web vault where the
user can request access.
PartialCipherSupportis the single predicate and it fails safe: an absent or unrecognizeddevice type maps to
ClientType.All, notWeb, so an unidentified caller is treated as unable torender the shape. Browser extension, desktop, mobile and CLI are all distinct client types.
FilterUnsupportedCipherTypespattern with a sibling filter — sameidea, applied to a response shape rather than a cipher type.
be discarded.
The web vault ships with the server, so there's no version skew to handle. Other clients will need a
minimum-version check when they gain support; the predicate takes the request's device type, so that
can be added without touching call sites.
One note for reviewers: the guard also covers write-returns, since they share the single-cipher
helpers. It can't fire there in practice — once write-path gating lands a gated cipher can't be
mutated at all, and today nothing is gated — so no successful mutation will 404.
Behavior is unchanged
The no-op gate authorizes everything, so every existing response is still full. Verified
by regenerating the OpenAPI spec: no schemas added or removed, and
data+partialDatacoexist on all four models (datais blob encryption,partialDataisPAM — unrelated fields).
The bulk read paths use the gate's self-loading overload rather than eagerly querying
collections, so nothing extra is queried while the feature is off.
Fitness guard
CipherLeaseFilterEnforcementTestsasserts, by reflection, the invariants that make thisfail closed: no public setter on any of the 14 secret properties, no way for application
code to mint a witness, each
Full*derives from its partial counterpart, and everyFull*public ctor requires a witness. A future refactor that reopens one of those holesfails a test rather than silently leaking.
One behavior change worth calling out
OrganizationExportResponseModelfilters gated ciphers out rather than emittingpartial ones — a partially-stripped export is not a usable backup. Under the no-op gate
nothing is gated, so this is inert today, but it is a deliberate semantic difference from
the read paths.
Verification
dotnet build bitwarden-server.slnxclean; Api.Test 2016 passed, Core.Test 6128passed (plus a follow-up commit adding the camelCase envelope + a
JsonDocumentwire-shape test in
PartialCipherDataTests;PartialCipherDataTestsrun: 15 passed).Client-compatibility coverage: the predicate across every client type (including the
fail-safe paths), and end-to-end through sync,
GetAlland single reads for both a web-vault and anincompatible caller. Pre-existing leasing-agnostic controller tests pass unchanged via a new
AutoFixture customization that stubs the gate unrestricted.
Out of scope (follow-ups)
CipherLeaseGate— needsCollection.AccessRuleIdand its persistence, soit lands with the leasing domain. Until then no cipher is ever partial.
CipherServicewrite-path gating (EnsureCanMutate*) — mutation refusal is a separateconcern and is inert under the no-op gate.
🚨 Breaking Changes
None. The wire contract is unchanged and behavior is identical under the no-op gate.
Note for future callers: constructing a bare
CipherMiniResponseModel(or its siblings)now yields a partial response — use the
Full*variant with a witness to emitsecret data.