cannamanage/docs/sprint-3/cannamanage-sprint3-security-review.md

# Security Review: CannaManage Sprint 3 — Phases 1-3

**Date:** 2026-06-11  
**Module:** cannamanage (all modules)  
**Reviewer:** Roo (Security Reviewer)  
**Branch:** sprint/3-staff-portal (pre-implementation)  
**Type:** Design-level security review (plan analysis)  
**Verdict:** ✅ PASS (with advisory findings)

---

## Scope

Pre-implementation security analysis of Sprint 3 Phases 1-3:
- **Phase 1:** Staff Permission Foundation + Token Revocation
- **Phase 2:** Club Settings Controller + Email Domain Whitelist
- **Phase 3:** Staff Management + Invite Flow

---

## Scan Results

| Tool | Status | Notes |
|------|--------|-------|
| SonarQube (SAST) | ⏭️ Not applicable | Design-level review — no code to scan |
| Datarake (Secrets) | ⏭️ Not applicable | Design-level review |
| Snyk Code | ⏭️ Not applicable | Design-level review |

---

## Security Checklist (Adapted for Design Review)

| # | Rule | Check | Result | Notes |
|---|------|-------|--------|-------|
| 1 | SEC-001..004 | No hardcoded credentials in plan | ✅ | SMTP credentials use `${SMTP_*}` env vars, JWT secret uses `${cannamanage.security.jwt.secret}` |
| 2 | SEC-005 | Credentials via @Value/env | ✅ | All sensitive config externalized in `application.yml` via env vars |
| 3 | SEC-011 | No SQL injection vectors | ✅ | All DB access via Spring Data JPA repositories — no raw SQL concatenation |
| 4 | SEC-012 | No path traversal | ✅ | No file I/O in Phases 1-3 |
| 5 | SEC-016 | Input validation on all entry points | ⚠️ | See Finding #1 (invite token validation) and Finding #2 (regex pattern) |
| 6 | SEC-018 | No info disclosure in errors | ⚠️ | See Finding #3 (invite token error messages) |
| 7 | SEC-033 | PII handling | ✅ | Staff email stored in DB (expected); permissions are non-PII |
| 8 | SEC-035 | No PII in LLM processing | ✅ | N/A — no LLM integration |
| 9 | SEC-040 | No sensitive data in logs | ⚠️ | See Finding #4 (invite token logging risk) |
| 10 | — | Privilege escalation vectors | ⚠️ | See Finding #5 (JWT permissions vs DB permissions race) |
| 11 | — | Token revocation completeness | ⚠️ | See Finding #6 (60s window + refresh token interaction) |
| 12 | — | Cryptographic token generation | ⚠️ | See Finding #7 (invite token entropy) |
| 13 | — | Rate limiting on sensitive endpoints | ⚠️ | See Finding #8 (set-password endpoint) |
| 14 | — | SMTP injection | ✅ | Spring Mail handles email header injection prevention |
| 15 | — | Tenant isolation in new tables | ✅ | `staff_accounts` has `tenant_id`; queries via TenantFilterAspect |

---

## Findings

### ⚠️ Medium Severity (should address during implementation)

#### Finding #1: Invite token lacks complexity requirements for password

**Phase:** 3 (Step 3.9: `POST /auth/set-password`)  
**Risk:** Password strength not mentioned in plan  
**Description:** The `SetPasswordRequest` DTO accepts a password, but the plan doesn't specify minimum complexity requirements. Weak passwords would undermine the security of staff accounts that have elevated permissions.

**Recommendation:**
```java
// In SetPasswordRequest DTO validation
@NotBlank
@Size(min = 12, message = "Password must be at least 12 characters")
@Pattern(regexp = "^(?=.*[a-z])(?=.*[A-Z])(?=.*\\d).*$",
         message = "Password must contain uppercase, lowercase, and digit")
private String password;
```

**Severity:** Medium — Staff accounts have elevated permissions (RECORD_DISTRIBUTION, VIEW_MEMBER_LIST with PII access).

