an

a SplFileInfo [...] no?

an SplFileInfo afaik

The class_exists() call is useless (if the class is missing, the MimeTypeGuesserInterface is missing too).

It is not useless in the following case: new DataUriNormalizer() (without argument).

The constructor argument is optional, so the existence of the interface will not be checked and you cannot know if MimeTypeGuesser exists or no.

We would need a type-check on this to make sure it's an SplFileInfo. I know that you pointed to supportsNormalization as checking for this, but that's not good enough. Not being a normalizer expert, I just looked at this method at it confused me. It's not clear without checking for the type.

The pro as you pointed out is clarity.
The con is the (little) performance impact.

As dealing with data: URI is slow by definition, we can add it here.

Why you limit explode() too 2 & use just one variable?

Because it won't work:

php > var_dump(explode('/', 'text/plain', 1));
array(1) {
  [0] =>
  string(10) "text/plain"
}

minutiae detail, but I'd like this line moved right before the if statement that actually uses $typeName - it's not needed up here.

What if the object does not have an openFile() method?

It is not possible because of https://github.com/symfony/symfony/pull/16164/files#diff-b7fc65c7d852312152e353f395fc70a8R77

So when can $object not be an instance of SplFileObject then?

When it's an instance of SplFileInfo (SplFileObject extends SplFileInfo but the reverse is not true).

I agree thought that this reads weird to me. The entire class suffers a little bit of complexity since we're handling SplFileInfo, SplFileObject and File (even though they are all instances of SplFileInfo). It might be better to have 2 (or 3) uri normalizers and use a base class to share code. Using some private functions my add clarity (e.g. private function getMimeType(\SplFileInfo $object) and extractSplFileObject(\SplFileInfo $object)).

After looking for awhile, I don't think there is a problem - it can just be more clear :)

Splitting this class in 3 normalizers is not convenient for users using the component. It will require to instantiate and register 3 normalizers only to have a correct data: URI support.

Making this decision seems a little bit arbitrary... but it kind of makes sense.

^data:([a-z0-9]+\/[a-z0-9]+(;[a-z0-9\-]+\=[a-z0-9\-]+)?)?(;base64)?,
[a-z0-9\!\$\&\\\'\,\(\)\*\+\,\;\=\-\.\_\~\:\@\/\?\%\s]*\s*$

...could be made posessive/greedy for better performance...

^data:([a-z0-9]++\/[a-z0-9]++(;[a-z0-9\-]++\=[a-z0-9\-]++)?)?(;base64)?,
[a-z0-9\!\$\&\\\'\,\(\)\*\+\,\;\=\-\.\_\~\:\@\/\?\%\s]*+\s*+$

On the other hand, is matching the string all the way to the end really necessary (with a large list of allowed characters)?

I mean we're not validating if the actual data corresponds with the encoding specification, the user could still (for example) use the base64 option but not send base64 encoded data.

We're taking the correctness of the data (more or less) for granted -- so why allow the regex engine to even bother scanning it?

There is two main reasons:

• this is very security sensitive (in my former implementation, it was allowing to access to /etc/shadow for instance and, being not a security expert, I prefer to be conservative - what about specials chars...)
• the PHP stream wrapper can generate an error if the data is invalid, the regex prevent it

However 👍 to make the regex as fast as possible

Uh-oh, i can't imagine how the /etc/shadow file could get exposed since the
prefix is predetermined, can you please give me some documentation?

On 20:14, Thu, Feb 4, 2016 Kévin Dunglas notifications@github.com wrote:

In src/Symfony/Component/Serializer/Normalizer/DataUriNormalizer.php
#16164 (comment):

•    return $data instanceof \SplFileInfo;
  

• }

• /**

• \* {@inheritdoc}
  

• *
  

• \* Regex adapted from Brian Grinstead code.
  

• *
  

• \* @see https://gist.github.com/bgrins/6194623
  

• *
  

• \* @throws InvalidArgumentException
  

• \* @throws UnexpectedValueException
  

• */
  

• public function denormalize($data, $class, $format = null, array $context = array())
• {

•    if (!preg_match('/^data:([a-z0-9]+\/[a-z0-9]+(;[a-z0-9-]+\=[a-z0-9-]+)?)?(;base64)?,[a-z0-9!\$&\\'\,()*+\,\;\=-._~:\@\/\?\%\s]_\s_$/i', $data)) {
  

There is two main reasons:

• this is very security sensitive (in my former implementation, it was
  allowing to access to /etc/shadow for instance and, being not a security
  expert, I prefer to be conservative - what about specials chars...)
• the PHP stream wrapper can generate an error if the data is invalid,
  the regex prevent it

However [image: 👍] to make the regex as fast as possible

—
Reply to this email directly or view it on GitHub
https://github.com/symfony/symfony/pull/16164/files#r51913006.

@dunglas the original RegEx does not match application/octet-stream because the second part of the mime type is not allowed to contain a dash.

Which means the DataUriNormalizer can't match a data uri that it produced (when no mime type info was available).

Also there's an issue in the normalizer logic, the Symfony mime type guesser tries to fetch the path of the file and assert that the file exists. This does not work with php://memory / php://temp or data: type files.

I could submit a PR with the aforementioned speedups as well as some logic to make the mime type guessing part optional (ex.: supply a mime type via $context).

I'm checking out this list and apparently literal dots and underscores are also (sometimes) used in mime-type names (especially in the second part) and dashes are (sometimes) used in the first part.

So the RegEx needs to allow for those too.

I think we should check for other class values to an throw an exception if we don't support the given class. That makes it easier for users to spot errors (for example, referring a normalizer they didn't intend to use).

Why not applying the regex to the data here too?

Why not but will it be easy to debug that this validator is skipped regardless of the $type parameter just because the data coming from the user doesn't match a regex? IMO throwing an exception in denormalize() as we do right now is more intuitive.

This test looks useless to me.

I agree with you that it's not very useful but it guaranties that nobody remove the implements or an interface.