search

OWASP / glue

Application Security Automation
Other
522 stars 112 forks source link

Fix retirejs task #143

Closed fmscorreia closed 5 years ago

fmscorreia commented 5 years ago

This PR fixes the broken retirejs task, which fails and ultimately does not print the base report.

Problem description

When running the retirejs task from owasp/glue:latest Docker image, it prints an empty base report, while a standalone retirejs v2.0.1 installation reports several vulnerabilities for the same project. Running the retirejs task from glue git master branch fails with the following error:

Problem running RetireJS
#<NoMethodError: undefined method `key?' for ["version", "2.0.1"]:Array>
/Users/filipe.correia/repos/fmscorreia/glue/lib/glue/tasks/retirejs.rb:91:in `block in parse_retire_results'

Solution

In a0fc117, the issue from the problem description was erroneously "fixed". The error at retirejs.rb:91 is not a code bug, but a consequence of the newer retirejs v2.0.1 having a different report output format from retirejs v1.6.2, the version included in the owasp/glue:latest Docker image build. It now encapsulates results in a structure with additional metadata, causing the error.

The erroneous fix from a0fc117 works for retirejs v2.0.1, in the sense that it now prints a correct report, but it breaks the Travis build since the tests aren't written for the newer report format.

An actual bug was found at retirejs.rb:191, where a fetch is performed without first checking for the existence of the key, leading to failure if the retrieved value is nil:

Problem running RetireJS
#<NoMethodError: undefined method `each_with_object' for nil:NilClass>
/Users/filipe.correia/repos/fmscorreia/glue/lib/glue/tasks/retirejs.rb:191:in `vulnerability_hashes'

Additional notes

A future Docker build of owasp/glue:latest will result in a broken retirejs task unless retirejs is explicitly locked to an older version, or the task and respective tests are updated to support the new report output format.

omerlh commented 5 years ago

Maybe consider removing the code and add support as a dynamic task?

fmscorreia commented 5 years ago

Thank you for the quick reply, @omerlh !

After downgrading to retirejs v1.6.2, I have tested with 3 repositories, 2 of which with over 2k dependencies. The glue task reports the same vulnerabilities as the standalone retirejs.

Regarding the possibility of adding a dynamic task, I'd say it would be simpler and quicker to adapt a jq script to the output schemas of different retirejs versions.

omerlh commented 5 years ago

That what I did with Zap and it was really cool, a lot faster than writing code. Also, allow me to change the format according to Zap changes, without changing code. Does there any missing tests? Maybe worth adding them?

fmscorreia commented 5 years ago

You mean tests for dynamic tasks? I've not looked into those yet, but indeed if a dynamic task is added for retirejs there should be tests to replace the rspec ones. However, I'd address the dynamic task in another PR.

omerlh commented 5 years ago

No, sorry for the confusion. I asked if the existing tests for the existing tests should be modified - as they passed before the bug fix

fmscorreia commented 5 years ago

Ah, yes! I'll add a test to this PR then, verifying the case that was causing the problem: when the proto_result fetch returns nil

fmscorreia commented 5 years ago

On my latest commit:

What lead me to finding the issue was a project that included the jquery-ui.js library, for which retirejs yields a report listing some components that have no vulnerabilities:

{"version":"1.9.0","component":"jquery-ui-autocomplete","detection":"filecontent"},
{"version":"1.9.0","component":"jquery-ui-tooltip","detection":"filecontent"}

This caused the aforementioned error in retirejs.rb:vulnerability_hashes, because these two results have no vulnerabilities key. In conclusion, I added a test with a report on jquery-ui.js, recreating the specific conditions that caused the error.

fmscorreia commented 5 years ago

The changes I've pushed so far work for retirejs v1.6.2. If we upgrade to a newer version, additional changes will be necessary to adapt to the new report schema, and report files for the rspec tests must also be generated with the newer version of retirejs, otherwise a lot of tests will fail.

omerlh commented 5 years ago

Ok, merging. Thanks!

fmscorreia commented 5 years ago

Thank you :)