Skip to content

[PM-42557] - Fix TXT generation - #8333

Merged
jrmccannon merged 1 commit into
mainfrom
jmccannon/ac/pm-42557-secure-random
Sep 10, 2026
Merged

[PM-42557] - Fix TXT generation#8333
jrmccannon merged 1 commit into
mainfrom
jmccannon/ac/pm-42557-secure-random

Conversation

@jrmccannon

@jrmccannon jrmccannon commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

🎟️ Tracking

PM-42557

📔 Objective

Swapped the RandomString for the TXT generation with updated SecureRandomString method.

@jrmccannon
jrmccannon requested a review from a team as a code owner September 9, 2026 17:38
@jrmccannon
jrmccannon requested a review from BTreston September 9, 2026 17:38
@github-actions

github-actions Bot commented Sep 9, 2026

Copy link
Copy Markdown
Contributor

🤖 Bitwarden Claude Code Review

Overall Assessment: APPROVE

This PR replaces CoreHelpers.RandomString with CoreHelpers.SecureRandomString for generating the DNS TXT verification token in CreateOrganizationDomainCommand, moving the token off the non-cryptographic System.Random and onto RandomNumberGenerator. Verified that SecureRandomString applies rejection sampling to avoid modulo bias and that its default character set (62 alphanumerics, special: false) keeps the emitted value byte-for-byte compatible in shape and length with the prior implementation, so the 47-character bw= record and client-side parity are unaffected. An accompanying unit test asserts the bw= prefix, the 44-character alphanumeric payload, and that two successive generations differ. No security, correctness, or breaking-change findings were identified.

Code Review Details

No findings met the confidence threshold for posting.

Notes considered and intentionally not raised as findings:

  • The two remaining CoreHelpers.RandomString call sites in Billing (SubscriberService.cs, UpdatePaymentMethodCommand.cs) are pre-existing, untouched by this diff, and generate short non-security display suffixes.
  • The new test would also pass against the previous System.Random implementation, since it verifies token shape and uniqueness rather than randomness provenance. Proving CSPRNG usage from a unit test would require injecting an abstraction, which is out of scope for this fix.

@codecov

codecov Bot commented Sep 9, 2026

Copy link
Copy Markdown

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 64.03%. Comparing base (e520797) to head (4cdeccd).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@            Coverage Diff             @@
##             main    #8333      +/-   ##
==========================================
- Coverage   64.03%   64.03%   -0.01%     
==========================================
  Files        2473     2473              
  Lines      106017   106017              
  Branches     9613     9613              
==========================================
- Hits        67886    67885       -1     
- Misses      35771    35772       +1     
  Partials     2360     2360              

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

@jrmccannon jrmccannon added the t:bugfix Change Type - Bugfix label Sep 9, 2026
@jrmccannon
jrmccannon merged commit 882358a into main Sep 10, 2026
50 of 52 checks passed
@jrmccannon
jrmccannon deleted the jmccannon/ac/pm-42557-secure-random branch September 10, 2026 13:46
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

t:bugfix Change Type - Bugfix

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants