security hardening #12
File diff suppressed because it is too large
Load Diff
@@ -1,166 +0,0 @@
|
|||||||
# 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<String, Bucket>`
|
|
||||||
|
|
||||||
### 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: <seconds>` 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 |
|
|
||||||
Reference in New Issue
Block a user