Replace exec() in responsetime condition with safe operator mapping#1335
Replace exec() in responsetime condition with safe operator mapping#1335Prajwal5755 wants to merge 6 commits intoOWASP:masterfrom
Conversation
|
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:
WalkthroughReplaces exec-based responsetime evaluation with a safe operator-driven approach: adds an Changes
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes 🚥 Pre-merge checks | ✅ 2 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (2 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 |
![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 (1)
nettacker/core/lib/http.py (1)
3-8: Import ordering:operatorshould be placed alphabetically among stdlib imports.The
operatormodule import should come aftercopyand beforerandomto maintain alphabetical ordering enforced byisort.Suggested fix
import asyncio import copy +import operator import random import re import time -import operator🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/core/lib/http.py` around lines 3 - 8, The stdlib imports in nettacker/core/lib/http.py are out of alphabetical order—move the import of operator so it appears after copy and before random to satisfy isort; update the import block (imports: asyncio, copy, operator, random, re, time) leaving everything else unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nettacker/core/lib/http.py`:
- Around line 85-104: Replace the per-call local operators dict with a
module-level constant OPERATORS (UPPER_SNAKE_CASE) defined after imports and use
it instead of recreating operators inside the block; remove the redundant inner
check "if len(operator_value)==2 and operator_value[0] in operators" (since the
outer validation already ensures this), ensure proper PEP 8 spacing around
operators in the lines that assign operator_value and compare, and fix the typo
in the comment from "respnse" to "response"; keep using
response["responsetime"], value and set condition_results["responsetime"]
appropriately on success/failure.
---
Nitpick comments:
In `@nettacker/core/lib/http.py`:
- Around line 3-8: The stdlib imports in nettacker/core/lib/http.py are out of
alphabetical order—move the import of operator so it appears after copy and
before random to satisfy isort; update the import block (imports: asyncio, copy,
operator, random, re, time) leaving everything else unchanged.
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nettacker/core/lib/http.py`:
- Around line 85-86: Guard the value before calling .split(): check that
conditions[condition] is a str (e.g., with isinstance(conditions[condition],
str)) before doing operator_value = conditions[condition].split(); if it isn't a
str, set operator_value = [] or continue so the subsequent check
(len(operator_value) == 2 and operator_value[0] in OPERATORS) won't raise
AttributeError. Update the code around operator_value, conditions and OPERATORS
to handle non-string condition values safely.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nettacker/core/lib/http.py (1)
90-90: Use the already-guarded local value for parsing.Line 90 can use
value_str.split()(from Line 85) instead of re-readingconditions[condition]; it keeps the guard path tighter and avoids redundant lookup.Proposed cleanup
- operator_value = conditions[condition].split() + operator_value = value_str.split()🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/core/lib/http.py` at line 90, The code redundantly re-reads conditions[condition] when parsing operators; use the already-guarded local variable value_str (assigned earlier) instead: replace the use of conditions[condition].split() with value_str.split() so operator_value is derived from the validated local value in the function that handles condition parsing (look for operator_value, value_str, condition in the HTTP parsing logic).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nettacker/core/lib/http.py`:
- Line 94: The if-statement line with
OPERATORS[operator_value[0]](response["responsetime"], value) is too long; to
fix, move the inline comment above the if or wrap the condition across multiple
lines so it stays under 99 chars—update the line in nettacker/core/lib/http.py
around the if that uses OPERATORS, operator_value, response, and value, placing
the comment on its own line or splitting the call into multiple lines (e.g.,
break after OPERATORS[...] and before the parentheses) to satisfy the project's
line-length rules.
---
Nitpick comments:
In `@nettacker/core/lib/http.py`:
- Line 90: The code redundantly re-reads conditions[condition] when parsing
operators; use the already-guarded local variable value_str (assigned earlier)
instead: replace the use of conditions[condition].split() with value_str.split()
so operator_value is derived from the validated local value in the function that
handles condition parsing (look for operator_value, value_str, condition in the
HTTP parsing logic).
There was a problem hiding this comment.
🧹 Nitpick comments (1)
nettacker/core/lib/http.py (1)
85-90: Redundant dictionary access: usevalue_strinstead of re-accessingconditions[condition].Line 85 assigns
value_str = conditions[condition], but line 90 accessesconditions[condition].split()again instead of using the already-validatedvalue_str. This is both redundant and inconsistent with the type guard pattern above it.♻️ Proposed fix
if condition == "responsetime": value_str = conditions[condition] if not isinstance(value_str, str): condition_results["responsetime"] = [] continue - operator_value = conditions[condition].split() + operator_value = value_str.split() if len(operator_value) == 2 and operator_value[0] in OPERATORS:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/core/lib/http.py` around lines 85 - 90, The code redundantly re-reads conditions[condition] after validating it; change the operator_value assignment to use the already-validated local variable value_str (i.e., set operator_value = value_str.split()) in the function where value_str, condition_results and operator_value are used (look for the block that assigns value_str = conditions[condition] and then computes operator_value) so the type guard is consistent and the extra dictionary access is removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@nettacker/core/lib/http.py`:
- Around line 85-90: The code redundantly re-reads conditions[condition] after
validating it; change the operator_value assignment to use the already-validated
local variable value_str (i.e., set operator_value = value_str.split()) in the
function where value_str, condition_results and operator_value are used (look
for the block that assigns value_str = conditions[condition] and then computes
operator_value) so the type guard is consistent and the extra dictionary access
is removed.
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
nettacker/modules/scan/slow_endpoint.yaml (1)
44-54: Response conditions are correctly formatted.The
responsetime: "> 3"format correctly matches the new safe operator-based parsing introduced innettacker/core/lib/http.py. Thestatus_codeandurlconditions use the expected nested structure withregexandreversekeys.Minor observation: The
urlcondition withregex: .*will match any URL, making it effectively a pass-through. If this is intentional (to ensure the URL field exists), it's fine. Otherwise, you could remove it sincecondition_type: andwith fewer conditions achieves the same result.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/modules/scan/slow_endpoint.yaml` around lines 44 - 54, The response conditions block uses the new operator-based parsing correctly (responsetime: "> 3") and nested regex structure for status_code and url; if the url: regex: .* entry is only intended to assert presence it can stay, otherwise remove the url condition from the response block to avoid a no-op rule—update the response section (look for the response block and keys responsetime, status_code, url, and condition_type) to either document the intent for url: regex: .* or delete that url condition to simplify the AND condition set.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@nettacker/modules/scan/slow_endpoint.yaml`:
- Around line 38-42: The schema/port mismatch in slow_endpoint.yaml (keys schema
and ports) causes false positives because schema is "http" while ports includes
443; update the YAML to either add "https" to the schema list and ensure HTTPS
requests set ssl: true or are handled conditionally for ports == 443, or remove
port 443 from the ports list if only plain HTTP scanning (schema: "http") is
intended—adjust the handling around the schema and ssl flag used by the scanner
accordingly.
---
Nitpick comments:
In `@nettacker/modules/scan/slow_endpoint.yaml`:
- Around line 44-54: The response conditions block uses the new operator-based
parsing correctly (responsetime: "> 3") and nested regex structure for
status_code and url; if the url: regex: .* entry is only intended to assert
presence it can stay, otherwise remove the url condition from the response block
to avoid a no-op rule—update the response section (look for the response block
and keys responsetime, status_code, url, and condition_type) to either document
the intent for url: regex: .* or delete that url condition to simplify the AND
condition set.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 785085af-5ada-47b9-975a-533ce2ce7302
📒 Files selected for processing (1)
nettacker/modules/scan/slow_endpoint.yaml
| schema: | ||
| - "http" | ||
| ports: | ||
| - 80 | ||
| - 443 |
There was a problem hiding this comment.
Protocol/port mismatch will cause false positives.
The schema is set to only "http" while ports includes 443. HTTP requests to port 443 will fail or timeout because port 443 expects HTTPS/TLS connections. This will likely trigger false positives since the connection failures or TLS handshake timeouts may exceed 3 seconds and be incorrectly flagged as "slow endpoints."
Consider either:
- Adding
"https"to the schema list and adjustingssl: truefor HTTPS requests, or - Removing port
443from the ports list if only HTTP scanning is intended.
🔧 Proposed fix: Add HTTPS schema
schema:
- "http"
+ - "https"
ports:
- 80
- 443Note: If adding HTTPS, you may also want to set ssl: true or handle it conditionally for HTTPS requests.
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@nettacker/modules/scan/slow_endpoint.yaml` around lines 38 - 42, The
schema/port mismatch in slow_endpoint.yaml (keys schema and ports) causes false
positives because schema is "http" while ports includes 443; update the YAML to
either add "https" to the schema list and ensure HTTPS requests set ssl: true or
are handled conditionally for ports == 443, or remove port 443 from the ports
list if only plain HTTP scanning (schema: "http") is intended—adjust the
handling around the schema and ssl flag used by the scanner accordingly.
|
Hi @Prajwal5755, thanks for working on this this is an important fix 👍 I noticed this PR addresses the same root cause as the issue I recently opened (#1408), which also proposes replacing the exec()-based evaluation in response_conditions_matched() with a safe operator mapping. To avoid duplicate work and review conflicts, it might be helpful to open an issue before submitting a PR for core engine changes, especially in shared components like http.py. That makes it easier to coordinate fixes and prevents multiple PRs targeting the same code path at the same time. In this case both approaches are valid, and it may make sense to merge the ideas from both PRs into a single fix (for example, using the operator module while keeping the additional guards) |
Hi, thanks for pointing this out and for opening issue #1408. I wasn’t aware of the issue earlier, but I see both approaches are addressing the same root cause. In my PR I’ve also replaced I agree it’s good to align solutions and avoid duplication. I’m open to making changes if needed based on maintainer feedback. Thanks for the suggestion, I’ll make sure to check for existing issues before submitting similar changes going forward. |
|
2 participants
Proposed change
This PR replaces the use of 'exec()' in the 'responsetime' condition exvaluation inside 'response_conditions_matched()' function with a safe operator mapping using Python's built-in 'operator' module
Previously, dynamic code execution was used to evaluate comparision operators, which:
This change:
More importantly no functional behavior has been changed.
Type of change
Checklist
make pre-commit, it didn't generate any changesmake test, all tests passed locally