Skip to content

[PM-44810] fix: Ignore "Vault Items" send type parsing [bug-factory poc] #7488

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
andrebispo5 wants to merge 2 commits into
base: main
Choose a base branch
from
+62 −1
Open

[PM-44810] fix: Ignore "Vault Items" send type parsing [bug-factory poc] #7488

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
5dc3788
[PM-44810] fix: [Defect] Android - When Send Control Policy includes …
Oct 14, 2026
dad597c
Add test for allowedSendTypes with only unknown types and shorten tes…
andrebispo5 Oct 15, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion app/src/main/kotlin/com/x8bit/bitwarden/data/auth/repository/model/PolicyInformation.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,5 +1,6 @@
package com.x8bit.bitwarden.data.auth.repository.model

import com.bitwarden.core.data.serializer.SafeListSerializer
import com.bitwarden.network.model.SendAccessTypeJson
import com.bitwarden.network.model.SendTypeJson
import com.bitwarden.network.model.SyncResponseJson
Expand Down Expand Up @@ -128,7 +129,8 @@ sealed class PolicyInformation {
* when [whoCanAccess] is [SendAccessTypeJson.SPECIFIC_PEOPLE].
* @property deletionHours The number of hours until a Send is deleted, if enforced.
* @property allowedSendTypes The types of Sends that are allowed to be created, if
* restricted.
* restricted. Send types unknown to this client are dropped rather than failing the decode of
* the whole policy.
*/
@Serializable
data class SendControls(
Expand All @@ -147,6 +149,7 @@ sealed class PolicyInformation {
@SerialName("deletionHours")
val deletionHours: Int?,

@Serializable(with = SafeListSerializer::class)

@claude claude Bot Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

⚠️ IMPORTANT: A policy that allows only Vault Items now decodes to 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.singleAllowedSendType uses allowedSendTypes?.singleOrNull(), which gives null for an empty list. So handleAddSendClick shows the full type picker and the user can start a Text or File Send, both of which the policy forbids.
  • ViewSendViewModel.isSendTypeAllowed uses allowedSendTypes?.any { ... } ?: true, which gives false for an empty list. So every existing Send is shown as TypeNotAllowed.

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.

@andrebispo5 andrebispo5 Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@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.

@claude claude Bot Oct 15, 2026 •
edited
Loading

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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 allowedSendTypes from 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) ⚠️ whole policy dropped — 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 ⚠️ whole policy dropped ✅ 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:

  1. Even in that case the PR is strictly stronger than main — disableSend, deletionHours, whoCanAccess, disableHideEmail and allowedDomains from that same policy now apply, where before they were thrown away with it.
  2. There is no correct client behaviour available until SendTypeJson.ITEM exists. The Item-type TODO branches would throw, and SendItemType has 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

@claude claude Bot Oct 15, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

@david-livefront david-livefront Oct 15, 2026

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I believe this can just be @Contextual

@andrebispo5 andrebispo5 Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let me try, but I believe it will discard the other policy options.

@andrebispo5 andrebispo5 Oct 16, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yup tested and it does not work it will make it all null.

@SerialName("allowedSendTypes")
val allowedSendTypes: List<SendTypeJson>?,
Comment on lines +152 to 154

@andrebispo5 andrebispo5 Oct 15, 2026

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@copilot This is not a real case but one created but this new type not being handled at the moment. This will be addressed in PM-41095 ticket. The Send Policy needs to have at least one type enabled otherwise it is disabled and no policy is applied.

) : PolicyInformation()
Expand Down
58 changes: 58 additions & 0 deletions ...st/kotlin/com/x8bit/bitwarden/data/auth/repository/util/SyncResponseJsonExtensionsTest.kt
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -195,6 +195,64 @@ class SyncResponseJsonExtensionsTest {
)
}

@Test
fun `policyInformation drops unknown allowedSendTypes and keeps the SendControls policy`() {
val policy = createMockSdkPolicy(
type = PolicyType.SEND_CONTROLS,
data = """
{
"disableSend": false,
"disableHideEmail": true,
"whoCanAccess": 2,
"allowedDomains": "bitwarden.com",
"deletionHours": 168,
"allowedSendTypes": [0, 2]
}
""".trimIndent(),
)

assertEquals(
PolicyInformation.SendControls(
disableSend = false,
disableHideEmail = true,
whoCanAccess = SendAccessTypeJson.SPECIFIC_PEOPLE,
allowedDomains = "bitwarden.com",
deletionHours = 168,
allowedSendTypes = listOf(SendTypeJson.TEXT),
),
policy.policyInformation,
)
}

@Test
fun `policyInformation maps allowedSendTypes with only unknown types to an empty list`() {
val policy = createMockSdkPolicy(
type = PolicyType.SEND_CONTROLS,
data = """
{
"disableSend": false,
"disableHideEmail": true,
"whoCanAccess": 2,
"allowedDomains": "bitwarden.com",
"deletionHours": 168,
"allowedSendTypes": [2]
}
""".trimIndent(),
)

assertEquals(
PolicyInformation.SendControls(
disableSend = false,
disableHideEmail = true,
whoCanAccess = SendAccessTypeJson.SPECIFIC_PEOPLE,
allowedDomains = "bitwarden.com",
deletionHours = 168,
allowedSendTypes = emptyList(),
),
policy.policyInformation,
)
}

@Test
@Suppress("MaxLineLength")
fun `policyInformation converts the OrganizationUserNotification Json data to policy information`() {
Expand Down
Loading