Skip to content

Conversation

nievasdev
Copy link

Summary

Add codemod to transform deprecated RSA-PSS crypto options:

  • hashhashAlgorithm
  • mgf1Hashmgf1HashAlgorithm

Changes

  • Only transforms crypto.generateKeyPair() and crypto.generateKeyPairSync() calls with 'rsa-pss' key type
  • Supports both direct calls (crypto.generateKeyPair) and destructured imports (generateKeyPair)
  • Comprehensive test coverage for various usage patterns

Test plan

  • Tests pass for basic transformations
  • Tests pass for destructured imports
  • Tests verify non-rsa-pss calls are ignored
  • Lint and type checks pass

Resolves #144

Add codemod to transform deprecated RSA-PSS crypto options:
- hash -> hashAlgorithm
- mgf1Hash -> mgf1HashAlgorithm

Only transforms crypto.generateKeyPair() and crypto.generateKeyPairSync()
calls with 'rsa-pss' key type. Includes comprehensive test coverage
for various usage patterns.

Resolves: nodejs#144
Initial code generated with Claude Code assistance.

Verified functionality through detailed review and testing.
Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

good starting ! But there are missing piece:

  • if user do import * as foo from 'node:crypto'
  • if use do const { generateKeyPair: foo } = require('node:crypto')
    Solution: have this pr merged #151 EDIT it's merged you should use this utility

Also for strictness on parameters analyst you should do something like that:

for (const call of callNode) {
	const optionsMatch = call.getMatch("OPTIONS");
	if (!optionsMatch) continue;

	// Find the object node that contains the URL options
	const objectNode = optionsMatch.find({
		rule: {
			kind: "object"
		}
	});

	if (!objectNode) continue;

	// Find all property pairs in the object
	const pairs = objectNode.findAll({
		rule: {
			kind: "pair"
		}
	});

	// find hash pair then replace it
	pairs.find(pair => {
		const keyNode = pair.find({
				rule: {
					kind: "property_identifier"
				}
			});
		if (!keyNode) return false;

		const key = keyNode.text();
		if (key === "hash") {
			const valueNode = pair.find({
				rule: {
					kind: "string_fragment"
				}
			}) || pair.find({
				rule: {
					kind: "string"
				}
			});

			if (!valueNode) return false;

			const value = valueNode.text().slice(1, -1); // Remove quotes
			const replacement = `hashAlgorithm: '${value}'`;
			edits.push(call.replace(replacement));
			return true;
		}
		return false;
	});
}

  - Add support for namespace imports (import * as crypto)
  - Add support for destructured renamed imports ({generateKeyPair: foo})
  - Implement stricter parameter analysis for hash/mgf1Hash detection
  - Add comprehensive test cases for new import patterns

  Addresses feedback from nodejs#164
@nievasdev
Copy link
Author

Hi @AugustinMauroy, I updated the branch, I would appreciate any feedback, thank you for your attention.

Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit

…dingPath utility

  Replace manual namespace import parsing with resolveBindingPath utility for
  cleaner code. This change only affects namespace imports (import * as crypto)
  while maintaining all existing functionality for other import patterns.

  Changes:
  - Import resolveBindingPath from @nodejs/codemod-utils
  - Simplify namespace import handling in getCryptoBindings()
  - Fix TypeScript types for SgRoot<JS> and SgNode<JS> compatibility
  - Maintain backward compatibility and all test coverage
@AugustinMauroy
Copy link
Member

this case should be handled

const algo =  'sha256';

crypto.generateKeyPairSync('rsa-pss', {
  modulusLength: 2048,
  mgf1Hash: algo
});

  Fix transformation logic to properly handle both string literals and variable
  references in hash/mgf1Hash properties. Previously only property names were
  replaced, now the entire property is transformed while preserving the value.

  Changes:
  - Replace entire property pair instead of just the key name
  - Support both string literals and identifier variables as values
  - Add test cases for variable value scenarios (variable-value.js)
  - Ensure backward compatibility with existing string literal cases

  Fixes case where mgf1Hash: algo was not being transformed to mgf1HashAlgorithm: algo
@nievasdev
Copy link
Author

@AugustinMauroy @brunocroh thanks for you feedback, I did the changes witch you ask for,
Sorry for the delay, I had to be away for a while.

nievasdev and others added 3 commits August 13, 2025 13:17
… utility

  Replace manual import pattern parsing (~107 lines) with resolveBindingPath calls (~22 lines).
  This approach handles all import patterns (namespace, named, aliased, require) uniformly
  and improves maintainability as new patterns are automatically supported by the utility.
…suite

  - Add template_string support in workflow.ts for template literals
  - Add test cases for template literals, spread operators, nested objects
  - Consolidate and optimize test suite
  - Remove redundant tests and improve coverage
@AugustinMauroy AugustinMauroy marked this pull request as ready for review August 14, 2025 07:24
subdivision of the main code into another auxiliary function to facilitate its reading
- Merge import-aliases.js functionality into destructured.js

- Add namespace imports, function aliases, and variable assignments

- Maintain comprehensive coverage of all import patterns

- Consolidate ES6 imports, CommonJS requires, and alias patterns
- Remove duplicate test case consolidated into destructured.js

- Eliminate redundancy while maintaining test coverage

- Optimize test suite from 12 to 11 unique cases
- ast-node-types.js: Test complex AST value types (member_expression, call_expression, etc.)

- dynamic-options.js: Test unsupported dynamic options pattern

- variable-type.js: Test unsupported variable key type pattern
- Remove "@types/node": "^24.0.3" from package.json

- Fix an example in the readme
  - Add resolveVariableValue() function to detect variables containing 'rsa-pss'
  - Update type detection logic to handle both string literals and variables
  - Transform variable-type.js test case (now supported)
  - Translate Spanish comments to English in ast-node-types.js
  - Add getPromisifiedBindings() function to detect util.promisify() patterns
  - Extend getCryptoBindings() to include promisified crypto function wrappers
  - Add promisified-wrappers.js test case covering 6 different scenarios
  - Update workflow documentation to reflect new capabilities
  - Support patterns like: const genAsync = util.promisify(crypto.generateKeyPair)
  - Use resolveBindingPath for dynamic promisify binding detection
  - Support destructured imports: const {promisify} = require('util')
  - Add test cases for destructured/aliased promisify patterns
  - Fix variable naming conflict and improve fallback logic
  - Fix TypeScript error: conditional_expression → ternary_expression
Copy link
Member

@brunocroh brunocroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice work, thanks! 🚀

nievasdev and others added 2 commits August 16, 2025 17:09
  - Add support for this.property patterns in classes and objects
  - Refactor workflow.ts into modular architecture with separate files
  - Add processThisPropertyReferences() for class/object property handling
  - Create comprehensive binding resolution and validation utilities
  - Add new test case for this.property transformations
  - Clean up redundant comments and improve code documentation
  - Update README with new supported patterns and capabilities
@nievasdev
Copy link
Author

@brunocroh @AugustinMauroy. The code was getting big, so I refactored to a modular architecture.

@brunocroh
Copy link
Member

@brunocroh @AugustinMauroy. The code was getting big, so I refactored to a modular architecture.

IMHO: The previous code probably had some areas that could be improved, but it was easier to follow.

TLDR: workflow.ts was split, but I don’t see much benefit. Splitting makes sense only when a file has multiple responsibilities or needs independent changes (like PR #172). Adding extra files here hides logic and makes it harder to follow. A consistent structure across codemods would help, but one-off architecture/splits just add complexity.

I can see that workflow.ts was split, but I don’t see any real advantage to this. Splits like this make sense when a file has issues with Separation of Concerns or when we’re trying to follow the Single Responsibility Principle.

In our workflow, we have a script that rewrites the file. I believe splitting the file only makes sense in cases where different parts need to change independently, like what PR #172 shows. In those scenarios, having a shared file for reusable parts can make sense.

In many other cases, adding extra files and utility functions doesn’t simplify the code. Instead, it hides what’s happening, making the logic harder to follow when it’s split across 3 or 4 files.

Maybe in the future, we can create a structure or architecture to apply consistently across all codemod recipes, making them easier to follow. However, the real advantage of such structures comes when they form a pattern used across the project. Having just one codemod recipe with a different and more complicated pattern will only make it harder to understand, as it will be completely different from the rest.


Again, just my opinion. Let’s see what the maintainers think.

@AugustinMauroy
Copy link
Member

Again, just my opinion. Let’s see what the maintainers think.

Agreed with Bruno. But @nievasdev I understand that you spend a lot of time on your PR and that it takes a lot of energy. And I'm very happy to have people who are so invested in the project.

@nievasdev
Copy link
Author

nievasdev commented Aug 17, 2025

@brunocroh you got a point, I think you're right, I'll make the code structure like the previous ones,
@AugustinMauroy you spend a lot of time too,
I'm not going to abandon the project. I understand that these first issues are for learning, so don't worry about me getting discouraged, haha 😄
And thank you very much for taking the time to review my commits.

…e workflow

  - Merge 5 separate files (constants, helpers, transformations, etc.) into workflow.ts
Copy link
Member

@AugustinMauroy AugustinMauroy left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

for me it's an LGTM

dependabot bot and others added 5 commits August 19, 2025 15:28
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
Signed-off-by: dependabot[bot] <[email protected]>
Co-authored-by: dependabot[bot] <49699333+dependabot[bot]@users.noreply.github.com>
…d edge cases

  - Remove safeGet* wrapper functions
  - Consolidate destructured.js test (merge aliases/mixed imports)
  - Delete redundant tests: ast-node-types, variable-type
  - Add 5 new edge case tests: complex-ast-patterns, computed-properties, method-chaining, ternary-operators, variable-key-type
  - Update basic.js transformations
Copy link
Member

@brunocroh brunocroh left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM 🚀🎉

@AugustinMauroy AugustinMauroy requested a review from a team August 21, 2025 10:02
nievasdev and others added 5 commits August 21, 2025 16:56
  Use proper type assertions for SgRoot<javascriptTypes> and SgRoot<TypesMap>
  compatibility in utility functions getModuleStatements and resolveBindings.
- Remove 'as any' casts in crypto-rsa-pss-update workflow
- Update correct-ts-specifiers test snapshot to match current behavior
…tibility

  - Restore package-lock.json from main
@nievasdev
Copy link
Author

@brunocroh @AugustinMauroy, how are you? Are there other things we should change? How should I continue with this work?

@brunocroh
Copy link
Member

@brunocroh @AugustinMauroy, how are you? Are there other things we should change? How should I continue with this work?

We need one more member to review/approve before proceeding with the merge.
We are currently waiting for feedback from the team.

@nievasdev
Copy link
Author

Good, could I take another issue while I wait, or is it better if I just wait?

@AugustinMauroy
Copy link
Member

Good, could I take another issue while I wait, or is it better if I just wait?

yeah if you want you can open an second pr

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Feat: handle RSA-PSS generate key pair options
3 participants