Repository navigation
feat: add JavaScript/TypeScript crypto detection for Node.js - #539
Siddhartha20242 wants to merge 20 commits into
Conversation
824c228 to
201e343
Compare
Adds JavaScript module with ESLint bridge for parsing JS/TS. Detection rules cover: - Hash (createHash, update, digest) - HMAC (createHmac, update, digest) - Ciphers (createCipheriv, createDecipheriv) - Signatures (createSign, createVerify) - RSA public/private key encryption - KDF (PBKDF2, scrypt, HKDF) - Key generation & DH/ECDH - PRNG detection - TLS protocol and cipher suite config Includes CBOM translation layer, sensor integration, and test suite. Reference of the Issue: cbomkit#521 Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Before: generateKeyPairSync('ed25519', {
publicKeyEncoding: {...}
}) lost the object argument because the runner parsed one line at a time.
After: statements spanning multiple lines are joined before regex parsing,
so all arguments are captured. Also tracks the start line for accurate
source locations.
Verified on chain-test: generateKeyPairSync now reports 2 args
['string', 'object'] instead of 1.
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
6a06e7c to
43cd0d7
Compare
|
Hi @san-zrl, this PR is ready for review whenever you have a moment, could you take a look? |
|
Hi @Siddhartha20242 - Can you fix the build error before we proceed with this PR? Tx. |
Standalone calls like crypto.createHash('sha256').digest() were dropped
by the detection engine. Now always wrap in CallExpressionWithBlockTree
so both assigned and standalone calls use the same code path.
Verified: FemtoCoin detections went from 1 to 5 assets.
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
|
Hi @Siddhartha20242 - This looks better, but now we have a test failure. |
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
|
Hi @Siddhartha20242 - The build behavior is highly dependent on the environment. Are there dependencies on external tools that may differ between your system, my system and the CI VM? On my system it fails with |
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
|
Hi @san-zrl , the JS module needs Node.js which is automatically downloaded by frontend-maven-plugin during build). The eslint-runner.js uses only built-in Node modules (fs, path) and there is no external npm packages. That's why I removed the failing npm install step: there was nothing to install. Requirements: JDK 17 + internet on first build. No npm packages needed. All checks are green on 787a3c5 which is ready for review when you have a moment. 
|
|
Hi @Siddhartha20242 - I ran an automatic evaluation with our standard AI. No idea how good this really is but it gives a few hints that should be addressed. Please have a look and let me know your thoughts. We can do a deeper analysis later. PR-539 Evaluation: JavaScript/TypeScript Crypto Detection for Node.jsSummaryThis PR adds a new 1. Architecture & Design✅ What's well done
|
| Area | Status |
|---|---|
| Happy-path single-file detection | ✅ |
| CBOM node type assertions (hash, HMAC, KDF, key agreement) | ✅ |
| Variable constant resolution | ✅ (single level only) |
node:crypto / ES module imports |
❌ |
generateKeyPair / generateKeyPairSync |
❌ (rules disabled) |
| Synchronous KDF variants | ❌ |
TLS connect / rejectUnauthorized |
❌ |
| Dynamic algorithm expressions | ❌ |
| Weak algorithm detection | ❌ |
| Parser error recovery | ❌ |
| Multiple findings per file / deduplication | ❌ |
| Column accuracy in issue reporting | ❌ |
JavaScriptNoMD5UseRule reporting correct rule key |
❌ |
Most per-family tests (e.g., NodeCryptoCipherTest) extend TestBase but delegate to the shared verifier with a one-line body and minimal assertions. Finding order is assumed by positional findingId, making the tests fragile to traversal-order changes.
9. Checklist Items
- DCO sign-off — not verified on all commits; should be checked before merge.
mvn clean packagepasses — the PR description checklist item is unchecked, suggesting CI may not be green.
Blockers (must fix before merge)
| # | Issue | Location |
|---|---|---|
| B1 | Utils.printNodeTree called unconditionally — leaks algorithm data to logs on every analysis |
JavaScriptTranslationProcess.java |
| B2 | createCipher/createDecipher absent from runner's cryptoMethods — rules defined in Java will never fire |
eslint-runner.js |
| B3 | currentRule ignored in reportIssue() — all issues reported under wrong rule key |
JavaScriptScanContext.java |
| B4 | No process timeout on Node subprocess — can hang SonarQube analysis indefinitely | ESLintExecutor.java |
| B5 | .gitignore excludes javascript/src/main/resources/node/ but eslint-runner.js lives there |
.gitignore |
| B6 | minimizeJar: false — unexplained regression in plugin JAR size; root cause must be documented |
sonar-cryptography-plugin/pom.xml |
High-Priority Issues (should fix before merge)
| # | Issue | Location |
|---|---|---|
| H1 | generateKeyPair/generateKeyPairSync disabled with TODO; Javadoc claims they are detected |
NodeCryptoKeyGen.java |
| H2 | False-positive risk: generic method names (update, digest, connect) matched on any object |
eslint-runner.js |
| H3 | variableValues never populated by runner; variable resolution always empty |
eslint-runner.js |
| H4 | currentContext overwritten on every block entry, never restored; scope resolution incorrect for nested blocks |
JavaScriptDetectionEngine.java |
| H5 | createSecretKey tagged GENERATION but semantically is an import; CBOM will misreport |
NodeCryptoKeyGen.java |
| H6 | Column discarded; all issues reported at line level only | JavaScriptScanContext.java |
| H7 | Bracket counting ignores string/comment/regex content — malformed statement buffering on real-world JS | eslint-runner.js |
| H8 | Path traversal in JAR extraction; no canonical-path containment check | ESLintExecutor.java |
Minor / Cleanup Issues
fsandpathimported but unused in runnerLOGGERdeclared but unused inJavaScriptLanguageTranslationCollections.unmodifiableList(enrichedValues.stream().toList())— double-wrapping;stream().toList()is already unmodifiablesonar-cryptography-plugin/pom.xmlindentation of the<csharp>dependency was changed without reason (whitespace-only diff mixed with functional change)JavaScriptAggregator.reset()reinitializesJavaScriptLanguageSupport— unlike other aggregators — which may have unintended side effects during tests@SuppressWarnings("java:S3776")onJavaScriptDetectionEngineshould prompt refactoring rather than suppression- Detection test methods use positional
findingId— fragile to rule/traversal ordering changes
Overall Assessment
The PR establishes a solid structural foundation and successfully integrates JavaScript/TypeScript scanning into the plugin pipeline. The happy-path detection for createHash, createHmac, createCipheriv, KDF APIs, and TLS works end-to-end. However, there are 6 blockers that must be resolved before this is production-ready — most critically the unconditional debug logging of cryptographic data, the wrong rule key being reported, and the Node subprocess having no timeout. The regex-scanning approach in eslint-runner.js is an inherent limitation that should be clearly documented as a known gap (vs. a proper JS parser), and the false-positive risk from generic method names needs to be addressed.
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
- M1: remove unused fs/path requires from eslint-runner.js - M2: remove unused LOGGER field + imports from JavaScriptLanguageTranslation - M3: drop redundant Collections.unmodifiableList wrap (stream().toList() already returns unmodifiable); remove unused Collections import - M4: restore csharp dependency indentation to match main Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
- H1: re-enable generateKeyPair / generateKeyPairSync - H3: wire variableValues from runner; drop dead resolveConstant fallback - H4: save/restore currentContext around BlockTree traversal - H5: createSecretKey reports kind=import - H6: report issues at (line, column) - H7: replace regex scanner with ESLint/espree AST parser - H8: block path traversal in JAR extraction; register files for deleteOnExit - pom: restore npm install for eslint dep - plugin pom: keep minimizeJar=true with comment Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
Signed-off-by: Siddhartha bhat <bhattaraiankit2212@gmail.com>
|
Hi @san-zrl pushed all the fixes from the evaluation that you had gave me that are 6 blockers, H1–H8, and 7 minor items. The regex scanner is now a real ESLint/espree AST parser mvn clean package + mvn test are green locally (22 tests). The CI workflow appears to be awaiting approval so could you approve it so CI runs on the latest commits? Two design notes from your original evaluation that are now resolved: Ready for re-review whenever you have a moment. |
|
Hi @Siddhartha20242 - CI build is green now. I'm currently on vacation; will do another review when I return next week. |
There was a problem hiding this comment.
Hi @Siddhartha20242 - Your progress is good. Here's the promised review of the recent changes. Please have a look at the newly introduced issues and fix them. Biggest problem in my opinion is N2.
PR-539 Review (Updated)
All items from the previous review round are assessed, followed by new findings discovered in the updated code.
Previous Review Items: Resolution Status
Blockers
B1 — Unconditional Utils.printNodeTree debug logging ✅ Resolved
Removed entirely from JavaScriptTranslationProcess. The redundant Collections.unmodifiableList double-wrap was cleaned up at the same time.
B2 — createCipher/createDecipher absent from runner's method set ✅ Resolved
Both names are now in CRYPTO_MODULE_METHODS. FACTORY_RESULT_TYPES already had them; both maps are now consistent.
B3 — All issues reported under the wrong rule key ✅ Resolved
SensorIssueReporter.report() reads the @Rule(key=...) annotation from the concrete check class and falls back to INVENTORY_RULE_KEY only when none is present. The IssueReporter interface signature was updated to carry currentRule through.
B4 — No process timeout on Node subprocess ✅ Resolved
ESLintExecutor sets new ExecuteWatchdog(60_000L), documented with a comment.
B5 — .gitignore conflicted with tracked eslint-runner.js ✅ Resolved
Only the build-time-downloaded subdirectories (node/node/, node/node_modules/) are now excluded. Source files (eslint-runner.js, package.json, package-lock.json) are correctly tracked.
B6 — minimizeJar: false unexplained ✅ Resolved
minimizeJar is restored to true. The explanatory comment was added then removed by a cleanup commit, so there is again no explanation in the file. A one-line comment is still warranted.
High-Priority Items
H1 — generateKeyPair/generateKeyPairSync disabled ✅ Resolved
Both rules are back in NodeCryptoKeyGen.buildRules(). The test fixture now exercises both calls. Javadoc matches reality.
H2 — False positives from generic method names ✅ Resolved
The runner now uses ESLint/espree AST traversal. shouldEmit() gates every emission on both object type and method name. A dedicated bridge test asserts that logger.update(...) and db.connect(...) are not reported.
H3 — variableValues never populated ✅ Resolved
recordConstant() is invoked from VariableDeclarator and AssignmentExpression visitors. A bridge test asserts blockTree.variableValues().containsEntry("algorithm", "sha256") end-to-end.
H4 — currentContext overwritten and never restored ✅ Resolved
JavaScriptDetectionEngine.run() now saves and restores currentContext in a try/finally block around every BlockTree traversal.
H5 — createSecretKey tagged GENERATION despite being an import
buildForContext() now uses new SecretKeyContext(Map.of("kind", "import")), which is correct. However, the action factory is still new KeyActionFactory<>(KeyAction.Action.GENERATION). The action and the context metadata are contradictory — GENERATION implies a new key was produced, "kind": "import" says it was imported. The context fix is the right half; the action tag should use an import-appropriate value or the discrepancy must be documented as intentional.
H6 — Column discarded; issues reported at line level only ✅ Resolved
selectIssueRange() uses inputFile.newRange(safeLine, startOffset, safeLine, startOffset + 1) with clamping and a selectLine() fallback on IllegalArgumentException.
H7 — Regex scanner replaced with ESLint/espree AST parser ✅ Resolved (core issue)
The runner is now a proper ESLint rule using Linter and espree. String, template, and comment handling is correct. This is the most substantial improvement in the update.
H8 — Path traversal in JAR extraction ✅ Resolved
Both the JAR and filesystem extraction paths normalize the resolved target and throw IOException on escape. Files are individually registered with deleteOnExit().
Minor Cleanup Items (M1–M4)
All resolved: unused fs/path imports removed from the runner, Collections.unmodifiableList double-wrap removed, pom.xml indentation normalised.
New Findings in the Updated Code
These are issues that either did not exist before or were introduced by the fixes above.
🔴 Must Fix
N1 — context.getAncestors() is deprecated in ESLint 8 and removed in ESLint 9
// eslint-runner.js:292
const ancestors = context.getAncestors();This API was deprecated in ESLint 8 and does not exist in ESLint 9. The runner pins eslint@8.57.1 so it works today, but will break silently the moment ESLint is bumped. The non-deprecated replacement is one line:
const ancestors = context.getSourceCode().getAncestors(node);Fix this now to avoid a future broken upgrade.
N2 — TypeScript is listed as supported but espree cannot parse .ts files
CryptoJavaScriptSensor includes "ts" in SUPPORTED_LANGUAGES and JavaScriptRulesDefinition has a "typescript" entry in REPOSITORY_KEYS_BY_LANGUAGE. TypeScript syntax (type annotations, generics, interfaces) is not valid ES2022; espree will produce a fatal parse error and return an empty tree — silently, with no warning to the user. Net effect is that the current implementation does not support typescript at all in contradiction to the PR title.
🟡 Should Fix
N3 — generateKeyPair/generateKeyPairSync use KeyContext instead of typed key contexts
The re-enabled rules in NodeCryptoKeyGen use the base KeyContext() while the import rules use typed PublicKeyContext/PrivateKeyContext. Key-pair generation is the most security-relevant operation in this file — it should produce typed CBOM nodes. If KeyContext is intentional (e.g. because a single generation call produces both key types and the engine cannot split them), this must be documented with a comment.
N4 — createSecretKey action is GENERATION but context says "kind": "import"
As noted under H5: the context fix is correct but the KeyAction.Action.GENERATION tag on CREATE_SECRET_KEY still says the operation generates a key. Importing a key is not generation. This will produce inaccurate CBOM output for createSecretKey callers. Apply the same "kind": "import" rationale to the action.
N5 — Watchdog timeout kill is not distinguishable from other failures in the log
When the watchdog kills the Node process, Commons Exec throws ExecuteException with a non-zero exit value. The catch block logs stderr, which will be empty for a timeout kill. The message "ESLint runner failed" gives no indication that a 60-second timeout was the cause. Add a ExecuteWatchdog.killedProcess() check before logging:
} catch (ExecuteException e) {
if (watchdog.killedProcess()) {
LOG.warn("ESLint runner timed out after {}ms", WATCHDOG_TIMEOUT_MILLIS);
} else {
LOG.warn("ESLint runner failed: {}", stderr.toString(StandardCharsets.UTF_8));
}
throw new IOException("ESLint runner failed with exit code " + e.getExitValue(), e);
}N6 — sourceType: 'module' rejects top-level return statements in CommonJS files
parserOptions: { ecmaVersion: 2022, sourceType: 'module', ... }sourceType: 'module' is used for all files. Top-level return is a valid CommonJS pattern (e.g. in generated code or some bundler outputs) and is a syntax error under ES module semantics. Such files will hit the fatal parse-error path and return an empty tree. Consider retrying with sourceType: 'script' when the first parse returns a fatal error.
🔵 Nit / Low
N7 — Module-level current shared state lacks a thread-safety comment
let current = null; // line 109current is set before each linter.verify() call and captured immediately in the rule's create(). This is correct for the synchronous stdin model. A one-line comment explaining why this is safe would prevent a future reader from flagging it as a race condition or "fixing" it incorrectly.
N8 — NodeCryptoRuleTestBase only checks that detection fired, not what was detected
// NodeCryptoRuleTestBase.asserts()
if (!nodes.isEmpty()) { sawTranslatedNodes = true; }The base class introduced for the new per-family tests only verifies that nodes are emitted — it does not assert algorithm values, context types, or node kinds. NodeCryptoKeyGenTest, NodeCryptoCipherTest, etc. all inherit this and make no additional assertions. This means a rule could fire with the wrong algorithm or wrong context type and the tests would still pass. At minimum, add assertions for the context type (DigestContext, KeyContext, etc.) in the base class or in each subclass.
N9 — minimizeJar: true has no explanatory comment after cleanup commit
The explanatory comment added in f810391b was removed by c5cd9cf6. Future contributors who hit a runtime class-not-found error may turn minimizeJar back to false without understanding why it was true. Restore the comment.
Summary
| # | Severity | Item | Status |
|---|---|---|---|
| B1–B6 | Blocker | All six blockers from round 1 | ✅ Resolved |
| H1–H4, H6–H8 | High | Seven of eight high-priority items | ✅ Resolved |
| H5 | High | createSecretKey action/context contradiction |
|
| N1 | 🔴 Must fix | context.getAncestors() deprecated, breaks on ESLint 9 |
New |
| N2 | 🔴 Must fix | TypeScript listed as supported; silently returns empty results | New |
| N3 | 🟡 Should fix | generateKeyPair rules use KeyContext instead of typed contexts |
New |
| N4 | 🟡 Should fix | createSecretKey GENERATION action contradicts "kind": "import" context |
Residual from H5 |
| N5 | 🟡 Should fix | Watchdog timeout kill logged as generic failure | New |
| N6 | 🟡 Should fix | sourceType: 'module' breaks CJS files with top-level return |
New |
| N7 | 🔵 Nit | current shared state needs a thread-safety comment |
New |
| N8 | 🔵 Nit | Rule test base class asserts detection fired but not what was detected | New |
| N9 | 🔵 Nit | minimizeJar comment removed; rationale is lost again |
New |
Two items (N1, N2) must be fixed before merge. N1 is a one-line code change. N2 requires either removing "ts" from the supported-languages list or adding an explicit warning when TypeScript files fail to parse.
|
Hi @Siddhartha20242, thank you for the work you have put into this PR and for addressing the earlier review comments. After reviewing the approach more closely, I am going to close this PR so we can settle the parser and engine integration before continuing with an implementation. Our other language modules use either an available Sonar language parser (Java, Python, Go) or ANTLR (C#). This PR uses ESLint to select crypto calls, then builds a reduced AST of its own. Although it invokes the shared Could you first evaluate these two options and propose how you would implement JS and TS support?
The proposal should show how the parser feeds our existing This closes the current approach, not the request for JavaScript and TypeScript support. Thanks again. |
|
Hi @n1ckl0sk0rtge , thanks for the detailed feedback :) SonarJs yes it's confirmed which i have mentioned in the meeting too. SonarQube's docs say custom rules aren't supported for JS/TS, and there's no API to hook into its parser or AST externally. The only path is importing ESLint issues, which skips our detection engine entirely. ANTLR which leaves us only ANTLR which is same as the C# module. i have few assumptions going in:
This should fix the #539 issue to ---------- the toBlockTree() problem (an empty block on each call, so dependent rules couldn't traverse from createHash to update/digest) was a symptom of not having a real AST. Feasibility test plan i.e first will be checking the basic versions of the five engine interfaces, one rule for crypto.createHash(), and will be testted against a simple sequential case first, then the inline chained version, to confirm the dependent rule path actually traverses correctly. |
|
Hi @n1ckl0sk0rtge, I posted the evaluation and proposal on #521: #521 (comment) The first follow-up PR is the feasibility test you described: #564 It has one rule (createHash with update and digest) and one typed TypeScript fixture, run through the shared engine to a CBOM digest node. I'd like your view on the open questions in the #521 comment before I expand anything. |
Summary
This PR adds (currently in the working phase) JavaScript and TypeScript support to the Sonar Cryptography plugin. It detects cryptographic API usage in Node.js projects and generates a CBOM.
Build status
mvn clean compilepassesReference: #521