README.md YAML Security#2562
Conversation
📝 WalkthroughWalkthroughReplaced many README XML examples with YAML (OAuth2, Basic Auth, TLS/SSL, XML/JSON protection, Rate Limiting), added an XML-protection tutorial and a DTD sample, normalized monitoring/example paths, trimmed formatting in one tutorial, and added a roadmap bullet for tutorial tests. Changes
Sequence Diagram(s)(omitted — changes are documentation/tutorial-focused and do not introduce new multi-component control flow) Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related PRs
Suggested labels
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 3✅ Passed checks (3 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. 📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: CHILL Plan: Pro 📒 Files selected for processing (1)
🚧 Files skipped from review as they are similar to previous changes (1)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
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}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @distribution/tutorials/security/90-XML-Protection.yaml:
- Line 1: Update the YAML schema header to reference the released Membrane API
Gateway schema v6.3.10 instead of the nonexistent v7.0.5: replace the current
schema URL string "https://www.membrane-api.io/v7.0.5.json" in the YAML header
comment with "https://www.membrane-api.io/v6.3.10.json" so the
yaml-language-server validates against the correct schema.
🧹 Nitpick comments (2)
README.md (1)
874-908: API Keys example remains in XML while other security sections migrated to YAML.Given the PR title "README.md YAML Security," this section appears inconsistent with the broader migration. The API Keys configuration (lines 881–896) was not modified in this PR. If completeness is a goal, consider migrating this example to YAML as well to align with OAuth2, Basic Auth, and other security features.
This would ensure all security configuration examples follow the same YAML pattern and improve consistency for users scanning the README for security patterns.
distribution/tutorials/security/90-XML-Protection.yaml (1)
2-13: Consider adding expected outputs to enhance tutorial clarity.The tutorial would be more helpful if it documented:
- The expected error response when limits are exceeded (e.g., HTTP status code, error message)
- A third example showing valid XML that successfully passes through (e.g.,
<foo a="1" b="2"/>)This would help users understand both failure and success cases.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (5)
README.mddistribution/tutorials/advanced/10-PathParameters.yamldistribution/tutorials/data/hello-dtd.xmldistribution/tutorials/security/90-XML-Protection.yamldocs/ROADMAP.md
💤 Files with no reviewable changes (1)
- distribution/tutorials/advanced/10-PathParameters.yaml
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (2)
- GitHub Check: Automated tests
- GitHub Check: Analyze (java)
🔇 Additional comments (11)
docs/ROADMAP.md (1)
9-9: Roadmap entry aligns well with PR objectives.The new bullet entry supports the PR's focus on expanding tutorial examples with YAML-based security configurations.
README.md (7)
950-986: YAML OAuth2 configuration is well-structured and documented.The migration from XML to YAML is clear, and the example appropriately documents the LDAP dependency with a fallback note at line 984. The reference to a complete walkthrough at line 986 is helpful.
992-1006: Basic Authentication YAML example is correct and minimal.The structure is straightforward and the hardcoded credentials are clearly documented as an example pattern.
1014-1020: TLS examples are syntactically sound and demonstrate both public and backend use cases.The HTTPS target example (lines 1014–1020) is straightforward. The public endpoint configuration (lines 1024–1036) clearly shows client-to-gateway TLS setup with private key and certificate locations.
Also applies to: 1024-1036
1052-1062: XML Protection configuration aligns with security best practices.The
removeDTD: truesetting is appropriate and matches the intent of the new hello-dtd.xml test file in the tutorials.
1078-1084: JSON Protection example uses global scope, which differs from XML Protection's API scope.Verify that this placement is intentional and correct for the jsonProtection plugin. The limits configured (maxDepth, maxObjectSize, maxArraySize) are reasonable.
1094-1099: Rate Limiting configuration is clear and uses valid ISO 8601 duration syntax.The
PT1Hformat (1 hour) is correct, and the global placement mirrors the JSON Protection example.
1038-1038: Verify that referenced example paths exist in the repository.The README links to example directories at lines 1038, 1193, and 1213. Ensure these paths are valid:
/distribution/examples/security/ssl-tls(line 1038)distribution/examples/monitoring-tracing/prometheus-grafana(line 1193)./distribution/examples/monitoring-tracing/opentelemetry(line 1213)Check that all three directories exist and contain the expected example files referenced in the documentation.
distribution/tutorials/data/hello-dtd.xml (1)
1-3: Test data file is valid and appropriately scoped.The XML file is properly formatted and contains a DOCTYPE definition—exactly the type of input the XML Protection tutorial should demonstrate handling. The file serves a clear purpose in the tutorial workflow for testing DTD removal.
distribution/tutorials/security/90-XML-Protection.yaml (2)
17-20: LGTM! Good security configuration.The
xmlProtectionconfiguration demonstrates solid security practices:
removeDTD: trueprevents XXE attacksmaxAttributeCountandmaxElementNameLengthprovide DoS protection
21-22: Flow structure is correct.The
returnstep will only be reached if the XML passes all protection checks, providing clear success feedback to users.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @README.md:
- Around line 952-987: Replace the non-functional ldapUserDataProvider block
inside the oauth2authserver config with a working staticUserDataProvider entry
(e.g., add staticUserDataProvider: users: - user: username: john password:
password email: john@example.com) and ensure the staticClientList callbackUrl
uses https (change http://localhost:2000/oauth2callback to
https://localhost:2000/oauth2callback or another secure URL) so the example is
functional and avoids unencrypted credential exposure.
🧹 Nitpick comments (4)
README.md (4)
991-1007: Add security warning for hardcoded credentials example.While this example demonstrates basic authentication configuration, the hardcoded plaintext credentials (
password: secret) and lack of TLS on the public endpoint should include a warning that this is for demonstration only. Production deployments should use secure credential storage and require TLS on public-facing APIs.Consider adding a brief disclaimer such as: "This example is for demonstration. In production, use secure credential storage and enable TLS on the public endpoint."
1047-1063: Explain XML Protection behavior on violation.The XML Protection example doesn't specify what happens when limits are exceeded (e.g., when
maxAttributeCountis exceeded). Does the request get rejected, or is the message sanitized? This should be clarified either inline or in the referenced documentation link.Additionally, consider indicating whether these are the only available protection parameters or if others exist.
1069-1085: Resolve inconsistency between documented and shown JSON Protection parameters.Lines 1069-1075 describe several JSON Protection parameters including "String Length", but the configuration example (lines 1079-1085) only shows
maxDepth,maxObjectSize, andmaxArraySize. The text also includes "..." (line 1075), which is vague.Either update the example to show all documented parameters or clarify in the text which parameters are shown versus which are available in the full documentation.
1095-1100: Add brief clarification of ISO 8601 duration format.The
requestLimitDuration: PT1Huses ISO 8601 format, but this might not be immediately obvious to all users. Consider adding a brief inline comment or explanatory text (e.g., "PT1H = 1 hour in ISO 8601 format") to improve clarity for users unfamiliar with this standard.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mddocs/ROADMAP.md
🚧 Files skipped from review as they are similar to previous changes (1)
- docs/ROADMAP.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (1)
README.md (1)
1025-1037: Clarify keystore/truststore configuration and path resolution.The TLS configuration example mixes server certificate and truststore concepts. Line 1034 shows
certificates: - certificate: location: truststore.p12, but it's unclear whether this is:
- A server certificate for the public endpoint, or
- A truststore for validating backend certificates, or
- Something else
Additionally, clarify how file paths (e.g.,
keystore.p12) are resolved relative to the Membrane installation directory. Are they relative to theconffolder?Please clarify the keystore vs. truststore distinction in the TLS example and document the path resolution behavior.
|
This pull request needs "/ok-to-test" from an authorized committer. |
…tency with keystore/truststore conventions
…nto readme-yaml-snippets-security
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Fix all issues with AI agents
In @README.md:
- Around line 953-987: The README example uses ldapUserDataProvider with an
empty map, which is inconsistent with working examples; replace the
ldapUserDataProvider block with a staticUserDataProvider block and define inline
users (e.g., include a users list with user entries containing username,
password, and email) so the minimal OAuth2 authorization server example runs
without an LDAP server; update the configuration symbol ldapUserDataProvider to
staticUserDataProvider and add the users under the users key accordingly while
leaving bearerToken, claims, and scopes as-is.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (2)
README.mddocs/ROADMAP.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
docs/ROADMAP.md (1)
9-9: LGTM. The addition is clear and fits naturally within the roadmap's testing/quality goals.README.md (3)
989-1007: LGTM. Clear, minimal example demonstrating multiple users with basic authentication. YAML structure is correct.
1009-1039: LGTM. Two complementary examples clearly demonstrate TLS configuration: backend TLS and public endpoint TLS. YAML syntax and structure are correct.
1041-1087: All parameters and documentation are accurate. ThexmlProtectionparameters (removeDTD,maxAttributeCount,maxElementNameLength) andjsonProtectionparameters (maxDepth,maxObjectSize,maxArraySize) are correctly documented. The configuration syntax and scope usage (xmlProtection inapi.flow, jsonProtection inglobal) are valid. Documentation links to membrane-api.io are current.
… user configuration
…nto readme-yaml-snippets-security
There was a problem hiding this comment.
Actionable comments posted: 0
🧹 Nitpick comments (1)
README.md (1)
951-987: OAuth2 Authorization Server example is comprehensive but lacks context on referenced files.The YAML configuration is well-structured and demonstrates the key components effectively. However, the example references
logindialogandconsentFile.jsonwithout explaining where these should be located or how they should be structured. This could leave readers unclear about the complete setup.Consider adding brief inline comments or a reference to related configuration or example files that demonstrate the structure of these referenced resources:
api: port: 8000 flow: - oauth2authserver: issuer: http://localhost:8000 location: logindialog # Path to login dialog template consentFile: consentFile.json # Consent configuration file # ... rest of configOr include a link like: "See OAuth2 Authorization Server Example for the complete setup including login dialogs and consent files."
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
README.md
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
- GitHub Check: Analyze (java)
🔇 Additional comments (4)
README.md (4)
993-1043: Basic Authentication and SSL/TLS examples are clear and correct.Both examples demonstrate proper YAML syntax and provide practical configurations. The progression from simple backend TLS (using
https://) to more complex public endpoint TLS with keystores is well-structured.
1045-1091: Verify XML and JSON protection limit defaults are realistic.The examples demonstrate appropriate YAML syntax and reference valid security concerns. However, the configured limits merit validation:
maxAttributeCount: 3(line 1062) may be overly restrictive for real-world XML APIs that use standard attributesmaxDepth: 3(line 1086) may similarly be too shallow for legitimate nested JSON structuresConsider clarifying whether these are intentionally minimal for documentation purposes, or if you should provide separate examples showing more realistic defaults alongside strict/paranoid configurations.
1099-1122: Load Balancing and WebSockets sections remain in XML while Rate Limiting (and most other security examples) now use YAML.The Rate Limiting example uses YAML with correct ISO 8601 duration format (
PT1H= 1 hour), but the Load Balancing configuration immediately following (lines 1110-1122) reverts to XML format. Similarly, WebSockets (lines 1128-1136) use XML.Given the PR focuses on YAML-centric configuration, clarify whether this inconsistency is intentional:
- If Load Balancing should also be in YAML, convert it for consistency
- If XML is intentionally retained for certain features, consider adding a comment explaining why those sections differ
1043-1043: The example directory references in README.md are correct and accessible. All referenced paths exist in the repository:
/distribution/examples/security/ssl-tls✓distribution/examples/monitoring-tracing/prometheus-grafana✓distribution/examples/monitoring-tracing/opentelemetry✓No changes needed.
Likely an incorrect or invalid review comment.
3 participants
Summary by CodeRabbit
Documentation
New Features
Chores
✏️ Tip: You can customize this high-level summary in your review settings.