Browse files

Merge pull request #5 from starzel/master

Make sure the id is safe before setting the filename as id
  • Loading branch information...
2 parents 861aa76 + cd628c2 commit 67935919b330cddb03fda08c81670439a34ab3ca @do3cc do3cc committed May 6, 2013
Showing with 6 additions and 3 deletions.
  1. +4 −3 CHANGES.rst
  2. +2 −0 src/plone/app/blob/field.py
View
7 CHANGES.rst
@@ -4,7 +4,8 @@ Changelog
1.5.9 (unreleased)
------------------
-- Nothing changed yet.
+- Make sure the id is safe before setting the filename as id
+ [pbauer]
1.5.8 (2013-04-06)
@@ -28,8 +29,8 @@ Changelog
- Fix BLOB migration when LinguaPlone is installed.
Also for ATFile.
- CAUTION: when the fix was discussed with witsch,
- he pointed to the fact that the files would be
+ CAUTION: when the fix was discussed with witsch,
+ he pointed to the fact that the files would be
entirely loaded in memory during migration.
This could potentially eat too much memory.
[gotcha]
View
2 src/plone/app/blob/field.py
@@ -1,4 +1,5 @@
from os import fstat
+from zope.container.interfaces import INameChooser
from zope.interface import implements
from StringIO import StringIO
from Acquisition import Implicit, aq_base
@@ -269,6 +270,7 @@ def fixAutoId(self, instance):
filename = IUserPreferredFileNameNormalizer(request).normalize(filename)
if filename and not filename == instance.getId():
# a file name was given, so the instance needs to be renamed...
+ filename = INameChooser(instance.__parent__).chooseName(filename, instance)
instance.setId(filename)
security.declareProtected(View, 'download')

9 comments on commit 6793591

@tdesvenain
Plone Foundation member

Hi @pbauer, @tisto,

This breaks tests in Products.ATContentTypes :

test_atfile.py l 94

def testUpperCaseFilename(self):
    fakefile = Fakefile()
    fakefile.filename = 'Some-filename-With-Uppercase.txt'
    id = 'file.2005-11-18.4066860573'
    self.folder.invokeFactory(self.portal_type, id)
    self.folder[id].setFile(fakefile)
    self.assertFalse(id in self.folder)
    self.assertTrue(fakefile.filename in self.folder)

def testUpperCaseFilenameWithFunnyCharacters(self):
    fakefile = Fakefile()
    fakefile.filename = 'Zope&Plo?ne .txt'
    id = 'file.2005-11-18.4066860574'
    self.folder.invokeFactory(self.portal_type, id)
    self.folder[id].setFile(fakefile)
    self.assertFalse(id in self.folder)
    self.assertTrue('Zope-Plo-ne .txt' in self.folder)

we get

'some-filename-with-uppercase.txt'
and
'zope-plo-ne.txt'

(and also a test in test_atimage.py)

should we revert this commit or update the ATContentTypes tests ?
I can do both, but i don't know what the community wants

Thomas

@tisto
Plone Foundation member

@tdesvenain You should revert the commit for now. We can not keep the build broken for a couple of days until we made a decision.

I'd say @pbauer should adapt his code to make the ATContentTypes tests pass.

The general problem here is that we see such regressions only if someone adds the package to auto-checkout. This was obviously forgotten in this case.

@tdesvenain
Plone Foundation member
@tdesvenain
Plone Foundation member
@tisto
Plone Foundation member

@tdesvenain I agree. Revert the commit in p.a.blob.

@tdesvenain
Plone Foundation member
@do3cc
Plone Foundation member

There is also no tag for 1.5.8.
I want to reintroduce this pull request with slightly modified behavior. A different INameChooser that does not lowercase everything but will still prevent " " in names. I'd also update the tests in Products.CMFPlone

@do3cc
Plone Foundation member

Too bad, but I can't just write a different INameChooser, the INameChooser gets adapted to the container, not to the object that wants a name.

IIRC the fix solves the problem that bad filenames of files uploaded do not throw exceptions. Also iirc, these exceptions get lost when using some form of quickuploader.

INameChooser is the canonical solution to choose good names, writing different code with different logic for choosing a safe name to make tests in CMFPlone pass will cause code bloat.

Right now the tests in CMFPlone are testing functionality that is provided by plone.app.blob. pab is the standard in all supported Plone versions. Therefor I'd propose to check whether pab contains similar tests and then remove the tests from CMFPlone.

@tdesvenain
Plone Foundation member
Please sign in to comment.