Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix: prevent saving empty cve #2669

Merged
merged 3 commits into from
Oct 2, 2020
Merged

fix: prevent saving empty cve #2669

merged 3 commits into from
Oct 2, 2020

Conversation

edersonbrilhante
Copy link
Contributor

Description:

The fields CVE in fiends allows null value, however, when the finding is updated empty is overwritten this.
I notified this issue when I was using an openapi client consuming the API:
raise ValueError("Invalid value for `cve`, length must be greater than or equal to `1`")

label: bugfix

@Maffooch
Copy link
Contributor

Was there no migration generated with the change to models.py?

Copy link
Collaborator

@madchap madchap left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Missing the migration file definitely.

@edersonbrilhante
Copy link
Contributor Author

Hello @madchap and @Maffooch :)
Thank you for the review.

Django is not generating the migrations.

https://docs.djangoproject.com/en/2.2/topics/db/models/#field-options
Django document says:

null
If True, Django will store empty values as NULL in the database. Default is False.

blank
If True, the field is allowed to be blank. Default is False.

Note that this is different than null. null is purely database-related, whereas blank is validation-related. If a field has blank=True, form validation will allow entry of an empty value. If a field has blank=False, the field will be required.

Just changes in null are needed migrations

@madchap
Copy link
Collaborator

madchap commented Jul 14, 2020

Turns out that's true! Sorry about that, and thanks for double-checking @edersonbrilhante !

@madchap madchap requested review from madchap and a team July 14, 2020 14:54
@valentijnscholten
Copy link
Member

valentijnscholten commented Jul 14, 2020

Have you tested it to be sure that nothing breaks by seeing a None value instead of '' or 0?

@edersonbrilhante
Copy link
Contributor Author

Hi :).
Sorry for the late reply.

  • With value 0

Screenshot 2020-07-16 at 19 51 29

  • With empty string

Screenshot 2020-07-16 at 19 51 18

  • With value null

Screenshot 2020-07-16 at 19 51 00

  • With valid string

Screenshot 2020-07-16 at 19 51 58

  • With invalid string

Screenshot 2020-07-16 at 19 56 23

Copy link
Contributor

@Maffooch Maffooch left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

That was a fun fact about Django models. Thanks @edersonbrilhante!

@valentijnscholten
Copy link
Member

Are these screenshots from the API? What I meant was under the hood the cve field is used for certain things like deduplication and I was wondering if that all still works. What if existing findings have cve as '' or as 0 and new findings get 'None' for cve. Will the dedupe still work? Will the hash be the same? Also there are some if cve kind of constructs in templates, which should be checked but I think those are fine because for 0 or '' or None python always returns False I think

@mtesauro mtesauro self-requested a review August 21, 2020 02:36
Copy link
Contributor

@mtesauro mtesauro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Approved

@edersonbrilhante
Copy link
Contributor Author

Hi @valentijnscholten, I was busy with another project, so I couldn't check this earlier :(.
Yes, these screenshots are from API.
I will check your questions

@edersonbrilhante
Copy link
Contributor Author

Just remembering, I want to fix the issue that overwrites the cve field from None to '', when I am using the interface.
The hash is not changed when the finding is updated. So parsers with cve = None are ok.

Parsers with cve = ''

  • dojo/tools/anchore_engine/parser.py
  • dojo/tools/ibm_app/parser.py
  • dojo/tools/snyk/parser.py
  • dojo/tools/sonatype/parser.py
    I can change the parser to set None as default value, because I see that as a bug. What do you think?

@madchap
Copy link
Collaborator

madchap commented Sep 4, 2020

I think they should be updated, yes. If you can take care of them, it's be great :)

@edersonbrilhante
Copy link
Contributor Author

Just remembering, I want to fix the issue that overwrites the cve field from None to '', when I am using the interface.
The hash is not changed when the finding is updated. So parsers with cve = None are ok.

Parsers with cve = ''

  • dojo/tools/anchore_engine/parser.py
  • dojo/tools/ibm_app/parser.py
  • dojo/tools/snyk/parser.py
  • dojo/tools/sonatype/parser.py
    I can change the parser to set None as default value, because I see that as a bug. What do you think?

Hi folks.
First, sorry for fixing this so late. I changed the files:

  • dojo/tools/anchore_engine/parser.py
  • dojo/tools/ibm_app/parser.py
  • dojo/tools/snyk/parser.py
  • dojo/tools/sonatype/parser.py

@madchap madchap dismissed their stale review October 2, 2020 11:26

addressed

@madchap madchap merged commit d08fd0a into DefectDojo:dev Oct 2, 2020
@edersonbrilhante edersonbrilhante deleted the fix_finding_cve_empty branch October 13, 2020 09:55
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants