refactor: replace uuid with native Crypto API #11769
Conversation
|
Important Review skippedReview was skipped due to path filters ⛔ Files ignored due to path filters (1)
CodeRabbit blocks several paths by default. You can override this behavior by explicitly including those paths in the path filters. For example, including You can disable this status message by setting the WalkthroughThe PR removes the Changes
Sequence Diagram(s)sequenceDiagram
participant CallSite as Call Site
participant PT as PlatformTools
participant RG as RandomGenerator
participant Crypto as Native Crypto APIs
CallSite->>PT: generateUuid()
PT->>RG: uuidv4()
alt Node.js crypto.randomUUID available
RG->>Crypto: crypto.randomUUID()
Crypto-->>RG: uuid
else globalThis.crypto.randomUUID available
RG->>Crypto: globalThis.crypto.randomUUID()
Crypto-->>RG: uuid
else Fallback: getRandomValues
RG->>Crypto: crypto.getRandomValues(16)
Crypto-->>RG: bytes
RG->>RG: apply RFC4122 version/variant mask & format
end
RG-->>PT: uuid string
PT-->>CallSite: uuid string
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Suggested reviewers
Poem
Pre-merge checks and finishing touches✅ Passed checks (5 passed)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (2)
src/util/StringUtils.ts (1)
1-1: Consider using native crypto for SHA-1 instead of RandomGenerator.The PR objectives specify replacing sha.js with the native crypto API, but
RandomGenerator.sha1()is a pure JavaScript implementation (ported from PHP's locutus library), not a native crypto call. The comment inRandomGenerator.tsitself recommends "using Node's native crypto modules directly in a streaming fashion for faster and more efficient hashing."Consider using the native crypto API for SHA-1:
import crypto from "crypto"Then update the
hashfunction:export function hash(input: string, options: IHashOptions = {}): string { - const hashedInput = RandomGenerator.sha1(input) + const hashedInput = crypto.createHash('sha1').update(input).digest('hex') if (options.length && options.length > 0) { return hashedInput.slice(0, options.length) } return hashedInput }For browser environments, this would need platform-specific handling similar to
PlatformTools.generateUuid(). Do you want me to propose a complete cross-platform solution?Also applies to: 120-120
src/platform/PlatformTools.ts (1)
281-289: Verify type safety; error handling is optional depending on supported browser versions.Node.js version 16.13.0 (minimum in package.json) satisfies the 14.17.0+ requirement for
crypto.randomUUID(). The code is compatible with the project's minimum Node.js version.For the browser path, the type safety concern is valid:
(globalThis as any).crypto.randomUUID()bypasses TypeScript checking. Consider improving type safety by replacing the cast with proper optional chaining:- return (globalThis as any).crypto.randomUUID() + return globalThis.crypto.randomUUID()Error handling for older browsers (lacking crypto.randomUUID support) is optional and depends on your minimum supported browser versions. If you need to support browsers older than Chrome 92, Firefox 95, or Safari 15.4, add defensive checks before the call.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (6)
package.json(0 hunks)src/cache/DbQueryResultCache.ts(2 hunks)src/platform/PlatformTools.ts(2 hunks)src/query-builder/InsertQueryBuilder.ts(2 hunks)src/util/StringUtils.ts(2 hunks)test/github-issues/10569/issue-10569.ts(2 hunks)
💤 Files with no reviewable changes (1)
- package.json
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-27T20:50:10.364Z
Learnt from: alumni
Repo: typeorm/typeorm PR: 11581
File: docs/docs/drivers/postgres.md:23-23
Timestamp: 2025-07-27T20:50:10.364Z
Learning: The correct data source type for Aurora PostgreSQL in TypeORM is `aurora-postgres`, not `aurora-data-api-pg`. The `aurora-data-api-pg` driver was renamed to `aurora-postgres` according to the CHANGELOG.md. This is defined in the DatabaseType union type and AuroraPostgresConnectionOptions interface.
Applied to files:
src/query-builder/InsertQueryBuilder.ts
🧬 Code graph analysis (4)
src/query-builder/InsertQueryBuilder.ts (1)
src/platform/PlatformTools.ts (1)
PlatformTools(18-290)
src/util/StringUtils.ts (1)
src/util/RandomGenerator.ts (1)
RandomGenerator(1-165)
src/cache/DbQueryResultCache.ts (1)
src/platform/PlatformTools.ts (1)
PlatformTools(18-290)
test/github-issues/10569/issue-10569.ts (1)
src/platform/PlatformTools.ts (1)
PlatformTools(18-290)
🔇 Additional comments (4)
src/cache/DbQueryResultCache.ts (1)
8-8: LGTM! Clean migration to native UUID generation.The replacement of
uuidv4()withPlatformTools.generateUuid()is correctly implemented for the Spanner-specific case where auto-generated columns are not supported.Also applies to: 271-271
src/query-builder/InsertQueryBuilder.ts (1)
14-14: LGTM! Proper UUID generation fallback.The replacement correctly uses
PlatformTools.generateUuid()for databases that don't natively support UUID generation. The integration withlocallyGeneratedtracking is maintained.Also applies to: 1410-1410
test/github-issues/10569/issue-10569.ts (1)
6-6: LGTM! Test updated to match new UUID generation approach.The test correctly uses
PlatformTools.generateUuid()consistent with the rest of the codebase.Also applies to: 36-36
src/platform/PlatformTools.ts (1)
2-2: LGTM! Proper crypto import.The crypto import is correctly added to support native UUID generation.
There was a problem hiding this comment.
Can break platforms that don't support crypto. Read the discussion in #11693
Also: the internal sha1 implementation has a flaw (when whitespace chars are in the input) - so this is a breaking change that cannot be merged into master (we use master for the 0.3 version).
|
@alumni |
- Remove sha.js and uuid dependencies - Add PlatformTools.generateUuid() with multi-level fallbacks - Add PlatformTools.sha1() using RandomGenerator for consistency - Update all usages: NamingStrategy, StringUtils, Cache, QueryBuilder - Support React Native, Hermes, and other limited environments - Zero breaking changes: SHA1 output remains identical
mag123c
force-pushed
the
refactor/remove-sha-uuid-deps
branch
from
74bddfd to
1f469bc
Compare
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
src/platform/BrowserPlatformTools.template (1)
143-203: Consider eliminating code duplication.The
sha1()andgenerateUuid()implementations are duplicated betweenPlatformTools.tsandBrowserPlatformTools.template. While some duplication is expected given the platform-specific nature of this file, the UUID generation fallback logic (lines 171-202) is identical.If feasible within your build/bundling constraints, consider extracting the common fallback logic to a shared utility that both platform implementations can reference. This would reduce maintenance burden and ensure consistency. However, if the template replacement mechanism makes this impractical, the current duplication is acceptable.
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
⛔ Files ignored due to path filters (1)
package-lock.jsonis excluded by!**/package-lock.json
📒 Files selected for processing (9)
package.json(0 hunks)src/cache/DbQueryResultCache.ts(2 hunks)src/naming-strategy/DefaultNamingStrategy.ts(9 hunks)src/naming-strategy/LegacyOracleNamingStrategy.ts(2 hunks)src/platform/BrowserPlatformTools.template(1 hunks)src/platform/PlatformTools.ts(3 hunks)src/query-builder/InsertQueryBuilder.ts(2 hunks)src/util/StringUtils.ts(2 hunks)test/github-issues/10569/issue-10569.ts(2 hunks)
💤 Files with no reviewable changes (1)
- package.json
🚧 Files skipped from review as they are similar to previous changes (2)
- src/query-builder/InsertQueryBuilder.ts
- src/util/StringUtils.ts
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2025-07-26T12:22:52.215Z
Learnt from: alumni
Repo: typeorm/typeorm PR: 11580
File: src/util/ImportUtils.ts:58-62
Timestamp: 2025-07-26T12:22:52.215Z
Learning: In TypeORM's codebase, `importOrRequireFile` is always called with resolved file paths. For example, `DirectoryExportedClassesLoader.ts` calls `PlatformTools.pathResolve(file)` before passing the path to `importOrRequireFile`, and `PlatformTools.pathResolve()` is a wrapper around Node.js `path.resolve()`. This means the cache keys in `getNearestPackageJson` will always be absolute paths, eliminating concerns about cache consistency between relative and absolute path formats.
Applied to files:
src/platform/PlatformTools.ts
🧬 Code graph analysis (5)
src/naming-strategy/LegacyOracleNamingStrategy.ts (1)
src/platform/PlatformTools.ts (1)
PlatformTools(18-348)
src/cache/DbQueryResultCache.ts (1)
src/platform/PlatformTools.ts (1)
PlatformTools(18-348)
src/platform/PlatformTools.ts (1)
src/util/RandomGenerator.ts (1)
RandomGenerator(1-165)
src/naming-strategy/DefaultNamingStrategy.ts (1)
src/platform/PlatformTools.ts (1)
PlatformTools(18-348)
test/github-issues/10569/issue-10569.ts (1)
src/platform/PlatformTools.ts (1)
PlatformTools(18-348)
🔇 Additional comments (8)
src/cache/DbQueryResultCache.ts (1)
8-8: LGTM!The migration from the
uuidpackage toPlatformTools.generateUuid()is clean and maintains the same functionality for Spanner cache ID generation.Also applies to: 271-271
src/naming-strategy/LegacyOracleNamingStrategy.ts (1)
2-2: LGTM!The migration to
PlatformTools.sha1()maintains backward compatibility since it delegates to the sameRandomGenerator.sha1()implementation, ensuring database constraint names remain unchanged.Also applies to: 57-57
src/naming-strategy/DefaultNamingStrategy.ts (1)
2-2: LGTM!All constraint name generation methods have been consistently updated to use
PlatformTools.sha1(). Since this delegates to the same underlyingRandomGenerator.sha1()implementation, existing database constraint names will remain unchanged.Also applies to: 63-63, 76-76, 92-92, 102-102, 117-117, 133-133, 144-144, 155-155
test/github-issues/10569/issue-10569.ts (1)
6-6: LGTM!The test has been updated to use
PlatformTools.generateUuid()instead of the externaluuidpackage, maintaining the same test behavior.Also applies to: 36-36
src/platform/PlatformTools.ts (3)
2-2: LGTM!The crypto import addition and formatting cleanup are appropriate for the new functionality.
Also applies to: 134-136
281-290: LGTM!The
sha1()method correctly delegates toRandomGenerator.sha1(), maintaining backward compatibility for constraint names. The lazy import avoids circular dependencies.
292-347: Security concern acknowledged but implementation is already defensive; fallback only reached in unsupported environments.The review correctly identifies that Math.random() is not cryptographically secure (line 324-327). However, verification shows the implementation has three secure fallbacks before reaching it:
- TypeORM requires Node.js >=16.13.0, which includes
crypto.randomUUID()(available since Node 15.7.0)- Browser/modern environments try
globalThis.crypto.randomUUID()- Fallback to
crypto.getRandomValues()with RFC 4122 manual implementation- Only then Math.random() as last resort
The code already acknowledges this in the comment at line 324: "Last resort: Math.random (not cryptographically secure)."
For the cache key usage, UUIDs are used in DbQueryResultCache for cache IDs, where unpredictability is less critical than for security tokens. The Math.random() fallback would only execute in truly unsupported/legacy environments—modern React Native includes crypto polyfills, modern browsers support the crypto API.
Rather than throwing an error (which could break legitimate edge cases), consider adding a warning log when the Math.random() fallback is used, with clear documentation about supported environments.
src/platform/BrowserPlatformTools.template (1)
143-150: LGTM!The
sha1()method correctly delegates toRandomGenerator.sha1()for consistent hashing across platforms.
| /** | ||
| * Generates UUID v4 with fallback for environments without crypto support. | ||
| */ | ||
| static generateUuid(): string { | ||
| try { | ||
| // Try modern browser crypto API | ||
| if ( | ||
| typeof globalThis !== "undefined" && | ||
| globalThis.crypto && | ||
| typeof globalThis.crypto.randomUUID === "function" | ||
| ) { | ||
| return globalThis.crypto.randomUUID() | ||
| } | ||
| } catch (e) { | ||
| // Fall through to polyfill | ||
| } | ||
|
|
||
| // Fallback implementation for older browsers and React Native | ||
| // Based on RFC 4122 version 4 UUID | ||
| const randomBytes = new Uint8Array(16) | ||
|
|
||
| if ( | ||
| typeof globalThis !== "undefined" && | ||
| globalThis.crypto && | ||
| typeof globalThis.crypto.getRandomValues === "function" | ||
| ) { | ||
| globalThis.crypto.getRandomValues(randomBytes) | ||
| } else { | ||
| // Last resort: Math.random (not cryptographically secure) | ||
| for (let i = 0; i < 16; i++) { | ||
| randomBytes[i] = Math.floor(Math.random() * 256) | ||
| } | ||
| } | ||
|
|
||
| // Set version (4) and variant bits according to RFC 4122 | ||
| randomBytes[6] = (randomBytes[6] & 0x0f) | 0x40 | ||
| randomBytes[8] = (randomBytes[8] & 0x3f) | 0x80 | ||
|
|
||
| // Convert to UUID string format | ||
| const hexValues: string[] = [] | ||
| randomBytes.forEach((byte) => { | ||
| hexValues.push(byte.toString(16).padStart(2, "0")) | ||
| }) | ||
|
|
||
| return [ | ||
| hexValues.slice(0, 4).join(""), | ||
| hexValues.slice(4, 6).join(""), | ||
| hexValues.slice(6, 8).join(""), | ||
| hexValues.slice(8, 10).join(""), | ||
| hexValues.slice(10, 16).join(""), | ||
| ].join("-") | ||
| } |
There was a problem hiding this comment.
Same security concern: Math.random() fallback is not cryptographically secure.
This implementation has the same security issue as PlatformTools.ts: the Math.random() fallback (lines 181-183) produces weak, predictable UUIDs when crypto APIs are unavailable.
Apply the same fix recommended for PlatformTools.ts:
} else {
- // Last resort: Math.random (not cryptographically secure)
- for (let i = 0; i < 16; i++) {
- randomBytes[i] = Math.floor(Math.random() * 256)
- }
+ throw new Error(
+ "Cryptographically secure random number generation is not available. " +
+ "Please ensure your browser supports crypto.getRandomValues()."
+ )
}Modern browsers have supported crypto.getRandomValues() since 2013-2014, so this fallback should rarely (if ever) be needed in practice.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| /** | |
| * Generates UUID v4 with fallback for environments without crypto support. | |
| */ | |
| static generateUuid(): string { | |
| try { | |
| // Try modern browser crypto API | |
| if ( | |
| typeof globalThis !== "undefined" && | |
| globalThis.crypto && | |
| typeof globalThis.crypto.randomUUID === "function" | |
| ) { | |
| return globalThis.crypto.randomUUID() | |
| } | |
| } catch (e) { | |
| // Fall through to polyfill | |
| } | |
| // Fallback implementation for older browsers and React Native | |
| // Based on RFC 4122 version 4 UUID | |
| const randomBytes = new Uint8Array(16) | |
| if ( | |
| typeof globalThis !== "undefined" && | |
| globalThis.crypto && | |
| typeof globalThis.crypto.getRandomValues === "function" | |
| ) { | |
| globalThis.crypto.getRandomValues(randomBytes) | |
| } else { | |
| // Last resort: Math.random (not cryptographically secure) | |
| for (let i = 0; i < 16; i++) { | |
| randomBytes[i] = Math.floor(Math.random() * 256) | |
| } | |
| } | |
| // Set version (4) and variant bits according to RFC 4122 | |
| randomBytes[6] = (randomBytes[6] & 0x0f) | 0x40 | |
| randomBytes[8] = (randomBytes[8] & 0x3f) | 0x80 | |
| // Convert to UUID string format | |
| const hexValues: string[] = [] | |
| randomBytes.forEach((byte) => { | |
| hexValues.push(byte.toString(16).padStart(2, "0")) | |
| }) | |
| return [ | |
| hexValues.slice(0, 4).join(""), | |
| hexValues.slice(4, 6).join(""), | |
| hexValues.slice(6, 8).join(""), | |
| hexValues.slice(8, 10).join(""), | |
| hexValues.slice(10, 16).join(""), | |
| ].join("-") | |
| } | |
| /** | |
| * Generates UUID v4 with fallback for environments without crypto support. | |
| */ | |
| static generateUuid(): string { | |
| try { | |
| // Try modern browser crypto API | |
| if ( | |
| typeof globalThis !== "undefined" && | |
| globalThis.crypto && | |
| typeof globalThis.crypto.randomUUID === "function" | |
| ) { | |
| return globalThis.crypto.randomUUID() | |
| } | |
| } catch (e) { | |
| // Fall through to polyfill | |
| } | |
| // Fallback implementation for older browsers and React Native | |
| // Based on RFC 4122 version 4 UUID | |
| const randomBytes = new Uint8Array(16) | |
| if ( | |
| typeof globalThis !== "undefined" && | |
| globalThis.crypto && | |
| typeof globalThis.crypto.getRandomValues === "function" | |
| ) { | |
| globalThis.crypto.getRandomValues(randomBytes) | |
| } else { | |
| throw new Error( | |
| "Cryptographically secure random number generation is not available. " + | |
| "Please ensure your browser supports crypto.getRandomValues()." | |
| ) | |
| } | |
| // Set version (4) and variant bits according to RFC 4122 | |
| randomBytes[6] = (randomBytes[6] & 0x0f) | 0x40 | |
| randomBytes[8] = (randomBytes[8] & 0x3f) | 0x80 | |
| // Convert to UUID string format | |
| const hexValues: string[] = [] | |
| randomBytes.forEach((byte) => { | |
| hexValues.push(byte.toString(16).padStart(2, "0")) | |
| }) | |
| return [ | |
| hexValues.slice(0, 4).join(""), | |
| hexValues.slice(4, 6).join(""), | |
| hexValues.slice(6, 8).join(""), | |
| hexValues.slice(8, 10).join(""), | |
| hexValues.slice(10, 16).join(""), | |
| ].join("-") | |
| } |
|
@alumni Changes MadePlatform Compatibility
Updated both Node and Browser PlatformTools implementations SHA-1 Breaking Change
Please let me know if this direction is acceptable or if you'd prefer a different approach. |
|
So first of all, if there's any breaking change, we would like to do this in the next major version of TypeORM, which is developed in the The Crypto WebAPI ( We could add a For SHA1, we can't use native crypto since it's async, so we can use our own implementation. Our implementation has a bug with certain characters (missing I think the UUIDv4 change is not breaking, but the SHA1 will be. |
Address collaborator feedback by scoping this PR to UUID replacement only. Changes: - Add RandomGenerator.uuidv4() with native crypto.randomUUID() and RFC 4122 fallback - Remove uuid dependency from package.json - Remove Math.random() fallback as it violates RFC 4122 compliance - Throw error if crypto.getRandomValues() is unavailable - Revert all SHA1-related changes (breaking change - will be addressed separately) - Restore sha.js dependency Breaking change note: SHA1 implementation changes are breaking and will be addressed in a separate PR targeting the ext branch for the next major version.
mag123c
force-pushed
the
refactor/remove-sha-uuid-deps
branch
from
d525cb4 to
76df701
Compare
|
@alumni Changes MadeUUID Implementation
SHA1 Scope Reduction
Should I update the PR title to reflect the reduced scope (e.g., "refactor: replace uuid with native Crypto API")? Or keep the current title and clarify in comments? And, May I proceed with creating a PR targeting the Looking forward to your feedback! |
|
@mag123c a few more things: the lockfile needs updating after Looking at the source code of |
mag123c
force-pushed
the
refactor/remove-sha-uuid-deps
branch
from
c355083 to
c0ad3aa
Compare
mag123c
changed the title
|
@alumni Thank you for the detailed feedback on React Native/Hermes support! Updates Made
Before implementing, I conducted research to ensure this is the optimal approach 1. Hermes crypto support
2. RFC 4122 compliance
3. Others
|
|
Fixed missing The entry was accidentally removed during a previous lock update when npm pruned the optional package. Manually restored from master's lock file. |
|
@mag123c this should be done against |
|
@pkuczynski I think here it's a problem of how we created the tasks, we should probably create 3 of them (shajs, uuid, and buffer), not just one :P However, if |
3 participants
Description of change
This change by removing unnecessary external cryptographic dependencies (sha.js and uuid) and replacing them with native Crypto API functionality available in modern Node.js and browser environments.
Pull-Request Checklist
masterbranchSummary by CodeRabbit
Chores
Refactor
✏️ Tip: You can customize this high-level summary in your review settings.