Defect #14819
Newlines in attachment filename causes crash
| Status: | Closed | Start date: | ||
|---|---|---|---|---|
| Priority: | Normal | Due date: | ||
| Assignee: |  Jean-Philippe Lang | % Done: | 0% | |
| Category: | Attachments | |||
| Target version: | 2.4.0 | |||
| Resolution: | Fixed | Affected version: | 2.3.1 |
Description
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 /.
Patch:
--- 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]Associated revisions
Strip eols from file names (#14819).
Remove EOLs from attachments filename (#14819).
Typo (#14819).
History
#1
Updated by Etienne Massip over 7 years ago
- Category set to Attachments
- Target version set to Candidate for next major release
#2
Updated by Jean-Philippe Lang about 7 years ago
The handling of filenames with new lines seems broken anyway. Shouldn't we remove new lines from filenames instead or in addition to this patch?
#3
Updated by Felix Schäfer about 7 years ago
I wouldn't mind removing them, but that solves the problem only for new uploads. Existing uploads could be handled either by taking care of the routes as above, or by having a migration to normalize existing filenames of the DB.
#4
Updated by Jean-Philippe Lang about 7 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.
#5
Updated by Felix Schäfer about 7 years ago
Try attachment.update_column(:filename, filename), other than that it should work.
The fix in itself is good for us, we don't care wether it's solved one way or the other :-) Thanks!
#6
Updated by Jean-Philippe Lang about 7 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.
About #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.