Skip to content

Conversation

@ieow
Copy link

@ieow ieow commented May 8, 2025

Motivation and Context

Jira Link:

Description

How has this been tested?

Screenshots (if appropriate):

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)

Checklist:

  • My code follows the code style of this project. (run lint)
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.
  • My code requires a db migration.

Note

Introduces a Noble-based ECIES path alongside existing eccrypto.

  • Adds src/nobleEncryption.ts implementing nobleEncrypt/nobleDecrypt (ECDH via @noble/secp256k1, KDF with sha512, AES-CBC via @noble/ciphers, HMAC-SHA256 for MAC) plus helpers to convert between NobleEcies and existing Ecies, and thin encrypt/decrypt wrappers
  • Extends tests (test/encrypt_decrypt.spec.ts) to verify interoperability and equivalence between Noble-based and existing implementations, including MAC/padding behavior and conversions
  • Adds dependencies: @noble/secp256k1, @noble/ciphers, @noble/hashes

Written by Cursor Bugbot for commit 6dd217f. This will update automatically on new commits. Configure here.

@chaitanyapotti
Copy link
Member

Copy link

@cursor cursor bot left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Cursor Bugbot has reviewed your changes and found 1 potential issue.

const sharedSecret = getSharedSecret(ephemPrivateKey, publicKeyTo);

// need to remove first byte
const sharedSecretSliced = sharedSecret.slice(1);
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Noble uses wrong shared secret format causing incompatibility

High Severity

The getSharedSecret call returns the full uncompressed point (65 bytes: 0x04 | X | Y), and slice(1) yields 64 bytes (X | Y). However, the existing eccrypto implementation uses only the X coordinate (~32 bytes) from derive(). Hashing 64 bytes vs 32 bytes produces completely different encryption keys and MACs, making the noble implementation incompatible with the existing one. The test expecting convertedEcies to equal encrypted would fail.

Additional Locations (1)

Fix in Cursor Fix in Web

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants