add McpProtection plugin#2977
Open
christiangoerdes wants to merge 49 commits into
Open
Conversation
… modularity and reusability
…patterns and add corresponding unit tests
…PCValidator, and enhance method null check in Rule
…nd enhance configuration options
… request handling
…pc-protection # Conflicts: # core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParams.java
…or` and add helper method `getJsonVisibleChildElementSpecs`.
…thods, and update references
…proper error handling and add corresponding test.
…idation, and batch size limiting
… parameter validation, and batch size limits
…d update associated logic, tests, and documentation
…ma handling logic
…s and documentation adjustments
…values in examples and responses
…`error` with updated tests and supporting classes
…lace with `schemaValidation` supporting `params`, `response`, and `error` validation. Update tests and tutorials accordingly.
…ince JSON validation is sufficient
…cumentation with descriptions and examples, and update schema handling logic and tests
…h location-based and inline schema examples
… validation logic and tests.
…ith new test cases, and improve conversion handling in ScalarValueConverter.
…ributes` value conversion
…ation, and add Docker run scripts for examples
…rations, refine examples, improve schema validation coverage, and update tests
…schema validation tutorials, restructure tutorial file hierarchy
…response handling, replace Groovy script with static response definitions for improved clarity and maintainability.
…ckend configuration with static success responses and clarify target URL setup.
…request validation, method restrictions, and tool-based access control.
…PC request handling, method restrictions, tool access control, and error scenarios.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (3)
🚧 Files skipped from review as they are similar to previous changes (3)
📝 WalkthroughWalkthroughThis PR introduces JSON-RPC and MCP protection interceptors with comprehensive request/response validation, method allow/deny rules, and optional schema validation. The changes include YAML parsing infrastructure improvements for handling arbitrary-key properties with special characters, JSON schema generation visibility filters to exclude child elements from schemas, and a foundational allow/deny rule framework for configurable authorization. ChangesJSON-RPC and MCP Endpoint Protection
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Suggested labels
Poem
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
|
Collaborator
Author
|
/ok-to-test |
|
This pull request needs "/ok-to-test" from an authorized committer. |
![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: 12
🧹 Nitpick comments (3)
core/src/main/java/com/predic8/membrane/core/util/config/allowdeny/Rule.java (2)
53-57: ⚡ Quick winConsider chaining the PatternSyntaxException as the cause.
Wrapping the
PatternSyntaxExceptionwithout chaining it as a cause loses detailed diagnostic information about why the pattern is invalid (error index, description). Chaining the exception would preserve this information for debugging while keeping the expected error message prefix.♻️ Proposed fix to chain the exception
try { compiledPattern = Pattern.compile(this.pattern); } catch (PatternSyntaxException e) { - throw new ConfigurationException("Invalid regex pattern: " + this.pattern); + throw new ConfigurationException("Invalid regex pattern: " + this.pattern, e); }🤖 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/util/config/allowdeny/Rule.java` around lines 53 - 57, The catch block in Rule that compiles the regex (Pattern.compile(this.pattern)) currently throws a new ConfigurationException without chaining the caught PatternSyntaxException, losing diagnostic details; modify the catch to pass the caught PatternSyntaxException as the cause when constructing the ConfigurationException (i.e., include the exception `e` as the second argument) so the original error index/description is preserved and visible for debugging.
64-72: 💤 Low valueAdd javadoc to deprecated methods explaining the migration path.
The deprecated
setMethod()andgetMethod()methods lack javadoc explaining why they are deprecated and which methods to use instead. Adding@deprecatedjavadoc tags would help users understand the migration path.📝 Suggested javadoc
+ /** + * `@deprecated` Use {`@link` `#setPattern`(String)} instead. + */ `@Deprecated` public void setMethod(String method) { setPattern(method); } + /** + * `@deprecated` Use {`@link` `#getPattern`()} instead. + */ `@Deprecated` public String getMethod() { return getPattern(); }🤖 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/util/config/allowdeny/Rule.java` around lines 64 - 72, Add proper `@deprecated` Javadoc to the deprecated methods setMethod and getMethod explaining they are deprecated in favor of setPattern and getPattern respectively; update the Javadoc block for setMethod to state that callers should use setPattern(String) going forward and for getMethod to state use getPattern() instead, optionally include since/version and a short migration example or note about behavioral parity. Ensure the `@deprecated` tag appears in the standard Javadoc format above each method and mention the replacement method names (setPattern/getPattern) and the version when deprecated.core/src/test/java/com/predic8/membrane/core/interceptor/mcp/MCPProtectionInterceptorTest.java (1)
58-223: ⚡ Quick winAdd explicit coverage for disabled method groups and notification requests.
The matrix never exercises
toolsList: false,toolsCall: false, ornotifications: false, and it also does not cover a request withoutid. That leaves the newisMethodAllowed()false-branches untested and would miss regressions around notification handling.🤖 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/mcp/MCPProtectionInterceptorTest.java` around lines 58 - 223, The test matrix in requestCases() misses branches where toolsList, toolsCall, and notifications are false and also lacks a notification (no "id") request; add additional Stream entries that: 1) use a config string with toolsList: false and assert that a tools/list request is rejected with the appropriate error; 2) use toolsCall: false and assert tools/call requests are rejected (both allowed-name and denied-name paths) to hit isMethodAllowed() false branch; 3) use notifications: false and send a notification-style JSON-RPC request (method present, no "id") to assert correct handling (accepted/rejected per spec); reference the existing helper case builders continues(...) and rejects(...) and the toolsEnabled/toolsCallEnabled constants to add these cases so the false-branches in isMethodAllowed() and notification handling are exercised.
🤖 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
`@annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.java`:
- Around line 144-151: The code currently only checks for zero JSON-visible
children from getJsonVisibleChildElementSpecs(elementInfo) and then uses
visibleChildElementSpecs.getFirst(), which silently drops extra children; change
the validation to require exactly one JSON-visible child by checking
visibleChildElementSpecs.size() == 1 (or equivalent) and throw a
ProcessingException (using the same elementInfo.getElement() context) if the
list is empty or contains more than one, so the generator fails fast when
multiple JSON-visible `@MCChildElement` entries are present.
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/error/LineYamlErrorRenderer.java`:
- Around line 285-343: findLastSegmentStart currently uses lastIndexOf and
misparses dots inside quoted property segments like $.api.methods['rpc.echo'];
replace it with a backward scan that is quote-aware and bracket-aware: iterate
from jsonPath.length()-1 to 0, track bracketDepth (increment on ']' and
decrement on '['), track inSingleQuote when inside brackets (toggle on unescaped
'\''), and when you encounter a '.' with bracketDepth==0 and not inSingleQuote
return its index, or when you close a bracket pair and bracketDepth returns to 0
return the index of that '['; update findLastSegmentStart to use this logic so
getLastSegment()/getParentPath() properly handle quoted property segments (keep
existing helpers
isQuotedPropertySegment/decodeQuotedPropertySegment/getLastSegment unchanged).
In
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ScalarValueConverter.java`:
- Around line 109-121: convertAnySetterValue currently sends any
non-String/Object map value into bind(...), which breaks scalar types (Integer,
Boolean, enums) and silently accepts non-textual nodes for Map<String,String>;
change the logic in convertAnySetterValue (and where you call getMapValueType)
to detect scalar-compatible value types (String, all primitive wrappers/Number
types, Boolean, Character, and enums) and handle them via
SCALAR_MAPPER.convertValue(node, valueType) instead of bind; for String keep
evaluateSpelForString when node.isTextual(), but for non-textual String nodes
either convert via SCALAR_MAPPER or reject/throw so objects/arrays are not
silently treated as empty strings; only call
bind(ctx.updateContext(getElementName(valueType)).addProperty(key), valueType,
node) for actual bean/object types.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCErrorValidation.java`:
- Line 26: Add the missing id attribute to the MCElement annotation on the
JsonRPCErrorValidation class: update the `@MCElement` on class
JsonRPCErrorValidation to include id = "json-rpc-error-validation" (keeping
component = false) so it matches the sibling elements like
JsonRPCParamValidation and JsonRPCResponseValidation for consistent
configuration IDs.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java`:
- Around line 133-143: Add an explicit request-side marker that is set only
after handleRequest(...) has confirmed the request is POST/JSON-RPC-eligible
(e.g., set a boolean property like "JSON_RPC_ELIGIBLE" on the Exchange inside
handleRequest after the eligibility checks), then modify handleResponse(...) to
first check for that marker and skip validation entirely if the marker is
missing; do not fabricate a ResponseValidationContext when the marker is
absent—only construct or use RESPONSE_VALIDATION_CONTEXT (and call
getValidator().validateResponse(...)) when the exchange carries the eligibility
marker so non-JSON-RPC responses are not rewritten.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCSchemaValidation.java`:
- Around line 197-215: createInlineSchemaLocation currently builds syntheticFile
using sanitize(methodName)/sanitize(schemaRole) which is lossy and can cause
different methods to collide; replace the lossy sanitize() use with a
collision-free encoding by encoding methodName and schemaRole as Base64 URL-safe
strings of their UTF-8 bytes (e.g., Base64.getUrlEncoder().withoutPadding()),
use those encoded tokens when forming syntheticFile in
createInlineSchemaLocation (still preserve the "membrane:%s" fallback when
beanBaseLocation is null), and remove or stop calling the sanitize() helper so
registry.getSchema(SchemaLocation.of(...), ...) receives unique synthetic
locations per raw methodName/schemaRole.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java`:
- Around line 169-180: In validateSingleResponse(), ensure error responses are
subject to the same id->request correlation as successful ones: before calling
validateErrorResponse() when response.isError() is true, call
context.methodFor(response.getId()) and if it returns null return
invalidResponse(payloadType, response.getId(), "JSON-RPC response id '%s' does
not match any request.".formatted(response.getId())); otherwise proceed to
validateErrorResponse(); reference the existing methods/variables
validateSingleResponse, validateErrorResponse, context.methodFor,
response.isError, invalidResponse and payloadType.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MCPProtectionInterceptor.java`:
- Around line 116-151: The cached MCPProtectionValidator instance (validator) is
not invalidated when configuration changes via setMethods(MCPProtectionMethods)
or setTools(List<Rule>), so getValidator() continues to return the old policy;
modify setMethods and setTools to clear the cached validator (set validator to
null) after updating the fields so subsequent calls to getValidator() will call
createValidator() and pick up the new configuration; reference the validator
field, setMethods, setTools, getValidator and createValidator when making this
change.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/mcp/MCPProtectionValidator.java`:
- Around line 121-130: The ValidationError currently only carries responseId
(via responseId(JSONRPCRequest)) which conflates parsed notifications and
malformed requests (both become null); update the ValidationError record to
include an explicit boolean (e.g. notification or respond flag) indicating
whether the incoming request was a notification, set that flag in responseId or
where ValidationError is constructed, and then update
MCPProtectionInterceptor.reject() to check that flag and suppress sending a
JSON-RPC error response for true notifications while still sending an error for
non-notifications with id=null; reference the responseId(JSONRPCRequest) method,
the ValidationError record, and MCPProtectionInterceptor.reject() when making
the changes.
In `@core/src/test/resources/json/rpc/error.schema.json`:
- Around line 4-20: The schema currently forces error.data to be present which
conflicts with JSONRPCResponse.error(...) creating new JSONRPCError(..., null)
and `@JsonInclude`(NON_NULL) behavior; update
core/src/test/resources/json/rpc/error.schema.json by removing "data" from the
top-level "required" array so error.data is optional (but keep the nested
"required": ["reason"] inside the data properties so if data exists it must
contain reason); this will make validation compatible with
JSONRPCResponse.error, JSONRPCError, and places like
JsonRPCProtectionInterceptor that may omit data.
In `@distribution/tutorials/security/json-rpc/membrane.cmd`:
- Around line 1-24: The two Windows launcher scripts (membrane.cmd and
run-docker.cmd) currently have LF-only line endings; convert both files to use
CRLF (Windows) line endings so cmd.exe can execute them reliably, then recommit;
ensure you preserve the existing labels and commands (e.g., :search_up, :found,
MEMBRANE_HOME, MEMBRANE_CALLER_DIR and the call to scripts\run-membrane.cmd)
and, if your repo requires, add/update a .gitattributes entry to enforce CRLF
for *.cmd files before committing to prevent future LF-only commits.
In `@distribution/tutorials/security/json-rpc/run-docker.sh`:
- Around line 6-14: The docker container is created with an interactive TTY flag
which conflicts with attaching later via docker start -a; modify the cid
assignment that uses docker create (the line defining cid="$(docker create -it
-p 2000-2010:2000-2010 predic8/membrane:7.2.2 "$@")") to remove the -it option
so the container is created without allocating a TTY, leaving the rest of the
script (docker cp, docker start -a, cleanup/trap) unchanged.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/util/config/allowdeny/Rule.java`:
- Around line 53-57: The catch block in Rule that compiles the regex
(Pattern.compile(this.pattern)) currently throws a new ConfigurationException
without chaining the caught PatternSyntaxException, losing diagnostic details;
modify the catch to pass the caught PatternSyntaxException as the cause when
constructing the ConfigurationException (i.e., include the exception `e` as the
second argument) so the original error index/description is preserved and
visible for debugging.
- Around line 64-72: Add proper `@deprecated` Javadoc to the deprecated methods
setMethod and getMethod explaining they are deprecated in favor of setPattern
and getPattern respectively; update the Javadoc block for setMethod to state
that callers should use setPattern(String) going forward and for getMethod to
state use getPattern() instead, optionally include since/version and a short
migration example or note about behavioral parity. Ensure the `@deprecated` tag
appears in the standard Javadoc format above each method and mention the
replacement method names (setPattern/getPattern) and the version when
deprecated.
In
`@core/src/test/java/com/predic8/membrane/core/interceptor/mcp/MCPProtectionInterceptorTest.java`:
- Around line 58-223: The test matrix in requestCases() misses branches where
toolsList, toolsCall, and notifications are false and also lacks a notification
(no "id") request; add additional Stream entries that: 1) use a config string
with toolsList: false and assert that a tools/list request is rejected with the
appropriate error; 2) use toolsCall: false and assert tools/call requests are
rejected (both allowed-name and denied-name paths) to hit isMethodAllowed()
false branch; 3) use notifications: false and send a notification-style JSON-RPC
request (method present, no "id") to assert correct handling (accepted/rejected
per spec); reference the existing helper case builders continues(...) and
rejects(...) and the toolsEnabled/toolsCallEnabled constants to add these cases
so the false-branches in isMethodAllowed() and notification handling are
exercised.
🪄 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: 52a1cfb4-4797-449f-8609-965108d7790d
📒 Files selected for processing (48)
annot/src/main/java/com/predic8/membrane/annot/generator/JsonSchemaGenerator.javaannot/src/main/java/com/predic8/membrane/annot/generator/kubernetes/model/SchemaObject.javaannot/src/main/java/com/predic8/membrane/annot/model/ChildElementInfo.javaannot/src/main/java/com/predic8/membrane/annot/model/OtherAttributesInfo.javaannot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.javaannot/src/main/java/com/predic8/membrane/annot/yaml/ParsingContext.javaannot/src/main/java/com/predic8/membrane/annot/yaml/error/LineYamlErrorRenderer.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/MethodSetter.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/CollectionBinder.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ScalarValueConverter.javaannot/src/main/java/com/predic8/membrane/annot/yaml/parsing/definition/ComponentDefinitionExtractor.javaannot/src/test/java/com/predic8/membrane/annot/YAMLParsingErrorTest.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/BatchRule.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCErrorValidation.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCInlineSchema.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCMethodSchemas.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCMethodsDefinitions.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParamValidation.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCResponseValidation.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCSchemaValidation.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/SchemaSetter.javacore/src/main/java/com/predic8/membrane/core/interceptor/mcp/MCPProtectionInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/mcp/MCPProtectionMethods.javacore/src/main/java/com/predic8/membrane/core/interceptor/mcp/MCPProtectionValidator.javacore/src/main/java/com/predic8/membrane/core/jsonrpc/JSONRPCRequest.javacore/src/main/java/com/predic8/membrane/core/util/config/allowdeny/Allow.javacore/src/main/java/com/predic8/membrane/core/util/config/allowdeny/Deny.javacore/src/main/java/com/predic8/membrane/core/util/config/allowdeny/Rule.javacore/src/test/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptorTest.javacore/src/test/java/com/predic8/membrane/core/interceptor/mcp/MCPProtectionInterceptorTest.javacore/src/test/resources/json/rpc/echo-params.schema.jsoncore/src/test/resources/json/rpc/echo-result.schema.jsoncore/src/test/resources/json/rpc/error.schema.jsoncore/src/test/resources/json/rpc/generic-rpc-params.schema.jsondistribution/src/test/java/com/predic8/membrane/tutorials/security/JsonRpcAllowDenyAndBatchValidationTutorialTest.javadistribution/src/test/java/com/predic8/membrane/tutorials/security/JsonRpcProtectionTutorialTest.javadistribution/tutorials/README.mddistribution/tutorials/json/README.mddistribution/tutorials/security/json-rpc/10-JSON-RPC-Allow-Deny-and-Batch-Validation.yamldistribution/tutorials/security/json-rpc/20-JSON-RPC-Protection-with-Schema-Validation.yamldistribution/tutorials/security/json-rpc/echo-params.schema.jsondistribution/tutorials/security/json-rpc/echo-result.schema.jsondistribution/tutorials/security/json-rpc/membrane.cmddistribution/tutorials/security/json-rpc/membrane.shdistribution/tutorials/security/json-rpc/run-docker.cmddistribution/tutorials/security/json-rpc/run-docker.sh
💤 Files with no reviewable changes (1)
- distribution/tutorials/json/README.md
Comment on lines
285
to
343
| private static String getParentPath(String jsonPath) { | ||
| // Handle both $.parent.child and $.parent[0] formats | ||
| int lastDot = jsonPath.lastIndexOf('.'); | ||
| int lastBracket = jsonPath.lastIndexOf('['); | ||
| return jsonPath.substring(0, findLastSegmentStart(jsonPath)); | ||
| } | ||
|
|
||
| if (lastBracket > lastDot) { | ||
| // Last segment is array index like [0] | ||
| return jsonPath.substring(0, lastBracket); | ||
| } else { | ||
| // Last segment is object key like .field | ||
| return jsonPath.substring(0, lastDot); | ||
| /** | ||
| * Returns the last part of a JSONPath created by {@link ParsingContext}. | ||
| * Examples: | ||
| * `$.api.methods` -> `methods` | ||
| * `$.api.methods['rpc.echo']` -> `rpc.echo` | ||
| * `$.api.methods[0]` -> `0` | ||
| */ | ||
| private static String getLastSegment(String jsonPath) { | ||
| String segment = jsonPath.substring(findLastSegmentStart(jsonPath)); | ||
|
|
||
| if (isPropertySegment(segment)) { | ||
| return segment.substring(1); | ||
| } | ||
|
|
||
| if (isQuotedPropertySegment(segment)) { | ||
| return decodeQuotedPropertySegment(segment); | ||
| } | ||
|
|
||
| if (isArrayIndexSegment(segment)) { | ||
| return segment.substring(1, segment.length() - 1); | ||
| } | ||
|
|
||
| throw new IllegalArgumentException("Unsupported JSONPath segment: " + segment); | ||
| } | ||
|
|
||
| private static String getLastSegment(String jsonPath) { | ||
| // Handle both $.parent.child and $.parent[0] formats | ||
| int lastDot = jsonPath.lastIndexOf('.'); | ||
| private static boolean isPropertySegment(String segment) { | ||
| return segment.startsWith("."); | ||
| } | ||
|
|
||
| private static boolean isQuotedPropertySegment(String segment) { | ||
| return segment.startsWith("['") && segment.endsWith("']"); | ||
| } | ||
|
|
||
| private static String decodeQuotedPropertySegment(String segment) { | ||
| return segment.substring(2, segment.length() - 2) | ||
| .replace("\\'", "'") | ||
| .replace("\\\\", "\\"); | ||
| } | ||
|
|
||
| private static boolean isArrayIndexSegment(String segment) { | ||
| return segment.startsWith("[") && segment.endsWith("]"); | ||
| } | ||
|
|
||
| private static int findLastSegmentStart(String jsonPath) { | ||
| int lastBracket = jsonPath.lastIndexOf('['); | ||
| int lastDot = jsonPath.lastIndexOf('.'); | ||
|
|
||
| if (lastBracket > lastDot) { | ||
| // Array index like [0] | ||
| String bracket = jsonPath.substring(lastBracket); | ||
| return bracket.substring(1, bracket.length() - 1); // Extract "0" from "[0]" | ||
| } else { | ||
| // Object key like .field | ||
| return jsonPath.substring(lastDot + 1); | ||
| return lastBracket; | ||
| } | ||
| if (lastDot >= 0) { | ||
| return lastDot; | ||
| } | ||
| throw new IllegalArgumentException("Cannot determine parent path of: " + jsonPath); | ||
| } |
Contributor
There was a problem hiding this comment.
Make the JSONPath tail parser quote-aware.
findLastSegmentStart() still uses raw lastIndexOf('.') / lastIndexOf('['), so it splits inside quoted property names that ParsingContext.addProperty(...) now emits. For example, $.api.methods['rpc.echo'] resolves the . inside rpc.echo, which makes getParentPath() / getLastSegment() misparse any error where pc.getKey() == null. In that case getFormattedReport() will either throw or highlight the wrong YAML node instead of rendering the original parsing error.
Suggested fix
private static int findLastSegmentStart(String jsonPath) {
- int lastBracket = jsonPath.lastIndexOf('[');
- int lastDot = jsonPath.lastIndexOf('.');
-
- if (lastBracket > lastDot) {
- return lastBracket;
- }
- if (lastDot >= 0) {
- return lastDot;
- }
- throw new IllegalArgumentException("Cannot determine parent path of: " + jsonPath);
+ boolean inQuotedProperty = false;
+ boolean escaped = false;
+ int lastSegmentStart = -1;
+
+ for (int i = 1; i < jsonPath.length(); i++) {
+ char c = jsonPath.charAt(i);
+
+ if (inQuotedProperty) {
+ if (escaped) {
+ escaped = false;
+ } else if (c == '\\') {
+ escaped = true;
+ } else if (c == '\'') {
+ inQuotedProperty = false;
+ }
+ continue;
+ }
+
+ if (c == '[' && i + 1 < jsonPath.length() && jsonPath.charAt(i + 1) == '\'') {
+ lastSegmentStart = i;
+ inQuotedProperty = true;
+ i++;
+ continue;
+ }
+
+ if (c == '.' || c == '[') {
+ lastSegmentStart = i;
+ }
+ }
+
+ if (lastSegmentStart >= 0) {
+ return lastSegmentStart;
+ }
+ throw new IllegalArgumentException("Cannot determine parent path of: " + jsonPath);
}A small regression test that calls getFormattedReport() with pc.getKey() == null for a path like $.api.methods['rpc.echo'] would lock this down.
🤖 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
`@annot/src/main/java/com/predic8/membrane/annot/yaml/error/LineYamlErrorRenderer.java`
around lines 285 - 343, findLastSegmentStart currently uses lastIndexOf and
misparses dots inside quoted property segments like $.api.methods['rpc.echo'];
replace it with a backward scan that is quote-aware and bracket-aware: iterate
from jsonPath.length()-1 to 0, track bracketDepth (increment on ']' and
decrement on '['), track inSingleQuote when inside brackets (toggle on unescaped
'\''), and when you encounter a '.' with bracketDepth==0 and not inSingleQuote
return its index, or when you close a bracket pair and bracketDepth returns to 0
return the index of that '['; update findLastSegmentStart to use this logic so
getLastSegment()/getParentPath() properly handle quoted property segments (keep
existing helpers
isQuotedPropertySegment/decodeQuotedPropertySegment/getLastSegment unchanged).
Comment on lines
+109
to
+121
| private Object convertAnySetterValue(ParsingContext<?> ctx, Method setter, JsonNode node, String key) { | ||
| Class<?> valueType = getMapValueType(setter); | ||
| if (valueType == null || valueType == Object.class) { | ||
| return SCALAR_MAPPER.convertValue(node, Object.class); | ||
| } | ||
| if (valueType == String.class) { | ||
| return node.isTextual() ? evaluateSpelForString(key, node.asText()) : node.asText(); | ||
| } | ||
| return bind( | ||
| ctx.updateContext(getElementName(valueType)).addProperty(key), | ||
| valueType, | ||
| node | ||
| ); |
Contributor
There was a problem hiding this comment.
Handle scalar @MCOtherAttributes values before falling back to object binding.
This helper now treats every non-String/Object map value as a nested bean and sends it to bind(...). That breaks scalar declarations like Map<String, Integer>, Map<String, Boolean>, and enums, because ObjectBinder.bind(...) expects an object-style node. It also turns non-textual input for Map<String, String> into node.asText() — which is "" for objects/arrays — so malformed YAML can be silently accepted as an empty string instead of being rejected.
Suggested direction
private Object convertAnySetterValue(ParsingContext<?> ctx, Method setter, JsonNode node, String key) {
Class<?> valueType = getMapValueType(setter);
if (valueType == null || valueType == Object.class) {
return SCALAR_MAPPER.convertValue(node, Object.class);
}
if (valueType == String.class) {
- return node.isTextual() ? evaluateSpelForString(key, node.asText()) : node.asText();
+ if (!node.isTextual()) {
+ throw new ConfigurationParsingException(
+ "Invalid value for '%s': expected String, but got %s.".formatted(key, node.getNodeType()),
+ null,
+ ctx == null ? null : ctx.key(key)
+ );
+ }
+ return evaluateSpelForString(key, node.asText());
+ }
+ if (valueType.isEnum() || Number.class.isAssignableFrom(valueType) || valueType == Boolean.class) {
+ return convertScalarOrSpel(node, valueType);
}
return bind(
ctx.updateContext(getElementName(valueType)).addProperty(key),
valueType,
node
);
}🤖 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
`@annot/src/main/java/com/predic8/membrane/annot/yaml/parsing/binding/ScalarValueConverter.java`
around lines 109 - 121, convertAnySetterValue currently sends any
non-String/Object map value into bind(...), which breaks scalar types (Integer,
Boolean, enums) and silently accepts non-textual nodes for Map<String,String>;
change the logic in convertAnySetterValue (and where you call getMapValueType)
to detect scalar-compatible value types (String, all primitive wrappers/Number
types, Boolean, Character, and enums) and handle them via
SCALAR_MAPPER.convertValue(node, valueType) instead of bind; for String keep
evaluateSpelForString when node.isTextual(), but for non-textual String nodes
either convert via SCALAR_MAPPER or reject/throw so objects/arrays are not
silently treated as empty strings; only call
bind(ctx.updateContext(getElementName(valueType)).addProperty(key), valueType,
node) for actual bean/object types.
Comment on lines
+133
to
+143
| public Outcome handleResponse(Exchange exc) { | ||
| if (exc.getResponse() == null || !schemaValidation.hasResponseValidation()) { | ||
| return CONTINUE; | ||
| } | ||
|
|
||
| ResponseValidationContext context = exc.getProperty(RESPONSE_VALIDATION_CONTEXT, ResponseValidationContext.class); | ||
| if (context == null && schemaValidation.hasErrorValidation()) { | ||
| context = new ResponseValidationContext(payloadType(exc.getResponse().getBodyAsStringDecoded()), java.util.Map.of()); | ||
| } | ||
|
|
||
| return rejectResponse(exc, getValidator().validateResponse(exc.getResponse().getBodyAsStringDecoded(), context)); |
Contributor
There was a problem hiding this comment.
Only run response validation for exchanges that actually went through JSON-RPC request handling.
handleRequest() deliberately ignores non-POST traffic, but handleResponse() can still validate and overwrite those responses whenever error validation is enabled because it fabricates a ResponseValidationContext when none was stored. That makes the interceptor rewrite unrelated responses into JSON-RPC 500 errors.
Use an explicit request-side marker (set only after the POST/JSON-RPC eligibility checks pass) and require that marker in handleResponse() before validating.
Suggested fix
private static final ObjectMapper OM = new ObjectMapper();
private static final String RESPONSE_VALIDATION_CONTEXT = JsonRPCProtectionInterceptor.class.getName() + ".responseValidationContext";
+ private static final String JSON_RPC_REQUEST_SEEN = JsonRPCProtectionInterceptor.class.getName() + ".jsonRpcRequestSeen";
@@
public Outcome handleRequest(Exchange exc) {
if (!exc.getRequest().isPOSTRequest()) {
return CONTINUE;
}
@@
if (!exc.getRequest().isJSON()) {
return rejectRequest(exc, new ValidationError(
@@
));
}
+
+ exc.setProperty(JSON_RPC_REQUEST_SEEN, Boolean.TRUE);
RequestValidationResult validation = getValidator().validateRequest(exc.getRequest().getBodyAsStringDecoded());
@@
public Outcome handleResponse(Exchange exc) {
- if (exc.getResponse() == null || !schemaValidation.hasResponseValidation()) {
+ if (exc.getResponse() == null || !schemaValidation.hasResponseValidation()) {
+ return CONTINUE;
+ }
+ if (!Boolean.TRUE.equals(exc.getProperty(JSON_RPC_REQUEST_SEEN, Boolean.class))) {
return CONTINUE;
}🤖 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/json/rpc/JsonRPCProtectionInterceptor.java`
around lines 133 - 143, Add an explicit request-side marker that is set only
after handleRequest(...) has confirmed the request is POST/JSON-RPC-eligible
(e.g., set a boolean property like "JSON_RPC_ELIGIBLE" on the Exchange inside
handleRequest after the eligibility checks), then modify handleResponse(...) to
first check for that marker and skip validation entirely if the marker is
missing; do not fabricate a ResponseValidationContext when the marker is
absent—only construct or use RESPONSE_VALIDATION_CONTEXT (and call
getValidator().validateResponse(...)) when the exchange carries the eligibility
marker so non-JSON-RPC responses are not rewritten.
… validation logic to handle notifications explicitly.
predic8
requested changes
Jun 26, 2026
| * `$.api.methods['rpc.echo']` -> `rpc.echo` | ||
| * `$.api.methods[0]` -> `0` | ||
| */ | ||
| private static String getLastSegment(String jsonPath) { |
| ); | ||
| } | ||
|
|
||
| private static Class<?> getMapValueType(Method setter) { |
| * such as `.` or `'` that JSONPath would otherwise interpret as syntax. | ||
| */ | ||
| private static String toJsonPathProperty(String property) { | ||
| if (property.matches("[A-Za-z_][A-Za-z0-9_]*")) { |
predic8
reviewed
Jun 28, 2026
| private JsonRPCValidator getValidator() { | ||
| if (validator == null) { | ||
| validator = createValidator(); | ||
| } |
Member
There was a problem hiding this comment.
The lazy null-check here is dead code — init() always runs before handleRequest/handleResponse and already sets validator. The defensive guard suggests uncertainty about the lifecycle. Consider dropping it:
private JsonRPCValidator getValidator() {
return validator;
}
predic8
reviewed
Jun 28, 2026
| if (probe == null) { | ||
| return false; | ||
| } | ||
| return compiledPattern != null && compiledPattern.matcher(probe).matches(); |
Member
There was a problem hiding this comment.
The compiledPattern != null guard is defending against a state that can't occur in practice — @Required on setPattern guarantees the pattern is always set before the bean is used. The silent false-return on a null pattern could mask a misconfigured rule and let everything through. Consider dropping the guard and letting it fail fast instead:
return compiledPattern.matcher(probe).matches();
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
New Features
Documentation
Tests