#1 Query cve-search' database for package vulnerabilities #2

kartiksibal · 2017-06-09T11:15:19Z

Signed-off-by: Kartik Sibal kartiksibal@gmail.com

pombredanne

Here is some quick feedback

pombredanne · 2017-06-09T11:21:34Z

test_api_data.py

+	api.urlopen.return_value = test_data
+	api.output_cve_id()
+
+	assert api.output_cve_id.data["data"]["item"] == "CVE-2007-1004"


You want a line return an the end of every files.

pombredanne · 2017-06-09T11:22:00Z

test_api_data.py

+
+def test_output_cve_id():
+
+	##BUG##


What is this comment about? and why an empty line before?

pombredanne · 2017-06-09T11:23:41Z

api_data.py

+	vulnerabilities of the requested package. If 
+	vulnerability exists, outputs cve-id(s).
+	"""
+	package_name = raw_input('Enter package name: ')


You should rather use args instead and not expect user input at all.

Yes, I was developing it into, command line based args and flags.
Will try to get it done in the next commit, by today.

pombredanne · 2017-06-09T11:24:09Z

api_data.py

+	raw_data = urlopen(url).read()
+	data = json.loads(raw_data)
+
+	if len(data) > 0:


you could use instead if data: to the same effect

You are fast. 😛

pombredanne · 2017-06-09T11:24:48Z

api_data.py

+	data = json.loads(raw_data)
+
+	if len(data) > 0:
+		print 'Vulnerabilties Found:\n'


You should rather have a function that return data and a main that accepts command line args and deal with printing.

Yes, as stated in the previous comment. Working on that.

pombredanne · 2017-06-09T11:25:50Z

api_data.py

+from urllib import urlopen 
+
+def output_cve_id(): 
+	"""Takes as input, a package name, package version. 


Use Take rather than Takes also start the docstring on the next line.

pombredanne · 2017-06-09T11:26:24Z

test_api_data.py

@@ -0,0 +1,50 @@
+import api_data as api
+
+from mock import Mock


If you use packages for tests, then they should be at least part of some tests requirements file.

I'll add that to the requirements.txt.

tdruez · 2017-06-09T12:04:15Z

Couple comments on style:

You are using tabs for indentation, you should always use spaces instead (you may have to set the proper config in your editor). See http://pep8.org/#tabs-or-spaces
Install and run pycodestyle https://pypi.python.org/pypi/pycodestyle before each commit, this would allow to catch style issue before submitting the code. We will make this validation part of the test integration :)

kartiksibal · 2017-06-09T14:11:02Z

api_data.py

+		print ('No vulnerabilites found')
+
+if __name__ == '__main__':
+	output_cve_id()


@pombredanne I had added a line after the EOF, I think i deleted it un-consiously. Will be more alert next time

@kartiksibal as I mentioned, run pycodestyle on your code before committing, it will let you know about those type of issues.

@tdruez won't forget it next time 😄

pombredanne

Looking good overall

pombredanne · 2017-06-09T18:53:54Z

api_data.py

+	from urllib import urlopen 
+
+def output_cve_id(type=None, name=None, version=None): 
+	"""Takes as input, a package name, package version. 


I guess my previous commit comment on this still applies? ;)

pombredanne · 2017-06-09T18:56:55Z

api_data.py

-		url = 'https://cve.circl.lu/api/search/' + package_name + package_ver
-
+	if version:
+		url = 'https://cve.circl.lu/api/search/' + name + version


what is there is no name? Also you might want to use the .format() syntax rather than a +

@pombredanne The name is a basic necessity. I was thinking of maybe keeping the whole function in a try-catch block, since if the name is none. We have nothing to search?

pombredanne · 2017-06-09T20:52:08Z

api_data.py

+	vulnerabilities of the requested package. If 
+	vulnerability exists, outputs cve-id(s).
+	"""
+	if version:


What is there name is None?

This code will crash. Write some tests... and you will see.

@pombredanne As mentioned above, i wasn't sure if going with, keeping the function in a try-except block an effective way to counter this?

@kartiksibal nope, you should test the values you get:
Did you get a name and version? --> query for name+version
Did you get only a name? --> query for name
Did you get only a version --> query for version

Eventually you could check if this can be searched more specifically in CPEs?

@pombredanne A query for just version without a package name isn't possible right?
I'll fix this in the next commit.

pombredanne · 2017-06-09T20:53:13Z

api_data.py

+	raw_data = urlopen(url).read()
+	data = json.loads(raw_data)
+
+	if data:


Just return data, do not print it there. Print after calling the function

tdruez · 2017-06-09T23:22:36Z

