Undesirable side-effects of is_callable() use

[lupinitylabs]

Slack Notification Channel Version

2.5.0

Laravel Version

10.4.1

PHP Version

8.2.4

Database Driver & Version

No response

Description

The use of is_callable() at SlackAttachment.php has undesirable and potentially dangerous side-effects:

 public function field($title, $content = '')
    {
        if (is_callable($title)) {
            $callback = $title;

            $callback($attachmentField = new SlackAttachmentField);

So, imagine you are creating a message with an attachment field like this:

 (new \Illuminate\Notifications\Messages\SlackMessage())
    ->attachment(fn($attachment) => $attachment->field('dd', 'Whoops')
);

In this case, is_callable('dd') will of course return true and call dd() with the SlackAttachmentField as a parameter:

Illuminate\Notifications\Messages\SlackAttachmentField^ {#5154 // vendor/laravel/slack-notification-channel/src/Messages/SlackAttachment.php:216
  #title: null
  #content: null
  #short: true

The same goes for any other callable functions, things like DB::supportedDrivers, session_destroy... I leave it up to the readers' imagination what this could be used for.

A possible fix would be to accept closures only and check by if ($title instanceof Closure). While this would be a breaking change, I think it is worth consideration.

What do you think?

Steps To Reproduce

 (new \Illuminate\Notifications\Messages\SlackMessage())
    ->attachment(fn($attachment) => $attachment->field('dd', 'Whoops')
);

[driesvints]

Hey @lupinitylabs. I feel like this isn't really dangerous as it's up to the developer to sanitise an input for the field method. Of course, you're welcome to attempt the PR to see what Taylor thinks. Thanks

[lupinitylabs]

How would you sanitize this? The only possibility I would see is to check using is_callable() during validation if the value you try to put there is actually callable, and then filter it out...

Besides this, I feel that it is a bit of a nuisance to have to try to come up with field titles that are not function names... 😕

[driesvints]

Isn't $attachment->field('dd', 'Whoops') userland code? Just don't pass dd as an argument there?

[lupinitylabs]

I can imagine several occasions where you send a slack message to a user where the user might be able to define the titles of the attachment fields. So, from my point of view, either this is flagged as something bad, and you should never use attachments even with escaped and sanitized user-provided data (then there should probably be a warning in the docs), or measures should be taken that these unexpected side-effects are not occurring? 🤔

[driesvints]

I personally don't see a situation where a user would ever fill this out themselves but if you do feel free to attempt a PR to improve this 👍