Skip to content

Conversation

@danparizher
Copy link
Contributor

Summary

Updated S508 (snmp-insecure-version) and S509 (snmp-weak-cryptography) rules to support both old and new PySNMP API module paths. Previously, these rules only detected the old API path pysnmp.hlapi.*, but now they correctly detect all PySNMP API variants including pysnmp.hlapi.asyncio.*, pysnmp.hlapi.v1arch.*, pysnmp.hlapi.v3arch.*, and pysnmp.hlapi.auth.*.

Fixes #21364

Problem Analysis

The S508 and S509 rules used exact pattern matching on qualified names:

  • S509 only matched ["pysnmp", "hlapi", "UsmUserData"]
  • S508 only matched ["pysnmp", "hlapi", "CommunityData"]

This meant that newer PySNMP API paths were not detected, such as:

  • pysnmp.hlapi.asyncio.UsmUserData
  • pysnmp.hlapi.v3arch.asyncio.UsmUserData
  • pysnmp.hlapi.v3arch.asyncio.auth.UsmUserData
  • pysnmp.hlapi.auth.UsmUserData
  • Similar variants for CommunityData in S508

Additionally, the old API path pysnmp.hlapi.auth.* was also missing from both rules.

Approach

Instead of exact pattern matching, both rules now check if:

  1. The qualified name starts with ["pysnmp", "hlapi"]
  2. The qualified name ends with the target class name ("UsmUserData" for S509, "CommunityData" for S508)

This flexible approach matches all PySNMP API paths without hardcoding each variant, making the rules more maintainable and future-proof.

Test Plan

Added comprehensive test cases to both S508.py and S509.py test files covering:

  • New API paths: pysnmp.hlapi.asyncio.*, pysnmp.hlapi.v1arch.*, pysnmp.hlapi.v3arch.*
  • Old API path: pysnmp.hlapi.auth.*
  • Both insecure and secure usage patterns

All existing tests pass, and new snapshot tests were added and accepted. Manual verification confirms both rules correctly detect all PySNMP API variants.

@astral-sh-bot
Copy link

astral-sh-bot bot commented Nov 11, 2025

ruff-ecosystem results

Linter (stable)

✅ ecosystem check detected no linter changes.

Linter (preview)

✅ ecosystem check detected no linter changes.

Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thanks! I had one suggestion about simplifying the segments checks, and let's make this a preview change just to be safe since these are stable rules.

@danparizher danparizher requested a review from ntBre November 13, 2025 23:49
@ntBre ntBre added rule Implementing or modifying a lint rule preview Related to preview mode features labels Nov 19, 2025
Copy link
Contributor

@ntBre ntBre left a comment

Choose a reason for hiding this comment

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

Thanks!

I just reverted the removal of the stable tests and reused the existing preview_rules test runner for these two rules.

@ntBre ntBre merged commit 0d47334 into astral-sh:main Nov 19, 2025
37 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

preview Related to preview mode features rule Implementing or modifying a lint rule

Projects

None yet

Development

Successfully merging this pull request may close these issues.

S508 and S509 don’t support the current PySNMP API

2 participants