Skip to content

fix(playground): enforce keeper/deployer signer separation in keeper-cosign - #2428

Open
Bayyan16 wants to merge 1 commit into
dcccrypto:playgroundfrom
Bayyan16:fix/keeper-cosign-signer-separation
Open

fix(playground): enforce keeper/deployer signer separation in keeper-cosign#2428
Bayyan16 wants to merge 1 commit into
dcccrypto:playgroundfrom
Bayyan16:fix/keeper-cosign-signer-separation

Conversation

@Bayyan16

Copy link
Copy Markdown
Contributor

Summary

Fixes #2427.

This PR enforces signer identity separation in the playground keeper co-signing flow.

The POST /api/playground/keeper-cosign route 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 deployer public key without verifying that it was distinct from the server keeper public key.

When both logical signer roles used the same public key:

deployerPk == keeperPk

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:

Creator/deployer:
- ConfigureAuthMark oracle authority
- UpdateAssetAuthority current authority
- Transaction fee payer

Server keeper:
- UpdateAssetAuthority new authority
- Server-side partial signer

The route previously derived both public keys independently:

const deployerPk = new PublicKey(deployer);
const keeperPk = new PublicKey(keeper.publicKey());

However, it did not enforce the required identity invariant:

deployerPk != keeperPk

The transaction was then signed by the server keeper:

keeper.partialSign(tx);

For distinct public keys, this correctly produced:

deployer signature: missing
keeper signature:   present
fully signed:       false

For aliased public keys, the transaction signer set was deduplicated:

logical signer roles: 2
unique signer keys:   1
keeper signature:     present
fully signed:         true

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:

if (deployerPk.equals(keeperPk)) {
  return NextResponse.json(
    {
      error: "deployer must be distinct from keeper",
    },
    { status: 400 },
  );
}

The validation is intentionally placed immediately after deriving keeperPk.

This ensures the request is rejected before:

- RPC slot retrieval
- recent blockhash retrieval
- instruction construction
- transaction construction
- keeper.partialSign()
- transaction serialization

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:

app/__tests__/api/keeper-cosign-signer-separation.test.ts

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:

deployer == keeperPubkey

Expected behavior:

HTTP 400

Expected response:

{
  "error": "deployer must be distinct from keeper"
}

The test additionally verifies that rejection occurs before any downstream operation:

Connection.prototype.getSlot:
  not called

Connection.prototype.getLatestBlockhash:
  not called

keeper.partialSign:
  not called

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:

deployer != keeperPubkey

Expected behavior:

HTTP 200

The test verifies that the legitimate keeper co-signing flow remains unchanged:

getSlot:
  called exactly once

getLatestBlockhash:
  called exactly once

keeper.partialSign:
  called exactly once

partialTxBase64:
  returned

keeperPubkey:
  matches the configured keeper

nowSlot:
  matches the mocked slot

The returned transaction is deserialized and its signer state is inspected.

Expected signer state:

deployer signer slot:
  present

deployer signature:
  null

keeper signer slot:
  present

keeper signature:
  present

Transaction.verifySignatures():
  false

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:

creator/deployer signer identity != server keeper signer identity

After this change, the server keeper cannot satisfy both logical authorization roles within the same transaction.

The intended signing model remains:

server keeper signature
        +
independent creator signature
        =
fully authorized transaction

Behavior Before This PR

For an aliased request:

deployer == keeperPubkey

the route continued through:

1. RPC slot retrieval
2. blockhash retrieval
3. ConfigureAuthMark construction
4. UpdateAssetAuthority construction
5. keeper.partialSign()
6. transaction serialization

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:

deployer == keeperPubkey

the route now returns:

HTTP 400

with:

{
  "error": "deployer must be distinct from keeper"
}

No RPC request, transaction construction, or signing operation is performed.

For a legitimate request:

deployer != keeperPubkey

the existing partial-signing behavior remains unchanged.


Files Changed

app/app/api/playground/keeper-cosign/route.ts
app/__tests__/api/keeper-cosign-signer-separation.test.ts

The production change is intentionally minimal and localized to the affected route.

