fix: Enhance PII checks to fail on new/existing PII fields in no_pii annotated models by Akanshu-2u · Pull Request #559 · openedx/edx-lint

Akanshu-2u · 2026-07-23T09:32:14Z

Description:

This PR introduces a new pylint checker (pii-invalid-no-pii-annotation / W7633) to ensure that Django models explicitly marked as not containing PII remain compliant with OEP-0030. It enforces this compliance by failing CI if any model annotated with .. no_pii: contains fields that match known PII terms. This ensures that existing incorrect annotations are audited and corrected, and prevents developers from accidentally merging new sensitive fields into these models without proper review and annotation updates.

Solution:

Created a new Pylint plugin: Added a checker that specifically targets concrete Django models.
Annotation validation: The checker parses model docstrings and preceding comment lines to identify classes explicitly annotated with .. no_pii: .
Strict field checking: When a .. no_pii: model is found, the checker scans its class attributes and init assignments. If any tentative PII field is detected (matching the configurable pii-terms), it raises a W7633 warning. This forces the developer to either rename the non-sensitive field or update the model's metadata to .. pii: .

PII detection while testing on CI:

https://github.com/Akanshu-2u/edx-platform/actions/runs/31375567882/job/93413902806

Inline comment example to bypass linter check (False positives):

    # pylint: disable-next=pii-invalid-no-pii-annotation  # field does not store user PII data, safe under OEP-30
    email = models.BooleanField(default=False, null=False, blank=False)

Private JIRA Link:

BOMS-587

… models

Akanshu-2u · 2026-07-28T09:17:58Z

+        if self._pii_terms_cache is not None:
+            return
+        cfg = self.linter.config
+        raw_terms = getattr(cfg, "pii_terms", ["email", "username"])


Add None as default and validate if its None then throw an error.

Implemented.

ttak-apphelix · 2026-07-31T10:11:50Z

