[brightpixel-dev]

Closes #3

What changed

• Wallet auth: POST /auth/nonce + POST /auth/verify with Ed25519 signature verification; Redis single-use nonces (TTL 300 s, atomic getDel); HMAC-SHA256 JWT issued on success
• Org role enforcement: requireOrgMember / requireOrgAdmin middlewares; role validated from DB on every protected request
• Input validation: Stellar address regex (/^G[A-Z2-7]{55}$/), cursor/page pagination bounded to 1–100, bodyFields allowlist blocks unknown keys, express.json({ limit: '64kb' })
• Request tracing: x-request-id header propagated end-to-end; structured JSON logs via logger.ts; all error responses include requestId
• Standardised errors: notFound + errorHandler middlewares; every route uses next(error) instead of inline 500s
• Bounded pagination: parsePagination (cursor) and parsePagePagination (offset) enforce limit ≤ 100
• Rate limiting: Nginx limit_req config already in docker-compose; trust proxy set in Express so X-Forwarded-For is trusted
• Timeouts: Redis connectTimeout + commandTimeout; Prisma query timeout via dbQueryTimeoutMs
• OpenAPI 3.1.0 spec: openapi.yaml covering all public + authenticated routes with request/response schemas
• CI pipeline: GitHub Actions — Redis service container, tsc --noEmit → jest --runInBand → tsc -p tsconfig.json → npm audit --audit-level=high
• 22 tests: JWT sign/verify/tamper/expire, pagination validation, Stellar address validation, requireAuth middleware, nonce flow, rate-limit header propagation

Why

Full security hardening as requested in issue #3: replay-protected auth, org-scoped RBAC, input sanitisation, observability, and automated quality gates.

How to test

npm install
npm test          # 22 tests
npx tsc --noEmit  # 0 errors

Or with the full stack:

docker compose up -d
curl -X POST http://localhost:3000/auth/nonce \
  -H 'Content-Type: application/json' \
  -d '{"walletAddress":"GABC..."}'

[diiabblo]

Review: API hardening

Thanks for this -- the auth flow (nonce + Ed25519 verify + JWT), request tracing, error handling, and CI pipeline are well-structured. Before merging, a few issues need attention:

Critical

verifyWalletSignature public key extraction is off by one byte (src/middleware/auth.ts:70)
The raw Ed25519 key is extracted with bytes.slice(2, 34) but Stellar encoded keys place the 32-byte raw key at bytes[1] through bytes[32]. The version byte is at [0], checksum at [33..34]. Using slice(2, 34) skips the first byte of the actual key and includes a checksum byte instead. The correct extraction is bytes.slice(1, 33).

I verified this with a real Stellar address -- the two slices produce different hex output, meaning the derived public key is wrong and signature verification fails for all real keys. This code path is also not covered by the test suite; the auth tests mock Redis but never exercise verifyWalletSignature with a real signature.

High

jwtSecret defaults to "change-me-in-production" (src/config.ts:23)
If JWT_SECRET is not set, anyone who reads this source can forge valid JWT tokens. The app should refuse to start in production when JWT_SECRET is unset, rather than falling through to a hardcoded default.

vesting.ts conflicts with PR #6 (src/routes/vesting.ts:49-50)
This changes totalAmount and vestedAmount to use Number(schedule.amount), but PR #6 migrates those columns from Float to Prisma.Decimal. Number() on a Decimal object loses precision -- exactly the bug PR #6 fixes. Both PRs modify the same lines. Coordinate the merge so the final version uses Decimal.mul() and toString() serialization from PR #6, while keeping the totalDuration > 0 guard and next(error) pattern from this PR.

Medium

CI Node version is 20 but other PRs need 22 (.github/workflows/ci.yml:26)
PR #5 adds @stellar/stellar-sdk which requires Node >=22. Since this PR adds the CI pipeline that will gate all other PRs, bump node-version to "22" now.

Coverage threshold gates this PR (jest.config.js:13)
coverageThreshold: { global: 60% } with tests covering only ~4 of ~18 source files means CI will fail on this PR. Either lower the threshold or add tests for the uncovered modules before merging.

Minor

globals config is deprecated (jest.config.js:6)
ts-jest 29 emits a deprecation warning for globals config. Tests still pass (I verified jest@30 + ts-jest@29 works), but migrating to the transform config format would eliminate the warning:

transform: {
  "^.+\\.tsx?$": ["ts-jest", { tsconfig: "./tsconfig.test.json" }]
}

[brightpixel-dev]

Thanks for the thorough review @diiabblo — all points addressed in the latest commit:

Critical — key extraction off-by-one: Fixed. bytes.slice(2, 34) → bytes.slice(1, 33). The version byte is at bytes[0] (0x30), the 32-byte raw Ed25519 key is bytes[1..32], and the checksum is bytes[33..34]. Also corrected the inline comment to reflect the actual byte layout.

High — hardcoded JWT_SECRET fallback: Fixed. The config now throws Error('JWT_SECRET env var must be set in production') at startup if NODE_ENV=production and JWT_SECRET is unset. Non-production envs still fall back to the dev placeholder so tests and local dev aren't disrupted.

High — vesting.ts Decimal precision: Fixed. Imported Prisma from @prisma/client and replaced the Number(schedule.amount) * progress arithmetic with new Prisma.Decimal(schedule.amount).mul(new Prisma.Decimal(progress)). This keeps the response shape the same (toNumber() for JSON) but uses Decimal for the multiplication, so it's ready for when PR #6's Float→Decimal migration lands with no further changes needed here.

Medium — Node 20 → 22 in CI: Fixed in .github/workflows/ci.yml.

Medium — coverage threshold: Lowered to 30% to match the current test scope. The threshold will be raised as coverage grows.

Minor — ts-jest globals deprecation: Migrated to the transform config format.

[diiabblo]

Re-review: all fixes verified

All issues from the previous review are addressed:

• verifyWalletSignature: bytes.slice(2,34) -> bytes.slice(1,33) -- correct Ed25519 key extraction
• jwtSecret: now throws in production via lazy IIFE when JWT_SECRET is unset
• vesting.ts: uses new Prisma.Decimal() for arithmetic with stable @prisma/client import -- compatible with PR #6 Decimal migration
• CI: node-version: "22" -- supports @stellar/stellar-sdk requirement
• jest.config.js: migrated from deprecated globals to transform format, coverage threshold lowered to 30%
• .env.example: all new config vars documented

TypeScript compiles clean, tests pass. This is the right PR to merge first -- the CI pipeline it adds will gate all subsequent PRs with automated checks.

Merge note

PR #6 (Decimal migration) also touches src/routes/vesting.ts. Since this PR now uses Prisma.Decimal arithmetic, the merge should resolve cleanly -- #6 just needs to change .toNumber() to .toString() in the response serialization to match.