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

Add curl advisories importer #1439

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

ambuj-1211
Copy link

This pr resolves the issue #1166.
The advisories were added properly and the site was working fine. I also registered the importer in the init.py file.
This image shows the curl advisories in vulnerablecode

@ambuj-1211
Copy link
Author

@TG1999 please let me know if there are any further changes and everything is correct or not.

Copy link
Collaborator

@ziadhany ziadhany left a comment

Choose a reason for hiding this comment

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

@ambuj-1211 This is a good start but try to make use of all available data ex: details, CWE , ..

vulnerabilities/tests/test_curl.py Outdated Show resolved Hide resolved
vulnerabilities/importers/curl.py Outdated Show resolved Hide resolved
@ambuj-1211
Copy link
Author

on it

@ambuj-1211
Copy link
Author

@ziadhany @TG1999 Done with the changes as mentioned.

Copy link
Collaborator

@ziadhany ziadhany left a comment

Choose a reason for hiding this comment

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

@ambuj-1211 Nice work! just a few nits for your consideration.

vulnerabilities/tests/test_curl.py Outdated Show resolved Hide resolved
vulnerabilities/importers/curl.py Outdated Show resolved Hide resolved
vulnerabilities/importers/curl.py Outdated Show resolved Hide resolved
vulnerabilities/importers/curl.py Outdated Show resolved Hide resolved
vulnerabilities/importers/curl.py Show resolved Hide resolved
vulnerabilities/importers/curl.py Outdated Show resolved Hide resolved
vulnerabilities/importers/curl.py Outdated Show resolved Hide resolved
vulnerabilities/importers/curl.py Outdated Show resolved Hide resolved
@ziadhany
Copy link
Collaborator

ziadhany commented Apr 9, 2024

@ambuj-1211 please add a curl improver to the valid_versions.py

ex: https://github.com/nexB/vulnerablecode/blob/main/vulnerabilities/improvers/valid_versions.py#L472

@ambuj-1211
Copy link
Author

ambuj-1211 commented Apr 10, 2024

work! just a few nits for your consi

I think you tagged someone else in this comment, is it a mistake?
And I am working on the changes now.

@ziadhany
Copy link
Collaborator

work! just a few nits for your consi

I think you tagged someone else in this comment, is it a mistake?
And I am working on the changes now.

sorry it was a mistake and I fixed it

@ambuj-1211
Copy link
Author

@ziadhany please review the changes and do let me know.

Copy link
Collaborator

@ziadhany ziadhany left a comment

Choose a reason for hiding this comment

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

Great work! please fix those failing tests then squash the commits

vulnerabilities/importers/curl.py Outdated Show resolved Hide resolved
@ambuj-1211
Copy link
Author

@ziadhany I made the changes, should I also update my branch?

@ziadhany
Copy link
Collaborator

ziadhany commented Apr 13, 2024

@ambuj-1211 Absolutely! Once all the changes are in place, squashing them into a single commit before merging will create a cleaner history for everyone.

to fix the test update the vulnerabilities/tests/test_data/curl/expected-curl-advisory-output.json
and it would be good if you add more test examples :
vulnerabilities/tests/test_data/curl/expected-curl-advisory-output1.json
vulnerabilities/tests/test_data/curl/expected-curl-advisory-output2.json

@ambuj-1211 ambuj-1211 force-pushed the add-curl-advisories-importer branch from 2f84bec to 5ae6ac0 Compare May 7, 2024 18:26
@ambuj-1211
Copy link
Author

@ziadhany @TG1999 i have made all the required changes and squashed all the commits to make the history clean, moreover I also had updated my branch according to the main branch.
Please review it.

@ambuj-1211
Copy link
Author

@ziadhany some checks are not successfull but they are because of openssl url do I need to change it to some thing, I am asking because it is not related to curl advisories.

@ziadhany
Copy link
Collaborator

ziadhany commented May 9, 2024

@ziadhany some checks are not successfull but they are because of openssl url do I need to change it to some thing, I am asking because it is not related to curl advisories.

@ambuj-1211 No, this is the docs test ( not related to this pull request )

@ambuj-1211
Copy link
Author

so this pr is good to go?

Copy link
Collaborator

@ziadhany ziadhany left a comment

Choose a reason for hiding this comment

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

LGTM, just a minor nit for your consideration

@ambuj-1211 ambuj-1211 force-pushed the add-curl-advisories-importer branch 2 times, most recently from f2729d8 to c9ebb70 Compare May 10, 2024 13:12
- added curl importer
- added tests for curl importer
Signed-off-by: ambuj <kulshreshthaak.12@gmail.com>
@ambuj-1211 ambuj-1211 force-pushed the add-curl-advisories-importer branch from c9ebb70 to 24b5eaa Compare May 10, 2024 13:19
@ambuj-1211
Copy link
Author

@ziadhany check the files now I have changed the files and squashed the commits, I think now the history and everything is clean and good to go, please let me know if there are any more nits.

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.

None yet

2 participants