---

#### Finding #2: Email domain whitelist regex — ReDoS vulnerability

**Phase:** 2 (Step 2.7) + Phase 3 (Step 3.5)  
**Risk:** Denial of Service via catastrophic backtracking  
**Description:** The plan uses `email.matches(club.getAllowedEmailPattern())` where the pattern is admin-supplied. A malicious or poorly-constructed regex like `^(a+)+@evil.com$` can cause exponential backtracking.

**Recommendation:**
```java
private void validateEmailDomain(String email, Club club) {
    if (club.getAllowedEmailPattern() == null) return;
    
    // 1. Validate pattern syntax
    Pattern pattern;
    try {
        pattern = Pattern.compile(club.getAllowedEmailPattern());
    } catch (PatternSyntaxException e) {
        throw new InvalidEmailPatternException("Invalid regex pattern on club settings");
    }
    
    // 2. Apply with timeout protection
    Matcher matcher = pattern.matcher(email);
    ExecutorService executor = Executors.newSingleThreadExecutor();
    Future<Boolean> future = executor.submit(matcher::matches);
    try {
        boolean matches = future.get(100, TimeUnit.MILLISECONDS);
        if (!matches) throw new EmailDomainNotAllowedException(email);
    } catch (TimeoutException e) {
        future.cancel(true);
        throw new InvalidEmailPatternException("Email pattern validation timed out — pattern may be too complex");
    } finally {
        executor.shutdownNow();
    }
}
```

Alternatively, restrict allowed patterns to a simple domain list (no arbitrary regex):
```java
// Simpler, safer approach
@Column(name = "allowed_email_domains")
private String allowedEmailDomains;  // comma-separated: "example.de,club.de"
```

**Severity:** Medium — An admin can DoS their own tenant's invite flow. Cross-tenant impact is blocked by TenantFilterAspect.

---

#### Finding #3: Invite token error responses may leak information

**Phase:** 3 (Step 3.9: `POST /auth/set-password`)  
**Risk:** Information disclosure / account enumeration  
**Description:** Different error messages for "token not found", "token expired", and "token already used" allow an attacker to enumerate valid invite tokens and determine their state.

**Recommendation:** Return a generic error message for all failure cases:
```java
// ❌ AVOID — reveals token state
if (token == null) throw new NotFoundException("Token not found");
if (token.getExpiresAt().isBefore(Instant.now())) throw new BadRequestException("Token expired");
if (token.getUsedAt() != null) throw new BadRequestException("Token already used");

// ✅ PREFERRED — generic response
if (token == null || token.getExpiresAt().isBefore(Instant.now()) || token.getUsedAt() != null) {
    throw new BadRequestException("Invalid or expired invitation link. Please contact your club administrator.");
}
```

