diff --git a/docs/superpowers/specs/2026-05-04-security-hardening-design.md b/docs/superpowers/specs/2026-05-04-security-hardening-design.md new file mode 100644 index 0000000..b2f3f04 --- /dev/null +++ b/docs/superpowers/specs/2026-05-04-security-hardening-design.md @@ -0,0 +1,166 @@ +# Security Hardening Design — XpenselyServer +**Date:** 2026-05-04 +**Scope:** Input validation, authorization enforcement, rate limiting + +--- + +## Problem Statement + +The XpenselyServer has three security gaps: + +1. **No input validation** — request bodies are accepted without any field constraints, allowing null fields, negative amounts, oversized strings, and malformed data to reach the database layer. +2. **Authorization bypass** — every endpoint trusts caller-supplied user IDs from query params or request bodies rather than the authenticated JWT. Any authenticated user can read or destroy another user's expense lists. +3. **No rate limiting** — no protection against brute-force or abuse of sensitive endpoints (invite generation, account creation, invite acceptance). + +--- + +## Section 1 — Input Validation + +### Dependency +Add `spring-boot-starter-validation` to `pom.xml`. + +### Request Model Constraints + +**`AppUserCreateRequest`** +- `username`: `@NotBlank @Size(min=3, max=30) @Pattern(regexp="^[a-zA-Z0-9_.-]+$")` +- `googleId`: `@NotBlank` + +**`ExpenseInput`** +- `title`: `@NotBlank @Size(max=100)` +- `owner`: `@NotBlank` +- `amount`: `@NotNull @DecimalMin("0.01")` +- `date`: `@NotNull` +- `category`: `@NotBlank` + +**`ExpenseChangeRequest`** — same constraints as `ExpenseInput` for the corresponding fields. + +**`InviteRequest`** +- `inviteCode`: `@NotBlank @Size(min=6, max=6)` +- `userId` field removed (derived from JWT — see Section 2) + +### Controller Changes +Add `@Valid` to every `@RequestBody` parameter in `AppUserController` and `ExpenseListController`. + +### Error Handling +Add a `@ControllerAdvice` class `GlobalExceptionHandler` that: +- Catches `MethodArgumentNotValidException` → returns `400 Bad Request` with a map of `{ field: errorMessage }` pairs +- Catches `IllegalArgumentException` → returns `400 Bad Request` with the exception message + +This replaces the current pattern of returning `500 INTERNAL_SERVER_ERROR` or `417 EXPECTATION_FAILED` for validation failures. + +### Cleanup +Remove the stray `@Id` and `@GeneratedValue` JPA annotations from `ExpenseInput` — it is a DTO, not an entity. + +--- + +## Section 2 — Authorization Model + +### Core Principle +Stop trusting caller-supplied user IDs. Derive the authenticated user from the JWT on every request. + +### New Component: `AuthenticatedUserResolver` +A `@Component` with a single method: +```java +AppUser resolveCurrentUser(Authentication auth) +``` +- Extracts the `sub` claim (Google ID) from the JWT +- Calls `UserService.getUserByGoogleId(sub)` to return the `AppUser` +- Throws `ResponseStatusException(403)` if no user is found for the JWT subject + +### Endpoint Changes + +| Endpoint | Change | +|---|---| +| `GET /api/expenselist/all` | **Removed** — no legitimate non-admin use case | +| `GET /api/expenselist/byUser?userId=X` | **Replaced** by `GET /api/expenselist/mine` — returns lists for the JWT user, no param | +| `GET /api/expenselist/byUsername?username=X` | **Removed** — redundant with `/mine` | +| `GET /api/expenselist/byId?id=X` | **Guard added** — 403 if authenticated user is neither owner nor sharedWith | +| `DELETE /api/expenselist/{id}` | **Guard added** — 403 if authenticated user is not the owner | +| `POST /api/expenselist/{id}/add` | **Guard added** — 403 if authenticated user is not owner or sharedWith | +| `PUT /api/expenselist/{id}/update` | **Guard added** — 403 if authenticated user is not owner or sharedWith | +| `DELETE /api/expenselist/{id}/delete` | **Guard added** — 403 if authenticated user is not owner or sharedWith | +| `POST /api/expenselist/{listId}/invite` | **Guard added** — 403 if authenticated user is not the owner | +| `POST /api/expenselist/accept-invite` | **`userId` removed from body** — derived from JWT instead | +| `GET /api/users?id=X` | **Guard added** — 403 if id doesn't match JWT user's id | +| `GET /api/users/byGoogleId?id=X` | **Guard added** — 403 if id doesn't match JWT sub | +| `DELETE /api/users?id=X` | **Guard added** — 403 if id doesn't match JWT user's id | + +### Ownership Check Helper +Each guard is implemented as a private method in its controller (2–3 lines): +```java +private void assertOwner(AppUser authenticated, ExpenseList list) { + if (!list.getOwner().getId().equals(authenticated.getId())) + throw new ResponseStatusException(HttpStatus.FORBIDDEN); +} + +private void assertMember(AppUser authenticated, ExpenseList list) { + boolean isOwner = list.getOwner().getId().equals(authenticated.getId()); + boolean isShared = list.getSharedWith() != null && list.getSharedWith().getId().equals(authenticated.getId()); + if (!isOwner && !isShared) + throw new ResponseStatusException(HttpStatus.FORBIDDEN); +} +``` + +### Test Profile +The `@Profile("test")` security chain in `SecurityConfig` is untouched. Existing tests continue to work without authentication. + +--- + +## Section 3 — Rate Limiting + +### Dependency +Add `bucket4j-core` to `pom.xml`. In-memory storage — no external cache needed for single-instance deployment. + +### Implementation +A `RateLimitFilter` extending `OncePerRequestFilter`, registered as a `@Component` with `@Profile("!test")`: + +- **Key for authenticated requests:** JWT `sub` claim (per-user bucket) +- **Key for unauthenticated requests:** remote IP address (pre-auth fallback) +- Buckets stored in a `ConcurrentHashMap` + +### Limits + +| Endpoint pattern | Limit | +|---|---| +| All endpoints (default) | 60 requests / minute | +| `POST /api/expenselist/*/invite` | 5 requests / minute | +| `POST /api/expenselist/accept-invite` | 10 requests / minute | +| `POST /api/users/createUser` | 3 requests / minute | + +Sensitive endpoints get their own per-user bucket independent of the general bucket. + +### Response +When a bucket is exhausted: `429 Too Many Requests` with a `Retry-After: ` header indicating time until refill. + +--- + +## Architecture Summary + +``` +Request + └── RateLimitFilter (per-user/IP buckets) + └── SecurityFilterChain (JWT validation) + └── Controller + ├── @Valid on @RequestBody → GlobalExceptionHandler on failure + ├── AuthenticatedUserResolver → AppUser from JWT sub + └── assertOwner / assertMember → 403 on violation +``` + +No new service layer is introduced. The authorization checks are lightweight and local to each controller method. + +--- + +## Files Affected + +| File | Change | +|---|---| +| `pom.xml` | Add `spring-boot-starter-validation`, `bucket4j-core` | +| `model/AppUserCreateRequest.java` | Add validation annotations | +| `model/ExpenseInput.java` | Add validation annotations, remove JPA annotations | +| `model/ExpenseChangeRequest.java` | Add validation annotations | +| `model/InviteRequest.java` | Add validation annotations, remove `userId` field | +| `controller/AppUserController.java` | Add `@Valid`, ownership guards, use `AuthenticatedUserResolver` | +| `controller/ExpenseListController.java` | Add `@Valid`, ownership guards, remove/rename endpoints, use `AuthenticatedUserResolver` | +| `security/AuthenticatedUserResolver.java` | **New** — resolves JWT sub to `AppUser` | +| `security/RateLimitFilter.java` | **New** — per-user/IP rate limiting | +| `controller/GlobalExceptionHandler.java` | **New** — structured 400/403 error responses |