From dc0fee9a106e4801c1c64b29e12c133812d69619 Mon Sep 17 00:00:00 2001 From: Eduardo Flores Date: Sat, 20 Jun 2026 19:20:53 -0600 Subject: [PATCH] fix(security): harden actuator exposure, data-reset authz, credentials, approver identity, and class-name validation Closes five findings from the repo-wide audit: - Remove `env` from actuator exposure in the base and production profiles. The base value is inherited by every profile that does not override it (including local), so leaving it there leaked resolved properties. - Restrict the destructive POST /data/** reset endpoint to PLUGIN_ADMIN. It is @Profile("!production") so it was reachable by any authenticated user (incl. PLUGIN_VIEWER) in the docker profile. - Drop plain-text credential defaults from the base profile AND the Java field defaults in SecurityProperties. Each profile now supplies its own credentials; a no-profile boot fails fast (password cannot be null) instead of silently running with admin123. The isolated SecurityConfig test now supplies explicit test credentials. - Derive the approver from the authenticated principal in approve/reject instead of trusting the request body. The body field is still accepted (optional) for backward compatibility with the load-plugins scripts and the CI contract test, but its value is ignored. - Validate mainClassName against a fully-qualified class-name pattern on upload, mirroring the existing jobName/version sanitization. --- .../plugins/service/JarUploadService.java | 31 ++++++++++++++++ .../frbatchservice/config/SecurityConfig.java | 3 ++ .../config/SecurityProperties.java | 28 +++++++++----- .../web/JobManagementController.java | 37 ++++++++++++++----- .../application-production.properties | 2 +- .../src/main/resources/application.properties | 16 +++++--- .../DynamicJobLoadingIntegrationTest.java | 6 ++- ...urityConfigPasswordEncoderProfileTest.java | 8 ++++ 8 files changed, 104 insertions(+), 27 deletions(-) diff --git a/fr-batch-service/src/main/java/com/fronzec/frbatchservice/batchjobs/plugins/service/JarUploadService.java b/fr-batch-service/src/main/java/com/fronzec/frbatchservice/batchjobs/plugins/service/JarUploadService.java index 4d20756..bacd12d 100644 --- a/fr-batch-service/src/main/java/com/fronzec/frbatchservice/batchjobs/plugins/service/JarUploadService.java +++ b/fr-batch-service/src/main/java/com/fronzec/frbatchservice/batchjobs/plugins/service/JarUploadService.java @@ -47,6 +47,14 @@ public class JarUploadService { /** Allowed characters in job name and version used as path components. */ private static final Pattern SAFE_NAME = Pattern.compile("[a-zA-Z0-9._\\-]+"); + /** + * Fully-qualified Java class name pattern. + * Matches names like {@code com.fronzec.plugins.ticketpdf.TicketPdfJobPlugin}. + * Requires at least one package segment followed by a simple class name. + */ + private static final Pattern FQCN = + Pattern.compile("([a-zA-Z_$][a-zA-Z0-9_$]*\\.)+[a-zA-Z_$][a-zA-Z0-9_$]*"); + private final JobDefinitionRepository jobDefinitionRepository; private final Optional autoApproveConfig; private final JarSignatureVerifier jarSignatureVerifier; @@ -90,6 +98,7 @@ public JarUploadResponse uploadJar( try { validateJar(file); + validateMainClassName(mainClassName); String checksum = ChecksumUtil.computeSha256(file); @@ -172,6 +181,28 @@ private void validateJar(MultipartFile file) { } } + /** + * Validates that {@code mainClassName} is a fully-qualified Java class name. + * + *

Requires at least one package segment (e.g. {@code com.example.MyClass}). + * A bare simple name ({@code MyClass}) is rejected because a plugin entry-point + * without a package would conflict with the default package and is almost always + * an input error. + * + * @param mainClassName the class name to validate + * @throws InvalidJarException if the value is blank or does not match the FQCN pattern + */ + private void validateMainClassName(String mainClassName) { + if (mainClassName == null || mainClassName.isBlank()) { + throw new InvalidJarException("mainClassName must not be blank"); + } + if (!FQCN.matcher(mainClassName).matches()) { + throw new InvalidJarException( + "mainClassName must be a fully-qualified Java class name " + + "(e.g. com.example.MyClass): " + mainClassName); + } + } + /** Reads up to {@code n} bytes from the beginning of the file. */ private byte[] readHeader(MultipartFile file, int n) { try (InputStream in = file.getInputStream()) { diff --git a/fr-batch-service/src/main/java/com/fronzec/frbatchservice/config/SecurityConfig.java b/fr-batch-service/src/main/java/com/fronzec/frbatchservice/config/SecurityConfig.java index 8939f0e..4314a17 100644 --- a/fr-batch-service/src/main/java/com/fronzec/frbatchservice/config/SecurityConfig.java +++ b/fr-batch-service/src/main/java/com/fronzec/frbatchservice/config/SecurityConfig.java @@ -31,6 +31,7 @@ *

  • {@code GET /jobs/plugins} — public (plugin discovery)
    • *
  • {@code GET /jobs/**} — {@code PLUGIN_VIEWER} or {@code PLUGIN_ADMIN}
    • *
  • {@code POST /jobs/**}, {@code PUT /jobs/**}, {@code DELETE /jobs/**} — {@code PLUGIN_ADMIN}
    • + *
  • {@code POST /data/**} — {@code PLUGIN_ADMIN} (destructive data-reset; not active in production)
    • *
  • {@code /actuator/health} — public
    • * * @@ -64,6 +65,8 @@ public SecurityFilterChain securityFilterChain(HttpSecurity http) throws Excepti .hasAnyRole("PLUGIN_VIEWER", "PLUGIN_ADMIN") .requestMatchers("/jobs/**") .hasRole("PLUGIN_ADMIN") + .requestMatchers("/data/**") + .hasRole("PLUGIN_ADMIN") .anyRequest() .authenticated()) .httpBasic(Customizer.withDefaults()); diff --git a/fr-batch-service/src/main/java/com/fronzec/frbatchservice/config/SecurityProperties.java b/fr-batch-service/src/main/java/com/fronzec/frbatchservice/config/SecurityProperties.java index a203979..e95b2e8 100644 --- a/fr-batch-service/src/main/java/com/fronzec/frbatchservice/config/SecurityProperties.java +++ b/fr-batch-service/src/main/java/com/fronzec/frbatchservice/config/SecurityProperties.java @@ -4,25 +4,35 @@ import org.springframework.boot.context.properties.ConfigurationProperties; /** - * Externalised security credentials from {@code application.properties}. + * Externalised security credentials bound from {@code app.security.*} properties. * - *

    Note: Passwords are stored in plain text ({@code {noop}}) for development. - * Migrate to {@code {bcrypt}} hashes before production deployment. + *

    Passwords have NO Java-level default. Each active profile must supply its own credentials: + *

    + * + *

    This is deliberate: a missing password is fail-safe. With no active profile (a + * misconfiguration) the password is {@code null}, {@link SecurityConfig} fails to build the + * {@code UserDetails} ({@code password cannot be null}), and the app refuses to start rather + * than booting with a known insecure default. Tests that load {@link SecurityConfig} in + * isolation must supply explicit credentials via the context runner's property values. */ @ConfigurationProperties("app.security") public class SecurityProperties { - /** Username for the admin role (PLUGIN_ADMIN). */ + /** Username for the admin role (PLUGIN_ADMIN). Defaults to {@code "admin"}. */ private String adminUser = "admin"; - /** Password for the admin user — plain text in dev, bcrypt hash in production. */ - private String adminPassword = "{noop}admin123"; + /** Password for the admin user. No default — supplied per active profile (fail-safe if absent). */ + private String adminPassword; - /** Username for the viewer role (PLUGIN_VIEWER). */ + /** Username for the viewer role (PLUGIN_VIEWER). Defaults to {@code "viewer"}. */ private String viewerUser = "viewer"; - /** Password for the viewer user — plain text in dev, bcrypt hash in production. */ - private String viewerPassword = "{noop}viewer123"; + /** Password for the viewer user. No default — supplied per active profile (fail-safe if absent). */ + private String viewerPassword; public String getAdminUser() { return adminUser; diff --git a/fr-batch-service/src/main/java/com/fronzec/frbatchservice/web/JobManagementController.java b/fr-batch-service/src/main/java/com/fronzec/frbatchservice/web/JobManagementController.java index bdf29f9..07666e6 100644 --- a/fr-batch-service/src/main/java/com/fronzec/frbatchservice/web/JobManagementController.java +++ b/fr-batch-service/src/main/java/com/fronzec/frbatchservice/web/JobManagementController.java @@ -203,6 +203,12 @@ public ResponseEntity disableDefinition(@PathVariable long id) { * Approves a job definition, setting its status to {@code APPROVED} and recording * the approver and timestamp. * + *

    The approver identity is always derived from the authenticated principal + * (via {@link AuditService#currentUserId()}). Any {@code approvedBy} field + * supplied in the request body is accepted for backward compatibility with existing + * scripts (e.g. {@code load-plugins.hurl}) but is intentionally ignored — it + * cannot be trusted because callers can forge any value. + * *

    Returns {@code 200 OK} with the updated definition on success. * {@code 404 Not Found} if the definition does not exist. * {@code 409 Conflict} if the definition is already approved. @@ -210,7 +216,7 @@ public ResponseEntity disableDefinition(@PathVariable long id) { @PutMapping("/definitions/{id}/approve") public ResponseEntity approveDefinition( @PathVariable long id, - @RequestBody ApprovalRequest request) { + @RequestBody(required = false) ApprovalRequest request) { Optional opt = jobDefinitionRepository.findById(id); if (opt.isEmpty()) { log.warn("Cannot approve — job definition not found: id={}", id); @@ -226,22 +232,25 @@ public ResponseEntity approveDefinition( "message", "Job definition " + id + " is already approved")); } + // Derive the approver from the authenticated principal, not from the request body. + // The body field is kept for backward-compat deserialization but its value is discarded. + String approver = AuditService.currentUserId(); entity.setApprovalStatus("APPROVED"); - entity.setApprovedBy(request.approvedBy()); + entity.setApprovedBy(approver); entity.setApprovedAt(LocalDateTime.now()); JobDefinitionEntity saved = jobDefinitionRepository.save(entity); log.info( "Job definition approved: id={}, jobName={}, approvedBy={}", saved.getId(), saved.getJobName(), - request.approvedBy()); + approver); auditService.logEvent( new AuditEvent( AuditEventType.JOB_APPROVED, saved.getJobName(), - AuditService.currentUserId(), - "Approved by " + request.approvedBy(), + approver, + "Approved by " + approver, AuditEvent.SUCCESS, LocalDateTime.now())); @@ -252,36 +261,44 @@ public ResponseEntity approveDefinition( * Rejects a job definition, setting its status to {@code REJECTED} and recording * the reviewer and timestamp. * + *

    The reviewer identity is always derived from the authenticated principal + * (via {@link AuditService#currentUserId()}). Any {@code approvedBy} field + * supplied in the request body is accepted for backward compatibility but is + * intentionally ignored — it cannot be trusted because callers can forge any value. + * *

    Returns {@code 200 OK} with the updated definition on success. * {@code 404 Not Found} if the definition does not exist. */ @PutMapping("/definitions/{id}/reject") public ResponseEntity rejectDefinition( @PathVariable long id, - @RequestBody ApprovalRequest request) { + @RequestBody(required = false) ApprovalRequest request) { Optional opt = jobDefinitionRepository.findById(id); if (opt.isEmpty()) { log.warn("Cannot reject — job definition not found: id={}", id); return ResponseEntity.notFound().build(); } + // Derive the reviewer from the authenticated principal, not from the request body. + // The body field is kept for backward-compat deserialization but its value is discarded. + String reviewer = AuditService.currentUserId(); JobDefinitionEntity entity = opt.get(); entity.setApprovalStatus("REJECTED"); - entity.setApprovedBy(request.approvedBy()); + entity.setApprovedBy(reviewer); entity.setApprovedAt(LocalDateTime.now()); JobDefinitionEntity saved = jobDefinitionRepository.save(entity); log.info( "Job definition rejected: id={}, jobName={}, rejectedBy={}", saved.getId(), saved.getJobName(), - request.approvedBy()); + reviewer); auditService.logEvent( new AuditEvent( AuditEventType.JOB_REJECTED, saved.getJobName(), - AuditService.currentUserId(), - "Rejected by " + request.approvedBy(), + reviewer, + "Rejected by " + reviewer, AuditEvent.SUCCESS, LocalDateTime.now())); diff --git a/fr-batch-service/src/main/resources/application-production.properties b/fr-batch-service/src/main/resources/application-production.properties index db23ea3..c72a3e7 100644 --- a/fr-batch-service/src/main/resources/application-production.properties +++ b/fr-batch-service/src/main/resources/application-production.properties @@ -7,7 +7,7 @@ # Spring Web Configuration #--------------------- server.servlet.context-path=/api/batch-service -management.endpoints.web.exposure.include=health,info,env,prometheus +management.endpoints.web.exposure.include=health,info,prometheus management.endpoint.health.group.plugins.include=pluginHealthIndicator # Validate that health group members exist (catches misconfiguration) management.endpoint.health.validate-group-membership=true diff --git a/fr-batch-service/src/main/resources/application.properties b/fr-batch-service/src/main/resources/application.properties index 7da0f0c..043bb82 100644 --- a/fr-batch-service/src/main/resources/application.properties +++ b/fr-batch-service/src/main/resources/application.properties @@ -2,7 +2,9 @@ # Spring Web Configuration #--------------------- server.servlet.context-path=/api/batch-service -management.endpoints.web.exposure.include=health,info,env,prometheus +# Do NOT expose `env` here: it dumps resolved properties (incl. credentials) and +# this base value is inherited by any profile that does not override it. +management.endpoints.web.exposure.include=health,info,prometheus management.endpoint.health.group.plugins.include=pluginHealthIndicator #>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> # Jackson Object Mapper Configuration @@ -99,14 +101,16 @@ fr-batch-service.jobs.job2.chunk-size=10 fr-batch-service.jobs.job2.retry.max-attempts=3 fr-batch-service.jobs.job2.retry.backoff-millis=2000 #>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> -# Security Configuration (HTTP Basic, dev plain-text passwords) -# Note: {noop} prefix documents that passwords are plain text. -# Migrate to {bcrypt} hashes before production deployment. +# Security Configuration (HTTP Basic) +# Passwords have NO defaults here — each active profile MUST supply them: +# local: application-local.properties (admin123 / viewer123, noop) +# docker: application-docker.properties (admin123 / viewer123, noop) +# production: application-production.properties (bcrypt hashes via env vars) +# Usernames default to admin/viewer as a convenience; passwords are intentionally +# absent so the app fails to start rather than running with leaked plain-text defaults. #--------------------- app.security.admin-user=admin -app.security.admin-password={noop}admin123 app.security.viewer-user=viewer -app.security.viewer-password={noop}viewer123 #>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>>> # Rest clients configs #--------------------- diff --git a/fr-batch-service/src/test/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoadingIntegrationTest.java b/fr-batch-service/src/test/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoadingIntegrationTest.java index 5eaba6f..72bcb53 100644 --- a/fr-batch-service/src/test/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoadingIntegrationTest.java +++ b/fr-batch-service/src/test/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoadingIntegrationTest.java @@ -236,6 +236,10 @@ void enableDefinition_setsEnabledTrue() throws Exception { @Test @Order(3) void approveDefinition_setsApproved() throws Exception { + // The approvedBy field in the request body is accepted for backward compatibility + // but intentionally ignored — the controller derives the approver from the + // authenticated principal (AuditService.currentUserId()). In the test context + // there is no authenticated user, so the fallback "system" is recorded. mockMvc .perform( put(JOBS_BASE + "/definitions/" + definitionId + "/approve") @@ -243,7 +247,7 @@ void approveDefinition_setsApproved() throws Exception { .content("{\"approved_by\":\"test-user\"}")) .andExpect(status().isOk()) .andExpect(jsonPath("$.approval_status").value("APPROVED")) - .andExpect(jsonPath("$.approved_by").value("test-user")) + .andExpect(jsonPath("$.approved_by").value("system")) .andExpect(jsonPath("$.approved_at").exists()); } diff --git a/fr-batch-service/src/test/java/com/fronzec/frbatchservice/config/SecurityConfigPasswordEncoderProfileTest.java b/fr-batch-service/src/test/java/com/fronzec/frbatchservice/config/SecurityConfigPasswordEncoderProfileTest.java index c040e7a..1f9b1c4 100644 --- a/fr-batch-service/src/test/java/com/fronzec/frbatchservice/config/SecurityConfigPasswordEncoderProfileTest.java +++ b/fr-batch-service/src/test/java/com/fronzec/frbatchservice/config/SecurityConfigPasswordEncoderProfileTest.java @@ -24,9 +24,16 @@ class SecurityConfigPasswordEncoderProfileTest { // SecurityConfig is annotated with @EnableWebSecurity, which imports the // HttpSecurity infrastructure — no SecurityAutoConfiguration needed here. + // SecurityProperties has no Java-level password defaults (fail-safe by design), so the + // isolated web slice must supply explicit credentials or UserDetails building would fail. + private static final String[] TEST_CREDENTIALS = { + "app.security.admin-password={noop}test-admin", "app.security.viewer-password={noop}test-viewer" + }; + private WebApplicationContextRunner runnerWithProfiles(String... profiles) { return new WebApplicationContextRunner() .withUserConfiguration(SecurityConfig.class) + .withPropertyValues(TEST_CREDENTIALS) .withInitializer(ctx -> ctx.getEnvironment().setActiveProfiles(profiles)); } @@ -73,6 +80,7 @@ void defaultProfile_hasSingleNoOpEncoder() { // No profile active at all — the !production NoOp bean must still be the only one. new WebApplicationContextRunner() .withUserConfiguration(SecurityConfig.class) + .withPropertyValues(TEST_CREDENTIALS) .run( context -> { assertThat(context).hasNotFailed();