fix(js-x-ray): measure scrypt salt in bytes and merge warnings into one per call

.changeset/weak-scrypt-single-warning.md

@@ -0,0 +1,5 @@
+---
+"@nodesecure/js-x-ray": minor
+---
+
+Measure the scrypt salt length in bytes and report one warning per call with every failing check joined in the value

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

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" && salt.value.length < 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
+      })
+    );
   }
 }
 

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,28 +147,22 @@ 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);
     });
   });
 
   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 } = 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);
-      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");
     });
   });
 
@@ -181,9 +173,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 +183,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);
     });