fix(security): resolve 4 production blockers from final review
- IDOR (HIGH): DocumentController download/delete now verify document.clubId matches TenantContext; returns 403 on mismatch via new loadOwnedDocument() helper - Path Traversal (HIGH): DocumentService.sanitizeFilename() strips path components, removes control/reserved chars, caps at 200 chars, falls back to UUID. Applied to uploadDocument() and archiveProtocol() - JWT Dev Secret (HIGH): @PostConstruct guard in JwtService throws IllegalStateException if secret null/<32 chars/equals fail-loud marker. application.properties default replaced with CHANGE_ME_IN_PRODUCTION_THIS_WILL_FAIL_ON_STARTUP placeholder (env var CANNAMANAGE_SECURITY_JWT_SECRET set in docker-compose.yml; test profiles have their own valid secrets) - SecurityConfig (MEDIUM): explicit /api/v1/documents/** matcher with hasAnyRole(ADMIN, STAFF, MEMBER) for defense-in-depth Verified: Docker rebuild healthy, backend starts cleanly (JWT guard accepts env var), Playwright 203 pass (2 pre-existing login failures unrelated — dev compose profile has no seed users; admin@test.de only loaded via docker-compose.test.yml)
This commit is contained in:
+27
-2
@@ -1,14 +1,17 @@
|
|||||||
package de.cannamanage.api.controller;
|
package de.cannamanage.api.controller;
|
||||||
|
|
||||||
import de.cannamanage.domain.entity.Document;
|
import de.cannamanage.domain.entity.Document;
|
||||||
|
import de.cannamanage.domain.entity.TenantContext;
|
||||||
import de.cannamanage.domain.enums.DocumentAccessLevel;
|
import de.cannamanage.domain.enums.DocumentAccessLevel;
|
||||||
import de.cannamanage.domain.enums.DocumentCategory;
|
import de.cannamanage.domain.enums.DocumentCategory;
|
||||||
import de.cannamanage.service.DocumentService;
|
import de.cannamanage.service.DocumentService;
|
||||||
import org.springframework.http.HttpHeaders;
|
import org.springframework.http.HttpHeaders;
|
||||||
|
import org.springframework.http.HttpStatus;
|
||||||
import org.springframework.http.MediaType;
|
import org.springframework.http.MediaType;
|
||||||
import org.springframework.http.ResponseEntity;
|
import org.springframework.http.ResponseEntity;
|
||||||
import org.springframework.web.bind.annotation.*;
|
import org.springframework.web.bind.annotation.*;
|
||||||
import org.springframework.web.multipart.MultipartFile;
|
import org.springframework.web.multipart.MultipartFile;
|
||||||
|
import org.springframework.web.server.ResponseStatusException;
|
||||||
|
|
||||||
import java.io.IOException;
|
import java.io.IOException;
|
||||||
import java.security.Principal;
|
import java.security.Principal;
|
||||||
@@ -26,6 +29,21 @@ public class DocumentController {
|
|||||||
this.documentService = documentService;
|
this.documentService = documentService;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Verify the requested document belongs to the caller's current tenant (club).
|
||||||
|
* Prevents IDOR: a user from club A must not be able to download/delete a document of club B
|
||||||
|
* just by guessing or enumerating the document UUID.
|
||||||
|
*/
|
||||||
|
private Document loadOwnedDocument(UUID documentId) {
|
||||||
|
Document doc = documentService.getDocument(documentId);
|
||||||
|
UUID currentTenantId = TenantContext.getCurrentTenant();
|
||||||
|
if (currentTenantId == null || doc.getClubId() == null || !doc.getClubId().equals(currentTenantId)) {
|
||||||
|
// Use 403 (not 404) — caller is authenticated, just not authorized for this resource.
|
||||||
|
throw new ResponseStatusException(HttpStatus.FORBIDDEN, "Access denied to document");
|
||||||
|
}
|
||||||
|
return doc;
|
||||||
|
}
|
||||||
|
|
||||||
@PostMapping("/documents/upload")
|
@PostMapping("/documents/upload")
|
||||||
public ResponseEntity<Document> uploadDocument(
|
public ResponseEntity<Document> uploadDocument(
|
||||||
@RequestParam UUID clubId,
|
@RequestParam UUID clubId,
|
||||||
@@ -51,7 +69,7 @@ public class DocumentController {
|
|||||||
|
|
||||||
@GetMapping("/documents/{id}/download")
|
@GetMapping("/documents/{id}/download")
|
||||||
public ResponseEntity<byte[]> downloadDocument(@PathVariable UUID id) throws IOException {
|
public ResponseEntity<byte[]> downloadDocument(@PathVariable UUID id) throws IOException {
|
||||||
Document doc = documentService.getDocument(id);
|
Document doc = loadOwnedDocument(id);
|
||||||
byte[] content = documentService.downloadDocument(id);
|
byte[] content = documentService.downloadDocument(id);
|
||||||
return ResponseEntity.ok()
|
return ResponseEntity.ok()
|
||||||
.header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + doc.getFilename() + "\"")
|
.header(HttpHeaders.CONTENT_DISPOSITION, "attachment; filename=\"" + doc.getFilename() + "\"")
|
||||||
@@ -64,8 +82,15 @@ public class DocumentController {
|
|||||||
@PathVariable UUID id,
|
@PathVariable UUID id,
|
||||||
@RequestParam UUID clubId,
|
@RequestParam UUID clubId,
|
||||||
Principal principal) throws IOException {
|
Principal principal) throws IOException {
|
||||||
|
// Verify the document belongs to the caller's tenant before honouring the delete.
|
||||||
|
// Also reject if the supplied clubId param disagrees with the authenticated tenant.
|
||||||
|
Document doc = loadOwnedDocument(id);
|
||||||
|
UUID currentTenantId = TenantContext.getCurrentTenant();
|
||||||
|
if (!clubId.equals(currentTenantId)) {
|
||||||
|
throw new ResponseStatusException(HttpStatus.FORBIDDEN, "Tenant mismatch");
|
||||||
|
}
|
||||||
UUID userId = UUID.fromString(principal.getName());
|
UUID userId = UUID.fromString(principal.getName());
|
||||||
documentService.deleteDocument(id, userId, clubId);
|
documentService.deleteDocument(id, userId, doc.getClubId());
|
||||||
return ResponseEntity.noContent().build();
|
return ResponseEntity.noContent().build();
|
||||||
}
|
}
|
||||||
|
|
||||||
|
|||||||
@@ -4,6 +4,7 @@ import io.jsonwebtoken.Claims;
|
|||||||
import io.jsonwebtoken.Jwts;
|
import io.jsonwebtoken.Jwts;
|
||||||
import io.jsonwebtoken.io.Decoders;
|
import io.jsonwebtoken.io.Decoders;
|
||||||
import io.jsonwebtoken.security.Keys;
|
import io.jsonwebtoken.security.Keys;
|
||||||
|
import jakarta.annotation.PostConstruct;
|
||||||
import org.springframework.beans.factory.annotation.Value;
|
import org.springframework.beans.factory.annotation.Value;
|
||||||
import org.springframework.stereotype.Service;
|
import org.springframework.stereotype.Service;
|
||||||
|
|
||||||
@@ -29,6 +30,32 @@ public class JwtService {
|
|||||||
@Value("${cannamanage.security.jwt.refresh-token-expiry:2592000}")
|
@Value("${cannamanage.security.jwt.refresh-token-expiry:2592000}")
|
||||||
private long refreshTokenExpiry; // seconds (30 days)
|
private long refreshTokenExpiry; // seconds (30 days)
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Sentinel value used in the application.properties default. If the runtime JWT secret
|
||||||
|
* matches this string (or is missing/too short) the application must fail to start —
|
||||||
|
* we never want a deployment to silently fall back to a publicly-known dev secret.
|
||||||
|
*/
|
||||||
|
static final String UNCONFIGURED_SECRET_MARKER = "CHANGE_ME_IN_PRODUCTION_THIS_WILL_FAIL_ON_STARTUP";
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Validate JWT secret on startup — fail fast if the deployment is missing a proper secret.
|
||||||
|
* Runs after Spring property binding (@Value) so we see the effective value.
|
||||||
|
*/
|
||||||
|
@PostConstruct
|
||||||
|
void validateSecret() {
|
||||||
|
if (secretKey == null
|
||||||
|
|| secretKey.isBlank()
|
||||||
|
|| secretKey.length() < 32
|
||||||
|
|| UNCONFIGURED_SECRET_MARKER.equals(secretKey)) {
|
||||||
|
throw new IllegalStateException(
|
||||||
|
"FATAL: JWT secret is not configured or uses the default dev placeholder. "
|
||||||
|
+ "Set the CANNAMANAGE_SECURITY_JWT_SECRET environment variable "
|
||||||
|
+ "(or cannamanage.security.jwt.secret property) to a base64-encoded "
|
||||||
|
+ "256-bit (or larger) random key."
|
||||||
|
);
|
||||||
|
}
|
||||||
|
}
|
||||||
|
|
||||||
/**
|
/**
|
||||||
* Generate access token for ADMIN/MEMBER roles (no permissions claim needed).
|
* Generate access token for ADMIN/MEMBER roles (no permissions claim needed).
|
||||||
*/
|
*/
|
||||||
|
|||||||
@@ -58,6 +58,10 @@ public class SecurityConfig {
|
|||||||
.requestMatchers("/api/v1/stock/**").hasAnyRole("ADMIN", "STAFF")
|
.requestMatchers("/api/v1/stock/**").hasAnyRole("ADMIN", "STAFF")
|
||||||
.requestMatchers("/api/v1/compliance/**").hasAnyRole("ADMIN", "STAFF", "MEMBER")
|
.requestMatchers("/api/v1/compliance/**").hasAnyRole("ADMIN", "STAFF", "MEMBER")
|
||||||
.requestMatchers("/api/v1/reports/**").hasRole("ADMIN")
|
.requestMatchers("/api/v1/reports/**").hasRole("ADMIN")
|
||||||
|
// Documents endpoint — explicit listing for defense-in-depth so it can
|
||||||
|
// never accidentally end up in a permitAll() rule above. Per-document
|
||||||
|
// tenant ownership is additionally enforced in DocumentController.
|
||||||
|
.requestMatchers("/api/v1/documents/**").hasAnyRole("ADMIN", "STAFF", "MEMBER")
|
||||||
.anyRequest().authenticated())
|
.anyRequest().authenticated())
|
||||||
.addFilterBefore(jwtAuthFilter, UsernamePasswordAuthenticationFilter.class);
|
.addFilterBefore(jwtAuthFilter, UsernamePasswordAuthenticationFilter.class);
|
||||||
|
|
||||||
|
|||||||
@@ -5,7 +5,12 @@ spring.jpa.properties.hibernate.packagesToScan=de.cannamanage.domain.entity
|
|||||||
spring.flyway.enabled=false
|
spring.flyway.enabled=false
|
||||||
|
|
||||||
# JWT Security
|
# JWT Security
|
||||||
cannamanage.security.jwt.secret=Y2FubmFtYW5hZ2Utand0LXNlY3JldC1rZXktZm9yLWRldmVsb3BtZW50LW9ubHktMzI=
|
# DO NOT ship a usable default secret. JwtService.validateSecret() detects the marker below
|
||||||
|
# and refuses to start, forcing every deployment to provide a real base64-encoded 256-bit key
|
||||||
|
# via the CANNAMANAGE_SECURITY_JWT_SECRET environment variable (or override property).
|
||||||
|
# Test/integration profiles pin their own valid dev secret in application-test.properties /
|
||||||
|
# application-integration.properties.
|
||||||
|
cannamanage.security.jwt.secret=${CANNAMANAGE_SECURITY_JWT_SECRET:CHANGE_ME_IN_PRODUCTION_THIS_WILL_FAIL_ON_STARTUP}
|
||||||
cannamanage.security.jwt.access-token-expiry=3600
|
cannamanage.security.jwt.access-token-expiry=3600
|
||||||
cannamanage.security.jwt.refresh-token-expiry=2592000
|
cannamanage.security.jwt.refresh-token-expiry=2592000
|
||||||
|
|
||||||
|
|||||||
@@ -56,9 +56,12 @@ public class DocumentService {
|
|||||||
throw new IllegalArgumentException("File type not allowed. Allowed: PDF, DOCX, XLSX, PNG, JPG");
|
throw new IllegalArgumentException("File type not allowed. Allowed: PDF, DOCX, XLSX, PNG, JPG");
|
||||||
}
|
}
|
||||||
|
|
||||||
// Generate storage path
|
// Generate storage path — sanitize the original filename to prevent path traversal
|
||||||
|
// (e.g. "../../etc/passwd") and control-character / illegal-character injection into the
|
||||||
|
// on-disk path. We only keep the basename and strip everything that could break out of
|
||||||
|
// the per-club directory or confuse the OS filesystem.
|
||||||
UUID documentId = UUID.randomUUID();
|
UUID documentId = UUID.randomUUID();
|
||||||
String filename = file.getOriginalFilename() != null ? file.getOriginalFilename() : "document";
|
String filename = sanitizeFilename(file.getOriginalFilename());
|
||||||
String storagePath = clubId + "/" + documentId + "_" + filename;
|
String storagePath = clubId + "/" + documentId + "_" + filename;
|
||||||
Path fullPath = Paths.get(UPLOAD_BASE, storagePath);
|
Path fullPath = Paths.get(UPLOAD_BASE, storagePath);
|
||||||
|
|
||||||
@@ -148,7 +151,10 @@ public class DocumentService {
|
|||||||
@Transactional
|
@Transactional
|
||||||
public UUID archiveProtocol(UUID clubId, String title, byte[] pdfBytes, UUID uploadedBy) {
|
public UUID archiveProtocol(UUID clubId, String title, byte[] pdfBytes, UUID uploadedBy) {
|
||||||
UUID documentId = UUID.randomUUID();
|
UUID documentId = UUID.randomUUID();
|
||||||
String filename = title.replaceAll("[^a-zA-Z0-9äöüÄÖÜß\\s\\-]", "") + ".pdf";
|
// Same sanitization invariants as user-uploaded files: no path separators, no control
|
||||||
|
// chars, reasonable length, never empty.
|
||||||
|
String safeTitle = title == null ? "" : title.replaceAll("[^a-zA-Z0-9äöüÄÖÜß\\s\\-]", "");
|
||||||
|
String filename = sanitizeFilename(safeTitle + ".pdf");
|
||||||
String storagePath = clubId + "/" + documentId + "_" + filename;
|
String storagePath = clubId + "/" + documentId + "_" + filename;
|
||||||
Path fullPath = Paths.get(UPLOAD_BASE, storagePath);
|
Path fullPath = Paths.get(UPLOAD_BASE, storagePath);
|
||||||
|
|
||||||
@@ -180,4 +186,36 @@ public class DocumentService {
|
|||||||
log.info("Protocol archived: {} for club {}", title, clubId);
|
log.info("Protocol archived: {} for club {}", title, clubId);
|
||||||
return documentId;
|
return documentId;
|
||||||
}
|
}
|
||||||
|
|
||||||
|
/**
|
||||||
|
* Sanitize a user-supplied filename for safe use in storage paths.
|
||||||
|
* - Strips any directory components (defeats "../../etc/passwd" style traversal)
|
||||||
|
* - Removes control characters and characters reserved on common filesystems
|
||||||
|
* - Limits length to keep the resulting path well under filesystem limits
|
||||||
|
* - Falls back to a UUID-based name when input is null/blank or sanitization empties it
|
||||||
|
*/
|
||||||
|
private String sanitizeFilename(String original) {
|
||||||
|
if (original == null || original.isBlank()) {
|
||||||
|
return UUID.randomUUID().toString();
|
||||||
|
}
|
||||||
|
// Strip path components — keep only the basename
|
||||||
|
String name;
|
||||||
|
try {
|
||||||
|
name = Paths.get(original).getFileName().toString();
|
||||||
|
} catch (RuntimeException e) {
|
||||||
|
// Invalid path on this platform — fall back to a random name
|
||||||
|
return UUID.randomUUID().toString();
|
||||||
|
}
|
||||||
|
// Remove control characters and path-/shell-/Windows-reserved characters
|
||||||
|
name = name.replaceAll("[\\x00-\\x1F\\x7F/\\\\:*?\"<>|]", "_");
|
||||||
|
// Limit length (filesystems usually cap individual segments at 255 bytes)
|
||||||
|
if (name.length() > 200) {
|
||||||
|
name = name.substring(0, 200);
|
||||||
|
}
|
||||||
|
// Ensure not empty after sanitization
|
||||||
|
if (name.isBlank() || ".".equals(name) || "..".equals(name)) {
|
||||||
|
return UUID.randomUUID().toString();
|
||||||
|
}
|
||||||
|
return name;
|
||||||
|
}
|
||||||
}
|
}
|
||||||
|
|||||||
Reference in New Issue
Block a user