brakeman report - Direct Model attribute used as the filename

Question

The brakeman showing the following error, The files are managed with paperclip.

In my controller:

asset_file ||= AssetFile.find(params[:id])

if asset_file
// downloading file
send_file asset_file.uploaded_file.path, :type => asset_file.uploaded_file_content_type
else
flash[:error] = t('document.mind_your_asset_file')
redirect_to root_url
end

Answer 1

Please keep in mind Brakeman does not report errors[0], it reports warnings. It generates warnings about potential security issues in the application. In other words, it will warn you about things which you, as a human, will judge not to be real problems. It is essentially impossible for a pure static analysis security tool to never report false positives.

You didn't actually ask a question, so I will assume you either wish to know why this warning was reported and/or how to fix it. If neither of these are your question, please clarify.

Brakeman knows AssetFile is a model (most likely because it is defined in the app/models directory). It knows that send_file allows access to the file system. When it sees send_file AssetFile.find(params[:id]).uploaded_file.path it interprets this to mean a model attribute (likely a value from the database), which may be user-controllable. Therefore it generates a File Access warning letting you know the code may allow an attacker to access arbitrary files on the server.

I suppose the next question is why Brakeman reports this when you are using paperclip and so this is probably safe. Well, because Brakeman doesn't know anything about paperclip. This has come up a number of times, however, so I will look into whether or not this is safe and see about whitelisting this usage.

For the second question of what to do about it - well you don't have to do anything. While zero Brakeman warnings is a noble goal, there will always be false positives. For this specific warning, there is nothing you can do to make this code appear any safer to Brakeman without changing Brakeman itself.

If the intent of this post was actually to report a false positive, it would be better to open an issue for Brakeman.

[0] I guess technically it reports its own errors in the "exceptions" table which can be seen in your screenshot. It would probably be helpful to report those to the Brakeman project to see if they can be fixed.