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

Mitigated On/Before/After now use DateTimeFilter #11472

Merged
merged 5 commits into from
Jan 16, 2025

Conversation

hblankenship
Copy link
Collaborator

Fixes filtering for mitigation for the on/before/after fields

[sc-7759]

Copy link

dryrunsecurity bot commented Dec 26, 2024

DryRun Security Summary

The code change enhances the Defect Dojo application's security filtering capabilities by introducing more granular filtering options for mitigation dates and EPSS metrics in the dojo/filters.py file.

Expand for full summary

Summary:

This code change appears to be a security-focused update to the dojo/filters.py file in the Defect Dojo application. The key changes include the addition of a new DateTimeFilter and updates to the ApiFindingFilter, FindingFilterHelper, and FindingFilterWithoutObjectLookups classes to introduce more granular filtering capabilities around mitigation dates and EPSS (Exploitability Prediction Score) metrics.

These changes suggest that the application is expanding its ability to filter and sort findings, which could lead to improved security posture by allowing users to more easily identify and focus on findings that may pose a higher risk or have been mitigated in a timely manner. However, it's important to ensure that these new filtering capabilities are properly secured and do not introduce any vulnerabilities, such as potential SQL injection or other injection-based attacks. The application should also validate and sanitize all user input to these filters to prevent malicious input from being used to bypass security controls or access sensitive data.

Files Changed:

  • dojo/filters.py: This file contains various Django filters used for filtering and sorting data in the Defect Dojo application. The key changes in this patch include:
    • Addition of a new DateTimeFilter to the list of imported filters.
    • Updates to the ApiFindingFilter class to add new DateTimeFilter fields for mitigated_on, mitigated_before, and mitigated_after, along with corresponding filtering methods.
    • Updates to the FindingFilterHelper and FindingFilterWithoutObjectLookups classes to add similar changes for mitigated_on, mitigated_before, and mitigated_after fields, as well as new fields for epss_score, epss_score_range, epss_percentile, and epss_percentile_range to allow filtering findings based on their EPSS metrics.

Code Analysis

We ran 9 analyzers against 1 file and 0 analyzers had findings. 9 analyzers had no findings.

View PR in the DryRun Dashboard.

dojo/filters.py Outdated
Comment on lines 1460 to 1462
mitigated_on = DateTimeFilter(field_name="mitigated", lookup_expr="exact")
mitigated_before = DateTimeFilter(field_name="mitigated", lookup_expr="lt")
mitigated_after = DateTimeFilter(field_name="mitigated", lookup_expr="gt")
Copy link
Contributor

Choose a reason for hiding this comment

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

If a user were to be reliant on passing a date to this filter, would the API accept the absence of the timestamp? For example, if the query was mitigated_on=2025-01-02 would a validation error be thrown?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

A validation error does not get thrown if you use 2025-01-02 without the timestamp. Should it?

Copy link
Contributor

Choose a reason for hiding this comment

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

No, a validation error should not be raised. I was asking to make sure that this change would not cause any issues for users that were currently using dates

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

Copy link
Contributor

@cneill cneill left a comment

Choose a reason for hiding this comment

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

This doesn't seem to be operating how I would expect... I spun up a test instance, created some findings, and marked them mitigated. The UI uses a date-picker widget that won't allow you to enter a timestamp, so I couldn't test that.

With the API, I have 3 mitigated findings with these mitigated values:

❯ curl -H "Authorization: Token ${DD_API_KEY}"  "http://localhost:8080/api/v2/findings/" | jq '.results[] | select(.mitigated != null) | .mitigated'

"2025-01-13T18:08:05.911321Z"
"2025-01-13T18:08:05.922679Z"
"2025-01-13T18:08:05.930498Z"

Testing mitigated_on with a simple date:

❯ curl -H "Authorization: Token ${DD_API_KEY}"  "http://localhost:8080/api/v2/findings/?mitigated_on=2025-01-13"
{"count":0,"next":null,"previous":null,"results":[],"prefetch":{}}%

It wants an exact timestamp:

❯ curl -H "Authorization: Token ${DD_API_KEY}"  "http://localhost:8080/api/v2/findings/?mitigated_on=2025-01-13T18:08:05.911321Z"
{"count":1,"next":null,"previous":null,"results":[{"id":1, ... }]}

The mitigated_after and mitigated_before filters sort of work as expected with simple dates, but I assume it is just plugging in a 00:00:00 for the time, so for example this returns my 3 findings that were mitigated today, 2025-01-13, where mitigated_after would only have included things after today (23:59:59) in the past.

❯ curl -H "Authorization: Token ${DD_API_KEY}"  "http://localhost:8080/api/v2/findings/?mitigated_after=2025-01-13"
{"count":3,"next":null,"previous":null,"results":[{"id":1, ... }]}

I think this needs some additional tweaking, and could really use some unit tests.

@hblankenship
Copy link
Collaborator Author

hblankenship commented Jan 14, 2025

This doesn't seem to be operating how I would expect... I spun up a test instance, created some findings, and marked them mitigated. The UI uses a date-picker widget that won't allow you to enter a timestamp, so I couldn't test that.

With the API, I have 3 mitigated findings with these mitigated values:

❯ curl -H "Authorization: Token ${DD_API_KEY}"  "http://localhost:8080/api/v2/findings/" | jq '.results[] | select(.mitigated != null) | .mitigated'

"2025-01-13T18:08:05.911321Z"
"2025-01-13T18:08:05.922679Z"
"2025-01-13T18:08:05.930498Z"

Testing mitigated_on with a simple date:

❯ curl -H "Authorization: Token ${DD_API_KEY}"  "http://localhost:8080/api/v2/findings/?mitigated_on=2025-01-13"
{"count":0,"next":null,"previous":null,"results":[],"prefetch":{}}%

It wants an exact timestamp:

❯ curl -H "Authorization: Token ${DD_API_KEY}"  "http://localhost:8080/api/v2/findings/?mitigated_on=2025-01-13T18:08:05.911321Z"
{"count":1,"next":null,"previous":null,"results":[{"id":1, ... }]}

The mitigated_after and mitigated_before filters sort of work as expected with simple dates, but I assume it is just plugging in a 00:00:00 for the time, so for example this returns my 3 findings that were mitigated today, 2025-01-13, where mitigated_after would only have included things after today (23:59:59) in the past.

❯ curl -H "Authorization: Token ${DD_API_KEY}"  "http://localhost:8080/api/v2/findings/?mitigated_after=2025-01-13"
{"count":3,"next":null,"previous":null,"results":[{"id":1, ... }]}

I think this needs some additional tweaking, and could really use some unit tests.

With simple dates being given, it does indeed use 00:00:00 as the time. The request was to make it use DateTimeFilter which means it expects a full datetime. Per the Story:

The UI and API use a date filter for the mitigated field rather than a datetime filter. The datetime filter should be used instead

So when you said 'after' and gave 2025-01-13, the assumption is that you meant 2025-01-13T00:00:00. That said, I think we can alter the after to add a 23:59:59 timestamp if it is not added instead of the 00:00:00.
In a similar manner, mitigated_on will NOT work without a specific time. DateTimeFilters being what they are. Happy to entertain thoughts on this one.
Also, why do we even have on, before, and after when the mitigated filter is a range filter? I am sure it is so we do not introduce breaking changes...
Finally, should the UI change to use datetime instead of simple date?

Copy link
Contributor

@cneill cneill left a comment

Choose a reason for hiding this comment

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

This one's squirrelly... I'm making some assumptions with my comments here, so others might reasonably disagree. But I tried to capture the spirit of what users are likely to want

Comment on lines +1716 to +1717
if value.hour == 0 and value.minute == 0 and value.second == 0:
value = value.replace(hour=23, minute=59, second=59)
Copy link
Contributor

Choose a reason for hiding this comment

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

This seems like the right way to handle "after" when the user provides a simple date without a timestamp. Given that the UI currently uses a simple date picker without the ability to specify time (which seems fine), this would do what I would intuitively expect. We don't have to worry about the possibility of the user asking for "after 2025-01-01 00:00:00" here unless they're fiddling around in the Developer Console or something.

I suppose we could include microseconds here or use gte <date+1> 00:00:00 to deal with edge cases around the last second of the day...

dojo/filters.py Outdated
mitigated_on = DateFilter(field_name="mitigated", lookup_expr="exact", label="Mitigated On")
mitigated_before = DateFilter(field_name="mitigated", lookup_expr="lt", label="Mitigated Before")
mitigated_after = DateFilter(field_name="mitigated", lookup_expr="gt", label="Mitigated After")
mitigated_on = DateTimeFilter(field_name="mitigated", lookup_expr="exact", label="Mitigated On")
Copy link
Contributor

Choose a reason for hiding this comment

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

Given the simple date-picker presented in the UI, I think searching for anything with a timestamp between [ <date> 00:00:00, <date+1> 00:00:00 ) would be the right approach here. Otherwise, I think this filter will only ever return Findings mitigated at exactly 00:00:00, which would be pretty uncommon.

Comment on lines +1549 to +1550
if value.hour == 0 and value.minute == 0 and value.second == 0:
value = value.replace(hour=23, minute=59, second=59)
Copy link
Contributor

Choose a reason for hiding this comment

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

This logic is a little trickier when it comes to the API, because the user could intentionally query for "after 2025-01-01 00:00:00"... But I think this approach strikes the right balance - it seems much more likely that we'll receive dates without timestamps than dates with explicit 00:00:00 timestamps, and this would mirror the UI's behavior as well. In any case, the user can always search for "after 2024-12-31" to achieve the same thing if that's truly what they want.

dojo/filters.py Outdated
mitigated_on = DateFilter(field_name="mitigated", lookup_expr="exact")
mitigated_before = DateFilter(field_name="mitigated", lookup_expr="lt")
mitigated_after = DateFilter(field_name="mitigated", lookup_expr="gt")
mitigated_on = DateTimeFilter(field_name="mitigated", lookup_expr="exact")
Copy link
Contributor

Choose a reason for hiding this comment

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

It seems very unlikely to me that a user will ever actually want to list Findings mitigated on an exact timestamp down to the microsecond, even if this would theoretically give them that option. I think searching the range [ <date> 00:00:00, <date+1> 00:00:00 ) is the right approach here too, regardless of whether the user specified a timestamp.

Copy link
Contributor

@cneill cneill left a comment

Choose a reason for hiding this comment

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

Looks good! 😄

@mtesauro mtesauro merged commit c829868 into bugfix Jan 16, 2025
75 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants