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

Disable backslash escaping in "is allowed" helper #169

Merged
merged 1 commit into from
Feb 6, 2023
Merged

Disable backslash escaping in "is allowed" helper #169

merged 1 commit into from
Feb 6, 2023

Conversation

mnastalski
Copy link
Contributor

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

@spaze
Copy link
Owner

spaze commented Feb 3, 2023

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

Because I have a test that compares Windows paths

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

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
Copy link
Contributor Author

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
Copy link
Contributor Author

mnastalski commented Feb 5, 2023

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 spaze merged commit 8ca05ab into spaze:main Feb 6, 2023
@spaze
Copy link
Owner

spaze commented Feb 6, 2023

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

@spaze
Copy link
Owner

spaze commented Feb 6, 2023

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
Copy link
Contributor Author

mnastalski commented Feb 7, 2023

Yup, everything passes now! Thanks!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants