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

Validate IP addresses in alert profiles #2649

Conversation

johannaengland
Copy link
Contributor

Closes #1876

@johannaengland johannaengland requested a review from hmpf August 4, 2023 11:34
@johannaengland johannaengland self-assigned this Aug 4, 2023
@github-actions
Copy link

github-actions bot commented Aug 4, 2023

Test results

     10 files       10 suites   7m 33s ⏱️
3 242 tests 3 145 ✔️   96 💤 1
6 968 runs  6 774 ✔️ 192 💤 2

For more details on these failures, see this check.

Results for commit ba54047.

♻️ This comment has been updated with latest results.

@johannaengland johannaengland force-pushed the bug/ip-validation-alert-profiles branch from 846ee92 to 3b9f306 Compare August 4, 2023 14:05
@codecov
Copy link

codecov bot commented Aug 4, 2023

Codecov Report

Merging #2649 (20aec08) into 5.6.x (2831b68) will increase coverage by 0.11%.
The diff coverage is 85.36%.

❗ Current head 20aec08 differs from pull request most recent head ba54047. Consider uploading reports for the commit ba54047 to get more accurate results

@@            Coverage Diff             @@
##            5.6.x    #2649      +/-   ##
==========================================
+ Coverage   54.13%   54.24%   +0.11%     
==========================================
  Files         558      558              
  Lines       40641    40667      +26     
==========================================
+ Hits        22000    22059      +59     
+ Misses      18641    18608      -33     
Files Changed Coverage Δ
python/nav/web/alertprofiles/forms.py 60.52% <85.18%> (+4.74%) ⬆️
python/nav/web/alertprofiles/views.py 42.82% <85.71%> (+2.66%) ⬆️

... and 2 files with indirect coverage changes

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

python/nav/web/alertprofiles/views.py Outdated Show resolved Hide resolved
@johannaengland johannaengland requested a review from hmpf August 7, 2023 13:33
@johannaengland
Copy link
Contributor Author

Validation of IP addresses now works, both for a single one as well as multiple.

But while adding form validation for that I broke being able to select multiple options of a given list (e.g. AlertType). Because now when trying to access that list of chosen alert types in clean I only get the first element of the list as value.

@hmpf
Copy link
Contributor

hmpf commented Aug 21, 2023

But while adding form validation for that I broke being able to select multiple options of a given list (e.g. AlertType). Because now when trying to access that list of chosen alert types in clean I only get the first element of the list as value.

More context please so I can reproduce. Is this caught by a test? What do you expect to be in filter, match_field and value? The usual gotcha with request.(GET|POST) is that every value is a list and you get the entire list with get_list(), but the first element of the list with get, and forms know when to do what.

@johannaengland
Copy link
Contributor Author

It is not caught by a test yet.

To reproduce follow the following steps:

  1. Choose any filter
  2. Add an expression to it with the type AlertType
  3. Choose the in operator and select multiple values
  4. Save and see that instead of saving these multiple values we have the first choice split into letters

@johannaengland
Copy link
Contributor Author

I have now added a test that reproduces the error. Hope that helps

@sonarcloud
Copy link

sonarcloud bot commented Aug 28, 2023

Kudos, SonarCloud Quality Gate passed!    Quality Gate passed

Bug A 0 Bugs
Vulnerability A 0 Vulnerabilities
Security Hotspot A 0 Security Hotspots
Code Smell A 0 Code Smells

No Coverage information No Coverage information
No Duplication information No Duplication information

Copy link
Member

@lunkwill42 lunkwill42 left a comment

Choose a reason for hiding this comment

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

I'm glad you're doing this cleanup :)

validated_ip_addresses = []
for ip in ip_list:
try:
IP(ip)
Copy link
Member

Choose a reason for hiding this comment

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

Actually, I would advise against using IPy.IP for validation of strings, precisely because this library is extremely lenient in what is considered acceptable as input.

Case in point:

>>> from IPy import IP
>>> IP(42)
IP('0.0.0.42')
>>> IP('42')
IP('42.0.0.0')
>>> IP(23948234)
IP('1.109.107.202')
>> IP('100.200')
IP('100.200.0.0')
>>>

Please consider using nav.utils.is_valid_ip() instead (possibly with the strict flag set).

# If input was a IP adress we should replace space with | (pipe).
# FIXME We might want some data checks here
if match_field.data_type == MatchField.IP:
# FIXME We might want to check that it is a valid IP adress.
Copy link
Member

Choose a reason for hiding this comment

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

Haha, I love the fact that there was a FIXME comment here from before 😆

@johannaengland
Copy link
Contributor Author

Replaced by #2667

@johannaengland johannaengland deleted the bug/ip-validation-alert-profiles branch September 12, 2023 11:09
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.

Alert Profiles does not validate IP addresses in filter expressions
3 participants