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 apache_httpd improver #1102

Merged
merged 2 commits into from
Jan 27, 2023
Merged

Conversation

TG1999
Copy link
Member

@TG1999 TG1999 commented Jan 26, 2023

Signed-off-by: Tushar Goel tushar.goel.dav@gmail.com

Copy link
Member

@AyanSinhaMahapatra AyanSinhaMahapatra left a comment

Choose a reason for hiding this comment

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

Thanks @TG1999 ! Looks good to me, just posted some questions/comments on code style/structure which is not required for merge btw.

{
"system": "apache_httpd",
"value": "moderate",
"scoring_elements": ""
Copy link
Member

Choose a reason for hiding this comment

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

Shouldn't we have a null/none instead of a empty string when there is no value? This wouldn't matter because if "": would produce the same as if None: but it's good practice AFAIK :P

@@ -209,3 +227,80 @@ def fetch_links(url):
"post_ajp_proxy",
"pre_ajp_proxy",
}


class ApacheHTTPDImprover(Improver):
Copy link
Member

Choose a reason for hiding this comment

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

Why not have these classes as seperate files in the improvers directory? I only see the oval improver has it's own file, is that only because of it's size? We would probably want to be consistent here and have the improvers in their respective directory unless that's inconvenient/not possible because of something.

@@ -147,7 +165,7 @@ def fetch_links(url):
return links
Copy link
Member

Choose a reason for hiding this comment

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

See line 87 above in the importer class, there is a lot of dict/key usage, often several levels like:
for vendor in data["affects"]["vendor"]["vendor_data"] and then again for products in vendor["product"]["product_data"].

Shouldn't we probably use something like the AdvisoryData classes (or a special subclass for that) which would model the advisory data directly, and then use those classes everywhere instead of dicts. This has some advantages:

  1. code would be more pythonic and readable (and better looking)
  2. we can and should test for the advisory format and handle keyerrors gracefully (idk if the data format changes that much at all, but it could in future?)

Copy link
Member

Choose a reason for hiding this comment

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

Also see https://github.com/nexB/vulnerablecode/pull/1102/files#diff-aa2cf137c50d3fb0242d00e7fbe786cc8e3d8c3d22c32b8afedde439965f2fb9R49, any reason we are using SPDX version of the license expression with spdx_license_expression instead of the scancode license expression version?

Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
Signed-off-by: Tushar Goel <tushar.goel.dav@gmail.com>
@TG1999 TG1999 merged commit 13427e1 into nexB:main Jan 27, 2023
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

3 participants