From ea98d44d0325de2ad32bd62d176ec966d3f9d139 Mon Sep 17 00:00:00 2001 From: HoyeongJeon Date: Mon, 14 Sep 2026 21:54:19 +0900 Subject: [PATCH 1/3] fix(js-x-ray): measure the scrypt salt length in bytes --- .changeset/weak-scrypt-salt-bytes.md | 5 ++ docs/crypto.weak-scrypt.md | 2 +- .../src/probes/crypto/isWeakScrypt.ts | 2 +- .../test/probes/crypto/isWeakScrypt.spec.ts | 78 ++++++++----------- 4 files changed, 41 insertions(+), 46 deletions(-) create mode 100644 .changeset/weak-scrypt-salt-bytes.md diff --git a/.changeset/weak-scrypt-salt-bytes.md b/.changeset/weak-scrypt-salt-bytes.md new file mode 100644 index 00000000..38ab09cb --- /dev/null +++ b/.changeset/weak-scrypt-salt-bytes.md @@ -0,0 +1,5 @@ +--- +"@nodesecure/js-x-ray": patch +--- + +Measure the scrypt salt length in bytes instead of characters diff --git a/docs/crypto.weak-scrypt.md b/docs/crypto.weak-scrypt.md index e4e6bdc7..f28125e6 100644 --- a/docs/crypto.weak-scrypt.md +++ b/docs/crypto.weak-scrypt.md @@ -9,7 +9,7 @@ Detect usage of **weak scrypt** parameters with the Node.js core `crypto.scrypt()` function. This probe checks for: - **low-cost**: scrypt parameters (cost, blockSize, parallelization) that do not meet [OWASP minimum recommendations](https://cheatsheetseries.owasp.org/cheatsheets/Password_Storage_Cheat_Sheet.html#scrypt). -- **short-salt**: salt is a hardcoded string literal shorter than 16 characters. +- **short-salt**: salt is a hardcoded string literal shorter than 16 bytes. - **hardcoded-salt**: salt is a hardcoded string literal (should be randomly generated). ## Example diff --git a/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts b/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts index 6ebbce4d..f9e0a2f8 100644 --- a/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts +++ b/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts @@ -110,7 +110,7 @@ function main(node: ESTree.CallExpression, ctx: ProbeContext) { } if (isStringLiteral(salt)) { - if (typeof salt.value === "string" && salt.value.length < 16) { + if (typeof salt.value === "string" && Buffer.byteLength(salt.value) < 16) { sourceFile.warnings.push( generateWarning("crypto.weak-scrypt", { value: "short-salt", diff --git a/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts b/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts index bf49149b..42a2d941 100644 --- a/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts +++ b/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts @@ -5,16 +5,20 @@ import { describe, it } from "node:test"; // Import Internal Dependencies import { AstAnalyser } from "../../../src/AstAnalyser.ts"; +function analyse(code: string) { + return new AstAnalyser({ + optionalWarnings: ["crypto.weak-scrypt"] + }).analyse(code); +} + describe("isWeakScrypt", () => { describe("short-salt", () => { - it("should warn when salt is a short string literal (less than 16 chars)", () => { + it("should warn when salt is a short string literal (less than 16 bytes)", () => { const code = ` import crypto from 'crypto'; crypto.scrypt(password, "short", 64, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); @@ -26,9 +30,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, "", 64, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); @@ -37,14 +39,24 @@ describe("isWeakScrypt", () => { }); describe("hardcoded-salt", () => { - it("should warn when salt is a hardcoded string literal (16 chars or more)", () => { + it("should warn when salt is a hardcoded string literal of 16 bytes or more", () => { const code = ` import crypto from 'crypto'; crypto.scrypt(password, "this-is-a-long-hardcoded-salt", 64, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); + + assert.strictEqual(outputWarnings.length, 1); + assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); + assert.strictEqual(outputWarnings[0].value, "hardcoded-salt"); + }); + + it("should measure the length of salt in bytes (6 chars but 18 bytes)", () => { + const code = ` + import crypto from 'crypto'; + crypto.scrypt(password, "가나다라마바", 64, (err, key) => {}); + `; + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); @@ -58,9 +70,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { cost: 1024 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); @@ -72,9 +82,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { "cost": 1024 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].kind, "crypto.weak-scrypt"); @@ -86,9 +94,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { cost: 16384 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].value, "low-cost"); @@ -99,9 +105,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { N: 131072, r: 4 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].value, "low-cost"); @@ -112,9 +116,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { N: 8192, p: 9 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 1); assert.strictEqual(outputWarnings[0].value, "low-cost"); @@ -125,9 +127,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { cost: 16384, parallelization: 5 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 0); }); @@ -137,9 +137,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { N: 131072, p: 1, r: 8 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 0); }); @@ -149,9 +147,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, salt, 64, { N: 8192, p: 10 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 0); }); @@ -163,9 +159,7 @@ describe("isWeakScrypt", () => { import crypto from 'crypto'; crypto.scrypt(password, "abc", 64, { cost: 1024 }, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 2); const values = outputWarnings.map((w) => w.value); @@ -181,9 +175,7 @@ describe("isWeakScrypt", () => { const salt = crypto.randomBytes(16); crypto.scrypt(password, salt, 64, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 0); }); @@ -193,9 +185,7 @@ describe("isWeakScrypt", () => { const crypto = { scrypt() {} }; crypto.scrypt(password, "short", 64, (err, key) => {}); `; - const { warnings: outputWarnings } = new AstAnalyser({ - optionalWarnings: ["crypto.weak-scrypt"] - }).analyse(code); + const { warnings: outputWarnings } = analyse(code); assert.strictEqual(outputWarnings.length, 0); }); From 657753a5ef38fa82b2ac61c07cc2b0b1042f49c2 Mon Sep 17 00:00:00 2001 From: HoyeongJeon Date: Mon, 14 Sep 2026 22:12:25 +0900 Subject: [PATCH 2/3] feat(js-x-ray): merge weak-scrypt warnings into one per call --- .changeset/weak-scrypt-single-warning.md | 5 +++ .../src/probes/crypto/isWeakScrypt.ts | 36 +++++++------------ .../test/probes/crypto/isWeakScrypt.spec.ts | 8 ++--- 3 files changed, 21 insertions(+), 28 deletions(-) create mode 100644 .changeset/weak-scrypt-single-warning.md diff --git a/.changeset/weak-scrypt-single-warning.md b/.changeset/weak-scrypt-single-warning.md new file mode 100644 index 00000000..5d7cf51e --- /dev/null +++ b/.changeset/weak-scrypt-single-warning.md @@ -0,0 +1,5 @@ +--- +"@nodesecure/js-x-ray": minor +--- + +Report one weak-scrypt warning per call with every failing check joined in the value diff --git a/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts b/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts index f9e0a2f8..054a92f6 100644 --- a/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts +++ b/workspaces/js-x-ray/src/probes/crypto/isWeakScrypt.ts @@ -76,6 +76,7 @@ function main(node: ESTree.CallExpression, ctx: ProbeContext) { const { sourceFile } = ctx; const salt = node.arguments.at(1); const options = node.arguments.at(3); + const reasons: string[] = []; if (options && options.type === "ObjectExpression") { const { properties } = options; @@ -99,33 +100,22 @@ function main(node: ESTree.CallExpression, ctx: ProbeContext) { parallelizationValue ?? kDefaultParallelization ) ) { - sourceFile.warnings.push( - generateWarning("crypto.weak-scrypt", { - value: "low-cost", - location: node.loc - }) - ); + reasons.push("low-cost"); } } } - if (isStringLiteral(salt)) { - if (typeof salt.value === "string" && Buffer.byteLength(salt.value) < 16) { - sourceFile.warnings.push( - generateWarning("crypto.weak-scrypt", { - value: "short-salt", - location: node.loc - }) - ); - } - else { - sourceFile.warnings.push( - generateWarning("crypto.weak-scrypt", { - value: "hardcoded-salt", - location: node.loc - }) - ); - } + if (isStringLiteral(salt) && typeof salt.value === "string") { + reasons.push(Buffer.byteLength(salt.value) < 16 ? "short-salt" : "hardcoded-salt"); + } + + if (reasons.length > 0) { + sourceFile.warnings.push( + generateWarning("crypto.weak-scrypt", { + value: reasons.join(", "), + location: node.loc + }) + ); } } diff --git a/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts b/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts index 42a2d941..b8f6275c 100644 --- a/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts +++ b/workspaces/js-x-ray/test/probes/crypto/isWeakScrypt.spec.ts @@ -154,17 +154,15 @@ describe("isWeakScrypt", () => { }); describe("combined warnings", () => { - it("should emit both short-salt and low-cost warnings", () => { + it("should report low-cost and short-salt in a single warning", () => { const code = ` import crypto from 'crypto'; crypto.scrypt(password, "abc", 64, { cost: 1024 }, (err, key) => {}); `; const { warnings: outputWarnings } = analyse(code); - assert.strictEqual(outputWarnings.length, 2); - const values = outputWarnings.map((w) => w.value); - assert.ok(values.includes("short-salt")); - assert.ok(values.includes("low-cost")); + assert.strictEqual(outputWarnings.length, 1); + assert.strictEqual(outputWarnings[0].value, "low-cost, short-salt"); }); }); From 35070362dc2075fca3574460cca1aa31aa3b35c4 Mon Sep 17 00:00:00 2001 From: HoyeongJeon Date: Mon, 14 Sep 2026 23:39:18 +0900 Subject: [PATCH 3/3] chore: combine both changesets into one --- .changeset/weak-scrypt-salt-bytes.md | 5 ----- .changeset/weak-scrypt-single-warning.md | 2 +- 2 files changed, 1 insertion(+), 6 deletions(-) delete mode 100644 .changeset/weak-scrypt-salt-bytes.md diff --git a/.changeset/weak-scrypt-salt-bytes.md b/.changeset/weak-scrypt-salt-bytes.md deleted file mode 100644 index 38ab09cb..00000000 --- a/.changeset/weak-scrypt-salt-bytes.md +++ /dev/null @@ -1,5 +0,0 @@ ---- -"@nodesecure/js-x-ray": patch ---- - -Measure the scrypt salt length in bytes instead of characters diff --git a/.changeset/weak-scrypt-single-warning.md b/.changeset/weak-scrypt-single-warning.md index 5d7cf51e..8409d549 100644 --- a/.changeset/weak-scrypt-single-warning.md +++ b/.changeset/weak-scrypt-single-warning.md @@ -2,4 +2,4 @@ "@nodesecure/js-x-ray": minor --- -Report one weak-scrypt warning per call with every failing check joined in the value +Measure the scrypt salt length in bytes and report one warning per call with every failing check joined in the value \ No newline at end of file