Project

General

Profile

Actions

Defect #14819

closed

Newlines in attachment filename causes crash

Added by Felix Schäfer over 10 years ago. Updated over 10 years ago.

Status:
Closed
Priority:
Normal
Category:
Attachments
Target version:
Start date:
Due date:
% Done:

0%

Estimated time:
Resolution:
Fixed
Affected version:

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]
Actions #1

Updated by Etienne Massip over 10 years ago

  • Category set to Attachments
  • Target version set to Candidate for next major release
Actions #2

Updated by Jean-Philippe Lang over 10 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?

Actions #3

Updated by Felix Schäfer over 10 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.

Actions #4

Updated by Jean-Philippe Lang over 10 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.

Actions #5

Updated by Felix Schäfer over 10 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!

Actions #6

Updated by Jean-Philippe Lang over 10 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.

Actions

Also available in: Atom PDF