fix: Enhance PII checks to fail on new/existing PII fields in no_pii annotated models - #559
fix: Enhance PII checks to fail on new/existing PII fields in no_pii annotated models#559Akanshu-2u wants to merge 10 commits into
Conversation
Akanshu-2u
changed the title
| if self._pii_terms_cache is not None: | ||
| return | ||
| cfg = self.linter.config | ||
| raw_terms = getattr(cfg, "pii_terms", ["email", "username"]) |
There was a problem hiding this comment.
Add None as default and validate if its None then throw an error.
|
|
||
| # Message definitions | ||
| msgs = { | ||
| ("W%d33" % BASE_ID): ( |
There was a problem hiding this comment.
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?
|
|
||
| # Message definitions | ||
| msgs = { | ||
| ("W%d33" % BASE_ID): ( |
There was a problem hiding this comment.
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.)?
| pii_fields = self._collect_pii_fields(node) | ||
| for field_name, field_node in pii_fields: | ||
| self.add_message( | ||
| "pii-invalid-no-pii-annotation", |
There was a problem hiding this comment.
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.
|
|
||
|
|
||
| @check_visitors | ||
| class PiiAnnotationChecker(BaseChecker): |
There was a problem hiding this comment.
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.
| if self._is_pii_name(child.target.name): | ||
| found.append((child.target.name, child)) | ||
|
|
||
| # Instance attributes set inside methods: ``self.email = ...`` |
There was a problem hiding this comment.
What about self.email: str = ...?
| """Parse pii-terms config on first call within a module.""" | ||
| if self._parsed_pii_terms is not None: | ||
| return | ||
| cfg = self.linter.config |
There was a problem hiding this comment.
I would recommend a better variable name than cfg
|
|
||
| Substring match of any pii-term inside *name* → PII. | ||
| """ | ||
| lower = name.lower() |
There was a problem hiding this comment.
I would recommend a better variable name than lower
| 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) |
There was a problem hiding this comment.
_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?
3 participants
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:
.. no_pii:... 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:
Inline comment example to bypass linter check (False positives):
Private JIRA Link:
BOMS-587