api_data.py

 	"""
 	if version:
-		url = 'https://cve.circl.lu/api/search/' + name + version
+		url = ('https://cve.circl.lu/api/search/{}/{}').format(name,version)


Here the parenthesis around the string are not needed: '{}'.format()
Since we are using Python 3.6+, feel free to use the format string as in:
f'https://cve.circl.lu/api/search/{name}/{version}'

@tdruez Pro tip!
Thanks 😄

pombredanne · 2017-06-17T13:56:05Z

api_data.py

+from urllib.request import urlopen
+
+def output_cve_id(type=None, name=None, version=None):
+    """Take as input, a package name, package version.


Minor style point: start the doc on the next line as in

""" Take as input, a package name....

Also it is best to start with what you return rather than what you accept

pombredanne · 2017-06-17T13:56:22Z

api_data.py

@@ -0,0 +1,32 @@
+#!/usr/bin/env python


You are missing a license header ;)

pombredanne · 2017-06-17T13:56:49Z

api_data.py

+    if data:
+        for item in data['data']:
+            return item['id']
+    else:


else, return None is not needed, this is the default in Python

pombredanne · 2017-06-17T13:57:12Z

api_data.py

+    vulnerability exists, outputs cve-id(s).
+    """
+    if not name:
+        return None


return is enough, None is the default

pombredanne · 2017-06-17T13:57:53Z

test_api_data.py

+'''
+
+def test_output_cve_id():
+    #BUG: 


What is this comment?

I guess you did not remove this?

@pombredanne I haven't been able to find a workaround. it'd be great if you took a look.

@kartiksibal the think I am not comfy with is the #BUG comment which is meaningless... either you explain in a full narrative comment what the bug is or you remove the #BUG line ;)

tdruez · 2017-06-19T15:36:32Z

@kartiksibal please fix the remaining issues reported by @pombredanne so we can merge this PR.
Also, make sure to run pycodestyle and fix all styling issue as well.

tdruez · 2017-06-19T15:51:42Z

api_data.py

+from urllib.request import urlopen
+
+def output_cve_id(type=None, name=None, version=None):
+    """Take as input, a package name, package version.


type is never used, it should be removed.
Since you always need a name, no need to make it optional, remove the =None.

I had a talk with @pombredanne regarding this.
This is the way he suggested it to be.
We can discuss this further? :-)

tdruez · 2017-06-19T15:53:25Z

api_data.py

+    vulnerabilities of the requested package. If
+    vulnerability exists, outputs cve-id(s).
+    """
+    id = []


In Python id is a built-in function https://docs.python.org/2/library/functions.html#id, you should avoid using it for your variable names.
Also, since you are naming a list, you should use the plural: ids.

tdruez · 2017-06-19T15:54:28Z

api_data.py

+    raw_data = urlopen(url).read()
+    data = json.loads(raw_data)
+
+    if data and name and not version:


You already tested if not name: above, no need to test it again here.

tdruez · 2017-06-19T15:55:12Z

api_data.py

+
+        return id
+
+if __name__ == '__main__':


You can remove this section, this file is not meant to be run as a script but called as a library.

@tdruez I agree with you. But don't you think the api code has a use case in it's own too?
If someone doesn't want to use the django app and just get data?

Right now, in that state it has absolutely no value. There's a usecase for using vulnerablecode as a script, but we'll get to it later, with a proper cli.

tdruez · 2017-06-19T15:57:01Z

api_data.py

+        return id
+
+    if data and version and name:
+        for item in data:


This section is duplicated with the previous one. The only difference seems to be data['data'] in the first case and data. You should use the if to set the proper content in the variable, then us the same loop.

@tdruez The JSON output changes if a version is added to the URL. Hence, this added part.

As you said, the variable content changes, but the iteration logic stay the same.
Thus, you need to set the proper variable but use the same loop.

tdruez · 2017-06-21T19:36:25Z

api_data.py

+import json
+from urllib.request import urlopen
+
+def data_cve_circl(name, version=None):


Surround top-level function and class definitions with two blank lines.

http://pep8.org/#blank-lines

tdruez · 2017-06-21T19:37:25Z

api_data.py

+    extracted_data = []
+
+    if version:
+        data = data


This line has no value. The following is enough:

if not version: data = data['data']

Also, I think this section of the code belongs to data_cve_circl().
The extraction logic should not care about the version.

@tdruez The data that the API returns, differs, in the case a version is specified. Hence, the version check is necessary.

tdruez · 2017-06-21T19:39:15Z

api_data.py

+        data = data['data']
+
+    for item in data:
+        results = {}


This should be replaced by a dict comprehension:

{key: value for (key, value) in iterable}

@tdruez

results = {} {results[name]: item[name] for name in fields_names for item in data}

It is kind of erratic, but I am getting a KeyError. I am unable to debug the issue, could you guide?

Thanks

Try this instead:

results = {name: item[name] for name in fields_names} extracted_data.append(results)

@tdruez You missed for item in data and oddly, it is storing just a single entry.

I think we should let go off the dict. comprehension because we need a try except block incase of a KeyError which tends to pop up if a key is missing somewhere in the data. This shouldn't happen, since the data obviously follows a set pattern. But, there is something fishy in the backend.

Of course the for item in data is still required to build the list. My point is about the dict.
Anyway, keep it the way it works for now, this is not a required change, just nice to have.

pombredanne · 2017-06-23T12:34:37Z

api_data.py

+    url = f'https://cve.circl.lu/api/search/{name}'
+
+    if version:
+        url += f'/{version}'


This f is Python 3-only, right? I guess this is OK as we said we would only support Python3, right?

Yes, that was an update in 3.6 and yes as you said earlier to support 3.6+. So, this can stay.

pombredanne · 2017-06-23T12:35:30Z

api_data.py

+def extract_fields(data, fields_names, version=False):
+    """
+    Extracts requested data fields using data generated by
+    cve-search' api. Takes as input data, fields requested


you are missing a quote.

pombredanne · 2017-06-23T12:36:24Z

api_data.py

+
+def extract_fields(data, fields_names, version=False):
+    """
+    Extracts requested data fields using data generated by


Please use singular order-style Extract and not Extracts and start your doc with what is returned rather than what it does.

pombredanne · 2017-06-23T12:37:02Z

test_api_data.py

+'''
+
+def test_output_cve_id():
+    #BUG: 


I guess you did not remove this?

pombredanne · 2017-06-23T12:37:17Z

test_api_data.py

@@ -0,0 +1,49 @@
+import api_data as api


You are also missing a license header

Pulling changes

* Add PEP 517/518 pyproject.toml file * Add setuptools_scm to handle versioning * Add setup.py content to setup.cfg * Update setup.py to act as a shim (so pip install -e works) Addresses: #2 Signed-off-by: Steven Esser <sesser@nexb.com>

#1 Query cve-search' db

a973fb7

pombredanne reviewed Jun 9, 2017

View reviewed changes

#1 Use args instead of user-input

1f3bf76

kartiksibal commented Jun 9, 2017

View reviewed changes

pombredanne reviewed Jun 9, 2017

View reviewed changes

#1 Use format

115933b

pombredanne reviewed Jun 9, 2017

View reviewed changes

tdruez reviewed Jun 9, 2017

View reviewed changes

kartiksibal added 2 commits June 16, 2017 09:29

#1 general fixes

4e5d290

#1 Changes in testdata

d2dad49

pombredanne requested changes Jun 17, 2017

View reviewed changes

kartiksibal added a commit that referenced this pull request Jun 18, 2017

#2 Use django app to query cve-search and display cve-ids

dc51a18

#1 Minor fixes

892a28c

tdruez requested changes Jun 19, 2017

View reviewed changes

kartiksibal added 2 commits June 20, 2017 23:07

#2 Return cvss,summary and cve-ids

2c542bd

#2 Remove boilerplate code

50e094b

tdruez requested changes Jun 21, 2017

View reviewed changes

#2 Remove dict comprehension

fe1441d

pombredanne reviewed Jun 23, 2017

View reviewed changes

pombredanne requested changes Jun 23, 2017

View reviewed changes

kartiksibal added 2 commits June 27, 2017 22:07

#2 Use dict comprehension & added test cases

240ec00

#2 Added test cases

2ad3a9c

kartiksibal force-pushed the cve_search_api branch from c4b7974 to 7560ac5 Compare June 27, 2017 18:15

#2 Fixes unit tests

09413f9

kartiksibal force-pushed the cve_search_api branch from 7560ac5 to 09413f9 Compare June 27, 2017 18:16

tdruez merged commit 57c8c93 into develop Jun 27, 2017

tdruez deleted the cve_search_api branch June 27, 2017 18:19

Hritik14 pushed a commit that referenced this pull request Feb 8, 2022

Merge pull request #2 from nexB/main

c585f02

Pulling changes

Hritik14 mentioned this pull request Feb 14, 2022

Basic documentation #603

Closed

Hritik14 mentioned this pull request Mar 1, 2022

Better Documentation #638

Open

5 tasks

Uh oh!

#1 Query cve-search' database for package vulnerabilities #2

#1 Query cve-search' database for package vulnerabilities #2

Uh oh!

Conversation

kartiksibal commented Jun 9, 2017

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kartiksibal Jun 9, 2017 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

tdruez commented Jun 9, 2017

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pombredanne left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

kartiksibal Jun 9, 2017 •

edited

Loading