Edgewall Software
Modify

Opened 10 years ago

Closed 9 years ago

Last modified 8 years ago

#11395 closed enhancement (fixed)

Replace unicode control codes with spaces in attachment filename

Reported by: Ryan J Ollos Owned by: Jun Omae
Priority: normal Milestone: 1.1.3
Component: attachment Version:
Severity: normal Keywords: unicode control codes
Cc: Ryan J Ollos Branch:
Release Notes:

Unicode control codes are replaced with spaces in attachment filenames.

API Changes:
Internal Changes:

Description (last modified by Ryan J Ollos)

It was discussed on the mailing list that control codes in attachment filename should be replaced with whitespaces.

Attachments (1)

awesomeattachments.patch (4.2 KB ) - added by Ryan J Ollos 8 years ago.

Download all attachments as: .zip

Change History (13)

comment:1 by Ryan J Ollos, 10 years ago

Description: modified (diff)

comment:2 by Ryan J Ollos, 10 years ago

As mentioned on the mailing list, the following behaviors are seen:

  • In Chrome the control code will be url-encoded
  • In Firefox the control code will be replaced with whitespace
  • In Opera an error will be issued that the file can't be found

Even with the patch, the browser will have the first chance to modify the filename. So as far as I can tell, if we implement some behavior in AttachmentModule._do_save to replace control codes with whitespace, it will really only help in the case that there are some browsers that pass along the filename with the control-codes still present.

The following case was also mentioned on the mailing list:

$ echo "good day!" > "file
"
trac-admin ../tracdev attachment add wiki:WikiStart "file
"

When trying to view the file through the browser we get: No handler matched request to /attachment/wiki/WikiStart/file1 .txt.

I'm more convinced that this is hardly worth worrying about, but might something like the following work well?: log:rjollos.git:t11395

I don't understand everything that is going on inside of _normalized_filename, but at least we can easily write unit tests for it, and it might be appropriate for trac.util.

Last edited 10 years ago by Ryan J Ollos (previous) (diff)

comment:3 by Ryan J Ollos, 10 years ago

Milestone: next-stable-1.0.x1.0.3
Owner: set to Ryan J Ollos
Status: newassigned

comment:4 by Ryan J Ollos, 9 years ago

Milestone: 1.0.31.1.3
Release Notes: modified (diff)
Resolution: fixed
Status: assignedclosed

It seemed least risky to finally push this to the trunk. Committed in [13603].

comment:5 by Ryan J Ollos, 9 years ago

Cc: Ryan J Ollos added
Owner: changed from Ryan J Ollos to Jun Omae

comment:6 by Jun Omae, 9 years ago

Resolution: fixed
Status: closedreopened

After [13603], attachment add command adds an attachment with the original path name on Windows if the path is not a full path.

Trac [C:\usr\var\trac\1.1.3dev]> attachment add wiki:WikiStart .\contrib\trac-svn-post-commit-hook.cmd
Trac [C:\usr\var\trac\1.1.3dev]> attachment add wiki:WikiStart C:\usr\src\trac\trunk\contrib\README
Trac [C:\usr\var\trac\1.1.3dev]> attachment list wiki:WikiStart

Name                                       Size       Author  Date                 Description
----------------------------------------------------------------------------------------------
.\\contrib\\trac-svn-post-commit-hook.cmd  3.2 KB     trac    2015-01-03 18:11:17
README                                     328 bytes  trac    2015-01-03 18:18:08
  • trac/attachment.py

     
    10601060        attachment = Attachment(self.env, realm, id)
    10611061        attachment.author = author
    10621062        attachment.description = description
    1063         filename = _normalized_filename(path)
     1063        filename = os.path.basename(_normalized_filename(path))
    10641064        with open(path, 'rb') as f:
    10651065            attachment.insert(filename, f, os.path.getsize(path))
    10661066

comment:7 by Ryan J Ollos, 9 years ago

Thanks, I didn't test on Windows. Please go ahead and commit the change.

comment:8 by Jun Omae, 9 years ago

Resolution: fixed
Status: reopenedclosed

Committed in [13607].

comment:9 by Ryan J Ollos, 8 years ago

In [13603], I made _normalized_filename a protected function. However, while working on AwesomeAttachmentPlugin I've considered that it would be useful if this function was public, and there seems little downside to the change. Is there any objection to the change?

I'm less certain, but perhaps the function would even be appropriate for trac.util?

in reply to:  9 ; comment:10 by Jun Omae, 8 years ago

Replying to Ryan J Ollos:

In [13603], I made _normalized_filename a protected function. However, while working on AwesomeAttachmentPlugin I've considered that it would be useful if this function was public, and there seems little downside to the change. Is there any objection to the change?

I think such plugins need a utility method to add a new attachment with the filename normalization and checks of file size and permissions, not _normalize_filename.

th:TracDragDropPlugin calls AttachmentModule.process_request() using hack to add a attachment. See th:source:tracdragdropplugin/0.12/tracdragdrop/web_ui.py@14397:203-209,229-243#L224.

in reply to:  10 comment:11 by Ryan J Ollos, 8 years ago

Replying to Jun Omae:

I think such plugins need a utility method to add a new attachment with the filename normalization and checks of file size and permissions, not _normalize_filename.

I had similar thoughts, but was concerned about adding a method that wouldn't have enough general utility. Maybe we can separate out the req and redirect from _do_save and make a public method. I'll take another look.

by Ryan J Ollos, 8 years ago

Attachment: awesomeattachments.patch added

comment:12 by Ryan J Ollos, 8 years ago

I did some work in log:rjollos.git:attachment_module_refactoring. attachment:awesomeattachments.patch shows that the reduction in code size would be significant for that plugin.

I haven't looked closely at th:TracDragDropPlugin. Would the refactoring be useful for that case? Also, maybe the method name read_file is not good. It could be named, file_from_fieldstorage, or is there another suggestion?

Modify Ticket

Change Properties
Set your email in Preferences
Action
as closed The owner will remain Jun Omae.
The resolution will be deleted. Next status will be 'reopened'.
to The owner will be changed from Jun Omae to the specified user.

Add Comment


E-mail address and name can be saved in the Preferences .
 
Note: See TracTickets for help on using tickets.