Skip to content

fix(validation): prevent mixed IPv4/IPv6 allowlist crashes#645

Merged
utksh1 merged 5 commits into
utksh1:mainfrom
jyotsnak1603:fix-607-ipv4-ipv6-validation
Jun 7, 2026
Merged

fix(validation): prevent mixed IPv4/IPv6 allowlist crashes#645
utksh1 merged 5 commits into
utksh1:mainfrom
jyotsnak1603:fix-607-ipv4-ipv6-validation

Conversation

@jyotsnak1603
Copy link
Copy Markdown
Contributor

@jyotsnak1603 jyotsnak1603 commented Jun 7, 2026

Description

This PR fixes a safe-mode validation bug where mixed IPv4 and IPv6 entries in allowed_networks could raise an uncaught TypeError during target validation.

Changes included:

  • Added an IP-version guard before calling subnet_of() and overlaps() in _net_within_allowed_networks()
  • Skips mixed-version network comparisons safely instead of raising an exception
  • Preserves existing same-version CIDR and wildcard matching behavior
  • Added regression tests for:
    • IPv4 target with IPv6 allowlist
    • IPv6 target with IPv4 allowlist
    • Mixed allowlists where a later same-version entry still allows the target

Related Issues

Fixes #607

Type of Change

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

How Has This Been Tested?

Executed:

py -3.11 -m pytest testing/backend/unit/test_validation.py -q

Result:

42 passed

Verified that:

  • Mixed IPv4/IPv6 allowlist entries no longer raise a TypeError
  • Same-version CIDR matching behavior remains unchanged
  • A later same-version allowlist entry is still evaluated correctly after mixed-version entries are skipped

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.

Copilot AI review requested due to automatic review settings June 7, 2026 06:01
Copilot AI reviewed Jun 7, 2026
View reviewed changes
Copy link
Copy Markdown

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Note

Copilot was unable to run its full agentic suite in this review.

This PR hardens safe-mode target validation when allowed_networks contains mixed IPv4/IPv6 CIDRs, preventing version-mismatch errors and adding regression coverage.

Changes:

  • Skip allowed-network entries whose IP version (v4/v6) doesn’t match the target network during safe-mode checks.
  • Add unit tests covering mixed-version allowed_networks scenarios to ensure validation doesn’t crash.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
testing/backend/unit/test_validation.py Adds regression tests for mixed IPv4/IPv6 allowed_networks handling.
backend/secuscan/validation.py Prevents cross-version ipaddress operations by skipping mismatched network versions.

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

Comment thread testing/backend/unit/test_validation.py Outdated
Comment on lines +1 to +4
# pyrefly: ignore [missing-import]
import pytest
import socket
from backend.secuscan.config import settings
Comment thread testing/backend/unit/test_validation.py Outdated
Comment on lines +70 to +78
def test_validate_target_ipv6_with_ipv4_allowed_network_does_not_crash(monkeypatch):
monkeypatch.setattr(settings, "allowed_networks", ["127.0.0.0/8"])
ok, msg = validate_target("::1", safe_mode=True)

assert ok is False
assert msg in {
"Public IPs/networks not allowed in safe mode (SecuScan Guardrail)",
"Target not within allowed networks in safe mode (SecuScan Guardrail)",
}
Comment thread backend/secuscan/validation.py
Comment on lines 72 to 76
allowed_net = ipaddress.ip_network(pattern, strict=False)
if net.version != allowed_net.version:
continue
if net.subnet_of(allowed_net) or net.overlaps(allowed_net):
return True
@jyotsnak1603
Copy link
Copy Markdown
Contributor Author

jyotsnak1603 commented Jun 7, 2026

I rebased this branch on the latest upstream/main and pushed the refreshed branch.

The remaining CI failures appear unrelated to this PR scope. This PR only modifies:

  • backend/secuscan/validation.py
  • testing/backend/unit/test_validation.py

The failing checks are currently coming from backend/secuscan/workflows.py and frontend unit tests, which are outside this PR’s changes.

Local targeted validation tests are passing:

py -3.11 -m pytest testing/backend/unit/test_validation.py -q

Result: 43 passed.

@utksh1 utksh1 added level:intermediate 35 pts difficulty label for moderate contributor PRs type:bug Bug fix work category bonus label type:testing Testing work category bonus label area:backend Backend API, database, or service work labels Jun 7, 2026
utksh1
utksh1 requested changes Jun 7, 2026
View reviewed changes
Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

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

The IPv4/IPv6 allowlist fix itself is small and in the right area, but this head is not merge-ready because required checks are red: backend-lint and frontend-checks fail, and backend-tests/benchmark are skipped. Please rebase on the current CI baseline and get the full check set green, then request review again.

@jyotsnak1603 jyotsnak1603 force-pushed the fix-607-ipv4-ipv6-validation branch from d496a3a to e1fff40 Compare June 7, 2026 09:53
@jyotsnak1603
Copy link
Copy Markdown
Contributor Author

jyotsnak1603 commented Jun 7, 2026

Hi @utksh1,

I rebased the branch on the latest upstream/main and force-pushed the refreshed head.

All required CI checks are now passing successfully, including backend-lint, backend-tests, frontend-checks, and benchmark.

Local targeted validation tests are also passing:

py -3.11 -m pytest testing/backend/unit/test_validation.py -q

Result: 43 passed.

Requesting re-review when convenient. Thank you.

utksh1
utksh1 approved these changes Jun 7, 2026
View reviewed changes
Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

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

Re-reviewed the latest update. The mixed IPv4/IPv6 allowlist crash fix is focused, has symmetric regression coverage, and visible checks are green. Approving.

@utksh1 utksh1 added the gssoc:approved Admin validation: approved for GSSoC scoring label Jun 7, 2026
utksh1
utksh1 approved these changes Jun 7, 2026
View reviewed changes
Copy link
Copy Markdown
Owner

@utksh1 utksh1 left a comment

Choose a reason for hiding this comment

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

Fresh rebase checks are green. The IPv4/IPv6 allowlist crash fix is focused and covered. Approving.

@utksh1
Copy link
Copy Markdown
Owner

utksh1 commented Jun 7, 2026

Approved with fresh green checks after branch update. Manual merge is still blocked by repository branch policy, and auto-merge is disabled for this repository, so I am leaving this approved instead of using admin bypass.

@utksh1 utksh1 merged commit 6541979 into utksh1:main Jun 7, 2026
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

Copilot code review Copilot Copilot left review comments

@utksh1 utksh1 utksh1 approved these changes

Assignees

No one assigned

Labels

area:backend Backend API, database, or service work gssoc:approved Admin validation: approved for GSSoC scoring level:intermediate 35 pts difficulty label for moderate contributor PRs type:bug Bug fix work category bonus label type:testing Testing work category bonus label

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

[BUG] Mixed IPv4/IPv6 allowed_networks can crash safe-mode target validation

3 participants

@jyotsnak1603 @utksh1