+
+    # Message definitions
+    msgs = {
+        ("W%d33" % BASE_ID): (


Every annotation-compliance check in annotations_check.py (feature-toggle-needs-doc, toggle-no-name, setting-boolean-default-value, etc.) is registered as an E (error), not W. Since this enforces the same category of thing — OEP-30 compliance intended to fail CI — should this be E7633 for consistency, rather than W7633?

Answered in this #559 (comment) .

ttak-apphelix · 2026-07-31T10:20:36Z

+
+    # Message definitions
+    msgs = {
+        ("W%d33" % BASE_ID): (


Can we confirm W7633 doesn't collide with an existing message ID under BASE_ID in another checker (range_check.py, super_check.py, getattr_check.py, etc.)?

Yes, I confirmed that W7633 does not collide with any existing message ID under BASE_ID across the other checkers (range_check.py, super_check.py, getattr_check.py, etc.).

I’ll keep W7633 as-is since the E76xx series in annotations_check.py is reserved for OEP toggle/setting compliance, while W7630/W7633 belong to the PII checker family.

ttak-apphelix · 2026-07-31T10:22:39Z

+        pii_fields = self._collect_pii_fields(node)
+        for field_name, field_node in pii_fields:
+            self.add_message(
+                "pii-invalid-no-pii-annotation",


Elsewhere in the codebase (TOGGLE_NOT_ANNOTATED_MESSAGE_ID = "feature-toggle-needs-doc"), the message symbol is a class-level constant referenced in both msgs and add_message(). Here "pii-invalid-no-pii-annotation" is inlined as a bare string in both places — minor, but worth aligning with the existing pattern for consistency.

implemented.

ttak-apphelix · 2026-07-31T11:40:11Z

+
+
+@check_visitors
+class PiiAnnotationChecker(BaseChecker):


annotations_check.py already has AnnotationBaseChecker built on code_annotations.find_static.StaticSearch/AnnotationConfig for parsing .. toggle_name:-style annotations, and .. no_pii:/.. pii: follow the same annotation grammar per OEP-30. Could this checker subclass AnnotationBaseChecker instead of using custom regexes + a 10-line source lookahead? The hand-rolled scan is fragile (e.g. two classes declared close together could bleed a no_pii comment across class boundaries) and duplicates parsing logic this repo already solves correctly elsewhere.

Before the changes, AnnotationBaseChecker / StaticSearch was mainly designed for multi-field annotation groups and loads YAML from code_annotations/contrib/config/. There is no pii_annotations.yaml there.

Since no_pii is a standalone annotation rather than an annotation group, iter_groups() doesn't cover it. Also, StaticSearch only returns the file, line number, and annotation, so we'd still need additional logic to determine which ClassDef owns that annotation.

So using it would add complexity without solving the core issue more directly than the current approach.

rahulkanneri-2u · 2026-08-07T08:48:20Z

+                    if self._is_pii_name(child.target.name):
+                        found.append((child.target.name, child))
+
+            # Instance attributes set inside methods: ``self.email = ...``


What about self.email: str = ...?

Good catch. We only handled self.email = ... Assign in methods; self.email: str = ... is AnnAssign and was a gap. I’ve added AnnAssign handling for instance attrs and a regression test test_no_pii_annotated_instance_attr_in_method_flagged.

rahulkanneri-2u · 2026-08-07T08:49:52Z

+        """Parse pii-terms config on first call within a module."""
+        if self._parsed_pii_terms is not None:
+            return
+        cfg = self.linter.config


I would recommend a better variable name than cfg

Renamed cfg to linter_config.

rahulkanneri-2u · 2026-08-07T08:50:14Z

+
+        Substring match of any pii-term inside *name* → PII.
+        """
+        lower = name.lower()


I would recommend a better variable name than lower

Updated for readability: renamed lower to normalized_name.

rahulkanneri-2u · 2026-08-07T08:55:54Z

+    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)


_class_has_no_pii_annotation just calls _docstring_has_no_pii with no extra logic, So can we just change line number 113 to call _docstring_has_no_pii directly and delete this (_class_has_no_pii_annotation) wrapper?

Good suggestion. I inlined the call to _docstring_has_no_pii(node) in visit_classdef and removed _class_has_no_pii_annotation since it had no additional logic.

bmtcril

Thanks for this, seems reasonable to me after a rebase!

Akanshu-2u added 4 commits July 23, 2026 09:23

fix: Enhance PII checks to fail on new PII fields in no_pii annotated…

28d4180

… models

fix: added linter

d169c01

fix: removed password from pii-terms

5bbb13b

fix: added field type inline disable

1f8b2bd

Akanshu-2u changed the title ~~fix: Enhance PII checks to fail on new PII fields in no_pii annotated models~~ fix: Enhance PII checks to fail on new/existing PII fields in no_pii annotated models Jul 27, 2026

Akanshu-2u added 2 commits July 27, 2026 17:37

Merge branch 'master' into aaich/BOMS-587-annotation

e2914d2

fix: modified changelog

894511d

Akanshu-2u commented Jul 28, 2026

View reviewed changes

Akanshu-2u added 2 commits July 28, 2026 10:55

fix: modified the default value and the var name change

d331504

fix: modified the likely pii message and removed likely term

85ed381

ttak-apphelix reviewed Jul 31, 2026

View reviewed changes

Akanshu-2u added 2 commits August 3, 2026 09:53

fix: fixed the bleed of annotation

1cd18b0

fix: removed implementation of comment check

dc5dc1f

rahulkanneri-2u reviewed Aug 7, 2026

View reviewed changes

fix: added check for self.email = str: check

e52099c

rahulkanneri-2u approved these changes Aug 10, 2026

View reviewed changes

Akanshu-2u requested a review from bmtcril August 10, 2026 10:53

bmtcril reviewed Aug 13, 2026

View reviewed changes

Merge branch 'master' into aaich/BOMS-587-annotation

9088747

bmtcril approved these changes Aug 14, 2026

View reviewed changes

bmtcril merged commit ec1b6a1 into openedx:master Aug 14, 2026
5 checks passed

This was referenced Aug 25, 2026

fix: added inline commit lint openedx/openedx-platform#39022

Merged

fix: added inline commit lint edx/edx-platform#442

Merged

Uh oh!

Conversation

Akanshu-2u commented Jul 23, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Description:

Solution:

PII detection while testing on CI:

Inline comment example to bypass linter check (False positives):

Private JIRA Link:

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

bmtcril left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Akanshu-2u commented Jul 23, 2026 •

edited

Loading