We ran into an issue where the embedly module is creating file objects, and thus validating them without actually having a local file to scan. I know it's really not Antivirus's job to account for every weird stream wrapper, but I'm wondering if we should consider bypassing the scan if there is no local file, or it has zero length.

Any thoughts?

CommentFileSizeAuthor
#14 antivirus-scans_fail_oembed-1872212-14.patch3.24 KBkevin hankens
#10 antivirus-scans_fail_oembed-1872212-9.patch1.37 KBkevin hankens
#7 antivirus-scans_fail_oembed-1872212-7.patch870 byteserikwebb
#4 antivirus-scans_fail_oembed-1872212-4.patch979 byteserikwebb
#1 empty_file_skip_1872212_1.patch2.29 KBkevin hankens

Comments

kevin hankens’s picture

kevin hankens commented
Status: Active » Needs review
StatusFileSize
new empty_file_skip_1872212_1.patch2.29 KB

I was thinking something like this.

erikwebb’s picture

erikwebb commented

Should we be checking based on file size or maybe just created a list of "supported" stream wrappers (e.g. public:// and private://)?

kevin hankens’s picture

kevin hankens commented

Yeah, I'm not sure. We could possibly use hook_stream_wrappers() to generate a list of checkboxes that are configurable as "scannable streams." And have public and private be the default values.

erikwebb’s picture

erikwebb commented
StatusFileSize
new antivirus-scans_fail_oembed-1872212-4.patch979 bytes

How about something like this? I haven't been able to test it unfortunately, but this is what I was thinking.

Status: Needs review » Needs work

The last submitted patch, antivirus-scans_fail_oembed-1872212-4.patch, failed testing.

erikwebb’s picture

erikwebb commented
Status: Needs work » Needs review

We should also write a test to make sure Antivirus isn't failing on empty or remote files.

erikwebb’s picture

erikwebb commented
StatusFileSize
new antivirus-scans_fail_oembed-1872212-7.patch870 bytes
kevin hankens’s picture

kevin hankens commented

Yeah, I think that's the right idea, though it's throwing a fatal for me. I think it's because file_stream_wrapper_get_instance_by_uri() is returning an object. I don't have time to work on it tonight, but I'll work on it in the morning if need be.

Thx!

erikwebb’s picture

erikwebb commented
Status: Needs review » Needs work
kevin hankens’s picture

kevin hankens commented
Status: Needs work » Needs review
StatusFileSize
new antivirus-scans_fail_oembed-1872212-9.patch1.37 KB

It looks like there's inconsistent handling of the $file->uri, in some cases (media 1.x) the uri is /tmp/xxxxx without a stream wrapper. This would allow it to bypass the scanner.

I reworked the patch in #7 a little bit. Let me know if this looks sane :)

erikwebb’s picture

erikwebb commented

Looks good to me. Were you able to test with an oembed:// stream?

kevin hankens’s picture

kevin hankens commented

Yeah, it works well with oembed now. Should I try and write a test for this too?

erikwebb’s picture

erikwebb commented
Status: Needs review » Needs work

If you don't mind, that'd be great. I'm sure this is a relatively common case that we need to account for.

kevin hankens’s picture

kevin hankens commented
StatusFileSize
new antivirus-scans_fail_oembed-1872212-14.patch3.24 KB

Ok, give this a try. Testing for the absence of a remote file in the scanner targets is a little weird, but I think it's a legit test.

kevin hankens’s picture

kevin hankens commented
Status: Needs work » Needs review
erikwebb’s picture

erikwebb commented
Category: feature » bug
Status: Needs review » Fixed

Thanks!

http://drupalcode.org/project/antivirus.git/commit/8263f31

Status: Fixed » Closed (fixed)

Automatically closed -- issue fixed for 2 weeks with no activity.