Replace unicode control codes with spaces in attachment filename

[Ryan J Ollos]

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

[Ryan J Ollos]

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.

[Ryan J Ollos]

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

[Jun Omae]

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

@@ -1060 +1060 @@
         attachment = Attachment(self.env, realm, id)
         attachment.author = author
         attachment.description = description
-        filename = _normalized_filename(path)
+        filename = os.path.basename(_normalized_filename(path))
         with open(path, 'rb') as f:
             attachment.insert(filename, f, os.path.getsize(path))
 

[Ryan J Ollos]

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

[Jun Omae]

Committed in [13607].

[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'm less certain, but perhaps the function would even be appropriate for trac.util?

[Jun Omae]

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.

[Ryan J Ollos]

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.

[Ryan J Ollos]

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?