Skip to content

fix: Ensure passkey-derived keys are always treated as ed25519 #1625

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Merged
merged 5 commits into from
Sep 11, 2025
Merged

fix: Ensure passkey-derived keys are always treated as ed25519 #1625

merged 5 commits into from
Sep 11, 2025
+9 −5

Conversation

SmolinPavel
Copy link
Contributor

@SmolinPavel SmolinPavel commented Aug 29, 2025 •
edited
Loading

This PR addresses a bug in @near-js/biometric-ed25519 where passkey-derived key strings were not prefixed with ed25519:, causing KeyPair.fromString to throw errors:

Invalid encoded key format, must be <curve>:<encoded key>

The change ensures that all passkey-derived keys are properly prefixed, and adds a regression test to prevent future regressions.

Pre-flight checklist

  • I have read the Contributing Guidelines on pull requests.
  • Commit messages follow the conventional commits spec
  • If this is a code change: I have written unit tests.
  • If this changes code in a published package: I have run pnpm changeset to create a changeset JSON document appropriate for this change.
  • If this is a new API or substantial change: the PR has an accompanying issue (closes #0000) and the maintainers have approved on my working plan.

Motivation

The bug was caused by legacy behavior in @near-js/biometric-ed25519 where KeyPair.fromString was sometimes called with a raw base-encoded secret+public key string.

Legacy behavior (pre-fix) allowed KeyPairEd25519 to be constructed from a single-part string:

const parts = encodedKey.split(':');
if (parts.length === 1) {
    return new KeyPairEd25519(parts[0]);
}

See pre-fix state for reference.

The update introduced in #1355 made param to be KeyPairString, assuming there is always a prefix.

The update introduced in this PR enforces the prefix explicitly, ensuring compatibility with KeyPair.fromString and making the behavior consistent and predictable.

Test Plan

The regression tests were added, making sure that the call to KeyPair.fromString will fail without the ed25519 prefix, while the one with the prefix will work.

Related issues/PRs

#1355

@SmolinPavel
fix(biometric-ed25519): add "ed25519:" prefix to passkey-derived keys
91ca38c 
- Introduce `makeEd25519KeyString` helper to prepend the required prefix
- Update all call sites to use the helper when creating or retrieving KeyPair from passkey data
- Add regression test to ensure KeyPair.fromString parses correctly
- Test verifies that bare keys fail and prefixed keys succeed
@changeset-bot changeset-bot
Copy link

changeset-bot bot commented Aug 29, 2025 •
edited
Loading

🦋 Changeset detected

Latest commit: c0e8c19

The changes in this PR will be included in the next version bump.

This PR includes changesets to release 15 packages
Name Type
@near-js/biometric-ed25519 Patch
@near-js/accounts Patch
@near-js/client Patch
@near-js/cookbook Patch
@near-js/crypto Patch
@near-js/iframe-rpc Patch
@near-js/keystores-browser Patch
@near-js/keystores-node Patch
@near-js/keystores Patch
@near-js/providers Patch
@near-js/signers Patch
@near-js/tokens Patch
@near-js/transactions Patch
@near-js/types Patch
@near-js/utils Patch

Not sure what this means? Click here to learn what changesets are.

Click here if you're a maintainer who wants to add another changeset to this PR

@github-project-automation github-project-automation bot moved this to NEW❗ in DevTools Aug 29, 2025
@denbite
Copy link
Contributor

denbite commented Sep 9, 2025

hey, @SmolinPavel

thanks for contributing and highlighting the issue! the idea looks good to me, though I'd like to tweak the implementation slightly, since we know the output key will always be ed25519, we can replace KeyPair.fromString with a new KeyPairEd25519 that accepts an encoded secret key, this way we don't have to create a new function makeEd25519KeyString

as for the tests, we don't really have proper tests for this functionality yet, so if you have time/ability, it'd really valuable to add some that cover createKey and getKeys to reduce the chance of future issues, totally optional though

denbite
denbite reviewed Sep 9, 2025
View reviewed changes
packages/biometric-ed25519/src/index.ts Outdated
@@ -87,7 +87,7 @@ export const createKey = async (username: string): Promise<KeyPair> => {
const publicKeyBytes = get64BytePublicKeyFromPEM(publicKey);
const secretKey = sha256.create().update(Buffer.from(publicKeyBytes)).digest();
const pubKey = ed25519.getPublicKey(secretKey);
return KeyPair.fromString(baseEncode(new Uint8Array(Buffer.concat([Buffer.from(secretKey), Buffer.from(pubKey)]))) as KeyPairString);
return KeyPair.fromString(makeEd25519KeyString(secretKey, pubKey));
Copy link
Contributor

Choose a reason for hiding this comment

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

let's change that to new KeyPairEd25519

denbite
denbite requested changes Sep 9, 2025
View reviewed changes
packages/biometric-ed25519/src/index.ts Outdated
const firstKeyPair = KeyPair.fromString(baseEncode(new Uint8Array(Buffer.concat([Buffer.from(firstEDSecret), Buffer.from(firstEDPublic)]))) as KeyPairString);
const secondKeyPair = KeyPair.fromString(baseEncode(new Uint8Array(Buffer.concat([Buffer.from(secondEDSecret), Buffer.from(secondEDPublic)]))) as KeyPairString);
const firstKeyPair = KeyPair.fromString(makeEd25519KeyString(firstEDSecret, firstEDPublic));
const secondKeyPair = KeyPair.fromString(makeEd25519KeyString(secondEDSecret, secondEDPublic));
Copy link
Contributor

Choose a reason for hiding this comment

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

the same as above

@denbite denbite changed the title Fix: Ensure passkey-derived keys are prefixed with ed25519: and add regression test fix: Ensure passkey-derived keys are always treated as ed25519 Sep 11, 2025
@denbite denbite merged commit 12ece30 into near:master Sep 11, 2025
2 of 3 checks passed
@github-project-automation github-project-automation bot moved this from NEW❗ to Shipped 🚀 in DevTools Sep 11, 2025
@github-actions github-actions bot mentioned this pull request Sep 11, 2025
Open
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@denbite denbite denbite approved these changes

Assignees
No one assigned
Labels
None yet
Projects
DevTools
Status: Shipped 🚀
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@SmolinPavel @denbite