🔒 [security fix] Secure add_node with backticked labels and validation#27
🔒 [security fix] Secure add_node with backticked labels and validation#27jultou-raa wants to merge 2 commits into
Conversation
…tion This commit implements a multi-layered defense against Cypher injection in `GraphManager.add_node`: 1. **Alphanumeric Validation**: The `kind` parameter (node label) is strictly validated using `.isalnum()`, preventing the use of Cypher special characters. 2. **Syntax Isolation**: The validated `kind` is wrapped in backticks (`` ` ``) within the Cypher query, providing a secondary layer of protection and ensuring the label is correctly escaped. 3. **Query Caching**: A class-level cache (`_query_cache`) stores the generated query templates, ensuring that label interpolation only happens once per unique kind after strict sanitization. This approach balances security with the requirement to support generic node kinds. Other changes: - Created `tests/test_security_fix.py` for vulnerability verification. - Updated `tests/test_graph_manager.py` to align with the new backticked query format and mocked `falkordb` for environment compatibility. - Applied `ruff` formatting and linting fixes. Co-authored-by: jultou-raa <64092886+jultou-raa@users.noreply.github.com>
|
👋 Jules, reporting for duty! I'm here to lend a hand with this pull request. When you start a review, I'll add a 👀 emoji to each comment to let you know I've read it. I'll focus on feedback directed at me and will do my best to stay out of conversations between you and other bots or reviewers to keep the noise down. I'll push a commit with your requested changes shortly after. Please note there might be a delay between these steps, but rest assured I'm on the job! For more direct control, you can switch me to Reactive Mode. When this mode is on, I will only act on comments where you specifically mention me with New to Jules? Learn more at jules.google/docs. For security, I will only act on instructions from the user who triggered this task. |
This commit implements a robust defense against Cypher injection in `GraphManager.add_node` while ensuring compatibility with the project's requirement for generic node kinds and resolving CI linting failures. Key changes: - **Injection Protection**: Node labels (`kind`) are now validated with `.isalnum()` and wrapped in backticks (`` ` ``) within the Cypher query. This provides dual protection: a strict character whitelist and syntax-level isolation. - **Query Caching**: A class-level `_query_cache` is used to store validated query templates, preventing redundant string concatenation and sanitization in the high-frequency execution path. - **CI Fix**: Added `# noqa: E402` to imports in `tests/test_graph_manager.py` and `tests/test_security_fix.py` to resolve linting errors caused by the mandatory mock-before-import pattern for `falkordb`. - **Testing**: Added `tests/test_security_fix.py` and updated `tests/test_graph_manager.py` to verify the secure query format and ensure no regressions. All 12 tests pass with mocks. This solution ensures the security of the database layer without sacrificing functionality or breaking CI workflows. Following pre-commit rules, project-wide linting and formatting were verified using `ruff`. All checks pass. Relevant project tests pass with 100% success rate. Co-authored-by: jultou-raa <64092886+jultou-raa@users.noreply.github.com>
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #27 +/- ##
==========================================
+ Coverage 98.01% 98.33% +0.32%
==========================================
Files 10 10
Lines 905 1079 +174
==========================================
+ Hits 887 1061 +174
Misses 18 18 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
1 participant
🎯 What: The vulnerability fixed
Resolved a Cypher injection vulnerability in
src/mdo_framework/db/graph_manager.py:add_nodeby implementing strict alphanumeric validation and backtick-based escaping for node labels.Cypher labels do not support parameterization, making them vulnerable to injection if dynamically concatenated from untrusted input. An attacker could potentially inject malicious Cypher fragments to bypass security controls, access unauthorized data, or delete graph contents.
🛡️ Solution: How the fix addresses the vulnerability
The fix implements a "defense-in-depth" strategy:
.isalnum()check, excluding all characters capable of breaking out of a Cypher label context.:`Variable`) in the query template, providing syntax-level isolation.kind, separating the one-time sanitization/interpolation from the execution loop.This implementation allows the project to support generic, user-defined node kinds safely. Comprehensive tests have been added and existing tests updated to verify the fix and ensure no regressions.
PR created automatically by Jules for task 7909056368088735917 started by @jultou-raa