Pure js crypto #6
Conversation
WalkthroughThis update introduces a major refactor and modernization of the TLS client library, focusing on modular cryptography backends, explicit TypeScript module resolution, and improved test infrastructure. It adds new pure JavaScript and WebCrypto cryptographic implementations, enables runtime crypto backend selection, updates all imports to use explicit file extensions, and migrates tests from Jest to Node's built-in test framework. Several utility and configuration files are updated or removed, and documentation is improved to reflect these changes and provide usage guidance. Changes
Sequence Diagram(s)sequenceDiagram
participant User
participant TLSClient
participant CryptoBackend (WebCrypto/PureJS)
participant Server
User->>TLSClient: setCryptoImplementation(WebCrypto/PureJS)
User->>TLSClient: startHandshake()
TLSClient->>CryptoBackend: generateKeyPair()
TLSClient->>Server: send ClientHello
Server-->>TLSClient: send ServerHello
TLSClient->>CryptoBackend: importKey(serverPubKey)
TLSClient->>CryptoBackend: derive shared secret
TLSClient->>CryptoBackend: encrypt/decrypt, sign/verify, etc.
TLSClient-->>User: handshakeComplete
User->>TLSClient: sendApplicationData()
TLSClient->>CryptoBackend: encrypt(data)
TLSClient->>Server: send encrypted data
Server-->>TLSClient: send encrypted data
TLSClient->>CryptoBackend: decrypt(data)
TLSClient-->>User: onApplicationData()
Estimated code review effort🎯 5 (Critical) | ⏱️ ~90 minutes Poem
Note ⚡️ Unit Test Generation is now available in beta!Learn more here, or try it out under "Finishing Touches" below. ✨ Finishing Touches
🧪 Generate unit tests
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. 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
SupportNeed help? Create a ticket on our support page for assistance with any issues or questions. CodeRabbit Commands (Invoked using PR comments)
Other keywords and placeholders
CodeRabbit Configuration File (
|
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 6
🔭 Outside diff range comments (2)
src/utils/parse-alert.ts (1)
14-17: Guard against buffers shorter than 2 bytes
DataView#getUint8will throw aRangeErrorwhenbuffer.byteLength < 2.
Add a length check so malformed/empty alerts fail gracefully.- const view = uint8ArrayToDataView(buffer) + if(buffer.byteLength < 2) { + throw new Error(`Alert record too short: expected ≥2 bytes, got ${buffer.byteLength}`) + } + const view = uint8ArrayToDataView(buffer)src/utils/finish-messages.ts (1)
40-42: Fail fast when crypto implementation is missingIf the host app forgets to inject an implementation,
crypto.importKey/crypto.hmacwill beundefined, causing a hard-to-trace runtime error. Add an explicit guard:- const hmacKey = await crypto.importKey(hashAlgorithm, finishKey) - return crypto.hmac(hashAlgorithm, hmacKey, handshakeHash) + if(typeof crypto.importKey !== 'function' || typeof crypto.hmac !== 'function') { + throw new Error('Crypto implementation not initialised. Call setCryptoImplementation() before using TLS helpers.') + } + const hmacKey = await crypto.importKey(hashAlgorithm, finishKey) + return crypto.hmac(hashAlgorithm, hmacKey, handshakeHash)
♻️ Duplicate comments (2)
src/crypto/aes-cbc.ts (1)
5-5: Same.ts-extension issue as noted earlier – the emitted JS will reference a non-JS file and fail at runtime unless a custom loader rewrites it.src/utils/parse-client-hello.ts (1)
1-4: Repeat of the import-extension concernAll four updated specifiers keep the
.tssuffix. Ensure your runtime/bundle strategy handles this, or change them to.js.
🧹 Nitpick comments (6)
.eslintrc.yaml (1)
3-9: YAML formatting issues flagged by lintersYamllint reports:
- Line 5 indentation – expected 2 spaces, found 4
- Missing trailing newline at EOF
Fixing keeps CI green:
parserOptions: - project: ./tsconfig.json + project: ./tsconfig.json ... - '@typescript-eslint/consistent-type-imports': 'error' + '@typescript-eslint/consistent-type-imports': 'error' +src/utils/finish-messages.ts (1)
1-1: Avoid shadowing the globalcryptonamespaceImporting the injectable implementation as plain
cryptocan be confusing, as Node/WebCrypto’s global also exists. Consider aliasing to make call-sites unambiguous:-import { crypto } from '../crypto/index.ts' +import { crypto as tlsCrypto } from '../crypto/index.ts'…and update subsequent usages.
.github/workflows/test.yaml (1)
35-39: Fix YAML-lint violations (trailing spaces & missing final newline)The workflow currently fails
yamllint.- - name: Test (PureJS Crypto) - run: npm run test:purejs-crypto - - - name: Test (WebCrypto) - run: npm run test:webcrypto + - name: Test (PureJS Crypto) + run: npm run test:purejs-crypto + + - name: Test (WebCrypto) + run: npm run test:webcrypto +src/utils/logger.ts (1)
6-6: Expose real trace logging instead of swallowing itA no-op
tracemakes troubleshooting TLS handshakes painful. Prefer wiring it toconsole.traceso stack traces are preserved.- trace: () => {}, + trace: console.trace.bind(console),package.json (1)
59-65: Consider pinning TypeScript to a minor version.While the updates to
@types/node(^22.0.0) andtypescript(^5.0.0) are appropriate, consider using a more specific version constraint for TypeScript (e.g.,^5.6.0) to avoid potential breaking changes between minor versions.src/crypto/index.ts (1)
1-7: Pluggable crypto interface enables flexible backend selection.The refactoring from a concrete implementation to a pluggable interface successfully enables runtime selection between pure JS and WebCrypto backends. The use of
Object.assignto populate the exported object is a common JavaScript pattern for dependency injection.Consider adding runtime safety checks to prevent usage before initialization:
+let isInitialized = false; + export const crypto = {} as Crypto<unknown> export function setCryptoImplementation(impl: Crypto<unknown>) { + if (isInitialized) { + throw new Error('Crypto implementation already set'); + } Object.assign(crypto, impl) + isInitialized = true }This would help catch configuration errors early and prevent accidental re-initialization.
📜 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 (46)
.eslintrc.yaml(1 hunks).github/workflows/test.yaml(2 hunks)jest.config.js(0 hunks)package.json(3 hunks)src/crypto/aes-cbc.ts(1 hunks)src/crypto/common.ts(1 hunks)src/crypto/index.ts(1 hunks)src/crypto/pure.ts(1 hunks)src/crypto/webcrypto.ts(1 hunks)src/index.ts(1 hunks)src/make-tls-client.ts(5 hunks)src/scripts/update-ca-certs.ts(1 hunks)src/tests/crypto.test.ts(12 hunks)src/tests/load-purejs-crypto.ts(1 hunks)src/tests/load-webcrypto.ts(1 hunks)src/tests/message-processor.test.ts(5 hunks)src/tests/mock-tls-server.ts(1 hunks)src/tests/test.tls.ts(0 hunks)src/tests/tls.test.ts(1 hunks)src/tests/utils.ts(2 hunks)src/types/crypto.ts(3 hunks)src/types/index.ts(1 hunks)src/types/tls.ts(1 hunks)src/types/x509.ts(0 hunks)src/utils/client-hello.ts(1 hunks)src/utils/constants.ts(0 hunks)src/utils/decryption-utils.ts(2 hunks)src/utils/finish-messages.ts(1 hunks)src/utils/generics.ts(3 hunks)src/utils/index.ts(1 hunks)src/utils/key-share.ts(1 hunks)src/utils/key-update.ts(1 hunks)src/utils/logger.ts(1 hunks)src/utils/packets.ts(2 hunks)src/utils/parse-alert.ts(1 hunks)src/utils/parse-certificate.ts(2 hunks)src/utils/parse-client-hello.ts(1 hunks)src/utils/parse-extensions.ts(1 hunks)src/utils/parse-server-hello.ts(1 hunks)src/utils/session-ticket.ts(1 hunks)src/utils/webcrypto.ts(1 hunks)src/utils/wrapped-record.ts(1 hunks)src/utils/x509.ts(4 hunks)tsconfig.build.json(1 hunks)tsconfig.json(1 hunks)tsconfig.lint.json(0 hunks)
💤 Files with no reviewable changes (5)
- tsconfig.lint.json
- src/types/x509.ts
- jest.config.js
- src/utils/constants.ts
- src/tests/test.tls.ts
🧰 Additional context used
🧬 Code Graph Analysis (10)
src/utils/parse-certificate.ts (1)
src/crypto/index.ts (1)
crypto(3-3)
src/tests/load-webcrypto.ts (2)
src/crypto/index.ts (1)
setCryptoImplementation(5-7)src/crypto/webcrypto.ts (1)
webcryptoCrypto(34-341)
src/tests/utils.ts (1)
src/utils/logger.ts (1)
logger(3-9)
src/tests/crypto.test.ts (3)
src/utils/generics.ts (1)
toHexStringWithWhitespace(8-12)src/crypto/index.ts (1)
crypto(3-3)src/utils/parse-certificate.ts (1)
verifyCertificateChain(155-227)
src/utils/generics.ts (4)
src/types/crypto.ts (3)
HashAlgorithm(19-19)Key(2-2)Crypto(47-101)src/crypto/index.ts (1)
crypto(3-3)src/crypto/webcrypto.ts (1)
hmac(321-329)src/crypto/pure.ts (1)
hmac(171-173)
src/utils/x509.ts (2)
src/utils/webcrypto.ts (1)
webcrypto(3-3)src/types/x509.ts (1)
X509Certificate(14-32)
src/make-tls-client.ts (2)
src/utils/constants.ts (1)
SUPPORTED_NAMED_CURVES(50-50)src/crypto/index.ts (1)
crypto(3-3)
src/utils/decryption-utils.ts (2)
src/utils/generics.ts (1)
hkdfExpand(132-165)src/crypto/index.ts (1)
crypto(3-3)
src/crypto/webcrypto.ts (5)
src/utils/webcrypto.ts (1)
webcrypto(3-3)src/types/crypto.ts (2)
AsymmetricCryptoAlgorithm(8-10)Crypto(47-101)src/utils/generics.ts (2)
concatenateUint8Arrays(23-35)strToUint8Array(55-57)src/crypto/common.ts (1)
parseRsaPublicKeyFromAsn1(6-13)src/crypto/aes-cbc.ts (1)
decryptAesCbc(7-16)
src/crypto/pure.ts (5)
src/types/crypto.ts (4)
AsymmetricCryptoAlgorithm(8-10)AuthenticatedSymmetricCryptoAlgorithm(4-6)HashAlgorithm(19-19)Crypto(47-101)src/utils/constants.ts (1)
AUTH_TAG_BYTE_LENGTH(48-48)src/crypto/common.ts (2)
parseRsaPublicKeyFromAsn1(6-13)bufToUint8Array(15-21)src/utils/generics.ts (2)
concatenateUint8Arrays(23-35)strToUint8Array(55-57)src/crypto/webcrypto.ts (2)
hmac(321-329)extract(332-340)
🪛 YAMLlint (1.37.1)
.eslintrc.yaml
[warning] 5-5: wrong indentation: expected 2 but found 4
(indentation)
[error] 9-9: no new line character at the end of file
(new-line-at-end-of-file)
.github/workflows/test.yaml
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (65)
tsconfig.build.json (1)
1-7: Build-only tsconfig looks soundThe file cleanly extends the root config and prunes
node_modulesand test sources from the production build. No functional or structural issues spotted.src/scripts/update-ca-certs.ts (1)
4-4:.tsimports are already rewritten to.jsby your compiler settings
Your tsconfig.json sets"rewriteRelativeImportExtensions": trueunder"compilerOptions", soimport '../utils/index.ts'will be emitted asimport '../utils/index.js'. Combined with"type": "module"in package.json, Node will load the generated.jsfiles without error. No change to this import is needed.Likely an incorrect or invalid review comment.
src/utils/parse-alert.ts (1)
1-2: Imports extension update looks goodExplicit
.tsextensions align with the newnodenextmodule-resolution strategy.src/utils/key-update.ts (1)
1-3: LGTM – import path normalisation onlyThe change is purely syntactic and matches the repo-wide convention.
.github/workflows/test.yaml (1)
21-21: Confirm Node 24 availability in CI runnersGitHub Actions images ship the latest stable & LTS versions, but Node 24 is currently in early release status. Ensure it’s present on
ubuntu-latestor pin to the latest LTS (20) to avoid sudden build failures.src/types/tls.ts (1)
1-4: LGTM – import extensions onlyChange aligns with repo convention; no further action needed.
src/utils/logger.ts (1)
1-1: Explicit type-only import looks goodSwitching to
import typeremoves the runtime dependency and is consistent with the new TS config.src/types/index.ts (1)
1-4: Barrel re-exports updated correctlyExplicit
.tsextensions align with the newnodenextresolution strategy; no issues spotted.src/utils/client-hello.ts (1)
1-2: Verify runtime viability of.tsextension in value import
import { crypto } from '../crypto/index.ts'relies on Node resolving.tsat runtime. When compiled to.js, the emitted import will still reference.tsunless a build step rewrites extensions. Make sure your bundler/loader (e.g. tsx, esbuild) rewrites or supports this, otherwise the module will fail to load in plain Node.src/tests/mock-tls-server.ts (1)
2-2: Type-only import is cleanNice use of
typequalifier; keeps the runtime bundle lighter.src/utils/parse-extensions.ts (1)
1-4: Imports modernised correctlyPath extension and
import typeusage match project-wide convention; looks good.src/utils/parse-server-hello.ts (1)
1-5: LGTM! Import path standardization aligns with pluggable crypto architecture.The explicit
.tsextensions and updated crypto import path support the new modular cryptographic backend while maintaining existing functionality.src/utils/parse-certificate.ts (2)
1-9: LGTM! Import standardization supports modular crypto architecture.The explicit
.tsextensions, type-only imports, and updated crypto import path align with the project-wide refactoring to support pluggable cryptographic backends.
101-101: LGTM! Code style improvement.Condensing the multi-line
crypto.importKeycall to a single line improves readability without affecting functionality.src/tests/load-webcrypto.ts (1)
1-4: LGTM! Clean test setup for WebCrypto implementation.This test setup file correctly configures the WebCrypto backend using the pluggable crypto architecture. The implementation is minimal and focused on its single responsibility.
src/utils/packets.ts (2)
1-3: LGTM! Import standardization improves module resolution.The explicit
.tsextensions and type-only imports align with the project-wide effort to standardize import paths and improve type safety.
105-105: LGTM! Explicit typing improves code clarity.Adding explicit
Uint8Arraytyping to the buffer variable enhances type safety and makes the code more self-documenting.src/utils/session-ticket.ts (1)
1-6: LGTM! Import updates support modular crypto architecture.The explicit
.tsextensions, type-only imports, and updated crypto import path align with the project-wide standardization while maintaining all existing functionality.src/tests/load-purejs-crypto.ts (1)
1-4: LGTM! Clean test setup implementation.The test setup correctly imports and configures the pure JavaScript crypto implementation. The explicit
.tsfile extensions align with the codebase-wide import path updates.src/utils/index.ts (1)
1-19: Consistent module path standardization.All export statements have been updated to use explicit
.tsfile extensions, improving module resolution consistency across the codebase.src/utils/decryption-utils.ts (1)
1-5: Improved import organization and crypto architecture alignment.The explicit
.tsextensions and type-only imports improve module resolution consistency. The addition ofhkdfExpandimport supports the new pluggable crypto architecture.src/tests/utils.ts (3)
1-2: Successful migration to Node.js native testing.The import changes correctly support the migration from Jest to Node.js built-in test runner and assert module.
8-10: Proper assertion for Uint8Array comparison.Converting Uint8Arrays to regular arrays before using
assert.deepEqualis the correct approach, as the native assert module handles plain arrays more reliably than typed arrays.
20-21: Logger configuration looks appropriate.The named import usage and empty configuration object maintain the same functionality as before while aligning with the updated import style.
src/crypto/common.ts (3)
4-4: Appropriate use of type-only import.Converting to a type-only import is correct since
PublicKeyis only used as a type annotation in the return type.
10-12: Function calls updated correctly.The calls to the renamed
bufToUint8Arrayfunction maintain the same functionality while using the improved naming.
15-21: Improved function naming and reusability.Renaming to
bufToUint8Arrayis more descriptive and exporting the function enables reuse across the crypto modules. The implementation correctly handles bothArrayBufferandUint8Arrayinputs.src/index.ts (1)
1-4: LGTM! Explicit import paths align with nodenext module resolution.The addition of explicit
.tsextensions andindex.tsreferences is correct for the updated TypeScript configuration using "nodenext" module resolution.src/tests/message-processor.test.ts (2)
1-2: LGTM! Correct migration to Node.js built-in test runner.The import statements correctly use Node.js native modules for testing, which aligns with the project's migration away from Jest.
15-15: LGTM! Correct assertion migration.The conversion from
expect(...).toBe(...)toassert.equal(...)is properly implemented and maintains the same test logic.Also applies to: 26-26, 28-28, 43-43, 58-58, 60-60
src/utils/key-share.ts (1)
1-5: LGTM! Import updates align with modular architecture.The explicit
.tsextensions,import typesyntax for type-only imports, and reference to the pluggable crypto interface are all correctly implemented.tsconfig.json (2)
3-5: LGTM! Modernized TypeScript configuration.The update to "nodenext" module system and resolution correctly supports the explicit import path strategy used throughout the codebase.
9-10: Ensure TypeScript version supports the new compiler options
I don’t see a local TypeScript install (npm list typescriptreturned empty). Please confirm your project is using a version of TS that includeserasableSyntaxOnlyandrewriteRelativeImportExtensions(these landed in TS 5.x):• tsconfig.json (lines 9–10):
"erasableSyntaxOnly": true, "rewriteRelativeImportExtensions": true,• package.json:
– Add or upgradetypescriptin devDependencies (e.g."typescript": "^5.1.0").
– Verify the compiler version your CI/editor uses:
bash npx tsc --versionsrc/utils/webcrypto.ts (1)
1-3: LGTM! Simplified webcrypto export supports modular architecture.The removal of runtime environment detection and direct export of Node.js webcrypto is appropriate for the new pluggable crypto design. This simplification makes the code cleaner and more predictable.
src/utils/wrapped-record.ts (1)
1-6: LGTM! Import statements properly updated.The changes correctly implement explicit
.tsextensions and separate type-only imports, aligning with the project-wide migration to improve module resolution and TypeScript type clarity.src/tests/crypto.test.ts (5)
1-6: Import statements correctly migrated to Node.js test framework.The changes properly replace Jest imports with Node.js built-in test modules and maintain explicit
.tsextensions throughout.
28-96: Jest assertions correctly replaced with Node.js assert calls.All
expect().toEqual()calls have been properly converted toassert.equal(). Good catch on adding the missingawaitforcrypto.exportKey(result.clientMacKey)on line 80.
224-224: Correct use ofassert.deepEqualfor array comparison.The conversion from
expect(Array.from(...)).toEqual(...)toassert.deepEqual(Array.from(...), ...)is appropriate for comparing array contents.
373-378: Error assertion properly converted to use predicate function.The Jest
.rejects.toThrowError()pattern has been correctly replaced withassert.rejects()using a predicate function that checks the error message.
423-437: Boolean assertions correctly useassert.ok().The
expect(...).toBe(true)calls have been properly replaced withassert.ok(...)for verifying truthy values.package.json (3)
3-6: Package configuration properly updated for ES modules.The version bump to 0.1.0 appropriately reflects the significant architectural changes, and setting
"type": "module"enables the ES module system required for the explicit.tsextensions used throughout the codebase.
8-11: Test scripts correctly split for dual crypto implementations.The separation into
test:purejs-cryptoandtest:webcryptoscripts properly supports testing both cryptographic backends. The use of--experimental-strip-typesenables direct TypeScript execution in Node.js.
45-47: Noble crypto libraries added for pure JavaScript implementation.The addition of
@noble/ciphers,@noble/curves, and@noble/hashesprovides the cryptographic primitives needed for the pure JavaScript crypto backend implementation.src/utils/generics.ts (2)
1-2: Import updates and explicit return type improve code quality.The type-only imports and explicit
.tsextensions follow the project conventions, and adding the explicitUint8Arrayreturn type toconcatenateUint8Arraysimproves type safety.Also applies to: 23-25
132-165: Well-implemented HKDF expand function.The
hkdfExpandfunction correctly implements the HKDF-Expand algorithm from RFC 5869:
- Proper validation of maximum output length
- Efficient single-buffer implementation minimizing allocations
- Correct iterative HMAC computation with proper counter handling
- Takes
cryptoas a parameter enabling dependency injectionThe implementation follows cryptographic best practices and integrates well with the new modular crypto architecture.
src/make-tls-client.ts (5)
1-20: LGTM! Import refactoring improves module resolution and type safety.The changes to use explicit
.tsextensions and separate type-only imports align with modern TypeScript best practices and improve tree-shaking capabilities.
26-30: Good addition of default parameters.Moving default values to the function signature improves code clarity and ensures
namedCurvesdefaults toSUPPORTED_NAMED_CURVESfrom the constants file.
49-49: Explicit typing enhances type safety.Explicitly typing
sessionIdandhandshakePacketStreamasUint8Arrayimproves code clarity and type safety.Also applies to: 64-64
713-715: Excellent use of nullish coalescing assignment.The simplified
getKeyPairfunction using??=operator is more concise and removes the need for non-null assertions, improving code safety.
772-772: Clean removal of non-null assertion.Removing the non-null assertion on
namedCurvesis appropriate since it now has a default value, eliminating the possibility of it being undefined.src/tests/tls.test.ts (1)
1-271: Comprehensive TLS test suite with excellent coverage.The test suite thoroughly covers TLS functionality for both versions 1.2 and 1.3, including:
- Handshake completion across all supported curves
- ALPN negotiation
- Data exchange with various payload sizes
- Session ticket handling and resumption (TLS 1.3)
- Traffic key updates
The migration from Jest to Node.js built-in test modules is implemented correctly.
src/utils/x509.ts (3)
4-5: Good fix for circular dependency prevention.Importing the
X509Certificatetype from the main types index prevents circular dependencies while maintaining explicit.tsextensions.
29-36: Improved code readability.Splitting the method chain makes the code more readable and easier to debug if needed.
61-63: Clean simplification of DER loading.Since the underlying library handles both DER and PEM formats, this simplification reduces code duplication while maintaining functionality.
src/types/crypto.ts (2)
2-2: Excellent generic type design for pluggable crypto implementations.The refactoring to use generic types allows different crypto backends to use their own key representations (e.g.,
CryptoKeyfor WebCrypto,Uint8Arrayfor pure JS) while maintaining type safety. This is a well-architected solution for the modular crypto system.Also applies to: 23-45, 47-101
91-91: Good addition of required authTag parameter.Making
authTagexplicitly required inauthenticatedDecryptimproves API clarity and prevents potential runtime errors from missing authentication tags.src/crypto/pure.ts (9)
1-21: LGTM! Well-chosen cryptographic libraries.The imports use reputable libraries (@noble ecosystem, micro-rsa-dsa-dh) that are well-regarded for pure JavaScript cryptographic implementations. The type definitions are appropriate for the cipher constructor pattern.
47-63: LGTM! Appropriate key management for pure JS implementation.The key import/export methods correctly handle raw byte arrays. The
generateKeyPairmethod appropriately generates uncompressed public keys for elliptic curves (needed for TLS) and handles different curve types correctly.
65-79: LGTM! Correct shared secret calculation with appropriate curve handling.The method correctly handles the different secret formats between curve types, properly removing the y-coordinate prefix byte for P-256/P-384 curves while preserving the full secret for X25519.
89-95: LGTM! Correct RSA-PKCS1.5 encryption implementation.The method properly handles RSA encryption using the PKCS1.5 standard with appropriate error handling for unsupported cipher suites and consistent use of the shared RSA key parsing function.
96-112: LGTM! Correct AES-128-CBC implementation.Both encryption and decryption methods correctly implement AES-128-CBC with appropriate error handling. The
disablePadding: trueoption is appropriate when padding is handled at a higher level.
113-125: LGTM! Correct authenticated encryption implementation.Both methods properly handle authenticated encryption by correctly separating/combining the authentication tag with the ciphertext. The implementation works correctly with the @noble cipher libraries' expected format.
126-165: LGTM! Comprehensive signature verification implementation.The method correctly handles multiple signature algorithms with appropriate key parsing and verification parameters. The use of
prehash: trueandformat: 'der'for ECDSA, and proper RSA PSS/PKCS1 implementations demonstrates good understanding of the cryptographic standards.
166-182: LGTM! Correct hash, HMAC, and HKDF implementations.All three methods are correctly implemented using the @noble/hashes API. The salt normalization in the
extractmethod properly handles string conversion and provides appropriate zero-fill fallback, consistent with the webcrypto implementation.
184-187: LGTM! Correct ASN.1 public key parsing.The helper function correctly parses ASN.1 public keys using the appropriate parser and consistently uses the shared
bufToUint8Arrayutility.
| 'SHA-384': sha384 | ||
| } | ||
|
|
||
| const AUTH_TAG_BYTE_LENGTH = 16 |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Remove duplicate constant definition.
AUTH_TAG_BYTE_LENGTH is already defined in src/utils/constants.ts at line 47. This duplication could lead to inconsistency if the values diverge.
Import the constant instead:
+import { AUTH_TAG_BYTE_LENGTH } from '../utils/constants.ts'
import { concatenateUint8Arrays, strToUint8Array } from '../utils/generics.ts'
import { bufToUint8Array, parseRsaPublicKeyFromAsn1 } from './common.ts'
-const AUTH_TAG_BYTE_LENGTH = 16📝 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.
| const AUTH_TAG_BYTE_LENGTH = 16 | |
| import { AUTH_TAG_BYTE_LENGTH } from '../utils/constants.ts' | |
| import { concatenateUint8Arrays, strToUint8Array } from '../utils/generics.ts' | |
| import { bufToUint8Array, parseRsaPublicKeyFromAsn1 } from './common.ts' | |
| // ... rest of the module ... | |
| // (The duplicate `const AUTH_TAG_BYTE_LENGTH = 16` has been removed) |
🤖 Prompt for AI Agents
In src/crypto/pure.ts at line 45, remove the duplicate definition of
AUTH_TAG_BYTE_LENGTH and instead import it from src/utils/constants.ts where it
is already defined at line 47. Replace the local constant declaration with an
import statement to ensure consistency and avoid duplication.
src/crypto/pure.ts
Outdated
| randomBytes(length) { | ||
| // not the most secure but will do for now | ||
| const bytes = new Uint8Array(length) | ||
| for(let i = 0; i < length; i++) { | ||
| bytes[i] = Math.floor(Math.random() * 256) % 256 | ||
| } | ||
|
|
||
| return bytes | ||
| }, |
There was a problem hiding this comment.
Critical security vulnerability: Use cryptographically secure random number generator.
Using Math.random() for cryptographic operations is a serious security flaw as it's not cryptographically secure and predictable. This could compromise all generated keys and cryptographic operations.
Replace with a secure random source:
randomBytes(length) {
- // not the most secure but will do for now
- const bytes = new Uint8Array(length)
- for(let i = 0; i < length; i++) {
- bytes[i] = Math.floor(Math.random() * 256) % 256
- }
-
- return bytes
+ if(typeof crypto !== 'undefined' && crypto.getRandomValues) {
+ const bytes = new Uint8Array(length)
+ crypto.getRandomValues(bytes)
+ return bytes
+ }
+
+ // Fallback for Node.js
+ const crypto = require('crypto')
+ return new Uint8Array(crypto.randomBytes(length))
}📝 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.
| randomBytes(length) { | |
| // not the most secure but will do for now | |
| const bytes = new Uint8Array(length) | |
| for(let i = 0; i < length; i++) { | |
| bytes[i] = Math.floor(Math.random() * 256) % 256 | |
| } | |
| return bytes | |
| }, | |
| randomBytes(length) { | |
| if (typeof crypto !== 'undefined' && crypto.getRandomValues) { | |
| const bytes = new Uint8Array(length) | |
| crypto.getRandomValues(bytes) | |
| return bytes | |
| } | |
| // Fallback for Node.js | |
| const crypto = require('crypto') | |
| return new Uint8Array(crypto.randomBytes(length)) | |
| }, |
🤖 Prompt for AI Agents
In src/crypto/pure.ts around lines 80 to 88, the randomBytes function uses
Math.random(), which is not cryptographically secure. Replace the implementation
to use a cryptographically secure random number generator, such as the Web
Crypto API's crypto.getRandomValues() method, to generate the random bytes
securely.
src/crypto/webcrypto.ts
Outdated
| const cipher = new ChaCha20Poly1305(rawKey) | ||
| plaintext = cipher.open(iv, ciphertext, aead)! | ||
| if(!plaintext) { | ||
| throw new Error('Failed to authenticate ChaCha20 ciphertext') | ||
| } |
There was a problem hiding this comment.
Avoid non-null assertion on cipher.open() result.
The non-null assertion on line 270 could lead to runtime errors if decryption fails. Consider proper error handling instead.
const cipher = new ChaCha20Poly1305(rawKey)
-plaintext = cipher.open(iv, ciphertext, aead)!
-if(!plaintext) {
- throw new Error('Failed to authenticate ChaCha20 ciphertext')
-}
+const result = cipher.open(iv, ciphertext, aead)
+if(!result) {
+ throw new Error('Failed to authenticate ChaCha20 ciphertext')
+}
+plaintext = result📝 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.
| const cipher = new ChaCha20Poly1305(rawKey) | |
| plaintext = cipher.open(iv, ciphertext, aead)! | |
| if(!plaintext) { | |
| throw new Error('Failed to authenticate ChaCha20 ciphertext') | |
| } | |
| const cipher = new ChaCha20Poly1305(rawKey) | |
| const result = cipher.open(iv, ciphertext, aead) | |
| if (!result) { | |
| throw new Error('Failed to authenticate ChaCha20 ciphertext') | |
| } | |
| plaintext = result |
🤖 Prompt for AI Agents
In src/crypto/webcrypto.ts around lines 269 to 273, avoid using the non-null
assertion on the result of cipher.open() as it may cause runtime errors if
decryption fails. Instead, assign the result to a variable without the assertion
and add a conditional check to throw an error if the result is null or
undefined, ensuring proper error handling for failed decryption.
| function cleanBigNum(bn: Uint8Array) { | ||
| if(bn.length > 32 && bn[0] === 0) { | ||
| bn = bn.slice(1) | ||
| } else if(bn.length < 32) { | ||
| bn = concatenateUint8Arrays([ | ||
| new Uint8Array(32 - bn.length).fill(0), | ||
| bn | ||
| ]) | ||
| } | ||
|
|
||
| return bn | ||
| } No newline at end of file |
There was a problem hiding this comment.
cleanBigNum assumes 32-byte values for all curves.
The function assumes all signature components should be 32 bytes, but P-384 uses 48-byte values. This could cause issues with ECDSA-SECP384R1-SHA384 signatures.
Consider making the function curve-aware:
-function cleanBigNum(bn: Uint8Array) {
- if(bn.length > 32 && bn[0] === 0) {
+function cleanBigNum(bn: Uint8Array, expectedLength: number) {
+ if(bn.length > expectedLength && bn[0] === 0) {
bn = bn.slice(1)
- } else if(bn.length < 32) {
+ } else if(bn.length < expectedLength) {
bn = concatenateUint8Arrays([
- new Uint8Array(32 - bn.length).fill(0),
+ new Uint8Array(expectedLength - bn.length).fill(0),
bn
])
}
return bn
}Then update the convertASN1toRS function to pass the appropriate length based on the curve.
📝 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.
| function cleanBigNum(bn: Uint8Array) { | |
| if(bn.length > 32 && bn[0] === 0) { | |
| bn = bn.slice(1) | |
| } else if(bn.length < 32) { | |
| bn = concatenateUint8Arrays([ | |
| new Uint8Array(32 - bn.length).fill(0), | |
| bn | |
| ]) | |
| } | |
| return bn | |
| } | |
| function cleanBigNum(bn: Uint8Array, expectedLength: number) { | |
| if (bn.length > expectedLength && bn[0] === 0) { | |
| bn = bn.slice(1) | |
| } else if (bn.length < expectedLength) { | |
| bn = concatenateUint8Arrays([ | |
| new Uint8Array(expectedLength - bn.length).fill(0), | |
| bn | |
| ]) | |
| } | |
| return bn | |
| } |
🤖 Prompt for AI Agents
In src/crypto/webcrypto.ts around lines 355 to 366, the cleanBigNum function
incorrectly assumes all curves use 32-byte values, but P-384 requires 48 bytes.
Modify cleanBigNum to accept a length parameter representing the expected byte
size for the curve, and use this length instead of the hardcoded 32 when slicing
or padding the Uint8Array. Then update the convertASN1toRS function to determine
the correct length based on the curve used and pass it to cleanBigNum
accordingly.
|
|
||
| const key = await crypto.importKey(algorithm, secret) | ||
| return crypto.expand(algorithm, length, key, length, hkdfLabel) | ||
| return hkdfExpand(algorithm, length, key, length, hkdfLabel, crypto) |
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Fix parameter duplication in hkdfExpand call.
The hkdfExpand function is called with the length parameter twice. Based on the function signature in src/utils/generics.ts (lines 131-164), the correct parameters should be: (alg, hashLength, key, expLength, info, crypto).
- return hkdfExpand(algorithm, length, key, length, hkdfLabel, crypto)
+ return hkdfExpand(algorithm, SUPPORTED_CIPHER_SUITE_MAP[cipherSuite]?.hashLength || 32, key, length, hkdfLabel, crypto)However, I need to verify the correct hashLength parameter. Let me check the cipher suite configuration:
🏁 Script executed:
#!/bin/bash
# Description: Find how hashLength is determined for the algorithm parameter
# Expected: Find SUPPORTED_CIPHER_SUITE_MAP structure to understand hashLength mapping
ast-grep --pattern 'SUPPORTED_CIPHER_SUITE_MAP = {
$$$
}'Length of output: 61
🏁 Script executed:
#!/bin/bash
# Description: Locate the definition of SUPPORTED_CIPHER_SUITE_MAP to verify its hashLength entries
rg -R "SUPPORTED_CIPHER_SUITE_MAP" -n srcLength of output: 446
🏁 Script executed:
#!/bin/bash
# Description: Locate the definition of SUPPORTED_CIPHER_SUITE_MAP to inspect its hashLength values
rg "SUPPORTED_CIPHER_SUITE_MAP" -n srcLength of output: 3665
🏁 Script executed:
#!/bin/bash
# Extract lines around the hkdfExpand call in decryption-utils.ts
sed -n '230,300p' src/utils/decryption-utils.tsLength of output: 1850
🏁 Script executed:
#!/bin/bash
# Extract the definition of SUPPORTED_CIPHER_SUITE_MAP from constants.ts
awk '/export const SUPPORTED_CIPHER_SUITE_MAP/,/};/p' src/utils/constants.tsLength of output: 122
🏁 Script executed:
#!/bin/bash
# Print the first 250 lines of constants.ts to capture SUPPORTED_CIPHER_SUITE_MAP
sed -n '1,250p' src/utils/constants.ts
# Print lines around hkdfExpand in generics.ts
sed -n '120,200p' src/utils/generics.tsLength of output: 7628
Ensure hkdfExtractAndExpandLabel uses the real hashLength, not the expand length twice
The HKDF‐Expand helper is being invoked with the same length for both its hash‐output size and desired output size. To conform to the TLS spec (and how hkdfExpand is typed), you should:
- Add a
hashLengthparameter tohkdfExtractAndExpandLabel(e.g. the suite’shashLength). - Pass that
hashLengthintohkdfExpandinstead of theexpLengthtwice. - Update every call site (e.g. in
deriveTrafficKeysForSideand any other consumers) to supply the suite’shashLength.
Suggested diff in src/utils/decryption-utils.ts around the current implementation (lines ~254–260):
-export async function hkdfExtractAndExpandLabel(
- algorithm: HashAlgorithm,
- secret: Uint8Array,
- label: string,
- context: Uint8Array,
- length: number
-) {
+export async function hkdfExtractAndExpandLabel(
+ algorithm: HashAlgorithm,
+ secret: Uint8Array,
+ label: string,
+ context: Uint8Array,
+ expLength: number,
+ hashLength: number
+) {
const tmpLabel = `tls13 ${label}`
// … build hkdfLabel …
const key = await crypto.importKey(algorithm, secret)
- return hkdfExpand(algorithm, length, key, length, hkdfLabel, crypto)
+ return hkdfExpand(algorithm, hashLength, key, expLength, hkdfLabel, crypto)
}And update e.g. deriveTrafficKeysForSide:
- const encKey = await hkdfExtractAndExpandLabel(hashAlgorithm, masterSecret, 'key', new Uint8Array(), keyLength)
+ const { hashLength } = SUPPORTED_CIPHER_SUITE_MAP[cipherSuite]
+ const encKey = await hkdfExtractAndExpandLabel(
+ hashAlgorithm,
+ masterSecret,
+ 'key',
+ new Uint8Array(),
+ keyLength,
+ hashLength
+ )Repeat similar adjustments for the IV derivation call.
🤖 Prompt for AI Agents
In src/utils/decryption-utils.ts at line 260, the function
hkdfExtractAndExpandLabel incorrectly passes the same length value for both the
hash output size and the desired output size to hkdfExpand. To fix this, add a
new parameter hashLength to hkdfExtractAndExpandLabel representing the suite's
hash output size, then pass hashLength as the hash output size argument to
hkdfExpand instead of length. Also, update all call sites of
hkdfExtractAndExpandLabel, such as deriveTrafficKeysForSide and IV derivation
functions, to provide the correct hashLength value from the cryptographic suite.
There was a problem hiding this comment.
Actionable comments posted: 10
♻️ Duplicate comments (2)
src/crypto/webcrypto.ts (2)
220-224: Implement explicit PKCS7 padding instead of slicingThe slice operation on the encrypted output is incorrect because WebCrypto's AES-CBC doesn't automatically add PKCS7 padding. You need to manually pad the plaintext before encryption.
357-368: cleanBigNum incorrectly assumes 32-byte values for all curvesThe function hardcodes 32 bytes but P-384 requires 48-byte signature components. This will cause verification failures for ECDSA-SECP384R1-SHA384.
🧹 Nitpick comments (5)
.github/workflows/test.yaml (1)
37-39: Fix YAML formatting issues.Static analysis detected formatting issues that should be addressed:
Apply this diff to fix the formatting:
- - name: Test (PureJS) + - name: Test (PureJS) run: npm run test:purejs - + - name: Test (WebCrypto) - run: npm run test:webcrypto + run: npm run test:webcrypto +src/crypto/insecure-rand.ts (1)
2-11: Remove unnecessary modulo operation.The function correctly serves as an insecure fallback, but there's a minor inefficiency:
Apply this diff to remove the redundant modulo:
- bytes[i] = Math.floor(Math.random() * 256) % 256 + bytes[i] = Math.floor(Math.random() * 256)
Math.floor(Math.random() * 256)already returns values in the range 0-255, so the modulo operation is unnecessary.src/scripts/handshake.ts (1)
57-64: Improve argument parsing robustness.The function returns an empty string when an argument exists without a value, which could cause issues. Consider returning
undefinedfor missing values.Apply this diff for more consistent behavior:
function readArg(arg: string) { const index = process.argv.indexOf(arg) if(index === -1) { return undefined } - return process.argv[index + 1] || '' + return process.argv[index + 1] }src/scripts/jsc.ts (2)
91-104: Consider improving loop readability.The while loop with assignment in the condition works but could be clearer.
Consider refactoring for better readability:
- let cmd: JscIncomingCmd - while(cmd = readCmd(), cmd.type !== 'exit') { + while(true) { + const cmd = readCmd() + if(cmd.type === 'exit') break + if(cmd.type === 'send') { const data = base64ToBytes(cmd.dataB64) await tls.handleReceivedBytes(data) continue } if(cmd.type === 'send-application-data') { const data = base64ToBytes(cmd.dataB64) await tls.write(data) continue } }
109-126: Consider using TextDecoder for byte-to-string conversion.The manual byte iteration works but standard APIs would be more efficient.
Consider using TextDecoder for better performance:
function bytesToB64(bytes: Uint8Array) { - let binary = '' - for(const byte of bytes) { - binary += String.fromCharCode(byte) - } - - return btoa(binary) + return btoa(new TextDecoder('latin1').decode(bytes)) }
📜 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 (30)
.eslintignore(1 hunks).github/workflows/test.yaml(2 hunks).gitignore(1 hunks)README.md(3 hunks)package.json(2 hunks)src/crypto/aes-cbc.ts(0 hunks)src/crypto/insecure-rand.ts(1 hunks)src/crypto/pure-js.ts(1 hunks)src/crypto/webcrypto.ts(1 hunks)src/make-tls-client.ts(7 hunks)src/scripts/build-jsc.ts(1 hunks)src/scripts/fallbacks/crypto.ts(1 hunks)src/scripts/handshake.ts(1 hunks)src/scripts/jsc.ts(1 hunks)src/tests/jsc.test_mac.ts(1 hunks)src/tests/load-purejs-crypto.ts(1 hunks)src/tests/tls.test.ts(1 hunks)src/tests/utils.ts(2 hunks)src/types/tls.ts(2 hunks)src/utils/additional-root-cas.js(1 hunks)src/utils/client-hello.ts(3 hunks)src/utils/decryption-utils.ts(3 hunks)src/utils/finish-messages.ts(2 hunks)src/utils/generics.ts(4 hunks)src/utils/index.ts(1 hunks)src/utils/logger.ts(1 hunks)src/utils/packets.ts(4 hunks)src/utils/parse-certificate.ts(3 hunks)src/utils/webcrypto.ts(0 hunks)src/utils/x509.ts(3 hunks)
💤 Files with no reviewable changes (2)
- src/crypto/aes-cbc.ts
- src/utils/webcrypto.ts
✅ Files skipped from review due to trivial changes (3)
- .eslintignore
- .gitignore
- src/scripts/fallbacks/crypto.ts
🚧 Files skipped from review as they are similar to previous changes (13)
- src/utils/index.ts
- src/tests/load-purejs-crypto.ts
- src/utils/logger.ts
- src/utils/finish-messages.ts
- src/utils/client-hello.ts
- src/tests/utils.ts
- src/utils/parse-certificate.ts
- src/types/tls.ts
- src/utils/packets.ts
- src/utils/x509.ts
- src/make-tls-client.ts
- package.json
- src/utils/decryption-utils.ts
🧰 Additional context used
🧬 Code Graph Analysis (6)
src/crypto/insecure-rand.ts (1)
src/crypto/webcrypto.ts (1)
randomBytes(209-212)
src/scripts/handshake.ts (2)
src/make-tls-client.ts (1)
makeTLSClient(24-860)src/utils/generics.ts (1)
uint8ArrayToStr(69-71)
src/utils/generics.ts (4)
src/types/crypto.ts (3)
HashAlgorithm(19-19)Key(2-2)Crypto(47-101)src/crypto/index.ts (1)
crypto(3-3)src/crypto/pure-js.ts (1)
hmac(164-166)src/crypto/webcrypto.ts (1)
hmac(323-331)
src/crypto/webcrypto.ts (4)
src/types/crypto.ts (2)
AsymmetricCryptoAlgorithm(8-10)Crypto(47-101)src/utils/constants.ts (1)
AUTH_TAG_BYTE_LENGTH(48-48)src/utils/generics.ts (2)
concatenateUint8Arrays(23-35)asciiToUint8Array(55-67)src/crypto/common.ts (1)
parseRsaPublicKeyFromAsn1(6-13)
src/scripts/jsc.ts (4)
src/crypto/index.ts (1)
setCryptoImplementation(5-7)src/utils/additional-root-cas.js (1)
TLS_ADDITIONAL_ROOT_CA_LIST(3-3)src/make-tls-client.ts (1)
makeTLSClient(24-860)src/utils/generics.ts (1)
concatenateUint8Arrays(23-35)
src/crypto/pure-js.ts (4)
src/types/crypto.ts (4)
AsymmetricCryptoAlgorithm(8-10)AuthenticatedSymmetricCryptoAlgorithm(4-6)HashAlgorithm(19-19)Crypto(47-101)src/crypto/insecure-rand.ts (1)
randomBytes(2-11)src/crypto/common.ts (2)
parseRsaPublicKeyFromAsn1(6-13)bufToUint8Array(15-21)src/utils/generics.ts (2)
concatenateUint8Arrays(23-35)asciiToUint8Array(55-67)
🪛 YAMLlint (1.37.1)
.github/workflows/test.yaml
[error] 37-37: trailing spaces
(trailing-spaces)
[error] 39-39: no new line character at the end of file
(new-line-at-end-of-file)
🔇 Additional comments (15)
src/utils/generics.ts (2)
1-2: LGTM! Explicit module resolution implemented correctly.The use of explicit
.tsextensions and type-only imports aligns perfectly with the PR objectives for modernizing module resolution and improving TypeScript compatibility.
23-25: Good type annotation enhancement.Adding the explicit
Uint8Arrayreturn type improves type safety and code clarity without changing functionality..github/workflows/test.yaml (2)
21-21: Verify Node.js 24 compatibility.The update to Node.js 24 looks good for getting latest features and performance improvements, but please verify compatibility with all dependencies and crypto APIs used in the project.
35-39: Excellent separation of crypto backend testing.Splitting the tests into separate PureJS and WebCrypto steps provides better visibility and aligns perfectly with the new pluggable crypto architecture.
src/utils/additional-root-cas.js (1)
3-10: Excellent modernization of global object handling.The updates improve the code in multiple ways:
globalThisis the modern standard for cross-environment global access- More descriptive variable naming with
TLS_ADDITIONAL_ROOT_CA_LIST- Proper fallback chain for different environments
- Clean use of logical OR assignment operator
src/scripts/build-jsc.ts (2)
3-16: Well-configured esbuild setup for JSC environment.The build configuration is appropriately tailored for JavaScriptCore:
- ESM format and browser platform targeting
- Crypto fallback alias for environments lacking native crypto APIs
- Efficient bundling with tree shaking enabled
- Metafile generation for analysis capabilities
18-22: Useful bundle analysis feature.The optional
--analyzeflag provides valuable insights into bundle composition and is well-implemented using esbuild's built-in analysis capabilities.src/scripts/handshake.ts (1)
1-6: LGTM! Clean imports and logger setup.The imports correctly use explicit
.tsextensions and the logger configuration follows best practices with environment variable control.README.md (2)
7-77: Excellent documentation updates!The crypto implementation section clearly explains the two available options and provides helpful code examples for both webcrypto and pure-js implementations.
167-173: Great addition of testing documentation!The new "Testing Connections" section clearly documents how to use the handshake script for manual TLS testing.
src/tests/tls.test.ts (2)
51-195: Excellent test coverage and structure!The test suite comprehensively covers TLS 1.2 and 1.3 functionality including handshakes, data exchange, ALPN negotiation, and session resumption. Good use of async patterns and proper resource cleanup.
247-273: Well-designed helper function!The
connectTLShelper provides a clean abstraction for establishing TLS connections in tests with proper option handling.src/scripts/jsc.ts (1)
1-41: Good setup for JSC environment!The imports correctly use explicit extensions and the global type declarations properly handle JSC-specific functions. Setting the pure JS crypto implementation is appropriate for this environment.
src/tests/jsc.test_mac.ts (2)
20-107: Well-structured JSC integration tests!The test suite properly validates TLS functionality in the JSC environment with good resource management and clear test cases.
109-170: Clean bridge implementation!The
runJscBridgefunction provides a good abstraction for managing the JSC process and handling bidirectional communication. The silent error handling intryReadCmdappropriately filters non-JSON output.
| import type { AsymmetricCryptoAlgorithm, AuthenticatedSymmetricCryptoAlgorithm, Crypto, HashAlgorithm } from '../types/index.ts' | ||
| import { asciiToUint8Array, concatenateUint8Arrays } from '../utils/generics.ts' | ||
| import { bufToUint8Array, parseRsaPublicKeyFromAsn1 } from './common.ts' | ||
| import { randomBytes } from './insecure-rand.ts' |
There was a problem hiding this comment.
Critical security issue: Using insecure random number generation
The code imports randomBytes from ./insecure-rand.ts which uses Math.random() - this is not cryptographically secure and must not be used for cryptographic operations. This compromises the security of all key generation and random value generation.
Consider using a cryptographically secure random number generator. For pure JS environments, you could use:
What are some cryptographically secure random number generators available in pure JavaScript for environments without crypto.getRandomValues?
Also applies to: 81-81
🤖 Prompt for AI Agents
In src/crypto/pure-js.ts at lines 19 and 81, the code imports and uses
randomBytes from ./insecure-rand.ts, which relies on Math.random() and is not
cryptographically secure. Replace this with a cryptographically secure random
number generator suitable for pure JavaScript environments, such as using the
Web Crypto API's crypto.getRandomValues if available, or a secure polyfill for
environments without it. Update the import and all usages of randomBytes
accordingly to ensure secure random value generation.
| if(alg === 'P-256' || alg === 'P-384') { | ||
| // @ts-expect-error: need uncompressed public key for TLS | ||
| const pubKey = curve.getPublicKey(secretKey, false) | ||
| return { privKey: secretKey, pubKey } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Fix TypeScript error instead of suppressing it
The @ts-expect-error comment indicates a type mismatch when using uncompressed public keys. Consider properly typing this instead of suppressing the error.
You might need to cast or properly type the return value from getPublicKey. Check if the noble curves library has proper type definitions for uncompressed keys, or create a type assertion that's more specific than suppressing all errors.
🤖 Prompt for AI Agents
In src/crypto/pure-js.ts around lines 58 to 61, the use of @ts-expect-error to
suppress a TypeScript error when calling curve.getPublicKey with the
uncompressed flag should be replaced by a proper type assertion or casting.
Investigate the noble curves library type definitions for getPublicKey to find
the correct return type for uncompressed keys, then explicitly cast or type the
pubKey variable accordingly to satisfy TypeScript without suppressing errors.
| const P384_PRIVATE_KEY_DER_PREFIX = new Uint8Array([ | ||
| 0x30, 0x81, 0xb6, 0x02, 0x01, 0x00, 0x30, 0x10, 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02, 0x01, 0x06, 0x05, 0x2b, 0x81, 0x04, 0x00, 0x22, 0x04, 0x81, 0x9e, 0x30, 0x81, 0x9b, 0x02, 0x01, 0x01, 0x04, 0x30 | ||
| ]) | ||
|
|
||
| const P256_PRIVATE_KEY_DER_PREFIX = new Uint8Array([ | ||
| 0x30, 0x81, 0xb6, 0x02, 0x01, 0x00, 0x30, 0x10, 0x06, 0x07, 0x2a, 0x86, 0x48, 0xce, 0x3d, 0x02, 0x01, 0x06, 0x05, 0x2b, 0x81, 0x04, 0x00, 0x22, 0x04, 0x81, 0x9e, 0x30, 0x81, 0x9b, 0x02, 0x01, 0x01, 0x04, 0x30 | ||
| ]) |
There was a problem hiding this comment.
P-256 and P-384 private key DER prefixes are identical
The DER prefixes for P-256 and P-384 private keys are incorrectly identical. These curves have different OIDs that must be reflected in their DER encoding:
- P-256 uses OID 1.2.840.10045.3.1.7
- P-384 uses OID 1.3.132.0.34
Please verify the correct DER prefixes for each curve:
#!/bin/bash
# Generate test keys and examine their DER structure
openssl ecparam -name prime256v1 -genkey -noout -out p256.key
openssl ecparam -name secp384r1 -genkey -noout -out p384.key
# Convert to PKCS8 and examine the DER structure
openssl pkcs8 -topk8 -nocrypt -in p256.key -outform DER | xxd -p | head -n 3
openssl pkcs8 -topk8 -nocrypt -in p384.key -outform DER | xxd -p | head -n 3
# Clean up
rm -f p256.key p384.key🤖 Prompt for AI Agents
In src/crypto/webcrypto.ts around lines 19 to 25, the DER prefixes for P-256 and
P-384 private keys are incorrectly identical, but they must differ to reflect
their distinct OIDs. Update the P-256 and P-384 DER prefix constants to use the
correct OID values: P-256 should use OID 1.2.840.10045.3.1.7 and P-384 should
use OID 1.3.132.0.34. Verify the correct byte sequences by generating keys and
inspecting their DER encoding with OpenSSL as shown, then replace the existing
Uint8Array values accordingly.
| const form = toUint8Array(await subtle.exportKey('pkcs8', key)) | ||
| const algPrefix = key.algorithm.name === 'X25519' | ||
| ? X25519_PRIVATE_KEY_DER_PREFIX | ||
| : P384_PRIVATE_KEY_DER_PREFIX | ||
| return form.slice(algPrefix.length) | ||
| } |
There was a problem hiding this comment.
Incorrect DER prefix used when exporting ECDH private keys
The code always uses P384_PRIVATE_KEY_DER_PREFIX when exporting ECDH private keys, but this is incorrect for P-256 keys. You need to determine which curve the key belongs to and use the appropriate prefix.
-const algPrefix = key.algorithm.name === 'X25519'
- ? X25519_PRIVATE_KEY_DER_PREFIX
- : P384_PRIVATE_KEY_DER_PREFIX
+let algPrefix: Uint8Array
+if (key.algorithm.name === 'X25519') {
+ algPrefix = X25519_PRIVATE_KEY_DER_PREFIX
+} else if (key.algorithm.name === 'ECDH') {
+ // Check the named curve to determine the correct prefix
+ const namedCurve = (key.algorithm as WebCrypto.EcKeyAlgorithm).namedCurve
+ algPrefix = namedCurve === 'P-256' ? P256_PRIVATE_KEY_DER_PREFIX : P384_PRIVATE_KEY_DER_PREFIX
+} else {
+ throw new Error(`Unsupported algorithm: ${key.algorithm.name}`)
+}📝 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.
| const form = toUint8Array(await subtle.exportKey('pkcs8', key)) | |
| const algPrefix = key.algorithm.name === 'X25519' | |
| ? X25519_PRIVATE_KEY_DER_PREFIX | |
| : P384_PRIVATE_KEY_DER_PREFIX | |
| return form.slice(algPrefix.length) | |
| } | |
| const form = toUint8Array(await subtle.exportKey('pkcs8', key)) | |
| let algPrefix: Uint8Array | |
| if (key.algorithm.name === 'X25519') { | |
| algPrefix = X25519_PRIVATE_KEY_DER_PREFIX | |
| } else if (key.algorithm.name === 'ECDH') { | |
| // Determine prefix based on the named curve | |
| const namedCurve = (key.algorithm as WebCrypto.EcKeyAlgorithm).namedCurve | |
| algPrefix = namedCurve === 'P-256' | |
| ? P256_PRIVATE_KEY_DER_PREFIX | |
| : P384_PRIVATE_KEY_DER_PREFIX | |
| } else { | |
| throw new Error(`Unsupported algorithm: ${key.algorithm.name}`) | |
| } | |
| return form.slice(algPrefix.length) | |
| } |
🤖 Prompt for AI Agents
In src/crypto/webcrypto.ts around lines 162 to 167, the code incorrectly uses
P384_PRIVATE_KEY_DER_PREFIX for all ECDH private keys. Update the logic to check
the specific named curve of the key's algorithm (e.g., 'P-256' or 'P-384') and
select the corresponding DER prefix accordingly before slicing the exported key
bytes.
| signature = convertASN1toRS(signature) | ||
| verifyArgs = { name: 'ECDSA', hash: 'SHA-256' } | ||
| break | ||
| case 'ECDSA-SECP384R1-SHA384': | ||
| signature = convertASN1toRS(signature) | ||
| verifyArgs = { name: 'ECDSA', hash: 'SHA-384' } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Make signature conversion curve-aware
The convertASN1toRS function needs to know which curve is being used to properly handle signature component lengths. Consider passing the expected component length based on the algorithm.
-function convertASN1toRS(signatureBytes: Uint8Array) {
+function convertASN1toRS(signatureBytes: Uint8Array, componentLength: number) {
const data = AsnParser.parse(signatureBytes, ECDSASigValue)
- const r = cleanBigNum(new Uint8Array(data.r))
- const s = cleanBigNum(new Uint8Array(data.s))
+ const r = cleanBigNum(new Uint8Array(data.r), componentLength)
+ const s = cleanBigNum(new Uint8Array(data.s), componentLength)
return concatenateUint8Arrays([ r, s ])
}And update the verify method:
case 'ECDSA-SECP256R1-SHA256':
- signature = convertASN1toRS(signature)
+ signature = convertASN1toRS(signature, 32)
verifyArgs = { name: 'ECDSA', hash: 'SHA-256' }
break
case 'ECDSA-SECP384R1-SHA384':
- signature = convertASN1toRS(signature)
+ signature = convertASN1toRS(signature, 48)
verifyArgs = { name: 'ECDSA', hash: 'SHA-384' }
breakAlso applies to: 350-355
🤖 Prompt for AI Agents
In src/crypto/webcrypto.ts around lines 305 to 310 and also 350 to 355, the
convertASN1toRS function is called without specifying the curve, which is
necessary for correctly handling signature component lengths. Modify the calls
to convertASN1toRS to pass the expected component length or curve information
based on the algorithm being used. Additionally, update the verify method
accordingly to utilize this curve-aware signature conversion.
| process.exit(1) | ||
| } | ||
|
|
||
| const port = Number(readArg('--port') || 443) |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add port number validation.
The port parsing doesn't validate if the result is a valid port number (1-65535) or if the conversion succeeded (NaN check).
Apply this diff to add validation:
-const port = Number(readArg('--port') || 443)
+const portArg = readArg('--port') || '443'
+const port = Number(portArg)
+if(isNaN(port) || port < 1 || port > 65535) {
+ console.error('Invalid port number. Must be between 1 and 65535')
+ process.exit(1)
+}📝 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.
| const port = Number(readArg('--port') || 443) | |
| -const port = Number(readArg('--port') || 443) | |
| +const portArg = readArg('--port') || '443' | |
| +const port = Number(portArg) | |
| +if (isNaN(port) || port < 1 || port > 65535) { | |
| + console.error('Invalid port number. Must be between 1 and 65535') | |
| + process.exit(1) | |
| +} |
🤖 Prompt for AI Agents
In src/scripts/handshake.ts at line 14, the port number parsed from the argument
is not validated for correctness. Add validation to check if the parsed port is
a number and falls within the valid range of 1 to 65535. If the port is invalid
or NaN, handle the error appropriately, such as by logging an error message and
exiting the process or setting a default valid port.
| onTlsEnd(error) { | ||
| if(error) { | ||
| reject(error) | ||
| return | ||
| } | ||
|
|
||
| resolve() | ||
| console.error('TLS connect ended: ', error) | ||
| } |
There was a problem hiding this comment.
Fix incorrect error logging on successful TLS end.
The code logs to console.error even when there's no error, which will print "TLS connect ended: undefined" on successful completion.
Apply this diff to fix the logic:
onTlsEnd(error) {
if(error) {
reject(error)
return
}
resolve()
- console.error('TLS connect ended: ', error)
+ console.log('TLS connection ended successfully')
}📝 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.
| onTlsEnd(error) { | |
| if(error) { | |
| reject(error) | |
| return | |
| } | |
| resolve() | |
| console.error('TLS connect ended: ', error) | |
| } | |
| onTlsEnd(error) { | |
| if(error) { | |
| reject(error) | |
| return | |
| } | |
| resolve() | |
| console.log('TLS connection ended successfully') | |
| } |
🤖 Prompt for AI Agents
In src/scripts/handshake.ts around lines 36 to 44, the console.error call logs a
message even when there is no error, causing "TLS connect ended: undefined" to
appear on successful completion. Move the console.error call inside the
if(error) block so it only logs when an error occurs, and remove or replace the
log on successful completion with a more appropriate message if needed.
| TLS_ADDITIONAL_ROOT_CA_LIST.push( | ||
| readFileSync('./cert/public-cert.pem', 'utf-8') | ||
| ) |
There was a problem hiding this comment.
Missing import for TLS_ADDITIONAL_ROOT_CA_LIST.
The variable TLS_ADDITIONAL_ROOT_CA_LIST is used but not imported, which will cause a runtime error.
Add the missing import at the top of the file:
import { asciiToUint8Array, crypto, makeTLSClient, SUPPORTED_NAMED_CURVE_MAP } from '../index.ts'
+import { TLS_ADDITIONAL_ROOT_CA_LIST } from '../utils/additional-root-cas.js'
import type { CipherSuite, TLSClientOptions, TLSPresharedKey, TLSSessionTicket } from '../types/index.ts'📝 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.
| TLS_ADDITIONAL_ROOT_CA_LIST.push( | |
| readFileSync('./cert/public-cert.pem', 'utf-8') | |
| ) | |
| import { asciiToUint8Array, crypto, makeTLSClient, SUPPORTED_NAMED_CURVE_MAP } from '../index.ts' | |
| +import { TLS_ADDITIONAL_ROOT_CA_LIST } from '../utils/additional-root-cas.js' | |
| import type { CipherSuite, TLSClientOptions, TLSPresharedKey, TLSSessionTicket } from '../types/index.ts' |
🤖 Prompt for AI Agents
In src/tests/tls.test.ts around lines 47 to 49, the variable
TLS_ADDITIONAL_ROOT_CA_LIST is used without being imported, causing a runtime
error. Add an import statement for TLS_ADDITIONAL_ROOT_CA_LIST at the top of the
file from its appropriate module to ensure it is defined before use.
| export function asciiToUint8Array(str: string) { | ||
| const bytes: Uint8Array = new Uint8Array(str.length) | ||
| for(let i = 0; i < str.length; i++) { | ||
| const charCode = str.charCodeAt(i) | ||
| if(charCode < 0 || charCode > 255) { | ||
| throw new Error(`Invalid ASCII character at index ${i}: ${str[i]}`) | ||
| } | ||
|
|
||
| bytes[i] = charCode | ||
| } | ||
|
|
||
| return bytes | ||
| } |
There was a problem hiding this comment.
Fix ASCII validation range.
The function name suggests ASCII-only conversion, but the validation allows characters up to 255 (extended ASCII/Latin-1). Standard ASCII is 0-127.
Apply this diff to fix the ASCII validation:
- if(charCode < 0 || charCode > 255) {
+ if(charCode > 127) {
throw new Error(`Invalid ASCII character at index ${i}: ${str[i]}`)
}If you intentionally want to support extended ASCII (0-255), consider renaming the function to latin1ToUint8Array or extendedAsciiToUint8Array for clarity.
🤖 Prompt for AI Agents
In src/utils/generics.ts between lines 55 and 67, the asciiToUint8Array function
currently validates characters up to 255, which includes extended ASCII, not
standard ASCII. To fix this, change the validation range to only allow character
codes from 0 to 127 to enforce standard ASCII. Alternatively, if extended ASCII
support is intended, rename the function to latin1ToUint8Array or
extendedAsciiToUint8Array for clarity.
src/utils/generics.ts
Outdated
There was a problem hiding this comment.
💡 Verification agent
🧩 Analysis chain
Verify buffer management and HKDF algorithm implementation.
The buffer management logic needs verification, particularly:
- Line 168:
hmac.slice(0, t.length - end)could be problematic whenendapproachest.length - Algorithm correctness: For the first iteration (c=1),
start=0andend=0, sot.slice(start, end + infoLength + 1)becomest.slice(0, infoLength + 1), which should be justinfo | 1for T(1).
Please verify this implementation against the HKDF specification (RFC 5869) and test with known test vectors:
🏁 Script executed:
#!/bin/bash
# Verify HKDF expand implementation against RFC 5869 test vectors
ast-grep --pattern 'export const hkdfExpand = async($$$) => {
$$$
}'
# Look for existing HKDF tests
fd -e ts -e js | xargs rg -l "hkdf.*test|test.*hkdf" -i
# Check if there are test vectors or validation
rg "test.*vector|vector.*test" -A 5 -B 5Length of output: 2021
Add HKDF-Expand Unit Tests with RFC 5869 Vectors
I didn’t find any existing tests for hkdfExpand. To ensure the buffer logic and algorithm are correct—and guard against regressions—please add unit tests using official RFC 5869 test vectors. For example:
• Create a test file (e.g. src/utils/generics.test.ts)
• Include at least:
- SHA-256 test vector with non-empty salt/info and OKM length
- SHA-256 vector with empty salt/info
- Edge case for maximum allowed length
• Verify that for each vector,await hkdfExpand(…)matches the expected OKM
This will confirm both the buffer slicing (t.slice(start, end+infoLength+1)) and hmac.slice(0, t.length - end) logic are correct.
🤖 Prompt for AI Agents
In src/utils/generics.ts around lines 142 to 175, there are no unit tests for
the hkdfExpand function, which is critical to verify the buffer handling and
algorithm correctness. Create a new test file named src/utils/generics.test.ts
and add unit tests using official RFC 5869 test vectors. Include tests for
SHA-256 with non-empty salt/info and OKM length, tests with empty salt/info, and
edge cases for maximum allowed output length. Each test should call await
hkdfExpand with the vector inputs and assert that the output matches the
expected OKM to ensure the buffer slicing and HMAC logic are correct.
Summary by CodeRabbit
New Features
Bug Fixes
Refactor
Documentation
Chores