Repository navigation
[PM-44810] fix: Ignore "Vault Items" send type parsing [bug-factory poc] - #7488
andrebispo5 wants to merge 2 commits into
Conversation
…"Vault Items,"… [bug-factory poc] Automated fix from the AI-49 bug factory. Proof: ./gradlew :app:testStandardDebugUnitTest --tests "com.x8bit.bitwarden.data.auth.repository.util.SyncResponseJsonExtensionsTest" must fail on base and pass with the fix.
github-actions
Bot
added
app:password-manager
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #7488 +/- ##
=======================================
Coverage 86.80% 86.80%
=======================================
Files 895 895
Lines 65676 65676
Branches 9384 9384
=======================================
Hits 57007 57007
Misses 5189 5189
Partials 3480 3480
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Harness. 🚀 New features to boost your workflow:
|
🤖 Bitwarden Claude Code ReviewOverall Assessment: REQUEST CHANGES This PR applies Code Review Details
|
![claude[bot]](https://avatars.githubusercontent.com/in/1236702?s=60&v=4){kind=small}
| @SerialName("deletionHours") | ||
| val deletionHours: Int?, | ||
|
|
||
| @Serializable(with = SafeListSerializer::class) |
There was a problem hiding this comment.
allowedSendTypes = emptyList(), and the create flow treats that list as "no restriction".
Details and fix
This PR makes [2] decode to emptyList(), and the new test asserts exactly that. The consumers then disagree about what an empty list means:
SendViewModel.singleAllowedSendTypeusesallowedSendTypes?.singleOrNull(), which givesnullfor an empty list. SohandleAddSendClickshows the full type picker and the user can start a Text or File Send, both of which the policy forbids.ViewSendViewModel.isSendTypeAllowedusesallowedSendTypes?.any { ... } ?: true, which givesfalsefor an empty list. So every existing Send is shown asTypeNotAllowed.
The result: the list screen says existing Text and File Sends are not allowed, while the add button still lets the user create new ones. This is no worse than the base branch, where the whole policy was dropped. But the empty-list state is new, and nothing handles it.
Suggested fix: handle the empty list explicitly. One option is to resolve it in PolicyManagerImpl.resolveEffectiveSendPolicy (for example, treat an empty allowedSendTypes as disableSend = true until PM-41095 adds Item support). Another is to have SendViewModel block creation when the list is non-null and empty. Add a test that covers whichever you choose.
There was a problem hiding this comment.
@claude This will be handled by another ticket, we just need to make sure that when a restriction is applied it is not being ignored.
There was a problem hiding this comment.
Claude finished @andrebispo5's task in 1m 28s —— View job
Confirmed — restrictions are enforced, and this PR is what stops them being ignored
- Read the PR diff
- Trace
allowedSendTypesfrom decode →PolicyManagerImpl→SendViewModel/AddEditSendViewModel/ViewSendViewModel - Build the enforcement matrix for every policy shape
- Confirm the residual case is PM-41095 only
@andrebispo5 Agreed, and the code backs you up — the "restriction being ignored" case is exactly what this PR fixes, and it is wider than just the type list.
Why the ignore is wider than allowedSendTypes
Policy.policyInformation decodes the whole blob with decodeFromStringOrNull (SyncResponseJsonExtensions.kt:157), so one unknown enum value returns null for the entire SendControls object. PolicyManagerImpl.resolveEffectiveSendPolicy then drops it in mapNotNull (PolicyManagerImpl.kt:166-169), and that org never enters organizationIdsWithSendControls (line 171). Every field of that policy is lost — not just the type list, but disableSend, disableHideEmail, deletionHours, whoCanAccess and allowedDomains too.
So on main, an admin who ticks "Vault Items" alongside Text gets a policy where even disableSend is silently ignored on Android. That is the real fail-open, and this PR closes it.
Enforcement matrix
allowedSendTypes from server |
main |
With this PR |
|---|---|---|
absent / null |
policy enforced, no type restriction | unchanged |
[0], [1], [0,1] |
policy enforced | unchanged |
[0,2] (Text + Vault Items) |
disableSend, deletionHours, whoCanAccess, disableHideEmail, allowedDomains and the type restriction all ignored |
✅ decodes to [TEXT]; singleAllowedSendType resolves to Text, so the picker is skipped and only a Text Send can be created (SendViewModel.kt:335-343, 940-943), types filter hidden (line 535), and every other field is enforced |
[2] only |
✅ all non-type fields enforced; type dimension decodes to emptyList() and stays permissive → PM-41095 |
The [0,2] row is the one an admin actually produces when they allow Text plus Vault Items, and it goes from "nothing enforced" to "fully enforced, Text only". No restriction that this client can express is ignored after the change.
The one residual case, and why it is correctly deferred
[2]-only decodes to emptyList(), and singleOrNull() on an empty list is null, so the type dimension reads as unrestricted. Two things make that the right call for this PR:
- Even in that case the PR is strictly stronger than
main—disableSend,deletionHours,whoCanAccess,disableHideEmailandallowedDomainsfrom that same policy now apply, where before they were thrown away with it. - There is no correct client behaviour available until
SendTypeJson.ITEMexists. The Item-typeTODObranches would throw, andSendItemTypehas no Item value — which is PM-41095, as you say.
Worth confirming server-side (not verifiable from this repo): that the policy editor enforces at least one type selected, so a [2]-only policy is not something a client can receive in the first place. If that holds, the residual row is unreachable today.
The view side is unaffected either way: policyRestriction is gated on isSendDisabled, which comes from sendView.disabled set by the server (ViewSendViewModel.kt:272, 331-340), not derived from the allowlist.
No blocking findings. No code changes made.
Note: I could not run Gradle in this job (the test command was not in the allowed tool list), so the verification above is from reading the code plus the test run already recorded in the PR description.
· Branch PM-44810-send-controls-vault-items
There was a problem hiding this comment.
🟡 Changes recommended
The creation flow still offers disallowed types when filtering leaves an empty allowlist.
1 open finding
What changed in this PR
Preserves Send Controls policies containing unsupported Send types without enabling Item Send support.
Changes:
- Filters unknown
allowedSendTypesvalues during decoding. - Tests mixed supported/unsupported lists and unknown-only lists.
| File | Description |
|---|---|
| app/src/test/kotlin/com/x8bit/bitwarden/data/auth/repository/util/SyncResponseJsonExtensionsTest.kt | Adds regression tests for unknown Send types. |
| app/src/main/kotlin/com/x8bit/bitwarden/data/auth/repository/model/PolicyInformation.kt | Applies and documents tolerant allowlist decoding. |
🧠 Review effort: Balanced
💡 Configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
| @Serializable(with = SafeListSerializer::class) | ||
| @SerialName("allowedSendTypes") | ||
| val allowedSendTypes: List<SendTypeJson>?, |
andrebispo5
changed the title
andrebispo5
changed the title
| @SerialName("deletionHours") | ||
| val deletionHours: Int?, | ||
|
|
||
| @Serializable(with = SafeListSerializer::class) |
There was a problem hiding this comment.
You're right — withdrawing my earlier finding
@andrebispo5 I re-read both consumers and the behaviour matches what you describe. No code changes made.
Create side — SendViewModel.kt:940-943: allowedSendTypes?.singleOrNull()?.toSendItemType() returns null for an empty list, and isAddSendTypePickerVisible (SendViewModel.kt:535) is singleAllowedSendType == null. An empty allowlist falls through to the unrestricted path — exactly the "as if it is not in enforcement" semantics you describe, and the same user-visible outcome as the base branch where the whole policy was dropped. The PR additionally makes the rest of the policy (disableHideEmail, deletionHours, allowedDomains) survive instead of being lost with it, which is a strict improvement.
View side — my review claimed every existing Send would be marked TypeNotAllowed. That was wrong about the gating. policyRestriction returns null unless isSendDisabled is true (ViewSendViewModel.kt:334), and isSendDisabled is sendView.disabled (line 272) — set by the server, not derived from the allowlist. So the branch is only reachable for Sends the server has already marked non-compliant, and for a Vault-Items-only policy TypeNotAllowed is the accurate label for a Text Send there. Nothing regresses for compliant Sends, so the create/view inconsistency I described does not occur.
That leaves no behavioural gap in this PR. The rest is genuinely PM-41095 — once SendTypeJson.ITEM exists and the Item-type TODO branches are filled in, the allowlist decodes non-empty and enforcement follows automatically.
One thing worth confirming on your side, since it is server-side knowledge I cannot verify from this repo: that "at least one type enabled" is enforced at policy-save time, so a client can never receive a policy whose entire allowlist consists of types it does not know. If that invariant ever loosens, the empty-list-to-unrestricted path becomes a real fail-open. Not a blocker here either way.
No blocking findings from me on this change.
Note: my original comment could not be edited — the comment-update tool returned 404 Not Found on every retry after the first write, so this is posted as a new comment on the same line. View job run
| @SerialName("deletionHours") | ||
| val deletionHours: Int?, | ||
|
|
||
| @Serializable(with = SafeListSerializer::class) |
There was a problem hiding this comment.
I believe this can just be @Contextual
There was a problem hiding this comment.
let me try, but I believe it will discard the other policy options.
There was a problem hiding this comment.
yup tested and it does not work it will make it all null.
branch: PM-44810-send-controls-vault-items
base: main
🎟️ Tracking
https://bitwarden.atlassian.net/browse/PM-44810
📔 Objective
Automated fix produced by the bug factory (AI-49 proof of concept). Needs review from the owning team before merge.
verified: genuine, live, unduplicated.
SendTypeJsonhas no value for2and no serializer default (SendTypeJson.kt:11-30). A SendControls policy listing "Vault Items" therefore fails to decode as a whole and is silently dropped (JsonExtensions.kt:37-46). The same code ships inv2026.9.1-bwpm.Why
SafeListSerializerinstead of adding the Item typeThe fix drops send types this client does not know when decoding
allowedSendTypes, instead of adding the Item type (2) toSendTypeJson. Adding it now is not safe:2fails to decode and is silently removed by theSafeListSerializerthatSyncResponseJson.sendsalready uses. AddingITEMwould let those Sends reach the SDK and the UI.TODO("[PM-41095] Support Item SendType")branches for the Item type (for example the Send list inSendDataExtensions.kt), which throw at runtime. A user with a single Item Send would crash the Send tab.SendViewModelmaps the single allowed type to aSendItemTypewhen it is created, which has no Item value.Adding
SendTypeJson.ITEMand handling it properly belongs to PM-41095 (Support Item SendType).📸 Screenshots
None: this fix is proven by a JVM unit test, and the factory drove no device (code-only target).
🧪 Proof
./gradlew :app:testStandardDebugUnitTest --tests "com.x8bit.bitwarden.data.auth.repository.util.SyncResponseJsonExtensionsTest"on the base code (must fail) exited 1; with the fix (must pass) exited 0.Base run
Fix run
🤖 Automated review
⏰ Reminders before merge