Test fixes #1291
Test fixes #1291
Conversation
WalkthroughThe pull request introduces SQL injection prevention infrastructure with a new Changes
Sequence DiagramsequenceDiagram
participant Test as Test Code
participant Builder as SafeConditionBuilder
participant Validator as Validation Layer
participant Parser as Parsing Layer
Test->>Builder: build_safe_condition(condition_string)
Builder->>Parser: Parse condition string
alt Compound Condition
Parser->>Parser: Split by logical operators
loop Each clause
Parser->>Validator: Validate column/operator
Validator-->>Parser: ✓ Valid
Parser->>Parser: Bind parameter
end
else Single Condition
Parser->>Validator: Validate column/operator
Validator-->>Parser: ✓ Valid
Parser->>Parser: Bind parameter
end
Parser-->>Builder: Safe SQL + Parameters
Builder-->>Test: (sql_string, param_dict)
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 inconclusive)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests (beta)
Comment |
|
This fixes tests. All tests passing in devcontainer. Should pass when other PR is merged. |
![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
🧹 Nitpick comments (6)
front/php/server/db.php (1)
31-34: Path normalization looks correct; trailing-slash guarantee is helpfulNormalizing
$dbFolderPathand$logFolderPathto always end with'/'avoids subtle path-join issues and works correctly with the rest of the file. Given that they are now normalized, the extrartrim(..., '/') . '/'when constructing$DBFILEand$DBFILE_LOCKED_FILEis redundant; in a follow-up you could simplify to:- $DBFILE = rtrim($dbFolderPath, '/') . '/app.db'; + $DBFILE = $dbFolderPath . 'app.db'; - $DBFILE_LOCKED_FILE = rtrim($logFolderPath, '/') . '/db_is_locked.log'; + $DBFILE_LOCKED_FILE = $logFolderPath . 'db_is_locked.log';The current implementation is functionally fine if you prefer to keep the extra defensive trimming.
Also applies to: 46-49
test/backend/test_safe_builder_unit.py (1)
94-97: Prefer catchingValueErrorinstead of bareExceptioninbuild_safe_condition
_parse_conditiononly raisesValueErrorfor bad input, so catching a bareExceptionhere will also hide programming errors (e.g., TypeError) and re-wrap them asValueError, which is what Ruff’s BLE001 warns about. You can keep the public behaviour while tightening the handler:- try: - return self._parse_condition(condition_string) - except Exception: - raise ValueError(f"Invalid condition format: {condition_string}") + try: + return self._parse_condition(condition_string) + except ValueError: + raise ValueError(f"Invalid condition format: {condition_string}")This keeps invalid user input mapped to
ValueErrorwithout masking unexpected exceptions.install/production-filesystem/services/scripts/cron_script.sh (1)
3-10: Simplifiedcron_restart_backendhandling; note that all matching entries are now removedThe new flow (
/services/start-backend.sh &followed bysed -i '/cron_restart_backend/d' "${LOG_EXECUTION_QUEUE}") is much simpler than the prior temporary-file logic and should be easier to maintain. One behavioural change is that everycron_restart_backendline in the queue file will be deleted on each run; if the queue can legitimately contain multiple restart requests that you intend to process separately, you may want to only delete a single occurrence per execution.test/backend/sql_safe_builder.py (2)
421-447:pattern1currently rejects values containing single quotes (even after{s-quote}expansion)In
_parse_single_condition,pattern1uses([^\']*)for the value:pattern1 = r"^\s*(\w+)\s+(=|!=|<>|<|>|<=|>=|LIKE|NOT\s+LIKE)\s+\'([^\']*)\'\s*$"Because this forbids any internal
', conditions like:AND devName = 'O{s-quote}Connor'will be sanitized to include a real
'and then fail to match, ending up as “unsupported condition”. If you want this builder to support legitimate values containing quotes (while still parameterizing them), consider switching to a non‑greedy group similar to the test helper:- pattern1 = r"^\s*(\w+)\s+(=|!=|<>|<|>|<=|>=|LIKE|NOT\s+LIKE)\s+\'([^\']*)\'\s*$" + pattern1 = r"^\s*(\w+)\s+(=|!=|<>|<|>|<=|>=|LIKE|NOT\s+LIKE)\s+\'(.+?)\'\s*$"This keeps everything parameterized but stops rejecting values solely because they contain a single quote.
171-175: Broadexcept Exceptioninbuild_safe_condition/build_conditionmay obscure real bugsBoth
build_safe_conditionandbuild_conditioncatch a bareException—either rewrapping it as aValueErroror returning("", {}). That gives you a fail‑closed API for invalid conditions, but it also masks unexpected programmer errors (e.g., TypeError) as simple format issues, which is what BLE001/B904 are warning about.If you want clearer failure modes while staying safe, consider something along these lines:
- try: - return self._parse_condition(condition_string) - except Exception: - raise ValueError(f"Invalid condition format: {condition_string}") + try: + return self._parse_condition(condition_string) + except ValueError as exc: + raise ValueError(f"Invalid condition format: {condition_string}") from excand, in
build_condition, narrowing the handler to the specific exceptions you expect (or letting unexpected ones surface) instead of catching allException. That way invalid user input still fails closed, but genuine bugs aren’t silently downgraded.Also applies to: 592-593
test/backend/test_sql_injection_prevention.py (1)
143-143: Prefix unused unpacked variables with underscore.The
paramsvariable is unpacked but never used in these test functions. Consider prefixing it with an underscore to indicate it's intentionally unused.Apply this pattern to silence the linter warnings:
- condition, params = builder.get_safe_condition_legacy(malicious_input) + condition, _params = builder.get_safe_condition_legacy(malicious_input)Based on static analysis hints.
Also applies to: 154-154, 209-209
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (7)
front/php/server/db.php(2 hunks)install/production-filesystem/services/scripts/cron_script.sh(1 hunks)install/production-filesystem/services/start-crond.sh(1 hunks)test/api_endpoints/test_nettools_endpoints.py(1 hunks)test/backend/sql_safe_builder.py(1 hunks)test/backend/test_safe_builder_unit.py(2 hunks)test/backend/test_sql_injection_prevention.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (2)
test/backend/sql_safe_builder.py (1)
test/backend/test_safe_builder_unit.py (2)
_parse_condition(99-118)_build_simple_condition(120-143)
test/backend/test_sql_injection_prevention.py (2)
test/backend/test_compound_conditions.py (1)
builder(22-24)test/backend/sql_safe_builder.py (3)
SafeConditionBuilder(15-659)get_safe_condition_legacy(639-659)build_condition(549-602)
🪛 Ruff (0.14.4)
test/backend/test_safe_builder_unit.py
96-96: Do not catch blind exception: Exception
(BLE001)
test/backend/sql_safe_builder.py
22-46: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
49-63: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
66-66: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
69-76: Mutable class attributes should be annotated with typing.ClassVar
(RUF012)
173-173: Do not catch blind exception: Exception
(BLE001)
174-174: Within an except clause, raise exceptions with raise ... from err or raise ... from None to distinguish them from errors in exception handling
(B904)
174-174: Avoid specifying long messages outside the exception class
(TRY003)
300-300: Avoid specifying long messages outside the exception class
(TRY003)
447-447: Avoid specifying long messages outside the exception class
(TRY003)
455-455: Avoid specifying long messages outside the exception class
(TRY003)
458-458: Avoid specifying long messages outside the exception class
(TRY003)
461-461: Avoid specifying long messages outside the exception class
(TRY003)
482-482: Avoid specifying long messages outside the exception class
(TRY003)
485-485: Avoid specifying long messages outside the exception class
(TRY003)
492-492: Avoid specifying long messages outside the exception class
(TRY003)
516-516: Avoid specifying long messages outside the exception class
(TRY003)
519-519: Avoid specifying long messages outside the exception class
(TRY003)
592-592: Do not catch blind exception: Exception
(BLE001)
test/backend/test_sql_injection_prevention.py
143-143: Unpacked variable params is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
154-154: Unpacked variable params is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
209-209: Unpacked variable params is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
212-212: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
⏰ 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: docker_dev
🔇 Additional comments (6)
install/production-filesystem/services/start-crond.sh (1)
26-29: crond log-level change is consistent between log message and executionAdding
-l 1to both the echoed command and the actual/usr/sbin/crondinvocation keeps them aligned while lowering log verbosity. As long as your targetcrondimplementation supports-l, there’s no behavioural regression introduced here.test/backend/test_safe_builder_unit.py (1)
9-13: Import narrowing toMockonly is appropriateSwitching to
from unittest.mock import Mockremoves an unusedpatchimport and doesn’t change test behaviour.test/api_endpoints/test_nettools_endpoints.py (1)
150-157: Feature-flaggingnormalanddetailnmap modes is a good way to keep default runs fastWrapping the
"normal"and"detail"cases inpytest.param(..., marks=pytest.mark.feature_complete)cleanly separates the slower scans from the default parametrization while still allowing them to run when explicitly selected. Just ensure thefeature_completemarker is declared in your pytest configuration (e.g., inpytest.inior equivalent) to avoidPytestUnknownMarkWarning.test/backend/sql_safe_builder.py (1)
21-109: Whitelist + sanitization + parameter binding design looks solid for test purposesThe use of explicit
ALLOWED_*whitelists,_sanitize_string(control-character stripping and whitespace normalization with{s-quote}handling), and parameter binding in the_build_*helpers gives this test-sideSafeConditionBuildera conservative, fail-closed behaviour suitable for exercising SQL-injection defences. I don’t see functional issues in this initialization/sanitization layer.test/backend/test_sql_injection_prevention.py (2)
18-18: Import fix successfully addresses the NameError.The addition of this import statement resolves the previously failing test suite, as stated in the PR objectives.
21-24: Well-structured pytest fixture.The fixture correctly provides a fresh
SafeConditionBuilderinstance for each test, following pytest best practices.
| condition, params = builder.get_safe_condition_legacy(malicious_input) | ||
| # Should handle case variations safely | ||
| if "union" in condition.lower() or "select" in condition.lower(): | ||
| assert False, f"Injection not blocked: {malicious_input}" |
There was a problem hiding this comment.
🛠️ Refactor suggestion | 🟠 Major
Replace assert False with pytest.fail() for reliability.
Using assert False is problematic because it can be removed when running Python with optimization (python -O), potentially causing tests to pass when they should fail.
Apply this diff:
if "union" in condition.lower() or "select" in condition.lower():
- assert False, f"Injection not blocked: {malicious_input}"
+ pytest.fail(f"Injection not blocked: {malicious_input}")Based on static analysis hints.
📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| assert False, f"Injection not blocked: {malicious_input}" | |
| if "union" in condition.lower() or "select" in condition.lower(): | |
| pytest.fail(f"Injection not blocked: {malicious_input}") |
🧰 Tools
🪛 Ruff (0.14.4)
212-212: Do not assert False (python -O removes these calls), raise AssertionError()
Replace assert False
(B011)
🤖 Prompt for AI Agents
In test/backend/test_sql_injection_prevention.py around line 212, replace the
inplace "assert False, f\"Injection not blocked: {malicious_input}\"" with a
call to pytest.fail(f"Injection not blocked: {malicious_input}") to prevent the
check from being skipped under optimized Python; also ensure pytest is imported
at the top of the test file (add "import pytest" if missing).
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
test/backend/test_sql_injection_prevention.py (1)
143-143: Consider using underscore prefix for intentionally unused variables.Lines 143, 154, and 209 unpack the
paramsvariable but never use it. While this doesn't affect test correctness, using_paramsor_signals the value is intentionally ignored.Based on static analysis hints.
Example for line 143:
- condition, params = builder.get_safe_condition_legacy(input_with_placeholder) + condition, _params = builder.get_safe_condition_legacy(input_with_placeholder)Apply similar changes to lines 154 and 209.
Also applies to: 154-154, 209-209
📜 Review details
Configuration used: CodeRabbit UI
Review profile: CHILL
Plan: Pro
📒 Files selected for processing (1)
test/backend/test_sql_injection_prevention.py(2 hunks)
🧰 Additional context used
🧬 Code graph analysis (1)
test/backend/test_sql_injection_prevention.py (2)
test/backend/test_compound_conditions.py (1)
builder(22-24)test/backend/sql_safe_builder.py (3)
SafeConditionBuilder(15-659)get_safe_condition_legacy(639-659)build_condition(549-602)
🪛 Ruff (0.14.5)
test/backend/test_sql_injection_prevention.py
143-143: Unpacked variable params is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
154-154: Unpacked variable params is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
209-209: Unpacked variable params is never used
Prefix it with an underscore or any other dummy variable pattern
(RUF059)
⏰ 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: docker_dev
🔇 Additional comments (2)
test/backend/test_sql_injection_prevention.py (2)
11-11: LGTM! Import and fixture setup are correct.The addition of
import pytestand thefrom sql_safe_builder import SafeConditionBuilderimport successfully resolve theNameErrormentioned in the PR objectives. The pytest fixture pattern is properly implemented.Also applies to: 18-18, 21-24
27-233: Comprehensive test coverage for SQL injection prevention.The test suite provides thorough coverage of SQL injection attack vectors including:
- Single quotes, UNION, OR 1=1 attacks
- Nested SELECT, hex encoding, comment injection
- Null bytes, case variations, time-based attacks
- Stacked queries and invalid columns/operators
The tests correctly validate both rejection of malicious input (returning empty conditions) and proper parameterization of valid input.
| if "union" in condition.lower() or "select" in condition.lower(): | ||
| if "union" in condition.lower() or "select" in condition.lower(): | ||
| self.fail(f"Injection not blocked: {malicious_input}") | ||
|
|
||
| def test_time_based_injection_attempt(self): | ||
| """Test that time-based injection attempts are blocked.""" | ||
| malicious_input = "AND IF(1=1, SLEEP(5), 0)" | ||
| condition, params = self.builder.get_safe_condition_legacy(malicious_input) | ||
|
|
||
| # Should return empty condition when SQL functions detected | ||
| self.assertEqual(condition, "") | ||
| self.assertEqual(params, {}) | ||
|
|
||
| def test_stacked_queries_attempt(self): | ||
| """Test that stacked query attempts are blocked.""" | ||
| malicious_input = "'; INSERT INTO admin VALUES ('hacker', 'password'); --" | ||
| condition, params = self.builder.get_safe_condition_legacy(malicious_input) | ||
|
|
||
| # Should return empty condition when semicolon detected | ||
| self.assertEqual(condition, "") | ||
| self.assertEqual(params, {}) | ||
|
|
||
|
|
||
| if __name__ == '__main__': | ||
| # Run the tests | ||
| unittest.main(verbosity=2) No newline at end of file | ||
| pytest.fail(f"Injection not blocked: {malicious_input}") |
There was a problem hiding this comment.
Remove duplicate condition statement.
Lines 211-212 contain identical if statements, which creates incorrect control flow. Line 212 appears to be leftover code from the refactoring that replaced assert False with pytest.fail().
Apply this diff to remove the duplicate:
# Should handle case variations safely
if "union" in condition.lower() or "select" in condition.lower():
- if "union" in condition.lower() or "select" in condition.lower():
- pytest.fail(f"Injection not blocked: {malicious_input}")
+ pytest.fail(f"Injection not blocked: {malicious_input}")📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| if "union" in condition.lower() or "select" in condition.lower(): | |
| if "union" in condition.lower() or "select" in condition.lower(): | |
| self.fail(f"Injection not blocked: {malicious_input}") | |
| def test_time_based_injection_attempt(self): | |
| """Test that time-based injection attempts are blocked.""" | |
| malicious_input = "AND IF(1=1, SLEEP(5), 0)" | |
| condition, params = self.builder.get_safe_condition_legacy(malicious_input) | |
| # Should return empty condition when SQL functions detected | |
| self.assertEqual(condition, "") | |
| self.assertEqual(params, {}) | |
| def test_stacked_queries_attempt(self): | |
| """Test that stacked query attempts are blocked.""" | |
| malicious_input = "'; INSERT INTO admin VALUES ('hacker', 'password'); --" | |
| condition, params = self.builder.get_safe_condition_legacy(malicious_input) | |
| # Should return empty condition when semicolon detected | |
| self.assertEqual(condition, "") | |
| self.assertEqual(params, {}) | |
| if __name__ == '__main__': | |
| # Run the tests | |
| unittest.main(verbosity=2) | |
| \ No newline at end of file | |
| pytest.fail(f"Injection not blocked: {malicious_input}") | |
| if "union" in condition.lower() or "select" in condition.lower(): | |
| pytest.fail(f"Injection not blocked: {malicious_input}") |
🤖 Prompt for AI Agents
In test/backend/test_sql_injection_prevention.py around lines 211 to 213 there
are two identical if statements checking for "union" or "select" in
condition.lower(); remove the duplicated if (the leftover from refactor) so only
a single check remains that calls pytest.fail(f"Injection not blocked:
{malicious_input}") when the condition matches.
2 participants
This PR addresses several test-related issues that were preventing proper test execution and optimizes test performance by marking slower tests appropriately.
Changes Made
1. Fixed SQL Injection Prevention Test Import Error
from sql_safe_builder import SafeConditionBuilderNameError: name 'SafeConditionBuilder' is not defined2. Optimized Nmap Endpoint Tests
@pytest.mark.feature_complete--mark feature_complete. This speeds up the regular test suite while maintaining full test coverage for CI/CD pipelines.Testing
Impact
This PR ensures the test suite is functional and optimized for both development workflow and CI/CD requirements.
Summary by CodeRabbit
Tests