Skip to content

Conversation

ayu-ch
Copy link

@ayu-ch ayu-ch commented Jun 2, 2025

Aims to solve the issue #123

@nandit123
Copy link
Member

/describe

Copy link

Title

fix: improve error handling in encryption/decryption functions


User description

Aims to solve the issue #123


PR Type

Bug fix, Enhancement


Description

  • Add custom error classes for encryption/decryption operations

  • Improve error handling with specific error types

  • Add data validation for corrupted encrypted data

  • Enhance password validation with specific error messages


Diagram Walkthrough

flowchart LR
  A["Encryption/Decryption Functions"] --> B["Custom Error Classes"]
  B --> C["EncryptionError"]
  B --> D["DecryptionError"]
  D --> E["InvalidPasswordError"]
  D --> F["CorruptedDataError"]
  A --> G["Data Validation"]
  G --> H["Password Verification"]
Loading

File Walkthrough

Relevant files
Error handling
EncryptionErrors.ts
Add custom encryption error classes                                           

src/errors/EncryptionErrors.ts

  • Create custom error hierarchy for encryption operations
  • Add EncryptionError base class with cause tracking
  • Add DecryptionError with specialized subclasses
  • Include InvalidPasswordError and CorruptedDataError types
+43/-0   
encryptionBrowser.ts
Enhance browser encryption error handling                               

src/Lighthouse/uploadEncrypted/encryptionBrowser.ts

  • Import custom error classes from EncryptionErrors
  • Replace generic error throwing with specific EncryptionError
  • Add data validation for corrupted cipher data
  • Handle OperationError as InvalidPasswordError in decryption
+29/-16 
encryptionNode.ts
Enhance Node.js encryption error handling                               

src/Lighthouse/uploadEncrypted/encryptionNode.ts

  • Import custom error classes from EncryptionErrors
  • Replace generic error throwing with specific EncryptionError
  • Add cipher data validation before decryption
  • Handle OperationError as InvalidPasswordError in decryption
+29/-16 

@aakash-taneja
Copy link
Collaborator

/review

@aakash-taneja
Copy link
Collaborator

/improve

Copy link

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

🎫 Ticket compliance analysis ✅

123 - PR Code Verified

Compliant requirements:

• Create error classes for encryption/decryption errors
• Fix decryptFile function to throw errors instead of returning undefined
• Distinguish between different error types (invalid password vs corrupted data)
• Update error handling in both browser and Node.js implementations

Requires further human verification:

• Verify that all calling code has been updated to handle thrown errors instead of checking for undefined

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review

Error Handling

The nested try-catch block in decryptFile might cause confusion. Consider refactoring to make the error flow clearer or adding comments to explain the nested error handling strategy.

try {
  const decryptedContent = await window.crypto.subtle.decrypt(
    {
      name: 'AES-GCM',
      iv: iv,
    },
    aesKey,
    data
  )
  return decryptedContent
} catch (error: any) {                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      
  if (error.name === 'OperationError') {
    throw new InvalidPasswordError();
  }
  throw error;
}
Type Safety

The cause parameter is typed as Error | unknown but then checked if it's an Error. Consider using a more specific type or adding more robust type checking.

constructor(message: string, cause?: Error | unknown) {
  super(message);
  this.name = 'DecryptionError';

  if (cause instanceof Error) {
    this.cause = cause;
  } else if (cause !== undefined) {                                                                                                                                                                                       
    this.cause = new Error(String(cause));
  }

Copy link

PR Code Suggestions ✨

CategorySuggestion                                                                                                                                    Impact
Possible issue
Improve null check safety

Consider adding a more specific check for null/undefined cipher before accessing
the byteLength property to prevent potential runtime errors. The current check
could throw if cipher is not null but also not an object with byteLength.

src/Lighthouse/uploadEncrypted/encryptionBrowser.ts [62-64]

-if (!cipher || cipher.byteLength < 28) {
+if (!cipher || typeof cipher !== 'object' || cipher.byteLength < 28) {
   throw new CorruptedDataError('Invalid or corrupted encrypted data');
 }
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that the check for cipher is insufficient and could lead to runtime errors if a truthy non-ArrayBuffer value is passed, proposing a more robust validation.

Medium
General
Fix formatting issue

The excessive whitespace in the catch block line might indicate a formatting
issue or potential hidden characters. Clean up the line to ensure consistent
code formatting and prevent potential runtime issues.

src/Lighthouse/uploadEncrypted/encryptionBrowser.ts [90-95]

-catch (error: any) {                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                                      
+catch (error: any) {
   if (error.name === 'OperationError') {
     throw new InvalidPasswordError();
   }
   throw error;
 }

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 2

__

Why: The suggestion correctly identifies and fixes a formatting issue with excessive whitespace, which improves code readability but has no functional impact.

Low
  • More

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

Successfully merging this pull request may close these issues.

3 participants