Skip to content

fix(detectors): reject null invoice threshold - #554

Open
DevaanshPathak wants to merge 2 commits into
GenAI-Security-Project:mainfrom
DevaanshPathak:agent/fix-issue-125-threshold-config
Open

fix(detectors): reject null invoice threshold#554
DevaanshPathak wants to merge 2 commits into
GenAI-Security-Project:mainfrom
DevaanshPathak:agent/fix-issue-125-threshold-config

Conversation

@DevaanshPathak

Copy link
Copy Markdown

Summary

  • reject an explicitly configured max_invoice_amount=None during detector initialization
  • preserve the existing default-threshold behavior when the configuration key is omitted
  • add focused regression coverage for null, omitted, and valid positive values

Root cause

The validation guard checked whether the retrieved value was non-null rather than whether the configuration key was present. This made an explicit null value indistinguishable from an omitted setting during validation, even though later lookup returned the null value instead of the default threshold.

Impact

Invalid configuration now fails fast with a clear ValueError instead of allowing the detector to initialize and later crash with a TypeError while processing an approval event.

Testing

  • uv run pytest tests/unit/ctf/test_invoice_threshold_bypass_config.py — 3 passed
  • uv run pytest tests/unit/ctf — 30 passed, 1 skipped
  • uv run black --check tests/unit/ctf/test_invoice_threshold_bypass_config.py
  • uv run isort --check-only finbot/ctf/detectors/implementations/invoice_threshold_bypass.py tests/unit/ctf/test_invoice_threshold_bypass_config.py

Fixes #125

@DevaanshPathak
DevaanshPathak marked this pull request as ready for review August 4, 2026 17:15
Copilot AI lite review requested due to automatic review settings August 4, 2026 17:15

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

Pull request overview

This PR fixes configuration validation for InvoiceThresholdBypassDetector so that an explicitly configured max_invoice_amount=None is rejected during initialization, while leaving the existing “omitted key uses default threshold” behavior intact.

Changes:

  • Update _validate_config() to validate max_invoice_amount based on key presence (so explicit None is rejected).
  • Add unit tests covering explicit None, omitted key, and a valid positive threshold value.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.

File Description
finbot/ctf/detectors/implementations/invoice_threshold_bypass.py Adjusts config validation to fail fast when max_invoice_amount is explicitly set to None.
tests/unit/ctf/test_invoice_threshold_bypass_config.py Adds regression tests for null, omitted, and positive max_invoice_amount configurations.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment thread tests/unit/ctf/test_invoice_threshold_bypass_config.py Outdated
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Bug_028_MUST_FIX: Test Case DET-CFG-008 InvoiceThresholdBypassDetector silently accepts max_invoice_amount=None

2 participants