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

False negatives when denied call used in: first class callable, dynamic call, anonymous class, array callable, ... #275

Open
janedbal opened this issue Dec 3, 2024 · 3 comments

Comments

@janedbal
Copy link

janedbal commented Dec 3, 2024

Hi, this package is very similar to our rule in shipmonk/phpstan-rules under forbidCustomFunctions

I was considering to switch to your package as it has far more features (and possibly phase-out our rule one day), but when I ran quick comparison of the behaviour of our packages, I found that phpstan-disallowed-calls is missing quite a few features, most notably it suffers from those false-negatives:

  • First class callables not detected
    • denied_function(...);
  • Dynamic calls not detected
    • $fn = 'denied_function'; $fn();
  • Anonymous classes not detected
    • new class extends ClassWithForbiddenConstructor {};
  • Callables not detected
    • array_map('denied_function', $array);
    • array_map([$class, 'forbiddenMethod'], $array);
  • See the MR for full diff, there are few other differences

Due to that, I'm sticking with our implementation for now, but if you ever consider implementing those, please let me know and I'll reconsider again :)

@spaze
Copy link
Owner

spaze commented Dec 4, 2024

Hi, thanks for pointing out the issues. I've noticed your PR adding most of the missing detections to shipmonk/phpstan-rules, feel free to send a similar one, if you'd like to have those also supported here.

The purpose of spaze/phpstan-disallowed-calls has never been to detect all possible usages (as documented!) but I'll happily add more detections. Until then feel free to use whatever suits your needs the best :-)

spaze added a commit that referenced this issue Dec 6, 2024
For example:
```php
$sneaky = 'print_r';
$sneaky('foo');
```
and
```php
('print_r')('foo');
```

Ref #275
spaze added a commit that referenced this issue Dec 6, 2024
The things were already detected and now there's a test for them as well. Anonymous classes are displayed as `class@anonymous` similar to what `get_class(new class {})` would return.

- Test disallowed constants on anonymous child classes
- Test disallowed method calls on anonymous classes
- Test anonymous class inheriting from a disallowed class

Ref #275
@spaze
Copy link
Owner

spaze commented Dec 6, 2024

I have started working on at least some of the missing detections, the list and the progress is below, will update it as more detections are added. There still may be some differences, or something I've missed. I'm getting lost a bit in shipmonk-rnd/phpstan-rules#292 so I'd appreciate if you could, once I think I've added all the detections below, run the tests again and interpret them for me like you did above :-)

spaze added a commit that referenced this issue Dec 7, 2024
Detect
- `${'variable'}('foo')` dynamic function calls
- `$object->$method()` and `$object->${'method'}()`
- `Class::$method()` and `Class::${'method'}()`

Ref #275
Follow-up to #276
spaze added a commit that referenced this issue Dec 8, 2024
This is the syntax supported by PHP 8.1:
```
$callable = print_r(...);
$callable(42);
```
or
```
$blade = new \Waldo\Quux\Blade();
$callable = $blade->runner(...);
$callable(303);
```

The errors for the disallowed code are reported on lines with `(...)`, not when the callable is called.

Ref #275
spaze added a commit that referenced this issue Dec 8, 2024
This is the syntax supported by PHP 8.1:
```php
$func = print_r(...);
$func();
```
or
```php
$obj = new Object();
$func = $obj->method(...);
$func();
```
or
```php
$func = Class::method(...);
$func();
```

The errors for the disallowed code are reported on lines with `(...)`, not when the callable is called.

Ref #275
spaze added a commit that referenced this issue Jan 9, 2025
Reuses existing detection from DisallowedFunctionRuleErrors (for simple `'function'` callables) and DisallowedMethodRuleErrors (for `[$object, 'method']` callables) called from the new DisallowedCallableParameterRuleErrors service.

Ref #275
spaze added a commit that referenced this issue Jan 9, 2025
Reuses existing detection from DisallowedFunctionRuleErrors (for simple `'function'` callables) and DisallowedMethodRuleErrors (for `[$object, 'method']` callables) called from the new DisallowedCallableParameterRuleErrors service.

Ref #275
spaze added a commit that referenced this issue Jan 9, 2025
Detects disallowed methods and functions in callable parameters

For example:
- `array_map('function', []);`
- `array_map([$object, 'method'], []);`
- `array_map([Class::class, 'staticMethod']);`
- `IntlChar::enumCharTypes('function');`
- also in custom functions and methods

Ref #275
spaze added a commit that referenced this issue Jan 9, 2025
spaze added a commit that referenced this issue Jan 9, 2025
spaze added a commit that referenced this issue Jan 9, 2025
Callables detection added in #281 but I missed callable detection in constructors, e.g.:

```php
new ConstructorCallable('strlen');
new ConstructorCallable([$obj, 'method']);
```

Follow-up to #281
Ref #275
@spaze
Copy link
Owner

spaze commented Jan 9, 2025

I have released 4.1.1 now, adds new detections as listed in the comment above.

@janedbal can you please check again what's missing or what are the differences now? And thanks for shipmonk-rnd/phpstan-rules#288, I have learned some new things I have used here (like ParametersAcceptorSelector::selectFromArgs and ArgumentsNormalizer::reorderArgs).

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

No branches or pull requests

2 participants