spaze / phpstan-disallowed-calls

PHPStan rules to detect disallowed method & function calls, constant, namespace, attribute & superglobal usages
MIT License
263 stars 17 forks source link

Disable backslash escaping in "is allowed" helper #169

Closed mnastalski closed 1 year ago

mnastalski commented 1 year ago

allowIn will fail to match files on Windows without the FNM_NOESCAPE flag

spaze commented 1 year ago

Thanks. can you explain the bug you're seeing? Does this change fix it?

Because I have a test that compares Windows paths https://github.com/spaze/phpstan-disallowed-calls/blob/cc97bfb42eaf100cc474674c5170695b439cd474/tests/IsAllowedFileHelperTest.php#L84-L88 The matches() method calls absolutizePath() which calls normalizePath() which normalizes paths: https://github.com/phpstan/phpstan-src/blob/e5c1a594aa87df3a174afed58a1ef5a6cefe8d49/src/File/FileHelper.php#L77

mnastalski commented 1 year ago

Sure! Yes, the change does fix the issue.

My config is something like this:

    disallowedFunctionCalls:
        -
            function: 'foo()'
            message: 'use bar() instead'
            allowIn:
                - app/Models/Foo.php

When I run phpstan for Foo.php on a Windows machine I get:

 ------ ------------------------------------------------ 
  Line   Foo.php
 ------ ------------------------------------------------ 
  X      Calling foo() is forbidden, use bar() instead  
  Y      Calling foo() is forbidden, use bar() instead  

Adding FNM_NOESCAPE flag like in the PR makes phpstan pass:

[OK] No errors
mnastalski commented 1 year ago

As for the tests, they also seem a bit problematic on Windows (this is on PHP 8.1.3):

FFFF...FFFFFFF.................FFFF..FF.FFF.F                     45 / 45 (100%)

FAILURES!
Tests: 45, Assertions: 50, Failures: 21.

Log: https://gist.github.com/mnastalski/aebfe41bbfb6ae56ccdadb781c5d0ee0

If I add the flag the amount of errors is much lower:

...............................FFFF..F.......                     45 / 45 (100%)

There were 5 failures:

1) Spaze\PHPStan\Rules\Disallowed\IsAllowedFileHelperTest::testMatches with data set #0 ('src', 'XX\phpstan-di...ts/src', '/foo/bar/src')
Failed asserting that false is true.

XX\phpstan-disallowed-calls\tests\IsAllowedFileHelperTest.php:46

2) Spaze\PHPStan\Rules\Disallowed\IsAllowedFileHelperTest::testMatches with data set #1 ('src/*', 'XX\phpstan-di...do.php', '/foo/bar/src/waldo.php')
Failed asserting that false is true.

XX\phpstan-disallowed-calls\tests\IsAllowedFileHelperTest.php:46

3) Spaze\PHPStan\Rules\Disallowed\IsAllowedFileHelperTest::testMatches with data set #2 ('../src/*', 'XX\phpstan-di...do.php', '/foo/src/waldo.php')
Failed asserting that false is true.

XX\phpstan-disallowed-calls\tests\IsAllowedFileHelperTest.php:46

4) Spaze\PHPStan\Rules\Disallowed\IsAllowedFileHelperTest::testMatches with data set #3 ('src/foo/../*', 'XX\phpstan-di...do.php', '/foo/bar/src/waldo.php')
Failed asserting that false is true.

XX\phpstan-disallowed-calls\tests\IsAllowedFileHelperTest.php:46

5) Spaze\PHPStan\Rules\Disallowed\IsAllowedFileHelperTest::testMatches with data set #6 ('\src\foo\bar\', 'XX\phpstan-di...oo/bar', '/foo/bar/src/foo/bar')
Failed asserting that false is true.

XX\phpstan-disallowed-calls\tests\IsAllowedFileHelperTest.php:46

As you can see the Windows paths test you mentioned also fails. The first assertion (based on isAllowedHelper) will pass however, if the 2nd array value uses DIRECTORY_SEPARATOR instead of forward slashes, so for example:

        yield [
            '\\src\\foo\\bar\\',
            __DIR__ . DIRECTORY_SEPARATOR . 'src' . DIRECTORY_SEPARATOR . 'foo' . DIRECTORY_SEPARATOR . 'bar',
            '/foo/bar/src/foo/bar',
        ];

I played around with the second assertion (based on isAllowedHelperWithRootDir) a bit but I couldn't make it pass

spaze commented 1 year ago

I see, thanks for the PR! I'll try to get tests working on Windows too, sorry it's not working now 😊

spaze commented 1 year ago

I have released 2.11.6 with this PR. I have started testing on Windows too so you should be fine running it on Windows too, let me know. Thanks again, not just for this PR but for bringing the whole Windows problem to my attention.

mnastalski commented 1 year ago

Yup, everything passes now! Thanks!