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:

// 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:

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):

// 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:

// ❌ 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:

// 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)

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:

// 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:

// 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