[jrepucci-rh]

Description

Claude flagged a gap in how the RSA algorithm rules catch double variable assignments. This PR fixes that.

AI Generated by: Claude (using Opus 5)

Type of change

• New rule(s)
• New / updated test fixtures
• Bug fix (false positive / false negative)
• Test framework or tooling
• Documentation

Checklist

• New / changed rules have matching test fixtures
• make test passes locally
• Rule files validate with opengrep --validate
• Commits follow Conventional Commits
• No private or internal data is included in this change

Notes for reviewers

Header comments in these files were written by AI and likely inaccurate. View them as notes for the reviewer that may point to blind spots in the patterns. Take them with a grain of salt.

[jrepucci-rh]

Currently Claude is saying that some of the checks for variable chaining can cause false positives but the examples it gives are decently odd edge cases so I think they're fine to ignore:

semgrep-rules/javascript/crypto/algorithm/ecdh/rules.yaml wraps the receiver of the sink in the deep-expression operator — <... $ECDH ...>.computeSecret(...) — and leaves the aliasing
  step unconstrained — $ECDH2 = <... $ECDH ...>. Together those mean "any later expression that merely mentions an ECDH instance, then .computeSecret() on anything mentioning that".
  crypto.createDiffieHellman() exposes the same method name, so any file that holds both objects in one expression gets the DH agreement labelled algorithmFamily: ECDH.

  Confirmed by scanning (--json --no-git-ignore --taint-intrafile) — all of these fire ecdh.compute-secret on a call that is not ECDH:

  const ecdh = crypto.createECDH('prime256v1');
  const ctx = { ec: ecdh, ff: crypto.createDiffieHellman(2048) };
  ctx.ff.computeSecret(pub);                        // reported as ECDH

  const peers = [ecdh, crypto.createDiffieHellman(2048)];
  peers[1].computeSecret(pub);                      // reported as ECDH

  [ecdh, crypto.createDiffieHellman(2048)][1].computeSecret(pub);   // reported as ECDH

  const dh = crypto.createDiffieHellman(ecdh.getPrivateKey().length * 8);
  dh.computeSecret(pub);                            // reported as ECDH

[ddelabru]

Currently Claude is saying that some of the checks for variable chaining can cause false positives

I read a similar Claude review and think I understand the nature of the potential false positive. After checking the deep expression operator in the Semgrep rule pattern documentation, I see how this could generate false positives but I think in this context the risk is acceptable. However, the same review also pointed out a lack of new tests to cover these additional patterns, which is more of a concern to me. The tests can be another way for us to evaluate the soundness of the generated code.

[isasmendiagus]

Agree with Dominic on the tests.

On the false positives: I tried an alternative that keeps the chained case and removes them. Instead of $CTX2 = <... $CTX ...>, use two patterns: $CTX2 = $CTX and $CTX2 = $CTX.$M(...). Tested with opengrep: your four ECDH cases stop reporting, the existing fixtures pass, and this RSA one goes away too:

const signer = crypto.createSign('RSA-SHA256');
const ctx = { signer, hasher: crypto.createHash('sha256') };
ctx.hasher.update(d);   // reported as RSA signing today

Could you apply that here and in #8, and add the alias case plus this negative to the fixtures?

[jrepucci-rh]

Addressed these comments and added some new tests. How do they look?

[isasmendiagus]

LGTM, thanks for the extra fixtures.