semgrep / semgrep-rules

Semgrep rules registry
https://semgrep.dev/registry
Other
809 stars 396 forks source link

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

Open tino opened 3 years ago

tino commented 3 years ago

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?

Version: 0.61.0

tino commented 3 years ago

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 commented 2 years ago

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?

stale[bot] commented 2 years ago

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] commented 2 years ago

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

tino commented 2 years ago

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

minusworld commented 2 years ago

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.