feat: validate prism proofs for all web requests#1
Conversation
To be reused for accessing prism endpoints.
WalkthroughThis pull request introduces significant enhancements to the codebase, focusing on expanding cryptographic functionality, improving type definitions, and introducing new client and utility classes. The changes primarily revolve around Certificate Transparency (CT) log processing, Prism account management, and proof validation. Key additions include a new Changes
Sequence DiagramsequenceDiagram
participant Background as Background Process
participant PrismClient as Prism Client
participant ProofValidator as Proof Validator
participant CTLogClient as CT Log Client
Background Process->>PrismClient: Request Account
PrismClient-->>Background Process: Return Account Info
Background Process->>ProofValidator: Validate Prism Proof
ProofValidator-->>Background Process: Proof Validation Result
Background Process->>CTLogClient: Fetch Signed Tree Head
CTLogClient-->>Background Process: Return Tree Head
Poem
Finishing Touches
Thank you for using CodeRabbit. We offer it for free to the OSS community and would appreciate your support in helping us grow. If you find it useful, would you consider giving us a shout-out on your favorite social media? 🪧 TipsChatThere are 3 ways to chat with CodeRabbit:
Note: Be mindful of the bot's finite context window. It's strongly recommended to break down tasks such as reading entire modules into smaller chunks. For a focused discussion, use review comments to chat about specific files and their changes, instead of using the PR comments. 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: 10
🧹 Nitpick comments (16)
src/http_client.ts (2)
32-32: Include HTTP status code in error messagesWhen throwing an error due to a failed HTTP request, including the status code in the error message can aid in debugging and provide more context about the failure.
Apply this diff to enhance the error message:
if (!response.ok) { - throw new Error(`Http request failed: ${response.statusText}`); + throw new Error(`HTTP request failed with status ${response.status} (${response.statusText})`); }
53-53: Include HTTP status code in error messagesSimilarly, update the error message in the
postJsonmethod for consistency.Apply this diff:
if (!response.ok) { - throw new Error(`Http request failed: ${response.statusText}`); + throw new Error(`HTTP request failed with status ${response.status} (${response.statusText})`); }src/proof_validation.ts (2)
22-24: Correct the function documentation forvalidatePrismProofThe comment incorrectly states that
validatePrismProofvalidates a Merkle proof from a CT log server. It should specify that it validates a Merkle proof from a Prism node.Apply this diff to correct the comment:
/** - * Validates a Merkle proof from a CT log server + * Validates a Merkle proof from a Prism node */
34-34: Address the TODO: Calculate Prism leaf hash in the browser extensionThere's a TODO comment indicating the need to calculate the Prism leaf hash. Implementing this functionality is essential for the proof validation process.
Do you want me to help implement the leaf hash calculation? I can provide code to complete this task or open a GitHub issue to track it.
src/background.ts (2)
23-25: Remove unused variableiand clean uprun_nodefunctionThe variable
iis incremented but not used in therun_nodefunction. This is unnecessary and may lead to confusion. Consider removing it to clean up the code.Apply this diff to remove the unused variable:
- let i = 0; while (true) { await sleep(60000); - i += 1; console.log(`Node is still running ...`); }
90-98: Use strict equality operator and improve error message formattingUsing
!==instead of!=ensures strict comparison and prevents unexpected type coercion. Also, using template literals enhances the readability of the error message.Apply this diff:
-if (accountRes.account.signed_data.length != 1) { +if (accountRes.account.signed_data.length !== 1) { console.error( - "Incorrect number of prism entries (", - accountRes.account.signed_data.length, - ") for Log", - b64LogId, - ); + `Incorrect number of prism entries (${accountRes.account.signed_data.length}) for Log ${b64LogId}` ); return; }src/prism_client.ts (3)
14-19: Implement error handling for network requests ingetAccountThe
getAccountmethod assumes that the network request will always succeed. To make the code robust, add error handling to catch network errors or unexpected responses.Apply this diff to include error handling:
async getAccount(accountId: string): Promise<PrismAccountResponse> { const body = { id: accountId, }; - return await this.postJson("/get-account", body); + try { + return await this.postJson("/get-account", body); + } catch (error) { + console.error(`Error fetching account ${accountId}:`, error); + throw error; + } }
21-23: Implement error handling for network requests ingetCommitmentSimilar to
getAccount, thegetCommitmentmethod should handle potential network errors to prevent unhandled exceptions.Apply this diff to include error handling:
async getCommitment(): Promise<PrismCommitmentResponse> { - return await this.fetchJson("/get-current-commitment"); + try { + return await this.fetchJson("/get-current-commitment"); + } catch (error) { + console.error("Error fetching commitment:", error); + throw error; + } }
4-11: Define comprehensive interfaces for API responsesEnsure that the interfaces
PrismAccountResponseandPrismCommitmentResponseaccurately represent all possible API responses, including error cases. This will improve type safety and code clarity.Consider updating the interfaces to include potential error fields:
interface PrismAccountResponse { account?: PrismAccount; proof: PrismProof; + error?: string; } interface PrismCommitmentResponse { commitment: string; + error?: string; }src/ct_log_types.ts (1)
38-42: Consider typing thetiled_logsarrayThe
any[]type fortiled_logsreduces type safety. Consider defining a proper interface for tiled logs or document why the type is intentionally loose.src/byte_arrays.ts (1)
14-25: Consider optimizing array concatenation for large arraysThe current implementation creates a new array and copies data multiple times. For large arrays or frequent operations, consider using a more efficient approach.
export function concatenateArrays(...arrays: Uint8Array[]): Uint8Array { + // Early return for single array or empty input + if (arrays.length === 0) return new Uint8Array(0); + if (arrays.length === 1) return arrays[0]; + const totalLength = arrays.reduce((sum, arr) => sum + arr.length, 0); const result = new Uint8Array(totalLength); let offset = 0; for (const arr of arrays) { result.set(arr, offset); offset += arr.length; } return result; }src/prism_types.ts (1)
61-65: Add RSA algorithm support for broader compatibilityThe current
PrismCryptoAlgorithmenum only supports elliptic curve algorithms. Consider adding RSA support for broader compatibility with existing systems.src/prism_serialization.ts (1)
72-74: Add explicit null check for service_challengeThe optional chaining operator is not sufficient for runtime type safety. Add an explicit null check to prevent potential undefined behavior.
const serviceChallengeBytes = account.service_challenge - ? serviceChallengeToBincode(account.service_challenge) + ? (account.service_challenge === null + ? new Uint8Array([0]) + : serviceChallengeToBincode(account.service_challenge)) : new Uint8Array([0]);src/ct_parsing.ts (1)
Line range hint
44-122: Add JSDoc documentation for the function.The implementation correctly follows RFC 6962 for pre-certificate entries. Consider adding documentation to explain the function's purpose and return value format.
Add this documentation:
/** * Generates the MerkleTreeLeaf bytes for a pre-certificate as specified in RFC 6962. * * @param certDer - The DER-encoded pre-certificate * @param issuerDer - The DER-encoded issuer certificate * @param sct_time - The timestamp for the SCT * @param sct_extensions - The SCT extensions * @returns A Uint8Array containing the MerkleTreeLeaf bytes with 0x00 prefix */src/conversion.ts (2)
36-70: Consider using TypedArray.from for better performance.The implementation is correct but could be optimized.
Consider this alternative implementation:
export function hexToBytes(hexString: string): Uint8Array { const normalizedHex = hexString.replace(/^0x/i, "").toUpperCase(); if (!/^[0-9A-F]*$/.test(normalizedHex)) { throw new Error("Invalid hexadecimal string"); } const paddedHex = normalizedHex.length % 2 ? "0" + normalizedHex : normalizedHex; - const bytes = new Uint8Array(paddedHex.length / 2); - - for (let i = 0; i < paddedHex.length; i += 2) { - bytes[i / 2] = parseInt(paddedHex.substr(i, 2), 16); - } - - return bytes; + return Uint8Array.from(paddedHex.match(/.{2}/g) || [], byte => parseInt(byte, 16)); }
93-115: Consider using a lookup table for better performance.While the implementation is correct, bit operations in a loop can be slower than using a lookup table.
Consider this optimization:
export function bytesToBits(uint8Array: Uint8Array): Uint8Array { + const BITS_LOOKUP = new Uint8Array(256 * 8); + for (let i = 0; i < 256; i++) { + for (let j = 0; j < 8; j++) { + BITS_LOOKUP[i * 8 + j] = (i >> (7 - j)) & 1; + } + } + const bits = new Uint8Array(uint8Array.length * 8); for (let i = 0; i < uint8Array.length; i++) { - const byte = uint8Array[i]; - for (let j = 0; j < 8; j++) { - bits[i * 8 + j] = (byte >> (7 - j)) & 1; - } + bits.set(BITS_LOOKUP.subarray(uint8Array[i] * 8, uint8Array[i] * 8 + 8), i * 8); } return bits; }
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (14)
src/background.ts(3 hunks)src/byte_arrays.ts(1 hunks)src/conversion.ts(2 hunks)src/ct_log_client.ts(1 hunks)src/ct_log_store.ts(1 hunks)src/ct_log_types.ts(2 hunks)src/ct_parsing.ts(4 hunks)src/ct_proof_validation.ts(0 hunks)src/hashing.ts(1 hunks)src/http_client.ts(1 hunks)src/prism_client.ts(1 hunks)src/prism_serialization.ts(1 hunks)src/prism_types.ts(1 hunks)src/proof_validation.ts(1 hunks)
💤 Files with no reviewable changes (1)
- src/ct_proof_validation.ts
✅ Files skipped from review due to trivial changes (1)
- src/ct_log_store.ts
🔇 Additional comments (8)
src/background.ts (2)
86-87: Avoid logging sensitive informationLogging
b64LogIdmay expose sensitive information. Ensure that logs do not contain any sensitive data to adhere to security best practices.[security]
Apply this diff to modify the log message:
- console.error("CT Log", b64LogId, "not found in prism"); + console.error("CT Log not found in prism");
100-100: Address the TODO regardingsigned_data.keyvalidationThere's a TODO comment questioning whether
signed_data.keyneeds to be checked. It's important to resolve TODOs before merging to ensure code completeness.Would you like assistance in implementing the check for
signed_data.keyor creating a GitHub issue to track this task?src/hashing.ts (1)
1-3: Ensurecrypto.subtle.digestis supported in all target environmentsThe
crypto.subtle.digestmethod may not be available in all browsers or environments, particularly in older versions or certain browser extension contexts. Verify that this method is supported in all intended environments, or provide a fallback implementation.Consider adding a feature detection mechanism or polyfill to handle environments where
crypto.subtleis not available.src/ct_log_types.ts (1)
61-66:⚠️ Potential issueVerify alignment with RFC 6962 for CtSignedTreeHead changes
The modifications to
CtSignedTreeHeadinterface deviate from the standard Certificate Transparency format defined in RFC 6962. Thetree_head_signatureis a crucial field for verifying the authenticity of the tree head.Consider reverting to RFC 6962 compliant field names or document why we're deviating:
export interface CtSignedTreeHead { version: number; signatureType: number; timestamp: number; treeSize: number; rootHash: Uint8Array; + treeHeadSignature: string; // Required for RFC 6962 compliance }src/ct_parsing.ts (1)
6-13: LGTM! Well-organized imports and constants.The new imports and constants are properly structured for Certificate Transparency processing.
src/conversion.ts (3)
Line range hint
1-19: LGTM! Well-documented base64 decoder.The function is properly renamed and documented with clear examples.
21-34: LGTM! Well-documented base64 encoder.The function is properly renamed and documented with clear examples.
72-91: LGTM! Well-implemented hex encoder.The function correctly handles all cases with proper padding and optional prefix.
| } | ||
|
|
||
| async getSignedTreeHead(): Promise<CtSignedTreeHead> { | ||
| export class CTLogClient extends HttpClient { |
There was a problem hiding this comment.
Add constructor to pass baseUrl to HttpClient
The CTLogClient class extends HttpClient but does not define a constructor to pass the baseUrl to the HttpClient's constructor. Since HttpClient requires a baseUrl parameter, this will lead to a runtime error due to baseUrl being undefined.
Apply this diff to add the constructor:
export class CTLogClient extends HttpClient {
+ constructor(baseUrl: string | URL) {
+ super(baseUrl);
+ }
async getSignedTreeHead(): Promise<CtSignedTreeHeadResponse> {
return await this.fetchJson("/ct/v1/get-sth", {});
}| console.log("Validating prism proof for", key); | ||
| console.log("key:", bytesToHex(keyHash)); | ||
| console.log("val:", bytesToHex(valueHash)); | ||
| console.log("leaf:", bytesToHex(leafHash)); | ||
| console.log("expected:", expectedRootHashHex); | ||
|
|
||
| const bits = bytesToBits(keyHash) | ||
| .reverse() | ||
| .slice(256 - proof.siblings.length); | ||
| const sibling_hashes = proof.siblings.map(hexToBytes); | ||
|
|
||
| const calculatedRootHash = await calculateRootHash( | ||
| leafHash, | ||
| bits, | ||
| sibling_hashes, | ||
| PRISM_INTERNAL_DOMAIN_SEPARATOR, | ||
| ); | ||
|
|
||
| console.log( | ||
| "Comparing hashes", | ||
| bytesToHex(calculatedRootHash), | ||
| bytesToHex(expectedRootHash), | ||
| ); |
There was a problem hiding this comment.
Avoid logging sensitive data in production
The console.log statements are outputting potentially sensitive information such as hashes and keys used in proof validation. Logging such data can lead to security risks.
Consider removing these log statements or ensuring they are only included in a development or debug mode.
Apply this diff to remove or conditionally include the logs:
console.log("Validating prism proof for", key);
console.log("key:", bytesToHex(keyHash));
console.log("val:", bytesToHex(valueHash));
console.log("leaf:", bytesToHex(leafHash));
console.log("expected:", expectedRootHashHex);
+ // Remove or guard these logs with a debug flag if necessary.
...
console.log(
"Comparing hashes",
bytesToHex(calculatedRootHash),
bytesToHex(expectedRootHash),
);
+ // Remove or guard these logs with a debug flag if necessary.Also applies to: 95-101
| accountRes.account === undefined || | ||
| accountRes.proof.leaf === undefined | ||
| ) { |
There was a problem hiding this comment.
Safely access nested properties to prevent runtime errors
When checking accountRes.proof.leaf, if accountRes.proof is undefined, accessing accountRes.proof.leaf will cause a runtime error. Use optional chaining (?.) to safely access nested properties.
Apply this diff to prevent potential runtime errors:
if (
accountRes.account === undefined ||
- accountRes.proof.leaf === undefined
+ accountRes.proof?.leaf === undefined
) {📝 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.
| accountRes.account === undefined || | |
| accountRes.proof.leaf === undefined | |
| ) { | |
| accountRes.account === undefined || | |
| accountRes.proof?.leaf === undefined | |
| ) { |
|
|
||
| run_node(); | ||
|
|
||
| const prism = new PrismClient("http://127.0.0.1:50524"); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid hardcoding the Prism client URL
Hardcoding the URL "http://127.0.0.1:50524" reduces flexibility and makes it difficult to configure the application for different environments. Consider externalizing this URL using a configuration file, environment variable, or browser storage.
Here's a suggestion to use an environment variable:
-const prism = new PrismClient("http://127.0.0.1:50524");
+const PRISM_CLIENT_URL = process.env.PRISM_CLIENT_URL || "http://127.0.0.1:50524";
+const prism = new PrismClient(PRISM_CLIENT_URL);Alternatively, you could use browser extension storage or a configuration object.
📝 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 prism = new PrismClient("http://127.0.0.1:50524"); | |
| const PRISM_CLIENT_URL = process.env.PRISM_CLIENT_URL || "http://127.0.0.1:50524"; | |
| const prism = new PrismClient(PRISM_CLIENT_URL); |
| if (!isPrismProofValid) { | ||
| await domainVerificationStore.reportLogVerification( | ||
| domain, | ||
| log.description, | ||
| isPrismProofValid, | ||
| ); | ||
| return; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Continue processing SCTs after failed proof validation
Currently, if isPrismProofValid is false, the function returns early, which stops the processing of remaining SCTs. Consider using continue instead of return to ensure that all SCTs are processed.
Apply this diff to continue processing:
if (!isPrismProofValid) {
await domainVerificationStore.reportLogVerification(
domain,
log.description,
isPrismProofValid,
);
- return;
+ continue;
}Committable suggestion skipped: line range outside the PR's diff.
| export function areArraysEqual(arr1: Uint8Array, arr2: Uint8Array): boolean { | ||
| if (arr1.length !== arr2.length) { | ||
| return false; | ||
| } | ||
| return arr1.every((value, index) => value === arr2[index]); | ||
| } |
There was a problem hiding this comment.
Consider using constant-time comparison for cryptographic operations
The current array comparison using every might be vulnerable to timing attacks if used for comparing sensitive cryptographic data.
export function areArraysEqual(arr1: Uint8Array, arr2: Uint8Array): boolean {
if (arr1.length !== arr2.length) {
return false;
}
- return arr1.every((value, index) => value === arr2[index]);
+ // Constant-time comparison
+ let result = 0;
+ for (let i = 0; i < arr1.length; i++) {
+ result |= arr1[i] ^ arr2[i];
+ }
+ return result === 0;
}📝 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.
| export function areArraysEqual(arr1: Uint8Array, arr2: Uint8Array): boolean { | |
| if (arr1.length !== arr2.length) { | |
| return false; | |
| } | |
| return arr1.every((value, index) => value === arr2[index]); | |
| } | |
| export function areArraysEqual(arr1: Uint8Array, arr2: Uint8Array): boolean { | |
| if (arr1.length !== arr2.length) { | |
| return false; | |
| } | |
| // Constant-time comparison | |
| let result = 0; | |
| for (let i = 0; i < arr1.length; i++) { | |
| result |= arr1[i] ^ arr2[i]; | |
| } | |
| return result === 0; | |
| } |
| key: PrismCryptoPayload; | ||
| data: string; //base64 encoded bytes | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Consider using Uint8Array for cryptographic data instead of base64 strings
Storing cryptographic data as base64 strings in memory increases the attack surface. Consider using Uint8Array directly and only converting to base64 when necessary for transport.
| export function num64ToBincodeLe(num: number): Uint8Array { | ||
| const buffer = new ArrayBuffer(8); // 8 bytes for 64-bit integer | ||
| const view = new DataView(buffer); | ||
| view.setBigUint64(0, BigInt(num), true); | ||
| return new Uint8Array(buffer); | ||
| } |
There was a problem hiding this comment.
Add bounds checking for num64ToBincodeLe
The function should validate that the input number is within safe bounds for BigInt conversion to prevent potential overflow issues.
export function num64ToBincodeLe(num: number): Uint8Array {
+ if (!Number.isSafeInteger(num) || num < 0) {
+ throw new Error(`Invalid number for 64-bit conversion: ${num}`);
+ }
const buffer = new ArrayBuffer(8);
const view = new DataView(buffer);
view.setBigUint64(0, BigInt(num), true);
return new Uint8Array(buffer);
}📝 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.
| export function num64ToBincodeLe(num: number): Uint8Array { | |
| const buffer = new ArrayBuffer(8); // 8 bytes for 64-bit integer | |
| const view = new DataView(buffer); | |
| view.setBigUint64(0, BigInt(num), true); | |
| return new Uint8Array(buffer); | |
| } | |
| export function num64ToBincodeLe(num: number): Uint8Array { | |
| if (!Number.isSafeInteger(num) || num < 0) { | |
| throw new Error(`Invalid number for 64-bit conversion: ${num}`); | |
| } | |
| const buffer = new ArrayBuffer(8); // 8 bytes for 64-bit integer | |
| const view = new DataView(buffer); | |
| view.setBigUint64(0, BigInt(num), true); | |
| return new Uint8Array(buffer); | |
| } |
| export function sthFromBytes(bytes: Uint8Array): CtSignedTreeHead { | ||
| const view = new DataView(bytes.buffer, 0, bytes.length); | ||
| const version = view.getUint8(0); | ||
| const signatureType = view.getUint8(1); | ||
| const timestamp = Number(view.getBigUint64(2)); | ||
| const treeSize = Number(view.getBigUint64(10)); | ||
| const rootHash = bytes.slice(18, 50); | ||
|
|
||
| return { | ||
| version, | ||
| signatureType, | ||
| timestamp, | ||
| treeSize, | ||
| rootHash, | ||
| }; | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add bounds checking for input array.
The function correctly parses STH bytes, but should validate the input length to prevent buffer overruns.
Add this validation at the start of the function:
export function sthFromBytes(bytes: Uint8Array): CtSignedTreeHead {
+ if (bytes.length < 50) {
+ throw new Error('Invalid STH bytes: insufficient length');
+ }
const view = new DataView(bytes.buffer, 0, bytes.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.
| export function sthFromBytes(bytes: Uint8Array): CtSignedTreeHead { | |
| const view = new DataView(bytes.buffer, 0, bytes.length); | |
| const version = view.getUint8(0); | |
| const signatureType = view.getUint8(1); | |
| const timestamp = Number(view.getBigUint64(2)); | |
| const treeSize = Number(view.getBigUint64(10)); | |
| const rootHash = bytes.slice(18, 50); | |
| return { | |
| version, | |
| signatureType, | |
| timestamp, | |
| treeSize, | |
| rootHash, | |
| }; | |
| } | |
| export function sthFromBytes(bytes: Uint8Array): CtSignedTreeHead { | |
| if (bytes.length < 50) { | |
| throw new Error('Invalid STH bytes: insufficient length'); | |
| } | |
| const view = new DataView(bytes.buffer, 0, bytes.length); | |
| const version = view.getUint8(0); | |
| const signatureType = view.getUint8(1); | |
| const timestamp = Number(view.getBigUint64(2)); | |
| const treeSize = Number(view.getBigUint64(10)); | |
| const rootHash = bytes.slice(18, 50); | |
| return { | |
| version, | |
| signatureType, | |
| timestamp, | |
| treeSize, | |
| rootHash, | |
| }; | |
| } |
| /** | ||
| * Converts a number to its binary representation as an array of bits. | ||
| * The number is treated as a 64-bit value and converted to 64 bits. | ||
| * | ||
| * @param num - The number to convert to bits | ||
| * @returns A Uint8Array where each element is a single bit (0 or 1) | ||
| * @example | ||
| * ```typescript | ||
| * const bits = numberToBits(5); | ||
| * // bits represents the 64-bit binary form of 5 | ||
| * ``` | ||
| */ | ||
| export function numberToBits(num: number): Uint8Array { | ||
| const bytes = new Uint8Array(8); | ||
| for (let i = 0; i < 8; i++) { | ||
| bytes[i] = (num >> (56 - i * 8)) & 0xff; | ||
| } | ||
| return bytesToBits(bytes); | ||
| } |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Add input validation for number range.
The function should validate that the input number is within the safe integer range.
Add this validation:
export function numberToBits(num: number): Uint8Array {
+ if (!Number.isSafeInteger(num) || num < 0) {
+ throw new Error('Input must be a non-negative safe integer');
+ }
const bytes = new Uint8Array(8);
for (let i = 0; i < 8; i++) {
bytes[i] = (num >> (56 - i * 8)) & 0xff;
}
return bytesToBits(bytes);
}📝 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.
| /** | |
| * Converts a number to its binary representation as an array of bits. | |
| * The number is treated as a 64-bit value and converted to 64 bits. | |
| * | |
| * @param num - The number to convert to bits | |
| * @returns A Uint8Array where each element is a single bit (0 or 1) | |
| * @example | |
| * ```typescript | |
| * const bits = numberToBits(5); | |
| * // bits represents the 64-bit binary form of 5 | |
| * ``` | |
| */ | |
| export function numberToBits(num: number): Uint8Array { | |
| const bytes = new Uint8Array(8); | |
| for (let i = 0; i < 8; i++) { | |
| bytes[i] = (num >> (56 - i * 8)) & 0xff; | |
| } | |
| return bytesToBits(bytes); | |
| } | |
| /** | |
| * Converts a number to its binary representation as an array of bits. | |
| * The number is treated as a 64-bit value and converted to 64 bits. | |
| * | |
| * @param num - The number to convert to bits | |
| * @returns A Uint8Array where each element is a single bit (0 or 1) | |
| * @example |
Reuse code from CT log Merkle proof validation as much as possible.
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (6)
src/proof_validation.ts (2)
87-88: 🛠️ Refactor suggestionAvoid logging sensitive error information
Logging the error object may expose sensitive information. Consider logging error messages without including the error details or ensure that sensitive data is not exposed when logging.
Apply this diff to adjust the logging:
- console.error("Error validating Merkle proof:", error); + console.error("Error validating Merkle proof.");
122-123: 🛠️ Refactor suggestionAvoid logging sensitive error information
Logging the error object may expose sensitive information. Consider logging error messages without including the error details or ensure that sensitive data is not exposed when logging.
Apply this diff to adjust the logging:
- console.error("Error validating Merkle proof:", error); + console.error("Error validating Merkle proof.");src/ct_log_client.ts (1)
19-19:⚠️ Potential issueAdd constructor to pass
baseUrltoHttpClientThe
CTLogClientclass extendsHttpClientbut does not define a constructor to pass thebaseUrlto theHttpClient's constructor. SinceHttpClientrequires abaseUrlparameter, this will lead to a runtime error due tobaseUrlbeingundefined.src/ct_parsing.ts (1)
39-54:⚠️ Potential issueAdd bounds checking for input array.
The function correctly parses STH bytes, but should validate the input length to prevent buffer overruns.
src/background.ts (2)
105-105: 🛠️ Refactor suggestionAvoid hardcoding the Prism client URL
Hardcoding the URL reduces flexibility and makes it difficult to configure the application for different environments.
175-175:⚠️ Potential issueUse optional chaining for nested properties
When checking
accountRes.proof.leaf, use optional chaining to prevent potential runtime errors.
🧹 Nitpick comments (5)
src/proof_validation.ts (1)
51-90: Consider providing more detailed error handlingWhile the current implementation catches errors during validation, providing more specific error messages or introducing custom error types can improve debuggability without exposing sensitive information.
src/prism_client.ts (1)
25-38: Consider adding request body type.The implementation looks good, but consider adding a type for the request body to improve type safety.
+interface GetAccountRequest { + id: string; +} async getAccount(accountId: string): Promise<PrismAccountResponse> { - const body = { + const body: GetAccountRequest = { id: accountId, }; return await this.postJson("/get-account", body); }src/ct_parsing.ts (1)
Line range hint
67-145: Consider breaking down the complex byte manipulation.The function is well-documented but contains complex byte manipulation logic. Consider extracting the byte manipulation into smaller, focused helper functions for better maintainability.
For example:
function writeTimestamp(view: DataView, offset: number, timestamp: BigInt): number { view.setBigUint64(offset, timestamp); return offset + 8; } function writeIssuerKeyHash(bytes: Uint8Array, offset: number, hash: Uint8Array): number { bytes.set(hash, offset); return offset + hash.length; }src/background.ts (2)
187-187: Track TODO for light node integration.The comment indicates a pending task to query commitment from light node instead of REST.
Would you like me to create a GitHub issue to track this TODO item?
121-128: Improve node simulation logging.The current implementation only logs a basic message. Consider adding more informative logging:
- console.log(`Node is still running ...`); + console.log(`Prism light node running (iteration ${i}). Waiting for implementation...`);
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (10)
src/background.ts(3 hunks)src/ct_log_client.ts(2 hunks)src/ct_log_store.ts(1 hunks)src/ct_parsing.ts(4 hunks)src/ct_proof_validation.ts(0 hunks)src/hashing.ts(1 hunks)src/http_client.ts(1 hunks)src/prism_client.ts(1 hunks)src/proof_validation.ts(1 hunks)src/verification_store.ts(3 hunks)
💤 Files with no reviewable changes (1)
- src/ct_proof_validation.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- src/ct_log_store.ts
- src/http_client.ts
🧰 Additional context used
🪛 Biome (1.9.4)
src/verification_store.ts
[error] 40-40: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
🔇 Additional comments (10)
src/verification_store.ts (1)
63-63: Ensure proper promise handling in asynchronous operationsThe
reportLogVerificationmethod returns aPromise<void>, but the operation provided toenqueuedoes not explicitly return a value. Consider ensuring that any asynchronous operations within the method are properly handled to prevent unhandled promise rejections.src/hashing.ts (1)
1-19: Well-implemented SHA-256 hashing functionThe
sha256function is correctly implemented using the Web Crypto API. The use ofasync/awaitenhances readability, and the detailed JSDoc comments improve maintainability.src/prism_client.ts (3)
8-11: LGTM! Well-structured response type.The interface is well-defined with appropriate optional/required fields.
17-19: LGTM! Clear and focused interface.The interface is simple and well-documented.
46-48: LGTM! Simple and focused implementation.The method is well-documented and handles the endpoint correctly.
src/ct_log_client.ts (4)
8-13: LGTM! Well-structured response type.The interface includes all necessary fields for a Signed Tree Head response.
25-25: LGTM! Simple and focused implementation.The method correctly fetches the Signed Tree Head.
Line range hint
29-46: LGTM! Well-documented and type-safe implementation.The method handles parameters correctly and includes comprehensive documentation.
Line range hint
47-62: LGTM! Clear implementation with good error documentation.The method is well-documented and handles parameters appropriately.
src/background.ts (1)
84-85: 🛠️ Refactor suggestionConsider continuing validation after error.
The early return after an error prevents validation of remaining SCTs. Consider using
continueinstead to process all SCTs.- return; + continue;Likely invalid or redundant comment.
| * @returns Promise resolving to the operation result | ||
| */ | ||
| private enqueue<T>(operation: () => Promise<T> | T): Promise<T> { | ||
| return (this.queue = this.queue.then(() => operation())); |
There was a problem hiding this comment.
🛠️ Refactor suggestion
Avoid assignment within return statement
Assigning this.queue within the return statement can be confusing and may reduce code readability. It's better to separate the assignment from the return statement for clarity.
Apply this diff to separate the assignment:
private enqueue<T>(operation: () => Promise<T> | T): Promise<T> {
- return (this.queue = this.queue.then(() => operation()));
+ this.queue = this.queue.then(() => operation());
+ return this.queue;
}📝 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.
| return (this.queue = this.queue.then(() => operation())); | |
| private enqueue<T>(operation: () => Promise<T> | T): Promise<T> { | |
| this.queue = this.queue.then(() => operation()); | |
| return this.queue; | |
| } |
🧰 Tools
🪛 Biome (1.9.4)
[error] 40-40: The assignment should not be in an expression.
The use of assignments in expressions is confusing.
Expressions are often considered as side-effect free.
(lint/suspicious/noAssignInExpressions)
1 participant
Summary by CodeRabbit
Based on the comprehensive changes across multiple files, here are the updated release notes:
New Features
Improvements
Security Enhancements
Performance