Skip to content

Increased retention offer code entropy from 16-bit to 32-bit#26736

Closed
kevinansfield wants to merge 1 commit intomainfrom
worktree-kind-foraging-manatee
Closed

Increased retention offer code entropy from 16-bit to 32-bit#26736
kevinansfield wants to merge 1 commit intomainfrom
worktree-kind-foraging-manatee

Conversation

@kevinansfield
Copy link
Copy Markdown
Member

no issue

  • Retention offer codes now use Uint32Array instead of Uint16Array, generating 8-character hex codes instead of 4-character ones
  • Increases the code space from ~65K to ~4.3 billion possible values

Retention offer codes now use Uint32Array instead of Uint16Array,
generating 8-character hex codes instead of 4-character ones.
@kevinansfield kevinansfield requested a review from sagzy March 9, 2026 13:38
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai bot commented Mar 9, 2026

Walkthrough

This pull request modifies the retention offer code generation mechanism in the admin settings interface. The primary change increases the cryptographic randomness used for generating retention offer hashes from 16-bit to 32-bit values, resulting in hash strings of 8 hexadecimal characters instead of 4. The corresponding test suite is updated to validate the new 8-character format for both retention offer names and codes.

Possibly related issues

  • TryGhost/Ghost-Security#142: Directly addresses expanding retention offer hash generation from 16-bit to 32-bit Uint32Array to increase code entropy and improve randomness.
🚥 Pre-merge checks | ✅ 3
✅ Passed checks (3 passed)
Check name Status Explanation
Title check ✅ Passed The title clearly and concisely describes the main change: increasing retention offer code entropy from 16-bit to 32-bit, which directly matches the changeset modifications.
Description check ✅ Passed The description is directly related to the changeset, explaining the switch from Uint16Array to Uint32Array and the resulting code space expansion.
Docstring Coverage ✅ Passed No functions found in the changed files to evaluate docstring coverage. Skipping docstring coverage check.

✏️ Tip: You can configure your own custom pre-merge checks in the settings.

✨ Finishing Touches
  • 📝 Generate docstrings (stacked PR)
  • 📝 Generate docstrings (commit on current branch)
🧪 Generate unit tests (beta)
  • Create PR with unit tests
  • Post copyable unit tests in a comment
  • Commit unit tests in branch worktree-kind-foraging-manatee

Comment @coderabbitai help to get the list of available commands and usage tips.

Copy link
Copy Markdown
Contributor

@coderabbitai coderabbitai bot left a comment

Choose a reason for hiding this comment

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

🧹 Nitpick comments (1)
apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx (1)

453-455: Extract the hash formatter so the 8-character guarantee can be tested deterministically.

This line is correct, but keeping the random generation and hex formatting inline makes the new width requirement hard to lock down. A future padding regression here would still slip past the current acceptance test most runs. Pulling this into a tiny helper would let you stub a value like 0x0000000a and assert 0000000a directly.

🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.

In
`@apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx`
around lines 453 - 455, Extract the inline hex-padding logic from
createRetentionOffer into a small exported helper (e.g., formatHashHex or
to8CharHex) that accepts a numeric value and returns an 8-character, zero-padded
lowercase hex string; update createRetentionOffer to call this helper with the
random number produced by crypto.getRandomValues, and add tests that import the
helper and pass deterministic inputs (for example 0x0000000a) to assert it
returns "0000000a". Ensure the helper is unit-tested for edge cases (0, max
uint32, small values) so the 8-character padding requirement is enforced
deterministically.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.

Nitpick comments:
In
`@apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx`:
- Around line 453-455: Extract the inline hex-padding logic from
createRetentionOffer into a small exported helper (e.g., formatHashHex or
to8CharHex) that accepts a numeric value and returns an 8-character, zero-padded
lowercase hex string; update createRetentionOffer to call this helper with the
random number produced by crypto.getRandomValues, and add tests that import the
helper and pass deterministic inputs (for example 0x0000000a) to assert it
returns "0000000a". Ensure the helper is unit-tested for edge cases (0, max
uint32, small values) so the 8-character padding requirement is enforced
deterministically.

ℹ️ Review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: 171554a3-4ec0-44e2-86c1-14aecf0e0970

📥 Commits

Reviewing files that changed from the base of the PR and between be0ea50 and 102c018.

📒 Files selected for processing (2)
  • apps/admin-x-settings/src/components/settings/growth/offers/edit-retention-offer-modal.tsx
  • apps/admin-x-settings/test/acceptance/membership/offers.test.ts

@sagzy
Copy link
Copy Markdown
Contributor

sagzy commented Mar 9, 2026

Closing in favor of #26738, which contains an additional length check for the offer name, as Stripe imposes a 40 char limit to coupon names

@sagzy sagzy closed this Mar 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants