-
Notifications
You must be signed in to change notification settings - Fork 8
feat: Transitive dependencies for javascript #171
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
base: main
Are you sure you want to change the base?
Conversation
|
/test-heavy |
tmihalac
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its looks good, I added a few small comments
| content = function.page_content | ||
|
|
||
| if not self.is_function(function): | ||
| raise ValueError('Only function document is supported') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we want to raise an error here, or write to logs and continue?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
actually it is not an expected behavior,
if not function doc was sent we need to debug and understand why
if it will only write to log we might miss the error
src/vuln_analysis/utils/functions_parsers/javascript_functions_parser.py
Outdated
Show resolved
Hide resolved
src/vuln_analysis/utils/functions_parsers/javascript_functions_parser.py
Outdated
Show resolved
Hide resolved
src/vuln_analysis/utils/functions_parsers/javascript_functions_parser.py
Outdated
Show resolved
Hide resolved
src/vuln_analysis/utils/functions_parsers/javascript_functions_parser.py
Outdated
Show resolved
Hide resolved
src/exploit_iq_commons/utils/functions_parsers/javascript_functions_parser.py
Show resolved
Hide resolved
src/vuln_analysis/utils/functions_parsers/javascript_functions_parser.py
Outdated
Show resolved
Hide resolved
src/vuln_analysis/utils/functions_parsers/javascript_functions_parser.py
Outdated
Show resolved
Hide resolved
src/vuln_analysis/utils/functions_parsers/javascript_functions_parser.py
Outdated
Show resolved
Hide resolved
src/vuln_analysis/utils/functions_parsers/javascript_functions_parser.py
Outdated
Show resolved
Hide resolved
ca11846 to
6bb533c
Compare
|
LGTM |
RedTanny
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
|
/tests |
|
Caution There are some errors in your PipelineRun template.
|
|
/test vulnerability-analysis-on-pr |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @TamarW0
Thanks for you efforts, Great job in overall!!
Please see my comments, i have several questions and comments.
In addition, Three missing essential things:
-
You need to install NodeJS and npm ( usually npm shipped together with NodeJS) in the Dockerfile => File https://github.com/RHEcosystemAppEng/vulnerability-analysis/blob/59f0f48233972998bd62b6f0aeeeb50790ff8a50/Dockerfile
Better to install the versions that you used or NodeJS LTS. -
What about the file extensions ( inclusion and exclusions needed for a NodeJS project analysis?, you need to update that as well ( better also to update it UI Client Application), Let me know what they're going to be , So I Will also create the javascript template in Integration tests accordingly .
vulnerability-analysis/kustomize/base/includes.json
Lines 16 to 29 in 59f0f48
"JavaScript": [ "**/*.js", "**/*.jsx", "webpack.config.js", "rollup.config.js", "babel.config.js", ".babelrc", ".eslintrc.js", ".eslintrc.json", "tsconfig.json", "*.config.js", "*.config.json", "public/**/*", "src/**/*"
vulnerability-analysis/kustomize/base/excludes.json
Lines 29 to 39 in 59f0f48
"JavaScript": [ "node_modules/**/*", "dist/**/*", "build/**/*", "test/**/*", "tests/**/*", "example/**/*", "examples/**/*", "package.json", "package-lock.json", "yarn.lock" -
Kidnly rebase your work on top of main ( to fetch in the integration tests I've added to the CI of the repository) , and add your Javascript IT test case to here: https://github.com/RHEcosystemAppEng/vulnerability-analysis/blob/main/ci/it/integration-tests-input.json#L4-L93
| def supported_files_extensions(self) -> list[str]: | ||
| return ['.js', '.mjs', '.cjs'] |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TamarW0 Did you decide to support html files only in a later/future iteration of this enhancement?
I think in that same opportunity, It's good also to handle jsx files of react as well. ( and this will extend the support also for frontend/web applications as well ( not only NodeJS).
Please create a new ticket for this future work.
src/exploit_iq_commons/utils/functions_parsers/javascript_functions_parser.py
Show resolved
Hide resolved
src/vuln_analysis/utils/functions_parsers/javascript_functions_parser.py
Outdated
Show resolved
Hide resolved
|
|
||
| return False | ||
|
|
||
| def _is_subclass_of(self, child_class: str, parent_class: str, code_documents: dict[str, Document]) -> bool: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TamarW0 Are you fully supports inheritance of classes ? I haven't checked deeply yet.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I added the support in transitive and prototype inheritance.
I didn't support variables inheritance, as we can not know the type of the variable anyway so I think it doesn't give us more info
is it make more sense now?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TamarW0 OK that's good. ( prototype inheritance should get methods of super classes as well).
I Think that for typescript in the future, this would be a a very good addition... ( or maybe also for javascript that it will use d.ts files of libs of nodejs for types)
Please create a jira ticket for it ( once Jira tracker is up and running again) for future tracking , to be handled in a future iteration, 10x!.
src/vuln_analysis/utils/functions_parsers/javascript_functions_parser.py
Show resolved
Hide resolved
49a8521 to
ed1aee8
Compare
6b466e8 to
daefbcc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@TamarW0 Great work!
Just go over all comments, and open tickets for opened items (planned for future iterations) before merging.
Thanks!.
Also try to add another test for IT testing, as discussed.
LGTM Approved.
No description provided.