Example Report

# PR Review: `feature/payment-api` → `main`

**PR:** #87 — feature/payment-api → main
**Date:** 2026-04-08
**Provider:** claude (claude-sonnet-4-6)
**Files reviewed:** 6 files, 318 insertions, 44 deletions

---

## 📋 Summary

This PR adds a new payment processing API endpoint that allows users to submit credit card charges
through a third-party payment gateway (Stripe). It introduces a new route `/api/payments/charge`,
a PaymentService class, and database models for storing transaction records. The overall structure
is well-organised, but there are two critical security issues that must be resolved before this
feature goes live — both involve sensitive user data that could be exposed if left unaddressed.
Test coverage is minimal and needs to be expanded before this is safe to deploy.

---

## 🗂️ At a Glance

| # | Category | Severity | What's the problem? | Must fix before release? |
|---|----------|----------|---------------------|--------------------------|
| 1 | 🚨 Critical | 🔴 High | Full card numbers are being logged to the application log | Yes |
| 2 | 🚨 Critical | 🔴 High | Payment amount is not validated — negative charges are possible | Yes |
| 3 | 🔒 Security | 🔴 High | Stripe secret key may be exposed if the environment variable is missing | Yes |
| 4 | 🔒 Security | 🟡 Medium | No rate limiting on the charge endpoint — abuse risk | Yes |
| 5 | ⚡ Performance | 🟡 Medium | Database transaction records are fetched without pagination | No |
| 6 | 🛠️ Quality | 🟢 Low | PaymentService constructor is doing too many things at once | No |
| 7 | 🧪 Testing | 🔴 High | No tests for failed payment scenarios or invalid input | Yes |
| 8 | 🧪 Testing | 🟡 Medium | No test for the idempotency key logic | No |

---

## 🚨 Critical Issues

> These problems **must be fixed before this change goes live**. They could cause data loss, broken features, or incorrect behaviour for users.

| Severity | What's the problem? | Real-world impact on users | File / Location | What the developer should do |
|----------|---------------------|----------------------------|-----------------|------------------------------|
| 🔴 Critical | Full credit card numbers are being written to the application log inside the charge handler | Card numbers become visible in log dashboards, alerting tools, and to anyone with log access — a direct PCI-DSS violation | `src/api/payments/charge.js:34` | Remove the `console.log(cardDetails)` line entirely. Never log raw card data. Use `cardDetails.last4` only if you need a log entry. |
| 🔴 Critical | The payment `amount` field is accepted from the request body without any validation | A malicious user could send a negative amount (e.g. `-500`) which may result in money being refunded to an attacker rather than charged | `src/services/PaymentService.js:58` | Add a validation check: `if (amount <= 0 || !Number.isFinite(amount)) throw new ValidationError(...)`. Use a schema validator like `zod` or `joi` for the whole request body. |

---

## 🔒 Security

> Security issues can expose user data, allow unauthorised access, or leave the system open to attacks.

| Severity | What's the risk? | Who is affected? | File / Location | Recommended fix |
|----------|-----------------|-----------------|-----------------|-----------------|
| 🔴 High | If the `STRIPE_SECRET_KEY` environment variable is not set, the code falls back to an empty string (`""`), meaning Stripe calls will fail silently or use no authentication | Any user trying to make a payment — charges may silently fail or, worse, go through with no real authentication | `src/config/stripe.js:12` | Throw a startup error if the key is missing: `if (!process.env.STRIPE_SECRET_KEY) throw new Error("STRIPE_SECRET_KEY is required")` |
| 🟡 Medium | The `/api/payments/charge` endpoint has no rate limiting. An attacker could send thousands of requests per minute to probe card numbers or exhaust your Stripe API quota | All users — could cause service disruption or unexpected Stripe billing | `src/api/payments/charge.js` | Add rate limiting middleware (e.g. `express-rate-limit`) capped at ~10 requests per minute per IP for this route. |

---

## ⚡ Performance

> Performance problems can make the app feel slow, increase infrastructure costs, or cause outages under heavy usage.

| Severity | What's the problem? | Impact on users / system | File / Location | Recommended fix |
|----------|---------------------|--------------------------|-----------------|-----------------|
| 🟡 Medium | The `getTransactionHistory()` method fetches all transaction records for a user from the database in one query, with no `LIMIT` or pagination | A user with hundreds of transactions could cause the API to time out or return several MB of JSON, slowing down their dashboard | `src/services/PaymentService.js:102` | Add `LIMIT` and `OFFSET` (or cursor-based pagination) to the query. Expose `page` and `pageSize` query parameters on the endpoint. |

---

## 🛠️ Code Quality & Improvements

> Not emergencies, but fixing these makes the codebase easier to maintain and reduces the chance of future bugs.

