feat(auth): add POST /auth/challenge endpoint - #33
Conversation
Issue a time-limited, single-use challenge tied to a validated Stellar address, clean up expired AuthNonce rows, and keep the deterministic message format used by /auth/verify. Closes ShadeProtocol#6 Co-authored-by: Cursor <cursoragent@cursor.com>
📝 WalkthroughWalkthroughAdds ChangesStellar authentication challenge flow
Estimated code review effort: 3 (Moderate) | ~25 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant auth.routes
participant createChallengeController
participant createNonce
participant AuthNonce
Client->>auth.routes: POST /challenge with address
auth.routes->>createChallengeController: forward request
createChallengeController->>createNonce: validated address
createNonce->>AuthNonce: delete stale nonces and create challenge
AuthNonce-->>createNonce: message, nonce, expiresAt
createNonce-->>createChallengeController: challenge result
createChallengeController-->>Client: HTTP 200 response
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 passed)
✨ 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 |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/controllers/auth.controllers.ts`:
- Line 32: Remove the unused error binding from the catch clause in the relevant
auth controller method, while preserving the existing catch block behavior.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d427b60f-fa66-4009-9d90-8d66a28eb7bc
📒 Files selected for processing (5)
src/controllers/auth.controllers.tssrc/routes/auth.routes.tssrc/services/auth.services.tstests/integration/auth.routes.test.tstests/unit/auth.services.test.ts
|
GM @josephchimebuka |
Co-authored-by: Cursor <cursoragent@cursor.com>
codebestia
left a comment
codebestia
left a comment
There was a problem hiding this comment.
Hello @josephchimebuka
Good job so far.
From my personal review, here are some changes i recommend to be added.
- Add error logging in createChallengeController's catch block and/or propagate the error to your app logger (include request context except secrets). This helps diagnose failures in staging/production. For now, use a mock or console.error till a better app logger infrastructure is provided.
- Confirm & enforce a DB unique constraint on active nonces for an address and implement createNonce to either upsert or use a serialized transaction to avoid race conditions when two requests run in parallel.
- Fix verification to use the stored message (authNonce.message) or otherwise guarantee createdAt parity. (Critical)
- Wrap deleteMany + create in a transaction to prevent duplicate active nonces or unexpected behavior. (High)
- Update/Add tests for concurrency and full sign/verify E2E flow
Wrap nonce cleanup and creation in a transaction, enforce one active nonce per address via a partial unique index, verify signatures against the stored challenge message, add error logging in the challenge controller, and expand concurrency plus sign/verify E2E tests. Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (3)
prisma/schema.prisma (1)
60-61: 🚀 Performance & Scalability | 🔵 Trivial | ⚡ Quick winIndex the expiry cleanup predicate.
Each challenge deletes expired rows globally, but
@@index([address])cannot supportexpiresAt < now. Add@@index([expiresAt])and its migration to prevent this cleanup from becoming a table scan as nonce traffic grows.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@prisma/schema.prisma` around lines 60 - 61, Add an index on the challenge model’s expiresAt field alongside the existing address index, and create the corresponding database migration. Ensure the expiry cleanup query using expiresAt < now can use this index rather than scanning the table.tests/unit/auth.sign-verify.e2e.test.ts (2)
35-44: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDuplicate
authNonceRecordmock construction.Both tests build near-identical stored-nonce mock objects differing only in nonce/message/expiresAt. Consider a small factory to reduce duplication.
♻️ Example helper
+function buildAuthNonceRecord(overrides: Partial<Record<string, unknown>> = {}) { + return { + id: 'uuid-1', + address, + usedAt: null, + createdAt: mockDate, + merchantId: null, + ...overrides, + }; +}Also applies to: 80-89
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/auth.sign-verify.e2e.test.ts` around lines 35 - 44, Introduce a small factory helper for constructing authNonceRecord mocks and use it in both test cases. Keep shared fields—such as id, address, usedAt, createdAt, and merchantId—centralized in the helper while allowing nonce, message, and expiresAt to vary per test.
31-107: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd a negative-path signature test.
This file only exercises the happy path (valid signature against the stored message). Since it's scoped to "stored challenge verification," a tampered-signature or mismatched-address case would give confidence
verifySignatureactually rejects bad signatures rather than always returningvalid: true.test('tampered signature fails verification against the stored message', async () => { const nonce = 'c'.repeat(64); const message = buildChallengeMessage(address, nonce, mockDate); const badSignature = keypair.sign(Buffer.from(message + 'x', 'utf-8')).toString('hex'); prismaMock.authNonce.findUnique.mockResolvedValue({ id: 'uuid-1', address, nonce, message, expiresAt: new Date('2026-06-21T12:05:00.000Z'), usedAt: null, createdAt: mockDate, merchantId: null, }); const result = await verifySignature(address, nonce, badSignature); expect(result.valid).toBe(false); });🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/unit/auth.sign-verify.e2e.test.ts` around lines 31 - 107, Add a negative-path test alongside the existing wallet-signed challenge test in the verifySignature suite. Mock authNonce.findUnique with the stored challenge, sign a tampered message or otherwise provide an invalid signature, call verifySignature with the matching address and nonce, and assert the result is invalid while preserving the existing happy-path test.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/services/auth.services.ts`:
- Around line 26-35: Handle concurrent same-address challenge issuance in the
authNonce transaction by adding per-address serialization or a defined
retry/conflict policy, so concurrent requests return the intended challenge
response instead of an unhandled P2002 failure. In
tests/unit/auth.services.test.ts lines 45-49, stop modeling the transaction mock
as serialized behavior; in lines 112-130, add a database-backed concurrent test
asserting the chosen outcome.
---
Nitpick comments:
In `@prisma/schema.prisma`:
- Around line 60-61: Add an index on the challenge model’s expiresAt field
alongside the existing address index, and create the corresponding database
migration. Ensure the expiry cleanup query using expiresAt < now can use this
index rather than scanning the table.
In `@tests/unit/auth.sign-verify.e2e.test.ts`:
- Around line 35-44: Introduce a small factory helper for constructing
authNonceRecord mocks and use it in both test cases. Keep shared fields—such as
id, address, usedAt, createdAt, and merchantId—centralized in the helper while
allowing nonce, message, and expiresAt to vary per test.
- Around line 31-107: Add a negative-path test alongside the existing
wallet-signed challenge test in the verifySignature suite. Mock
authNonce.findUnique with the stored challenge, sign a tampered message or
otherwise provide an invalid signature, call verifySignature with the matching
address and nonce, and assert the result is invalid while preserving the
existing happy-path test.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: f8fd2942-15c7-4783-9e5c-951ff209791e
📒 Files selected for processing (7)
prisma/migrations/20260729100000_auth_nonce_active_address_unique/migration.sqlprisma/schema.prismasrc/controllers/auth.controllers.tssrc/services/auth.services.tstests/integration/auth.routes.test.tstests/unit/auth.services.test.tstests/unit/auth.sign-verify.e2e.test.ts
Acquire a per-address Postgres advisory lock inside the nonce transaction, return an existing active challenge on P2002 instead of 500, and add concurrency simulation plus conflict-policy tests. Co-authored-by: Cursor <cursoragent@cursor.com>
Co-authored-by: Cursor <cursoragent@cursor.com>
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
src/services/auth.services.ts (1)
34-53: 🗄️ Data Integrity & Integration | 🟠 Major | 🏗️ Heavy liftConfirm concurrent same-address challenge semantics
createNonceInTransactionserializes challenge creation per address and deletes any existing active unused nonce before inserting a new one, whileverifySignaturerejects invalid or replaced nonces. This makes last-issued-nonce semantics explicit, but a later challenge call can render previously returned nonce(s) invalid before/auth/verifyis reached. If that behavior is not intended, keep the per-address lock but preserve the earlier active nonce unless the caller explicitly requests a refresh/replay.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@src/services/auth.services.ts` around lines 34 - 53, The createNonceInTransaction flow currently deletes every active unused nonce for the address, enforcing last-issued-nonce behavior. Confirm the intended concurrent challenge semantics; if earlier challenges must remain valid, retain the per-address advisory lock but remove the address-and-unused deletion condition and only delete expired nonces, unless an explicit refresh or replay request is provided.
🧹 Nitpick comments (1)
tests/integration/auth.challenge.concurrency.test.ts (1)
113-129: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winStrengthen the assertion to make the last-write-wins outcome explicit.
This test proves 5 parallel same-address requests each resolve successfully but only 1 nonce survives — it doesn't verify whether the 4 discarded nonces are the ones actually returned to earlier callers (i.e., that already-handed-out challenges become silently invalid). Asserting
resultsagainststore(e.g., only the last-settled result matches the survivingactiveNonces[0]) would make this intentional behavior explicit and guard against accidental regressions/refinements.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/integration/auth.challenge.concurrency.test.ts` around lines 113 - 129, The concurrency test should explicitly verify the last-write-wins behavior: after confirming one active nonce remains, assert that only the result from the last-settled request matches activeNonces[0], while the other four returned challenges do not match the surviving nonce. Use the existing results, activeNonces, and createNonce flow without changing the response-shape assertions.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Outside diff comments:
In `@src/services/auth.services.ts`:
- Around line 34-53: The createNonceInTransaction flow currently deletes every
active unused nonce for the address, enforcing last-issued-nonce behavior.
Confirm the intended concurrent challenge semantics; if earlier challenges must
remain valid, retain the per-address advisory lock but remove the
address-and-unused deletion condition and only delete expired nonces, unless an
explicit refresh or replay request is provided.
---
Nitpick comments:
In `@tests/integration/auth.challenge.concurrency.test.ts`:
- Around line 113-129: The concurrency test should explicitly verify the
last-write-wins behavior: after confirming one active nonce remains, assert that
only the result from the last-settled request matches activeNonces[0], while the
other four returned challenges do not match the surviving nonce. Use the
existing results, activeNonces, and createNonce flow without changing the
response-shape assertions.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: dc38500e-014b-4c00-b2dd-741f8dcdf957
📒 Files selected for processing (5)
src/services/auth.services.tstests/integration/auth.challenge.concurrency.test.tstests/integration/auth.routes.test.tstests/unit/auth.services.test.tstests/unit/auth.sign-verify.e2e.test.ts
🚧 Files skipped from review as they are similar to previous changes (2)
- tests/unit/auth.sign-verify.e2e.test.ts
- tests/integration/auth.routes.test.ts
codebestia
left a comment
codebestia
left a comment
There was a problem hiding this comment.
LGTM!
Thank you for your contribution.
2 participants
Summary
POST /api/v1/auth/challengethat validates a Stellar public key withStrKey.isValidEd25519PublicKey, issues a 5-minute single-use nonce, and returns{ message, nonce, expiresAt }.AuthNoncecleanup before each challenge, and the existing deterministicbuildChallengeMessageformat so/auth/verifystays compatible.Test plan
npm test -- --testPathPatterns='auth\.(services|routes)'(26 passing)POST /api/v1/auth/challengewith a validG...address returns200{ message, nonce, expiresAt }400{ error: "Invalid Stellar address" }messagematches what/auth/verifyreconstructs for signature checksCloses #6
Made with Cursor
Summary by CodeRabbit
POST /challengeendpoint that validates Stellar Ed25519 public addresses and returns a nonce challenge (message, nonce, expiration).