fix(core): resolve CORS regex logic bug and implement value-based regex testing#1375
Open
jess-tech-lab wants to merge 10 commits intoOWASP:masterfrom
Open
fix(core): resolve CORS regex logic bug and implement value-based regex testing#1375jess-tech-lab wants to merge 10 commits intoOWASP:masterfrom
jess-tech-lab wants to merge 10 commits intoOWASP:masterfrom
Conversation
Contributor
Summary by CodeRabbit
WalkthroughRemoved a trailing space from the Changes
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Suggested reviewers
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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.
🧹 Nitpick comments (2)
tests/test_yaml_regexes.py (2)
25-37: Render the sample fromnettacker_fuzzer.data, not fixed defaults.
resolve_input_format()currently ignoresdata,prefix, andsuffix, so the new semantic check is only accurate for modules that happen to usehttp,80, andtestpath. Building the sample from the YAML fuzzer config keeps the assertion aligned with the value the request step would actually emit.♻️ Suggested refactor
def resolve_input_format(value): - """Resolve a literal or nettacker_fuzzer header value to a concrete string by substituting - fuzzer double-brace variables with representative values. Single-brace placeholders like - {target} are left as-is so they remain valid literals in both the test string and the regex.""" - if isinstance(value, dict) and "nettacker_fuzzer" in value: - fmt = value["nettacker_fuzzer"]["input_format"] - else: - fmt = str(value) - return ( - fmt.replace("{{schema}}", "http") - .replace("{{ports}}", "80") - .replace("{{paths}}", "testpath") - ) + """Resolve a literal or nettacker_fuzzer header value to a representative concrete string.""" + if not (isinstance(value, dict) and "nettacker_fuzzer" in value): + return str(value) + + fuzzer = value["nettacker_fuzzer"] + rendered = fuzzer.get("input_format", "") + for key, candidates in fuzzer.get("data", {}).items(): + sample = candidates[0] if isinstance(candidates, list) and candidates else candidates + if sample is not None: + rendered = rendered.replace(f"{{{{{key}}}}}", str(sample)) + + return f'{fuzzer.get("prefix", "")}{rendered}{fuzzer.get("suffix", "")}'🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_yaml_regexes.py` around lines 25 - 37, resolve_input_format currently substitutes {{schema}}, {{ports}}, {{paths}} with fixed defaults; change it to build the sample from the nettacker_fuzzer config: read value["nettacker_fuzzer"]["data"] (and also use prefix and suffix when present) and replace the double-brace variables in input_format with the corresponding entries from that data dict instead of hard-coded "http", "80", "testpath", then return prefix + rendered_input_format + suffix so the sample matches what the request step would actually emit. Use the existing resolve_input_format function and the keys input_format, data, prefix, suffix to locate fields.
98-98: Usestr | Nonefor the explicit type annotation.The parameter
header_valuehas an implicitOptionaltype (annotationstrwith defaultNone), which is inconsistent and reduces type clarity. Since the project targets Python 3.10+, usestr | Noneinstead.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_yaml_regexes.py` at line 98, Update the type annotation for the parameter in the is_valid_regex function to explicitly use the modern union syntax: change header_value's annotation from implicit Optional (defaulting to None) to "str | None" so the signature reads is_valid_regex(regex: str, header_value: str | None = None) to match Python 3.10+ typing style and improve clarity.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_yaml_regexes.py`:
- Around line 25-37: resolve_input_format currently substitutes {{schema}},
{{ports}}, {{paths}} with fixed defaults; change it to build the sample from the
nettacker_fuzzer config: read value["nettacker_fuzzer"]["data"] (and also use
prefix and suffix when present) and replace the double-brace variables in
input_format with the corresponding entries from that data dict instead of
hard-coded "http", "80", "testpath", then return prefix + rendered_input_format
+ suffix so the sample matches what the request step would actually emit. Use
the existing resolve_input_format function and the keys input_format, data,
prefix, suffix to locate fields.
- Line 98: Update the type annotation for the parameter in the is_valid_regex
function to explicitly use the modern union syntax: change header_value's
annotation from implicit Optional (defaulting to None) to "str | None" so the
signature reads is_valid_regex(regex: str, header_value: str | None = None) to
match Python 3.10+ typing style and improve clarity.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: d5515219-1c5b-4d19-991c-fef36cf95392
📒 Files selected for processing (2)
nettacker/modules/vuln/http_cors.yamltests/test_yaml_regexes.py
Contributor
There was a problem hiding this comment.
Actionable comments posted: 1
🧹 Nitpick comments (1)
tests/test_yaml_regexes.py (1)
102-102: Usestr | Nonefor the optionalheader_valueparameter.The function parameter
header_value: str = Nonetriggers Ruff's RUF013 warning. Change it toheader_value: str | None = Noneto make the optional type explicit. Python 3.10+ is the minimum supported version, so the union syntax is appropriate.🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_yaml_regexes.py` at line 102, Update the is_valid_regex function signature to use the explicit optional union type for header_value to address Ruff RUF013: change the parameter annotation from header_value: str = None to header_value: str | None = None in the def is_valid_regex(...) declaration so the type is explicit (Python 3.10+ union syntax) and adjust any type hints/usage accordingly if needed.
🤖 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/test_yaml_regexes.py`:
- Around line 27-39: The test helper resolve_input_format currently collapses a
nettacker_fuzzer block into a single synthetic string (only "http", "80",
"testpath"); update it to fully expand all fuzzer-emitted variants (e.g.,
multiple schemas like "http"/"https", arrays in "ports"/"paths"/"data", and any
"prefix"/"suffix") and produce all concrete header values instead of one. Change
resolve_input_format to detect when value is a dict with "nettacker_fuzzer" and
iterate over each variant field (schema(s), ports, paths, prefix/suffix, data)
to build the Cartesian product of emitted strings, return a list of concrete
values, and update the test assertion logic that iterates (regex, header_value)
pairs so it asserts the response regex matches each concrete header_value for
every variant.
---
Nitpick comments:
In `@tests/test_yaml_regexes.py`:
- Line 102: Update the is_valid_regex function signature to use the explicit
optional union type for header_value to address Ruff RUF013: change the
parameter annotation from header_value: str = None to header_value: str | None =
None in the def is_valid_regex(...) declaration so the type is explicit (Python
3.10+ union syntax) and adjust any type hints/usage accordingly if needed.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: b6b35765-a666-47cb-9cce-86b60e47e6e6
📒 Files selected for processing (1)
tests/test_yaml_regexes.py
jess-tech-lab
force-pushed
the
fix-cors-origin
branch
from
cd498fa to
36b5a2c
Compare
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_yaml_schema_and_regex.py (1)
44-52: Use explicit| Nonetype hint instead of implicitOptional.PEP 484 prohibits implicit
Optionalwhen using= Noneas a default. The type hint should explicitly indicate the parameter can beNone.Proposed fix
-def is_valid_regex(regex: str, header_value: str = None) -> bool: +def is_valid_regex(regex: str, header_value: str | None = None) -> bool:🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_yaml_schema_and_regex.py` around lines 44 - 52, The parameter annotation for is_valid_regex should explicitly show it can be None; update the signature to use the union operator (header_value: str | None = None) instead of implicit Optional via default, keeping the default = None and leaving the function body unchanged so type checkers follow PEP 484/PEP 604; reference the is_valid_regex function to locate and update only the header_value type hint.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_yaml_schema_and_regex.py`:
- Around line 44-52: The parameter annotation for is_valid_regex should
explicitly show it can be None; update the signature to use the union operator
(header_value: str | None = None) instead of implicit Optional via default,
keeping the default = None and leaving the function body unchanged so type
checkers follow PEP 484/PEP 604; reference the is_valid_regex function to locate
and update only the header_value type hint.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 9db8fd79-0fda-4a60-8268-d3ca40e789b4
📒 Files selected for processing (2)
nettacker/modules/vuln/http_cors.yamltests/test_yaml_schema_and_regex.py
✅ Files skipped from review due to trivial changes (1)
- nettacker/modules/vuln/http_cors.yaml
Contributor
There was a problem hiding this comment.
🧹 Nitpick comments (1)
tests/test_yaml_schema_and_regex.py (1)
232-236: Rename unused loop variable to_header_name.The static analysis correctly identifies that
header_nameis not used within the loop body. Per Python convention, prefix with underscore to indicate intentional disuse.♻️ Proposed fix
- for header_name, header_block in value.items(): + for _header_name, header_block in value.items(): if isinstance(header_block, dict) and "regex" in header_block: regexes.append((header_block["regex"], None))🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@tests/test_yaml_schema_and_regex.py` around lines 232 - 236, Rename the unused loop variable header_name to _header_name in the loop under the conditional if field == "headers" and isinstance(value, dict) so static analysis won't flag it; update the for loop header for for header_name, header_block in value.items(): to use _header_name while leaving header_block and regexes.append((header_block["regex"], None)) unchanged.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Nitpick comments:
In `@tests/test_yaml_schema_and_regex.py`:
- Around line 232-236: Rename the unused loop variable header_name to
_header_name in the loop under the conditional if field == "headers" and
isinstance(value, dict) so static analysis won't flag it; update the for loop
header for for header_name, header_block in value.items(): to use _header_name
while leaving header_block and regexes.append((header_block["regex"], None))
unchanged.
ℹ️ Review info
⚙️ Run configuration
Configuration used: Path: .coderabbit.yaml
Review profile: CHILL
Plan: Pro
Run ID: 1bf6dd5f-5574-4a4a-b5f1-584a5365bd3f
📒 Files selected for processing (1)
tests/test_yaml_schema_and_regex.py
Contributor
Author
|
Hi @securestep9 and @arkid15r, I've rebased this branch against the latest changes to resolve conflicts with PR #1319. Please review when you have a chance. Thanks. |
Contributor
Author
|
@arkid15r and @securestep9 - can you review? Thanks. |
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 fixes a logic bug in the CORS reflection module and upgrades the testing infrastructure to prevent "silent failures" in YAML modules.
nettacker/modules/vuln/http_cors.yaml)Access-Control-Allow-Originregex to ensure the module correctly identifies vulnerabilities when the origin is reflected.tests/test_yaml_regexes.py)Originfrom the request fuzzer and verify the response regex against it.Closes #1374
Type of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake test, I confirm all tests passed locallydocs/folder