This PR does not modify:

- instruction encoding
- account-meta definitions
- keeper key loading
- transaction serialization format
- client-side transaction signing
- normal create-market behavior
- keeper registration authentication

Validation

Dedicated signer-separation regression suite

Command:

pnpm --dir app exec vitest run \
  __tests__/api/keeper-cosign-signer-separation.test.ts \
  --reporter=verbose

Result:

Test Files  1 passed
Tests       2 passed

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=verbose

Result:

Test Files  2 passed
Tests       9 passed

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 --noEmit

Result:

Exit code: 0

No TypeScript errors were produced.


Production build

Command:

pnpm --dir app run build

Result:

Final build exit code: 0

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:

git diff --check
git diff --cached --check
git diff --check upstream/playground...HEAD

Result:

No whitespace errors
No malformed patch output
No unrelated source changes

Compatibility and Regression Risk

Regression risk is low because the production behavior change is limited to one invalid identity relationship:

deployerPk.equals(keeperPk)

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:

- a valid keeper signature;
- an absent creator signature;
- a partially signed transaction;
- the expected keeper public key;
- the expected slot metadata.

Security Considerations

This PR directly fixes the confirmed signer-role aliasing condition.

Additional defense-in-depth improvements may be handled separately, including:

- cryptographic deployer wallet proof;
- binding the request to the actual slab creator/admin;
- validating slab ownership by the expected program;
- signed request payloads containing a nonce and expiration;
- replay protection;
- request-level rate limiting and security logging.

Those measures are complementary hardening controls and are not required for the narrow signer-separation fix implemented here.


Review Notes

Reviewers should verify that:

- the equality guard executes before RPC access;
- the equality guard executes before keeper.partialSign();
- the aliased request returns HTTP 400;
- no signer or RPC operation occurs after rejection;
- the distinct-deployer path still returns a partially signed transaction;
- the creator signature remains absent in the normal flow;
- the keeper signature remains present in the normal flow.

Checklist

  • Fixes [Security] Keeper-cosign signer-role aliasing bypasses the independent creator signature boundary #2427
  • Enforces keeper/deployer signer identity separation
  • Rejects the vulnerable condition before RPC access
  • Rejects the vulnerable condition before server-side signing
  • Adds negative security regression coverage
  • Adds positive compatibility coverage
  • Dedicated regression suite passes
  • Combined keeper security baseline passes
  • TypeScript validation passes
  • Production build passes
  • Patch contains only the affected route and regression test

@Bayyan16
Bayyan16 requested a review from dcccrypto as a code owner July 15, 2026 16:53
@vercel

vercel Bot commented Jul 15, 2026

Copy link
Copy Markdown

@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.

@coderabbitai

coderabbitai Bot commented Jul 15, 2026

Copy link
Copy Markdown

Important

Review skipped

Auto reviews are disabled on base/target branches other than the default branch.

Please check the settings in the CodeRabbit UI or the .coderabbit.yaml file in this repository. To trigger a single review, invoke the @coderabbitai review command.

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro

Run ID: babb8f60-b308-4935-9eed-08dda73d8a72

You can disable this status message by setting the reviews.review_status to false in the CodeRabbit configuration file.

Use the checkbox below for a quick retry:

  • 🔍 Trigger review
✨ Finishing Touches
🧪 Generate unit tests (beta)
  • Create PR with unit tests

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.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@dcccrypto

Copy link
Copy Markdown
Owner

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:

guard present → 2 passed
guard removed → 1 failed  ("rejects deployer equal to the server keeper before RPC or signing")

So the test genuinely binds to the fix. The companion case (preserves the normal partial-signing flow for a distinct deployer) correctly stays green under the mutation — it's the over-correction guard, which is exactly right to have.

Two things I liked specifically:

  • the guard runs before the RPC and signing work, so an aliased request can't burn a keeper signature or an RPC round-trip on its way to being rejected
  • the error message doesn't echo the keeper pubkey back to the requester

No changes requested from me.

@Bayyan16

Copy link
Copy Markdown
Contributor Author

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.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants