Skip to content

fix(utils): sanitize URLs for display by stripping userinfo and trailing slashes (#915) - #952

Open
oboehmer wants to merge 2 commits into
mainfrom
fix/915-url-sanitization
Open

oboehmer wants to merge 2 commits into
mainfrom
fix/915-url-sanitization

Conversation

@oboehmer

@oboehmer oboehmer commented Sep 13, 2026

Copy link
Copy Markdown
Collaborator

Description

Sanitizes controller URLs for display, banners, curl examples, failure reports, and auth check results/logs by stripping embedded user credentials (user:password@ or user@), trailing slashes (/), and surrounding whitespace while preserving scheme, hostname, port, path, query parameters, and fragments. Also hardens extract_host() to strip userinfo and accurately extract host from schemeless host:port URLs.

Closes

Related Issue(s)

Type of Change

  • Bug fix (non-breaking change that fixes an issue)
  • New feature (non-breaking change that adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work as expected)
  • Refactoring / Technical debt (internal improvements with no user-facing changes)
  • Documentation update
  • Chore (build process, CI, tooling, dependencies)
  • Other (please describe):

Test Framework Affected

  • PyATS
  • Robot Framework
  • Both
  • N/A (not test-framework specific)

Network as Code (NaC) Architecture Affected

  • All architectures
  • N/A (architecture-agnostic)

Platform Tested

nac-test supports macOS and Linux only

  • macOS (version tested: 26.6.2)
  • Linux (distro/version tested: )

Key Changes

  • Shared URL parsing primitive (nac_test/utils/url.py):
    • Implemented _split_url() so authority always lands in netloc even without scheme, eliminating code duplication across URL utilities.
    • Added _safe_port() to prevent ValueError crashes on malformed/non-numeric/out-of-range ports (e.g., :notaport or :99999), preserving the raw port text for user-visible typo detection.
    • Implemented sanitize_url_for_display(url: str) -> str using the shared primitive, correctly handling edge cases like @ in unencoded passwords, query params with @, IPv6 brackets, and protocol-relative URLs.
    • Hardened extract_host(url: str) -> str (drive-by fix): strips embedded userinfo (preventing password leaks into ping commands/reports) and corrects schemeless host:port/path parsing which previously returned the port instead of the hostname. Hostnames are consistently normalized to lowercase.
  • Log and Report Leak Prevention:
    • nac_test/core/controller_auth.py: Sanitized URL in preflight_auth_check before logging to logger.info(...), preventing credential leakage into log files.
    • nac_test/core/reporting/combined_generator.py: Sanitized URL directly at the _generate_pre_flight_failure_report boundary for controller_url, host, and _get_curl_example, ensuring HTML failure reports never expose credentials even when called independently.
    • nac_test/cli/ui/banners.py: Sanitized URLs in display_auth_failure_banner and display_unreachable_banner.
    • Maintained separation between display URL (sanitize_url_for_display) and auth cache keys (raw_url.rstrip("/")).
  • Comprehensive Test Coverage:
    • Parametrized TestSanitizeUrlForDisplay and TestExtractHost in tests/unit/utils/test_url.py with explicit test IDs covering standard URLs, embedded credentials, IPv6 literals, malformed ports, @ edge cases, and case normalization.
    • Added leak-regression assertions to tests/unit/core/test_combined_generator.py, tests/unit/core/test_controller_auth.py, and tests/unit/cli/ui/test_banners.py ensuring credentials never appear in stdout, logs, or HTML reports.

Testing Done

  • Unit tests added/updated
  • Integration tests performed
  • Manual testing performed:
    • PyATS tests executed successfully
    • Robot Framework tests executed successfully
    • D2D/SSH tests executed successfully (if applicable)
    • HTML reports generated correctly
  • All existing tests pass (pytest / pre-commit run -a)

Test Commands Used

.venv/bin/pre-commit run --all-files
.venv/bin/python -m pytest tests/unit/utils/test_url.py tests/unit/core/test_controller_auth.py tests/unit/core/test_combined_generator.py tests/unit/cli/ui/test_banners.py -q
.venv/bin/python -m pytest tests/unit tests/pyats_core -n auto --dist loadscope -q

Checklist

  • Code follows project style guidelines (pre-commit run -a passes)
  • Self-review of code completed
  • Code is commented where necessary (especially complex logic)
  • Documentation updated (if applicable)
  • No new warnings introduced
  • Changes work on both macOS and Linux
  • CHANGELOG.md updated (if applicable)

Screenshots (if applicable)

N/A

Additional Notes

None

@oboehmer oboehmer added bug Something isn't working security Security-related issues/vulnerabilities labels Sep 13, 2026
@oboehmer
oboehmer requested a review from aitestino September 13, 2026 07:49
@oboehmer oboehmer self-assigned this Sep 14, 2026
…redential leaks

- Refactor URL parsing onto shared _split_url() primitive in nac_test/utils/url.py
- Handle malformed/non-numeric ports in _safe_port() without raising ValueError
- Strip embedded user credentials and fix schemeless host:port in extract_host()
- Sanitize failure.controller_url directly at the pre-flight failure report boundary
- Add leak regression tests across banners, controller_auth, and combined_generator
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

bug Something isn't working security Security-related issues/vulnerabilities

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Sanitize controller_url on display (scrub embedded credentials; restore trailing-slash strip)

1 participant