add JsonRPCProtection plugin#2963
Conversation
|
/ok-to-test |
|
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 a JsonRPCProtectionInterceptor with regex-based Allow/Deny rules, batch controls, per-method JSON Schema param validation, a validator returning structured ValidationError records, public JSONRPCRequest factory exposure, tests and test schemas, plus a YAML introspector change to hide excluded child elements. ChangesJSON-RPC Request Protection
YAML Introspector
Sequence DiagramsequenceDiagram
participant Client as Client (HTTP POST)
participant Interceptor as JsonRPCProtectionInterceptor
participant Validator as JsonRPCValidator
participant RuleEngine as Rule matcher
Client->>Interceptor: POST body with JSON-RPC payload
Interceptor->>Validator: validate(body)
Validator->>Validator: parse payload (single or batch)
Validator->>RuleEngine: for each request -> matches(method) & permits()
alt Validation error or unauthorized
Validator->>Interceptor: ValidationError
Interceptor->>Client: HTTP 200 with JSON-RPC error (single or array)
else All requests valid
Interceptor->>Client: CONTINUE (forward request)
end
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related PRs
Suggested labels
Suggested reviewers
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 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 |
|
This pull request needs "/ok-to-test" from an authorized committer. |
… modularity and reusability
There was a problem hiding this comment.
Actionable comments posted: 3
🤖 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/json/rpc/JsonRPCProtectionInterceptor.java`:
- Around line 103-106: The setter JsonRPCProtectionInterceptor.setParams should
guard against a null argument to avoid config-time NPEs: update
setParams(Map<String,String> params) to check if the incoming params is null
(and simply return) before calling this.params.putAll(...); also ensure the
instance field this.params is initialized (e.g., non-null default) so putAll is
safe when a non-null map is provided.
- Around line 80-88: The response currently includes the RuntimeException
message (e.getMessage()) in JsonRPCProtectionInterceptor when building the
ValidationError for createErrorResponse; remove the exception message from the
client-facing error and replace it with a generic string (e.g. "Invalid JSON-RPC
payload") while keeping the original exception logged server-side
(log.debug/log.warn) for diagnostics; update the code in
JsonRPCProtectionInterceptor (the catch(RuntimeException e) block that calls
createErrorResponse(new ValidationError(...))) to stop interpolating
e.getMessage() into the ValidationError and instead pass the generic message.
In `@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Rule.java`:
- Around line 15-16: The matches method in Rule (public boolean matches(String
method)) can NPE when method is null; update it to return false if method is
null before using methodPattern.matcher(method). Concretely, in Rule.matches add
a null-check for the method parameter (e.g., if (method == null) return false)
and then keep the existing methodPattern != null &&
methodPattern.matcher(method).matches() logic so null method names are treated
as non-matches.
🪄 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: f2a0e788-43d7-4e66-9f95-bedf3c9bcba0
📒 Files selected for processing (6)
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Allow.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/BatchRule.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Deny.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Rule.javacore/src/test/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptorTest.java
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (2)
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java (1)
46-58: 💤 Low valueConsider caching the
JsonRPCValidatorinstance.Line 57 creates a new
JsonRPCValidatoron every request. SincebatchRuleandrulesare configuration-time immutable, the validator could be instantiated once (lazily afterinit()or after setters are called) and reused, avoiding per-request allocation overhead.🤖 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 46 - 58, The handleRequest method is creating a new JsonRPCValidator for every request; cache a single JsonRPCValidator instance in JsonRPCProtectionInterceptor (e.g., a private volatile field like jsonRpcValidator) and instantiate it once using the immutable batchRule and rules either in init() or via a synchronized/lazy initializer after setters are called; then have handleRequest call jsonRpcValidator.validate(body) instead of new JsonRPCValidator(...). Ensure thread-safety when initializing the cached JsonRPCValidator.core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java (1)
123-140: 💤 Low valueDefault-allow semantics when no rule matches.
If no rule matches the method (including when
rulesis empty), validation returnsnullallowing the request through. This "first-match-wins with implicit allow" design requires users to add a catch-allDeny method=".*"rule to enforce deny-by-default.Consider documenting this behavior clearly, or alternatively returning an error when a method matches no rules (deny-by-default), which is typically safer for security-critical APIs.
🤖 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/JsonRPCValidator.java` around lines 123 - 140, The current validateMethod in JsonRPCValidator returns null (allows) when no Rule in rules matches, causing implicit allow-by-default; change to deny-by-default by returning a ValidationError (same form as the existing denial) when the loop completes without a match: construct and return new ValidationError(payloadType, request, 403, ERR_METHOD_NOT_FOUND, "JSON-RPC method '%s' is not allowed.".formatted(request.getMethod())) instead of null; keep existing behavior where a matching rule that permits() returns null and a matching denying rule returns the same ValidationError so only the "no match" path is altered.
🤖 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/json/rpc/JsonRPCValidator.java`:
- Around line 49-65: In JsonRPCValidator, the catch blocks for
JsonProcessingException and RuntimeException currently embed exception messages
into the ValidationError returned to clients; change both catch handlers to
return a generic client-facing message like "Invalid JSON-RPC payload"
(preserving payload type via getPayloadType(body) or PayloadType.SINGLE) and
move the detailed exception text into server-side logging (e.g., log the
exception in the catch before returning the ValidationError). Update the
JsonProcessingException handler (which now uses e.getOriginalMessage()) and the
RuntimeException handler (which now uses e.getMessage()) to avoid leaking
internal details by returning the generic message and logging the exception
object instead.
- Around line 142-144: JsonRPCValidator.parseRequest currently serializes the
JsonNode to String then calls JSONRPCRequest.parse, causing unnecessary
round-trip parsing; change JSONRPCRequest by exposing its private
fromNode(JsonNode) as package-private or add a public/static parse(JsonNode)
overload, then update JsonRPCValidator.parseRequest to call that new
fromNode/parse(JsonNode) method instead of serializing to String so the JsonNode
is parsed directly; ensure method names referenced are
JSONRPCRequest.fromNode(JsonNode) (now non-private) and
JsonRPCValidator.parseRequest(JsonNode).
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java`:
- Around line 46-58: The handleRequest method is creating a new JsonRPCValidator
for every request; cache a single JsonRPCValidator instance in
JsonRPCProtectionInterceptor (e.g., a private volatile field like
jsonRpcValidator) and instantiate it once using the immutable batchRule and
rules either in init() or via a synchronized/lazy initializer after setters are
called; then have handleRequest call jsonRpcValidator.validate(body) instead of
new JsonRPCValidator(...). Ensure thread-safety when initializing the cached
JsonRPCValidator.
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java`:
- Around line 123-140: The current validateMethod in JsonRPCValidator returns
null (allows) when no Rule in rules matches, causing implicit allow-by-default;
change to deny-by-default by returning a ValidationError (same form as the
existing denial) when the loop completes without a match: construct and return
new ValidationError(payloadType, request, 403, ERR_METHOD_NOT_FOUND, "JSON-RPC
method '%s' is not allowed.".formatted(request.getMethod())) instead of null;
keep existing behavior where a matching rule that permits() returns null and a
matching denying rule returns the same ValidationError so only the "no match"
path is altered.
🪄 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: fc6a7617-e385-4fae-9153-310a89927817
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java
…patterns and add corresponding unit tests
…PCValidator, and enhance method null check in Rule
There was a problem hiding this comment.
Actionable comments posted: 1
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java (1)
68-76:⚠️ Potential issue | 🟡 Minor | ⚡ Quick winPreserve batch payload type in the runtime fallback.
The method already computes the payload type before parsing, but this branch hardcodes
PayloadType.SINGLE. A runtime failure during batch validation will therefore be serialized as a single JSON-RPC error object instead of a batch error array.🐛 Minimal fix
} catch (RuntimeException e) { log.debug("Invalid JSON-RPC payload.", e); return new ValidationError( - PayloadType.SINGLE, + getPayloadType(body), null, 400, ERR_INVALID_REQUEST, "Invalid JSON-RPC payload" );🤖 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/JsonRPCValidator.java` around lines 68 - 76, The runtime-catch in JsonRPCValidator currently returns a ValidationError with PayloadType.SINGLE, which ignores the precomputed payload type and causes batch payloads to be serialized as a single error; update the catch to use the previously computed payloadType (the variable calculated before parsing) when constructing the ValidationError so batch requests produce a batch error array instead of a single JSON-RPC error object.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java (1)
44-49: ⚡ Quick winAvoid initializing param schemas twice during interceptor startup.
init()callsparams.init(...), thencreateValidator()does the same work again. That doubles schema resolution/compilation for every configured mapping at startup.♻️ Minimal cleanup
`@Override` public void init() { super.init(); - params.init(router.getResolverMap(), router.getConfiguration().getUriFactory(), getBeanBaseLocation()); validator = createValidator(); }Also applies to: 111-113
🤖 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 44 - 49, The params schema is being initialized twice: once in init() via params.init(router.getResolverMap(), router.getConfiguration().getUriFactory(), getBeanBaseLocation()) and again inside createValidator(); remove the duplicate by choosing one location to perform params.init — e.g., keep params.init(...) in init() and update createValidator() to assume params are already initialized (remove or guard the call there), or alternatively move params.init(...) into createValidator() and delete it from init(); ensure validator is still assigned by createValidator() and that no other code path calls params.init() redundantly.
🤖 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/json/rpc/JsonRPCParams.java`:
- Around line 43-52: The current `@MCOtherAttributes` Map<String,String> params in
JsonRPCParams (and its setter setParams) exposes regex keys as XML attribute
names which is invalid; fix by switching to an explicit child-element model:
replace the Map params with a List<Param> where Param is a small POJO (fields
e.g. String method, String schema) and annotate the list/Param with the XML
element annotations (e.g. `@MCElement/`@MCAttribute on Param) so config becomes
<param method="regex" schema="..."/>; update JsonRPCParams#get/set to use
List<Param> and migrate any code that referenced the Map to build a runtime map
from that list. Alternatively, if preserving the Map is required, add validation
in setParams(Map) to detect illegal XML attribute characters in keys (e.g. '^',
'*', '\\', spaces, etc.) and throw a clear IllegalArgumentException explaining
regex keys are not allowed in XML.
---
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.java`:
- Around line 68-76: The runtime-catch in JsonRPCValidator currently returns a
ValidationError with PayloadType.SINGLE, which ignores the precomputed payload
type and causes batch payloads to be serialized as a single error; update the
catch to use the previously computed payloadType (the variable calculated before
parsing) when constructing the ValidationError so batch requests produce a batch
error array instead of a single JSON-RPC error object.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.java`:
- Around line 44-49: The params schema is being initialized twice: once in
init() via params.init(router.getResolverMap(),
router.getConfiguration().getUriFactory(), getBeanBaseLocation()) and again
inside createValidator(); remove the duplicate by choosing one location to
perform params.init — e.g., keep params.init(...) in init() and update
createValidator() to assume params are already initialized (remove or guard the
call there), or alternatively move params.init(...) into createValidator() and
delete it from init(); ensure validator is still assigned by createValidator()
and that no other code path calls params.init() redundantly.
🪄 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: 7a3a04b8-3f23-4f8f-8c76-446e54504db9
📒 Files selected for processing (8)
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParams.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptor.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCValidator.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Rule.javacore/src/main/java/com/predic8/membrane/core/jsonrpc/JSONRPCRequest.javacore/src/test/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptorTest.javacore/src/test/resources/json/rpc/echo-params.schema.jsoncore/src/test/resources/json/rpc/generic-rpc-params.schema.json
✅ Files skipped from review due to trivial changes (2)
- core/src/test/resources/json/rpc/echo-params.schema.json
- core/src/test/resources/json/rpc/generic-rpc-params.schema.json
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/Rule.java
|
Actionable comments posted: 0 |
…nd enhance configuration options
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParams.java (1)
107-113: 💤 Low valueConsider creating SchemaRegistry once and reusing it for all schemas.
createSchemaRegistry(resolver)is invoked insideloadSchema()for each configured method pattern. Since the registry configuration is identical across all entries, creating it once ininit()and passing it toloadSchema()would reduce initialization overhead.♻️ Proposed refactor
public void init(Resolver resolver, URIFactory uriFactory, String beanBaseLocation) { List<Param> effectiveMappings = getEffectiveMappings(); if (effectiveMappings.isEmpty()) { schemas = List.of(); return; } if (resolver == null || uriFactory == null) { throw new ConfigurationException("Cannot initialize JSON-RPC param schemas without resolver context."); } + SchemaRegistry schemaRegistry = createSchemaRegistry(resolver); schemas = effectiveMappings.stream() .map(entry -> new CompiledSchema( entry.getMethod(), compilePattern(entry.getMethod()), - loadSchema(entry.getMethod(), entry.getSchema(), resolver, uriFactory, beanBaseLocation) + loadSchema(entry.getMethod(), entry.getSchema(), schemaRegistry, uriFactory, beanBaseLocation) )) .toList(); }And update
loadSchemasignature accordingly:- private static Schema loadSchema(String methodPattern, String schemaPath, Resolver resolver, URIFactory uriFactory, String beanBaseLocation) { + private static Schema loadSchema(String methodPattern, String schemaPath, SchemaRegistry schemaRegistry, URIFactory uriFactory, String beanBaseLocation) { // ... - Schema schema = createSchemaRegistry(resolver).getSchema( + Schema schema = schemaRegistry.getSchema(Also applies to: 129-148
🤖 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/JsonRPCParams.java` around lines 107 - 113, The code repeatedly calls createSchemaRegistry(resolver) inside loadSchema(...) for every entry; instead create the SchemaRegistry once in init() (e.g., SchemaRegistry schemaRegistry = createSchemaRegistry(resolver)), store it in a local variable, and pass that same schemaRegistry into loadSchema(...) when building CompiledSchema in the effectiveMappings stream and in the other loop (the second block around lines 129-148). Update loadSchema's signature to accept SchemaRegistry (remove internal createSchemaRegistry call) and ensure both call sites (the mapping -> new CompiledSchema(...) creation and the other loadSchema usages) use the shared schemaRegistry to avoid recreating identical registries per entry.
🤖 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/json/rpc/JsonRPCParams.java`:
- Around line 107-113: The code repeatedly calls createSchemaRegistry(resolver)
inside loadSchema(...) for every entry; instead create the SchemaRegistry once
in init() (e.g., SchemaRegistry schemaRegistry =
createSchemaRegistry(resolver)), store it in a local variable, and pass that
same schemaRegistry into loadSchema(...) when building CompiledSchema in the
effectiveMappings stream and in the other loop (the second block around lines
129-148). Update loadSchema's signature to accept SchemaRegistry (remove
internal createSchemaRegistry call) and ensure both call sites (the mapping ->
new CompiledSchema(...) creation and the other loadSchema usages) use the shared
schemaRegistry to avoid recreating identical registries per entry.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: e7c0039a-7550-423c-ba05-306d03760700
📒 Files selected for processing (3)
annot/src/main/java/com/predic8/membrane/annot/yaml/McYamlIntrospector.javacore/src/main/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCParams.javacore/src/test/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptorTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/test/java/com/predic8/membrane/core/interceptor/json/rpc/JsonRPCProtectionInterceptorTest.java
… 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
| if (segment.startsWith(".")) { | ||
| return segment.substring(1); | ||
| } | ||
|
|
| } | ||
|
|
||
| if (segment.startsWith("['") && segment.endsWith("']")) { | ||
| return segment.substring(2, segment.length() - 2) |
| } | ||
|
|
||
| if (segment.startsWith("[") && segment.endsWith("]")) { | ||
| return segment.substring(1, segment.length() - 1); |
| throw unsupported(wanted, key, node); | ||
| } | ||
|
|
||
| private Object convertAnySetterValue(ParsingContext<?> ctx, Method setter, JsonNode node, String key) { |
| return path; | ||
| } | ||
|
|
||
| private static String toJsonPathProperty(String property) { |
| * <p>In YAML, the entries are written as a map under <code>schemaValidation.methods</code>.</p> | ||
| */ | ||
| @MCElement(name = "methods", component = false, id = "json-rpc-method-definitions") | ||
| public class JsonRPCMethodDefinitions { |
| * to validate successful upstream responses.</p> | ||
| */ | ||
| @MCElement(name = "method", component = false, id = "json-rpc-method-schema") | ||
| public class JsonRPCSchemas { |
| Map<String, Schema> resolvedParamSchemas = new LinkedHashMap<>(); | ||
| Map<String, Schema> resolvedResponseSchemas = new LinkedHashMap<>(); | ||
|
|
||
| for (Map.Entry<String, JsonRPCSchemas> entry : methods.getMethods().entrySet()) { |
Summary by CodeRabbit
New Features
Tests