**Severity:** Medium — The `/auth/set-password` endpoint is public (no auth required). Token enumeration could allow unauthorized password setting if tokens are guessable (see Finding #7).

---

#### Finding #4: Invite token values must not appear in logs

**Phase:** 3 (Steps 3.4, 3.5, 3.9)  
**Risk:** Token leakage via log files  
**Description:** The invite token is a bearer credential — anyone with the token value can set a password and gain staff access. If logged (e.g., in request logging, error logs, or debug statements), it becomes accessible to anyone with log access.

**Recommendation:**
```java
// In EmailService — do NOT log the full token
log.info("Invite email sent to {} for user {}", email, userId);
// NOT: log.info("Invite sent with token {}", token);

// In AuthController.setPassword — do NOT log the token value
log.info("Password set successfully for invite (user: {})", userId);
// NOT: log.debug("Processing set-password for token: {}", request.getToken());
```

Also ensure that Spring Boot request logging (if enabled) does not capture the token from the request body.

**Severity:** Medium — Log access is typically broader than intended credential access.

---

### ℹ️ Low Severity (advisory — defense-in-depth suggestions)

#### Finding #5: JWT permissions vs. DB permissions — stale permission window

**Phase:** 1 (Steps 1.9, 1.10, 1.12)  
**Risk:** Privilege escalation during 60s Caffeine cache TTL  
**Description:** Staff permissions are embedded in the JWT. When an admin changes a staff member's permissions, `revokeAllForUser()` is called, which adds the old token's `jti` to the blacklist. However:

1. The Caffeine cache has a 60s TTL — during this window, the old token could still be accepted by another node (if scaled horizontally in the future)
2. More critically: if the revoked token check is cached as "not revoked" by Caffeine BEFORE the revocation occurs, it remains cached for up to 60s

The plan correctly identifies this as an acceptable risk for a club-scale app.

**Recommendation:** Document this 60-second window as a known limitation. For future scaling, consider:
- Reducing Caffeine TTL to 10-15s (acceptable DB load for club scale)
- Using a `LoadingCache` that checks a "last revocation timestamp" before returning cached false results

**Severity:** Low — Acceptable for MVP. A staff member could perform unauthorized actions for at most 60 seconds after permission revocation.

---

#### Finding #6: Token revocation doesn't address refresh tokens

**Phase:** 1 (Step 1.6: `TokenRevocationService`)  
**Risk:** Revoked staff can still obtain new access tokens via refresh  
**Description:** The plan's `revokeAllForUser()` revokes access token `jti` values in the blacklist. But the refresh token (used at `POST /auth/refresh`) has a separate 30-day lifetime. If only access tokens are revoked, the staff member can use their refresh token to obtain a new (valid) access token with the OLD permissions still embedded.

**Recommendation:** On permission change or staff deactivation:
1. Revoke all access tokens (current plan) ✅
2. Also invalidate the user's refresh token (clear `refresh_token_hash` in `users` table) ✅ (must be explicitly added)

```java
public void revokeAllForUser(UUID userId) {
    // 1. Blacklist all active access tokens
    List<RevokedToken> tokens = ... ;
    revokedTokenRepository.saveAll(tokens);
    blacklistCache.invalidateAll(); // force re-check
    
    // 2. Also invalidate refresh token
    userRepository.clearRefreshTokenHash(userId);
}
```

**Severity:** Low — The plan's Step 3.11 updates `AuthService.login()` to reject `active=false` users, which partially mitigates this. But a user who is still `active=true` with changed permissions could refresh and get a new token with stale permissions if the refresh endpoint doesn't re-check permissions from DB.

---

#### Finding #7: Invite token must use cryptographically secure random generation

**Phase:** 3 (Step 3.2: `InviteToken` entity)  
**Risk:** Predictable tokens enable unauthorized account takeover  
**Description:** The plan specifies `token VARCHAR(255) NOT NULL UNIQUE` but doesn't specify the generation method. If `UUID.randomUUID()` is used, it's type-4 (PRNG-based) which is generally secure but predictable under certain JVM conditions.

**Recommendation:** Use `SecureRandom` with sufficient entropy:
```java
// In InviteTokenService or StaffService
private String generateSecureToken() {
    byte[] bytes = new byte[32]; // 256 bits of entropy
    new SecureRandom().nextBytes(bytes);
    return Base64.getUrlEncoder().withoutPadding().encodeToString(bytes);
}
```

This produces a 43-character URL-safe token with 256 bits of entropy — sufficient to prevent brute-force guessing even on the public `/auth/set-password` endpoint.

**Severity:** Low — The 72-hour expiry mitigates brute-force attacks. But with `UUID.randomUUID()` (122 bits of randomness), an attacker would need ~5.3 × 10³⁶ attempts — already computationally infeasible. This finding is defense-in-depth.

---

#### Finding #8: Rate limiting on `POST /auth/set-password`

**Phase:** 3 (Step 3.9)  
**Risk:** Brute-force invite token guessing  
**Description:** The `POST /auth/set-password` endpoint is public (no authentication required) and accepts a token in the request body. Without rate limiting, an attacker could attempt to brute-force valid tokens.

**Recommendation:** Apply rate limiting per IP:
```java
// In SecurityConfig or via @RateLimiter annotation
.requestMatchers("/api/v1/auth/set-password").permitAll()
// Add: rate limit 5 requests per minute per IP
```

Options:
- Spring Boot Starter Rate Limiter (Bucket4j)
- Custom `Filter` with Caffeine-based per-IP counter
- Nginx-level rate limiting (`limit_req_zone`)

**Severity:** Low — Token entropy (Finding #7) makes brute-force infeasible even without rate limiting. This is defense-in-depth.

---

## Identified False Positives

| Pattern | Why It's Safe |
|---------|--------------|
| SMTP credentials in `application.yml` | All via `${SMTP_*}` env var placeholders — not hardcoded |
| JWT secret in `JwtService` | Via `@Value("${cannamanage.security.jwt.secret}")` — externalized |
| `allowedEmailPattern` stored in DB | Admin-only configurable; only affects their own tenant |
| `staff_accounts.granted_permissions` as JSONB | Not a credential; contains only permission names |

---

## Architecture-Level Security Assessment

### Token Revocation Design (D1)

| Aspect | Security Rating | Notes |
|--------|----------------|-------|
| DB persistence | ✅ Strong | Survives restarts; durable record |
| Caffeine cache | ⚠️ Acceptable | 60s stale window documented and accepted |
| `jti` uniqueness | ✅ Strong | UUID-based, UNIQUE constraint in DB |
| Cleanup scheduler | ✅ Good | Prevents table bloat; respects token expiry |
| Horizontal scale readiness | ⚠️ Weak | Cache is per-instance; need shared cache for multi-node |

### Staff Permission Model

| Aspect | Security Rating | Notes |
|--------|----------------|-------|
| ADMIN bypass | ✅ Correct | `StaffPermissionChecker` returns true for ADMIN first |
| STAFF denial by default | ✅ Correct | If no permissions found → deny |
| JSONB storage | ✅ Good | Flexible, supports audit trail of changes |
| JWT-embedded permissions | ⚠️ Acceptable | Stale for max token lifetime after revocation |
| Tenant isolation | ✅ Strong | `tenant_id` on `staff_accounts`; TenantFilterAspect enforced |

### Invite Flow Security

| Aspect | Security Rating | Notes |
|--------|----------------|-------|
| Token expiry (72h) | ✅ Good | Reasonable window; not too long |
| Single-use enforcement | ✅ Strong | `used_at` timestamp prevents reuse |
| Public endpoint exposure | ⚠️ Acceptable | Only accepts token + password; generic errors recommended |
| Email delivery trust | ⚠️ Inherent risk | Email is not a secure channel; standard for invite flows |
| Active=false until password set | ✅ Strong | Prevents login before activation |

---

## Verdict

### ✅ PASS

No Critical or High severity findings. The design is fundamentally sound for a club-scale application.

**8 findings total:**
- 0 Critical
- 0 High
- 4 Medium (should address during implementation)
- 4 Low (defense-in-depth suggestions)

**Key implementation instructions for the developer:**
1. Add password complexity validation on `SetPasswordRequest` (Finding #1)
2. Add regex timeout or switch to domain list for email whitelist (Finding #2)
3. Use generic error messages on `/auth/set-password` (Finding #3)
4. Never log invite token values (Finding #4)
5. Clear refresh token hash on permission change (Finding #6)
6. Use `SecureRandom` for invite token generation (Finding #7)

---

## Review Metadata

| Field | Value |
|-------|-------|
| Review type | Pre-implementation (design-level) |
| Phases reviewed | 1, 2, 3 |
| OWASP categories checked | A01 (Broken Access Control), A02 (Crypto Failures), A03 (Injection), A04 (Insecure Design), A07 (Auth Failures) |
| Confidence | 88% (design review; actual implementation may introduce additional issues) |
| Re-review required | Yes — after implementation, run full SAST + code-level SEC-* checklist |