Upload class prefixes dots in file name with underscore #1380

Closed
wesley-murch opened this Issue May 22, 2012 · 8 comments

Comments

Projects
None yet
4 participants
@wesley-murch

I just discovered that if you use a dot in the file name in your config using the Upload class, it converts all but the first dot to UnderscoreDot. I have to rename manually if I want more than one dot in the file name.

$config['file_name'] = 'my.file.name.with.dots.PDF';

Creates/renames the file to:

my.file_.name_.with_.dots_.PDF

Probably something to do with the feature that allows the file extension to be missing in $config['file_name'], which uses the uploaded file's extension.

Tested with CI 2.1.0

@palicao

This comment has been minimized.

Show comment Hide comment
@palicao

palicao May 22, 2012

This is a security feature related to the potential vulnerability discussed here: http://httpd.apache.org/docs/1.3/mod/mod_mime.html#multipleext

Maybe it would be cleaner if the dots inside the filename are removed completely?

palicao commented May 22, 2012

This is a security feature related to the potential vulnerability discussed here: http://httpd.apache.org/docs/1.3/mod/mod_mime.html#multipleext

Maybe it would be cleaner if the dots inside the filename are removed completely?

@wesley-murch

This comment has been minimized.

Show comment Hide comment
@wesley-murch

wesley-murch May 22, 2012

I see, it's an edge case but could be relevant, and CI always errs on the side of caution rather than let developers shoot themselves in the foot. This is only relevant when the developer is specifying a file name, but that specified name could itself contain unknown characters (user input, etc.)...

Something like $config['convert_dots'] // TRUE by default could give the ability to opt-out if you're confident and know what you're doing, but again - I don't think it necessarily fits into the CI security-first model.

Some background on my case: I had to convert uploads to an existing file name convention that included dots to separate time intervals, like 2012.05.22_13.20.55.PDF.

If the only question is what to replace the dots with, I would say just use dash or underscore - not _.. Note that the first dot is not suffixed. If the replacement was underscore, I would expect this:

$config['file_name'] = 'my.file.name.with.dots.PDF';

To create this file name:

my_file_name_with_dots.PDF

...and not this one:

my.file_name_with_dots_PDF

Same applies if the dots were just removed, the segment to the right should be the file extension.

I see, it's an edge case but could be relevant, and CI always errs on the side of caution rather than let developers shoot themselves in the foot. This is only relevant when the developer is specifying a file name, but that specified name could itself contain unknown characters (user input, etc.)...

Something like $config['convert_dots'] // TRUE by default could give the ability to opt-out if you're confident and know what you're doing, but again - I don't think it necessarily fits into the CI security-first model.

Some background on my case: I had to convert uploads to an existing file name convention that included dots to separate time intervals, like 2012.05.22_13.20.55.PDF.

If the only question is what to replace the dots with, I would say just use dash or underscore - not _.. Note that the first dot is not suffixed. If the replacement was underscore, I would expect this:

$config['file_name'] = 'my.file.name.with.dots.PDF';

To create this file name:

my_file_name_with_dots.PDF

...and not this one:

my.file_name_with_dots_PDF

Same applies if the dots were just removed, the segment to the right should be the file extension.

@palicao

This comment has been minimized.

Show comment Hide comment
@palicao

palicao May 22, 2012

It makes perfectly sense to me. If it's ok for lead developers I can modify this behaviour.

palicao commented May 22, 2012

It makes perfectly sense to me. If it's ok for lead developers I can modify this behaviour.

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg May 23, 2012

Contributor

Looking at the code that adds the underscores, I do find something rather illogical:

            if ( ! in_array(strtolower($part), $this->allowed_types) OR $this->mimes_types(strtolower($part)) === FALSE)
            {
                    $filename .= '.'.$part.'_';
            }
            else
            {
                    $filename .= '.'.$part;
            }

Why would an underscore be added when the chunk being checked is not a valid (and of course known) MIME type? That edge case should be expected in the exactly opposite condition. @derekjones @wesbaker

Contributor

narfbg commented May 23, 2012

Looking at the code that adds the underscores, I do find something rather illogical:

            if ( ! in_array(strtolower($part), $this->allowed_types) OR $this->mimes_types(strtolower($part)) === FALSE)
            {
                    $filename .= '.'.$part.'_';
            }
            else
            {
                    $filename .= '.'.$part;
            }

Why would an underscore be added when the chunk being checked is not a valid (and of course known) MIME type? That edge case should be expected in the exactly opposite condition. @derekjones @wesbaker

@derekjones

This comment has been minimized.

Show comment Hide comment
@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg May 23, 2012

Contributor

I was just about to explain that I was referring to the check itself, but a second look at it convinces me that it's done right - sorry.

So, you guys want a configurable replacement?

Contributor

narfbg commented May 23, 2012

I was just about to explain that I was referring to the check itself, but a second look at it convinces me that it's done right - sorry.

So, you guys want a configurable replacement?

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Nov 6, 2012

Contributor

Might be worth auto-detecting if we're running under Apache, using in_array('mod_mime', apache_get_modules()) and/or parsing the Apache mods-enabled/mime.conf values and checking if the value that we're prefixing with an underscore is a valid type.

Contributor

narfbg commented Nov 6, 2012

Might be worth auto-detecting if we're running under Apache, using in_array('mod_mime', apache_get_modules()) and/or parsing the Apache mods-enabled/mime.conf values and checking if the value that we're prefixing with an underscore is a valid type.

@narfbg

This comment has been minimized.

Show comment Hide comment
@narfbg

narfbg Oct 21, 2013

Contributor
Contributor

narfbg commented Oct 21, 2013

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment