Skip to content

[PM-40216] feat: Data-protect OrganizationInviteLink.Code at rest#7969

Draft
r-tome wants to merge 3 commits into
mainfrom
pm-40216/data-protect-invite-link-codes
Draft

[PM-40216] feat: Data-protect OrganizationInviteLink.Code at rest#7969
r-tome wants to merge 3 commits into
mainfrom
pm-40216/data-protect-invite-link-codes

Conversation

@r-tome

@r-tome r-tome commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

https://bitwarden.atlassian.net/browse/PM-40216

📔 Objective

The invite link Code is a bearer secret — anyone with DB read access (breach, backup dump, self-hosted operator) could obtain every org's join capability and walk a compliant email to Accepted. This PR data-protects Code at rest via IDataProtector, matching the existing pattern in UserRepository and SendRepository.

Key changes:

  • OrganizationInviteLink.Code changes from Guid to string (wire type stays Guid); stored as "P|" + protector.Protect(code) in a new NVARCHAR(300) column
  • GetByCodeAsync is removed; all anonymous endpoints now require both OrganizationId and Code — fetch by org, then validate Code via CryptographicOperations.FixedTimeEquals (constant-time, no oracle)
  • Unique index on Code dropped (encrypted values are non-deterministic)
  • InviteLinkCodeValidator static helper centralises the constant-time comparison
  • SQL Server: coordinated-deploy migration (must run after all instances are on the new version); EF Core migrations for MySQL, PostgreSQL, SQLite
  • URL format becomes /#/join/{orgId}/{code}?key=... (clients PR companion)

Security properties after this change:

  • DB compromise no longer yields usable invite codes
  • Code mismatch and missing-org both return 404 with the same shape (no enumeration oracle)
  • Admin re-display of the invite URL continues to work (reversible protection, not a hash)

Stores invite link Code as IDataProtector-encrypted NVARCHAR(300) with a
"P|" prefix, replacing the plaintext UNIQUEIDENTIFIER. All anonymous
endpoints now require OrganizationId + Code; lookup by Code is replaced
with lookup-by-org + constant-time FixedTimeEquals comparison, eliminating
the Code as a DB lookup key and closing the DB-read bearer-secret exposure.

Includes SQL Server migration (coordinated deploy), EF Core migrations for
MySQL/PostgreSQL/SQLite, SSDT schema update, and the InviteLinkCodeValidator
helper for constant-time comparisons.
@r-tome r-tome requested review from a team as code owners July 13, 2026 14:49
@r-tome r-tome requested a review from sven-bitwarden July 13, 2026 14:49
@github-actions

github-actions Bot commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

Reviewed the data-protection of OrganizationInviteLink.Code at rest: the Guidstring type change, IDataProtector protect/unprotect in both Dapper and EF Core repositories, the constant-time InviteLinkCodeValidator, the anonymous-endpoint move to org-scoped fetch plus code comparison, and the MSSQL + EF migrations. The implementation matches the established UserRepository/SendRepository protection pattern, preserves dual-ORM parity, and closes the enumeration oracle with uniform 404s. No new findings meet the confidence bar.

Code Review Details

No new findings.

Notes from analysis (no action required):

  • Constant-time comparison is correct: FixedTimeEquals with a null/empty guard, and incoming Guid.ToString() is always lowercase.
  • Reversible protection is idempotent — ProtectData guards on the "P|" prefix and restores the plaintext Code in a finally, so admin re-display keeps working.
  • Migration casing is correct per provider: MSSQL casts UNIQUEIDENTIFIERNVARCHAR as uppercase and normalizes with LOWER(); PostgreSQL/MySQL/SQLite already render/store GUIDs as lowercase, so no normalization is needed there. This resolves the one open thread on the SQLite migration. New codes round-trip losslessly on all providers since they are stored as the protected string.
  • Feature remains gated behind pm-32497-generate-invite-link, so pre-existing production rows subject to the casing question are minimal.

migrationBuilder.DropIndex(
name: "IX_OrganizationInviteLink_Code",
table: "OrganizationInviteLink");
}

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.

QUESTION: Are pre-existing invite-link codes normalized to lowercase on SQLite/MySQL/PostgreSQL?

Details

