semgrep-rules icon indicating copy to clipboard operation
semgrep-rules copied to clipboard

django nontext-field-must-set-null-true false positive on django.contrib.gis.db.models

Open tino opened this issue 2 years ago • 6 comments

Describe the bug The rule:python.django.correctness.nontext-field-must-set-null-true.nontext-field-must-set-null-true triggers false positives when models is imported from django.contrib.gis.db.

To Reproduce

from django.contrib.gis.db import models

class MyModel(models.Model):
    shape = models.PolygonField(dim=3, srid=4326)
    grouper_id = models.CharField(max_length=50, blank=True)  # <-- fails on this line

output:

❯ semgrep --config https://semgrep.dev/p/django app/geodata/models.py
using config from https://semgrep.dev/p/django. Visit https://semgrep.dev/registry to see all public rules.
downloading config...
running 35 rules...
100%...
app/geodata/models.py
severity:error rule:python.django.correctness.nontext-field-must-set-null-true.nontext-field-must-set-null-true: null=True should be set if blank=True is set on non-text fields.
65:    grouper_id = models.CharField(max_length=50, blank=True)

Expected behavior Shouldn't fail

Priority How important is this to you?

  • [ ] P0: blocking me from making progress
  • [ ] P1: this will block me in the near future
  • [x] P2: annoying but not blocking me (I'll #nosemgrep for now :)

Version: 0.61.0

tino avatar Aug 17 '21 18:08 tino

Oh btw, how does one disable this single rule? I mean there is no way to do #nosemgrep python.django.correctness.nontext-field-must-set-null-true.nontext-field-must-set-null-true without going over my 88 line-length...

tino avatar Aug 17 '21 18:08 tino

I noticed this also fails for subclassed fields. For example with fields that serialize in some way (json, encryption, etc), this also fails:

class EncryptedField(models.TextField):
    # implement en/decryption in .get_prep_value() & .from_db_value
   ...

class FakeModel(models.Model):
  # ok: nontext-field-must-set-null-true
  fieldText = EncryptedField(blank=True)

Can we update the rule so it is smart enough to deal with this?

tino avatar Nov 16 '21 16:11 tino

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs. Thank you for your contributions.

stale[bot] avatar Mar 29 '22 01:03 stale[bot]

Stale-bot has closed this stale item. Please reopen it if this is in error.

stale[bot] avatar Apr 07 '22 09:04 stale[bot]

Thanks @ievans. Anything I can do to make this more clear? Or what should I expect in terms of answers here?

tino avatar Apr 19 '22 15:04 tino

Oh btw, how does one disable this single rule? I mean there is no way to do #nosemgrep python.django.correctness.nontext-field-must-set-null-true.nontext-field-must-set-null-true without going over my 88 line-length...

There's not currently a way to exclude individual rules... 😅 There is an old issue on the Semgrep repo (here) that you can add your voice to if you feel inclined.

I noticed this also fails for subclassed fields. For example with fields that serialize in some way (json, encryption, etc), this also fails ... Can we update the rule so it is smart enough to deal with this?

We can make a small update, but it will only work if the subclassed field is defined in the same file. Semgrep analysis is limited to single files right now so if the subclass is in a different file, Semgrep won't pick it up.

minusworld avatar May 16 '22 22:05 minusworld