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

unify response headers from different scan methods & new ScanFile endpoint #51

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

lukey78
Copy link

@lukey78 lukey78 commented Jul 13, 2024

This package already provides different scanning endpoints, including one to scan files on a (mounted) filesystem (scanPath).

This pull request tries to unify the response headers from the different scan methods and provides an additional /scanFile endpoint that uses the simplest scanning method of clamd (SCANFILE instead of ALLMATCHSCANFILE).

I also updated the documentation.

Generally, there are limitations with the current approach, as the headers and the JSON response is written multiple times if several files were scanned (e.g. multipart binary or when given a path instead of a file to the scanPath/scanFile endpoints). But changing that would of course hurt backwards compatibility.

allow scanning with the SCANFILE method of clamd
fmt.Printf(time.Now().Format(time.RFC3339)+" Scan result for: %v, %v\n", part.FileName(), s)
}

handleScanResults(response, part.FileName(), w)
Copy link
Collaborator

Choose a reason for hiding this comment

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

This will falsely return a 200 OK if a FormPost with multiple files are used to call the /scan endpoint and the first file is clean, but the second one contains a virus, once you w.WriteHeader(http.Status), the response status will not change if you write it again. Maybe you are aware of this, considering your comment about limitations. This PR fixes that bug

Copy link
Author

Choose a reason for hiding this comment

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

You're right - this is of course a security hole. Hope the maintainer merges your PR... then we could add more endpoints and get rid of some code duplications.

Copy link
Collaborator

@arizon-dread arizon-dread Jul 23, 2024

Choose a reason for hiding this comment

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

Yes, it's good that you're also interested in adding functionality and patching holes, it gives some potential for the future of the project. I'm also interested in enhancement and bug fixes.

Copy link
Contributor

Choose a reason for hiding this comment

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

Gentlemen, thanks a lot for your contributions. I did not react until now simply because I did not notice your open PRs. Sorry for that. Will try to set Github alerts for PRs going forward.

As a next step, can you update and test your changes against the current code base (fixed the database update)?

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.

3 participants