[HttpFoundation] UploadedFile needs Locale parameter to pass

[helios-ag]

Because function basename() is locale aware, the constructor need to pass a locale parameter and set it in constructor. Without setting a locale non-latin first characters cut off from beginning until a latin letter or a non-letter character found in the name.

from HttpFoundation\File\UploadedFile.php

    $this->originalName = basename($originalName);
    $this->mimeType = $mimeType ?: 'application/octet-stream';
    $this->size = $size;
    $this->error = $error ?: UPLOAD_ERR_OK;
    $this->test = (Boolean) $test;

    parent::__construct($path, UPLOAD_ERR_OK === $this->error);
}

Should be like this (an example):
setlocale(LC_ALL, 'ru_RU.UTF8');
$this->originalName = basename($originalName);
$this->mimeType = $mimeType ?: 'application/octet-stream';
$this->size = $size;
$this->error = $error ?: UPLOAD_ERR_OK;
$this->test = (Boolean) $test;

    parent::__construct($path, UPLOAD_ERR_OK === $this->error);
}

[stealth35]

Maybe just :

<?php
$this->originalName = substr(strrchr(str_replace('\\', '/', $originalName), '/'), 1);

BTW why use basename ?

[stof]

@fabpot ping

[helios-ag]

@fabpot ping

[dinamic]

@fabpot ping

[helios-ag]

@fabpot ping

[dinamic]

Imho, this should not modify the value that comes from the browser - one might need it as raw value.

This check should matter only if you are trying to directly save the uploaded file without specifying a filename to be used.

[dinamic]

@fabpot ping

[vicb]

@dinamic this value could be forged so sanitization is required.

Could anyone submit a PR with the fix suggested by @stealth35 + unit tests.

Thanks.

[dinamic]

@vicb I meant to say that the sanitization which is performed is going to be needed only if you rely on the $originalName to return you a value, which would be safe to use on the current system. Doing so however should fall beyond the scope of this class.

Let me explain more promptly my point of view.

I will give you an example case:

• I want to upload a file named "aux"
• The server is powered by Windows NT

While this is perfectly valid case and you will have no troubles running it under *NIX, you will fall into trouble if your hosting service is running on Windows-based OS, as the "AUX" is a reserved filename there.

More information: http://en.wikipedia.org/wiki/Filename#Reserved_characters_and_words

Imho, the value of originalName should not be filtered and it has to be up for the developer to choose a filename for the uploaded file. If some filtration has to be applied, it should be minimal and the developer should have access to the unmodified value.

Note: if we accept that the value would not be filtered, it has to be reflected into the docs and stress how important is to not rely on the safety of the value.

[vicb]

The purpose of the sanitization is not the make the name bullet proof but to prevent security issues (there is no way to prevent a developer from using it even if he must not) - the $originalName can still contain reserved characters or be a reserved name.

getClientOriginalName() already has a warning in its phpDoc: Then is should not be considered as a safe value.

What would be the advantage of having access to the unfiltered name ?

[dinamic]

Well, it would prevent us from getting one value from the client of our website and display a different one.

We could filter it, off course, but it has to be up for the developer to decide it and the framework should not force filtration. Otherwise, some developers might just go backward and use the $_FILES array to get the job done.

[vicb]

The sanitization removes the path segment and leaves only the basename. I have just re-tested (w/ chrome & ff) and only the basename is sent by the browser, meaning that the sanitization is transparent and will only have an effect on forged values.

There must be something I don't get ?

[dinamic]

Dunno, I just hate it when there's a restriction which might cause a bug in your application and you can't get the raw value and process it by yourself. Right now, we have multiple opened issues because of this and I cannot implement a workaround without actually modifying the sf2 code or completely handle the upload process, validation and entity creation by myself.

I just want a getter method which would return the unaltered value, so if the new sanitization blocks me or anyone else - he could handle it gracefully until fixed.

[vicb]

Dunno, I just hate it when there's a restriction which might cause a bug in your application and you can't get the raw value and process it by yourself. Right now, we have multiple opened issues because of this and I cannot implement a workaround without actually modifying the sf2 code or completely handle the upload process, validation and entity creation by myself.

As stated in my last comment, you'll get the raw value unless someone forges a request, how could that cause issues ?

[dinamic]

Well, I believe the same intention applied while setting basename() in first place :)