diff --git a/CHANGELOG.rst b/CHANGELOG.rst index 8fd45242..619615f7 100644 --- a/CHANGELOG.rst +++ b/CHANGELOG.rst @@ -13,6 +13,11 @@ Change Log Unreleased ~~~~~~~~~~ +6.2.0 - 2026-06-26 +~~~~~~~~~~~~~~~~~~~ + +* 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 ~~~~~~~~~~~~~~~~~~ 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..0f4ebc98 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,12 @@ 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 PII. +# Substring matching is used, so 'email' will match 'user_email_address'. +pii-terms = + email, + username diff --git a/edx_lint/pylint/pii_annotation_check.py b/edx_lint/pylint/pii_annotation_check.py new file mode 100644 index 00000000..6f6e80cb --- /dev/null +++ b/edx_lint/pylint/pii_annotation_check.py @@ -0,0 +1,308 @@ +""" +PII Annotation Checker — flags Django models annotated ``.. no_pii:`` that +still contain 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 + + +# Regex that detects ``.. no_pii:`` in class docstrings. +_NO_PII_DOCSTRING_RE = re.compile(r"\.\.\s*no_pii", re.IGNORECASE) + + +def register_checkers(linter): + """Register the PII annotation checker.""" + linter.register_checker(PiiAnnotationChecker(linter)) + + +@check_visitors +class PiiAnnotationChecker(BaseChecker): + """Flags concrete ``.. no_pii:`` Django models that still contain PII.""" + + 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_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 = ( + ( + "pii-terms", + { + "default": None, + "type": "csv", + "metavar": "", + "help": "List of PII 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._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): + """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 = {} + + def _reset_parsed_config(self): + """Reset per-module parsed configs to None.""" + self._parsed_pii_terms = 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 + 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 = [ + term.strip().lower() for term in raw_terms if term.strip() + ] + + def _pii_terms(self): + self._parse_and_store_config() + return self._parsed_pii_terms + + def _is_pii_name(self, name): + """ + Return True if *name* is a PII identifier. + + Substring match of any pii-term inside *name* → PII. + """ + 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:``. + """ + # 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._docstring_has_no_pii(node): + return + + pii_fields = self._collect_pii_fields(node) + for field_name, field_node in pii_fields: + self.add_message( + self.PII_INVALID_ANNOTATION_MESSAGE_ID, + node=field_node, + args=(node.name, field_name), + ) + + 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._parsed_django_model_bases is None: + 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 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 + 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 _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 _collect_pii_fields(self, node): + """ + 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 = ""`` + - Method-body instance assignments (both ``Assign`` and ``AnnAssign``): + ``self.email = ...`` and ``self.email: str = ...``. + """ + 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, 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, child)) + + # 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}", 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/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..2e965d89 --- /dev/null +++ b/test/plugins/test_pii_annotation_check.py @@ -0,0 +1,198 @@ +"""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, "--pii-terms=email,username") + + +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): + ''' + .. no_pii: Stores only course metadata. + ''' + course_id = None + email = None #=A + """ + messages = _run(source) + assert _has(messages, "A") + assert any("email" 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 = """\ + class Profile(Model): + '''.. no_pii:''' + email = None #=A + username = None #=B + phone_number = None + """ + messages = _run(source) + assert len(messages) == 2 + assert _has(messages, "A") + assert _has(messages, "B") + + +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.""" + 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): + '''.. no_pii:''' + def __init__(self, data): + self.username = data.username #=A + 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_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 = """\ + class Profile(Model): + '''.. no_pii:''' + email: str = "" #=A + """ + assert _has(_run(source), "A") + + +def test_no_pii_inline_disable_suppresses(): + """Inline disable for pii-invalid-no-pii-annotation suppresses the rule.""" + source = """\ + class Profile(Model): + '''.. no_pii:''' + email = None # pylint: disable=pii-invalid-no-pii-annotation + """ + assert not _run(source) + + +# -- 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): + '''.. no_pii:''' + username = None #=A + """ + 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)