OpenAPI Validator: sanitize request method logging and remove method from problem details#3033
Draft
predic8 wants to merge 4 commits into
Draft
OpenAPI Validator: sanitize request method logging and remove method from problem details#3033predic8 wants to merge 4 commits into
predic8 wants to merge 4 commits into
Conversation
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 (2)
🚧 Files skipped from review as they are similar to previous changes (1)
📝 WalkthroughWalkthroughThis change renames the non-printable masking helper, updates callers to the new name, and applies masked or masked-and-truncated request values in OpenAPI validation and error reporting. Validation error strings and tests are updated to use the renamed helper. ChangesMasking helper and logging updates
Estimated code review effort: 3 (Moderate) | ~20 minutes Possibly related PRs
Suggested reviewers: Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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
🤖 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/openapi/serviceproxy/OpenAPIInterceptor.java`:
- Around line 134-136: The OpenAPI validation failure logs still write the
request URI unsanitized while only masking the method, so update both the
handleRequest and handleResponse logging paths in OpenAPIInterceptor to pass
exc.getRequest().getUri() through maskNonPrintableCharacters as well. Keep the
existing log structure in the request/response validation blocks, but ensure
both method and URI are sanitized before being interpolated into the
warning/info message.
🪄 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: 016c32c2-ad6d-4548-9a1a-ea920ff9bdb4
📒 Files selected for processing (2)
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.javacore/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java
💤 Files with no reviewable changes (1)
- core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java
…` and introduce `truncateAndMaskNonPrintable` utility
Contributor
There was a problem hiding this comment.
♻️ Duplicate comments (1)
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.java (1)
192-195: 🔒 Security & Privacy | 🟠 Major | ⚡ Quick winURI still logged unmasked in
handleResponse— prior fix only applied tohandleRequest.
exc.getRequest().getUri()is logged raw here while the method is masked. A previous review already flagged this exact pattern in bothhandleRequestandhandleResponse; only thehandleRequestlog statement (Lines 135-139) was fixed. The URI is still attacker-controlled and can carry CRLF/non-printable characters, so log injection remains possible via this response-validation log line.🛡️ Proposed fix
log.info("OpenAPI response validation failed for {} {} against '{}': {}", - maskNonPrintable(exc.getRequest().getMethod()), exc.getRequest().getUri(), + truncateAndMaskNonPrintable(exc.getRequest().getMethod(), 20), + truncateAndMaskNonPrintable(exc.getRequest().getUri()), rec.api.getInfo().getTitle(), errors);🤖 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/openapi/serviceproxy/OpenAPIInterceptor.java` around lines 192 - 195, The response-validation log in OpenAPIInterceptor.handleResponse still prints exc.getRequest().getUri() raw while only the method is masked. Update this log statement to sanitize the URI the same way as handleRequest by applying the existing maskNonPrintable helper (or equivalent shared masking) to the request URI before logging. Keep the log context and reference the handleResponse path so both request and response validation logs consistently avoid attacker-controlled control characters.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.java (1)
136-137: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueInconsistent helper reference style.
Line 136 calls the fully-qualified
StringUtil.truncateAndMaskNonPrintable(...)while line 137 (and thehandleResponselog call) use the statically-imported form. Consider using the static import consistently for readability.♻️ Proposed fix
- StringUtil.truncateAndMaskNonPrintable(exc.getRequest().getMethod(),20), + truncateAndMaskNonPrintable(exc.getRequest().getMethod(), 20), truncateAndMaskNonPrintable(exc.getRequest().getUri()),🤖 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/openapi/serviceproxy/OpenAPIInterceptor.java` around lines 136 - 137, The logging call in OpenAPIInterceptor uses mixed helper reference styles for truncateAndMaskNonPrintable, which is inconsistent with the rest of the class. Update the request log formatting in the relevant log call to use the statically imported truncateAndMaskNonPrintable consistently, matching the handleResponse usage and avoiding the explicit StringUtil. prefix.
🤖 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.
Duplicate comments:
In
`@core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.java`:
- Around line 192-195: The response-validation log in
OpenAPIInterceptor.handleResponse still prints exc.getRequest().getUri() raw
while only the method is masked. Update this log statement to sanitize the URI
the same way as handleRequest by applying the existing maskNonPrintable helper
(or equivalent shared masking) to the request URI before logging. Keep the log
context and reference the handleResponse path so both request and response
validation logs consistently avoid attacker-controlled control characters.
---
Nitpick comments:
In
`@core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.java`:
- Around line 136-137: The logging call in OpenAPIInterceptor uses mixed helper
reference styles for truncateAndMaskNonPrintable, which is inconsistent with the
rest of the class. Update the request log formatting in the relevant log call to
use the statically imported truncateAndMaskNonPrintable consistently, matching
the handleResponse usage and avoiding the explicit StringUtil. prefix.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 084a922f-8ddd-4cda-8622-b2fe4ce7fe75
📒 Files selected for processing (8)
core/src/main/java/com/predic8/membrane/core/interceptor/log/CorrelationIdInterceptor.javacore/src/main/java/com/predic8/membrane/core/openapi/OpenAPIValidator.javacore/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/APIProxy.javacore/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.javacore/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.javacore/src/main/java/com/predic8/membrane/core/transport/http/HttpServerHandler.javacore/src/main/java/com/predic8/membrane/core/util/text/StringUtil.javacore/src/test/java/com/predic8/membrane/core/util/StringUtilTest.java
🚧 Files skipped from review as they are similar to previous changes (1)
- core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.java
…itization and validation logging
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
core/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationError.java (1)
65-68: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMagic number
15for method-length duplicated across call sites.The
15character cap for masked/truncated HTTP methods is repeated here and inValidationErrors.getErrorMessage(and again inOpenAPIInterceptor's logging calls). Extracting it to a shared constant (e.g., inStringUtilor a smallOpenApiLoggingconstants holder) would prevent future drift if the limit needs to change.♻️ Example approach
- return String.format("%s %s %s: %s", - truncateAndMaskNonPrintable(ctx.getMethod(),15), + return String.format("%s %s %s: %s", + truncateAndMaskNonPrintable(ctx.getMethod(), MAX_METHOD_LOG_LENGTH), truncateAndMaskNonPrintable(ctx.getPath()), ctx.getJSONpointer(), message);Define
MAX_METHOD_LOG_LENGTH = 15once (e.g. inStringUtil) and reuse it fromValidationError,ValidationErrors, andOpenAPIInterceptor.🤖 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/openapi/validators/ValidationError.java` around lines 65 - 68, The HTTP method truncation limit is hardcoded in multiple places, so the cap can drift over time. Extract the shared method-length limit used by ValidationError, ValidationErrors.getErrorMessage, and OpenAPIInterceptor into a single constant (for example in StringUtil or a small OpenApiLogging constants holder), then replace each literal 15 with that shared symbol so all logging paths stay consistent.
🤖 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/openapi/validators/ValidationError.java`:
- Around line 65-68: The HTTP method truncation limit is hardcoded in multiple
places, so the cap can drift over time. Extract the shared method-length limit
used by ValidationError, ValidationErrors.getErrorMessage, and
OpenAPIInterceptor into a single constant (for example in StringUtil or a small
OpenApiLogging constants holder), then replace each literal 15 with that shared
symbol so all logging paths stay consistent.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: bfa45e1e-79d9-4aa1-8c50-c22d98151076
📒 Files selected for processing (4)
core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.javacore/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationError.javacore/src/main/java/com/predic8/membrane/core/openapi/validators/ValidationErrors.javacore/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java
🚧 Files skipped from review as they are similar to previous changes (2)
- core/src/main/java/com/predic8/membrane/core/openapi/serviceproxy/OpenAPIInterceptor.java
- core/src/main/java/com/predic8/membrane/core/util/text/StringUtil.java
Member
|
/ok-to-test |
rrayst
requested changes
Jul 3, 2026
| .status(404) | ||
| .addSubSee("not-found") | ||
| .detail("There is no API on the path %s deployed. Please check the path.".formatted(exc.getOriginalRequestUri())) | ||
| .detail("There is no API on the path %s deployed. Please check the path.".formatted(truncateAndMaskNonPrintable(exc.getOriginalRequestUri()))) |
Member
There was a problem hiding this comment.
shouldn't 'escaping/making printable of the details(...) parameter' be a concern of the ProblemDetails message? Grepping the code for .detail( shows several occurences of "attacker-controlled" string parts in the parameter.
Member
There was a problem hiding this comment.
or maybe change the signature to .detail(String messageWithPlaceholders, Object... placeholders) ?
|
This pull request needs "/ok-to-test" from an authorized committer. |
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.
…ssignment
Summary by CodeRabbit