search

kylefarris / clamscan

A robust ClamAV virus scanning library supporting scanning files, directories, and streams with local sockets, local/remote TCP, and local clamscan/clamdscan binaries (with failover).
MIT License
236 stars 69 forks source link

Verify binary with only custom config file #67

Closed chris-maclean closed 3 years ago

chris-maclean commented 3 years ago

Pursuant to issue #66

I updated _is_clamav_binary to support CLI options. The --config-file= option is added if this.settings[scanner].config_file exists. While at this point only the clamdscan settings object supports a config_file property, this change would theoretically support a config_file property on settings.clamscan too.

The new test seeks to check that NodeClam.init will run without error when all of the following are true:

The third point may be a burden for testers to replicate in their environment, but it's an important part of issue #66 that the code changes here address. If successful, the _is_clamav_binary function will verify clamdscan by specifying the custom config file, and will not fall back to a different binary.

I'm open to changing how the test works, but I was unable to find a better way to answer the question "Did init go through any of the binary fallback options (lines ~192-205)?".

chris-maclean commented 3 years ago

I see this PR has failed CI testing with the "should handle a 0-byte file" test (Build 229). This test always passed during development, is there anything I should know about how to make this test pass? It even looks like it passed on Node 10.0 and 12.0, just not 11.0.

kylefarris commented 3 years ago

Thanks for the PR. Travis-CI seems to be having a fit with it so I'll pull the branch onto my machine for testing this evening and merge if all is good. I'll see if there's some testing solution I can come up with that solves the default config file conundrum.

The code looks high-quality and straightforward enough after a preliminary review, though, so, thanks again!

chris-maclean commented 3 years ago

Any thoughts on this PR?

chris-maclean commented 3 years ago

Can this PR be merged?

chris-maclean commented 3 years ago

Any thoughts on this PR?

kylefarris commented 3 years ago

Sorry @chris-maclean, I guess I got wrapped up in work and forgot about this PR. I'll see if I can look at it again today.

kylefarris commented 3 years ago

Just an update... I ran the test suite on my MacBook Pro and it literally crash it. Everything locked up and it rebooted itself 😅. Not sure what's going on there... it's a beast of a machine but it sounded like it was going to lift off to the moon. Gonna update ClamAV on there and try again. Hopefully we don't have a repeat scenario.

kylefarris commented 3 years ago

@chris-maclean Would you mind re-basing your fork to master? Some things have changed and I can't easily test it without you re-basing.

kylefarris commented 3 years ago

You could also do this and then I can do what's needed to get this merged: https://github.blog/2016-09-07-improving-collaboration-with-forks/

kylefarris commented 3 years ago

Actually, I can just merge this into a different branch on this repo and fix what I need from there. Sorry for the confusion.