diff --git a/src/custodians/local/local-signer.ts b/src/custodians/local/local-signer.ts index ff6282d..37980fc 100644 --- a/src/custodians/local/local-signer.ts +++ b/src/custodians/local/local-signer.ts @@ -53,14 +53,19 @@ export class LocalSigner implements Custodian { /** This custodian signs locally. */ public readonly kind: CustodianKind = 'local' - /** Wallets held by this signer, keyed by classic r-address. */ - private readonly wallets: Map - /** The primary account's r-address. */ private readonly primaryAddress: string + /** + * Wallets held by this signer, keyed by classic r-address. Each carries a seed + * and private key, so this is a real JS private field: a TypeScript `private` + * is erased at compile time and `console.log`/`util.inspect` would walk + * straight into the Map and print the key material. + */ + readonly #wallets: Map + private constructor(wallets: Map, primaryAddress: string) { - this.wallets = wallets + this.#wallets = wallets this.primaryAddress = primaryAddress } @@ -175,7 +180,7 @@ export class LocalSigner implements Custodian { * @returns One account per wallet, keyed by r-address. */ public async listAccounts(): Promise { - return Array.from(this.wallets.keys(), (address) => ({ + return Array.from(this.#wallets.keys(), (address) => ({ address, signer: this, })) @@ -262,7 +267,7 @@ export class LocalSigner implements Custodian { } private walletFor(address: string): Wallet { - const wallet = this.wallets.get(address) + const wallet = this.#wallets.get(address) if (wallet === undefined) { throw new AccountNotFoundError(address) } diff --git a/src/custodians/palisade/auth/palisade-auth.service.ts b/src/custodians/palisade/auth/palisade-auth.service.ts index d4e242d..b07aed4 100644 --- a/src/custodians/palisade/auth/palisade-auth.service.ts +++ b/src/custodians/palisade/auth/palisade-auth.service.ts @@ -40,8 +40,11 @@ export interface PalisadeAuthServiceOptions { /** The Palisade client ID. */ clientId: string /** - * The Palisade client secret. Held in memory only; never logged or - * persisted. Long-lived; rotation is the caller's responsibility (TDD §9.5). + * The Palisade client secret. Never logged or persisted: the service stores it + * in a real JS private field, so it is absent from `console.log` and + * `JSON.stringify` output. Note this option object itself is a plain object — + * logging the config you pass in will still print the secret. Long-lived; + * rotation is the caller's responsibility. */ clientSecret: string /** Injectable clock for deterministic tests. Defaults to `Date.now`. */ @@ -67,14 +70,23 @@ export interface PalisadeAuthServiceOptions { export class PalisadeAuthService { private readonly authPort: PalisadeAuthPort private readonly clientId: string - private readonly clientSecret: string private readonly now: () => number - private accessToken: string | null = null private tokenExpirationMs: number | null = null /** Shared in-flight refresh; concurrent callers await this one promise. */ private refreshPromise: Promise | null = null + /** + * The Palisade client secret. A real JS private field, not a TypeScript + * `private`: the latter is erased at compile time, leaving an ordinary + * enumerable property that `console.log`, `util.inspect`, and + * `JSON.stringify` all print in the clear. + */ + readonly #clientSecret: string + + /** Cached bearer token — `#`-private for the same reason as {@link #clientSecret}. */ + #accessToken: string | null = null + /** * Construct a PalisadeAuthService. * @@ -83,7 +95,7 @@ export class PalisadeAuthService { public constructor(options: PalisadeAuthServiceOptions) { this.authPort = options.authPort this.clientId = options.clientId - this.clientSecret = options.clientSecret + this.#clientSecret = options.clientSecret this.now = options.now ?? Date.now } @@ -95,8 +107,8 @@ export class PalisadeAuthService { * @returns A valid bearer token. */ public async getToken(forceRefresh = false): Promise { - if (!forceRefresh && this.accessToken !== null && !this.isTokenExpired()) { - return this.accessToken + if (!forceRefresh && this.#accessToken !== null && !this.isTokenExpired()) { + return this.#accessToken } if (!forceRefresh && this.refreshPromise !== null) { return this.refreshPromise @@ -139,7 +151,7 @@ export class PalisadeAuthService { * @returns The current cached token, or `null` if none is cached. */ public getCurrentToken(): string | null { - return this.accessToken + return this.#accessToken } /** @@ -152,7 +164,7 @@ export class PalisadeAuthService { try { response = await this.authPort.exchangeCredential( this.clientId, - this.clientSecret, + this.#clientSecret, ) } catch (error) { // Never surface credential material; wrap transport/auth failures uniformly. @@ -168,7 +180,7 @@ export class PalisadeAuthService { ) } - this.accessToken = token + this.#accessToken = token this.tokenExpirationMs = this.now() + validityMs(response.expiresIn) return token } diff --git a/src/custodians/ripple/auth/custody-auth.service.ts b/src/custodians/ripple/auth/custody-auth.service.ts index d9812bc..dc80dc5 100644 --- a/src/custodians/ripple/auth/custody-auth.service.ts +++ b/src/custodians/ripple/auth/custody-auth.service.ts @@ -56,8 +56,10 @@ export interface CustodyAuthServiceOptions { /** Token endpoint port (HTTP in production, in-memory fake in tests). */ authPort: CustodyAuthPort /** - * Intent-author private key (PEM). Held in memory only; never logged or - * persisted. + * Intent-author private key (PEM). Never logged or persisted: the service + * stores it in a real JS private field, so it is absent from `console.log` + * and `JSON.stringify` output. Note this option object itself is a plain + * object — logging the options you pass in will still print the key. */ privateKey: string /** @@ -86,15 +88,25 @@ export interface CustodyAuthServiceOptions { export class CustodyAuthService { private readonly authPort: CustodyAuthPort private readonly keypair: KeypairService - private readonly privateKey: string private readonly publicKey: string private readonly now: () => number - private accessToken: string | null = null private tokenExpirationMs: number | null = null /** Shared in-flight refresh; concurrent callers await this one promise. */ private refreshPromise: Promise | null = null + /** + * The Custody signing key. A real JS private field, not a TypeScript + * `private`: the latter is erased at compile time, leaving an ordinary + * enumerable property that `console.log`, `util.inspect`, and + * `JSON.stringify` all print in the clear. `#`-fields are unreachable at + * runtime and skipped by every one of those. + */ + readonly #privateKey: string + + /** Cached bearer token — `#`-private for the same reason as {@link #privateKey}. */ + #accessToken: string | null = null + /** * Construct a CustodyAuthService. * @@ -102,11 +114,11 @@ export class CustodyAuthService { */ public constructor(options: CustodyAuthServiceOptions) { this.authPort = options.authPort - this.privateKey = options.privateKey + this.#privateKey = options.privateKey this.now = options.now ?? Date.now - this.keypair = KeypairService.fromPrivateKey(this.privateKey) + this.keypair = KeypairService.fromPrivateKey(this.#privateKey) const derivedPublicKey = KeypairService.derivePublicKeyBase64( - this.privateKey, + this.#privateKey, ) if ( options.publicKey !== undefined && @@ -127,8 +139,8 @@ export class CustodyAuthService { * @returns A valid JWT bearer token. */ public async getToken(forceRefresh = false): Promise { - if (!forceRefresh && this.accessToken !== null && !this.isTokenExpired()) { - return this.accessToken + if (!forceRefresh && this.#accessToken !== null && !this.isTokenExpired()) { + return this.#accessToken } if (!forceRefresh && this.refreshPromise !== null) { return this.refreshPromise @@ -171,7 +183,7 @@ export class CustodyAuthService { * @returns The current cached token, or `null` if none is cached. */ public getCurrentToken(): string | null { - return this.accessToken + return this.#accessToken } /** @@ -198,7 +210,7 @@ export class CustodyAuthService { ) } - this.accessToken = token + this.#accessToken = token const exp = extractExpFromJwt(token) this.tokenExpirationMs = exp === null @@ -214,7 +226,7 @@ export class CustodyAuthService { */ private signFreshChallenge(): SignedChallenge { const challenge = randomUUID() - const signature = this.keypair.sign(this.privateKey, challenge) + const signature = this.keypair.sign(this.#privateKey, challenge) return { challenge, publicKey: this.publicKey, signature } } } diff --git a/src/custodians/ripple/auth/intent-signer.ts b/src/custodians/ripple/auth/intent-signer.ts index 505609f..c7f094f 100644 --- a/src/custodians/ripple/auth/intent-signer.ts +++ b/src/custodians/ripple/auth/intent-signer.ts @@ -25,7 +25,11 @@ const canonicalize = canonicalizeImport as unknown as ( */ export class IntentSigner { private readonly keypair: KeypairService - private readonly privateKey: string + /** + * The intent-signing key. A real JS private field so it cannot be reached or + * serialized at runtime; a TypeScript `private` is only a compile-time label. + */ + readonly #privateKey: string /** * Construct an IntentSigner. @@ -43,7 +47,7 @@ export class IntentSigner { ) } this.keypair = keypair - this.privateKey = privateKey + this.#privateKey = privateKey } /** @@ -61,7 +65,7 @@ export class IntentSigner { 'Failed to canonicalize Custody intent request body', ) } - return this.keypair.sign(this.privateKey, canonical) + return this.keypair.sign(this.#privateKey, canonical) } /** diff --git a/test/unit/security/secret-exposure.test.ts b/test/unit/security/secret-exposure.test.ts new file mode 100644 index 0000000..9b2e4fe --- /dev/null +++ b/test/unit/security/secret-exposure.test.ts @@ -0,0 +1,143 @@ +import { inspect } from 'node:util' + +import { Wallet } from 'xrpl' + +import { PalisadeAuthService } from '../../../src/custodians/palisade/auth/palisade-auth.service.js' +import { CustodyAuthService } from '../../../src/custodians/ripple/auth/custody-auth.service.js' +import { IntentSigner } from '../../../src/custodians/ripple/auth/intent-signer.js' +import { KeypairService } from '../../../src/custodians/ripple/auth/keypair.service.js' +import { LocalSigner } from '../../../src/index.js' +import { generateTestKey } from '../custody-auth/test-utils.js' + +/** + * Secrets must survive being printed or serialized. + * + * TypeScript's `private` is erased at compile time, so a `private` field is an + * ordinary enumerable property at runtime: `console.log(custodian)` (which uses + * `util.inspect`) and `JSON.stringify(authService)` both walk into it and emit + * the value in the clear. The fields these tests cover therefore use real JS + * private fields (`#name`), which are unreachable at runtime and skipped by both. + * + * The assertions target the *observable* property — the secret is absent from + * both renderings — not the mechanism, so they keep holding if the + * implementation changes (a redacting `toJSON`, a custom inspect hook, …) and + * fail if a field is reverted to `private`. + */ + +const TOKEN = 'test-access-token-value-do-not-leak' +const CLIENT_SECRET = 'test-client-secret-do-not-leak' + +/** + * The base64 body of a PEM key — a needle guaranteed to be key material rather + * than the `-----BEGIN …-----` boilerplate every PEM shares. + * + * @param pem - The PEM-encoded key. + * @returns A substring of the key body. + */ +function keyBody(pem: string): string { + const body = pem + .split('\n') + .filter((line) => !line.startsWith('-----') && line.trim() !== '') + .join('') + return body.slice(0, 24) +} + +/** + * Assert a secret appears in neither of the two renderings that leak it. + * + * @param value - The object to render. + * @param secret - The substring that must not appear. + */ +function expectNotLeaked(value: unknown, secret: string): void { + expect(secret.length).toBeGreaterThan(8) + // `depth: null` walks the whole graph, so a secret nested behind several + // objects (custodian → http client → auth service) is still caught. + expect(inspect(value, { depth: null })).not.toContain(secret) + expect(JSON.stringify(value)).not.toContain(secret) +} + +describe('CustodyAuthService', () => { + it('does not expose its signing key to console.log or JSON.stringify', () => { + const pem = generateTestKey('ed25519') + const service = new CustodyAuthService({ + authPort: { + fetchToken: async (): Promise<{ access_token: string }> => ({ + access_token: TOKEN, + }), + }, + privateKey: pem, + }) + // The needle is genuinely part of the key we handed in, so a pass means + // concealment rather than a mis-built assertion. + expect(pem).toContain(keyBody(pem)) + expectNotLeaked(service, keyBody(pem)) + }) + + it('does not expose the cached bearer token once one is fetched', async () => { + const service = new CustodyAuthService({ + authPort: { + fetchToken: async (): Promise<{ access_token: string }> => ({ + access_token: TOKEN, + }), + }, + privateKey: generateTestKey('ed25519'), + }) + await service.getToken() + // Still reachable through the intended accessor — concealed, not removed. + expect(service.getCurrentToken()).toBe(TOKEN) + expectNotLeaked(service, TOKEN) + }) +}) + +describe('PalisadeAuthService', () => { + it('does not expose its client secret or cached token', async () => { + const service = new PalisadeAuthService({ + // Deliberately not the shared `FakeAuthPort`: that fixture records every + // `(clientId, clientSecret)` pair it receives so other tests can assert + // on them, which would surface the secret through the *port* rather than + // the service under test. A port is handed the secret by design; what + // matters here is that the service's own state doesn't retain it visibly. + authPort: { + exchangeCredential: async (): Promise<{ + accessToken: string + expiresIn: number + }> => ({ accessToken: TOKEN, expiresIn: 3600 }), + }, + clientId: 'client-id-is-not-secret', + clientSecret: CLIENT_SECRET, + }) + expectNotLeaked(service, CLIENT_SECRET) + + expect(await service.getToken()).toBe(TOKEN) + expectNotLeaked(service, CLIENT_SECRET) + expectNotLeaked(service, TOKEN) + }) +}) + +describe('IntentSigner', () => { + it('does not expose the key it signs with', () => { + const pem = generateTestKey('ed25519') + const signer = new IntentSigner(KeypairService.fromPrivateKey(pem), pem) + expectNotLeaked(signer, keyBody(pem)) + }) +}) + +describe('LocalSigner', () => { + it('does not expose the wallet seed or private key', () => { + const wallet = Wallet.generate() + const signer = LocalSigner.fromSeed(wallet.seed as string) + // Still functional — the wallets are concealed, not discarded. + expect(signer.primary.address).toBe(wallet.classicAddress) + expectNotLeaked(signer, wallet.seed as string) + expectNotLeaked(signer, wallet.privateKey) + }) + + it('does not expose any seed when holding multiple wallets', () => { + const wallets = [Wallet.generate(), Wallet.generate()] + const signer = LocalSigner.create({ wallets }) + for (const wallet of wallets) { + expectNotLeaked(signer, wallet.seed as string) + expectNotLeaked(signer, wallet.privateKey) + } + }) +})