Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
1 change: 1 addition & 0 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -61,6 +61,7 @@
},
"resolutions": {
"@metamask/snaps-sdk": "^11.1.0",
"@metamask/snaps-utils": "12.2.0",

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

why we need this bump?

"axios@npm:1.14.0": "1.16.0"
},
"packageManager": "yarn@4.17.0",
Expand Down
1 change: 1 addition & 0 deletions packages/snap/package.json
Original file line number Diff line number Diff line change
Expand Up @@ -51,6 +51,7 @@
"@metamask/snaps-sdk": "^11.1.0",
"@metamask/superstruct": "^3.2.1",
"@metamask/utils": "^11.11.0",
"@scure/base": "~1.1.6",
"@stellar/stellar-sdk": "^15.0.1",
"@types/jest": "^30.0.0",
"async-mutex": "^0.5.0",
Expand Down
22 changes: 20 additions & 2 deletions packages/snap/snap.manifest.json
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,7 @@
"url": "https://github.com/MetaMask/snap-stellar-wallet.git"
},
"source": {
"shasum": "Inf8VxNlsHTyLYST3nsK1X0sdkSiAbkFciLrCTsYZNY=",
"shasum": "C5TM6OATflDM6FkRgpmaTGrnB/6zl3kSFBHqwpC0P+k=",
"location": {
"npm": {
"filePath": "dist/bundle.js",
Expand All @@ -23,7 +23,25 @@
},
"initialPermissions": {
"endowment:keyring": {
"allowedOrigins": ["https://portfolio.metamask.io"]
"allowedOrigins": ["https://portfolio.metamask.io"],
"capabilities": {
"scopes": ["stellar:pubnet", "stellar:testnet"],
"privateKey": {
"exportFormats": [
{
"encoding": "hexadecimal"
},
{
"encoding": "base58"
}
]
},
"bip44": {
"deriveIndex": true,
"deriveIndexRange": true,
"discover": true
}
}
},
"snap_getBip32Entropy": [
{
Expand Down
1 change: 1 addition & 0 deletions packages/snap/src/context.ts
Original file line number Diff line number Diff line change
Expand Up @@ -176,6 +176,7 @@ const keyringHandler = new KeyringHandler({
accountService,
onChainAccountService,
transactionService,
accountResolver,
handlers: keyringMethodHandlers,
});

Expand Down
27 changes: 26 additions & 1 deletion packages/snap/src/handlers/keyring/api.test.ts
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
import { assert, StructError } from '@metamask/superstruct';
import { assert, is, StructError } from '@metamask/superstruct';

import {
Base58Struct,
CreateAccountOptionsStruct,
ResolveAccountAddressRequestStruct,
DiscoverAccountsStruct,
Expand Down Expand Up @@ -628,3 +629,27 @@ describe('ListAccountTransactionsRequestStruct', () => {
);
});
});

describe('Base58Struct', () => {
it('accepts a valid base58 string', () => {
expect(is('StV1DL6CwTryKyV', Base58Struct)).toBe(true);
});

it.each(['0', 'O', 'I', 'l'])(
'rejects a string containing the excluded character %s',
(excluded) => {
expect(is(`StV1DL6CwTryKyV${excluded}`, Base58Struct)).toBe(false);
},
);

it('rejects an empty string', () => {
expect(is('', Base58Struct)).toBe(false);
});

it.each([123, null, undefined, {}, []])(
'rejects a non-string value',
(value) => {
expect(is(value, Base58Struct)).toBe(false);
},
);
});
25 changes: 25 additions & 0 deletions packages/snap/src/handlers/keyring/api.ts
Original file line number Diff line number Diff line change
@@ -1,4 +1,5 @@
import { KeyringRequestStruct } from '@metamask/keyring-api';
import { ExportAccountOptionsStruct } from '@metamask/keyring-api/v2';
import {
object,
min,
Expand All @@ -15,6 +16,7 @@ import {
nullable,
enums,
refine,
define,
} from '@metamask/superstruct';
import type { Infer } from '@metamask/superstruct';
import { base64 } from '@metamask/utils';
Expand Down Expand Up @@ -320,6 +322,29 @@ export const GetAccountRequestStruct = UuidStruct;
*/
export const DeleteAccountRequestStruct = UuidStruct;

/**
* Base58 string (Bitcoin alphabet). Used as a boolean `is` guard on the
* exported private key — NEVER as an asserting validator (a StructError would
* embed the key value in its message).
*/
export const Base58Struct = define<string>(

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

please consider the same validation as solana if we need base58

'Base58',
(value) =>
typeof value === 'string' && /^[1-9A-HJ-NP-Za-km-z]+$/u.test(value),
);

/**
* Handler-level request shape for `exportAccount`, distinct from the
* wire-level `ExportAccountRequestStruct` exported by both
* `@metamask/keyring-api` (root barrel, via `rpc.mjs`) and its `/v2`
* subpath (via `keyring-rpc.mjs`), which validates the outer JSON-RPC
* envelope: `{ jsonrpc, id, method, params: { id, options } }`.
*/
export const ExportAccountHandlerRequestStruct = object({
accountId: UuidStruct,
options: optional(ExportAccountOptionsStruct),
});

/**
* Validation struct for the listAccountAssets request.
*/
Expand Down
154 changes: 135 additions & 19 deletions packages/snap/src/handlers/keyring/keyring.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -7,9 +7,9 @@ import {
} from '@metamask/keyring-api';
import {
emitSnapKeyringEvent,
handleKeyringRequest,
MethodNotSupportedError,
} from '@metamask/keyring-snap-sdk';
import { handleKeyringRequest } from '@metamask/keyring-snap-sdk/v2';
import { InvalidParamsError, type JsonRpcRequest } from '@metamask/snaps-sdk';
import { create } from '@metamask/superstruct';
import type { Json } from '@metamask/utils';
Expand Down Expand Up @@ -51,6 +51,8 @@ import {
createMockTransactionService,
generateMockTransactions,
} from '../../services/transaction/__mocks__/transaction.fixtures';
import { WalletService } from '../../services/wallet';
import { getTestWallet } from '../../services/wallet/__mocks__/wallet.fixtures';
import {
getSlip44AssetId,
getDefaultEntropySource,
Expand All @@ -59,6 +61,7 @@ import {
} from '../../utils';
import { bufferToUint8Array } from '../../utils/buffer';
import { logger } from '../../utils/logger';
import { AccountResolver } from '../accountResolver';
import { SyncAccountsHandler } from '../cronjob/syncAccounts';

jest.mock('../../utils/logger');
Expand All @@ -68,12 +71,17 @@ jest.mock('../../utils/requestResponse', () => ({
validateOrigin: jest.fn(),
}));
jest.mock('@metamask/keyring-snap-sdk', () => ({
handleKeyringRequest: jest.fn(),
emitSnapKeyringEvent: jest.fn(),
MethodNotSupportedError: jest.requireActual('@metamask/keyring-snap-sdk')
.MethodNotSupportedError,
}));
jest.mock('@metamask/keyring-snap-sdk/v2', () => ({
handleKeyringRequest: jest.fn(),
}));

describe('KeyringHandler', () => {
const entropySourceId = 'entropy-source-1';
const NON_EXISTENT_ID = '00000000-0000-4000-8000-000000000000';
let keyringHandler: KeyringHandler;
let mockAccount: StellarKeyringAccount;
let mockAccountId: string;
Expand Down Expand Up @@ -128,14 +136,20 @@ describe('KeyringHandler', () => {
mockSignTransactionHandler = { handle: jest.fn() };
mockSignAuthEntryHandler = { handle: jest.fn() };

const { accountService, onChainAccountService } =
const { accountService, onChainAccountService, walletService } =
mockOnChainAccountService();
const { transactionService } = createMockTransactionService();
const accountResolver = new AccountResolver({
accountService,
onChainAccountService,
walletService,
});
keyringHandler = new KeyringHandler({
logger,
accountService,
onChainAccountService,
transactionService,
accountResolver,
handlers: {
[MultichainMethod.SignMessage]: mockSignMessageHandler,
[MultichainMethod.SignTransaction]: mockSignTransactionHandler,
Expand Down Expand Up @@ -214,28 +228,22 @@ describe('KeyringHandler', () => {

describe('getAccount', () => {
it('gets an account by its ID', async () => {
jest
.spyOn(AccountService.prototype, 'findById')
.mockResolvedValue(mockAccount);

const result = await keyringHandler.getAccount(mockAccountId);
expect(result).toStrictEqual(toKeyringAccount(mockAccount));
});

it('returns undefined if the account is not found', async () => {
jest
.spyOn(AccountService.prototype, 'findById')
.mockResolvedValue(undefined);
const { resolveAccountSpy } = getAccountServiceSpies();
resolveAccountSpy.mockResolvedValue({ account: mockAccount });

const result = await keyringHandler.getAccount(mockAccountId);

expect(result).toBeUndefined();
expect(resolveAccountSpy).toHaveBeenCalledWith({
accountId: mockAccountId,
});
expect(result).toStrictEqual(toKeyringAccount(mockAccount));
});

it('propagates errors when account retrieval fails', async () => {
jest
.spyOn(AccountService.prototype, 'findById')
.mockRejectedValue(new Error('Account retrieval failed'));
const { resolveAccountSpy } = getAccountServiceSpies();
resolveAccountSpy.mockRejectedValue(
new Error('Account retrieval failed'),
);

await expect(keyringHandler.getAccount(mockAccountId)).rejects.toThrow(
'Account retrieval failed',
Expand All @@ -249,6 +257,114 @@ describe('KeyringHandler', () => {
});
});

describe('getAccount (v2 semantics)', () => {
it('throws for an unknown account id instead of returning undefined', async () => {
const { resolveAccountSpy } = getAccountServiceSpies();
resolveAccountSpy.mockRejectedValue(
new AccountNotFoundException(NON_EXISTENT_ID),
);

await expect(keyringHandler.getAccount(NON_EXISTENT_ID)).rejects.toThrow(
AccountNotFoundException,
);
});
});

describe('getAccounts', () => {
it('returns the same result as listAccounts', async () => {
const { listAccountsSpy } = getAccountServiceSpies();
listAccountsSpy.mockResolvedValue([mockAccount]);

expect(await keyringHandler.getAccounts()).toStrictEqual(
await keyringHandler.listAccounts(),
);
});
});

describe('exportAccount', () => {
const setupExportWallet = () => {
const wallet = getTestWallet();
const { resolveAccountSpy } = getAccountServiceSpies();
resolveAccountSpy.mockResolvedValue({ account: mockAccount });
const resolveWalletSpy = jest
.spyOn(WalletService.prototype, 'resolveWallet')
.mockResolvedValue(wallet);

return { wallet, resolveAccountSpy, resolveWalletSpy };
};

it('returns the hex-encoded raw seed by default', async () => {
setupExportWallet();

const result = await keyringHandler.exportAccount(mockAccountId);

expect(result).toStrictEqual({
type: 'private-key',
encoding: 'hexadecimal',
privateKey: expect.stringMatching(/^0x[0-9a-f]{64}$/u),
});
});

it('respects the requested base58 encoding', async () => {
setupExportWallet();

const result = await keyringHandler.exportAccount(mockAccountId, {
type: 'private-key',
encoding: 'base58',
});

expect(result.encoding).toBe('base58');
// Same alphabet used by `Base58Struct` in ./api.ts.
expect(result.privateKey).toMatch(/^[1-9A-HJ-NP-Za-km-z]+$/u);
expect(result.privateKey).not.toMatch(/^0x[0-9a-f]{64}$/u);
});

it('throws for an unknown account id', async () => {
const { resolveAccountSpy } = getAccountServiceSpies();
resolveAccountSpy.mockRejectedValue(
new AccountNotFoundException(NON_EXISTENT_ID),
);

await expect(
keyringHandler.exportAccount(NON_EXISTENT_ID),
).rejects.toThrow(AccountNotFoundException);
});

it('throws without leaking the derived key when it fails the encoding guard', async () => {
const { wallet } = setupExportWallet();
const garbageKey = 'not-a-valid-encoded-key!!!';
jest.spyOn(wallet, 'exportKey').mockReturnValue(garbageKey);

let caughtError: unknown;
try {
await keyringHandler.exportAccount(mockAccountId);
} catch (error) {
caughtError = error;
}

expect(caughtError).toBeInstanceOf(Error);
expect((caughtError as Error).message).not.toContain(garbageKey);
});

it('throws when the derived key does not match the requested encoding', async () => {
const { wallet } = setupExportWallet();
// Valid hex, but the caller asked for base58 — a wallet.exportKey bug
// returning the wrong format must not silently pass validation.
jest
.spyOn(wallet, 'exportKey')
.mockReturnValue(
'0x1111111111111111111111111111111111111111111111111111111111111111',
);

await expect(
keyringHandler.exportAccount(mockAccountId, {
type: 'private-key',
encoding: 'base58',
}),
).rejects.toThrow('Derived private key failed encoding validation');
});
});

describe('createAccount', () => {
it('creates an account', async () => {
const { createAccountSpy } = getAccountServiceSpies();
Expand Down
Loading
Loading