security fixes by removing the unnecessary use of "exec" in the codebase#1525
Open
pUrGe12 wants to merge 15 commits intoOWASP:masterfrom
Open
security fixes by removing the unnecessary use of "exec" in the codebase#1525pUrGe12 wants to merge 15 commits intoOWASP:masterfrom
pUrGe12 wants to merge 15 commits intoOWASP:masterfrom
Conversation
Contributor
Summary by CodeRabbit
WalkthroughReplaces exec/dynamic code in utils with deterministic helpers and allowlists: adds Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~25 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 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}
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nettacker/core/utils/common.py (1)
322-342:⚠️ Potential issue | 🟡 MinorStrip whitespace from interceptor names before the allowlist check.
interceptors.split(",")preserves any surrounding whitespace from the YAML value (e.g.,"generate_and_replace_md5, other"→["generate_and_replace_md5", " other"]), so a benign configuration style that previously "just worked" (or failed silently underexec) now hard-fails withValueError: Interceptor ' other' is not allowed. A.strip()avoids this footgun without loosening the allowlist.🛠 Proposed fix
- if interceptors: - interceptors = interceptors.split(",") + if interceptors: + interceptors = [i.strip() for i in interceptors.split(",") if i.strip()]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/core/utils/common.py` around lines 322 - 342, The interceptors list may contain surrounding whitespace after interceptors.split(",") causing ALLOWED_INTERCEPTORS lookups to fail; update the processing in common.py so that after checking if interceptors is truthy you replace interceptors = interceptors.split(",") with a trimmed list (e.g., map/ comprehension that calls .strip() on each entry) before the allowlist check and before applying ALLOWED_INTERCEPTORS to interceptors_function_processed; ensure you still validate membership against ALLOWED_INTERCEPTORS and apply the interceptor functions (referencing the interceptors variable, ALLOWED_INTERCEPTORS, and interceptors_function_processed).
🧹 Nitpick comments (1)
nettacker/core/utils/common.py (1)
182-186: Consider documentingset_nested_valueand failure modes.The helper assumes every intermediate segment in
key_pathalready exists as a dict ind; a missing or non-dict intermediate raisesKeyError/TypeError. In the current call site fromgenerate_new_sub_steps, paths come fromfind_repeaterstraversing the same structure, so this is safe today. A short docstring (plus optional type hints) would make the contract explicit for future callers.✏️ Proposed docstring
-def set_nested_value(d, key_path, value): - keys = [k for k in key_path.split("/") if k] - for key in keys[:-1]: - d = d[key] - d[keys[-1]] = value +def set_nested_value(d: dict, key_path: str, value) -> None: + """Assign ``value`` into ``d`` at the nested location described by ``key_path``. + + ``key_path`` is a "/"-delimited string (leading/trailing/empty segments are ignored). + All intermediate segments must already exist as dicts; otherwise ``KeyError`` or + ``TypeError`` is raised. + """ + keys = [k for k in key_path.split("/") if k] + for key in keys[:-1]: + d = d[key] + d[keys[-1]] = valueAs per coding guidelines: "Keep functions small, use type hints where practical, and add docstrings for public APIs".
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/core/utils/common.py` around lines 182 - 186, The helper set_nested_value currently assumes every intermediate segment in key_path exists and is a dict which can raise KeyError/TypeError; add a concise docstring to set_nested_value explaining its contract (key_path is "/"-separated, intermediate keys must exist and be dicts), add type hints to the signature (e.g., d: dict, key_path: str, value: Any -> None), and add explicit validation of intermediate segments inside set_nested_value that raises a clear, descriptive exception (or a ValueError/TypeError with the failing key and full path) instead of allowing raw KeyError/TypeError to bubble up; mention generate_new_sub_steps and find_repeaters in the docstring as the current safe callers.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@nettacker/core/utils/common.py`:
- Around line 322-342: The interceptors list may contain surrounding whitespace
after interceptors.split(",") causing ALLOWED_INTERCEPTORS lookups to fail;
update the processing in common.py so that after checking if interceptors is
truthy you replace interceptors = interceptors.split(",") with a trimmed list
(e.g., map/ comprehension that calls .strip() on each entry) before the
allowlist check and before applying ALLOWED_INTERCEPTORS to
interceptors_function_processed; ensure you still validate membership against
ALLOWED_INTERCEPTORS and apply the interceptor functions (referencing the
interceptors variable, ALLOWED_INTERCEPTORS, and
interceptors_function_processed).
---
Nitpick comments:
In `@nettacker/core/utils/common.py`:
- Around line 182-186: The helper set_nested_value currently assumes every
intermediate segment in key_path exists and is a dict which can raise
KeyError/TypeError; add a concise docstring to set_nested_value explaining its
contract (key_path is "/"-separated, intermediate keys must exist and be dicts),
add type hints to the signature (e.g., d: dict, key_path: str, value: Any ->
None), and add explicit validation of intermediate segments inside
set_nested_value that raises a clear, descriptive exception (or a
ValueError/TypeError with the failing key and full path) instead of allowing raw
KeyError/TypeError to bubble up; mention generate_new_sub_steps and
find_repeaters in the docstring as the current safe callers.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: f76e53d6-7bf3-4bc0-8420-aad312e50129
📒 Files selected for processing (1)
nettacker/core/utils/common.py
Contributor
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 `@tests/core/utils/test_common.py`:
- Around line 242-245: The test
test_fuzzer_repeater_perform_disallowed_interceptor_raises uses
pytest.raises(match="os.system") which treats the string as a regex (the dot is
a metacharacter); update the match to a literal match by escaping the dot (e.g.,
use a raw string like r"os\.system") or use re.escape on the string returned by
_fuzzer_arrays so that common_utils.fuzzer_repeater_perform is asserted against
the exact "os.system" text rather than a regex.
🪄 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: 2befcfa7-3956-4f55-914f-e919bdb194b5
📒 Files selected for processing (1)
tests/core/utils/test_common.py
Contributor
There was a problem hiding this comment.
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
nettacker/core/utils/common.py (1)
333-354:⚠️ Potential issue | 🟡 MinorTrim interceptor tokens before allowlist lookup.
The code splits interceptors by comma without stripping whitespace, which would reject valid declarations like
"generate_and_replace_md5, generate_and_replace_md5"due to the leading space in the second token. Apply the suggested fix to handle spacing and empty tokens gracefully.Suggested fix
interceptors = copy.deepcopy(arrays[array_name]["nettacker_fuzzer"]["interceptors"]) if interceptors: - interceptors = interceptors.split(",") + interceptors = [ + interceptor.strip() + for interceptor in interceptors.split(",") + if interceptor.strip() + ]🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@nettacker/core/utils/common.py` around lines 333 - 354, The interceptors list is created by splitting a string but tokens are not stripped, causing matches against ALLOWED_INTERCEPTORS to fail for tokens with surrounding whitespace; update the processing around interceptors (the interceptors variable and the loop that iterates over it, and where you check ALLOWED_INTERCEPTORS) to trim each token and skip empty tokens (e.g., map/strip each token after split and filter out empty strings) before performing the allowlist lookup and applying the interceptor functions.
♻️ Duplicate comments (1)
tests/core/utils/test_common.py (1)
279-282:⚠️ Potential issue | 🟡 MinorEscape the dot in the regex match.
Ruff still flags this because
"os.system"is a regex where.matches any character; use a literal regex to keep pre-commit clean.Suggested fix
- with pytest.raises(ValueError, match="os.system"): + with pytest.raises(ValueError, match=r"os\.system"): common_utils.fuzzer_repeater_perform(arrays)#!/bin/bash set -euo pipefail # Description: Re-run the specific Ruff rule that reported this warning. # Expected: No RUF043 findings remain after escaping the dot. ruff check tests/core/utils/test_common.py --select RUF043🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/utils/test_common.py` around lines 279 - 282, The test test_fuzzer_repeater_perform_disallowed_interceptor_raises uses pytest.raises(match="os.system") which treats the dot as a wildcard; update the match string passed to pytest.raises in that test to use a literal-dot regex (escape the dot) so the assertion matches "os.system" exactly when calling common_utils.fuzzer_repeater_perform on the arrays returned by _fuzzer_arrays("os.system").
🧹 Nitpick comments (1)
tests/core/utils/test_common.py (1)
235-239: Removecreate=Truefrom the patch sinceread_from_fileis a real function.The
read_from_filefunction is defined innettacker/core/fuzzer.pyand does not need to be created by the mock. Omittingcreate=Trueensures the test fails immediately if the function is renamed or removed in production.Suggested fix
- with patch("nettacker.core.fuzzer.read_from_file", return_value=["a", "b"], create=True): + with patch("nettacker.core.fuzzer.read_from_file", return_value=["a", "b"]): result = common_utils.apply_data_functions(data)🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/core/utils/test_common.py` around lines 235 - 239, Update the test test_apply_data_functions_invokes_allowed_function to remove the unnecessary create=True from the patch call: patch("nettacker.core.fuzzer.read_from_file", return_value=["a", "b"], create=True) should be changed to patch("nettacker.core.fuzzer.read_from_file", return_value=["a", "b"]) so the mock targets the real function (nettacker.core.fuzzer.read_from_file) and will fail fast if that function is renamed or removed.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Outside diff comments:
In `@nettacker/core/utils/common.py`:
- Around line 333-354: The interceptors list is created by splitting a string
but tokens are not stripped, causing matches against ALLOWED_INTERCEPTORS to
fail for tokens with surrounding whitespace; update the processing around
interceptors (the interceptors variable and the loop that iterates over it, and
where you check ALLOWED_INTERCEPTORS) to trim each token and skip empty tokens
(e.g., map/strip each token after split and filter out empty strings) before
performing the allowlist lookup and applying the interceptor functions.
---
Duplicate comments:
In `@tests/core/utils/test_common.py`:
- Around line 279-282: The test
test_fuzzer_repeater_perform_disallowed_interceptor_raises uses
pytest.raises(match="os.system") which treats the dot as a wildcard; update the
match string passed to pytest.raises in that test to use a literal-dot regex
(escape the dot) so the assertion matches "os.system" exactly when calling
common_utils.fuzzer_repeater_perform on the arrays returned by
_fuzzer_arrays("os.system").
---
Nitpick comments:
In `@tests/core/utils/test_common.py`:
- Around line 235-239: Update the test
test_apply_data_functions_invokes_allowed_function to remove the unnecessary
create=True from the patch call: patch("nettacker.core.fuzzer.read_from_file",
return_value=["a", "b"], create=True) should be changed to
patch("nettacker.core.fuzzer.read_from_file", return_value=["a", "b"]) so the
mock targets the real function (nettacker.core.fuzzer.read_from_file) and will
fail fast if that function is renamed or removed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: aca42227-b250-4110-954f-2ea2e1eab9da
📒 Files selected for processing (2)
nettacker/core/utils/common.pytests/core/utils/test_common.py
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
1 participant
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.
Proposed change
This PR is to fix some potential security issues in Nettacker w.r.t to the user of
execin the codebase. The following places usedexecas a "lazy" method and not a necessity:generate_new_sub_stepsfunctionapply_data_functionsfunctionfuzzer_repeater_performfunctionType of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake testand I confirm all tests passed locallydocs/folder