Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Zend_Validate_File_Extension::isValid() fails if filename doesn't exist #648

Open
wants to merge 1 commit into
base: 1.9.4.x
from

Conversation

Projects
None yet
@edannenberg
Copy link
Collaborator

edannenberg commented Apr 2, 2019

Fixes regression introduced in 1.9.4.1. See #646. The root cause has been in the Magento core since probably forever, the class is just barely used so it wasn't an issue until now.

The few places that actually use the class have their own file checks in place already, like here.

@sreichel
Copy link
Collaborator

sreichel left a comment

Mage_Core_Model_Email_Template_Abstract seems to be the only file where it is used with own file check ...

It's used in all Zend File validator classes ...

@Flyingmana

This comment has been minimized.

Copy link
Member

Flyingmana commented Apr 3, 2019

I vote against this change, as it changes the API of this function in a way, which creates an incompatibility against ZF2 which also checks if the file is readable

https://github.com/zendframework/zend-validator/blob/master/src/File/Extension.php

@edannenberg

This comment has been minimized.

Copy link
Collaborator Author

edannenberg commented Apr 3, 2019

@sreichel

It's used in all Zend File validator classes ...

Not really, it's only used by Zend_Validate_File_ExcludeExtension. All other references are wrong doc blocks: https://github.com/OpenMage/magento-lts/blob/1.9.4.x/lib/Zend/Validate/File/Exists.php#L103

@Flyingmana

I vote against this change, as it changes the API of this function in a way, which creates an incompatibility against ZF2 which also checks if the file is readable

Why do we care about ZF2? The API as it is now is broken. It makes no sense that you have to create the file first before you can check if it has a valid extension. If you look at the implementation in ZF2 it's pretty clear that it got blindly refactored without much thought. For example File/Exists still sports the same wrong doc-blocks from ZF1 even though it got refactored.

@colinmollenhour
Copy link
Contributor

colinmollenhour left a comment

I agree with this change. Checking if the file is readable in this location is simply bad code and I see no compelling reason to maintain backwards compatibility.

@Flyingmana

This comment has been minimized.

Copy link
Member

Flyingmana commented Apr 3, 2019

its in this case not only backwards, but also forward compatibility

@Flyingmana

This comment has been minimized.

Copy link
Member

Flyingmana commented Apr 3, 2019

To explain it a bit more. I care, because 3th party modules may use this, and it will change behaviour for them.
Also we increase incompatibility with Magento2, which is still using the ZF Validator to validate file extensions, so instead of making it easier to do a future switch, we make it harder, as someone cant make use of this for module parts which should work for both versions in the same way.

@edannenberg

This comment has been minimized.

Copy link
Collaborator Author

edannenberg commented Apr 3, 2019

I care, because 3th party modules may use this, and it will change behaviour for them.

This would only change behaviour if someone relied on Zend_Validate_File_Extension to validate if a file exists, which is highly unlikely IMO.

Also we increase incompatibility with Magento2, which is still using the ZF Validator to validate file extensions, so instead of making it easier to do a future switch, we make it harder, as someone cant make use of this for module parts which should work for both versions in the same way.

I would recommend to open a ticket/PR with ZF2 or Magento 2 then so this can get fixed. It's clearly a bug.

@sreichel

This comment has been minimized.

Copy link
Collaborator

sreichel commented Apr 3, 2019

@edannenberg if I watch the same file again and again ... :) You're right.

Instead of removing this check ... how about check if $file param is set (@param array $file File data from Zend_File_Transfer)?

    if ($file !== null && !Zend_Loader::isReadable($value)) {
        return $this->_throw($file, self::NOT_FOUND);
    }
@chelevich

This comment has been minimized.

Copy link

chelevich commented Apr 3, 2019

In my project

  • I created MyOwn_Validate_File_Extension,
  • overridden isValid method
  • called parent and if error was NOT_FOUND I created the file in the same way as Mage::log() does,
  • then I re-validated the file with the second parent call
    And I used it in Mage::log()
    Here we can do the same in lib/Varien/Validate/File/Extension.php
@cslogic

This comment has been minimized.

Copy link

cslogic commented Apr 3, 2019

