Skip to content

Commit 67a57a1

Browse files
committed
fix(pkce): use rejection sampling to remove modulo bias
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).
1 parent 11ace89 commit 67a57a1

3 files changed

Lines changed: 38 additions & 5 deletions

File tree

.changeset/pkce-unbiased-random.md

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
"clerk": patch
3+
---
4+
5+
Fix biased character distribution in PKCE code verifier generation. Replaces `byte % CHARSET.length` with rejection sampling so every character in the 66-entry charset is equally likely, restoring full entropy.

packages/cli-core/src/lib/pkce.test.ts

Lines changed: 21 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -18,6 +18,27 @@ describe("PKCE", () => {
1818
expect(a).not.toBe(b);
1919
});
2020

21+
test("generateCodeVerifier produces an unbiased distribution", () => {
22+
// With 66 charset entries and 256-byte modulo, a naive `byte % 66`
23+
// over-represents the first 58 characters by ~33%. Rejection sampling
24+
// should keep per-character counts within ~10% of uniform over a
25+
// large sample.
26+
const CHARSET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~";
27+
const counts = new Map<string, number>(CHARSET.split("").map((c) => [c, 0]));
28+
const iterations = 2000;
29+
for (let i = 0; i < iterations; i++) {
30+
for (const ch of generateCodeVerifier()) {
31+
counts.set(ch, (counts.get(ch) ?? 0) + 1);
32+
}
33+
}
34+
const total = iterations * 43;
35+
const expected = total / CHARSET.length;
36+
const tolerance = expected * 0.1;
37+
for (const [, count] of counts) {
38+
expect(Math.abs(count - expected)).toBeLessThan(tolerance);
39+
}
40+
});
41+
2142
test("generateCodeChallenge produces valid base64url S256 hash", async () => {
2243
const verifier = "dBjftJeZ4CVP-mB92K27uhbUJU1p1r_wW1gFWFOEjXk";
2344
const challenge = await generateCodeChallenge(verifier);

packages/cli-core/src/lib/pkce.ts

Lines changed: 12 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -5,14 +5,21 @@
55

66
const VERIFIER_LENGTH = 43;
77
const CHARSET = "ABCDEFGHIJKLMNOPQRSTUVWXYZabcdefghijklmnopqrstuvwxyz0123456789-._~";
8+
// Largest multiple of CHARSET.length that fits in a byte. Values >= this
9+
// would map non-uniformly via modulo, so reject them (rejection sampling).
10+
const REJECTION_THRESHOLD = 256 - (256 % CHARSET.length);
811

912
export function generateCodeVerifier(): string {
10-
const bytes = crypto.getRandomValues(new Uint8Array(VERIFIER_LENGTH));
11-
let verifier = "";
12-
for (const byte of bytes) {
13-
verifier += CHARSET[byte % CHARSET.length];
13+
const verifier: string[] = [];
14+
while (verifier.length < VERIFIER_LENGTH) {
15+
const bytes = crypto.getRandomValues(new Uint8Array(VERIFIER_LENGTH));
16+
for (const byte of bytes) {
17+
if (byte >= REJECTION_THRESHOLD) continue;
18+
verifier.push(CHARSET.charAt(byte % CHARSET.length));
19+
if (verifier.length === VERIFIER_LENGTH) break;
20+
}
1421
}
15-
return verifier;
22+
return verifier.join("");
1623
}
1724

1825
export async function generateCodeChallenge(verifier: string): Promise<string> {

0 commit comments

Comments
 (0)