From 28d41803415a080fb50e5bf011666e8030fcb151 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 23 Jul 2026 09:23:03 +0000 Subject: [PATCH 01/10] fix: Enhance PII checks to fail on new PII fields in no_pii annotated models --- edx_lint/__init__.py | 2 +- edx_lint/files/pylintrc | 13 + edx_lint/pylint/pii_annotation_check.py | 288 ++++++++++++++++++++++ edx_lint/pylint/plugin.py | 2 + test/plugins/test_pii_annotation_check.py | 208 ++++++++++++++++ 5 files changed, 512 insertions(+), 1 deletion(-) create mode 100644 edx_lint/pylint/pii_annotation_check.py create mode 100644 test/plugins/test_pii_annotation_check.py diff --git a/edx_lint/__init__.py b/edx_lint/__init__.py index 6192f130..c73f8125 100644 --- a/edx_lint/__init__.py +++ b/edx_lint/__init__.py @@ -2,4 +2,4 @@ edx_lint standardizes lint configuration and additional plugins for use in Open edX code. """ -__version__ = "6.1.0" +__version__ = "6.2.0" diff --git a/edx_lint/files/pylintrc b/edx_lint/files/pylintrc index e3d2fe51..c9b26736 100644 --- a/edx_lint/files/pylintrc +++ b/edx_lint/files/pylintrc @@ -224,6 +224,9 @@ enable= unrecognized-inline-option, useless-suppression, + # PII safety checks + pii-invalid-no-pii-annotation, + # Disable the message, report, category or checker with the given id(s). You # can either give multiple identifiers separated by comma (,) or put this # option multiple times (only on the command line, not in the configuration @@ -510,3 +513,13 @@ int-import-graph= # Exceptions that will emit a warning when being caught. Defaults to # "builtins.Exception" overgeneral-exceptions=builtins.Exception + + +[PII] + +# Comma-separated list of identifier substrings treated as likely PII. +# Substring matching is used, so 'email' will match 'user_email_address'. +pii-terms = + email, + username, + password diff --git a/edx_lint/pylint/pii_annotation_check.py b/edx_lint/pylint/pii_annotation_check.py new file mode 100644 index 00000000..c24c8758 --- /dev/null +++ b/edx_lint/pylint/pii_annotation_check.py @@ -0,0 +1,288 @@ +""" +PII Annotation Checker — flags Django models annotated ``.. no_pii:`` that +still contain likely-PII fields or instance attributes (W7633). +""" + +import re + +from astroid import exceptions as astroid_exceptions +from astroid import nodes as astroid_nodes +from pylint.checkers import BaseChecker, utils + +from .common import BASE_ID, check_visitors + + +# Regexes that detect ``.. no_pii:`` in class docstrings and comment lines. +_NO_PII_DOCSTRING_RE = re.compile(r"\.\.\s*no_pii", re.IGNORECASE) +_NO_PII_COMMENT_RE = re.compile(r"[\s]*#[\s]*\.\.\s*no_pii", re.IGNORECASE) + +# Number of source lines *above* the ``class`` statement to scan for a +# comment-style ``# .. no_pii:`` annotation. +_ANNOTATION_LOOKAHEAD = 10 + + +def register_checkers(linter): + """Register the PII annotation checker.""" + linter.register_checker(PiiAnnotationChecker(linter)) + + +@check_visitors +class PiiAnnotationChecker(BaseChecker): + """ + Fires ``pii-invalid-no-pii-annotation`` (W7633) when a concrete Django model + is annotated ``.. no_pii:`` but still has fields matching the PII terms list. + Abstract and proxy models are skipped, mirroring django_find_annotations scope. + """ + + name = "pii-annotation-checker" + + # Message definitions + msgs = { + ("W%d33" % BASE_ID): ( + "Django model '%s' is annotated as no_pii but contains likely PII field(s): %s", + "pii-invalid-no-pii-annotation", + "Django model annotated with '.. no_pii:' contains fields that look like PII. " + "Replace the annotation with '.. pii:' and the required metadata. " + "Only concrete (non-abstract, non-proxy) Django Model subclasses are checked, " + "matching the scope of 'code_annotations django_find_annotations'.", + ), + } + + def __init__(self, *args, **kwargs): + super().__init__(*args, **kwargs) + self._source_lines = [] + self._pii_terms_cache = None + self._django_model_bases_cache = None + self._module_classdefs = {} + + @utils.only_required_for_messages("pii-invalid-no-pii-annotation") + def visit_module(self, node): + """Cache source lines and reset all per-module state.""" + # Reset config caches so option values are re-read for each module. + self._init_pii_caches() + self._django_model_bases_cache = None + self._module_classdefs = {} + try: + module_bytes = node.stream().read() + encoding = node.file_encoding or "utf-8" + self._source_lines = module_bytes.decode(encoding).splitlines() + except Exception: + self._source_lines = [] + + def _init_pii_caches(self): + """Reset per-module config caches to None.""" + self._pii_terms_cache = None + + def _ensure_config_cached(self): + """Populate pii-terms cache on first call within a module.""" + if self._pii_terms_cache is not None: + return + cfg = self.linter.config + raw_terms = getattr(cfg, "pii_terms", ["email", "username", "password"]) + self._pii_terms_cache = [t.strip().lower() for t in raw_terms if t.strip()] + + def _pii_terms(self): + self._ensure_config_cached() + return self._pii_terms_cache + + def _is_pii_name(self, name): + """ + Return True if *name* is a likely PII identifier. + + Substring match of any pii-term inside *name* → PII. + """ + lower = name.lower() + return any(term in lower for term in self._pii_terms()) + + @utils.only_required_for_messages("pii-invalid-no-pii-annotation") + def visit_classdef(self, node): + """ + Detect PII fields in Django model classes annotated with ``.. no_pii:``. + """ + # Index every class definition in the module for same-module ancestry BFS. + self._module_classdefs[node.name] = node + + if not self._is_annotation_eligible_django_model(node): + return + if not self._class_has_no_pii_annotation(node): + return + + pii_fields = self._collect_pii_fields(node) + if pii_fields: + self.add_message( + "pii-invalid-no-pii-annotation", + node=node, + args=(node.name, ", ".join(pii_fields)), + ) + + def _is_annotation_eligible_django_model(self, node): + """ + Return True if *node* is a concrete (non-abstract, non-proxy) Django model. + + Tries astroid's resolved ancestor walk first (works when Django is importable), + then falls back to raw AST base-name BFS for standalone pylint runs. + """ + model_bases = self._django_model_bases() + + # Primary path: astroid type-inference ancestor resolution. + is_model_subclass = False + try: + for ancestor in node.ancestors(): + if ancestor.name in model_bases: + is_model_subclass = True + break + except astroid_exceptions.AstroidError: + # Inference failed (e.g. Django not installed), fall back to raw AST. + pass + + # Fallback: walk raw AST base names for standalone/offline runs. + if not is_model_subclass: + is_model_subclass = self._raw_ast_is_model_subclass(node) + + if not is_model_subclass: + return False + + # Skip abstract and proxy models (detected via inner Meta class). + if any(self._meta_has_true_flag(node, flag) for flag in ("abstract", "proxy")): + return False + + return True + + def _django_model_bases(self): + """ + Return the set of base class names that identify a Django model. + + Lazily initialised from ``pii-django-model-bases`` linter option on + first call per module; reset to None by visit_module so options are + re-read for each module. + """ + if self._django_model_bases_cache is None: + raw = getattr(self.linter.config, "pii_django_model_bases", ["Model"]) + self._django_model_bases_cache = {b.strip() for b in raw if b.strip()} + return self._django_model_bases_cache + + def _raw_ast_is_model_subclass(self, node): + """ + Return True if *node* inherits from a model base by BFS over raw AST names. + + Only classes defined in the same module can be followed transitively. + External bases (e.g. ``django.db.models.Model``) are matched by bare + name against ``pii-django-model-bases``. + """ + model_bases = self._django_model_bases() + visited = set() + queue = list(self._direct_base_names(node)) + while queue: + name = queue.pop(0) + if name in visited: + continue + visited.add(name) + if name in model_bases: + return True + # Follow same-module parent if known. + parent_node = self._module_classdefs.get(name) + if parent_node is not None: + queue.extend(self._direct_base_names(parent_node)) + return False + + @staticmethod + def _direct_base_names(classdef_node): + """ + Yield the simple name of each direct base class in *classdef_node*. + + Handles both ``Name`` nodes (``Model``) and ``Attribute`` nodes + (``models.Model`` → yields ``"Model"``). + """ + for base in classdef_node.bases: + if isinstance(base, astroid_nodes.Name): + yield base.name + elif isinstance(base, astroid_nodes.Attribute): + yield base.attrname + + @staticmethod + def _meta_has_true_flag(classdef_node, flag_name): + """ + Return True if the inner ``Meta`` class sets ``flag_name = True``. + """ + for child in classdef_node.body: + if not (isinstance(child, astroid_nodes.ClassDef) and child.name == "Meta"): + continue + for stmt in child.body: + if not isinstance(stmt, astroid_nodes.Assign): + continue + for target in stmt.targets: + if (isinstance(target, astroid_nodes.AssignName) + and target.name == flag_name + and isinstance(stmt.value, astroid_nodes.Const) + and stmt.value.value is True): + return True + return False + + def _class_has_no_pii_annotation(self, node): + """ + Return True if *node* carries a ``.. no_pii:`` annotation. + + Checks the class docstring first, then comment lines above the class. + """ + return self._docstring_has_no_pii(node) or self._comment_has_no_pii(node) + + def _docstring_has_no_pii(self, node): + """ + Return True if the class docstring contains ``.. no_pii:``. + """ + docstring = node.doc_node.value if node.doc_node else "" + return bool(_NO_PII_DOCSTRING_RE.search(docstring)) + + def _comment_has_no_pii(self, node): + """ + Return True if a ``# .. no_pii:`` comment appears above the class. + + Scans up to ``_ANNOTATION_LOOKAHEAD`` source lines before the class + statement, covering decorators and blank lines between the comment and + the class declaration. + """ + if not self._source_lines: + return False + end = node.lineno - 1 + start = max(0, end - _ANNOTATION_LOOKAHEAD) + for line in self._source_lines[start:end]: + if _NO_PII_COMMENT_RE.match(line): + return True + return False + + def _collect_pii_fields(self, node): + """ + Return all PII-like field name strings found in the class body. + + Scans: + - Class-level ``Assign`` targets: ``email = models.EmailField()`` + - Class-level ``AnnAssign`` targets: ``email: str = ""`` + - ``self.X`` attribute assignments in method bodies, reported as ``"self.X"``. + """ + found = [] + + for child in node.body: + # Class-level simple assignment: ``email = ...`` + if isinstance(child, astroid_nodes.Assign): + for target in child.targets: + if isinstance(target, astroid_nodes.AssignName): + if self._is_pii_name(target.name): + found.append(target.name) + + # Class-level annotated assignment: ``email: str = ""`` + elif isinstance(child, astroid_nodes.AnnAssign): + if isinstance(child.target, astroid_nodes.AssignName): + if self._is_pii_name(child.target.name): + found.append(child.target.name) + + # Instance attributes set inside methods: ``self.email = ...`` + elif isinstance(child, astroid_nodes.FunctionDef): + for stmt in child.nodes_of_class(astroid_nodes.Assign): + for target in stmt.targets: + if (isinstance(target, astroid_nodes.AssignAttr) + and isinstance(target.expr, astroid_nodes.Name) + and target.expr.name == "self" + and self._is_pii_name(target.attrname)): + found.append(f"self.{target.attrname}") + + return found diff --git a/edx_lint/pylint/plugin.py b/edx_lint/pylint/plugin.py index b750c299..e9a711b1 100644 --- a/edx_lint/pylint/plugin.py +++ b/edx_lint/pylint/plugin.py @@ -9,6 +9,7 @@ getattr_check, i18n_check, module_trace, + pii_annotation_check, range_check, super_check, layered_test_check, @@ -21,6 +22,7 @@ getattr_check, i18n_check, module_trace, + pii_annotation_check, range_check, super_check, layered_test_check, diff --git a/test/plugins/test_pii_annotation_check.py b/test/plugins/test_pii_annotation_check.py new file mode 100644 index 00000000..b5948c75 --- /dev/null +++ b/test/plugins/test_pii_annotation_check.py @@ -0,0 +1,208 @@ +"""Tests for PiiAnnotationChecker (pii-invalid-no-pii-annotation / W7633). + +Fires when a concrete Django model has ``.. no_pii:`` but still contains PII fields. +""" + +from .pylint_test import run_pylint + +_ID = "pii-invalid-no-pii-annotation" + + +def _run(source): + return run_pylint(source, _ID) + + +def _has(messages, marker): + return any(m.startswith(f"{marker}:{_ID}:") for m in messages) + + +# -- basic detection ---------------------------------------------------------- + +def test_no_pii_docstring_with_pii_field(): + """.. no_pii: docstring + PII field fires on the class line.""" + source = """\ + class LearnerProfile(Model): #=A + ''' + .. no_pii: Stores only course metadata. + ''' + course_id = None + email = None + """ + messages = _run(source) + assert _has(messages, "A") + assert any("email" in m for m in messages) + + +def test_no_pii_comment_above_class(): + """# .. no_pii: comment above class with PII field fires.""" + source = """\ + # .. no_pii: + class CourseEnrollment(Model): #=A + course_id = None + username = None + """ + messages = _run(source) + assert _has(messages, "A") + assert any("username" in m for m in messages) + + +def test_no_pii_multiple_pii_fields_single_message(): + """Multiple PII fields produce exactly one message listing all.""" + source = """\ + class Profile(Model): #=A + '''.. no_pii:''' + email = None + username = None + phone_number = None + """ + messages = _run(source) + assert len(messages) == 1 + msg = list(messages)[0] + assert "email" in msg and "username" in msg and "phone_number" not in msg + + +def test_no_pii_with_non_pii_fields_ok(): + """.. no_pii: with genuinely non-PII fields does not fire.""" + source = """\ + class CourseGrade(Model): + '''.. no_pii:''' + is_passing = True + percent = 0.0 + """ + assert not _run(source) + + +def test_class_without_annotation_not_checked(): + """Model with PII fields but no annotation is out of scope for this rule.""" + source = """\ + class BadModel(Model): + email = None + username = None + """ + assert not _run(source) + + +def test_pii_annotated_class_not_checked(): + """Model with .. pii: annotation and PII fields is correct — no warning.""" + source = """\ + class UserProfile(Model): + ''' + .. pii: Stores learner email. + .. pii_types: email_address + .. pii_retirement: local_api + ''' + email = None + """ + assert not _run(source) + + +def test_no_pii_instance_attr_in_method_flagged(): + """self.username = ... inside __init__ of a .. no_pii: model fires.""" + source = """\ + class UserData(Model): #=A + '''.. no_pii:''' + def __init__(self, data): + self.username = data.username + self.is_active = data.active + """ + messages = _run(source) + assert _has(messages, "A") + assert any("self.username" in m for m in messages) + + +def test_no_pii_annotated_assignment_flagged(): + """email: str = '' (AnnAssign) on a .. no_pii: model fires.""" + source = """\ + class Profile(Model): #=A + '''.. no_pii:''' + email: str = "" + """ + assert _has(_run(source), "A") + + +def test_no_pii_inline_disable_suppresses(): + """Inline pylint:disable=pii-invalid-no-pii-annotation suppresses the rule.""" + source = """\ + class Profile(Model): # pylint: disable=pii-invalid-no-pii-annotation + '''.. no_pii:''' + email = None + """ + assert not _run(source) + + +def test_decorator_between_comment_annotation_and_class(): + """Decorator between # .. no_pii: comment and class is still detected.""" + source = """\ + # .. no_pii: + @some_decorator + class Enrollment(Model): #=A + username = None + """ + assert _has(_run(source), "A") + + +# -- model eligibility (mirrors django_find_annotations scope) ---------------- + +def test_plain_python_class_not_checked(): + """Plain Python class (not a Model subclass) is not in scope.""" + source = """\ + class ServiceHelper: + '''.. no_pii:''' + email = None + username = None + """ + assert not _run(source) + + +def test_abstract_django_model_not_checked(): + """Abstract Django model (Meta.abstract = True) is not checked.""" + source = """\ + class AbstractBase(Model): + '''.. no_pii:''' + email = None + class Meta: + abstract = True + """ + assert not _run(source) + + +def test_proxy_django_model_not_checked(): + """Proxy Django model (Meta.proxy = True) is not checked.""" + source = """\ + class ConcreteModel(Model): + '''.. no_pii:''' + course_id = None + + class ProxyView(ConcreteModel): + '''.. no_pii:''' + email = None + class Meta: + proxy = True + """ + assert not _run(source) + + +def test_concrete_model_indirect_inheritance_checked(): + """Concrete model inheriting Model indirectly is still checked.""" + source = """\ + class TimeStampedModel(Model): + '''.. no_pii:''' + course_id = None + + class CourseEnrollment(TimeStampedModel): #=A + '''.. no_pii:''' + username = None + """ + messages = _run(source) + assert _has(messages, "A") + assert any("username" in m for m in messages) + + +def test_non_model_class_with_pii_but_no_annotation_ignored(): + """Plain class with PII fields and no annotation — not checked.""" + source = """\ + class DataTransferObject: + email = None + username = None + """ + assert not _run(source) From d169c01c642fe8de33f32a49f8ea202cf3396f3a Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Thu, 23 Jul 2026 10:32:37 +0000 Subject: [PATCH 02/10] fix: added linter --- CHANGELOG.rst | 5 +++++ edx_lint/pylint/pii_annotation_check.py | 2 +- 2 files changed, 6 insertions(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 8fd45242..09b1d9a4 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,11 @@ Change Log Unreleased ~~~~~~~~~~ +6.2.0 - 2026-06-26 +~~~~~~~~~~~~~~~~~~~ + +Add `pii-invalid-no-pii-annotation` checker to fail when a new tentative PII field is introduced in a Django model annotated as `.. no_pii:`. + 5.7.0 - 2025-04-21 ~~~~~~~~~~~~~~~~~~ diff --git a/edx_lint/pylint/pii_annotation_check.py b/edx_lint/pylint/pii_annotation_check.py index c24c8758..f706cc32 100644 --- a/edx_lint/pylint/pii_annotation_check.py +++ b/edx_lint/pylint/pii_annotation_check.py @@ -66,7 +66,7 @@ def visit_module(self, node): module_bytes = node.stream().read() encoding = node.file_encoding or "utf-8" self._source_lines = module_bytes.decode(encoding).splitlines() - except Exception: + except Exception: # pylint: disable=broad-except self._source_lines = [] def _init_pii_caches(self): From 5bbb13b46789c3e578c7f7f1223683a253d58c68 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Fri, 24 Jul 2026 10:07:27 +0000 Subject: [PATCH 03/10] fix: removed password from pii-terms --- edx_lint/files/pylintrc | 3 +-- edx_lint/pylint/pii_annotation_check.py | 2 +- 2 files changed, 2 insertions(+), 3 deletions(-) diff --git a/edx_lint/files/pylintrc b/edx_lint/files/pylintrc index c9b26736..b8707210 100644 --- a/edx_lint/files/pylintrc +++ b/edx_lint/files/pylintrc @@ -521,5 +521,4 @@ overgeneral-exceptions=builtins.Exception # Substring matching is used, so 'email' will match 'user_email_address'. pii-terms = email, - username, - password + username diff --git a/edx_lint/pylint/pii_annotation_check.py b/edx_lint/pylint/pii_annotation_check.py index f706cc32..6f5153c0 100644 --- a/edx_lint/pylint/pii_annotation_check.py +++ b/edx_lint/pylint/pii_annotation_check.py @@ -78,7 +78,7 @@ def _ensure_config_cached(self): if self._pii_terms_cache is not None: return cfg = self.linter.config - raw_terms = getattr(cfg, "pii_terms", ["email", "username", "password"]) + raw_terms = getattr(cfg, "pii_terms", ["email", "username"]) self._pii_terms_cache = [t.strip().lower() for t in raw_terms if t.strip()] def _pii_terms(self): From 1f8b2bddf4e1c355eea72be645c94b5e7d2c7608 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 27 Jul 2026 09:09:22 +0000 Subject: [PATCH 04/10] fix: added field type inline disable --- edx_lint/pylint/pii_annotation_check.py | 16 ++++----- test/plugins/test_pii_annotation_check.py | 42 +++++++++++------------ 2 files changed, 29 insertions(+), 29 deletions(-) diff --git a/edx_lint/pylint/pii_annotation_check.py b/edx_lint/pylint/pii_annotation_check.py index 6f5153c0..c5b78088 100644 --- a/edx_lint/pylint/pii_annotation_check.py +++ b/edx_lint/pylint/pii_annotation_check.py @@ -39,7 +39,7 @@ class PiiAnnotationChecker(BaseChecker): # Message definitions msgs = { ("W%d33" % BASE_ID): ( - "Django model '%s' is annotated as no_pii but contains likely PII field(s): %s", + "Django model '%s' is annotated as no_pii but contains likely PII field: '%s'", "pii-invalid-no-pii-annotation", "Django model annotated with '.. no_pii:' contains fields that look like PII. " "Replace the annotation with '.. pii:' and the required metadata. " @@ -108,11 +108,11 @@ def visit_classdef(self, node): return pii_fields = self._collect_pii_fields(node) - if pii_fields: + for field_name, field_node in pii_fields: self.add_message( "pii-invalid-no-pii-annotation", - node=node, - args=(node.name, ", ".join(pii_fields)), + node=field_node, + args=(node.name, field_name), ) def _is_annotation_eligible_django_model(self, node): @@ -252,7 +252,7 @@ def _comment_has_no_pii(self, node): def _collect_pii_fields(self, node): """ - Return all PII-like field name strings found in the class body. + Return all PII-like field name strings and their AST nodes found in the class body. Scans: - Class-level ``Assign`` targets: ``email = models.EmailField()`` @@ -267,13 +267,13 @@ def _collect_pii_fields(self, node): for target in child.targets: if isinstance(target, astroid_nodes.AssignName): if self._is_pii_name(target.name): - found.append(target.name) + found.append((target.name, child)) # Class-level annotated assignment: ``email: str = ""`` elif isinstance(child, astroid_nodes.AnnAssign): if isinstance(child.target, astroid_nodes.AssignName): if self._is_pii_name(child.target.name): - found.append(child.target.name) + found.append((child.target.name, child)) # Instance attributes set inside methods: ``self.email = ...`` elif isinstance(child, astroid_nodes.FunctionDef): @@ -283,6 +283,6 @@ def _collect_pii_fields(self, node): and isinstance(target.expr, astroid_nodes.Name) and target.expr.name == "self" and self._is_pii_name(target.attrname)): - found.append(f"self.{target.attrname}") + found.append((f"self.{target.attrname}", stmt)) return found diff --git a/test/plugins/test_pii_annotation_check.py b/test/plugins/test_pii_annotation_check.py index b5948c75..565bbe8c 100644 --- a/test/plugins/test_pii_annotation_check.py +++ b/test/plugins/test_pii_annotation_check.py @@ -21,12 +21,12 @@ def _has(messages, marker): def test_no_pii_docstring_with_pii_field(): """.. no_pii: docstring + PII field fires on the class line.""" source = """\ - class LearnerProfile(Model): #=A + class LearnerProfile(Model): ''' .. no_pii: Stores only course metadata. ''' course_id = None - email = None + email = None #=A """ messages = _run(source) assert _has(messages, "A") @@ -37,9 +37,9 @@ def test_no_pii_comment_above_class(): """# .. no_pii: comment above class with PII field fires.""" source = """\ # .. no_pii: - class CourseEnrollment(Model): #=A + class CourseEnrollment(Model): course_id = None - username = None + username = None #=A """ messages = _run(source) assert _has(messages, "A") @@ -47,18 +47,18 @@ class CourseEnrollment(Model): #=A def test_no_pii_multiple_pii_fields_single_message(): - """Multiple PII fields produce exactly one message listing all.""" + """Multiple PII fields produce exactly one message per field.""" source = """\ - class Profile(Model): #=A + class Profile(Model): '''.. no_pii:''' - email = None - username = None + email = None #=A + username = None #=B phone_number = None """ messages = _run(source) - assert len(messages) == 1 - msg = list(messages)[0] - assert "email" in msg and "username" in msg and "phone_number" not in msg + assert len(messages) == 2 + assert _has(messages, "A") + assert _has(messages, "B") def test_no_pii_with_non_pii_fields_ok(): @@ -99,10 +99,10 @@ class UserProfile(Model): def test_no_pii_instance_attr_in_method_flagged(): """self.username = ... inside __init__ of a .. no_pii: model fires.""" source = """\ - class UserData(Model): #=A + class UserData(Model): '''.. no_pii:''' def __init__(self, data): - self.username = data.username + self.username = data.username #=A self.is_active = data.active """ messages = _run(source) @@ -113,9 +113,9 @@ def __init__(self, data): def test_no_pii_annotated_assignment_flagged(): """email: str = '' (AnnAssign) on a .. no_pii: model fires.""" source = """\ - class Profile(Model): #=A + class Profile(Model): '''.. no_pii:''' - email: str = "" + email: str = "" #=A """ assert _has(_run(source), "A") @@ -123,9 +123,9 @@ class Profile(Model): #=A def test_no_pii_inline_disable_suppresses(): """Inline pylint:disable=pii-invalid-no-pii-annotation suppresses the rule.""" source = """\ - class Profile(Model): # pylint: disable=pii-invalid-no-pii-annotation + class Profile(Model): '''.. no_pii:''' - email = None + email = None # pylint: disable=pii-invalid-no-pii-annotation """ assert not _run(source) @@ -135,8 +135,8 @@ def test_decorator_between_comment_annotation_and_class(): source = """\ # .. no_pii: @some_decorator - class Enrollment(Model): #=A - username = None + class Enrollment(Model): + username = None #=A """ assert _has(_run(source), "A") @@ -189,9 +189,9 @@ class TimeStampedModel(Model): '''.. no_pii:''' course_id = None - class CourseEnrollment(TimeStampedModel): #=A + class CourseEnrollment(TimeStampedModel): '''.. no_pii:''' - username = None + username = None #=A """ messages = _run(source) assert _has(messages, "A") From 894511d947bb39379e0e10533e97c626c1ec01c5 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 27 Jul 2026 12:18:25 +0000 Subject: [PATCH 05/10] fix: modified changelog --- CHANGELOG.rst | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 09b1d9a4..619615f7 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -16,7 +16,7 @@ Unreleased 6.2.0 - 2026-06-26 ~~~~~~~~~~~~~~~~~~~ -Add `pii-invalid-no-pii-annotation` checker to fail when a new tentative PII field is introduced in a Django model annotated as `.. no_pii:`. +* Added the `pii-invalid-no-pii-annotation` checker to validate that Django models claiming `.. no_pii:` do not actually contain PII fields. 5.7.0 - 2025-04-21 ~~~~~~~~~~~~~~~~~~ From d331504c94ba26e2b4fe0a2ce44c531ec0d489fb Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Tue, 28 Jul 2026 10:55:14 +0000 Subject: [PATCH 06/10] fix: modified the default value and the var name change --- edx_lint/pylint/pii_annotation_check.py | 60 ++++++++++++++++------- test/plugins/test_pii_annotation_check.py | 2 +- 2 files changed, 43 insertions(+), 19 deletions(-) diff --git a/edx_lint/pylint/pii_annotation_check.py b/edx_lint/pylint/pii_annotation_check.py index c5b78088..f132fb2f 100644 --- a/edx_lint/pylint/pii_annotation_check.py +++ b/edx_lint/pylint/pii_annotation_check.py @@ -48,19 +48,41 @@ class PiiAnnotationChecker(BaseChecker): ), } + # Options must be defined on the checker so it can be configured independently + options = ( + ( + "pii-terms", + { + "default": None, + "type": "csv", + "metavar": "", + "help": "List of PII-like terms to flag.", + }, + ), + ( + "pii-django-model-bases", + { + "default": "Model", + "type": "csv", + "metavar": "", + "help": "Base class *names* that identify a Django model.", + }, + ), + ) + def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) self._source_lines = [] - self._pii_terms_cache = None - self._django_model_bases_cache = None + self._parsed_pii_terms = None + self._parsed_django_model_bases = None self._module_classdefs = {} @utils.only_required_for_messages("pii-invalid-no-pii-annotation") def visit_module(self, node): """Cache source lines and reset all per-module state.""" - # Reset config caches so option values are re-read for each module. - self._init_pii_caches() - self._django_model_bases_cache = None + # Reset parsed configs so option values are re-read for each module. + self._reset_parsed_config() + self._parsed_django_model_bases = None self._module_classdefs = {} try: module_bytes = node.stream().read() @@ -69,21 +91,23 @@ def visit_module(self, node): except Exception: # pylint: disable=broad-except self._source_lines = [] - def _init_pii_caches(self): - """Reset per-module config caches to None.""" - self._pii_terms_cache = None + def _reset_parsed_config(self): + """Reset per-module parsed configs to None.""" + self._parsed_pii_terms = None - def _ensure_config_cached(self): - """Populate pii-terms cache on first call within a module.""" - if self._pii_terms_cache is not None: + def _parse_and_store_config(self): + """Parse pii-terms config on first call within a module.""" + if self._parsed_pii_terms is not None: return cfg = self.linter.config - raw_terms = getattr(cfg, "pii_terms", ["email", "username"]) - self._pii_terms_cache = [t.strip().lower() for t in raw_terms if t.strip()] + raw_terms = getattr(cfg, "pii_terms", None) + if raw_terms is None: + raise ValueError("The 'pii_terms' setting must be configured.") + self._parsed_pii_terms = [t.strip().lower() for t in raw_terms if t.strip()] def _pii_terms(self): - self._ensure_config_cached() - return self._pii_terms_cache + self._parse_and_store_config() + return self._parsed_pii_terms def _is_pii_name(self, name): """ @@ -156,10 +180,10 @@ def _django_model_bases(self): first call per module; reset to None by visit_module so options are re-read for each module. """ - if self._django_model_bases_cache is None: + if self._parsed_django_model_bases is None: raw = getattr(self.linter.config, "pii_django_model_bases", ["Model"]) - self._django_model_bases_cache = {b.strip() for b in raw if b.strip()} - return self._django_model_bases_cache + self._parsed_django_model_bases = {b.strip() for b in raw if b.strip()} + return self._parsed_django_model_bases def _raw_ast_is_model_subclass(self, node): """ diff --git a/test/plugins/test_pii_annotation_check.py b/test/plugins/test_pii_annotation_check.py index 565bbe8c..ed8bfc8f 100644 --- a/test/plugins/test_pii_annotation_check.py +++ b/test/plugins/test_pii_annotation_check.py @@ -9,7 +9,7 @@ def _run(source): - return run_pylint(source, _ID) + return run_pylint(source, _ID, "--pii-terms=email,username") def _has(messages, marker): From 85ed381dbd67f3b9475a513e2608074aacbef4e3 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Tue, 28 Jul 2026 12:15:54 +0000 Subject: [PATCH 07/10] fix: modified the likely pii message and removed likely term --- edx_lint/files/pylintrc | 2 +- edx_lint/pylint/pii_annotation_check.py | 10 +++++----- 2 files changed, 6 insertions(+), 6 deletions(-) diff --git a/edx_lint/files/pylintrc b/edx_lint/files/pylintrc index b8707210..0f4ebc98 100644 --- a/edx_lint/files/pylintrc +++ b/edx_lint/files/pylintrc @@ -517,7 +517,7 @@ overgeneral-exceptions=builtins.Exception [PII] -# Comma-separated list of identifier substrings treated as likely PII. +# Comma-separated list of identifier substrings treated as PII. # Substring matching is used, so 'email' will match 'user_email_address'. pii-terms = email, diff --git a/edx_lint/pylint/pii_annotation_check.py b/edx_lint/pylint/pii_annotation_check.py index f132fb2f..948961b6 100644 --- a/edx_lint/pylint/pii_annotation_check.py +++ b/edx_lint/pylint/pii_annotation_check.py @@ -1,6 +1,6 @@ """ PII Annotation Checker — flags Django models annotated ``.. no_pii:`` that -still contain likely-PII fields or instance attributes (W7633). +still contain PII fields or instance attributes (W7633). """ import re @@ -39,7 +39,7 @@ class PiiAnnotationChecker(BaseChecker): # Message definitions msgs = { ("W%d33" % BASE_ID): ( - "Django model '%s' is annotated as no_pii but contains likely PII field: '%s'", + "Django model '%s' is annotated as no_pii but contains PII field: '%s'", "pii-invalid-no-pii-annotation", "Django model annotated with '.. no_pii:' contains fields that look like PII. " "Replace the annotation with '.. pii:' and the required metadata. " @@ -56,7 +56,7 @@ class PiiAnnotationChecker(BaseChecker): "default": None, "type": "csv", "metavar": "", - "help": "List of PII-like terms to flag.", + "help": "List of PII terms to flag.", }, ), ( @@ -111,7 +111,7 @@ def _pii_terms(self): def _is_pii_name(self, name): """ - Return True if *name* is a likely PII identifier. + Return True if *name* is a PII identifier. Substring match of any pii-term inside *name* → PII. """ @@ -276,7 +276,7 @@ def _comment_has_no_pii(self, node): def _collect_pii_fields(self, node): """ - Return all PII-like field name strings and their AST nodes found in the class body. + Return all PII field name strings and their AST nodes found in the class body. Scans: - Class-level ``Assign`` targets: ``email = models.EmailField()`` From 1cd18b0d42b0bf24884d997d32361a4d31ceeaef Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 3 Aug 2026 09:53:16 +0000 Subject: [PATCH 08/10] fix: fixed the bleed of annotation --- edx_lint/pylint/pii_annotation_check.py | 40 ++++++++++------------- test/plugins/test_pii_annotation_check.py | 14 ++++++++ 2 files changed, 32 insertions(+), 22 deletions(-) diff --git a/edx_lint/pylint/pii_annotation_check.py b/edx_lint/pylint/pii_annotation_check.py index 948961b6..9793824d 100644 --- a/edx_lint/pylint/pii_annotation_check.py +++ b/edx_lint/pylint/pii_annotation_check.py @@ -28,27 +28,21 @@ def register_checkers(linter): @check_visitors class PiiAnnotationChecker(BaseChecker): - """ - Fires ``pii-invalid-no-pii-annotation`` (W7633) when a concrete Django model - is annotated ``.. no_pii:`` but still has fields matching the PII terms list. - Abstract and proxy models are skipped, mirroring django_find_annotations scope. - """ + """Flags concrete Django models annotated ``.. no_pii:`` that contain PII fields (W7633).""" name = "pii-annotation-checker" + PII_INVALID_ANNOTATION_MESSAGE_ID = "pii-invalid-no-pii-annotation" # Message definitions msgs = { ("W%d33" % BASE_ID): ( "Django model '%s' is annotated as no_pii but contains PII field: '%s'", - "pii-invalid-no-pii-annotation", - "Django model annotated with '.. no_pii:' contains fields that look like PII. " - "Replace the annotation with '.. pii:' and the required metadata. " - "Only concrete (non-abstract, non-proxy) Django Model subclasses are checked, " - "matching the scope of 'code_annotations django_find_annotations'.", + PII_INVALID_ANNOTATION_MESSAGE_ID, + "Model claims no_pii but has PII-named fields. Update annotation to '.. pii:' or rename the field.", ), } - # Options must be defined on the checker so it can be configured independently + # Options must be defined on the checker so it can be configured independently options = ( ( "pii-terms", @@ -118,7 +112,7 @@ def _is_pii_name(self, name): lower = name.lower() return any(term in lower for term in self._pii_terms()) - @utils.only_required_for_messages("pii-invalid-no-pii-annotation") + @utils.only_required_for_messages(PII_INVALID_ANNOTATION_MESSAGE_ID) def visit_classdef(self, node): """ Detect PII fields in Django model classes annotated with ``.. no_pii:``. @@ -134,7 +128,7 @@ def visit_classdef(self, node): pii_fields = self._collect_pii_fields(node) for field_name, field_node in pii_fields: self.add_message( - "pii-invalid-no-pii-annotation", + self.PII_INVALID_ANNOTATION_MESSAGE_ID, node=field_node, args=(node.name, field_name), ) @@ -258,17 +252,19 @@ def _docstring_has_no_pii(self, node): return bool(_NO_PII_DOCSTRING_RE.search(docstring)) def _comment_has_no_pii(self, node): - """ - Return True if a ``# .. no_pii:`` comment appears above the class. - - Scans up to ``_ANNOTATION_LOOKAHEAD`` source lines before the class - statement, covering decorators and blank lines between the comment and - the class declaration. - """ + """Return True if a ``# .. no_pii:`` comment appears above the class.""" if not self._source_lines: return False - end = node.lineno - 1 - start = max(0, end - _ANNOTATION_LOOKAHEAD) + end = node.lineno - 1 # line just before the ``class`` keyword + parent = node.parent + if isinstance(parent, astroid_nodes.Module): + start = 0 + for sibling in parent.body: + if sibling is node: + break + start = sibling.tolineno # last line of each preceding sibling + else: + start = max(0, end - _ANNOTATION_LOOKAHEAD) # fallback: nested class for line in self._source_lines[start:end]: if _NO_PII_COMMENT_RE.match(line): return True diff --git a/test/plugins/test_pii_annotation_check.py b/test/plugins/test_pii_annotation_check.py index ed8bfc8f..2bc78063 100644 --- a/test/plugins/test_pii_annotation_check.py +++ b/test/plugins/test_pii_annotation_check.py @@ -206,3 +206,17 @@ class DataTransferObject: username = None """ assert not _run(source) + + +def test_comment_annotation_no_bleed_across_class_boundary(): + """# .. no_pii: on SmallModel must not bleed into adjacent NearbyModel.""" + source = """\ + # .. no_pii: + class SmallModel(Model): + count = None + + class NearbyModel(Model): + email = None #=B + """ + messages = _run(source) + assert not _has(messages, "B") From dc5dc1f0f27997c9006f1509d7efe3b42f520530 Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 3 Aug 2026 11:05:03 +0000 Subject: [PATCH 09/10] fix: removed implementation of comment check --- edx_lint/pylint/pii_annotation_check.py | 41 +++-------------------- test/plugins/test_pii_annotation_check.py | 38 --------------------- 2 files changed, 4 insertions(+), 75 deletions(-) diff --git a/edx_lint/pylint/pii_annotation_check.py b/edx_lint/pylint/pii_annotation_check.py index 9793824d..e61402dd 100644 --- a/edx_lint/pylint/pii_annotation_check.py +++ b/edx_lint/pylint/pii_annotation_check.py @@ -12,13 +12,8 @@ from .common import BASE_ID, check_visitors -# Regexes that detect ``.. no_pii:`` in class docstrings and comment lines. +# Regex that detects ``.. no_pii:`` in class docstrings. _NO_PII_DOCSTRING_RE = re.compile(r"\.\.\s*no_pii", re.IGNORECASE) -_NO_PII_COMMENT_RE = re.compile(r"[\s]*#[\s]*\.\.\s*no_pii", re.IGNORECASE) - -# Number of source lines *above* the ``class`` statement to scan for a -# comment-style ``# .. no_pii:`` annotation. -_ANNOTATION_LOOKAHEAD = 10 def register_checkers(linter): @@ -66,24 +61,17 @@ class PiiAnnotationChecker(BaseChecker): def __init__(self, *args, **kwargs): super().__init__(*args, **kwargs) - self._source_lines = [] self._parsed_pii_terms = None self._parsed_django_model_bases = None self._module_classdefs = {} @utils.only_required_for_messages("pii-invalid-no-pii-annotation") def visit_module(self, node): - """Cache source lines and reset all per-module state.""" + """Reset all per-module state.""" # Reset parsed configs so option values are re-read for each module. self._reset_parsed_config() self._parsed_django_model_bases = None self._module_classdefs = {} - try: - module_bytes = node.stream().read() - encoding = node.file_encoding or "utf-8" - self._source_lines = module_bytes.decode(encoding).splitlines() - except Exception: # pylint: disable=broad-except - self._source_lines = [] def _reset_parsed_config(self): """Reset per-module parsed configs to None.""" @@ -238,11 +226,9 @@ def _meta_has_true_flag(classdef_node, flag_name): def _class_has_no_pii_annotation(self, node): """ - Return True if *node* carries a ``.. no_pii:`` annotation. - - Checks the class docstring first, then comment lines above the class. + Return True if the class docstring carries a ``.. no_pii:`` annotation. """ - return self._docstring_has_no_pii(node) or self._comment_has_no_pii(node) + return self._docstring_has_no_pii(node) def _docstring_has_no_pii(self, node): """ @@ -251,25 +237,6 @@ def _docstring_has_no_pii(self, node): docstring = node.doc_node.value if node.doc_node else "" return bool(_NO_PII_DOCSTRING_RE.search(docstring)) - def _comment_has_no_pii(self, node): - """Return True if a ``# .. no_pii:`` comment appears above the class.""" - if not self._source_lines: - return False - end = node.lineno - 1 # line just before the ``class`` keyword - parent = node.parent - if isinstance(parent, astroid_nodes.Module): - start = 0 - for sibling in parent.body: - if sibling is node: - break - start = sibling.tolineno # last line of each preceding sibling - else: - start = max(0, end - _ANNOTATION_LOOKAHEAD) # fallback: nested class - for line in self._source_lines[start:end]: - if _NO_PII_COMMENT_RE.match(line): - return True - return False - def _collect_pii_fields(self, node): """ Return all PII field name strings and their AST nodes found in the class body. diff --git a/test/plugins/test_pii_annotation_check.py b/test/plugins/test_pii_annotation_check.py index 2bc78063..65686137 100644 --- a/test/plugins/test_pii_annotation_check.py +++ b/test/plugins/test_pii_annotation_check.py @@ -33,19 +33,6 @@ class LearnerProfile(Model): assert any("email" in m for m in messages) -def test_no_pii_comment_above_class(): - """# .. no_pii: comment above class with PII field fires.""" - source = """\ - # .. no_pii: - class CourseEnrollment(Model): - course_id = None - username = None #=A - """ - messages = _run(source) - assert _has(messages, "A") - assert any("username" in m for m in messages) - - def test_no_pii_multiple_pii_fields_single_message(): """Multiple PII fields produce exactly one message per field.""" source = """\ @@ -130,17 +117,6 @@ class Profile(Model): assert not _run(source) -def test_decorator_between_comment_annotation_and_class(): - """Decorator between # .. no_pii: comment and class is still detected.""" - source = """\ - # .. no_pii: - @some_decorator - class Enrollment(Model): - username = None #=A - """ - assert _has(_run(source), "A") - - # -- model eligibility (mirrors django_find_annotations scope) ---------------- def test_plain_python_class_not_checked(): @@ -206,17 +182,3 @@ class DataTransferObject: username = None """ assert not _run(source) - - -def test_comment_annotation_no_bleed_across_class_boundary(): - """# .. no_pii: on SmallModel must not bleed into adjacent NearbyModel.""" - source = """\ - # .. no_pii: - class SmallModel(Model): - count = None - - class NearbyModel(Model): - email = None #=B - """ - messages = _run(source) - assert not _has(messages, "B") From e52099cd99d868b706c66fd1ee8ede741fd5455d Mon Sep 17 00:00:00 2001 From: Akanshu Aich Date: Mon, 10 Aug 2026 06:35:11 +0000 Subject: [PATCH 10/10] fix: added check for self.email = str: check --- edx_lint/pylint/pii_annotation_check.py | 109 ++++++++++++++-------- test/plugins/test_pii_annotation_check.py | 20 +++- 2 files changed, 88 insertions(+), 41 deletions(-) diff --git a/edx_lint/pylint/pii_annotation_check.py b/edx_lint/pylint/pii_annotation_check.py index e61402dd..6f6e80cb 100644 --- a/edx_lint/pylint/pii_annotation_check.py +++ b/edx_lint/pylint/pii_annotation_check.py @@ -23,7 +23,7 @@ def register_checkers(linter): @check_visitors class PiiAnnotationChecker(BaseChecker): - """Flags concrete Django models annotated ``.. no_pii:`` that contain PII fields (W7633).""" + """Flags concrete ``.. no_pii:`` Django models that still contain PII.""" name = "pii-annotation-checker" PII_INVALID_ANNOTATION_MESSAGE_ID = "pii-invalid-no-pii-annotation" @@ -31,13 +31,16 @@ class PiiAnnotationChecker(BaseChecker): # Message definitions msgs = { ("W%d33" % BASE_ID): ( - "Django model '%s' is annotated as no_pii but contains PII field: '%s'", + "Django model '%s' is annotated as no_pii " + "but contains PII field: '%s'", PII_INVALID_ANNOTATION_MESSAGE_ID, - "Model claims no_pii but has PII-named fields. Update annotation to '.. pii:' or rename the field.", + "Model claims no_pii but has PII-named fields. " + "Update annotation to '.. pii:' or rename the field.", ), } - # Options must be defined on the checker so it can be configured independently + # Options must be defined on the checker so it can be configured + # independently. options = ( ( "pii-terms", @@ -66,7 +69,7 @@ def __init__(self, *args, **kwargs): self._module_classdefs = {} @utils.only_required_for_messages("pii-invalid-no-pii-annotation") - def visit_module(self, node): + def visit_module(self, _node): """Reset all per-module state.""" # Reset parsed configs so option values are re-read for each module. self._reset_parsed_config() @@ -81,11 +84,13 @@ def _parse_and_store_config(self): """Parse pii-terms config on first call within a module.""" if self._parsed_pii_terms is not None: return - cfg = self.linter.config - raw_terms = getattr(cfg, "pii_terms", None) + linter_config = self.linter.config + raw_terms = getattr(linter_config, "pii_terms", None) if raw_terms is None: raise ValueError("The 'pii_terms' setting must be configured.") - self._parsed_pii_terms = [t.strip().lower() for t in raw_terms if t.strip()] + self._parsed_pii_terms = [ + term.strip().lower() for term in raw_terms if term.strip() + ] def _pii_terms(self): self._parse_and_store_config() @@ -97,20 +102,21 @@ def _is_pii_name(self, name): Substring match of any pii-term inside *name* → PII. """ - lower = name.lower() - return any(term in lower for term in self._pii_terms()) + normalized_name = name.lower() + return any(term in normalized_name for term in self._pii_terms()) @utils.only_required_for_messages(PII_INVALID_ANNOTATION_MESSAGE_ID) def visit_classdef(self, node): """ - Detect PII fields in Django model classes annotated with ``.. no_pii:``. + Detect PII fields in Django model classes annotated + with ``.. no_pii:``. """ - # Index every class definition in the module for same-module ancestry BFS. + # Index every class definition for same-module ancestry BFS. self._module_classdefs[node.name] = node if not self._is_annotation_eligible_django_model(node): return - if not self._class_has_no_pii_annotation(node): + if not self._docstring_has_no_pii(node): return pii_fields = self._collect_pii_fields(node) @@ -123,10 +129,12 @@ def visit_classdef(self, node): def _is_annotation_eligible_django_model(self, node): """ - Return True if *node* is a concrete (non-abstract, non-proxy) Django model. + Return True if *node* is a concrete (non-abstract, non-proxy) + Django model. - Tries astroid's resolved ancestor walk first (works when Django is importable), - then falls back to raw AST base-name BFS for standalone pylint runs. + Tries astroid's resolved ancestor walk first (works when Django is + importable), then falls back to raw AST base-name BFS for standalone + pylint runs. """ model_bases = self._django_model_bases() @@ -138,7 +146,8 @@ def _is_annotation_eligible_django_model(self, node): is_model_subclass = True break except astroid_exceptions.AstroidError: - # Inference failed (e.g. Django not installed), fall back to raw AST. + # Inference failed (e.g. Django not installed). + # Fall back to raw AST. pass # Fallback: walk raw AST base names for standalone/offline runs. @@ -149,7 +158,10 @@ def _is_annotation_eligible_django_model(self, node): return False # Skip abstract and proxy models (detected via inner Meta class). - if any(self._meta_has_true_flag(node, flag) for flag in ("abstract", "proxy")): + if any( + self._meta_has_true_flag(node, flag) + for flag in ("abstract", "proxy") + ): return False return True @@ -163,13 +175,20 @@ def _django_model_bases(self): re-read for each module. """ if self._parsed_django_model_bases is None: - raw = getattr(self.linter.config, "pii_django_model_bases", ["Model"]) - self._parsed_django_model_bases = {b.strip() for b in raw if b.strip()} + raw = getattr( + self.linter.config, + "pii_django_model_bases", + ["Model"], + ) + self._parsed_django_model_bases = { + base.strip() for base in raw if base.strip() + } return self._parsed_django_model_bases def _raw_ast_is_model_subclass(self, node): """ - Return True if *node* inherits from a model base by BFS over raw AST names. + Return True if *node* inherits from a model base using BFS over + raw AST names. Only classes defined in the same module can be followed transitively. External bases (e.g. ``django.db.models.Model``) are matched by bare @@ -211,25 +230,24 @@ def _meta_has_true_flag(classdef_node, flag_name): Return True if the inner ``Meta`` class sets ``flag_name = True``. """ for child in classdef_node.body: - if not (isinstance(child, astroid_nodes.ClassDef) and child.name == "Meta"): + if not ( + isinstance(child, astroid_nodes.ClassDef) + and child.name == "Meta" + ): continue for stmt in child.body: if not isinstance(stmt, astroid_nodes.Assign): continue for target in stmt.targets: - if (isinstance(target, astroid_nodes.AssignName) - and target.name == flag_name - and isinstance(stmt.value, astroid_nodes.Const) - and stmt.value.value is True): + if ( + isinstance(target, astroid_nodes.AssignName) + and target.name == flag_name + and isinstance(stmt.value, astroid_nodes.Const) + and stmt.value.value is True + ): return True return False - def _class_has_no_pii_annotation(self, node): - """ - Return True if the class docstring carries a ``.. no_pii:`` annotation. - """ - return self._docstring_has_no_pii(node) - def _docstring_has_no_pii(self, node): """ Return True if the class docstring contains ``.. no_pii:``. @@ -239,12 +257,14 @@ def _docstring_has_no_pii(self, node): def _collect_pii_fields(self, node): """ - Return all PII field name strings and their AST nodes found in the class body. + Return PII-like field names and their AST nodes found in + the class body. Scans: - Class-level ``Assign`` targets: ``email = models.EmailField()`` - Class-level ``AnnAssign`` targets: ``email: str = ""`` - - ``self.X`` attribute assignments in method bodies, reported as ``"self.X"``. + - Method-body instance assignments (both ``Assign`` and ``AnnAssign``): + ``self.email = ...`` and ``self.email: str = ...``. """ found = [] @@ -266,10 +286,23 @@ def _collect_pii_fields(self, node): elif isinstance(child, astroid_nodes.FunctionDef): for stmt in child.nodes_of_class(astroid_nodes.Assign): for target in stmt.targets: - if (isinstance(target, astroid_nodes.AssignAttr) - and isinstance(target.expr, astroid_nodes.Name) - and target.expr.name == "self" - and self._is_pii_name(target.attrname)): + if ( + isinstance(target, astroid_nodes.AssignAttr) + and isinstance(target.expr, astroid_nodes.Name) + and target.expr.name == "self" + and self._is_pii_name(target.attrname) + ): found.append((f"self.{target.attrname}", stmt)) + # Annotated instance assignments: ``self.email: str = ...`` + for stmt in child.nodes_of_class(astroid_nodes.AnnAssign): + target = stmt.target + if ( + isinstance(target, astroid_nodes.AssignAttr) + and isinstance(target.expr, astroid_nodes.Name) + and target.expr.name == "self" + and self._is_pii_name(target.attrname) + ): + found.append((f"self.{target.attrname}", stmt)) + return found diff --git a/test/plugins/test_pii_annotation_check.py b/test/plugins/test_pii_annotation_check.py index 65686137..2e965d89 100644 --- a/test/plugins/test_pii_annotation_check.py +++ b/test/plugins/test_pii_annotation_check.py @@ -1,6 +1,7 @@ """Tests for PiiAnnotationChecker (pii-invalid-no-pii-annotation / W7633). -Fires when a concrete Django model has ``.. no_pii:`` but still contains PII fields. +Fires when a concrete Django model has ``.. no_pii:`` but still contains +PII fields. """ from .pylint_test import run_pylint @@ -60,7 +61,7 @@ class CourseGrade(Model): def test_class_without_annotation_not_checked(): - """Model with PII fields but no annotation is out of scope for this rule.""" + """Model with PII fields but no annotation is out of scope.""" source = """\ class BadModel(Model): email = None @@ -97,6 +98,19 @@ def __init__(self, data): assert any("self.username" in m for m in messages) +def test_no_pii_annotated_instance_attr_in_method_flagged(): + """self.email: str = ... inside method of a .. no_pii: model fires.""" + source = """\ + class UserData(Model): + '''.. no_pii:''' + def __init__(self, value): + self.email: str = value #=A + """ + messages = _run(source) + assert _has(messages, "A") + assert any("self.email" in m for m in messages) + + def test_no_pii_annotated_assignment_flagged(): """email: str = '' (AnnAssign) on a .. no_pii: model fires.""" source = """\ @@ -108,7 +122,7 @@ class Profile(Model): def test_no_pii_inline_disable_suppresses(): - """Inline pylint:disable=pii-invalid-no-pii-annotation suppresses the rule.""" + """Inline disable for pii-invalid-no-pii-annotation suppresses the rule.""" source = """\ class Profile(Model): '''.. no_pii:'''