Newlines in attachment filename causes crash
|Assignee:||Jean-Philippe Lang||% Done:|
The routes for attachments require the filenames to conform to
/.*/ (see source:/branches/2.3-stable/config/routes.rb#L270). Unfortunately, this RegEx doesn't match newlines, which can occur in filenames of attachments. This causes actions with views showing attachments with full paths, for example using source:/branches/2.3-stable/app/helpers/application_helper.rb#L88, to crash.
Saddly, it is not possible to use a multiline RegEx (
/.*/m) to also match newlines in routes constraints, the best way we (Planio) have found to work around this is to use a negative match group with just
/ are not allowed in filenames, and the routes constraint thus allows everything but a
--- a/config/routes.rb +++ b/config/routes.rb @@ -264,8 +264,8 @@ RedmineApp::Application.routes.draw do get 'projects/:id/repository', :to => 'repositories#show', :path => nil # additional routes for having the file name at the end of url - get 'attachments/:id/:filename', :to => 'attachments#show', :id => /\d+/, :filename => /.*/, :as => 'named_attachment' - get 'attachments/download/:id/:filename', :to => 'attachments#download', :id => /\d+/, :filename => /.*/, :as => 'download_named_attachment' + get 'attachments/:id/:filename', :to => 'attachments#show', :id => /\d+/, :filename => /[^\/]*/, :as => 'named_attachment' + get 'attachments/download/:id/:filename', :to => 'attachments#download', :id => /\d+/, :filename => /[^\/]*/, :as => 'download_named_attachment' get 'attachments/download/:id', :to => 'attachments#download', :id => /\d+/ get 'attachments/thumbnail/:id(/:size)', :to => 'attachments#thumbnail', :id => /\d+/, :size => /\d+/, :as => 'thumbnail' resources :attachments, :only => [:show, :destroy]
#4 Updated by Jean-Philippe Lang over 8 years ago
I'd prefer the migration:
Index: db/migrate/20130911193200_remove_eols_from_attachments_filename.rb =================================================================== --- db/migrate/20130911193200_remove_eols_from_attachments_filename.rb (revision 0) +++ db/migrate/20130911193200_remove_eols_from_attachments_filename.rb (revision 0) @@ -0,0 +1,12 @@ +class RemoveEolsFromAttachmentsFilename < ActiveRecord::Migration + def up + Attachment.where("filename like ? or filename like ?", "%\r%", "%\n%").each do |attachment| + filename = attachment.filename.to_s.tr("\r\n", "_") + Attachment.where(:id => attachment.id).update_all(:filename => filename) + end + end + + def down + # nop + end +end
If it works for you, I'll commit this fix.
#6 Updated by Jean-Philippe Lang over 8 years ago
- Status changed from New to Closed
- Assignee set to Jean-Philippe Lang
- Target version changed from Candidate for next major release to 2.4.0
- Resolution set to Fixed
Migration committed, thanks for the feedback.
#update_column: I prefer to stick with raw updates in migrations.
#update_column does the raw update in the same way and runs some code to reflect the change in the instance attributes. This is neither usefull nor desirable in this migration.