fix(mpp/ts): enforce 32-byte HMAC secret floor at the @solana/mpp boundary (audit #24)#170
Conversation
…olana-foundation#24) mppx@0.5.x accepts any non-empty secretKey as the HMAC-SHA256 key that binds challenge IDs, so a weak key (e.g. "key") lets an attacker forge challenges. Until a floor lands upstream in mppx, gate at the @solana/mpp boundary: wrap Mppx.create to reject a secret shorter than 32 bytes (NIST SP 800-107), checking both the explicit secretKey and the MPP_SECRET_KEY env path. Mirrors the existing challenge-guard pattern; the missing-secret case still defers to mppx's own error. Bumps test secrets that were under the new floor.
Greptile SummaryThis PR adds a defense-in-depth HMAC secret-strength guard at the
Confidence Score: 3/5The guard logic and test-secret bumps are sound, but the spread-based delegation in secret-guard.ts may silently hollow out the guarded Mppx object if mppx exposes its namespace as a class with non-enumerable statics, which warrants verification before merging. The core concern is the spread pattern in secret-guard.ts: if mppx ships Mppx as a class, only create survives the spread and any other static methods would be undefined at runtime while TypeScript types still declare them. The missing tests for no-secret pass-through and conflicting-signal priority are secondary gaps. All other files are low-risk mechanical bumps. typescript/packages/mpp/src/server/secret-guard.ts warrants a close look at how MppxBase is structured upstream before merging; typescript/packages/mpp/src/tests/secret-guard.test.ts would benefit from two additional test cases. Important Files Changed
|
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| export const Mppx: typeof MppxBase = { | ||
| ...MppxBase, | ||
| create, | ||
| }; |
There was a problem hiding this comment.
{ ...MppxBase, create } only copies own enumerable properties from MppxBase. If mppx exposes Mppx as a class (where static methods have enumerable: false by spec), every method except the explicitly overridden create will be silently absent from the guarded object at runtime even though TypeScript's typeof MppxBase type says they exist. Current tests may all exercise create-only paths, masking this. A safer alternative is to use Object.create or an explicit Proxy so future mppx additions are automatically delegated.
| afterEach(() => { | ||
| delete process.env.MPP_SECRET_KEY; | ||
| }); | ||
|
|
||
| test('rejects an explicit secret key shorter than the floor', () => { | ||
| expect(() => Mppx.create({ methods: methods(), secretKey: tooShort })).toThrow(/at least 32 bytes/); | ||
| }); | ||
|
|
||
| test('accepts an explicit secret key at the floor', () => { | ||
| delete process.env.MPP_SECRET_KEY; | ||
| expect(() => Mppx.create({ methods: methods(), secretKey: strong })).not.toThrow(); | ||
| }); | ||
|
|
||
| test('rejects a short MPP_SECRET_KEY from the environment', () => { | ||
| process.env.MPP_SECRET_KEY = tooShort; | ||
| expect(() => Mppx.create({ methods: methods() })).toThrow(/at least 32 bytes/); | ||
| }); | ||
|
|
||
| test('accepts a strong MPP_SECRET_KEY from the environment', () => { | ||
| process.env.MPP_SECRET_KEY = strong; | ||
| expect(() => Mppx.create({ methods: methods() })).not.toThrow(); | ||
| }); |
There was a problem hiding this comment.
Two behaviours documented in the guard but not exercised by the suite: (1) when neither secretKey nor MPP_SECRET_KEY is set the guard must stay silent (currently only an inline comment guarantees this); (2) when both are set and they differ in strength, only secretKey is evaluated due to ?? short-circuiting — a short env var with a strong explicit key should be accepted, and a strong env var with a short explicit key should be rejected. Without these cases the guard's priority semantics are untested and a silent regression would be easy to miss.
2 participants
What
Follow-up to #169. The cross-language audit routed the
mppx-owned findings (#1/#24/#15/#9) to an upstream report rather than fixing them in-repo. This closes #24 (weak HMAC secret key) at pay-kit's own boundary as defense-in-depth, without forking or vendoringmppx.mppx@0.5.xaccepts any non-emptysecretKeyas the HMAC-SHA256 key that binds challenge IDs, so a weak key (e.g."key") lets an attacker forge challenges. Per NIST SP 800-107 the HMAC-SHA256 key should be at least the hash output size (32 bytes).How
server/secret-guard.tswrapsMppx.createand rejects a resolved secret shorter than 32 bytes, checking both the explicitsecretKeyand theMPP_SECRET_KEYenv path. When no secret is set at all it stays silent and defers to mppx's own error, so it owns only the strength check. This mirrors the existingchallenge-guard.tsboundary pattern and touches only the secret-strength gate, never signing or settlement.server/index.tsnow re-exports the guardedMppxsoimport { Mppx } from '@solana/mpp/server'gets the floor automatically.Notes
mppx(alongside feat: add session payments with on-chain Swig enforcement #1/fix: cascade changes from spec #15/chore: remove namespace types, zero eslint overrides #9); this is the pay-kit-side guard until that lands.Testing
pnpm typecheck,pnpm lint,pnpm format:checkcleanBased on the #169 branch; will retarget to main once #169 merges.