| Area | What's the issue? | Why it matters | File / Location | Recommended improvement |
|------|------------------|----------------|-----------------|-------------------------|
| Separation of concerns | `PaymentService` constructor connects to the database, initialises Stripe, and sets up a logger all at once | Makes the class hard to test in isolation and difficult to reuse — if any one dependency changes, the whole constructor breaks | `src/services/PaymentService.js:10–28` | Inject dependencies (db, stripeClient, logger) via constructor parameters instead of instantiating them inside the class. |
| Error handling | Generic `catch (e) { res.status(500).send("Error") }` gives users no useful feedback and swallows error details | Customers will see a confusing blank error; developers won't be able to debug payment failures | `src/api/payments/charge.js:61` | Map known error types (e.g. `StripeCardError`, `ValidationError`) to appropriate HTTP status codes and user-facing messages. |
| Magic numbers | The retry count `3` and timeout `5000` are hardcoded inline | If these need to change, a developer has to hunt through the file — easy to miss one | `src/services/PaymentService.js:77,83` | Extract to named constants at the top of the file: `const MAX_RETRIES = 3` and `const REQUEST_TIMEOUT_MS = 5000`. |

---

## 🧪 Test Coverage

> Tests are automated checks that verify the app works correctly. Gaps here mean bugs could slip through unnoticed.

| What's not being tested? | Why it matters | Possible consequence if it breaks | Suggested test to add |
|--------------------------|----------------|-----------------------------------|-----------------------|
| Failed payment scenarios (declined card, insufficient funds, network timeout) | Payments fail regularly in production — the app must handle these gracefully | A failed charge could silently appear as successful to the user, leading to undelivered goods or services | Unit test `PaymentService.charge()` with mocked Stripe errors: `StripeCardError`, `StripeConnectionError`, timeout |
| Negative or zero `amount` values | The current critical bug — no validation exists | Attacker could trigger a reverse charge or cause Stripe to throw an unhandled exception | Test that `charge({ amount: -100 })` throws a `ValidationError` before ever reaching Stripe |
| Idempotency key uniqueness | If the same key is reused, Stripe returns the original charge — the code must handle this | Duplicate charges or silent failures on retried requests | Test that retried requests with the same idempotency key return the cached result and do not create a second charge |
| Auth middleware on the charge route | The endpoint must only be accessible to authenticated users | An unauthenticated user could submit charges against other accounts | Integration test: assert that `POST /api/payments/charge` without a valid session returns `401 Unauthorized` |

---

## 💡 Suggestions & Nice-to-Haves

> Optional improvements — not required for this PR, but worth considering in future work.

| # | Suggestion | Benefit | Effort |
|---|-----------|---------|--------|
| 1 | Add a `POST /api/payments/refund` endpoint alongside the charge endpoint | Users and support staff can issue refunds without needing direct Stripe dashboard access | 🟡 Medium |
| 2 | Store a `payment_method_id` instead of re-sending card details on every request (use Stripe's Payment Methods API) | Improves security and enables one-click payments for returning customers | 🔴 High |
| 3 | Add a webhook handler for `payment_intent.succeeded` and `payment_intent.payment_failed` events from Stripe | Makes payment state reliable even when the user closes the browser mid-flow | 🟡 Medium |
| 4 | Consider using `decimal.js` or storing amounts in integer cents to avoid floating-point rounding errors | Prevents subtle billing bugs (e.g. `$10.10` being stored as `$10.099999…`) | 🟢 Low |
| 5 | Add OpenAPI / Swagger annotations to the new endpoint | Makes the API self-documenting for frontend teams and QA | 🟢 Low |

**Key takeaways:**
- Storing amounts as integer cents (pence/cents) is a widely adopted standard in payment systems and avoids entire classes of rounding bugs.
- Stripe Payment Methods API is the modern, PCI-compliant approach — re-sending raw card data on every request is against best practices and increases PCI scope.
- A webhook handler is essential for reliable payment state in production; without it, network failures between your server and Stripe create ghost transactions.

---

## ✅ Final Verdict

**REQUEST CHANGES**

This PR introduces valuable payment functionality but has two critical security issues that make it
unsafe to release as-is: credit card numbers are being logged, and payment amounts are unvalidated.
Both must be fixed before merging. Once those are resolved and test coverage for failure scenarios
is added, this will be in good shape to ship.

**Blocking items from Glance before merge:**
- Fix #1 — Remove credit card logging (row 1 in At a Glance)
- Fix #2 — Add amount validation to prevent negative charges (row 2)
- Fix #3 — Throw on missing `STRIPE_SECRET_KEY` at startup (row 3)
- Fix #7 — Add tests for failed payment scenarios and invalid input (row 7)