Add documents #353
Add documents #353
Conversation
|
Interesting idea! I'm curious what others think: is this the right approach? Do we want to store documents in the database? Does something like this belong in the wiki, or is this ok? |
|
I'm ok with the database, as a first simple step forward. |
| @@ -75,6 +75,15 @@ | |||
|
|
|||
| add_index "deliveries", ["supplier_id"], name: "index_deliveries_on_supplier_id", using: :btree | |||
|
|
|||
| create_table "documents", force: true do |t| | |||
| t.string "name" | |||
| t.binary "content", limit: 16.megabyte | |||
There was a problem hiding this comment.
I use the filename extension in this case.
IMHO storing the "unsafe" content-type provided by the browser is not much better than using the extension of the file.
|
It seems to me that some attachment code is bound to be duplicated in invoice attachments and here. Mime-type-checking, removing attachments, perhaps file-size error handling. I think we'd need to either our own concerns, or use some existing gem. Perhaps this attachment_fu is useful, it's one that is still being maintained (and seems to support full db storage). |
|
Ok, testing it now and doing some small changes. I've done some changes and rebased on master (whoops - can't do a pull request anymore to you) - it's on https://github.com/wvengen/foodsoft/tree/feature/documents . You may consider starting from there. One thing that I found very unintuitive is that mime-type is derived from the name. I entered a description there without extension, which meant the mimetype couldn't be guessed. So I think it works better when the mimetype is derived either from what the browser sends, or from the original filename. I guess we'd better save it in the db. |
|
i'd prefer the browser content type solution. should I do this change? |
|
I'm for the browser content-type solution as well. Would be great, yes. |
|
fyi: for the Travis CI build to succeed, you need to bundle install and commit the Gemfile.lock changes. |
|
What has to be done to complete this issue? |
|
Thanks for asking, I see the following issues:
For the rest, I'd say - all set. |
wvengen
force-pushed
the
master
branch
2 times, most recently
from
8100b26 to
d6bf09b
Compare
|
ping |
| if params[:document][:name] == '' | ||
| name = params[:document][:data].original_filename | ||
| name = File.basename(name) | ||
| @document.name = name.gsub(/[^\w\.\-]/, '_') |
There was a problem hiding this comment.
Great you added a check. Still it's easy to bypass when supplying a name parameter directly.
What about doing this substitution on serving the file? That way we can even change what characters to allow afterwards.
There was a problem hiding this comment.
i do not want to store the original_filename in the DB, so storing a more or less valid name (if not provided by the user already) seams more reasonable to me. This is not related to security. It's just for providing nice filenames.
There was a problem hiding this comment.
Ah, I see.
I'd really like to follow OWASP and filter filename characters to known safe ones.
There was a problem hiding this comment.
blacklisting as bad practice and whitelisting all unicode characters gets ugly very quickly. What's the problem with storing mad filenames? The only place where it could be problematic is when the browser reads the filename from the HTTP header and when the user uses a browser which is not able to to the without security impact (s)he might have even bigger problems than that!
|
Great! Disabled it by default, updated the plugin's README and merged. |
No description provided.