The MSSQL migration deliberately runs UPDATE ... SET [Code] = LOWER([Code]) with the comment "SQL Server casts UNIQUEIDENTIFIER to uppercase NVARCHAR; LOWER() normalizes for FixedTimeEquals". This is because InviteLinkCodeValidator.CodesMatch uses CryptographicOperations.FixedTimeEquals on raw UTF-8 bytes — a case-sensitive comparison — while the incoming request.Code.ToString() is always lowercase (Guid.ToString() in .NET is lowercase).

This SQLite migration (and the MySQL/Postgres ones) only drops the index / alters the column type; none of them lowercase existing rows. For legacy invite links created before this PR, Code was stored via EF's Guid-to-TEXT conversion — the same conversion used for Id/OrganizationId, which EF renders in uppercase in SQLite.

If SQLite stored those legacy codes uppercase, they will no longer match the lowercase incoming code after migration, silently breaking every pre-existing invite link on self-hosted SQLite deployments (Postgres uuid→text and MySQL char(36) are lowercase, so likely unaffected).

Can you confirm the stored casing on each EF provider, and add a normalization step for any provider that stores uppercase — mirroring the MSSQL LOWER() — so existing links keep working?

@codecov

codecov Bot commented Jul 13, 2026

Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 79.26829% with 34 lines in your changes missing coverage. Please review.
✅ Project coverage is 14.99%. Comparing base (d6cb0a1) to head (1f2be37).

Files with missing lines Patch % Lines
...ionFeatures/InviteLinks/InviteLinkCodeValidator.cs 0.00% 8 Missing ⚠️
...re/AdminConsole/Entities/OrganizationInviteLink.cs 14.28% 6 Missing ⚠️
...InviteLinks/AcceptOrganizationInviteLinkCommand.cs 0.00% 2 Missing ⚠️
...iteLinks/ConfirmOrganizationInviteLinkValidator.cs 0.00% 2 Missing ⚠️
...iteLinks/GetOrganizationInviteLinkPoliciesQuery.cs 0.00% 2 Missing ⚠️
...nviteLinks/GetOrganizationInviteLinkStatusQuery.cs 0.00% 2 Missing ⚠️
.../ValidateOrganizationInviteLinkEmailDomainQuery.cs 0.00% 2 Missing ⚠️
...e/Repositories/OrganizationInviteLinkRepository.cs 97.36% 0 Missing and 2 partials ⚠️
...e/Repositories/OrganizationInviteLinkRepository.cs 96.49% 0 Missing and 2 partials ⚠️
...InviteLinks/AcceptOrganizationInviteLinkRequest.cs 0.00% 1 Missing ⚠️
... and 5 more
Additional details and impacted files
@@             Coverage Diff             @@
##             main    #7969       +/-   ##
===========================================
- Coverage   62.03%   14.99%   -47.05%     
===========================================
  Files        2276     1382      -894     
  Lines       99404    60207    -39197     
  Branches     8980     4782     -4198     
===========================================
- Hits        61667     9027    -52640     
- Misses      35565    51025    +15460     
+ Partials     2172      155     -2017     

☔ View full report in Codecov by Harness.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
  • 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

@r-tome r-tome marked this pull request as draft July 13, 2026 15:00
@r-tome r-tome added the ai-review Request a Claude code review label Jul 13, 2026
@r-tome r-tome changed the title [PM-40216] Data-protect OrganizationInviteLink.Code at rest [PM-40216] feat: Data-protect OrganizationInviteLink.Code at rest Jul 13, 2026
@r-tome r-tome added the t:feature Change Type - Feature Development label Jul 13, 2026
r-tome added 2 commits July 14, 2026 11:16
…-invite-link-codes

# Conflicts:
#	test/Api.Test/AdminConsole/Controllers/OrganizationInviteLinksControllerTests.cs
#	test/Core.Test/AdminConsole/OrganizationFeatures/InviteLinks/GetOrganizationInviteLinkStatusQueryTests.cs
…ation

CI's migration order check failed because main gained
2026-07-14_00_AddCreationDateToOrganizationUserUserDetailsView.sql
after this branch's migration was authored.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

ai-review Request a Claude code review t:feature Change Type - Feature Development

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant