fix(security): hardening quick wins (S1–S5)#78
Merged
Conversation
…s, 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.
Contributor
📝 WalkthroughWalkthroughSeveral security hardening changes are applied to ChangesSecurity Hardening
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
fr-batch-service/src/test/java/com/fronzec/frbatchservice/config/SecurityConfigPasswordEncoderProfileTest.java (1)
79-84: ⚡ Quick winAdd a dedicated fail-fast test for missing passwords.
This test now injects credentials, so it no longer verifies the intended no-profile fail-safe boot failure. Add one explicit negative-path test that omits
app.security.*-passwordand asserts context startup failure.Suggested test addition
+ `@Test` + void defaultProfile_withoutCredentials_failsFast() { + new WebApplicationContextRunner() + .withUserConfiguration(SecurityConfig.class) + .run(context -> { + assertThat(context).hasFailed(); + assertThat(context.getStartupFailure()).hasMessageContaining("password cannot be null"); + }); + }🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@fr-batch-service/src/test/java/com/fronzec/frbatchservice/config/SecurityConfigPasswordEncoderProfileTest.java` around lines 79 - 84, The current test defaultProfile_hasSingleNoOpEncoder() injects credentials via TEST_CREDENTIALS, which prevents it from testing the intended fail-safe behavior when passwords are missing. Add a new dedicated test method that creates a WebApplicationContextRunner with SecurityConfig class but explicitly omits the password property values (do not include TEST_CREDENTIALS or similar), and assert that the application context startup fails when the required app.security.*-password properties are absent.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In
`@fr-batch-service/src/main/java/com/fronzec/frbatchservice/batchjobs/plugins/service/JarUploadService.java`:
- Around line 199-203: The InvalidJarException being thrown in the
JarUploadService contains user-controlled input (mainClassName) directly
embedded in the exception message, which poses a security risk as this message
is later logged by GlobalExceptionHandler and can be exploited for log injection
attacks via CR/LF payloads. Remove the mainClassName variable from the exception
message in the throw statement around lines 201-203, keeping only the
descriptive portion about the fully-qualified Java class name requirement, and
avoid concatenating any user-provided values directly into exception messages.
---
Nitpick comments:
In
`@fr-batch-service/src/test/java/com/fronzec/frbatchservice/config/SecurityConfigPasswordEncoderProfileTest.java`:
- Around line 79-84: The current test defaultProfile_hasSingleNoOpEncoder()
injects credentials via TEST_CREDENTIALS, which prevents it from testing the
intended fail-safe behavior when passwords are missing. Add a new dedicated test
method that creates a WebApplicationContextRunner with SecurityConfig class but
explicitly omits the password property values (do not include TEST_CREDENTIALS
or similar), and assert that the application context startup fails when the
required app.security.*-password properties are absent.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: eb8296c9-f2be-4a1b-86be-a3a9266f2c2e
📒 Files selected for processing (8)
fr-batch-service/src/main/java/com/fronzec/frbatchservice/batchjobs/plugins/service/JarUploadService.javafr-batch-service/src/main/java/com/fronzec/frbatchservice/config/SecurityConfig.javafr-batch-service/src/main/java/com/fronzec/frbatchservice/config/SecurityProperties.javafr-batch-service/src/main/java/com/fronzec/frbatchservice/web/JobManagementController.javafr-batch-service/src/main/resources/application-production.propertiesfr-batch-service/src/main/resources/application.propertiesfr-batch-service/src/test/java/com/fronzec/frbatchservice/batchjobs/plugins/loader/DynamicJobLoadingIntegrationTest.javafr-batch-service/src/test/java/com/fronzec/frbatchservice/config/SecurityConfigPasswordEncoderProfileTest.java
Comment on lines
+199
to
+203
| if (!FQCN.matcher(mainClassName).matches()) { | ||
| throw new InvalidJarException( | ||
| "mainClassName must be a fully-qualified Java class name " | ||
| + "(e.g. com.example.MyClass): " + mainClassName); | ||
| } |
Contributor
There was a problem hiding this comment.
Avoid reflecting raw mainClassName in exception messages.
At Line 201-203, the exception embeds user-controlled input directly. That message is later logged by GlobalExceptionHandler, so CR/LF payloads can forge log lines.
Suggested fix
if (!FQCN.matcher(mainClassName).matches()) {
throw new InvalidJarException(
"mainClassName must be a fully-qualified Java class name "
- + "(e.g. com.example.MyClass): " + mainClassName);
+ + "(e.g. com.example.MyClass)");
}As per coding guidelines, use the established logging framework safely and avoid exposing unsafe user-provided content in error/log paths.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In
`@fr-batch-service/src/main/java/com/fronzec/frbatchservice/batchjobs/plugins/service/JarUploadService.java`
around lines 199 - 203, The InvalidJarException being thrown in the
JarUploadService contains user-controlled input (mainClassName) directly
embedded in the exception message, which poses a security risk as this message
is later logged by GlobalExceptionHandler and can be exploited for log injection
attacks via CR/LF payloads. Remove the mainClassName variable from the exception
message in the throw statement around lines 201-203, keeping only the
descriptive portion about the fully-qualified Java class name requirement, and
avoid concatenating any user-provided values directly into exception messages.
Source: Coding guidelines
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
What
Five security fixes from the repo-wide deep-dive audit, all in
fr-batch-service. Each was reviewed in a fresh adversarial pass before this PR; two follow-up blockers found in that review are included.envfrom actuator exposure (base + production). The base value is inherited by any profile that doesn't override it —localdid not, so/actuator/envleaked resolved properties in the documented dev flow.POST /data/**reset toPLUGIN_ADMIN. It is@Profile("!production"), so it was reachable by any authenticated user (incl.PLUGIN_VIEWER) in thedockerprofile.SecurityProperties. A no-profile boot now fails fast (password cannot be null) instead of silently running withadmin123. The isolatedSecurityConfigtest supplies explicit test credentials.load-plugins.{hurl,sh}and the CI contract test.mainClassNameagainst an FQCN pattern on upload, mirroring the existingjobName/versionsanitization.Verification
./mvnw -B package→ BUILD SUCCESS, 125 tests, 0 failures.contract-testCI job exercises the approve flow end-to-end (it sendsapprovedBy), proving S4 backward-compat.Risk
No-profile startup now fails fast by design (fail-safe). All real profiles (
local,docker,production) supply their own credentials and are unaffected.Summary by CodeRabbit
Release Notes
Security Improvements
Bug Fixes