Skip to content

Commit e243b04

Browse files
authored
fix(security): unbiased maxai X-Random nonce + stricter URL/regex assertions (#12502)
Drains 7 of the 13 open CodeQL alerts that put the `codeql-ratchet` gate into regression (13 > baseline 11) on every open PR. The alerts arrived with the recent provider/media merges (#11461 MaxAI, #11513 UC, #12365 prefix shadowing), not with the work they are currently blocking. Production fix (js/biased-cryptographic-random): - open-sse/executors/maxai/signing.ts: the 6-digit `X-Random` wire slot was drawn as `randomBytes(4).readUInt32BE(0) % 900000`. 2^32 does not divide evenly by 900000, so the low ~4772 values of the range came out marginally more often. Extracted as `maxaiRandomSlot()` over `crypto.randomInt`, which rejection-samples internally. The emitted shape is unchanged (6 digits). Test assertions strengthened (never weakened): - tests/unit/helpers/ucClerkUrl.ts (new): `isUcClerkMintUrl()` matches the Clerk mint call by parsed origin (against `UC_CLERK_FAPI`) plus the `/v1/client/sessions/{sid}/tokens` path shape. - tests/unit/uc-image.test.ts, tests/unit/uc-video.test.ts: the mock fetch routers dispatched on `url.includes("clerk.uncensored.com")`, so any host merely embedding the name was served the mint response — a malformed URL built by the executor could not fail the test (js/incomplete-url-substring-sanitization x4). - tests/unit/maxai-image.test.ts: `new RegExp(PATH.replace(/\//g, "\\/"))` escaped only slashes (which need no escaping) and matched the path anywhere in a wrong URL; replaced by exact URL equality (js/incomplete-sanitization). - tests/unit/custom-provider-prefix-shadowing-11943.test.ts: the expected node mention was a RegExp with only `()` hand-escaped; replaced by an exact substring check (js/incomplete-sanitization). - tests/unit/maxai.test.ts: regression guard for the X-Random slot (6 digits, in range, spread across both halves of the range). The remaining 6 alerts are not defects and are left for an operator dismissal with justification (Hard Rule #14): the MaxAI HMAC-SHA1/SM3 signature and the CryptoJS `EVP_BytesToKey(MD5)` derivation are wire-protocol requirements — changing either breaks the provider — and `open-sse/utils/error.ts:749` already routes through `sanitizeErrorMessage()` (documented CodeQL sanitizer blind spot, docs/security/ERROR_SANITIZATION.md). Co-authored-by: Markus Hartung <diegosouzapw@users.noreply.github.com>
1 parent bf0d902 commit e243b04

7 files changed

Lines changed: 128 additions & 28 deletions

File tree

open-sse/executors/maxai/signing.ts

Lines changed: 20 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -23,7 +23,7 @@
2323
* The non-secret STRUCTURAL fields (appVersion, ctxKey, header names) carry safe
2424
* defaults so a transient parse miss can't break an otherwise-working signer.
2525
*/
26-
import { createHmac, createHash, createCipheriv, randomBytes } from "node:crypto";
26+
import { createHmac, createHash, createCipheriv, randomBytes, randomInt } from "node:crypto";
2727
import type { MaxaiSigningConstants, MaxaiHeaderNames } from "./constants.ts";
2828
import { MAXAI_DEFAULT_HEADER_NAMES } from "./constants.ts";
2929

@@ -39,8 +39,22 @@ const BLANK_USER_ROUTES = new Set([
3939

4040
const MAGIC = Buffer.from("Salted__", "ascii");
4141

42+
/**
43+
* The wire `X-Random` slot: a 6-digit decimal string (100000-999999).
44+
*
45+
* Uses `crypto.randomInt`, which rejection-samples internally, instead of
46+
* `randomBytes(4) % 900000` — a plain modulo over a 32-bit draw does not divide
47+
* evenly by 900000, so the low ~4772 values of the range came out marginally
48+
* more often. The emitted shape is unchanged (always exactly 6 digits).
49+
*/
50+
export function maxaiRandomSlot(): string {
51+
return String(randomInt(100000, 1000000));
52+
}
53+
4254
function hmacSha1Hex(message: string, key: string): string {
43-
return createHmac("sha1", Buffer.from(key, "utf8")).update(Buffer.from(message, "utf8")).digest("hex");
55+
return createHmac("sha1", Buffer.from(key, "utf8"))
56+
.update(Buffer.from(message, "utf8"))
57+
.digest("hex");
4458
}
4559

4660
function sm3Hex(message: string): string {
@@ -58,7 +72,9 @@ function evpBytesToKey(
5872
let block = Buffer.alloc(0);
5973
const pass = Buffer.from(passphrase, "utf8");
6074
while (derived.length < keyLen + ivLen) {
61-
block = createHash("md5").update(Buffer.concat([block, pass, salt])).digest();
75+
block = createHash("md5")
76+
.update(Buffer.concat([block, pass, salt]))
77+
.digest();
6278
derived = Buffer.concat([derived, block]);
6379
}
6480
return { key: derived.subarray(0, keyLen), iv: derived.subarray(keyLen, keyLen + ivLen) };
@@ -124,8 +140,7 @@ export function buildMaxaiSignedHeaders(
124140
constants: MaxaiSigningConstants
125141
): Record<string, string> {
126142
const reqTime = (input.now ?? (() => Date.now()))();
127-
const random =
128-
input.random?.() ?? String((randomBytes(4).readUInt32BE(0) % 900000) + 100000);
143+
const random = input.random?.() ?? maxaiRandomSlot();
129144
const h: MaxaiHeaderNames = { ...MAXAI_DEFAULT_HEADER_NAMES, ...constants.headerNames };
130145
const ctxKey = constants.ctxKey;
131146
const appVersion = constants.appVersion;

tests/unit/custom-provider-prefix-shadowing-11943.test.ts

Lines changed: 8 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -95,10 +95,14 @@ test("handleChat names the shadowed custom node when the built-in prefix has no
9595
/prefix "of" is reserved by the built-in provider "openference"/,
9696
`runtime error must explain that the prefix resolved to the built-in, got: ${message}`
9797
);
98-
assert.match(
99-
message,
100-
new RegExp(`"${SHADOWED_NODE_NAME.replace(/[()]/g, "\\$&")}" \\(${SHADOWED_NODE_ID}\\)`),
101-
`runtime error must name the shadowed node and its id, got: ${message}`
98+
// Exact substring, not a hand-escaped RegExp: the name carries regex
99+
// metacharacters (parentheses) and the previous `.replace(/[()]/g, …)` escaped
100+
// only those, so any other metachar in a future name would have been
101+
// interpreted instead of matched literally (CodeQL js/incomplete-sanitization).
102+
const expectedNodeMention = `"${SHADOWED_NODE_NAME}" (${SHADOWED_NODE_ID})`;
103+
assert.ok(
104+
message.includes(expectedNodeMention),
105+
`runtime error must name the shadowed node and its id (${expectedNodeMention}), got: ${message}`
102106
);
103107
assert.match(message, /Rename that node's prefix/);
104108
});

tests/unit/helpers/ucClerkUrl.ts

Lines changed: 31 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,31 @@
1+
/**
2+
* Strict recognizer for the UC (uncensored.com) Clerk session-token mint call,
3+
* shared by the uc-image / uc-video mock `fetch` routers.
4+
*
5+
* The mock routers used to dispatch on `url.includes("clerk.uncensored.com")`.
6+
* That is a substring test over a whole URL, so ANY host answers as long as the
7+
* name appears somewhere in it — `https://evil.example/?next=clerk.uncensored.com`
8+
* would have been served the mint response. A test whose router accepts a
9+
* malformed URL cannot fail when the executor builds one, which is exactly the
10+
* regression such a test exists to catch (and CodeQL flags it as
11+
* `js/incomplete-url-substring-sanitization`).
12+
*
13+
* This matches the real shape instead:
14+
* POST https://clerk.uncensored.com/v1/client/sessions/{sid}/tokens?_clerk_js_version=…
15+
* comparing the parsed origin against the production constant and pinning the
16+
* path shape.
17+
*/
18+
import { UC_CLERK_FAPI } from "../../../open-sse/executors/uc/constants.ts";
19+
20+
const MINT_PATH = /^\/v1\/client\/sessions\/[^/]+\/tokens$/;
21+
22+
/** True only for the Clerk mint endpoint on the real Clerk FAPI origin. */
23+
export function isUcClerkMintUrl(raw: unknown): boolean {
24+
let parsed: URL;
25+
try {
26+
parsed = new URL(String(raw));
27+
} catch {
28+
return false;
29+
}
30+
return parsed.origin === UC_CLERK_FAPI && MINT_PATH.test(parsed.pathname);
31+
}

tests/unit/maxai-image.test.ts

Lines changed: 14 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
} from "../../open-sse/handlers/imageGeneration/providers/maxaiImage.ts";
1010
import { IMAGE_PROVIDERS } from "../../open-sse/config/imageRegistry.ts";
1111
import { __setMaxaiConstantsForTest } from "../../open-sse/executors/maxai/constantsStore.ts";
12+
import { MAXAI_BASE_URL } from "../../open-sse/executors/maxai/protocol.ts";
1213
import { MOCK_CONSTANTS } from "./helpers/maxaiMockConstants.ts";
1314

1415
// Image generation signs like any request; seed the in-process constants memo
@@ -28,7 +29,9 @@ const CRED = {
2829
// --- Registry ------------------------------------------------------------
2930

3031
test("maxai is registered in IMAGE_PROVIDERS with the maxai-image format + 6 models", () => {
31-
const entry = (IMAGE_PROVIDERS as Record<string, { format?: string; baseUrl?: string; models?: unknown[] }>)["maxai"];
32+
const entry = (
33+
IMAGE_PROVIDERS as Record<string, { format?: string; baseUrl?: string; models?: unknown[] }>
34+
)["maxai"];
3235
assert.ok(entry, "maxai must exist in IMAGE_PROVIDERS");
3336
assert.equal(entry.format, "maxai-image");
3437
assert.match(String(entry.baseUrl), /api\.maxai\.me\/gpt\/get_image_generate_response/);
@@ -93,7 +96,10 @@ test("handleMaxaiImageGeneration returns OpenAI image data on success", async ()
9396
ok: true,
9497
status: 200,
9598
async json() {
96-
return { status: "OK", data: [{ png_url: "https://cdn/x.png", webp_url: "https://cdn/x.webp" }] };
99+
return {
100+
status: "OK",
101+
data: [{ png_url: "https://cdn/x.png", webp_url: "https://cdn/x.webp" }],
102+
};
97103
},
98104
async text() {
99105
return "";
@@ -111,8 +117,12 @@ test("handleMaxaiImageGeneration returns OpenAI image data on success", async ()
111117

112118
assert.equal(result.success, true);
113119
assert.deepEqual(result.data?.data, [{ url: "https://cdn/x.png" }]);
114-
// Hit the image endpoint with the signed body.
115-
assert.match(capturedUrl, new RegExp(MAXAI_IMAGE_PATH.replace(/\//g, "\\/")));
120+
// Hit the image endpoint with the signed body. Exact URL equality instead of a
121+
// hand-escaped RegExp over the path — the old `.replace(/\//g, "\\/")` escaped
122+
// only slashes (which need no escaping in a RegExp anyway) and would have let
123+
// any other metacharacter through (CodeQL js/incomplete-sanitization), while
124+
// also accepting the path appearing anywhere in a wrong URL.
125+
assert.equal(capturedUrl, MAXAI_BASE_URL + MAXAI_IMAGE_PATH);
116126
assert.equal(capturedBody.model_name, "flux-1-schnell");
117127
assert.equal(capturedBody.size, "512x512"); // flux passes size through
118128
assert.equal(capturedBody.n, 2);

tests/unit/maxai.test.ts

Lines changed: 49 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -12,6 +12,7 @@ import {
1212
computeMaxaiProof,
1313
maxaiAesEncrypt,
1414
buildMaxaiSignedHeaders,
15+
maxaiRandomSlot,
1516
} from "../../open-sse/executors/maxai/signing.ts";
1617
import {
1718
assembleMaxaiContext,
@@ -103,7 +104,13 @@ test("computeMaxaiProof blanks the user id only on /oauth/* routes", () => {
103104
// A blank-user route yields a different proof than the same route with a uid,
104105
// proving the uid is dropped for /oauth/* (and only there).
105106
const t = 1784594159681;
106-
const oauthWithUid = computeMaxaiProof("/oauth/signin_with_email", t, USER_ID, HMAC_KEY, APP_VERSION);
107+
const oauthWithUid = computeMaxaiProof(
108+
"/oauth/signin_with_email",
109+
t,
110+
USER_ID,
111+
HMAC_KEY,
112+
APP_VERSION
113+
);
107114
const oauthNoUid = computeMaxaiProof("/oauth/signin_with_email", t, "", HMAC_KEY, APP_VERSION);
108115
assert.equal(oauthWithUid, oauthNoUid); // uid ignored for /oauth/*
109116
const chatWithUid = computeMaxaiProof("/gpt/cwc/chat", t, USER_ID, HMAC_KEY, APP_VERSION);
@@ -306,7 +313,28 @@ test("buildMaxaiSignedHeaders emits the X-App/X-Browser companions + X-Authoriza
306313
assert.equal(h["X-App-Version"], MOCK_APP_VERSION);
307314
assert.equal(h["X-App-Env"], "MaxAI-Browser-Extension");
308315
assert.ok(h["X-Authorization"].length > 0);
309-
assert.equal(Buffer.from(h["X-Authorization"], "base64").subarray(0, 8).toString("ascii"), "Salted__");
316+
assert.equal(
317+
Buffer.from(h["X-Authorization"], "base64").subarray(0, 8).toString("ascii"),
318+
"Salted__"
319+
);
320+
});
321+
322+
test("maxaiRandomSlot emits an unbiased 6-digit X-Random slot", () => {
323+
// The wire slot is always exactly 6 decimal digits, i.e. 100000-999999.
324+
const samples = Array.from({ length: 4000 }, () => maxaiRandomSlot());
325+
for (const s of samples) {
326+
assert.match(s, /^\d{6}$/, `X-Random must be 6 digits, got: ${s}`);
327+
const n = Number(s);
328+
assert.ok(n >= 100000 && n <= 999999, `X-Random out of range: ${s}`);
329+
}
330+
// Regression guard for the modulo bias the previous
331+
// `randomBytes(4).readUInt32BE(0) % 900000` draw introduced: the value must
332+
// still spread across the whole range, not collapse onto its low end.
333+
assert.ok(new Set(samples).size > samples.length * 0.9, "X-Random must not repeat heavily");
334+
assert.ok(
335+
samples.some((s) => Number(s) < 550000) && samples.some((s) => Number(s) >= 550000),
336+
"X-Random must cover both halves of the 100000-999999 range"
337+
);
310338
});
311339

312340
// ── Context assembly ─────────────────────────────────────────────────────────
@@ -364,7 +392,12 @@ test("contentToText flattens multipart content, dropping non-text parts", () =>
364392
});
365393

366394
test("buildMaxaiChatBody pins field order + constants", () => {
367-
const body = buildMaxaiChatBody({ conversationId: "conv-1", text: "hi", modelName: "gpt-5.6", appVersion: APP_VERSION });
395+
const body = buildMaxaiChatBody({
396+
conversationId: "conv-1",
397+
text: "hi",
398+
modelName: "gpt-5.6",
399+
appVersion: APP_VERSION,
400+
});
368401
const keys = Object.keys(body);
369402
assert.equal(keys[0], "chat_mode");
370403
assert.equal(keys[3], "message_content");
@@ -379,7 +412,12 @@ test("buildMaxaiChatBody pins field order + constants", () => {
379412
// ── Vision input (image_url parts) ───────────────────────────────────────────
380413

381414
test("buildMaxaiChatBody text-only path is unchanged (no imageUrls)", () => {
382-
const body = buildMaxaiChatBody({ conversationId: "c", text: "hi", modelName: "gpt-5.6", appVersion: APP_VERSION });
415+
const body = buildMaxaiChatBody({
416+
conversationId: "c",
417+
text: "hi",
418+
modelName: "gpt-5.6",
419+
appVersion: APP_VERSION,
420+
});
383421
// Byte-identical to the pre-vision shape: a single text part.
384422
assert.deepEqual(body.message_content, [{ type: "text", text: "hi" }]);
385423
assert.deepEqual(body.doc_list, []);
@@ -563,8 +601,7 @@ test("maxaiRefreshAccessToken sends the exact web-app request + parses data.acce
563601

564602
test("maxaiRefreshAccessToken returns a structured error on non-200 (no throw)", async () => {
565603
const nowSec = Math.floor(Date.now() / 1000);
566-
const fakeFetch = (async () =>
567-
new Response("nope", { status: 418 })) as unknown as typeof fetch;
604+
const fakeFetch = (async () => new Response("nope", { status: 418 })) as unknown as typeof fetch;
568605
const result = await maxaiRefreshAccessToken({
569606
refreshToken: fakeJwt(nowSec + 1000, USER_ID),
570607
deviceId: "dev",
@@ -687,7 +724,9 @@ test("verifyMaxaiEmailCode maps code 10119 to an expired-code message", async ()
687724

688725
test("verifyMaxaiEmailCode defaults to an invalid-code message otherwise", async () => {
689726
const fakeFetch = (async () =>
690-
new Response(JSON.stringify({ data: { status: "FAIL" } }), { status: 200 })) as unknown as typeof fetch;
727+
new Response(JSON.stringify({ data: { status: "FAIL" } }), {
728+
status: 200,
729+
})) as unknown as typeof fetch;
691730
const r = await verifyMaxaiEmailCode({
692731
email: "x@y.z",
693732
code: "999999",
@@ -1009,10 +1048,9 @@ test("discoverMaxaiModels drops deprecated, non-chat, and non-curated models", a
10091048

10101049
test("discoverMaxaiModels falls back to the catalog window when max_tokens is absent", async () => {
10111050
const fakeFetch = (async () =>
1012-
new Response(
1013-
modelsConfigBody([{ model_name: "claude-5-sonnet", type: "chat" }]),
1014-
{ status: 200 }
1015-
)) as unknown as typeof fetch;
1051+
new Response(modelsConfigBody([{ model_name: "claude-5-sonnet", type: "chat" }]), {
1052+
status: 200,
1053+
})) as unknown as typeof fetch;
10161054
const { models } = await discoverMaxaiModels({
10171055
providerSpecificData: DISCOVERY_CRED.providerSpecificData,
10181056
accessToken: DISCOVERY_CRED.accessToken,

tests/unit/uc-image.test.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9,6 +9,7 @@ import {
99
UC_DIRECT_IMAGE_URL,
1010
} from "../../open-sse/handlers/imageGeneration/providers/ucImage.ts";
1111
import { IMAGE_PROVIDERS, parseImageModel } from "../../open-sse/config/imageRegistry.ts";
12+
import { isUcClerkMintUrl } from "./helpers/ucClerkUrl.ts";
1213

1314
// A valid PERSONA credential (durable Clerk cookie + sid + uid in psd). No API
1415
// key, so the handler takes the persona web path (mint -> POST -> poll).
@@ -144,7 +145,7 @@ function personaFetch(opts: {
144145
let pollsSeen = 0;
145146
return (async (url: string, init: RequestInit = {}) => {
146147
// 1) Clerk mint
147-
if (url.includes("clerk.uncensored.com")) {
148+
if (isUcClerkMintUrl(url)) {
148149
return {
149150
ok: true,
150151
status: 200,
@@ -265,7 +266,7 @@ test("handleUcImageGeneration (persona) times out with 504 when the result never
265266

266267
test("handleUcImageGeneration (persona) surfaces a Clerk mint failure", async () => {
267268
const fetchImpl = (async (url: string) => {
268-
if (url.includes("clerk.uncensored.com")) {
269+
if (isUcClerkMintUrl(url)) {
269270
return {
270271
ok: false,
271272
status: 401,

tests/unit/uc-video.test.ts

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -13,6 +13,7 @@ import {
1313
UC_DIRECT_VIDEO_URL,
1414
} from "../../open-sse/handlers/videoGeneration/providers/ucVideo.ts";
1515
import { VIDEO_PROVIDERS } from "../../open-sse/config/videoRegistry.ts";
16+
import { isUcClerkMintUrl } from "./helpers/ucClerkUrl.ts";
1617

1718
// A valid PERSONA credential (durable Clerk cookie + sid + uid in psd). No API
1819
// key, so the handler takes the persona web path (mint -> generate -> poll).
@@ -148,7 +149,7 @@ function personaFetch(opts: {
148149
let pollsSeen = 0;
149150
return (async (url: string, init: RequestInit = {}) => {
150151
// Clerk mint
151-
if (url.includes("clerk.uncensored.com")) {
152+
if (isUcClerkMintUrl(url)) {
152153
return {
153154
ok: true,
154155
status: 200,
@@ -339,7 +340,7 @@ test("handleUcVideoGeneration (persona) times out with 504 when never ready", as
339340

340341
test("handleUcVideoGeneration (persona) surfaces a Clerk mint failure", async () => {
341342
const fetchImpl = (async (url: string) => {
342-
if (url.includes("clerk.uncensored.com")) {
343+
if (isUcClerkMintUrl(url)) {
343344
return {
344345
ok: false,
345346
status: 401,

0 commit comments

Comments
 (0)