fix(playground): enforce keeper/deployer signer separation in keeper-cosign - #2428
fix(playground): enforce keeper/deployer signer separation in keeper-cosign#2428Bayyan16 wants to merge 1 commit into
Conversation
|
@Bayyan16 is attempting to deploy a commit to the Khubair Nasir's projects Team on Vercel. A member of the Team first needs to authorize it. |
|
Important Review skippedAuto reviews are disabled on base/target branches other than the default branch. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
|
Independent verification of the guard — not an approval (QA/Security own that), just evidence for whoever reviews. I've been finding a recurring failure mode in this codebase: tests that pass without exercising the thing they're named for (10 confirmed instances so far, including one of my own). So I checked whether this PR's test actually catches the vulnerability rather than just passing alongside it. Method: neutralised only the new guard on the PR branch — if (deployerPk.equals(keeperPk)) { → if (false) {Result: So the test genuinely binds to the fix. The companion case ( Two things I liked specifically:
No changes requested from me. |
|
Thank you @dcccrypto for independently validating the guard and its regression coverage. The mutation result confirms that the rejection test is non-vacuous, while the distinct-deployer case remains the intended compatibility control. I’ll keep the current head unchanged and leave formal approval and merge decisions to the appropriate reviewers. |
Summary
Fixes #2427.
This PR enforces signer identity separation in the playground keeper co-signing flow.
The
POST /api/playground/keeper-cosignroute is designed to return a transaction that is partially signed by the server-managed keeper while still requiring an independent creator/deployer wallet signature before submission.Previously, the route accepted a requester-controlled
deployerpublic key without verifying that it was distinct from the server keeper public key.When both logical signer roles used the same public key:
Solana transaction compilation deduplicated the signer accounts into a single required signer identity. As a result, the server keeper signature could satisfy both the creator/deployer and keeper signer roles, causing the transaction returned by the route to become fully signed without an independent creator signature.
This PR restores the intended authorization boundary by rejecting keeper/deployer signer aliasing before any RPC access, transaction construction, or server-side signing occurs.
Root Cause
The keeper co-sign route uses two logically independent authorization roles:
The route previously derived both public keys independently:
However, it did not enforce the required identity invariant:
The transaction was then signed by the server keeper:
For distinct public keys, this correctly produced:
For aliased public keys, the transaction signer set was deduplicated:
The issue was therefore caused by missing application-level signer-role separation rather than a failure in Solana signature verification.
Changes
1. Reject keeper/deployer signer aliasing
The route now rejects requests where the requester-controlled deployer public key equals the server-managed keeper public key:
The validation is intentionally placed immediately after deriving
keeperPk.This ensures the request is rejected before:
This ordering prevents invalid aliased requests from consuming RPC resources or reaching the server signing boundary.
2. Add deterministic signer-separation regression coverage
A new route-level regression suite was added:
The suite uses the real Solana transaction implementation while mocking only the external RPC metadata operations.
It includes both a negative security control and a positive compatibility control.
Regression Coverage
Case 1 — Reject aliased signer identities
Input condition:
Expected behavior:
Expected response:
{ "error": "deployer must be distinct from keeper" }The test additionally verifies that rejection occurs before any downstream operation:
This confirms that an invalid aliased request cannot reach RPC metadata retrieval, transaction construction, or server-side signing.
Case 2 — Preserve the normal distinct-deployer flow
Input condition:
Expected behavior:
The test verifies that the legitimate keeper co-signing flow remains unchanged:
The returned transaction is deserialized and its signer state is inspected.
Expected signer state:
This confirms that legitimate requests still receive a partially signed transaction and still require an independent creator/deployer wallet signature.
Security Invariant
This PR explicitly enforces the following invariant:
After this change, the server keeper cannot satisfy both logical authorization roles within the same transaction.
The intended signing model remains:
Behavior Before This PR
For an aliased request:
the route continued through:
Because the signer identities were identical, the returned transaction could verify as fully signed without an independent creator signature.
Behavior After This PR
For an aliased request:
the route now returns:
with:
{ "error": "deployer must be distinct from keeper" }No RPC request, transaction construction, or signing operation is performed.
For a legitimate request:
the existing partial-signing behavior remains unchanged.
Files Changed
The production change is intentionally minimal and localized to the affected route.
This PR does not modify:
Validation
Dedicated signer-separation regression suite
Command:
pnpm --dir app exec vitest run \ __tests__/api/keeper-cosign-signer-separation.test.ts \ --reporter=verboseResult:
Combined keeper security baseline
Command:
pnpm --dir app exec vitest run \ __tests__/api/gh1692-timing-safe-keeper-auth.test.ts \ __tests__/api/keeper-cosign-signer-separation.test.ts \ --reporter=verboseResult:
This confirms that the new signer-separation validation does not regress the existing keeper registration authentication controls.
TypeScript validation
Command:
pnpm --dir app exec tsc --noEmitResult:
No TypeScript errors were produced.
Production build
Command:
Result:
The production Next.js build completed successfully.
Observed Next.js deprecation and optional native binding warnings are pre-existing and do not affect the build result.
Patch integrity
The following checks completed successfully:
Result:
Compatibility and Regression Risk
Regression risk is low because the production behavior change is limited to one invalid identity relationship:
All requests using distinct creator and keeper public keys continue through the existing transaction-building and partial-signing path.
The positive regression test confirms that the normal route response still contains:
Security Considerations
This PR directly fixes the confirmed signer-role aliasing condition.
Additional defense-in-depth improvements may be handled separately, including:
Those measures are complementary hardening controls and are not required for the narrow signer-separation fix implemented here.
Review Notes
Reviewers should verify that:
Checklist