fix(scanner): resolve COR-001-004 scanner correctness issues (#151)

[safidnadaf]

What does this PR do?

Fixes four scanner correctness bugs: case-sensitive direction check and ignored plural source prefixes in az_net_003, a false positive on API failure in az_db_002, and a KeyError on malformed ARM IDs.

Type of change

[x] Bug fix

Testing

[x] Returns correct JSON output
[x] No hardcoded credentials or secrets

Related issue

Closes #151

Checklist

[x] I have not committed any real Azure credentials

Details

COR-001: direction check is now case-insensitive
COR-002: now checks both singular and plural source prefix fields
COR-003: API failures now skip the resource instead of falsely flagging it
COR-004: malformed ARM IDs no longer cause a crash

Added 9 regression tests covering all four fixes. All tests pass locally (16/16 affected, 83/83 full suite).

[Vishnu2707]

@ritiksah141 , can u have a look at this, if this will be disrupted by the new PR or not. Do provide your reviews as well.

[Vishnu2707]

@parthrohit22, @SHAURYAKSHARMA24 , can you do the initial review of this PR pls.

[ritiksah141]

I reviewed this end to end and checked the interaction with the newer infra/CI work.

Functional review:

• The scanner fixes look correct for the stated COR-001 through COR-004 issues.
• I do not see a runtime disruption because this PR only touches scanner rule logic, the Azure client helper, and scanner tests.
• GitHub reports the PR as mergeable against dev.
• A merge preview shows overlap in the same six files with latest dev, but no conflict markers. That should be an auto-merge/update concern rather than a manual conflict.

Blocking issue:

1. The branch is not passing the newer format gate from dev.

   Current dev now has ruff format --check . in CI. Running the formatter check locally against the files touched in this PR fails:

   ruff format --check scanner/azure_client.py scanner/rules/az_db_002.py scanner/rules/az_net_003.py tests/helpers/mock_azure.py tests/test_rules_database.py tests/test_rules_network.py

   Result:

   Would reformat: scanner/azure_client.py
   Would reformat: scanner/rules/az_db_002.py
   Would reformat: scanner/rules/az_net_003.py
   Would reformat: tests/helpers/mock_azure.py
   Would reformat: tests/test_rules_database.py
   Would reformat: tests/test_rules_network.py
   6 files would be reformatted
   

   Example locations include tests/test_rules_network.py:8, tests/test_rules_network.py:109, scanner/rules/az_db_002.py:36, and scanner/rules/az_net_003.py:51. Please rebase or merge latest dev, run ruff format on the touched files, and push so the PR is validated under the newer CI workflow.

Validation I ran locally:

ruff check scanner/azure_client.py scanner/rules/az_db_002.py scanner/rules/az_net_003.py tests/helpers/mock_azure.py tests/test_rules_database.py tests/test_rules_network.py
ruff format --check scanner/azure_client.py scanner/rules/az_db_002.py scanner/rules/az_net_003.py tests/helpers/mock_azure.py tests/test_rules_database.py tests/test_rules_network.py
pytest tests/test_rules_network.py tests/test_rules_database.py
pytest tests/test_rules_network.py tests/test_rules_database.py tests/test_clean_scan.py
pytest
bandit -r scanner/ -ll

Results:

• Focused rule tests passed: 16 passed.
• Rule plus clean-scan tests passed: 24 passed.
• Targeted ruff check passed.
• ruff format --check failed on the six touched files.
• Full pytest had one local AI vector-store failure in tests/test_ai_hallucination_guard.py::TestHallucinationGuard::test_vector_store_purity due ONNX Runtime not being able to create a working directory under the local temp path. That does not appear related to this scanner PR.
• bandit -r scanner/ -ll reports existing scanner findings in scanner/nvd_client.py and scanner/rules/az_db_004.py, not in the files changed by this PR.

Once the formatting/update is fixed, I think the scanner logic itself is in good shape.

[SHAURYAKSHARMA24]

Requesting changes.

The scanner fixes are in the right direction, but I found production-path issues that are not covered by the current tests.

1. AZ-NET-003 may fail with real Azure SDK enum-backed fields.

The rule normalises direction and access using str(...) and compares against "inbound" / "allow". However, the Azure SDK model allows these fields to be either plain strings or enum values (SecurityRuleDirection, SecurityRuleAccess). If an SDK enum instance is returned, stringifying it may not produce "Inbound" / "Allow", so the rule can miss real non-compliant NSG rules even though the mock-string tests pass.

Please normalise enum values safely, for example by checking .value when present, and add regression coverage using azure.mgmt.network.models.SecurityRule rather than only SimpleNamespace/string-backed mocks.

2. AZ-DB-002 has the same enum-normalisation risk.

The SQL auditing policy model allows state to be either a string or BlobAuditingPolicyState. The current logic uses str(getattr(policy, "state", "Disabled")).lower() != "enabled". If a real SDK enum is returned, an enabled policy may be treated as disabled, creating a false positive. The existing test passes because it uses state="Enabled" as a plain string.

Please normalise the policy state safely and add regression coverage using the real Azure SQL auditing policy model.

3. Current dev CI is still not satisfied.

The current dev workflow runs both:

ruff check .
ruff format --check .

The six touched files still need to be formatted against the current dev gate. Please update the branch from dev, run formatting on the touched files, and rerun CI.

Non-blocking follow-ups:

• parse_resource_id() returning resource_group="" avoids the KeyError, but scanner rules should explicitly skip/log malformed IDs instead of silently calling downstream API helpers with an empty resource group.
• For AZ-NET-003, findings triggered through source_address_prefixes should include the matched plural source prefix in metadata/evidence.
• Consider IPv6 unrestricted source ranges such as ::/0 and plural destination port ranges in follow-up coverage.

Because the production SDK-model paths are not properly covered, I cannot approve this as a scanner-correctness fix yet.

[Vishnu2707]

@safidnadaf , please do put a comment on the convo, if you have addressed so the people reviewing the PR can approve it if the changes are met according to the standards.

[parthrohit22]

Requesting changes.

I reviewed the updated production logic and the new Azure SDK-model regression tests.

The functional fixes are now correct for the stated COR-001 through COR-004 scope:

• AZ-NET-003 normalises direction and access safely for both strings and real Azure SDK enums.
• Direction comparison is case-insensitive.
• Both source_address_prefix and source_address_prefixes are checked.
• The matched plural prefix is included in finding metadata.
• AZ-DB-002 skips an indeterminate auditing-policy lookup instead of generating a false positive.
• Real BlobAuditingPolicyState enum values are handled correctly.
• Malformed SQL ARM IDs are skipped and logged in AZ-DB-002.
• The new real-SDK tests address the previous mock-only coverage concern.

However, the PR is not merge-ready because three conflicts remain:

• tests/helpers/mock_azure.py
• tests/test_rules_database.py
• tests/test_rules_network.py

These conflicts must be resolved carefully. Current dev contains the expanded MockAzureClient and comprehensive database/network regression coverage. Please use the latest dev versions as the baseline and add the COR-specific tests into them. Do not resolve the conflicts by replacing the current dev files with the older PR versions, because that would remove coverage and mock functionality for other scanner rules.

After resolving the conflicts, please rerun:

ruff check .
ruff format --check .
pytest tests/test_rules_network.py tests/test_rules_database.py tests/test_clean_scan.py -v --tb=short
pytest tests/ -v --tb=short --cov=api --cov=scanner --cov-report=term --cov-report=xml --cov-fail-under=25

The current successful Ruff and pytest checks were run against commit 0a423a3 before the latest-dev conflicts were resolved, so the final merged result still needs validation.

The Container Scan (Trivy) check is also failing with exit code 1. Please inspect the full job output and either fix the reported issue or document with the maintainers why it is an unrelated accepted baseline failure.

Once the conflicts are resolved without losing current dev coverage, the final CI run is complete, and the Trivy status is accounted for, I should be able to approve. The scanner logic itself is now in good shape.

Non-blocking follow-ups:

• Add ::/0 handling for unrestricted IPv6 sources.
• Check destination_port_ranges as well as destination_port_range.
• Update AZ-DB-004 and other consumers to explicitly skip/log malformed ARM IDs rather than calling helpers with an empty resource group.

[safidnadaf]

Conflicts resolved.

#163

@parthrohit22 - resolved the three-file conflict using latest dev as the baseline, per your instructions. No existing coverage was removed:

• tests/helpers/mock_azure.py: kept dev's expanded MockAzureClient (Graph/SDK credential stub, DNS, web apps, etc.) in full
• tests/test_rules_database.py: kept dev's AZ-DB-001 and AZ-DB-003 coverage, added the COR-specific AZ-DB-002 tests (API-failure, malformed ARM ID, real SDK-enum) alongside it
• tests/test_rules_network.py: kept dev's full AZ-NET-004 through AZ-NET-015 coverage, added the COR-specific AZ-NET-003 tests (case-insensitive direction, plural prefixes, real SDK-enum) alongside it

Ran the full validation you asked for:

• ruff check . / ruff format --check . → clean
• pytest tests/test_rules_network.py tests/test_rules_database.py tests/test_clean_scan.py -v → all passing
• pytest tests/ --cov=api --cov=scanner --cov-report=term --cov-fail-under=25 → 277 passed, 3 skipped, 73.36% coverage

Still open: the Trivy container scan failure – haven't dug into that yet, will take a look, but flagging in case it's a known/accepted baseline issue on the maintainer side.

Ready for another review whenever you get a chance.

Thank you.

[Vishnu2707]

@parthrohit22 @SHAURYAKSHARMA24 , i believe all the changes that you have requested has been implemented, have a final review when u have a chance and do approve the PR.

Thanks @safidnadaf

[parthrohit22]

Thanks @safidnadaf for addressing the review feedback and resolving the conflicts carefully against the latest dev branch.

Approved.

I reviewed the updated implementation and tests after commit 3693893. The PR now correctly addresses the #151 scope:

• Azure SDK enum-backed values are normalised safely.
• AZ-NET-003 handles case-insensitive direction checks and both singular/plural source prefixes.
• AZ-DB-002 avoids false positives on failed policy lookups.
• Malformed ARM IDs are skipped safely.
• Existing dev test coverage was preserved and the new regression cases are included.
• All current CI checks are passing. fix(scanner)- resolve COR-001-004 scanner correctness issues (#151)

I found no remaining blocking correctness, security or reliability issue within the scope of #151.

Remarks:

• The test helper appears to initialise _diagnostic_settings more than once. This is harmless but should be cleaned up when that helper is next modified.

• Some AZ-DB-002 enabled/disabled scenarios appear to have overlapping regression coverage. This is not harmful, although unnecessary duplication can make future maintenance noisier.

Assumptions / follow-up scope:

• This review assumes the intended unrestricted-source values for Issue #151 remain the values currently handled by the rule. IPv6 ::/0 support was not part of the stated acceptance criteria and should be assessed separately.

• The rule currently evaluates destination_port_range. Support for destination_port_ranges may be worth a separate correctness issue if Azure resources can express the relevant 443 exposure only through the plural field.

• Returning an empty resource group from parse_resource_id prevents the original KeyError, but other scanner rules may still need explicit malformed-ID skip/log handling. I did not treat that broader cross-rule hardening as part of this PR.

These remarks and assumptions are non-blocking and should not delay this correction.

[Vishnu2707]

Once @SHAURYAKSHARMA24 and @ritiksah141 , approves as they have req changes, i will merge the PR, looks fine to me.

[Vishnu2707]

Approved.

[ritiksah141]

Just need one duplication removal of the self._diagnostic_settings: Dict[str, Optional[bool]] = {} the mock_azure.py thats holding my approval otherwise everyting looks good

[SHAURYAKSHARMA24]

Approved. I revalidated the current d1cffbe head against current dev. The Azure SDK enum handling, plural source-prefix logic, SQL auditing-policy behavior, malformed-ID guard, and real-SDK regression coverage satisfy the #151 scope. All checks pass and the current merge result is conflict-free. IPv6 ::/0, plural destination ports, and broader malformed-ID hardening remain follow-up work rather than blockers.