chore(sdk): Break up the crypto service implementation into smaller more managable files - #888
Conversation
📝 WalkthroughWalkthroughThis pull request refactors the cryptographic library by extracting crypto implementations from the monolithic Changes
Estimated code review effort🎯 4 (Complex) | ⏱️ ~70 minutes Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
📝 Coding Plan
Comment Tip CodeRabbit can use oxc to improve the quality of JavaScript and TypeScript code reviews.Add a configuration file to your project to customize how CodeRabbit runs oxc. |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request significantly refactors the core cryptography service within the SDK. The primary goal was to improve the structure and readability of the crypto implementation by decomposing a large, single file into several smaller, more focused modules. This change enhances the clarity of the codebase, making it easier to understand, test, and extend specific cryptographic functionalities without affecting unrelated parts of the service. Highlights
🧠 New Feature in Public Preview: You can now enable Memory to help Gemini Code Assist learn from your team's feedback. This makes future code reviews more consistent and personalized to your project's style. Click here to enable Memory in your admin console. Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request successfully refactors the crypto service into smaller, more modular files, improving organization and maintainability. The changes involve moving various cryptographic operations (EC, RSA, symmetric, signing, key formatting, and key wrapping/unwrapping) into their respective core files and updating the main index.ts to re-export these functions. This significantly enhances the clarity and manageability of the codebase.
X-Test Failure Report✅ go-main |
X-Test Failure Report✅ java-main |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (9)
lib/tdf3/src/crypto/core/rsa.ts (1)
130-144: Replaceconsole.assertwith proper validation.
console.assert(line 134) is a debugging aid that may be stripped in production builds or disabled. For crypto code that needs to validate inputs, use explicit checks that throw meaningful errors.♻️ Proposed fix
export async function decryptWithPrivateKey( encryptedPayload: Binary, privateKey: PrivateKey ): Promise<Binary> { - console.assert(typeof encryptedPayload === 'object', 'encryptedPayload must be object'); + if (!encryptedPayload || typeof encryptedPayload !== 'object') { + throw new ConfigurationError('encryptedPayload must be a Binary object'); + } const cryptoKey = unwrapKey(privateKey);🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tdf3/src/crypto/core/rsa.ts` around lines 130 - 144, Replace the debug-only console.assert in decryptWithPrivateKey with explicit input validation that throws a descriptive error: check that encryptedPayload is the expected Binary object (and optionally that privateKey is present) before calling unwrapKey and crypto.subtle.decrypt, and throw a TypeError or custom Error with a clear message if validation fails so callers get a reliable runtime failure instead of a no-op assertion; update decryptWithPrivateKey to perform these checks prior to calling unwrapKey and crypto.subtle.decrypt.lib/tdf3/src/crypto/core/keys.ts (1)
12-58: Consider extracting shared curve/modulus parsing logic.The algorithm parsing logic (RSA modulus extraction and EC curve mapping) is duplicated verbatim between
wrapPublicKeyandwrapPrivateKey. This could be extracted into a shared helper to reduce duplication and ensure consistency.♻️ Proposed refactor to reduce duplication
+function parseAlgorithmMetadata(algorithm: KeyAlgorithm): { modulusBits?: number; curve?: string } { + if (algorithm.startsWith('rsa:')) { + return { modulusBits: parseInt(algorithm.split(':')[1], 10) }; + } else if (algorithm.startsWith('ec:')) { + const curvePart = algorithm.split(':')[1]; + const curve = + curvePart === 'secp256r1' + ? 'P-256' + : curvePart === 'secp384r1' + ? 'P-384' + : curvePart === 'secp521r1' + ? 'P-521' + : undefined; + return { curve }; + } + return {}; +} + export function wrapPublicKey(key: CryptoKey, algorithm: KeyAlgorithm): PublicKey { - const result: any = { + return { _brand: 'PublicKey', algorithm, _internal: key, - }; - if (algorithm.startsWith('rsa:')) { - result.modulusBits = parseInt(algorithm.split(':')[1], 10); - } else if (algorithm.startsWith('ec:')) { - const curvePart = algorithm.split(':')[1]; - result.curve = - curvePart === 'secp256r1' - ? 'P-256' - : curvePart === 'secp384r1' - ? 'P-384' - : curvePart === 'secp521r1' - ? 'P-521' - : undefined; - } - return result as PublicKey; + ...parseAlgorithmMetadata(algorithm), + } as PublicKey; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tdf3/src/crypto/core/keys.ts` around lines 12 - 58, The RSA/EC parsing logic duplicated in wrapPublicKey and wrapPrivateKey should be extracted into a small shared helper (e.g., parseKeyAlgorithm or getKeyParams) that takes the KeyAlgorithm string and returns an object like { modulusBits?: number, curve?: string }; replace the inline code in both functions to call this helper and assign result.modulusBits/result.curve accordingly, keeping the existing behavior for 'rsa:' and 'ec:' mappings and preserving the _brand/_internal fields and return types for wrapPublicKey and wrapPrivateKey.lib/tdf3/src/crypto/core/ec.ts (2)
81-95: Potential undefined access if curve type is widened.At line 88,
curveBits[curve]could beundefinedifcurvedoesn't match a key in the record. While the current type system should prevent this (sincecurveis validated to be anECCurve), adding a guard would make this more robust against future changes.🛡️ Proposed defensive check
const curveBits: Record<ECCurve, number> = { 'P-256': 256, 'P-384': 384, // P-521 derives 528 bits (66 bytes) 'P-521': 528, }; const bits = curveBits[curve]; + if (!bits) { + throw new ConfigurationError(`Unsupported curve for ECDH: ${curve}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tdf3/src/crypto/core/ec.ts` around lines 81 - 95, The lookup curveBits[curve] can be undefined if curve is widened; update the code that computes bits (the curveBits map and the const bits = curveBits[curve]) to validate the result and fail-fast: after computing bits, check if bits is a finite number and if not throw a clear error (e.g., "Unsupported ECCurve: " + curve) so deriveBits never receives undefined, referencing the variables curveBits, curve and bits and the deriveBits call where the value is used.
42-54: Unreachable default case.The
defaultcase at line 52-53 can never execute becausecurveToNamedCurve(called at line 33) would have already thrown for any unsupported curve value. Consider removing it or adding an exhaustiveness check instead.♻️ Proposed fix using exhaustiveness check
switch (namedCurve) { case 'P-256': algorithm = 'ec:secp256r1'; break; case 'P-384': algorithm = 'ec:secp384r1'; break; case 'P-521': algorithm = 'ec:secp521r1'; break; - default: - throw new ConfigurationError(`Unsupported curve: ${namedCurve}`); + default: { + const _exhaustive: never = namedCurve; + throw new ConfigurationError(`Unsupported curve: ${_exhaustive}`); + } }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tdf3/src/crypto/core/ec.ts` around lines 42 - 54, The switch on namedCurve (after curveToNamedCurve) has an unreachable default; replace it with an exhaustiveness check instead of throwing there: remove the default branch and add an explicit unreachable/assert helper (e.g., assertUnreachable(value) or a TypeScript never-branch) so TypeScript/linters know all cases are handled, or simply delete the default and rely on curveToNamedCurve to throw; reference the switch over namedCurve, curveToNamedCurve call, and ConfigurationError when making the change.lib/tdf3/src/crypto/core/symmetric.ts (3)
188-197: Usesubstringinstead of deprecatedsubstr.
String.prototype.substris deprecated. Usesubstringorsliceinstead for forward compatibility.♻️ Proposed fix
export function hex2Ab(hex: string): ArrayBuffer { const buffer = new ArrayBuffer(hex.length / 2); const bufferView = new Uint8Array(buffer); for (let i = 0; i < hex.length; i += 2) { - bufferView[i / 2] = parseInt(hex.substr(i, 2), 16); + bufferView[i / 2] = parseInt(hex.substring(i, i + 2), 16); } return buffer; }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tdf3/src/crypto/core/symmetric.ts` around lines 188 - 197, The hex2Ab function uses the deprecated String.prototype.substr; update hex2Ab to use substring (or slice) instead of substr when extracting two-character chunks (replace hex.substr(i, 2) with hex.substring(i, i+2) or hex.slice(i, i+2)) so the loop in hex2Ab continues to parse pairs correctly and returns the same ArrayBuffer; ensure you only change the substring call and keep buffer/Uint8Array logic and parseInt(..., 16) intact.
83-116: Replaceconsole.assertwith proper validation.Lines 89-91 use
console.assertfor null checks. These assertions may be stripped in production. For crypto operations, explicit validation with meaningful errors is preferred.♻️ Proposed fix
async function _doEncrypt( payload: Binary | SymmetricKey, key: SymmetricKey, iv: Binary, algorithm?: AlgorithmUrn ): Promise<EncryptResult> { - console.assert(payload != null); - console.assert(key != null); - console.assert(iv != null); + if (payload == null || key == null || iv == null) { + throw new ConfigurationError('encrypt requires payload, key, and iv'); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tdf3/src/crypto/core/symmetric.ts` around lines 83 - 116, In _doEncrypt replace the console.assert(null) checks with explicit validation that throws descriptive errors: verify payload, key, and iv are not null/undefined at the top of the function and throw TypeError or RangeError with clear messages (e.g., "payload is required", "key is required", "iv is required") before proceeding to use getSymmetricAlgoDomString, unwrapSymmetricKey, or _importKey; keep the rest of the logic unchanged so subsequent calls to unwrapSymmetricKey, getSymmetricAlgoDomString, _importKey and crypto.subtle.encrypt operate on validated inputs.
118-156: Sameconsole.assertissue in_doDecrypt.Lines 125-127 have the same
console.assertpattern that should be replaced with proper validation.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tdf3/src/crypto/core/symmetric.ts` around lines 118 - 156, Replace the three console.assert calls in _doDecrypt with explicit validation that throws descriptive errors when required inputs are missing: check payload, key, and iv (the parameters to _doDecrypt) and if any is null/undefined throw a TypeError (or a more specific error type used elsewhere) with a clear message like "payload is required", "key is required", or "iv is required" so callers get deterministic exceptions instead of silent assertions; keep the rest of _doDecrypt (authTag handling, getSymmetricAlgoDomString, unwrapSymmetricKey, _importKey, and the OperationError handling that throws DecryptError) unchanged.lib/tdf3/src/crypto/core/key-format.ts (2)
321-341: RSA key imported twice during algorithm detection.For RSA private keys without an
algorithmHint, the key is imported once at line 322-328 for modulus detection, then imported again at line 384-390 for actual use. While functionally correct, this could be optimized by reusing the first import if the algorithm matches.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tdf3/src/crypto/core/key-format.ts` around lines 321 - 341, The RSA private key is imported twice (tempKey used to read modulus, then re-imported later); change the flow in the algorithm-detection branch to keep and reuse the initially imported CryptoKey (tempKey) when algorithmHint is absent and the determined algorithm (set via modulusBits -> algorithm) matches the later expected RSA algorithm, instead of re-importing; store tempKey in a variable like importedKey and use it for subsequent operations (ensuring its usages/permissions match the later call's expectations) and only perform a second import if the algorithm differs or permissions are insufficient.
251-275: IIFE for throwing is unusual; consider simplification.The pattern at lines 260-262 using an IIFE to throw inside a ternary chain is clever but unconventional and harder to read. Consider extracting the curve mapping to a helper function for clarity.
♻️ Proposed refactor
+const EC_CURVE_MAP: Record<string, string> = { + secp256r1: 'P-256', + secp384r1: 'P-384', + secp521r1: 'P-521', +}; + // In importPublicKey: } else if (algorithm.startsWith('ec:')) { const curve = algorithm.split(':')[1]; - const namedCurve = - curve === 'secp256r1' - ? 'P-256' - : curve === 'secp384r1' - ? 'P-384' - : curve === 'secp521r1' - ? 'P-521' - : (() => { - throw new ConfigurationError(`Unsupported EC curve: ${curve}`); - })(); + const namedCurve = EC_CURVE_MAP[curve]; + if (!namedCurve) { + throw new ConfigurationError(`Unsupported EC curve: ${curve}`); + }🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@lib/tdf3/src/crypto/core/key-format.ts` around lines 251 - 275, Replace the IIFE-in-ternary that throws for unsupported EC curves with a clear helper or mapping to improve readability: extract the curve-to-namedCurve logic into a function (e.g., getNamedCurve or mapEcCurve) and call it inside the algorithm.startsWith('ec:') branch to set namedCurve, and have that helper throw the same ConfigurationError(`Unsupported EC curve: ${curve}`) for unknown curves; leave the surrounding usage handling (usage === 'derive' / 'sign' and resulting cryptoAlgorithm/keyUsages) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@lib/tdf3/src/crypto/core/key-format.ts`:
- Around line 116-130: The current bucketing loses the actual RSA modulus size;
instead of mapping modulusBits into buckets, set the algorithm to the exact size
string (e.g., algorithm = `rsa:${modulusBits}`) after validating it against
MIN_ASYMMETRIC_KEY_SIZE_BITS and the maximum supported size (keep the existing
ConfigurationError throws for too-small or too-large keys), so
PublicKeyInfo.algorithm preserves the real bit length (this will make the
parseInt(algorithm.split(':')[1], 10) in keys.ts return the actual modulus
size). Use the existing symbols modulusBits, extractRsaModulusBitLength,
PublicKeyInfo['algorithm'], MIN_ASYMMETRIC_KEY_SIZE_BITS, publicKeyPem and
ConfigurationError to locate and change the assignment logic.
In `@lib/tdf3/src/crypto/core/rsa.ts`:
- Around line 20-35: The rsaOaepSha1 function uses SHA-1 which is deprecated;
add a new exported rsaOaepSha256 function mirroring rsaOaepSha1 but setting
hash.name to 'SHA-256' (keep the same argument check against
MIN_ASYMMETRIC_KEY_SIZE_BITS and throw ConfigurationError on invalid sizes) and
document or comment that rsaOaepSha1 is retained only for legacy
interoperability; ensure rsaOaepSha256 returns the same RsaHashedKeyGenParams
shape (name: 'RSA-OAEP', modulusLength, publicExponent).
In `@lib/tdf3/src/crypto/core/signing.ts`:
- Around line 142-165: The code parses DER r/s components but reads
signature[offset + 1] and slices without bounds checks; update the parsing in
signing.ts (the block that computes rLen and sLen, using variables offset, rLen,
sLen, r, s and componentLen) to first validate that offset + 1 <
signature.length before reading the length bytes, then verify signature has at
least rLen and sLen bytes remaining (offset + rLen <= signature.length and
similar for s) before slicing, and also check r.length and s.length > 0 before
accessing r[0]/s[0]; throw ConfigurationError on any invalid/malformed lengths
to avoid out-of-bounds reads.
---
Nitpick comments:
In `@lib/tdf3/src/crypto/core/ec.ts`:
- Around line 81-95: The lookup curveBits[curve] can be undefined if curve is
widened; update the code that computes bits (the curveBits map and the const
bits = curveBits[curve]) to validate the result and fail-fast: after computing
bits, check if bits is a finite number and if not throw a clear error (e.g.,
"Unsupported ECCurve: " + curve) so deriveBits never receives undefined,
referencing the variables curveBits, curve and bits and the deriveBits call
where the value is used.
- Around line 42-54: The switch on namedCurve (after curveToNamedCurve) has an
unreachable default; replace it with an exhaustiveness check instead of throwing
there: remove the default branch and add an explicit unreachable/assert helper
(e.g., assertUnreachable(value) or a TypeScript never-branch) so
TypeScript/linters know all cases are handled, or simply delete the default and
rely on curveToNamedCurve to throw; reference the switch over namedCurve,
curveToNamedCurve call, and ConfigurationError when making the change.
In `@lib/tdf3/src/crypto/core/key-format.ts`:
- Around line 321-341: The RSA private key is imported twice (tempKey used to
read modulus, then re-imported later); change the flow in the
algorithm-detection branch to keep and reuse the initially imported CryptoKey
(tempKey) when algorithmHint is absent and the determined algorithm (set via
modulusBits -> algorithm) matches the later expected RSA algorithm, instead of
re-importing; store tempKey in a variable like importedKey and use it for
subsequent operations (ensuring its usages/permissions match the later call's
expectations) and only perform a second import if the algorithm differs or
permissions are insufficient.
- Around line 251-275: Replace the IIFE-in-ternary that throws for unsupported
EC curves with a clear helper or mapping to improve readability: extract the
curve-to-namedCurve logic into a function (e.g., getNamedCurve or mapEcCurve)
and call it inside the algorithm.startsWith('ec:') branch to set namedCurve, and
have that helper throw the same ConfigurationError(`Unsupported EC curve:
${curve}`) for unknown curves; leave the surrounding usage handling (usage ===
'derive' / 'sign' and resulting cryptoAlgorithm/keyUsages) unchanged.
In `@lib/tdf3/src/crypto/core/keys.ts`:
- Around line 12-58: The RSA/EC parsing logic duplicated in wrapPublicKey and
wrapPrivateKey should be extracted into a small shared helper (e.g.,
parseKeyAlgorithm or getKeyParams) that takes the KeyAlgorithm string and
returns an object like { modulusBits?: number, curve?: string }; replace the
inline code in both functions to call this helper and assign
result.modulusBits/result.curve accordingly, keeping the existing behavior for
'rsa:' and 'ec:' mappings and preserving the _brand/_internal fields and return
types for wrapPublicKey and wrapPrivateKey.
In `@lib/tdf3/src/crypto/core/rsa.ts`:
- Around line 130-144: Replace the debug-only console.assert in
decryptWithPrivateKey with explicit input validation that throws a descriptive
error: check that encryptedPayload is the expected Binary object (and optionally
that privateKey is present) before calling unwrapKey and crypto.subtle.decrypt,
and throw a TypeError or custom Error with a clear message if validation fails
so callers get a reliable runtime failure instead of a no-op assertion; update
decryptWithPrivateKey to perform these checks prior to calling unwrapKey and
crypto.subtle.decrypt.
In `@lib/tdf3/src/crypto/core/symmetric.ts`:
- Around line 188-197: The hex2Ab function uses the deprecated
String.prototype.substr; update hex2Ab to use substring (or slice) instead of
substr when extracting two-character chunks (replace hex.substr(i, 2) with
hex.substring(i, i+2) or hex.slice(i, i+2)) so the loop in hex2Ab continues to
parse pairs correctly and returns the same ArrayBuffer; ensure you only change
the substring call and keep buffer/Uint8Array logic and parseInt(..., 16)
intact.
- Around line 83-116: In _doEncrypt replace the console.assert(null) checks with
explicit validation that throws descriptive errors: verify payload, key, and iv
are not null/undefined at the top of the function and throw TypeError or
RangeError with clear messages (e.g., "payload is required", "key is required",
"iv is required") before proceeding to use getSymmetricAlgoDomString,
unwrapSymmetricKey, or _importKey; keep the rest of the logic unchanged so
subsequent calls to unwrapSymmetricKey, getSymmetricAlgoDomString, _importKey
and crypto.subtle.encrypt operate on validated inputs.
- Around line 118-156: Replace the three console.assert calls in _doDecrypt with
explicit validation that throws descriptive errors when required inputs are
missing: check payload, key, and iv (the parameters to _doDecrypt) and if any is
null/undefined throw a TypeError (or a more specific error type used elsewhere)
with a clear message like "payload is required", "key is required", or "iv is
required" so callers get deterministic exceptions instead of silent assertions;
keep the rest of _doDecrypt (authTag handling, getSymmetricAlgoDomString,
unwrapSymmetricKey, _importKey, and the OperationError handling that throws
DecryptError) unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: a7345910-3ac1-4062-8fdd-1f612b76e6ce
📒 Files selected for processing (7)
lib/tdf3/src/crypto/core/ec.tslib/tdf3/src/crypto/core/key-format.tslib/tdf3/src/crypto/core/keys.tslib/tdf3/src/crypto/core/rsa.tslib/tdf3/src/crypto/core/signing.tslib/tdf3/src/crypto/core/symmetric.tslib/tdf3/src/crypto/index.ts
2 participants
Refactors the core cryptography service within the SDK. The primary goal was to improve the structure and readability of the crypto implementation by decomposing a large, single file into several smaller, more focused modules. This change enhances the clarity of the codebase, making it easier to understand, test, and extend specific cryptographic functionalities without affecting unrelated parts of the service.
Summary by CodeRabbit
Release Notes