SQL Injection Protection#3031
Merged
Merged
Conversation
Contributor
|
Note Reviews pausedIt looks like this branch is under active development. To avoid overwhelming you with review comments due to an influx of new commits, CodeRabbit has automatically paused this review. You can configure this behavior by changing the Use the following commands to manage reviews:
Use the checkboxes below for quick actions:
📝 WalkthroughWalkthroughAdds CRS-derived SQL injection protection with rule loading, message scanning, interceptor blocking/warning behavior, build-time rule transpilation, tutorial coverage, and documentation plus license attribution updates. ChangesSQL Injection Protection Feature
Estimated code review effort: 4 (Complex) | ~60 minutes Suggested reviewers: Sequence Diagram(s)sequenceDiagram
participant Exchange
participant SqlInjectionProtectionInterceptor
participant SqlInjectionProtection
participant SqlInjectionRuleSet
participant SqlInjectionRule
Exchange->>SqlInjectionProtectionInterceptor: handleRequest / handleResponse
SqlInjectionProtectionInterceptor->>SqlInjectionProtection: scan(message)
SqlInjectionProtection->>SqlInjectionRuleSet: firstMatch(value)
SqlInjectionRuleSet->>SqlInjectionRule: matches(input)
SqlInjectionRule-->>SqlInjectionRuleSet: match result
SqlInjectionRuleSet-->>SqlInjectionProtection: Optional<Detection>
SqlInjectionProtection-->>SqlInjectionProtectionInterceptor: Detection or empty
SqlInjectionProtectionInterceptor-->>Exchange: continue, warn, or blocked response
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 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: 8
🧹 Nitpick comments (1)
distribution/src/main/java/com/predic8/membrane/build/CrsSqliRuleTranspiler.java (1)
151-156: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winFail on unsupported transformations instead of warning.
The generated
transformsarray is later deserialized withTransformation.valueOf(...); warning and still emitting an unknown CRS transform would turn a build-time drift into a runtime startup failure. Make unsupported transforms fatal while generating/checking.Fail the transpilation on unsupported transforms
- warnOnUnsupportedTransforms(id.group(1), actions); + failOnUnsupportedTransforms(id.group(1), actions); ObjectNode rule = MAPPER.createObjectNode(); @@ - private static void warnOnUnsupportedTransforms(String ruleId, String actions) { + private static void failOnUnsupportedTransforms(String ruleId, String actions) { Matcher m = TRANSFORM.matcher(actions); while (m.find()) { String t = m.group(1); if (!t.equals("none") && !SUPPORTED_TRANSFORMS.contains(t)) - System.err.println("WARNING: rule " + ruleId + " uses unsupported transformation '" + t + "'"); + throw new IllegalArgumentException("Rule " + ruleId + " uses unsupported transformation '" + t + "'"); } }Also applies to: 172-178, 191-197
🤖 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 `@distribution/src/main/java/com/predic8/membrane/build/CrsSqliRuleTranspiler.java` around lines 151 - 156, The CRS SQLi rule transpilation currently only warns via warnOnUnsupportedTransforms while still emitting unknown transform names into the generated rule. Update CrsSqliRuleTranspiler to fail fast when unsupported transforms are detected, so the build/check step stops instead of producing invalid output that later breaks Transformation.valueOf(...). Apply this in the transpilation path around the rule construction and in the other referenced rule-generation blocks, using the existing transforms(actions) and warnOnUnsupportedTransforms(...) call sites as the places to replace warning-only behavior with a fatal error.
🤖 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
`@core/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtection.java`:
- Around line 78-80: `scan(...)` and `inspectJson(...)` let malformed JSON
exceptions escape, which can abort the interceptor flow for bodies that only
appear to be JSON. Update `SqlInjectionProtection.scan` to catch JSON parsing
failures around the `message.isJSON()` / `inspectJson(...)` path and fall back
to the normal text inspection or pass-through behavior, using the existing
`scan`, `inspectJson`, and `message.getBodyAsStringDecoded()` flow as the
integration point. Apply the same defensive handling wherever `inspectJson` is
called so invalid JSON never breaks inspection.
- Around line 102-108: The parameter inspection in
SqlInjectionProtection.inspectParams currently checks only parsed query/form
values, leaving attacker-controlled parameter names unscanned. Update
inspectParams to inspect both each entry key and each entry value from
parseQueryString(queryString), using inspect(location, ...) on the parameter
name and preserving the existing Optional<Detection> flow. Keep the change
localized to inspectParams so the same logic covers query and form keys as well
as values.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtectionInterceptor.java`:
- Around line 155-162: Update the Javadoc on
SqlInjectionProtectionInterceptor.setOnDetect so the documented onDetect=block
behavior covers both request and response handling, not just request rejection
with HTTP 400. Make sure the `@MCAttribute` surface description explicitly
mentions that block also rejects detected responses with HTTP 502, while keeping
warn as log-only, so the generated YAML/XML docs reflect the full public
behavior.
- Around line 116-122: The exception handling in
SqlInjectionProtectionInterceptor’s catch block is labeling scanner failures as
a user problem via ProblemDetails.user(...), even though this is a gateway-side
inspection failure. Update the response construction in the
SqlInjectionProtectionInterceptor exception path to use the server/gateway
problem variant instead of user, while keeping the 500 status and existing
exception details. Locate the change in the catch block that logs “Error
inspecting {} for SQL injection” and builds the response with user(...) and
switch that to the appropriate non-user ProblemDetails builder.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtectionInterceptorTest.java`:
- Around line 77-84: The WARN-mode test is using a different SQL injection
payload than the one already verified to trigger detection, which can let the
test pass without exercising the WARN branch. Update warnModeDoesNotBlock() in
SqlInjectionProtectionInterceptorTest to reuse the same proven malicious request
payload as maliciousRequestBlockedWith400(), and keep asserting that
SqlInjectionProtectionInterceptor.handleRequest() returns CONTINUE when
OnDetect.WARN is set.
In `@distribution/pom.xml`:
- Around line 217-235: The generate profile still triggers the sqli drift check
because the `check-sqli-rules` execution in the `distribution/pom.xml` build
runs during `process-classes`. Update the generate profile’s override for the
`check-sqli-rules` execution so it is disabled there by setting its phase to
none, or move the check to a later lifecycle phase; use the `check-sqli-rules`
execution block and the profile-specific override as the exact spots to change.
In
`@distribution/src/main/java/com/predic8/membrane/build/CrsSqliRuleTranspiler.java`:
- Line 60: The chained CRS rule parsing in CrsSqliRuleTranspiler is too
permissive because OPERATOR only matches positive `@rx`, so unsupported chained
predicates like `@streq`, !`@streq`, and !`@rx` end up being emitted as weakened first
rules in the parse/flush logic around the pending rule handling. Update the
transpilation path in CrsSqliRuleTranspiler to detect these unsupported chained
conditions explicitly and either fail fast with a clear error or skip the rule
entirely, rather than letting the first predicate through without its required
second condition.
In `@LICENSE`:
- Around line 222-223: The LICENSE notice is conflating two different locations,
so update the provenance text to distinguish the unmodified CRS source file from
the transpiler that generates the bundled resource. Adjust the wording in the
LICENSE entry to mention the CRS source under distribution/crs-sqli-rules/ and
the transpiler under distribution/src/main/java/com/predic8/membrane/build/,
keeping the attribution precise and traceable.
---
Nitpick comments:
In
`@distribution/src/main/java/com/predic8/membrane/build/CrsSqliRuleTranspiler.java`:
- Around line 151-156: The CRS SQLi rule transpilation currently only warns via
warnOnUnsupportedTransforms while still emitting unknown transform names into
the generated rule. Update CrsSqliRuleTranspiler to fail fast when unsupported
transforms are detected, so the build/check step stops instead of producing
invalid output that later breaks Transformation.valueOf(...). Apply this in the
transpilation path around the rule construction and in the other referenced
rule-generation blocks, using the existing transforms(actions) and
warnOnUnsupportedTransforms(...) call sites as the places to replace
warning-only behavior with a fatal error.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: dc62c35f-4421-4f3c-8056-69fec325b3d1
📒 Files selected for processing (16)
LICENSEcore/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtection.javacore/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtectionInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionRule.javacore/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionRuleSet.javacore/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/Transformation.javacore/src/main/resources/com/predic8/membrane/core/interceptor/sqlinjection/crs-sqli-rules.jsoncore/src/test/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtectionInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtectionTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionRuleSetTest.javadistribution/crs-sqli-rules/README.mddistribution/crs-sqli-rules/REQUEST-942-APPLICATION-ATTACK-SQLI.confdistribution/pom.xmldistribution/src/main/java/com/predic8/membrane/build/CrsSqliRuleTranspiler.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/SqlInjectionProtectionTutorialTest.javadistribution/tutorials/security/100-SQL-Injection-Protection.yaml
|
This pull request needs "/ok-to-test" from an authorized committer. |
…ql-injection-protection
Member
Author
|
@CodeRabbit resume reviews |
Contributor
|
✅ Action performedReviews resumed. |
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 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 `@docs/ROADMAP.md`:
- Line 8: Fix the typo in the roadmap item by changing the misspelled word in
the OAuth2 README task from Udate to Update. Update the text in the roadmap
entry itself so the item reads correctly and remains consistent with the
intended task description.
🪄 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: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 7a848851-b814-4687-b4b0-e9b0271bc53f
📒 Files selected for processing (3)
core/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtectionInterceptor.javadistribution/tutorials/security/100-SQL-Injection-Protection.yamldocs/ROADMAP.md
🚧 Files skipped from review as they are similar to previous changes (2)
- distribution/tutorials/security/100-SQL-Injection-Protection.yaml
- core/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtectionInterceptor.java
…nnotations, and enhance docs
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (2)
core/src/test/java/com/predic8/membrane/core/interceptor/sqlinjection/TransformationTest.java (1)
156-160: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAvoid embedding invisible control characters raw in string literals.
The comments indicate a literal vertical tab (
\u000B) and non-breaking space (\u00A0) sit betweenaandbin these string literals, but they are invisible in the source. Editors, formatters, or VCS whitespace/line-ending normalization could silently strip or alter these bytes, causing the test to pass or fail for the wrong reason without anyone noticing the drift.♻️ Use explicit unicode escapes instead
- assertEquals("ab", removeWhitespace.apply("ab")); // vertical tab - assertEquals("ab", removeWhitespace.apply("a b")); // non-breaking space + assertEquals("ab", removeWhitespace.apply("a\u000Bb")); // vertical tab + assertEquals("ab", removeWhitespace.apply("a\u00A0b")); // non-breaking space🤖 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 `@core/src/test/java/com/predic8/membrane/core/interceptor/sqlinjection/TransformationTest.java` around lines 156 - 160, The test in TransformationTest.removeWhitespace currently embeds invisible control characters directly in string literals, which makes the intent fragile and hard to preserve. Update the assertions in removesVerticalTabAndNbsp to use explicit unicode escape sequences for the vertical tab and non-breaking space instead of raw characters, so the inputs remain stable across editors and normalization.core/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionRuleSet.java (1)
83-88: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winAdd rule context to
Transformation.valueOffailures.If the bundled JSON ever contains a
transformsvalue not present in theTransformationenum (e.g. transpiler/enum drift), this throwsIllegalArgumentExceptionwith no indication of which rule id caused it, surfacing only as a generic startup crash inSqlInjectionProtectionInterceptor.init().♻️ Proposed fix: wrap with rule id for diagnostics
private static `@NotNull` List<Transformation> getTransformations(JsonNode n) { List<Transformation> transforms = new ArrayList<>(); - for (var t : n.path("transforms")) - transforms.add(Transformation.valueOf(t.asText())); + for (var t : n.path("transforms")) { + try { + transforms.add(Transformation.valueOf(t.asText())); + } catch (IllegalArgumentException e) { + throw new IllegalStateException("Unknown transform '" + t.asText() + + "' in rule '" + n.path("id").asText() + "'", e); + } + } return transforms; }🤖 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 `@core/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionRuleSet.java` around lines 83 - 88, The getTransformations method in SqlInjectionRuleSet currently calls Transformation.valueOf directly, so enum drift fails with a generic IllegalArgumentException and no rule context. Update this method to catch the failure while iterating the transforms array and rethrow with the current rule identifier from the surrounding JsonNode so startup errors in SqlInjectionProtectionInterceptor.init() clearly identify which rule and transform caused the problem.
🤖 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.
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionRuleSet.java`:
- Around line 83-88: The getTransformations method in SqlInjectionRuleSet
currently calls Transformation.valueOf directly, so enum drift fails with a
generic IllegalArgumentException and no rule context. Update this method to
catch the failure while iterating the transforms array and rethrow with the
current rule identifier from the surrounding JsonNode so startup errors in
SqlInjectionProtectionInterceptor.init() clearly identify which rule and
transform caused the problem.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/sqlinjection/TransformationTest.java`:
- Around line 156-160: The test in TransformationTest.removeWhitespace currently
embeds invisible control characters directly in string literals, which makes the
intent fragile and hard to preserve. Update the assertions in
removesVerticalTabAndNbsp to use explicit unicode escape sequences for the
vertical tab and non-breaking space instead of raw characters, so the inputs
remain stable across editors and normalization.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 0ef899c8-4e15-4202-8f30-689885ddd192
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtection.javacore/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtectionInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionRuleSet.javacore/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/Transformation.javacore/src/test/java/com/predic8/membrane/core/interceptor/sqlinjection/TransformationTest.javadistribution/tutorials/security/100-SQL-Injection-Protection.yaml
🚧 Files skipped from review as they are similar to previous changes (4)
- distribution/tutorials/security/100-SQL-Injection-Protection.yaml
- core/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/Transformation.java
- core/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtection.java
- core/src/main/java/com/predic8/membrane/core/interceptor/sqlinjection/SqlInjectionProtectionInterceptor.java
Member
|
/ok-to-test |
rrayst
reviewed
Jul 3, 2026
rrayst
requested changes
Jul 3, 2026
rrayst
approved these changes
Jul 4, 2026
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
2 participants
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.
Summary by CodeRabbit
WARN(log-and-continue) andBLOCK(stop processing); detects issues across URI path, query params, form bodies, JSON bodies (with fallback), other text bodies, and responses.