At Mage.php on line 817, $logValidator is there to check if the file has one of $_allowedFileExtensions, which is ok. My bet is that there is nothing wrong with Zend_Validate_File_Extension, I just think it is not the right class to check the extension of an file that doesn't even exist. We gotta find another way to do that.

@edannenberg

This comment has been minimized.

Copy link
Collaborator Author

edannenberg commented Apr 3, 2019

@sreichel

Instead of removing this check ... how about check if $file param is set (@param array $file File data from Zend_File_Transfer)?

Good point but I would be surprised if Zend_File_Transfer doesn't handle this already so it would be kinda pointless to check again. Checking if a file exists is simply out of scope for Zend_Validate_File_Extension as it only has a single job, check the file extension for a given filename, straight from the ZF1 api docs:

isValid (line 189)

Defined by Zend_Validate_Interface

Returns true if and only if the fileextension of $value is included in the set extension list.
@colinmollenhour

This comment has been minimized.

Copy link
Contributor

colinmollenhour commented Apr 3, 2019

Given the context that this is an extremely simple class/method where the documentation clearly states what it was intended to do and that the code doesn't do what all other indicators say it should do, and this incorrect behavior is actually causing a serious bug that needs to be fixed, I think this BC break is acceptable. ZF is full of terrible code so this does not surprise me and seems pretty clearly to be a case where the author mistakenly thought that the file must exist for the pathinfo function to work. On top of that it is using Zend_Loader which is meant to be used for auto-loading classes and is dangerously close to methods like Zend_Loader::loadFile that would be used for RCE attacks.

What is the purpose of tracking ZF1 code closely with ZF2 code? ZF2 is hardly any better than ZF1, basically ZF1 with namespaces and it is basically a dead project itself.

@sreichel

This comment has been minimized.

Copy link
Collaborator

sreichel commented Apr 3, 2019

@edannenberg I agree with "it is out of scope" in that file - so I approved first.

Maybe I'm wrong - just quickly searched - but it seeems to be used in Mage_Catalog_Model_Product_Option_Type_File ... but also there is a is_readable($fileFullPath) check.

Mabye someone has time to check.

However ... if we decide to remove it, it should be removed from Zend_Validate_File_ExcludeExtension too.

Schrank referenced this pull request in OpenMage/magento-mirror Apr 4, 2019

@theroch

This comment has been minimized.

Copy link

theroch commented Apr 4, 2019

@sreichel I confirm with you, removing is not the correct approach.
How it is with this solution?

if (file_exists($value) && !Zend_Loader::isReadable($value)) {
            return $this->_throw($file, self::NOT_FOUND);
        }

It solves the problem for me.

Because this is a bug with the zend framework, I've modified my solution a little bit.
So you have to add the fixed Extension.php to app/code/core/Zend/Validate/File folder.

@theroch
Copy link

theroch left a comment

// Is file readable ?
        require_once 'Zend/Loader.php';
        if (file_exists($value) && !Zend_Loader::isReadable($value)) {
            return $this->_throw($file, self::NOT_FOUND);
        }

@theroch
Copy link

theroch left a comment

Zend_Validate_File_Extension::isValid() fails if filename doesn't exist
The purpose of the class is to check if a given filename has an
expected file extension. It shouldn't matter if the file actually
exists or not, the method only operates on the given string.

Also fixes Zend_Validate_File_ExcludeExtension which has the same
issue.

refs: #646

@edannenberg edannenberg force-pushed the edannenberg:fix/zend_validator branch from f8accab to 5dfcf3c Apr 4, 2019

@edannenberg

This comment has been minimized.

Copy link
Collaborator Author

edannenberg commented Apr 4, 2019

@sreichel

Maybe I'm wrong - just quickly searched - but it seeems to be used in Mage_Catalog_Model_Product_Option_Type_File ... but also there is a is_readable($fileFullPath) check.

The 2 places in Magento core where Zend_Validate_File_Extension is used don't rely on the faulty behaviour to ensure that the file exists. As it should be.

However ... if we decide to remove it, it should be removed from Zend_Validate_File_ExcludeExtension too.

Sure, we might as well. I amended the commit.

@theroch

@sreichel I confirm with you, removing is not the correct approach.
How it is with this solution?

It's a bit late for april's fools.. ;) Take a look at your patch, the if condition will never trigger, it's functionally equivalent to removing.

@theroch

This comment has been minimized.

Copy link

