Skip to content

fix(pkce): use rejection sampling to remove modulo bias#171

Merged
wyattjoh merged 1 commit intomainfrom
security/pkce-rejection-sampling
Apr 15, 2026
Merged

fix(pkce): use rejection sampling to remove modulo bias#171
wyattjoh merged 1 commit intomainfrom
security/pkce-rejection-sampling

Conversation

@wyattjoh
Copy link
Copy Markdown
Contributor

@wyattjoh wyattjoh commented Apr 15, 2026

Summary

  • Replace byte % CHARSET.length in generateCodeVerifier with rejection sampling. The 66-character PKCE charset does not divide evenly into 256, so the naive modulo biased the first 58 characters to appear ~33% more often than the last 8.
  • Restores uniform distribution and full RFC 7636 entropy without affecting the public API.
  • Flagged by CodeQL: js/biased-cryptographic-random.

Test plan

  • bun test packages/cli-core/src/lib/pkce.test.ts
  • New test asserts per-character counts stay within 10% of uniform over 2000 verifiers
  • bun run typecheck

@changeset-bot
Copy link
Copy Markdown

changeset-bot bot commented Apr 15, 2026

🦋 Changeset detected

Latest commit: 1eada24

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 1 package
Name Type
clerk Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@coderabbitai
Copy link
Copy Markdown

coderabbitai bot commented Apr 15, 2026

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 373a5ec3-579f-4358-a917-2140ea36510b

📥 Commits

Reviewing files that changed from the base of the PR and between 67a57a1 and 1eada24.

📒 Files selected for processing (3)
  • .changeset/pkce-unbiased-random.md
  • packages/cli-core/src/lib/pkce.test.ts
  • packages/cli-core/src/lib/pkce.ts
✅ Files skipped from review due to trivial changes (1)
  • .changeset/pkce-unbiased-random.md
🚧 Files skipped from review as they are similar to previous changes (2)
  • packages/cli-core/src/lib/pkce.ts
  • packages/cli-core/src/lib/pkce.test.ts

📝 Walkthrough

Walkthrough

Adds a changeset file and modifies PKCE verifier generation to remove modulo-based bias: generateCodeVerifier() now accumulates characters in a string[] and uses rejection sampling (skipping bytes >= REJECTION_THRESHOLD) to produce uniformly distributed characters from the 66-character URL-safe charset until reaching VERIFIER_LENGTH. A new Bun unit test performs Monte Carlo sampling to assert per-character frequencies remain within a tolerance across many generated verifiers.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~25 minutes

🚥 Pre-merge checks | ✅ 2 | ❌ 1

❌ Failed checks (1 warning)

Check name Status Explanation Resolution
Docstring Coverage ⚠️ Warning Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%. Write docstrings for the functions missing them to satisfy the coverage threshold.
✅ Passed checks (2 passed)
Check name Status Explanation
Title check ✅ Passed The title directly and clearly summarizes the main change: switching from modulo to rejection sampling to fix PKCE bias.
Description check ✅ Passed The description is well-detailed and directly related to the changeset, explaining the bias problem, the solution, and the test validation approach.

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


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

@wyattjoh
Copy link
Copy Markdown
Contributor Author

The naive `byte % CHARSET.length` loop over-represents the first 58
characters of the 66-entry PKCE charset by ~33% because 256 is not a
multiple of 66. Switch to rejection sampling so every character is
equally likely.

Flagged by CodeQL (js/biased-cryptographic-random).
@wyattjoh wyattjoh force-pushed the security/pkce-rejection-sampling branch from 67a57a1 to 1eada24 Compare April 15, 2026 22:47
@wyattjoh wyattjoh merged commit 0ba1d70 into main Apr 15, 2026
10 checks passed
@wyattjoh wyattjoh deleted the security/pkce-rejection-sampling branch April 15, 2026 23:17
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