Skip to content

fix(security): log every scan attempt blocked before execution#1698

Open
anshul23102 wants to merge 1 commit into
utksh1:mainfrom
anshul23102:fix/1623-audit-log-blocked-attempts
Open

fix(security): log every scan attempt blocked before execution#1698
anshul23102 wants to merge 1 commit into
utksh1:mainfrom
anshul23102:fix/1623-audit-log-blocked-attempts

Conversation

@anshul23102

Copy link
Copy Markdown
Contributor

Description

Fixes #1623

Previously, the audit log only recorded scans that passed every gate and began executing, via executor.create_task's "task_created" event. Any attempt rejected earlier — missing consent, unknown plugin, invalid preset, unauthorised target/credential/session policy, invalid input, or a target blocked by safe mode — raised its HTTPException and exited the code path before ever reaching a log write. An administrator had no way to determine, post-incident, whether a user had attempted to scan an unauthorised target: the misuse simply left no trace.

Approach

Added _log_blocked_scan_attempt, a thin wrapper around the existing db.log_audit with severity="warning", and called it at every rejection point in POST /task/start before its raise:

  • task_blocked_invalid_payload — payload size/field-length guard
  • task_blocked_consent — consent not granted
  • task_blocked_plugin_not_found — unknown plugin_id
  • task_blocked_invalid_preset — preset not valid for the plugin
  • task_blocked_policy — target/credential/session policy missing or insufficiently permissive
  • task_blocked_invalid_input — timeout/max_scan_time out of bounds
  • task_blocked_safe_mode — target rejected or validation timed out under safe mode (the exact scenario the issue's reproduction steps describe)
  • task_blocked_rate_limit — per-client-per-plugin quota exceeded

Moved db = await get_db() to the top of start_task so it's available to every rejection branch, including ones that previously ran before the database handle was fetched.

Related Issues

Closes #1623

Type of Change

  • Bug fix (non-breaking change which fixes an issue)

How Has This Been Tested?

New test file testing/backend/integration/test_task_start_audit_log.py (5 tests):

  • missing consent → task_blocked_consent audit entry, severity warning
  • unknown plugin → task_blocked_plugin_not_found entry
  • invalid preset → task_blocked_invalid_preset entry
  • safe-mode target rejection (public IP with no permissive target policy) → task_blocked_safe_mode entry with the target in context
  • regression guard: the existing successful-path task_created entry still fires exactly once, unaffected by the new instrumentation

Full suite run locally:

pytest testing/backend/unit -q -m "not benchmark"        # 2210 passed (6 pre-existing failures, confirmed identical on unmodified main — sandbox/subprocess tests, unrelated to this change)
pytest testing/backend/integration -q -m "not benchmark"  # 289 passed, 9 skipped (284 baseline + 5 new)
ruff check backend testing/backend                          # all checks passed
scripts/check-artifacts.sh origin/main                       # clean

Checklist

  • My code follows the code style of this project.
  • I have performed a self-review of my own code.
  • I have commented my code, particularly in hard-to-understand areas.
  • I have made corresponding changes to the documentation.
  • My changes generate no new warnings.

Additional Notes

I'm contributing this through GSSoC. While building the test for the safe-mode rejection path, I found a separate, more serious issue: start_task's safe-mode target check only looks at the literal "target" input key (if target := effective_inputs.get("target")), but plugins can declare their field under a different name — http_inspector's schema uses "url". For any plugin whose field isn't literally named "target", safe-mode validation is silently skipped entirely (confirmed: validate_target("http://8.8.8.8", True) correctly rejects it, but the request-level check never reaches it for http_inspector). I've flagged that separately rather than expanding this PR's scope, since it's a distinct bug from the audit-log gap this PR fixes.

Previously, the audit log only recorded scans that passed every gate and
began executing, via executor.create_task's "task_created" event. Any
attempt rejected earlier -- missing consent, unknown plugin, invalid
preset, unauthorised target/credential/session policy, invalid input,
or a target blocked by safe mode -- raised its HTTPException and exited
the code path before ever reaching a log write. An administrator had no
way to determine, post-incident, whether a user had attempted to scan an
unauthorised target: the misuse simply left no trace.

Fix: added _log_blocked_scan_attempt, a thin wrapper around the
existing db.log_audit with severity="warning", and called it at every
rejection point in POST /task/start before its raise:

- task_blocked_invalid_payload  (payload size/field-length guard)
- task_blocked_consent          (consent not granted)
- task_blocked_plugin_not_found (unknown plugin_id)
- task_blocked_invalid_preset   (preset not valid for the plugin)
- task_blocked_policy           (target/credential/session policy
                                  missing or insufficiently permissive)
- task_blocked_invalid_input    (timeout/max_scan_time out of bounds)
- task_blocked_safe_mode        (target rejected or validation timed
                                  out under safe mode -- the scenario
                                  the issue's repro steps describe)
- task_blocked_rate_limit       (per-client-per-plugin quota exceeded)

Moved db = await get_db() to the top of start_task so it's available
to every rejection branch, including ones that previously ran before
the database handle was fetched.

Testing:
- testing/backend/integration/test_task_start_audit_log.py: 5 new tests
  covering missing consent, unknown plugin, invalid preset, and safe-mode
  target rejection all producing an audit_log row with severity
  "warning", plus a regression guard confirming the existing successful-
  path 'task_created' entry is unaffected (exactly one entry, not zero
  or duplicated).
- pytest testing/backend/unit -q -m "not benchmark" -- 2210 passed (6
  pre-existing failures unrelated to this change, confirmed identical on
  unmodified main -- sandbox/subprocess tests failing in this sandbox)
- pytest testing/backend/integration -q -m "not benchmark" -- 289 passed,
  9 skipped (284 baseline + 5 new)
- ruff check backend testing/backend -- all checks passed
- scripts/check-artifacts.sh origin/main -- clean

Fixes utksh1#1623

@utksh1 utksh1 left a comment

Copy link
Copy Markdown
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The audit logging direction is right, but backend-unit CI is failing and the branch has merge conflicts. Please rebase on latest main, fix the failing tests, and push again.

@utksh1 utksh1 added level:intermediate 35 pts difficulty label for moderate contributor PRs type:security Security work category bonus label type:bug Bug fix work category bonus label area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests labels Jul 9, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area:backend Backend API, database, or service work area:security Security-sensitive implementation or tests level:intermediate 35 pts difficulty label for moderate contributor PRs type:bug Bug fix work category bonus label type:security Security work category bonus label

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug: audit log omits blocked and failed scan attempts, breaking forensics

2 participants