theroch commented Apr 4, 2019

@edannenberg That was not a joke. The file_exists funtion returns true if the file exists but the user has no read access to the file. is_readable returns only true if the user has read access or the file is not locked by another process.

@edannenberg

This comment has been minimized.

Copy link
Collaborator Author

edannenberg commented Apr 4, 2019

The file_exists funtion returns true if the file exists but the user has no read access to the file. is_readable returns only true if the user has read access or the file is not locked by another process.

Ok so it will virtually never trigger. What is accomplished in the rare case this actually does trigger? How is this relevant to a validator class who's sole purpose is to check a given string for valid file extensions?

Edit: If there actually is 3rd party code that relies on the faulty behaviour to ensure a file exists, your patch would still break BC except for the rare case that a file exists but isn't readable.

@sreichel

This comment has been minimized.

Copy link
Collaborator

sreichel commented Apr 4, 2019

@Flyingmana I understand your concerns ... but IMHO we need a quick fix here. Would it be OK to approve this and later change it, if there is a better solution?

@theroch

This comment has been minimized.

Copy link

theroch commented Apr 4, 2019

Ok so it will virtually never trigger. What is accomplished in the rare case this actually does trigger? How is this relevant to a validator class who's sole purpose is to check a given string for valid file extensions?

It has nothing to do with checking the permitted file extensions. However, the function was probably implemented, since it should usually be possible to read a file.
The function is also used in Mage_Core_Model_Email_Template_Abstract, but there the is_readable function is checked explicitly.
So the part could also be removed from the isValid method.

@colinmollenhour

This comment has been minimized.

Copy link
Contributor

colinmollenhour commented Apr 4, 2019

Alternatively we could just change the Mage code to check the file extensions using a non-Zend method since the Zend code seems to be the hang-up.. E.g.:

if ( ! ($extension = pathinfo($file, PATHINFO_EXTENSION)) || ! in_array($extension, $_allowedFileExtensions)) {
    return;
}

I never was a fan of using all of these silly wrappers for such basic functionality.. Example: Zend_Loader::isReadable() vs is_readable().

@cslogic

This comment has been minimized.

Copy link

cslogic commented Apr 4, 2019

Alternatively we could just change the Mage code to check the file extensions using a non-Zend method since the Zend code seems to be the hang-up..

That's exactly what I have meant earlier, and I still think that this is the best solution as it keep compatibility and fixes the problem...

@sreichel

This comment has been minimized.

Copy link
Collaborator

sreichel commented Apr 4, 2019

Alternatively we could just change the Mage code to check the file extensions using a non-Zend method since the Zend code seems to be the hang-up.. E.g.:

if ( ! ($extension = pathinfo($file, PATHINFO_EXTENSION)) || ! in_array($extension, $_allowedFileExtensions)) {
    return;
}

I never was a fan of using all of these silly wrappers for such basic functionality.. Example: Zend_Loader::isReadable() vs is_readable().

This is excatly what tho old method has done ...

    /**
     * Checking if file extensions is allowed. If passed then return true.
     *
     * @param $file
     * @return bool
     */
    public function isLogFileExtensionValid($file)
    {
        $result = false;
        $validatedFileExtension = pathinfo($file, PATHINFO_EXTENSION);
        if ($validatedFileExtension && in_array($validatedFileExtension, $this->_allowedFileExtensions)) {
            $result = true;
        }

        return $result;
    }

May "fix" it with changing this method? Just have to get allowed extensions from 'dev/log/allowedFileExtensions' config, instead of a private var?

@edannenberg

This comment has been minimized.

Copy link
Collaborator Author

edannenberg commented Apr 4, 2019

I really don't get the BC worries for this one. BC is important but really shouldn't cater to support shoddy code. This PR fixes the root cause while also restoring the intended functionality of the class, preventing any future issues. Do we really want to declare Zend_Validate_File_Extension to be broken with #wontfix because there is a tiny probability that some code relies on undocumented buggy behavior? Even though all usage we have seen so far suggests that users of the class did not mistake this for a file exists validator?

@Flyingmana

This comment has been minimized.

Copy link
Member

Flyingmana commented Apr 4, 2019

just for clarification. Iam against it, but I will not block it. If you have the required 2 approvers, I will merge it.

But, BC is not a topic which depends on how much sense a behavior makes, or if we like it or not.
Its about trust, trust that we do not break other peoples usecases, regardless how odd they may seem.
Its why there is semantic versioning.
There are reasons why some "bugs" in behavior are only fixed with minor or even major version increase. ZF2 may be already dead, but it is still used. Also remember the effort they had with replacing ZF1 with ZF2 in M2. Think how long it will take to get rid of ZF2
In my opinion this should either get fixed on Magento side, or by forking this class into an own namespace.
But as I said, I am not here as a blocker, just want to make sure you are aware of this and keep the longterm effect of this in mind.

@sreichel

This comment has been minimized.

Copy link
Collaborator

sreichel commented Apr 4, 2019

just for clarification. Iam against it, but I will not block it. If you have the required 2 approvers, I will merge it. [...] But as I said, I am not here as a blocker, just want to make sure you are aware of this and keep the longterm effect of this in mind.

@Flyingmana Thanks. This is why i've asked for review .... personally I don't want to overrule any of the initators/longest-time members. So i'm not going to approve.

When a raise an issue, i normally do not ask if we should wait for Magento fixing it - it does not happen :) In the case im "pretty" sure it will be fixed in "some" way "soon" ... maybe with fixing ZF.

If you dont want to change ZF, do the most simple for now and revert this part of the patch or "adjust" isLogFileExtensionValid()?

sreichel added a commit to sreichel/magento-lts that referenced this pull request Apr 4, 2019

Fix broken logging in 1.9.4.1, also see OpenMage#648
This reverts that part of last patch that uses Zend_Validate_File_Extension. The new feature, that gets allowed extensions from system.xml has moved to isLogFileExtensionValid().
@edannenberg

This comment has been minimized.

Copy link
Collaborator Author

edannenberg commented Apr 5, 2019

But, BC is not a topic which depends on how much sense a behavior makes, or if we like it or not.
Its about trust, trust that we do not break other peoples usecases, regardless how odd they may seem.

I'm not sure how we can fix anything in this repo then, for example #525 should have never get merged then because there might be a poor soul that still used it.. I understand the importance of BC and have to deal with it on a daily basis, blindly following that rule isn't the way to go about it. Instead we should look at each BC issue indiviually and assess the actual impact. Theoretical or buggy 3rd party code BC issues should never prevent valid use cases / bug fixes unless the expected impact outweights the benefits.

@colinmollenhour
Copy link
Contributor

colinmollenhour left a comment

Approving since:

  • Zend_Validate_File_Extension is not Zend_Validate_File_IsReadable.
  • I believe it is extremely unlikely that anyone is depending on this undocumented and unintuitive behavior and so the importance of fixing a bug outweighs BC/FC concerns (IMHO).
  • I can't really imagine where not checking if a file is readable is going to create a security threat - if the file path is readable then the method would already pass, and if it was not readable then what is the harm of passing validation for an unreadable or non-existent file? That is, if a file doesn't exist then it can't for example contain sensitive data or if it exists but is unreadable by the PHP process then the potentially sensitive data cannot be read anyway.
@cslogic

cslogic approved these changes Apr 5, 2019

@piotrekkaminski

This comment has been minimized.

Copy link

piotrekkaminski commented Apr 8, 2019

This is the current official patch that will be ported to earlier versions (this should work on latest) https://gist.github.com/piotrekkaminski/0596cae2d25bf467edbd3d3f03ab9f8f

@kkrieger85
Copy link
Contributor

kkrieger85 left a comment

Please use the official patch:

#648 (comment)

@royduin

This comment has been minimized.

Copy link
Contributor

royduin commented Apr 10, 2019

Did anyone contact Magento about this? What did they say? Do they confirm this is a bug and are they going to fix this?

@FredericMartinez

This comment has been minimized.

Copy link
Contributor

FredericMartinez commented Apr 10, 2019

@piotrekkaminski It works on production with 1.9.4.1 LTS, thanks!

@BassemN

This comment has been minimized.

Copy link

BassemN commented Apr 11, 2019

This is the current official patch that will be ported to earlier versions (this should work on latest) https://gist.github.com/piotrekkaminski/0596cae2d25bf467edbd3d3f03ab9f8f

@piotrekkaminski It works on production with 1.9.4.1 LTS, thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.