diff --git a/docs/superpowers/plans/2026-04-29-security-review-fixes.md b/docs/superpowers/plans/2026-04-29-security-review-fixes.md new file mode 100644 index 0000000..43e5c40 --- /dev/null +++ b/docs/superpowers/plans/2026-04-29-security-review-fixes.md @@ -0,0 +1,310 @@ +# Security Review Fixes Implementation Plan + +> **For agentic workers:** REQUIRED SUB-SKILL: Use superpowers:subagent-driven-development (recommended) or superpowers:executing-plans to implement this plan task-by-task. Steps use checkbox (`- [ ]`) syntax for tracking. + +**Goal:** Fix 5 security vulnerabilities found in the full-codebase security review — hardcoded JWT secret, missing authorization on tenant portal admin endpoints, dead code password/settings endpoints, and unprotected tenant lookups. + +**Architecture:** All fixes are surgical edits to existing files. No new files, no schema changes. The JWT secret fix adds one field to `ProvisioningProperties` and reads it in the provisioner. The authorization fixes add `@PreAuthorize` annotations. Dead code removal deletes unsafe endpoints and their unused frontend hooks. + +**Tech Stack:** Spring Boot, Spring Security (`@PreAuthorize`), Spring Boot `@ConfigurationProperties` + +--- + +### Task 1: Fix hardcoded JWT secret in DockerTenantProvisioner + +**Files:** +- Modify: `src/main/java/io/cameleer/saas/provisioning/ProvisioningProperties.java` +- Modify: `src/main/java/io/cameleer/saas/provisioning/DockerTenantProvisioner.java:223` + +- [ ] **Step 1: Add `jwtSecret` field to ProvisioningProperties** + +In `src/main/java/io/cameleer/saas/provisioning/ProvisioningProperties.java`, add `jwtSecret` as a new field after `corsOrigins`: + +```java +@ConfigurationProperties(prefix = "cameleer.saas.provisioning") +public record ProvisioningProperties( + String serverImage, + String serverUiImage, + String runtimeBaseImage, + String networkName, + String traefikNetwork, + String publicHost, + String publicProtocol, + String datasourceUrl, + String datasourceUsername, + String datasourcePassword, + String clickhouseUrl, + String clickhouseUser, + String clickhousePassword, + String oidcIssuerUri, + String oidcJwkSetUri, + String corsOrigins, + String jwtSecret +) {} +``` + +This binds from env var `CAMELEER_SAAS_PROVISIONING_JWTSECRET`. The installer already generates `CAMELEER_SERVER_SECURITY_JWTSECRET` — the compose template needs to also set `CAMELEER_SAAS_PROVISIONING_JWTSECRET` to the same value (or the deployer maps it manually). A missing value will be `null`, caught by the validation below. + +- [ ] **Step 2: Replace hardcoded secret with property value** + +In `src/main/java/io/cameleer/saas/provisioning/DockerTenantProvisioner.java`, replace line 223: + +```java +// OLD: +"CAMELEER_SERVER_SECURITY_JWTSECRET=cameleer-dev-jwt-secret-change-in-production", + +// NEW: +"CAMELEER_SERVER_SECURITY_JWTSECRET=" + props.jwtSecret(), +``` + +- [ ] **Step 3: Add startup validation for jwtSecret** + +In `DockerTenantProvisioner.java`, add a validation check at the end of the constructor (after line 36): + +```java +if (props.jwtSecret() == null || props.jwtSecret().isBlank()) { + log.warn("CAMELEER_SAAS_PROVISIONING_JWTSECRET is not set — provisioned servers will fail to start"); +} +``` + +- [ ] **Step 4: Commit** + +```bash +git add src/main/java/io/cameleer/saas/provisioning/ProvisioningProperties.java src/main/java/io/cameleer/saas/provisioning/DockerTenantProvisioner.java +git commit -m "fix(security): replace hardcoded JWT secret with config property + +Every provisioned tenant server was using the same hardcoded dev JWT +secret ('cameleer-dev-jwt-secret-change-in-production'), visible in +source code. An attacker could forge valid JWT tokens for any tenant +server. Now reads from CAMELEER_SAAS_PROVISIONING_JWTSECRET." +``` + +--- + +### Task 2: Add authorization to TenantPortalController admin endpoints + +**Files:** +- Modify: `src/main/java/io/cameleer/saas/portal/TenantPortalController.java` + +- [ ] **Step 1: Add `@PreAuthorize` to team management endpoints** + +Add `@PreAuthorize("hasAuthority('SCOPE_tenant:manage')")` before each of these method annotations: + +Line 76 — `inviteTeamMember`: +```java +@PreAuthorize("hasAuthority('SCOPE_tenant:manage')") +@PostMapping("/team/invite") +``` + +Line 82 — `removeTeamMember`: +```java +@PreAuthorize("hasAuthority('SCOPE_tenant:manage')") +@DeleteMapping("/team/{userId}") +``` + +Line 88 — `changeTeamMemberRole`: +```java +@PreAuthorize("hasAuthority('SCOPE_tenant:manage')") +@PatchMapping("/team/{userId}/role") +``` + +Line 114 — `resetTeamMemberPassword`: +```java +@PreAuthorize("hasAuthority('SCOPE_tenant:manage')") +@PostMapping("/team/{userId}/password") +``` + +Line 176 — `resetTeamMemberMfa`: +```java +@PreAuthorize("hasAuthority('SCOPE_tenant:manage')") +@DeleteMapping("/users/{userId}/mfa") +``` + +- [ ] **Step 2: Add `@PreAuthorize` to server management endpoints** + +Line 95 — `resetServerAdminPassword`: +```java +@PreAuthorize("hasAuthority('SCOPE_tenant:manage')") +@PostMapping("/server/admin-password") +``` + +Line 125 — `restartServer`: +```java +@PreAuthorize("hasAuthority('SCOPE_tenant:manage')") +@PostMapping("/server/restart") +``` + +Line 131 — `upgradeServer`: +```java +@PreAuthorize("hasAuthority('SCOPE_tenant:manage')") +@PostMapping("/server/upgrade") +``` + +- [ ] **Step 3: Add `@PreAuthorize` to CA certificate management endpoints** + +Line 289 — `stageCaCert`: +```java +@PreAuthorize("hasAuthority('SCOPE_tenant:manage')") +@PostMapping("/ca") +``` + +Line 304 — `activateCaCert`: +```java +@PreAuthorize("hasAuthority('SCOPE_tenant:manage')") +@PostMapping("/ca/{id}/activate") +``` + +Line 315 — `deleteCaCert`: +```java +@PreAuthorize("hasAuthority('SCOPE_tenant:manage')") +@DeleteMapping("/ca/{id}") +``` + +- [ ] **Step 4: Commit** + +```bash +git add src/main/java/io/cameleer/saas/portal/TenantPortalController.java +git commit -m "fix(security): add authorization to tenant portal admin endpoints + +All admin-level endpoints (team invite/remove/role, password resets, +server restart/upgrade, CA cert management) were accessible to any +org member including viewers. Now require SCOPE_tenant:manage, +matching the existing pattern on PATCH /auth-settings." +``` + +--- + +### Task 3: Remove dead code endpoints (settings duplicate + password without verification) + +**Files:** +- Modify: `src/main/java/io/cameleer/saas/portal/TenantPortalController.java` +- Modify: `ui/src/api/tenant-hooks.ts` + +- [ ] **Step 1: Remove `PATCH /settings` endpoint from controller** + +Delete lines 238-242 from `TenantPortalController.java`: + +```java +// DELETE THIS ENTIRE BLOCK: + @PatchMapping("/settings") + public ResponseEntity updateSettings(@RequestBody Map updates) { + portalService.updateTenantSettings(updates); + return ResponseEntity.ok().build(); + } +``` + +This is a duplicate of `PATCH /auth-settings` (line 231-235) which has proper `@PreAuthorize`. The frontend uses `useUpdateTenantAuthSettings` which calls `/auth-settings`. + +- [ ] **Step 2: Remove `POST /password` endpoint from controller** + +Delete lines 107-112 from `TenantPortalController.java`: + +```java +// DELETE THIS ENTIRE BLOCK: + @PostMapping("/password") + public ResponseEntity changeOwnPassword(@AuthenticationPrincipal Jwt jwt, + @RequestBody PasswordChangeRequest body) { + portalService.changePassword(jwt.getSubject(), body.password()); + return ResponseEntity.noContent().build(); + } +``` + +The frontend uses `POST /api/account/password` (via `AccountSettingsPage` + `AccountController`) which correctly requires current password verification. + +- [ ] **Step 3: Remove unused hooks from tenant-hooks.ts** + +Remove the `useChangeOwnPassword` hook (lines 124-128): + +```typescript +// DELETE THIS ENTIRE BLOCK: +export function useChangeOwnPassword() { + return useMutation({ + mutationFn: (password) => api.post('/tenant/password', { password }), + }); +} +``` + +Remove the `useUpdateTenantSettings` mutation hook (lines 152-158): + +```typescript +// DELETE THIS ENTIRE BLOCK: +export function useUpdateTenantSettings() { + const qc = useQueryClient(); + return useMutation>({ + mutationFn: (updates) => api.patch('/tenant/settings', updates), + onSuccess: () => qc.invalidateQueries({ queryKey: ['tenant', 'settings'] }), + }); +} +``` + +Note: Keep `useTenantSettings` (the GET query hook at lines 145-150) — the `GET /settings` endpoint returns tenant info (name, slug, tier) and is a legitimate read-only endpoint. + +- [ ] **Step 4: Commit** + +```bash +git add src/main/java/io/cameleer/saas/portal/TenantPortalController.java ui/src/api/tenant-hooks.ts +git commit -m "fix(security): remove unsafe dead code endpoints + +Remove PATCH /api/tenant/settings (duplicate of /auth-settings without +authorization — any org member could disable MFA) and POST +/api/tenant/password (allowed password change without current password +verification). Both were dead code — frontend uses the secure +alternatives. Also remove corresponding unused hooks." +``` + +--- + +### Task 4: Add authorization to TenantController lookup endpoints + +**Files:** +- Modify: `src/main/java/io/cameleer/saas/tenant/TenantController.java` + +- [ ] **Step 1: Add `@PreAuthorize` to getById and getBySlug** + +Line 58 — `getById`: +```java +@GetMapping("/{id}") +@PreAuthorize("hasAuthority('SCOPE_platform:admin')") +public ResponseEntity getById(@PathVariable UUID id) { +``` + +Line 65 — `getBySlug`: +```java +@GetMapping("/by-slug/{slug}") +@PreAuthorize("hasAuthority('SCOPE_platform:admin')") +public ResponseEntity getBySlug(@PathVariable String slug) { +``` + +This matches the existing `@PreAuthorize` on `listAll()` and `create()` in the same controller. These are vendor-only lookup endpoints — no tenant-scoped user should access arbitrary tenant records. + +- [ ] **Step 2: Commit** + +```bash +git add src/main/java/io/cameleer/saas/tenant/TenantController.java +git commit -m "fix(security): restrict tenant lookup endpoints to platform admins + +GET /api/tenants/{id} and GET /api/tenants/by-slug/{slug} were +accessible to any authenticated user, exposing serverEndpoint, +adminEmail, and provisionError. Now require SCOPE_platform:admin, +matching listAll() and create() in the same controller." +``` + +--- + +### Task 5: Verify and build + +- [ ] **Step 1: Run the build to verify all changes compile** + +```bash +cd /c/Users/Hendrik/Documents/projects/cameleer-saas && ./mvnw compile -q +``` + +Expected: BUILD SUCCESS with no compilation errors. + +- [ ] **Step 2: Run frontend type check** + +```bash +cd ui && npm run typecheck +``` + +Expected: No type errors from removed hooks (they were unused). diff --git a/src/main/java/io/cameleer/saas/portal/TenantPortalController.java b/src/main/java/io/cameleer/saas/portal/TenantPortalController.java index 68182ad..12e9754 100644 --- a/src/main/java/io/cameleer/saas/portal/TenantPortalController.java +++ b/src/main/java/io/cameleer/saas/portal/TenantPortalController.java @@ -73,18 +73,21 @@ public class TenantPortalController { return ResponseEntity.ok(portalService.listTeamMembers()); } + @PreAuthorize("hasAuthority('SCOPE_tenant:manage')") @PostMapping("/team/invite") public ResponseEntity> inviteTeamMember(@RequestBody InviteRequest body) { String userId = portalService.inviteTeamMember(body.email(), body.roleId()); return ResponseEntity.ok(Map.of("userId", userId != null ? userId : "")); } + @PreAuthorize("hasAuthority('SCOPE_tenant:manage')") @DeleteMapping("/team/{userId}") public ResponseEntity removeTeamMember(@PathVariable String userId) { portalService.removeTeamMember(userId); return ResponseEntity.noContent().build(); } + @PreAuthorize("hasAuthority('SCOPE_tenant:manage')") @PatchMapping("/team/{userId}/role") public ResponseEntity changeTeamMemberRole(@PathVariable String userId, @RequestBody RoleChangeRequest body) { @@ -92,6 +95,7 @@ public class TenantPortalController { return ResponseEntity.ok().build(); } + @PreAuthorize("hasAuthority('SCOPE_tenant:manage')") @PostMapping("/server/admin-password") public ResponseEntity resetServerAdminPassword(@RequestBody PasswordChangeRequest body) { try { @@ -104,13 +108,7 @@ public class TenantPortalController { } } - @PostMapping("/password") - public ResponseEntity changeOwnPassword(@AuthenticationPrincipal Jwt jwt, - @RequestBody PasswordChangeRequest body) { - portalService.changePassword(jwt.getSubject(), body.password()); - return ResponseEntity.noContent().build(); - } - + @PreAuthorize("hasAuthority('SCOPE_tenant:manage')") @PostMapping("/team/{userId}/password") public ResponseEntity resetTeamMemberPassword(@PathVariable String userId, @RequestBody PasswordChangeRequest body) { @@ -122,12 +120,14 @@ public class TenantPortalController { } } + @PreAuthorize("hasAuthority('SCOPE_tenant:manage')") @PostMapping("/server/restart") public ResponseEntity restartServer() { portalService.restartServer(); return ResponseEntity.noContent().build(); } + @PreAuthorize("hasAuthority('SCOPE_tenant:manage')") @PostMapping("/server/upgrade") public ResponseEntity upgradeServer() { portalService.upgradeServer(); @@ -173,6 +173,7 @@ public class TenantPortalController { return ResponseEntity.noContent().build(); } + @PreAuthorize("hasAuthority('SCOPE_tenant:manage')") @DeleteMapping("/users/{userId}/mfa") public ResponseEntity resetTeamMemberMfa(@PathVariable String userId) { try { @@ -235,12 +236,6 @@ public class TenantPortalController { return ResponseEntity.ok().build(); } - @PatchMapping("/settings") - public ResponseEntity updateSettings(@RequestBody Map updates) { - portalService.updateTenantSettings(updates); - return ResponseEntity.ok().build(); - } - @GetMapping("/{slug}/mfa-policy") public ResponseEntity> getMfaPolicy(@PathVariable String slug) { var tenantOpt = tenantService.getBySlug(slug); @@ -286,6 +281,7 @@ public class TenantPortalController { ); } + @PreAuthorize("hasAuthority('SCOPE_tenant:manage')") @PostMapping("/ca") public ResponseEntity stageCaCert( @RequestParam("cert") MultipartFile certFile, @@ -301,6 +297,7 @@ public class TenantPortalController { } } + @PreAuthorize("hasAuthority('SCOPE_tenant:manage')") @PostMapping("/ca/{id}/activate") public ResponseEntity activateCaCert(@PathVariable UUID id) { try { @@ -312,6 +309,7 @@ public class TenantPortalController { } } + @PreAuthorize("hasAuthority('SCOPE_tenant:manage')") @DeleteMapping("/ca/{id}") public ResponseEntity deleteCaCert(@PathVariable UUID id) { try { diff --git a/src/main/java/io/cameleer/saas/provisioning/DockerTenantProvisioner.java b/src/main/java/io/cameleer/saas/provisioning/DockerTenantProvisioner.java index d130804..162c3df 100644 --- a/src/main/java/io/cameleer/saas/provisioning/DockerTenantProvisioner.java +++ b/src/main/java/io/cameleer/saas/provisioning/DockerTenantProvisioner.java @@ -34,6 +34,9 @@ public class DockerTenantProvisioner implements TenantProvisioner { .responseTimeout(Duration.ofSeconds(30)) .build(); this.docker = DockerClientImpl.getInstance(config, httpClient); + if (props.jwtSecret() == null || props.jwtSecret().isBlank()) { + log.warn("CAMELEER_SAAS_PROVISIONING_JWTSECRET is not set — provisioned servers will fail to start"); + } } @Override @@ -220,7 +223,7 @@ public class DockerTenantProvisioner implements TenantProvisioner { "CAMELEER_SERVER_CLICKHOUSE_PASSWORD=" + props.clickhousePassword(), "CAMELEER_SERVER_TENANT_ID=" + slug, "CAMELEER_SERVER_SECURITY_BOOTSTRAPTOKEN=" + req.licenseToken(), - "CAMELEER_SERVER_SECURITY_JWTSECRET=cameleer-dev-jwt-secret-change-in-production", + "CAMELEER_SERVER_SECURITY_JWTSECRET=" + props.jwtSecret(), "CAMELEER_SERVER_SECURITY_OIDC_ISSUERURI=" + props.oidcIssuerUri(), "CAMELEER_SERVER_SECURITY_OIDC_JWKSETURI=" + props.oidcJwkSetUri(), "CAMELEER_SERVER_SECURITY_OIDC_AUDIENCE=https://api.cameleer.local", diff --git a/src/main/java/io/cameleer/saas/provisioning/ProvisioningProperties.java b/src/main/java/io/cameleer/saas/provisioning/ProvisioningProperties.java index 893b9f4..6efeaa9 100644 --- a/src/main/java/io/cameleer/saas/provisioning/ProvisioningProperties.java +++ b/src/main/java/io/cameleer/saas/provisioning/ProvisioningProperties.java @@ -19,5 +19,6 @@ public record ProvisioningProperties( String clickhousePassword, String oidcIssuerUri, String oidcJwkSetUri, - String corsOrigins + String corsOrigins, + String jwtSecret ) {} diff --git a/src/main/java/io/cameleer/saas/tenant/TenantController.java b/src/main/java/io/cameleer/saas/tenant/TenantController.java index 96fd2b6..c959836 100644 --- a/src/main/java/io/cameleer/saas/tenant/TenantController.java +++ b/src/main/java/io/cameleer/saas/tenant/TenantController.java @@ -56,6 +56,7 @@ public class TenantController { } @GetMapping("/{id}") + @PreAuthorize("hasAuthority('SCOPE_platform:admin')") public ResponseEntity getById(@PathVariable UUID id) { return tenantService.getById(id) .map(entity -> ResponseEntity.ok(toResponse(entity))) @@ -63,6 +64,7 @@ public class TenantController { } @GetMapping("/by-slug/{slug}") + @PreAuthorize("hasAuthority('SCOPE_platform:admin')") public ResponseEntity getBySlug(@PathVariable String slug) { return tenantService.getBySlug(slug) .map(entity -> ResponseEntity.ok(toResponse(entity))) diff --git a/ui/src/api/tenant-hooks.ts b/ui/src/api/tenant-hooks.ts index d5ea2fe..8199da8 100644 --- a/ui/src/api/tenant-hooks.ts +++ b/ui/src/api/tenant-hooks.ts @@ -121,12 +121,6 @@ export function useResetServerAdminPassword() { }); } -export function useChangeOwnPassword() { - return useMutation({ - mutationFn: (password) => api.post('/tenant/password', { password }), - }); -} - export function useResetTeamMemberPassword() { return useMutation({ mutationFn: ({ userId, password }) => @@ -149,14 +143,6 @@ export function useTenantSettings() { }); } -export function useUpdateTenantSettings() { - const qc = useQueryClient(); - return useMutation>({ - mutationFn: (updates) => api.patch('/tenant/settings', updates), - onSuccess: () => qc.invalidateQueries({ queryKey: ['tenant', 'settings'] }), - }); -} - export function useTenantAuditLog(filters: Omit) { const params = new URLSearchParams(); if (filters.action) params.set('action', filters.action);