Add stop_at_first_success to Prevent Duplicate Results (Fixes #1513) - #1514
Add stop_at_first_success to Prevent Duplicate Results (Fixes #1513)#1514Aarush289 wants to merge 24 commits into
Conversation
create pr
Document all modules ( fix OWASP#1269 ) (OWASP#1270)
Signed-off-by: Aarush <cs24b064@smail.iitm.ac.in>
Removed entry for FortiWeb authentication bypass vulnerability. Signed-off-by: Aarush <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush <cs24b064@smail.iitm.ac.in>
Signed-off-by: Aarush <cs24b064@smail.iitm.ac.in>
Merge new changes
Merge new modules
Merge changes from master
|
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:
Summary by CodeRabbit
WalkthroughAdds shared preflight checks and unique temporary-event claims for ChangesStop-at-first-success handling
Suggested reviewers: 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ 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: 3
🤖 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/base.py`:
- Around line 294-299: The branch that returns early on finding an existing
event leaks a partially mutated sub_step because run() deletes
sub_step["method"] and sub_step["response"] before the check; move the
"stop_at_first_success" check to run before deleting those keys or,
alternatively, ensure you restore sub_step["method"] and sub_step["response"]
(the originals saved in backup_response) before returning; locate the logic
around run(), the sub_step dict, the deletions of "method" and "response", and
the find_temp_events call to apply the fix.
- Around line 126-127: The dedupe key is missing the port so
find_temp_events(...) currently suppresses successes across different ports;
update all calls to find_temp_events(target, module_name, scan_id, event_name)
to pass the port (use event["response"]["port"]) and change the find_temp_events
function signature/implementation to include port in its lookup key; also update
any temp-event creation/storage logic used by find_temp_events so the stored key
includes port (apply the same change to the other two call sites referenced
around the other ranges).
🪄 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: 326ef8c1-32d8-4965-9438-406456214e8b
📒 Files selected for processing (1)
nettacker/core/lib/base.py
|
Can you do a small benchmark to see how this will affect the scan efficieny and latencies? If we're making multiple database calls for deduplication then it might increase the number of I/O calls? @Aarush289 |
|
Okay sure, will do that. |
|
@pUrGe12 I have done basic bench-marking, file I/O calls will be increased but there is negligible effect on system time and cpu usage. |
|
@codex review this PR using skill: https://github.com/nettacker-codex-ai/skills/blob/main/pr-review-recommender/SKILL.md |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 07d0385d26
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| "module_name": module_name, | ||
| "scan_id": scan_id, | ||
| "event_name": event["response"]["stop_at_first_success"], | ||
| "port": event.get("ports", ""), |
There was a problem hiding this comment.
For engines that inherit BaseEngine.run (such as FTP, SSH, and socket), run converts ports to the singular port before calling this method. Consequently this expression stores every success marker under "", and the lookup above also uses that blank port. After one port succeeds, a later success on a different port is discarded, causing false negatives despite the feature being scoped by target, module, and port. Read both ports and port, as the normal scan-result path already does.
Useful? React with 👍 / 👎.
| existing = find_temp_events( | ||
| target, module_name, scan_id, event_name, port=event.get("ports", "") | ||
| ) |
There was a problem hiding this comment.
When multiple generated substeps for the same target/module/port finish concurrently, each thread can complete this lookup before either reaches the marker insertion below. Both then insert a marker and submit a normal scan result, so the duplicate-results regression remains under the default concurrent execution. Because temp_events has no uniqueness constraint for this key, the marker must be claimed atomically and the result logged only by the thread whose claim succeeds.
Useful? React with 👍 / 👎.
| if "stop_at_first_success" in backup_response: | ||
| event_name = backup_response["stop_at_first_success"] | ||
| existing = find_temp_events( | ||
| target, module_name, scan_id, event_name, port=sub_step.get("ports", "") | ||
| ) | ||
| if existing: |
There was a problem hiding this comment.
For HTTP payloads, HttpEngine.run overrides this method and never performs the new preflight lookup, so every generated HTTP request is still sent even after an earlier request has succeeded; suppression happens only afterward in process_conditions. This is especially costly for the WAF-style payloads this option is intended for, where many probes remain after the first match, and means stop_at_first_success does not actually stop HTTP scanning at the first success. Move the check into shared code or add the equivalent before send_request in HttpEngine.run.
Useful? React with 👍 / 👎.
| if "stop_at_first_success" in event["response"]: | ||
| event_name = event["response"]["stop_at_first_success"] |
There was a problem hiding this comment.
The repository's HTTP_RESPONSE_SCHEMA in tests/test_yaml_schema_and_regex.py sets ignore_extra_keys=False but does not allow stop_at_first_success. As soon as a bundled HTTP module adopts this new framework option, the module-validation test rejects its response block with an unexpected-key error, so the feature cannot be integrated into the shipped YAML modules while keeping the required test suite green. Add this string-valued option to the response schema.
Useful? React with 👍 / 👎.
There was a problem hiding this comment.
Actionable comments posted: 4
🧹 Nitpick comments (2)
nettacker/database/sqlite.py (1)
7-13: 📐 Maintainability & Code Quality | 🔵 Trivial | 💤 Low valueDocument and type the public initializer.
Add a docstring and
-> Nonetosqlite_create_tables.As 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 current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@nettacker/database/sqlite.py` around lines 7 - 13, Update the public sqlite_create_tables initializer to declare a None return type and add a concise docstring describing its table-creation behavior, while preserving the existing database setup and metadata creation flow.Source: Coding guidelines
tests/database/test_db.py (1)
526-526: 🗄️ Data Integrity & Integration | 🔵 Trivial | ⚡ Quick winTest duplicate claim outcomes on both backends.
Add an APSW test with
changes()returning0. Add an ORM test wherecommit()raisesIntegrityError. Assert that each path reports a duplicate claim and rolls back the ORM session. Also test that a non-conflict persistence failure is not handled as a duplicate.Also applies to: 606-606, 616-616
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/database/test_db.py` at line 526, Extend the duplicate-claim tests around mock_connection.changes and the ORM claim path to cover both backends: assert APSW changes() returning 0 reports a duplicate claim, and assert ORM commit() raising IntegrityError reports a duplicate claim and rolls back the session. Add a non-conflict persistence failure case and verify it is not classified as a duplicate claim.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@nettacker/core/lib/base.py`:
- Around line 192-209: Update process_conditions() in nettacker/core/lib/base.py
to suppress and log an unsuccessful result only when the claim status explicitly
indicates a duplicate; propagate non-conflict persistence failures instead of
treating them as duplicates. In nettacker/database/db.py, update the APSW
operation at lines 354-361 and the ORM operation at lines 397-420 to return
distinct claimed, duplicate, and failed statuses, ensuring APSW retry exhaustion
reports failure unless a claim was actually stored.
- Around line 69-73: Unify port resolution across check_prior_success() and the
claim/insert path so both use the same effective port value. The resolver must
account for sub_step["ports"], the explicit port argument, and a numeric port
parsed from the URL using the existing precedence rules. Pass that resolved
value to find_temp_events() and serialize the identical value when inserting
claims, preventing duplicate requests for URL-only or port-only steps.
In `@nettacker/database/models.py`:
- Around line 37-46: Add upgrade logic in mysql_create_tables() and
postgres_create_database() to detect existing temp_events tables missing
uq_temp_events_claim and create the constraint/index, matching the existing
SQLite rebuild behavior. Keep Base.metadata.create_all() for initial setup and
ensure the migration is safe for already-updated databases.
In `@nettacker/database/sqlite.py`:
- Around line 34-36: Update the unique-constraint migration around the
has_constraint check to preserve existing temp_events rows: copy legacy rows
into the new TempEvents schema with deduplication before applying
uq_temp_events_claim, or ensure initialization is exclusive before any drop.
Keep the migration invoked by app.py initialization from losing active
dependency state across processes.
---
Nitpick comments:
In `@nettacker/database/sqlite.py`:
- Around line 7-13: Update the public sqlite_create_tables initializer to
declare a None return type and add a concise docstring describing its
table-creation behavior, while preserving the existing database setup and
metadata creation flow.
In `@tests/database/test_db.py`:
- Line 526: Extend the duplicate-claim tests around mock_connection.changes and
the ORM claim path to cover both backends: assert APSW changes() returning 0
reports a duplicate claim, and assert ORM commit() raising IntegrityError
reports a duplicate claim and rolls back the session. Add a non-conflict
persistence failure case and verify it is not classified as a duplicate claim.
🪄 Autofix
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 Plus
Run ID: 4f1348b4-bb11-4e50-a108-bfbd6877cfb1
📒 Files selected for processing (6)
nettacker/core/lib/base.pynettacker/core/lib/http.pynettacker/database/db.pynettacker/database/models.pynettacker/database/sqlite.pytests/database/test_db.py
3 participants
Proposed change
Your PR description goes here:
This PR introduces a new feature stop_at_first_success to prevent duplicate entries in scan results. Currently, the same successful detection can be logged multiple times for a given target, module, and port, which degrades user experience and clutters the output.
Fixes #1513
Before
After
The feature can be used as
Type of change
Checklist
make pre-commitand confirm it didn't generate any warnings/changesmake testand I confirm all tests passed locallydocs/folder